-
Notifications
You must be signed in to change notification settings - Fork 2
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
NW6 | Rabia Avci | Full-Stack-Project | Week-3 | Update videos endpoint #36
Conversation
✅ Deploy Preview for watch-next-cyf ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Some feedback on the implementation details, but all in all, it works 👍
db/initdb.sql
Outdated
INSERT INTO videos (title,src) VALUES ('Coding Adventure: Chess AI','https://www.youtube.com/watch?v=U4ogK0MIzqk'); | ||
INSERT INTO videos (title,src) VALUES ('Coding Adventure: Ant and Slime Simulations','https://www.youtube.com/watch?v=X-iSQQgOd1A'); | ||
INSERT INTO videos (title,src) VALUES ('Why the Tour de France is so brutal','https://www.youtube.com/watch?v=ZacOS8NBK6U'); | ||
INSERT INTO videos (title,src,rating) VALUES ('Never Gonna Give You Up','https://www.youtube.com/watch?v=dQw4w9WgXcQ', 5); |
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 quite an interesting choice to me. Without the users' input, you can't know what their rating is. Personally, I would always use null or -1 as a default value here.
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.
Thanks, Anna, default rate is set to NULL. I will update the front-end with 'not rated' when the value is null.
server/api.js
Outdated
const { rating } = req.body; | ||
|
||
try { | ||
const result = await db.query(`SELECT * FROM videos WHERE id=${videoId}`); |
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.
As mentioned last time, this way of composing SQL isn't very secure - talk to Zeliha about the way she does it which is correct
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.
done it!
server/api.js
Outdated
.status(200) | ||
.json({ success: true, message: "Video rating successfully" }); | ||
} catch (error) { | ||
res.status(500).json({ success: false, error: "Internal server error" }); |
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 good that you're using an appropriate error code, but I would like to see you being more specific here, yes an internal server error happened but why?
const result = await db.query(`SELECT * FROM videos WHERE id=${videoId}`); | ||
let video = result.rows[0]; | ||
if (!video) { | ||
return res.status(404).json({ error: "Video not found" }); |
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.
👍
f538bb4
to
961f4cd
Compare
Completed these tasks for this ticket: