Add unit tests to make sure firebase indices are not affected when accepting deletions

src/main/webapp/app/service/firebase/firebase-gene-review-service.spec.ts

@@ -10,8 +10,9 @@ import { SentryError } from 'app/config/sentry-error';
 import { getTumorNameUuid, ReviewLevel, TumorReviewLevel } from 'app/shared/util/firebase/firebase-review-utils';
 import { ReviewAction } from 'app/config/constants/firebase';
 import _ from 'lodash';
-import { ActionType } from 'app/pages/curation/collapsible/ReviewCollapsible';
 import { generateUuid } from 'app/shared/util/utils';
+import { FIREBASE_LIST_PATH_TYPE } from 'app/shared/util/firebase/firebase-path-utils';
+import { ActionType } from 'app/pages/curation/collapsible/ReviewCollapsible';
 
 describe('Firebase Gene Review Service', () => {
   const DEFAULT_USERNAME = 'Test User';
@@ -327,7 +328,96 @@ describe('Firebase Gene Review Service', () => {
         'Meta/BRAF/lastModifiedBy': mockAuthStore.fullName,
       });
     });
+
+    it('should delete from array last when accepting changes', async () => {
+      const hugoSymbol = 'BRAF';
+      const mutationName = 'V600E';
+      const mutation = new Mutation(mutationName);
+
+      const tumorReview = new Review('User', undefined, false, true);
+      const tumorReviewLevel = new TumorReviewLevel({
+        titleParts: ['Cancer Type: B-Lymphoblastic Leukemia/Lymphoma'],
+        historyLocation: 'BCR-ABL1 Fusion, B-Lymphoblastic Leukemia, Lymphoma',
+        valuePath: 'mutations/1/tumors/0/cancerTypes',
+        currentVal: 'B-Lymphoblastic Leukemia/Lymphoma',
+        reviewInfo: {
+          reviewPath: 'mutations/0/name_review',
+          review: tumorReview,
+          lastReviewedString: undefined,
+          uuid: 'tumor_uuid',
+          reviewAction: ReviewAction.DELETE,
+        },
+        historyData: {
+          newState: mutation,
+        },
+        historyInfo: {},
+      });
+
+      const tumorSummaryReview = new Review('User', '');
+      const tumorSummaryReviewLevel = new ReviewLevel({
+        titleParts: ['B-Lymphoblastic Leukemia/Lymphoma with t(9;22)(q34.1;q11.2);BCR-ABL1', 'Summary'],
+        historyLocation: 'BCR-ABL1 Fusion, B-Lymphoblastic Leukemia, Lymphoma with t(9;22)(q34.1;q11.2);BCR-ABL1, Summary',
+        valuePath: 'mutations/1/tumors/1/summary',
+        currentVal: 'Test',
+        reviewInfo: {
+          reviewPath: 'mutations/1/tumors/1/summary_review',
+          review: tumorSummaryReview,
+          lastReviewedString: '',
+          uuid: 'tumor_summary_uuid',
+          reviewAction: ReviewAction.UPDATE,
+        },
+        historyData: {
+          newState: mutation,
+        },
+        historyInfo: {},
+      });
+
+      const mutationReview = new Review('User', '', false, true);
+      const mutationReviewLevel = new ReviewLevel({
+        titleParts: ['T315I'],
+        historyLocation: 'T315I',
+        valuePath: 'mutations/22/name',
+        currentVal: 'Test',
+        reviewInfo: {
+          reviewPath: 'mutations/22/name_review',
+          review: mutationReview,
+          lastReviewedString: undefined,
+          uuid: 'tumor_summary_uuid',
+          reviewAction: ReviewAction.DELETE,
+        },
+        historyData: {
+          newState: mutation,
+        },
+        historyInfo: {},
+      });
+
+      await firebaseGeneReviewService.acceptChanges(
+        hugoSymbol,
+        [mutationReviewLevel, tumorReviewLevel, tumorSummaryReviewLevel],
+        false,
+        true,
+      );
+
+      // Multi-location updates should happen before deleting from array to ensure that indices are not stale
+      expect(mockFirebaseRepository.update.mock.invocationCallOrder[0]).toBeLessThan(
+        mockFirebaseRepository.deleteFromArray.mock.invocationCallOrder[0],
+      );
+    });
   });
+
+  describe('processDeletion', () => {
+    it('should delete items closest to the leaves of the tree first', async () => {
+      await firebaseGeneReviewService.processDeletion(2, {
+        [FIREBASE_LIST_PATH_TYPE.MUTATION_LIST]: { mutations: [0, 1] },
+        [FIREBASE_LIST_PATH_TYPE.TUMOR_LIST]: { 'mutations/3/tumors': [3] },
+        [FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST]: { 'mutations/1/tumors/0/TIs/4/treatment': [0] },
+      });
+      expect(mockFirebaseRepository.deleteFromArray).toHaveBeenNthCalledWith(1, 'mutations/1/tumors/0/TIs/4/treatment', [0]);
+      expect(mockFirebaseRepository.deleteFromArray).toHaveBeenNthCalledWith(2, 'mutations/3/tumors', [3]);
+      expect(mockFirebaseRepository.deleteFromArray).toHaveBeenNthCalledWith(3, 'mutations', [0, 1]);
+    });
+  });
+
   describe('rejectChanges', () => {
     it('should delete lastReviewed and set value back to lastReviewed when rejecting update', async () => {
       const hugoSymbol = 'BRAF';

src/main/webapp/app/service/firebase/firebase-gene-review-service.ts

@@ -28,6 +28,8 @@ import { FirebaseVusService } from './firebase-vus-service';
 import { SentryError } from 'app/config/sentry-error';
 import { ActionType } from 'app/pages/curation/collapsible/ReviewCollapsible';
 
+export type ItemsToDeleteMap = { [key in FIREBASE_LIST_PATH_TYPE]: { [path in string]: number[] } };
+
 export class FirebaseGeneReviewService {
   firebaseRepository: FirebaseRepository;
   authStore: AuthStore;
@@ -100,7 +102,7 @@ export class FirebaseGeneReviewService {
     const geneFirebasePath = getFirebaseGenePath(isGermline, hugoSymbol);
     const vusFirebasePath = getFirebaseVusPath(isGermline, hugoSymbol);
 
-    const itemsToDelete: { [key in FIREBASE_LIST_PATH_TYPE]: { [path in string]: number[] } } = {
+    const itemsToDelete: ItemsToDeleteMap = {
       [FIREBASE_LIST_PATH_TYPE.MUTATION_LIST]: {},
       [FIREBASE_LIST_PATH_TYPE.TUMOR_LIST]: {},
       [FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST]: {},
@@ -158,28 +160,7 @@ export class FirebaseGeneReviewService {
       });
     }
 
-    // We are deleting last because the indices will change after deleting from array.
-    let hasDeletion = false;
-    try {
-      // Todo: We should use multi-location updates for deletions once all our arrays use firebase auto-generated keys
-      // instead of using sequential number indices.
-      for (const pathType of [
-        FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST,
-        FIREBASE_LIST_PATH_TYPE.TUMOR_LIST,
-        FIREBASE_LIST_PATH_TYPE.MUTATION_LIST,
-      ]) {
-        for (const [firebasePath, deleteIndices] of Object.entries(itemsToDelete[pathType])) {
-          hasDeletion = true;
-          await this.firebaseRepository.deleteFromArray(firebasePath, deleteIndices);
-        }
-      }
-      // If user accepts a deletion individually, we need to refresh the ReviewPage with the latest data to make sure the indices are up to date.
-      if (reviewLevels.length === 1 && hasDeletion) {
-        return { shouldRefresh: true };
-      }
-    } catch (error) {
-      throw new SentryError('Failed to accept deletions in review mode', { hugoSymbol, reviewLevels, isGermline, itemsToDelete });
-    }
+    return this.processDeletion(reviewLevels.length, itemsToDelete);
   };
 
   rejectChanges = async (hugoSymbol: string, reviewLevels: ReviewLevel[], isGermline: boolean) => {
@@ -285,4 +266,29 @@ export class FirebaseGeneReviewService {
       return acc;
     }, {});
   };
+
+  processDeletion = async (reviewLevelLength: number, itemsToDelete: ItemsToDeleteMap) => {
+    // We are deleting last because the indices will change after deleting from array.
+    let hasDeletion = false;
+    try {
+      // Todo: We should use multi-location updates for deletions once all our arrays use firebase auto-generated keys
+      // instead of using sequential number indices.
+      for (const pathType of [
+        FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST,
+        FIREBASE_LIST_PATH_TYPE.TUMOR_LIST,
+        FIREBASE_LIST_PATH_TYPE.MUTATION_LIST,
+      ]) {
+        for (const [firebasePath, deleteIndices] of Object.entries(itemsToDelete[pathType])) {
+          hasDeletion = true;
+          await this.firebaseRepository.deleteFromArray(firebasePath, deleteIndices);
+        }
+      }
+      // If user accepts a deletion individually, we need to refresh the ReviewPage with the latest data to make sure the indices are up to date.
+      if (reviewLevelLength === 1 && hasDeletion) {
+        return { shouldRefresh: true };
+      }
+    } catch (error) {
+      throw new SentryError('Failed to accept deletions in review mode', { itemsToDelete });
+    }
+  };
 }