BAI-1464 fix hard -> soft delete and optimise patch script

backend/src/migrations/008_delete_reviews_with_removed_access_requests.ts

@@ -1,18 +1,10 @@
 import AccessRequestModel from '../models/AccessRequest.js'
-import ReviewModel from '../models/Review.js'
 import { removeAccessRequestReviews } from '../services/review.js'
 
 export async function up() {
-  const accessRequests = await AccessRequestModel.find({ deleted: false })
-  const reviews = await ReviewModel.find({})
-  for (const review of reviews) {
-    const reviewAccessRequestId = review.get('accessRequestId')
-    if (
-      reviewAccessRequestId &&
-      !accessRequests.some((accessRequest) => accessRequest.get('id') == reviewAccessRequestId)
-    ) {
-      await removeAccessRequestReviews(reviewAccessRequestId)
-    }
+  const deletedAccessRequests = await (AccessRequestModel as any).findDeleted()
+  for (const accessRequest of deletedAccessRequests) {
+    await removeAccessRequestReviews(accessRequest.get('id'))
   }
 }
 

backend/src/services/review.ts

@@ -1,5 +1,3 @@
-import { DeleteResult } from 'mongodb'
-
 import authentication from '../connectors/authentication/index.js'
 import { ModelAction } from '../connectors/authorisation/actions.js'
 import authorisation from '../connectors/authorisation/index.js'
@@ -91,13 +89,21 @@ export async function createAccessRequestReviews(model: ModelDoc, accessRequest:
   await Promise.all(createReviews)
 }
 
-export async function removeAccessRequestReviews(accessRequestId: string): Promise<DeleteResult> {
-  const deletions = await Review.deleteMany({ accessRequestId: accessRequestId })
-
-  if (deletions.acknowledged === false) {
-    throw InternalError('The requested access request reviews could not be deleted.', {
-      accessRequestId,
-    })
+export async function removeAccessRequestReviews(accessRequestId: string) {
+  // finding and then calling potentially multiple deletes is inefficient but the mongoose-softdelete
+  // plugin doesn't cover bulkDelete
+  const accessRequestReviews = await findReviewsForAccessRequests([accessRequestId])
+
+  const deletions: ReviewDoc[] = []
+  for (const accessRequestReview of accessRequestReviews) {
+    try {
+      deletions.push(await accessRequestReview.delete())
+    } catch (error) {
+      throw InternalError('The requested access request review could not be deleted.', {
+        accessRequestId,
+        error,
+      })
+    }
   }
 
   return deletions
@@ -149,7 +155,7 @@ export async function findReviewForResponse(
 
 //TODO This won't work for response refactor
 export async function findReviewsForAccessRequests(accessRequestIds: string[]) {
-  const reviews = await Review.find({
+  const reviews: ReviewDoc[] = await Review.find({
     accessRequestId: accessRequestIds,
   })
   return reviews.filter((review) => requiredRoles.accessRequest.includes(review.role))

backend/test/services/__snapshots__/review.spec.ts.snap

@@ -67,3 +67,13 @@ exports[`services > review > findReviewsForAccessRequests > success 1`] = `
   },
 ]
 `;
+
+exports[`services > review > removeAccessRequestReviews > successful 1`] = `
+[
+  {
+    "accessRequestId": [
+      "Hello",
+    ],
+  },
+]
+`;

backend/test/services/review.spec.ts

@@ -31,7 +31,6 @@ const reviewModelMock = vi.hoisted(() => {
   obj.updateOne = vi.fn(() => obj)
   obj.save = vi.fn(() => obj)
   obj.delete = vi.fn(() => obj)
-  obj.deleteMany = vi.fn(() => obj)
   obj.limit = vi.fn(() => obj)
   obj.unwind = vi.fn(() => obj)
   obj.at = vi.fn(() => obj)
@@ -127,20 +126,19 @@ describe('services > review', () => {
   })
 
   test('removeAccessRequestReviews > successful', async () => {
-    const deleteResultsMock: any = { acknowledged: true }
-    reviewModelMock.deleteMany.mockResolvedValue(deleteResultsMock)
-
     await removeAccessRequestReviews(reviewModelMock.accessRequestId)
 
-    expect(reviewModelMock.deleteMany).toBeCalled()
+    expect(reviewModelMock.find.mock.calls.at(0)).toMatchSnapshot()
+    expect(reviewModelMock.delete).toBeCalled()
   })
 
   test('removeAccessRequestReviews > could not delete failure', async () => {
-    const deleteResultsMock: any = { acknowledged: false }
-    reviewModelMock.deleteMany.mockResolvedValue(deleteResultsMock)
+    reviewModelMock.delete.mockImplementationOnce(() => {
+      throw Error('Error deleting object')
+    })
 
     expect(() => removeAccessRequestReviews('')).rejects.toThrowError(
-      /^The requested access request reviews could not be deleted./,
+      /^The requested access request review could not be deleted./,
     )
   })
 })