search

ShelterTechSF / askdarcel-web

The "Ask Darcel" web app.
GNU General Public License v3.0
28 stars 22 forks source link

689 authentication #1320

Closed schroerbrian closed 8 months ago

schroerbrian commented 9 months ago

Adds auth functionality to front-end. UI styling is not complete but that will come in a future PR. Since the auth pages are not known to users yet, I believe that this is okay to merge into Master. Check https://qaone.sfserviceguide.org/log-in and https://qaone.sfserviceguide.org/sign-up to test it out (though it will fully work until this PR is merged in: https://github.com/ShelterTechSF/askdarcel-api/pull/732)

richardxia commented 9 months ago

Also, it was neat checking out the version of this that you deployed to qaone! However, I noticed that email for passwordless signup went to my spam folder:

Screenshot 2024-02-15 at 12 11 19 AM

I think this means that the email wasn't sent from a domain with email authentication stuff like SPF and DKIM. Are the emails like this only because they're on some development-mode account, or is there extra work we'll need to do to ensure that the emails being sent out will be properly authenticated?

schroerbrian commented 8 months ago

Sorry for being late to leave another round of review, but thanks for addressing my last round of feedback! I managed to sit down and look through this some more. I'm afraid that we may need a third round after this, since I still haven't gotten a chance to take a look at every file yet.

I think there's still something that's a bit off about how state is being managed and synchronized between the Auth0 client, local storage, the AppProvider component and the AppContext, but I'm still sort of wrapping my head around it so I don't have any concrete suggestions other than my various inline comments. I've been trying to focus my review on this layer, since I assume most of the actual UI components for signing up, logging in, and logging out are relatively straightforward.

Overall, though, this looks great! If it would help with speeding up the review and revision feedback loops, I'm totally down to spend some time pair reviewing this during our next 1-1. If you feel like this is starting to take too long, then I also think this is probably in decent enough shape to merge, since we can still iterate on the auth stuff even after this merges.

Thanks so much for the comprehensive review! Your suggestions are very helpful and much appreciated. I've made most of your suggested changes. As for the more complex changes like combining AppProvder/UseContext files and removing the isAuthenticated bool, perhaps we can discuss on Monday and/or pair. I may spend a little more time looking into them tomorrow and am also still trying to figure out why removing the useMemo hook when creating the context object breaks the downward propagation of the authState.

schroerbrian commented 8 months ago

Sorry for being late to leave another round of review, but thanks for addressing my last round of feedback! I managed to sit down and look through this some more. I'm afraid that we may need a third round after this, since I still haven't gotten a chance to take a look at every file yet. I think there's still something that's a bit off about how state is being managed and synchronized between the Auth0 client, local storage, the AppProvider component and the AppContext, but I'm still sort of wrapping my head around it so I don't have any concrete suggestions other than my various inline comments. I've been trying to focus my review on this layer, since I assume most of the actual UI components for signing up, logging in, and logging out are relatively straightforward. Overall, though, this looks great! If it would help with speeding up the review and revision feedback loops, I'm totally down to spend some time pair reviewing this during our next 1-1. If you feel like this is starting to take too long, then I also think this is probably in decent enough shape to merge, since we can still iterate on the auth stuff even after this merges.

Thanks so much for the comprehensive review! Your suggestions are very helpful and much appreciated. I've made most of your suggested changes. As for the more complex changes like combining AppProvder/UseContext files and removing the isAuthenticated bool, perhaps we can discuss on Monday and/or pair. I may spend a little more time looking into them tomorrow and am also still trying to figure out why removing the useMemo hook when creating the context object breaks the downward propagation of the authState.

Alright! I think made almost all of your suggested changes and added some more cleanup of my own after fixing up some TypeScript definitions. The one thing I didn't change was useMemo. I couldn't easily find a way to get things to gel together with state. See my copious notes on that above. Thanks again for the great review!

schroerbrian commented 8 months ago

LGTM! Thanks for addressing all my comments! I think this is in pretty good shape to merge, and I think most of my remaining comments are pretty minor.

Sweet! I've made a few more changes in light of your comments and will merge it in now