Delete mutation/tumor/treatment as last step to avoid stale index

src/main/webapp/app/pages/curation/review/ReviewPage.tsx

@@ -47,14 +47,18 @@ const ReviewPage: React.FunctionComponent<IReviewPageProps> = (props: IReviewPag
   const [editorsToAcceptChangesFrom, setEditorsToAcceptChangesFrom] = useState<string[]>([]);
   const [isAcceptingAll, setIsAcceptingAll] = useState(false);
 
-  const fetchFirebaseData = () => {
+  const [isAccepting, setIsAccepting] = useState(false);
+
+  const fetchFirebaseData = async () => {
     if (!props.firebaseDb) {
       return;
     }
     // Fetch the data when the user enters review mode. We don't use a listener
     // because there shouldn't be another user editing the gene when it is being reviewed.
-    get(ref(props.firebaseDb, firebaseGenePath)).then(snapshot => setGeneData(snapshot.val()));
-    get(ref(props.firebaseDb, firebaseMetaReviewPath)).then(snapshot => setMetaReview(snapshot.val()));
+    const geneDataSnapshot = await get(ref(props.firebaseDb, firebaseGenePath));
+    setGeneData(geneDataSnapshot.val());
+    const metaReviewSnapshot = await get(ref(props.firebaseDb, firebaseMetaReviewPath));
+    setMetaReview(metaReviewSnapshot.val());
   };
 
   useEffect(() => {
@@ -105,7 +109,7 @@ const ReviewPage: React.FunctionComponent<IReviewPageProps> = (props: IReviewPag
     try {
       setIsAcceptingAll(true);
       await props.acceptReviewChangeHandler?.(hugoSymbol ?? '', reviewLevels, isGermline, true);
-      fetchFirebaseData();
+      await fetchFirebaseData();
     } catch (error) {
       notifyError(error);
     } finally {
@@ -190,10 +194,20 @@ const ReviewPage: React.FunctionComponent<IReviewPageProps> = (props: IReviewPag
               hugoSymbol={hugoSymbol ?? ''}
               isGermline={isGermline}
               baseReviewLevel={rootReview}
-              handleAccept={props.acceptReviewChangeHandler}
+              handleAccept={async (hugoArg, reviewLevelsArg, isGermlineArg, isAcceptAllArg) => {
+                setIsAccepting(true);
+                try {
+                  const returnVal = await props.acceptReviewChangeHandler?.(hugoArg, reviewLevelsArg, isGermlineArg, isAcceptAllArg);
+                  if (returnVal?.shouldRefresh) {
+                    await fetchFirebaseData();
+                  }
+                } finally {
+                  setIsAccepting(false);
+                }
+              }}
               handleReject={props.rejectReviewChangeHandler}
               handleCreateAction={props.createActionHandler}
-              disableActions={isAcceptingAll}
+              disableActions={isAcceptingAll || isAccepting}
               isRoot={true}
               firebase={{
                 path: getGenePathFromValuePath(hugoSymbol ?? '', rootReview.valuePath, isGermline),

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

@@ -6,7 +6,12 @@ import { FirebaseRepository } from 'app/stores/firebase/firebase-repository';
 import _ from 'lodash';
 import { Review } from '../../shared/model/firebase/firebase.model';
 import { buildHistoryFromReviews } from '../../shared/util/firebase/firebase-history-utils';
-import { extractArrayPath, parseFirebaseGenePath } from '../../shared/util/firebase/firebase-path-utils';
+import {
+  extractArrayPath,
+  FIREBASE_LIST_PATH_TYPE,
+  getFirebasePathType,
+  parseFirebaseGenePath,
+} from '../../shared/util/firebase/firebase-path-utils';
 import {
   ReviewLevel,
   TumorReviewLevel,
@@ -95,6 +100,12 @@ 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[] } } = {
+      [FIREBASE_LIST_PATH_TYPE.MUTATION_LIST]: {},
+      [FIREBASE_LIST_PATH_TYPE.TUMOR_LIST]: {},
+      [FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST]: {},
+    };
+
     let updateObject = {};
 
     const reviewHistory = buildHistoryFromReviews(this.authStore.fullName, reviewLevels);
@@ -115,17 +126,15 @@ export class FirebaseGeneReviewService {
       } else if (isDeleteReview(reviewLevel)) {
         const { firebaseArrayPath, deleteIndex } = extractArrayPath(reviewLevel.valuePath);
         const firebasePath = geneFirebasePath + '/' + firebaseArrayPath;
+        const firebasePathType = getFirebasePathType(firebaseArrayPath + '/' + deleteIndex);
+        if (firebasePathType !== undefined) {
+          const innerMap = itemsToDelete[firebasePathType];
+          innerMap[firebasePath] ? innerMap[firebasePath].push(deleteIndex) : (innerMap[firebasePath] = [deleteIndex]);
+        }
         if (review.demotedToVus) {
           const variants = parseAlterationName(reviewLevel.currentVal)[0].alteration.split(', ');
           updateObject = { ...updateObject, ...this.firebaseVusService.getVusUpdateObject(vusFirebasePath, variants) };
         }
-        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.
-          await this.firebaseRepository.deleteFromArray(firebasePath, [deleteIndex]);
-        } catch (error) {
-          throw new SentryError('Failed to accept deletion in review mode', { hugoSymbol, reviewLevel, isGermline });
-        }
       } else if (isCreateReview(reviewLevel) && isAcceptAll) {
         const createUpdateObject = await this.getCreateUpdateObject(hugoSymbol, reviewLevel, isGermline, ActionType.ACCEPT);
         updateObject = { ...updateObject, ...createUpdateObject };
@@ -140,7 +149,36 @@ export class FirebaseGeneReviewService {
     try {
       await this.firebaseRepository.update('/', updateObject);
     } catch (error) {
-      throw new SentryError('Failed to accept changes in review mode', { hugoSymbol, reviewLevels, isGermline, isAcceptAll, updateObject });
+      throw new SentryError('Failed to accept changes in review mode', {
+        hugoSymbol,
+        reviewLevels,
+        isGermline,
+        isAcceptAll,
+        updateObject,
+      });
+    }
+
+    // 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 });
     }
   };
 

src/main/webapp/app/shared/util/firebase/firebase-path-utils.spec.ts

@@ -1,5 +1,11 @@
 import { FB_COLLECTION } from 'app/config/constants/firebase';
-import { buildFirebaseGenePath, extractArrayPath, parseFirebaseGenePath } from './firebase-path-utils';
+import {
+  buildFirebaseGenePath,
+  extractArrayPath,
+  FIREBASE_LIST_PATH_TYPE,
+  getFirebasePathType,
+  parseFirebaseGenePath,
+} from './firebase-path-utils';
 
 describe('FirebasePathUtils', () => {
   describe('parseFirebaseGenePath', () => {
@@ -41,4 +47,15 @@ describe('FirebasePathUtils', () => {
       expect(extractArrayPath(path)).toEqual({ firebaseArrayPath, deleteIndex });
     });
   });
+
+  describe('getFirebasePathType', () => {
+    test.each([
+      { path: 'mutations/0', pathType: FIREBASE_LIST_PATH_TYPE.MUTATION_LIST },
+      { path: 'mutations/0/tumors/0', pathType: FIREBASE_LIST_PATH_TYPE.TUMOR_LIST },
+      { path: 'mutations/0/tumors/23/TIs/0/treatments/4', pathType: FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST },
+      { path: 'mutations/0/mutation_effect', pathType: undefined },
+    ])('should return correct path type', ({ path, pathType }) => {
+      expect(getFirebasePathType(path)).toEqual(pathType);
+    });
+  });
 });

src/main/webapp/app/shared/util/firebase/firebase-path-utils.ts

@@ -42,3 +42,20 @@ export const extractArrayPath = (valuePath: string) => {
   const firebaseArrayPath = pathParts.join('/');
   return { firebaseArrayPath, deleteIndex };
 };
+
+export enum FIREBASE_LIST_PATH_TYPE {
+  MUTATION_LIST,
+  TUMOR_LIST,
+  TREATMENT_LIST,
+}
+export const getFirebasePathType = (path: string) => {
+  if (/mutations\/\d+$/i.test(path)) {
+    return FIREBASE_LIST_PATH_TYPE.MUTATION_LIST;
+  }
+  if (/tumors\/\d+$/i.test(path)) {
+    return FIREBASE_LIST_PATH_TYPE.TUMOR_LIST;
+  }
+  if (/TIs\/\d+\/treatments\/\d+$/i.test(path)) {
+    return FIREBASE_LIST_PATH_TYPE.TREATMENT_LIST;
+  }
+};