Skip to content

Code Review!#15

Open
matiasgarcia91 wants to merge 35 commits into
reviewfrom
development
Open

Code Review!#15
matiasgarcia91 wants to merge 35 commits into
reviewfrom
development

Conversation

@matiasgarcia91

Copy link
Copy Markdown
Collaborator

No description provided.

mir4cles and others added 30 commits June 18, 2020 13:48
inlcuded "user" in GET/events route
database with more development data

@matiasgarcia91 matiasgarcia91 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Backend looks tidy Juri, not much to say, good job!

Comment thread index.js

app.use("/events", eventRouter);

app.use("/teams", teamRouter);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a small recommendation. If you're going to attach a router directly to / route, then put it as the last one. The reason for this is that if by any chance you have a route like /:id in the auth router, it will catch all the requests sent to other routers because its basically /anything

Comment thread models/comment.js
// });
// comment.hasMany(models.event, {
// foreignKey: "eventId",
// });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

whats up with this? didn't work?

Comment thread routers/events.js
const events = await Event.findAndCountAll({
limit,
offset,
include: ["attending", "user"],

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this works? i normally do it like:

include: [{ model: User, as "attending" }]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

it works as it is including all users attending (through rsvp model) and the user who created the event.
i had to try a lot of thing but this worked.

Comment thread routers/events.js
if (!rsvp[1]) {
return res
.status(400)
.send({ message: "User is already attending this event" });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nice backend safeguard

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants