-
Notifications
You must be signed in to change notification settings - Fork 11
Review books #25
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
base: master
Are you sure you want to change the base?
Review books #25
Conversation
jalada
left a comment
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 is looking good! A few bits and pieces here, see what you think.
| 'use strict'; | ||
|
|
||
| module.exports = { | ||
| up: (queryInterface, Sequelize) => { |
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.
Oh hello 4 space tabs!
We don't have a documented convention, but the rest of the code is 2 spaces for tabs.
| Text: { | ||
| defaultValue: false, | ||
| allowNull: false, | ||
| type: Sequelize.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.
What does this translate down to in PostgreSQL? Is it a VARCHAR? Because if so, this will have a 255 character limit.
| const Review = sequelize.define('Review', { | ||
| BookId: DataTypes.INTEGER, | ||
| UserId: DataTypes.INTEGER, | ||
| Rating: DataTypes.INTEGER, |
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.
We should validate that this is between the 2 numbers we accept. (1 to 5?)
|
|
||
| const router = Router(); | ||
|
|
||
| const Op = Sequelize.Op; |
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.
You could also change the import line above to something like import { Op }, Sequelize from 'sequelize;`. I think...
| book = await Book.findByPk(req.params.id); | ||
| reviews = await Review.findAll({where: {BookId: {[Op.eq]: book.id}}}); | ||
| if(reviews.length > 0) { | ||
| rating = reviews.reduce((a, review) => a + review.Rating,0) / reviews.length; |
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 code feels like something best placed in the Book or Review model instead, what do you think?
| if(reviews.length > 0) { | ||
| rating = reviews.reduce((a, review) => a + review.Rating,0) / reviews.length; | ||
| } | ||
| // res.json(book); |
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.
It's OK to delete this :)
| })); | ||
|
|
||
| // Get a single book by ID. | ||
| // TODO: Should render HTML, not return JSON. |
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 TODO can be removed; it's rendering HTML now!
| const review = await req.user.createReview({BookId, Text, Rating}); | ||
|
|
||
|
|
||
| // const loan = await req.user.createLoan({BookId, dueDate}); |
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.
What's this?
No description provided.