[SSF-121] Remove User ID from Routes for Current User Pages#96
Conversation
amywng
force-pushed
the
acw/SSF-121-remove-ids-from-routes
branch
from
32ab190 to
a692028
Compare
maxn990
left a comment
maxn990
left a comment
There was a problem hiding this comment.
Nice Amy! A few suggestions but otherwise looks great
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| } catch (error) { | ||
| console.log(error); | ||
| } |
There was a problem hiding this comment.
besides cleaning up the console logs, should there be any error handling in the frontend here? like should the user see anything
There was a problem hiding this comment.
i'm not going to remove it here because I can't leave the catch branch empty, but I commented about it in justin's alert pr
| }); | ||
|
|
||
| if (!pantry) { | ||
| throw new NotFoundException(`Pantry for User ${userId} not found`); |
There was a problem hiding this comment.
I think we should add a test for this case, I may have missed it but I don't see one
There was a problem hiding this comment.
we haven't refactored the pantries service tests yet but i added one commented out for now
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| } catch (error) { | ||
| console.log(error); | ||
| } |
Juwang110
left a comment
Juwang110
left a comment
There was a problem hiding this comment.
Everything looks really good! Just a couple small cleanup things. As a small note, there are usages of request-form/pantryId in the delivery confirmation modal and confirmDelivery apiClient method. I think that modal and stuff needs to be refactored anyway though.
One more thing though, is there a reason why we are not updating homepage.tsx with the new routes? I think it would make sense to replace Pantry Dashboard (ID: 1) and Request Form (Pantry ID: 1). Besides that looks good!
amywng
force-pushed
the
acw/SSF-121-remove-ids-from-routes
branch
from
31d48e3 to
4593542
Compare
* login page * signup page * forgot password flow * refactoring to use my login page instead of authenticator from amplify * refactoring to combine verification and new password modals * minor refactoring * prettier * minor refactoring * switching button color to ssf blue * minor refactoring * adding password requirement info and eye crossed out icon * changing password requirement text to be accurate * making all routes protected, refactoring password requirement visual * adding alert for password being 8 characters at least
amywng
force-pushed
the
acw/SSF-121-remove-ids-from-routes
branch
from
4593542 to
f892adc
Compare
5 participants
ℹ️ Issue
Closes SSF-121
📝 Description
✔️ Verification
Verified user-specific routes don't need ID params
🏕️ (Optional) Future Work / Notes
Did you notice anything ugly during the course of this ticket? Any bugs, design challenges, or unexpected behavior? Write it down so we can clean it up in a future ticket!