completely resstructed responses by having them in their own collection #1280
Conversation
| if (action === ResponseAction.Update && toEntity('user', user.dn) !== response.user) { | ||
| return { id: user.dn, success: false, info: 'Only the author can update a comment' } | ||
| } | ||
| return { |
There was a problem hiding this comment.
We should probably make sure that if we're creating a response, the user has access to the 'access request' they're creating the response against.
| const releases: any = await ReleaseModel.find({}) | ||
| for (const release of releases) { | ||
| release.commentIds = [] | ||
| if (release.get('comments') !== undefined) { |
There was a problem hiding this comment.
Question: What does .get do on a model? Why not release.comments here?
There was a problem hiding this comment.
Oh, perhaps because it's not actually on the Mongoose schema anymore.
| if (release.get('comments') !== undefined) { | ||
| for (const releaseComment of release.get('comments')) { | ||
| const newComment = new ResponseModel({ | ||
| user: toEntity('user', releaseComment.user), |
There was a problem hiding this comment.
The age old argument of "Should this be an entity or not". In this case, it's always going to be a user, so maybe it should just use releaseComment.user, instead of an entity.
| user: toEntity('user', releaseComment.user), | ||
| kind: ResponseKind.Comment, | ||
| comment: releaseComment.message, | ||
| createdAt: releaseComment.createdAt, |
There was a problem hiding this comment.
You should also likely add updatedAt, which should be set to the createdAt value.
| }) | ||
| await newComment.save() | ||
| release.commentIds.push(newComment._id) | ||
| release.set('comments', undefined, { strict: false }) |
There was a problem hiding this comment.
This is a fun way of doing this. I always set it and then have to run .markModified. .set() appears to do both!
| if (accessRequest.get('comments') !== undefined) { | ||
| for (const accessCommment of accessRequest.get('comments')) { | ||
| const newComment = new ResponseModel({ | ||
| user: toEntity('user', accessCommment.user), |
| for (const accessCommment of accessRequest.get('comments')) { | ||
| const newComment = new ResponseModel({ | ||
| user: toEntity('user', accessCommment.user), | ||
| kind: ResponseKind.Comment, |
There was a problem hiding this comment.
It might be useful to have whether it's a comment from an 'access request' or a 'release' on this model. At the moment it's tricky to go from a comment to the object it's placed on, which might be a useful operation. I'm less sure on whether this is a good or bad idea, so will raise it with the rest of the team.
There was a problem hiding this comment.
I'm conflicted - I was trying to keep it simple by not having this two way relationship between the two models, and just having releases/access requests store a list of commentIds.
Can you think of a useful use case of where we'd want to go to the release or access request from the comment itself? I think from a UI perspective we don't have comments on their own page anywhere, they are only displayed on the release or access request page.
We also might want to add comments to model cards or other places in the future which would increase complexity if we were to go with your approach, whereas it would work "out of the box" in its current format.
I'm happy to be persuaded to change if the rest of the team think it's useful, though. :)
| }) | ||
| await newComment.save() | ||
| accessRequest.commentIds.push(newComment._id) | ||
| accessRequest.set('comments', undefined, { strict: false }) |
There was a problem hiding this comment.
This is a dangerous operation. Could we comment out this line initially, then add it in as a later migration once we're confident that the prior migration worked as expected?
There was a problem hiding this comment.
I've done the same for releases and also for reviews where I previously did delete review.comments
backend/src/models/Release.ts
Outdated
| createdAt: string | ||
| updatedAt: string | ||
| } | ||
| // export interface ReviewComment { |
There was a problem hiding this comment.
Remove this commented out code?
backend/src/models/Response.ts
Outdated
| const ResponseSchema = new Schema<ResponseInterface>( | ||
| { | ||
| user: { type: String, required: true }, | ||
| kind: { type: String, enum: Object.values(ResponseKind) }, |
There was a problem hiding this comment.
kind should likely be required.
backend/src/models/Review.ts
Outdated
| timestamps: true, | ||
| }, | ||
| ), | ||
| type: String, |
There was a problem hiding this comment.
Instead, type this as an object id that references another collection (as documented https://mongoosejs.com/docs/populate.html):
fans: [{ type: Schema.Types.ObjectId, ref: 'Person' }]
Then, you can run .populate on the query and not have to do any extra round trips to the database.
| @@ -53,10 +55,11 @@ export const getRelease = [ | |||
| const release = await getReleaseBySemver(req.user, modelId, semver) | |||
| await audit.onViewRelease(req, release) | |||
| const files = await getFilesByIds(req.user, modelId, release.fileIds) | |||
| const releaseWithFiles = { ...release.toObject(), files } | |||
| const comments = await findResponsesById(req.user, release.commentIds) | |||
There was a problem hiding this comment.
Nitpick: Function should be findResponsesByIds, as you're providing an array of ids.
| @@ -36,7 +38,7 @@ registerPath({ | |||
| }) | |||
|
|
|||
| interface GetAccessRequestResponse { | |||
| accessRequest: AccessRequestInterface | |||
| accessRequest: AccessRequestInterface & { comments: ResponseInterface[] } | |||
There was a problem hiding this comment.
In every route you've changed the return type, you'll also need to change the 'OpenAPI' docs above. accessRequestInterfaceSchema is no longer an accurate return type.
backend/src/services/response.ts
Outdated
| } | ||
|
|
||
| response.comment = comment | ||
| response.save |
There was a problem hiding this comment.
I don't think this is doing what you want it to do.
backend/src/services/review.ts
Outdated
| @@ -57,6 +57,8 @@ export async function findReviews( | |||
| ModelAction.View, | |||
| ) | |||
|
|
|||
| //const reviewsWithResponses = reviews.map((review)) | |||
There was a problem hiding this comment.
Remove unnecessary commented code.
backend/src/services/smtp/smtp.ts
Outdated
| @@ -95,13 +96,19 @@ export async function notifyReviewResponseForRelease(review: ReviewDoc, release: | |||
| log.info('Not sending email due to SMTP disabled') | |||
| return | |||
| } | |||
| const reviewResponse = review.responses[0] | |||
|
|
|||
| const reviewResponse = await findResponseById(review.responseIds[0]) | |||
There was a problem hiding this comment.
I don't think this is accurate anymore. With comments this might not represent the latest decision. I think this might need to be the last item in the array.
backend/src/services/smtp/smtp.ts
Outdated
| @@ -124,12 +131,17 @@ export async function notifyReviewResponseForAccess(review: ReviewDoc, accessReq | |||
| log.info('Not sending email due to SMTP disabled') | |||
| return | |||
| } | |||
| const reviewResponse = review.responses[0] | |||
| const reviewResponse = await findResponseById(review.responseIds[0]) | |||
There was a problem hiding this comment.
Again, I think the 0 here is inaccurate.
|
|
||
| export const responseInterfaceSchema = z.object({ | ||
| user: z.string().optional().openapi({ example: 'Joe Bloggs' }), | ||
| kind: z.nativeEnum(ResponseKind).openapi({ example: 'comment' }), |
There was a problem hiding this comment.
Use the enum values here, don't hard code them!
… migration script
| @@ -63,13 +65,15 @@ export default function ReviewDecisionDisplay({ response, modelId }: ReviewDecis | |||
| </Typography> | |||
| )} | |||
| </Stack> | |||
| <Typography variant='caption'>as {getRoleDisplay(response.role, modelRoles)}</Typography> | |||
| <Typography variant='caption'> | |||
| as {getRoleDisplay(response.role || 'Error fetching model role', modelRoles)} | |||
There was a problem hiding this comment.
This is an unusual way for us to handle errors. Why does the "get response" endpoint not error in this scenario? If it did then we could handle the error as we normally would.
There was a problem hiding this comment.
It's not a problem getting the response object but getting the role. In our typing role is optional because regular comments do not have roles, only review responses.
There was a problem hiding this comment.
In theory the role property will always be there so we could do getRoleDisplay(response.role as string, modelRoles) but I'm not keen on this approach?
There was a problem hiding this comment.
I see. I think the best approach is to do the following then:
{response.role && (
<Typography variant='caption'>as {getRoleDisplay(response.role, modelRoles)}</Typography>
)}
Comments / Review responses are now stored in a single collection, and are populated as and when they need to be by the API.
This will improve accessing them for the purpose of editing.
This in draft state until the migration script is finished.