Fix #96: Add ability to retrieve timing history #132
Conversation
- param in query instead of body in endpoint request
routes/routes.js
Outdated
| User.findOne({ | ||
| where: {id: req.query.userId} | ||
| }).then(user => { |
There was a problem hiding this comment.
You actually don't need to fetch the user in this case, you can just skip right to the second query.
routes/routes.js
Outdated
| return res.status(200).json({ | ||
| previousTimingMs: userAnswers.map(userAnswer => userAnswer.elapsed_time_ms) | ||
| }) |
There was a problem hiding this comment.
Since 200 is the default response code, you don't need to specify it unless the code would be confusing otherwise.
There was a problem hiding this comment.
Sometimes (when I skip typing the answer) it returns 304.
routes/routes.js
Outdated
| }) | ||
| }).catch(error => { | ||
| console.log(error); | ||
| return res.status(400).json(error.errors) // TODO: handle better error messages |
There was a problem hiding this comment.
Since we don't know what the cause of the error was, we should probably use code 500 as it could be a bug on the server.
public/scripts/history.js
Outdated
| let params = { | ||
| userId: 'guest' // TODO: get actual userId | ||
| } | ||
| url.search = new URLSearchParams(params).toString(); |
There was a problem hiding this comment.
The server will know the user id of the user making the API request so you don't have to include it explicitly here, but will have to so the query works on the server so the TODO can go there.
routes/routes.js
Outdated
| @@ -38,5 +38,31 @@ router.post('/user/answers/question/:questionNumber', (req, res) => { | |||
| }) | |||
| }) | |||
|
|
|||
| router.get('/user/answers/question/:questionNumber', (req, res) => { | |||
| let pageSize = 3; // TODO: define it in more stadarized way | |||
There was a problem hiding this comment.
This could be a uppercase constant in the file in case any other routes need it.
Not sure the best name to use, maybe ANSWER_HISTORY_LIMIT ?
…tcuts-practice into 96__retrieve-timing-question
|
@vegetabill My latest commit fixes all the comments you left 😊 |
public/scripts/history.js
Outdated
| let params = { | ||
| userId: 'guest' | ||
| } | ||
| url.search = new URLSearchParams(params).toString(); |
There was a problem hiding this comment.
I think you missed this suggested revision. Let's move this to the server, which is where the real logic will be.
There was a problem hiding this comment.
Yes! I forgot to add it because I didn't understand very well. After this comment, I got it and updated the PR
Fix #96
Description:
questionNoanduserId.history.js