Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michaeltran3599
Copy link
Contributor

add rsvp model and routes to create and get an authenticated user's rsvp

add rsvp model and routes to create and get an authenticated user's rsvp
Copy link
Member

@Jumer-Caragay Jumer-Caragay left a 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.

min: 0,
max: 4,
required: function () { return this.attending }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Blank Line

min: 0,
max: 2,
required: function () { return this.attending && this.bus === 1 }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove additional blank line.

required: function () { return this.attending }

},
travelType: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max: 2,
required: function () { return this.attending }
},
bus: {
Copy link
Member

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,
Copy link
Member

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?

Copy link
Contributor

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: {
Copy link
Member

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', {
Copy link
Member

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

}
})

router.get('/rsvp/', authMiddleware, async (req, res) => {
Copy link
Member

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
Copy link
Contributor

@cfspecht cfspecht left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants