From 95c06590fa308ef7a3a33d5504ac24497c67877c Mon Sep 17 00:00:00 2001 From: Calvin Lu <59149377+calvinlu3@users.noreply.github.com> Date: Fri, 4 Oct 2024 14:01:46 -0400 Subject: [PATCH] Add tests for stale indices check --- .../firebase-gene-review-service.spec.ts | 92 ++++++++++++++++++- .../firebase/firebase-gene-review-service.ts | 52 ++++++----- 2 files changed, 120 insertions(+), 24 deletions(-) diff --git a/src/main/webapp/app/service/firebase/firebase-gene-review-service.spec.ts b/src/main/webapp/app/service/firebase/firebase-gene-review-service.spec.ts index 0992bbba3..81876d0ea 100644 --- a/src/main/webapp/app/service/firebase/firebase-gene-review-service.spec.ts +++ b/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'; diff --git a/src/main/webapp/app/service/firebase/firebase-gene-review-service.ts b/src/main/webapp/app/service/firebase/firebase-gene-review-service.ts index 878f1e6ff..b63bf5dbd 100644 --- a/src/main/webapp/app/service/firebase/firebase-gene-review-service.ts +++ b/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 }); + } + }; }