Add post review response and move review count to be a response header #749
Conversation
backend/src/routes.ts
Outdated
| server.get('/api/v2/reviews', ...getReviews) | ||
| server.get('/api/v2/reviews/count', ...getReviewsCount) | ||
| server.get('/api/v2/reviews/:modelId?', ...getReviews) | ||
| server.post('/api/v2/reviews/:modelId/:semver/:role', ...postReviewResponse) |
There was a problem hiding this comment.
I think this should be .post('/api/v2/reviews') and include semver, role and modelId in the body. This tends to be equivalent to other places where we create things.
There was a problem hiding this comment.
See previous comment and I would also add that these pieces of data are used to identify the specific resource so, for me, should be part of the URI. The body should contain the data that the user is wanting to write, not an identifier of that data
backend/src/routes.ts
Outdated
| @@ -221,8 +221,8 @@ if (config.experimental.v2) { | |||
| server.get('/api/v2/schema/:schemaId', ...getSchemaV2) | |||
| server.post('/api/v2/schemas', ...postSchemaV2) | |||
|
|
|||
| server.get('/api/v2/reviews', ...getReviews) | |||
| server.get('/api/v2/reviews/count', ...getReviewsCount) | |||
| server.get('/api/v2/reviews/:modelId?', ...getReviews) | |||
There was a problem hiding this comment.
I think modelId should be a parameter, since it's optional and similar to the 'active' flag. Separately, this type of route is really hard to represent in OpenAPI...
There was a problem hiding this comment.
I'm not massively set on either of these endpoints but I think a good approach to URI vs query params is to use URI params for attributes that are consistent over time. The idea being that you want to keep URIs consistent. So in terms of similarity to the 'active flag', the key difference for me is that the 'active' attribute of a review is not consistent over time and is expected to change so a query param is more suitable whereas the modelId of a review should never change so I should not expect a review to suddenly move to a different URI.
| @@ -9,6 +9,8 @@ import { parse, strictCoerceBoolean } from '../../../utils/v2/validate.js' | |||
| export const getReviewSchema = z.object({ | |||
| } = parse(req, getReviewSchema) | ||
| const reviews = await findReviews(req.user, active, modelId) | ||
| res.setHeader('x-count', reviews.length) |
There was a problem hiding this comment.
Traditionally this would be X-Count I think.
There was a problem hiding this comment.
http headers are case-insensitive?
There was a problem hiding this comment.
They're not case-sensitive, but I can see us keeping it upper-case to be consistent in other areas of the code but overall I don't think it's a massive issue if it's kept as-is.
There was a problem hiding this comment.
I also noticed that when unit testing, the header name gets lowercased anyway regardless of what case is used here
| params: { modelId, semver, role }, | ||
| body, | ||
| } = parse(req, postReviewResponseSchema) | ||
| const review = await respondToReview(req.user, modelId, semver, role, body) |
There was a problem hiding this comment.
Nitpick, not necessary to change, but adding some spaces between parts of your code can make it easier! Especially with the weird expansion on lines 28 - 31.
| /** | ||
| * Return the models where one of the user's entities is in the model's collaberators | ||
| * and the role in the review is in the list of roles in that collaborator entry. | ||
| */ |
There was a problem hiding this comment.
Could you add that this requires the model attribute?
No description provided.