Wiredcraft / test-fullstack

6 stars 43 forks source link

Implementation of the fullstack coding challenge #23

Closed mil22n closed 5 years ago

mil22n commented 6 years ago

Instructions for setting up the environment and running the app are in README.doc.md file.

I created the server using the Loopback framework, but it is pretty bare boned and with no authentication. There is a dummy login page which just stores the user in the localStorage.

For the client, I've used React with Redux.

The tests are not complete as I am late to submit the project but actions, reducers and a couple key components were covered.

Looking forward to your review.

Jamesford commented 6 years ago

Hey @mil22n

Had some time this morning to review your test and I've got some questions and feedback for the frontend part of it. Another colleague will follow up on the backend related part.

Questions

Why did you choose redux-promise over redux-thunk?

In the tests for Talk and Submit components, you're also mounting the Provier and Router, was there an intentional decision to test it like this?

Bug

If I arrive on the page without ever visiting, the logout button is shown. In order to login I first have to logout, despite never logging in to begin with. It also looks like anonymous users should not be able to vote, however it seems as though it's possible. Probably related to this bug.

milan-fullstack-test-bug

Comments

Next steps

So as said above, this is for the frontend part, you'll get some feedback on the backend soon (friday latest). For now though, if you could answer the questions above and maybe take a look at the bug. Aside from all that, it looks good, quite happy with the frontend code you've submitted.

mil22n commented 6 years ago

Hey @Jamesford, thanks for your comments and a very detailed and professional review.

Answers

Why did you choose redux-promise over redux-thunk?

Also it seems to have forced you into mixing Promises and Callbacks

Would you still use redux-promise (or the redux-promise-middleware version) for your next project?

In the tests for Talk and Submit components, you're also mounting the Provider and Router, was there an intentional decision to test it like this?

Bug fix

Fixed the issue of user with the undefined username being added to the auth state when the checkSession action was called.