-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add rsvp service #85
Add rsvp service #85
Conversation
Should be all good now. |
Update: Resolved all comments. |
Note this still has conflicts with yarn.lock and src/app.ts, you'll probably need to rebase to the main branch to fix this. Looks like main conflicts are just the version router being added separate from this branch and the express.json() fix. yarn.lock is just dependencies conflict, nuke it and yarn install again to fix. |
Yup, working on this offline w/ Alex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, for the most part! Some more super small changes, then we should be good to merge this in by Wednesday :)
Also, @aletya did you get a very basic Postman test of this up and working?
yup I tested everything with postman |
Cool beans, thanks for confirming! Just LMK once you get the changes in, and we'll be good to merge :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for the most part, also pinging @Timothy-Gonzalez on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far so good! A couple recommendations to make the code a bit cleaner.
Also, if you want a general gist of what you haven't tested, jest prints out a coverage report when you run tests. If you look under src/services/rsvp, you'll see:
src/services/rsvp | 94.85 | 78.94 | 100 | 94.85 |
rsvp-router.ts | 94.85 | 78.94 | 100 | 94.85 | 12,45-46,139-140,150-151,171-172
It will show you which lines aren't covered. Some probably don't need to be, but we should check cases like permissions & handling of parameters. Great coverage so far!
Also, aydan's recommendations as well. Just try to test all the edge cases really, and we should be good to go. Also, great job with rsvp, it looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits, but good otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great progress! The tests are much nicer now and hopefully you've learned a few tricks (jest is a powerful tool but also a complicated one). Only things left are testing the result stored in the actual database (not blindly trusting success == worked basically) and pretty minor things, so great job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great job!
Resolves #21
Added the rsvp service in rsvp-router.ts
Tested all 3 endpoints with a test database collection, but I'll come back and check if it still works after we finish reformatting our database.
3 endpoints:
GET /rsvp/:USERID
Gets USERID's RSVP decision if the currently authenticated user has elevated permissions.
GET /rsvp/
Gets currently authenticated user's RSVP decision.
PUT /rsvp/
{"isAttending": true/false}
Updates currently authenticated user's RSVP decision.