-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(index.js, RSVPModel.js, rsvp.js): add rsvp model and routes #16
base: master
Are you sure you want to change the base?
Conversation
add rsvp model and routes to create and get an authenticated user's rsvp
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.
If you have any questions about the comments, feel free to ask!
In addition to the conditional required and field types of the model,
consider looking at the implementation of the frontend
https://github.com/slohacks/attendee-application/blob/master/src/containers/RSVP.jsx
I'll look more into this.
Another implementation is ignore conditional required fields
and just initiate them to null.
src/models/RSVPModel.js
Outdated
min: 0, | ||
max: 4, | ||
required: function () { return this.attending } | ||
|
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.
Remove Blank Line
src/models/RSVPModel.js
Outdated
min: 0, | ||
max: 2, | ||
required: function () { return this.attending && this.bus === 1 } | ||
|
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.
Remove additional blank line.
src/models/RSVPModel.js
Outdated
required: function () { return this.attending } | ||
|
||
}, | ||
travelType: { |
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.
This tag should be transportation check out https://github.com/slohacks/attendee-application/blob/master/src/containers/data/rsvpData.json
max: 2, | ||
required: function () { return this.attending } | ||
}, | ||
bus: { |
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.
should be "buses"
required: function () { return this.attending && this.travelType === 0 } | ||
}, | ||
socal: { | ||
type: Number, |
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.
Currently, the frontend sends this as a string. thoughts on number vs string?
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.
Tommy and I think it is easier to store this data as a string as well, I am updating the Application model to reflect this.
max: 2, | ||
required: function () { return this.attending && this.bus === 0 } | ||
}, | ||
norcal: { |
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.
Same comment as above.
@@ -0,0 +1,57 @@ | |||
const mongoose = require('mongoose') | |||
|
|||
const RSVP = mongoose.model('RSVP', { |
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.
Think about how we should expire out RSVP, ex if user is accepted but RSVPS after 7 days how do we prevent them from creating a document
src/routers/rsvp.js
Outdated
} | ||
}) | ||
|
||
router.get('/rsvp/', authMiddleware, async (req, res) => { |
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.
/rsvp/:id is more concise, Change it to that implementation. I remember talking about it doesnt make sense for us to pull a specific RSVP but just to be consistent with other routes/models, change the implementation to reflect that.
…s user admin rights Added email check in authMiddleware to give @slohacks.com users access to all rsvps
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.
Talk with Tommy about storing the data as integers vs. strings, and Jumer's changes. Looks good otherwise.
add rsvp model and routes to create and get an authenticated user's rsvp