added a reaction selector for comments #1340
Conversation
ARADDCC002
commented
Jun 20, 2024
•
ARADDCC002
commented
…o be a button so that you can also add a reaction by clicking directly on existing icons
|
Updated look: |
There was a problem hiding this comment.
Could we get some quick unit tests for these.
|
|
||
| const response = await updateResponseReaction(req.user, responseId, kind as ReactionKindKeys) | ||
|
|
||
| await audit.onUpdateResponse(req, responseId) |
There was a problem hiding this comment.
Could we add the kind to the audit?
There was a problem hiding this comment.
We don't actually need to audit reactions at all, so I'm going to remove
backend/src/services/response.ts
Outdated
| if (response.reactions === undefined) { | ||
| response.reactions = [] | ||
| } |
There was a problem hiding this comment.
Is it better to update it like this or define a migration?
There was a problem hiding this comment.
We should do a migration for sure
backend/src/services/response.ts
Outdated
| response.reactions.push(newReaction) | ||
| } else { | ||
| updatedReaction.users.includes(user.dn) | ||
| ? (updatedReaction.users = updatedReaction.users.filter((reactionUser) => reactionUser !== user.dn)) |
There was a problem hiding this comment.
Could these brackets be removed?
There was a problem hiding this comment.
They are needed in this case, however I'm not a fan of using a ternary like this. Imo they should only be used when conditionally returning a value. I've updated to:
if (!updatedReaction) {
const newReaction: ResponseReaction = {
kind,
users: [user.dn],
}
response.reactions.push(newReaction)
} else if (updatedReaction.users.includes(user.dn)) {
updatedReaction.users = updatedReaction.users.filter((reactionUser) => reactionUser !== user.dn)
} else {
updatedReaction.users.push(user.dn)
}
| if (res.ok) { | ||
| mutateResponses() | ||
| setAnchorEl(null) | ||
| } |
There was a problem hiding this comment.
Do you need a if (!res.ok) condition to catch errors with this backend call? It appears to just not update giving no feadback if it errors.
backend/src/services/response.ts
Outdated
| if (response.entity !== toEntity('user', user.dn)) { | ||
| throw Forbidden('Only the original author can update a comment or review response.', { | ||
| userDn: user.dn, | ||
| responseId, | ||
| }) | ||
| } |
There was a problem hiding this comment.
This suggests that you can only get individual responses on responses that you've written. Is this expected?
There was a problem hiding this comment.
Yeah it doesn't really make any sense, this function has been removed
backend/src/services/response.ts
Outdated
| export async function getResponseById(user: UserInterface, responseId: string) { | ||
| const response = await ResponseModel.findOne({ _id: responseId }) | ||
|
|
||
| if (!response) { | ||
| throw NotFound(`The requested response was not found.`, { responseId }) | ||
| } | ||
|
|
||
| if (response.entity !== toEntity('user', user.dn)) { | ||
| throw Forbidden('Only the original author can update a comment or review response.', { | ||
| userDn: user.dn, | ||
| responseId, | ||
| }) | ||
| } | ||
|
|
||
| return response | ||
| } |
There was a problem hiding this comment.
I can't see anywhere this function is called. Is it necessary?
GB27247
force-pushed
the
dev/comment-reactions
branch
from
02728e5
to
d24706c
Compare
|
|
||
| registerPath({ | ||
| method: 'patch', | ||
| path: '/api/v2/response/{responseId}', |
| export const patchResponseReactionSchema = z.object({ | ||
| params: z.object({ | ||
| responseId: z.string(), | ||
| kind: z.string(z.nativeEnum(ResponseKind)), |
There was a problem hiding this comment.
This should be kind: z.nativeEnum(ReactionKind)
| method: 'patch', | ||
| path: '/api/v2/response/{responseId}', | ||
| tags: ['response'], | ||
| description: `Update either a comment or a review response's reactions`, |
| params: { responseId, kind }, | ||
| } = parse(req, patchResponseReactionSchema) | ||
|
|
||
| const response = await updateResponseReaction(req.user, responseId, kind as ReactionKindKeys) |
There was a problem hiding this comment.
If the zod schema is corrected, this type assertion won't be needed
backend/src/services/response.ts
Outdated
| updatedReaction.users.push(user.dn) | ||
| } | ||
|
|
||
| response.save() |
