Skip to content

Commit

Permalink
Add tests for stale indices check (#455)
Browse files Browse the repository at this point in the history
  • Loading branch information
@calvinlu3
calvinlu3 authored Oct 4, 2024
1 parent 267c190 commit df84037
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 24 deletions.
92 changes: 91 additions & 1 deletion src/main/webapp/app/service/firebase/firebase-gene-review-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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';
Expand Down
52 changes: 29 additions & 23 deletions src/main/webapp/app/service/firebase/firebase-gene-review-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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]: {},
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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 });
}
};
}

0 comments on commit df84037

Please sign in to comment.