From 3e628742b79fd11c4bf1ffd65bd1a7dbb81870bc Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Wed, 26 Aug 2026 17:14:32 -0400 Subject: [PATCH 1/3] disable deleting metrics that are in use --- .../src/api/repositories/QueryRepository.ts | 13 ++++ .../backend/src/api/services/MetricService.ts | 45 ++++++++----- .../unit/repositories/QueryRepository.test.ts | 27 ++++++++ .../test/unit/services/MetricService.test.ts | 63 ++++++++++++++++++- .../analysis/analysis.data.service.spec.ts | 5 +- .../core/analysis/analysis.data.service.ts | 8 ++- .../core/analysis/store/analysis.effects.ts | 10 ++- .../store/experiments.effects.spec.ts | 35 +++++++++++ .../experiments/store/experiments.effects.ts | 12 +++- .../http-cancel.interceptor.ts | 18 +++++- .../components/metrics/metrics.component.html | 23 +++++-- .../components/metrics/metrics.component.scss | 5 ++ .../components/metrics/metrics.component.ts | 8 ++- .../components/metrics/metrics.model.ts | 1 + .../projects/upgrade/src/assets/i18n/en.json | 1 + packages/types/src/Experiment/interfaces.ts | 1 + 16 files changed, 242 insertions(+), 33 deletions(-) diff --git a/packages/backend/src/api/repositories/QueryRepository.ts b/packages/backend/src/api/repositories/QueryRepository.ts index d6fdaf0f97..a79a4f74c2 100644 --- a/packages/backend/src/api/repositories/QueryRepository.ts +++ b/packages/backend/src/api/repositories/QueryRepository.ts @@ -66,4 +66,17 @@ export class QueryRepository extends Repository { return queryResult.length > 0 ? true : false; } + + public async getMetricKeysWithQueries(): Promise { + const queryResult = await this.createQueryBuilder('query') + .innerJoin('query.metric', 'metric') + .select('DISTINCT metric.key', 'metricKey') + .getRawMany() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError('QueryRepository', 'getMetricKeysWithQueries', {}, errorMsg); + throw errorMsgString; + }); + + return queryResult.map((row: { metricKey: string }) => row.metricKey); + } } diff --git a/packages/backend/src/api/services/MetricService.ts b/packages/backend/src/api/services/MetricService.ts index 16c8330147..e987b1c2a3 100644 --- a/packages/backend/src/api/services/MetricService.ts +++ b/packages/backend/src/api/services/MetricService.ts @@ -1,6 +1,7 @@ import { Service } from 'typedi'; import { InjectRepository } from '../../typeorm-typedi-extensions'; import { MetricRepository } from '../repositories/MetricRepository'; +import { QueryRepository } from '../repositories/QueryRepository'; import { Metric } from '../models/Metric'; import { SERVER_ERROR, IMetricUnit, IMetricMetaData, IGroupMetric, ISingleMetric } from 'upgrade_types'; import { SettingService } from './SettingService'; @@ -11,13 +12,18 @@ export const METRICS_JOIN_TEXT = '@__@'; @Service() export class MetricService { - constructor(@InjectRepository() private metricRepository: MetricRepository, public settingService: SettingService) {} + constructor( + @InjectRepository() private metricRepository: MetricRepository, + @InjectRepository() private queryRepository: QueryRepository, + public settingService: SettingService + ) {} public async getAllMetrics(logger: UpgradeLogger): Promise { logger.info({ message: 'Get all metrics' }); // check permission for metrics const metricData = await this.metricRepository.find(); - return this.metricDocumentToJson(metricData); + const metricKeysWithQueries = await this.queryRepository.getMetricKeysWithQueries(); + return this.metricDocumentToJson(metricData, new Set(metricKeysWithQueries)); } public async getMetricsByContext(context: string, logger: UpgradeLogger): Promise { @@ -139,35 +145,44 @@ export class MetricService { return keyArrayAndMeta; } - private metricDocumentToJson(metrics: Metric[]): IMetricUnit[] { + private metricDocumentToJson(metrics: Metric[], metricKeysWithQueries?: Set): IMetricUnit[] { const metricUnitArray: IMetricUnit[] = []; metrics.forEach((metric) => { const keyArray = metric.key.split(METRICS_JOIN_TEXT); let metricPointer = metricUnitArray; - keyArray.forEach((key) => { - const keyExist = metricPointer.reduce((aggregator, unit) => { - const isKey = unit && unit.key === key ? true : false; - if (isKey) { - metricPointer = unit.children; - } - return aggregator || isKey; - }, false); + let topLevelMetric: IMetricUnit; - if (keyExist === false) { + keyArray.forEach((key, index) => { + let unit = metricPointer.find((candidate) => candidate?.key === key); + + if (!unit) { // create the key - const newMetric = { + unit = { key, children: [], metadata: { type: metric.type as any }, allowedData: metric.allowedData, context: metric.context, }; - metricPointer.push(newMetric); + if (index === 0 && metricKeysWithQueries) { + unit.hasQuery = false; + } + metricPointer.push(unit); + } - metricPointer = newMetric.children; + if (index === 0) { + topLevelMetric = unit; } + + metricPointer = unit.children; }); + + // Only the top-level (grouped or simple) metric object carries hasQuery, true if any + // of the keys nested under it are referenced by a query. + if (topLevelMetric && metricKeysWithQueries?.has(metric.key)) { + topLevelMetric.hasQuery = true; + } }); return metricUnitArray; } diff --git a/packages/backend/test/unit/repositories/QueryRepository.test.ts b/packages/backend/test/unit/repositories/QueryRepository.test.ts index 092d538322..e0aea58db8 100644 --- a/packages/backend/test/unit/repositories/QueryRepository.test.ts +++ b/packages/backend/test/unit/repositories/QueryRepository.test.ts @@ -179,4 +179,31 @@ describe('QueryRepository Testing', () => { expect(mock.where).toHaveBeenCalledTimes(1); expect(mock.getMany).toHaveBeenCalledTimes(1); }); + + it('should return the distinct metric keys referenced by queries', async () => { + mock.getRawMany.mockResolvedValue([{ metricKey: 'metric1' }, { metricKey: 'metric2' }]); + const res = await repo.getMetricKeysWithQueries(); + + expect(repo.createQueryBuilder).toHaveBeenCalledTimes(1); + + expect(mock.innerJoin).toHaveBeenCalledTimes(1); + expect(mock.select).toHaveBeenCalledTimes(1); + expect(mock.getRawMany).toHaveBeenCalledTimes(1); + + expect(res).toEqual(['metric1', 'metric2']); + }); + + it('should throw an error when getting metric keys with queries fails', async () => { + mock.getRawMany.mockRejectedValue(err); + + expect(async () => { + await repo.getMetricKeysWithQueries(); + }).rejects.toThrow(err); + + expect(repo.createQueryBuilder).toHaveBeenCalledTimes(1); + + expect(mock.innerJoin).toHaveBeenCalledTimes(1); + expect(mock.select).toHaveBeenCalledTimes(1); + expect(mock.getRawMany).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/backend/test/unit/services/MetricService.test.ts b/packages/backend/test/unit/services/MetricService.test.ts index d0c3540501..49df966091 100644 --- a/packages/backend/test/unit/services/MetricService.test.ts +++ b/packages/backend/test/unit/services/MetricService.test.ts @@ -1,4 +1,4 @@ -import { MetricService } from '../../../src/api/services/MetricService'; +import { MetricService, METRICS_JOIN_TEXT } from '../../../src/api/services/MetricService'; import { Repository } from 'typeorm'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; @@ -6,6 +6,7 @@ import { IGroupMetric, IMetricMetaData, ISingleMetric } from 'upgrade_types'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { SettingService } from '../../../src/api/services/SettingService'; import { MetricRepository } from '../../../src/api/repositories/MetricRepository'; +import { QueryRepository } from '../../../src/api/repositories/QueryRepository'; import { SettingRepository } from '../../../src/api/repositories/SettingRepository'; import { CacheService } from '../../../src/api/services/CacheService'; import { configureLogger } from '../../utils/logger'; @@ -13,6 +14,7 @@ import { configureLogger } from '../../utils/logger'; describe('Audit Service Testing', () => { let service: MetricService; let repo: Repository; + let queryRepositoryMock: { getMetricKeysWithQueries: jest.Mock }; let module: TestingModule; const settingRes = [{ id: 'id', toCheckAuth: false, toFilterMetric: true }]; @@ -64,6 +66,13 @@ describe('Audit Service Testing', () => { }, ]; + const metricResultWithHasQuery = [ + { + ...metricResult[0], + hasQuery: true, + }, + ]; + beforeAll(() => { configureLogger(); }); @@ -85,6 +94,12 @@ describe('Audit Service Testing', () => { save: jest.fn().mockResolvedValue(metric), }, }, + { + provide: getRepositoryToken(QueryRepository), + useValue: { + getMetricKeysWithQueries: jest.fn().mockResolvedValue(['totalProblemsCompleted']), + }, + }, { provide: getRepositoryToken(SettingRepository), useValue: { @@ -106,6 +121,7 @@ describe('Audit Service Testing', () => { service = module.get(MetricService); repo = module.get>(getRepositoryToken(MetricRepository)); + queryRepositoryMock = module.get(getRepositoryToken(QueryRepository)); }); it('should be defined', async () => { @@ -118,7 +134,50 @@ describe('Audit Service Testing', () => { it('should return all metrics', async () => { const res = await service.getAllMetrics(new UpgradeLogger()); - expect(res).toEqual(metricResult); + expect(res).toEqual(metricResultWithHasQuery); + }); + + it('should only set hasQuery on the top-level metric object for grouped metrics', async () => { + const groupKey = `masteryWorkspace${METRICS_JOIN_TEXT}calculating_area_figures${METRICS_JOIN_TEXT}timeSeconds`; + const groupedMetricRows = [ + { + key: groupKey, + type: IMetricMetaData.CONTINUOUS, + allowedData: [], + context: ['home'], + }, + ]; + (repo.find as jest.Mock).mockResolvedValueOnce(groupedMetricRows); + queryRepositoryMock.getMetricKeysWithQueries.mockResolvedValueOnce([groupKey]); + + const res = await service.getAllMetrics(new UpgradeLogger()); + + expect(res).toEqual([ + { + key: 'masteryWorkspace', + hasQuery: true, + allowedData: [], + context: ['home'], + metadata: { type: IMetricMetaData.CONTINUOUS }, + children: [ + { + key: 'calculating_area_figures', + allowedData: [], + context: ['home'], + metadata: { type: IMetricMetaData.CONTINUOUS }, + children: [ + { + key: 'timeSeconds', + allowedData: [], + context: ['home'], + metadata: { type: IMetricMetaData.CONTINUOUS }, + children: [], + }, + ], + }, + ], + }, + ]); }); it('should save all simple metrics', async () => { diff --git a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts index 13e9664efb..5abeee0e1c 100644 --- a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts @@ -25,7 +25,10 @@ describe('AnalysisDataService', () => { service.fetchMetrics(); - expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl); + expect(mockHttpClient.get).toHaveBeenCalledWith( + expectedUrl, + expect.objectContaining({ context: expect.anything() }) + ); }); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.ts index 4e3d5f8106..ce96f264e8 100644 --- a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.ts @@ -1,7 +1,8 @@ import { Injectable } from '@angular/core'; -import { HttpClient } from '@angular/common/http'; +import { HttpClient, HttpContext } from '@angular/common/http'; import { UpsertMetrics } from './store/analysis.models'; import { API_ENDPOINTS } from '../api-endpoints.constants'; +import { SKIP_NAVIGATION_CANCEL } from '../http-interceptors/http-cancel.interceptor'; @Injectable() export class AnalysisDataService { @@ -9,7 +10,10 @@ export class AnalysisDataService { fetchMetrics() { const url = API_ENDPOINTS.metrics; - return this.http.get(url); + // The metrics catalog is app-wide state kept fresh reactively (e.g. after an experiment is + // deleted or its metrics change), not data scoped to whichever page triggered the fetch, so + // it shouldn't be aborted by HttpCancelInterceptor if the user navigates before it resolves. + return this.http.get(url, { context: new HttpContext().set(SKIP_NAVIGATION_CANCEL, true) }); } upsertMetrics(metrics: UpsertMetrics) { diff --git a/packages/frontend/projects/upgrade/src/app/core/analysis/store/analysis.effects.ts b/packages/frontend/projects/upgrade/src/app/core/analysis/store/analysis.effects.ts index e90b684501..03a4eb882a 100644 --- a/packages/frontend/projects/upgrade/src/app/core/analysis/store/analysis.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/analysis/store/analysis.effects.ts @@ -1,7 +1,7 @@ import { Injectable } from '@angular/core'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import * as AnalysisActions from './analysis.actions'; -import { switchMap, catchError, map, filter, withLatestFrom } from 'rxjs/operators'; +import { switchMap, catchError, map, filter, withLatestFrom, defaultIfEmpty } from 'rxjs/operators'; import { AnalysisDataService } from '../analysis.data.service'; import { Store, select } from '@ngrx/store'; import { AppState } from '../../core.state'; @@ -35,7 +35,9 @@ export class AnalysisEffects { switchMap((metrics) => this.analysisDataService.upsertMetrics(metrics).pipe( map(() => AnalysisActions.actionFetchMetrics()), - catchError(() => [AnalysisActions.actionUpsertMetricsFailure()]) + catchError(() => [AnalysisActions.actionUpsertMetricsFailure()]), + // Guard against HttpCancelInterceptor cancelling this request on navigation. + defaultIfEmpty(AnalysisActions.actionUpsertMetricsFailure()) ) ) ) @@ -57,7 +59,9 @@ export class AnalysisEffects { return AnalysisActions.actionDeleteMetricSuccess({ metrics: data, key }); } }), - catchError(() => [AnalysisActions.actionDeleteMetricFailure()]) + catchError(() => [AnalysisActions.actionDeleteMetricFailure()]), + // Guard against HttpCancelInterceptor cancelling this request on navigation. + defaultIfEmpty(AnalysisActions.actionDeleteMetricFailure()) ) ) ) diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts index 804d9ab623..888d9d1d87 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.spec.ts @@ -51,6 +51,9 @@ import { actionFetchRewardsDataForExperiment, actionFetchRewardsDataForExperimentSuccess, actionFetchRewardsDataForExperimentFailure, + actionUpdateExperimentMetrics, + actionUpdateExperimentMetricsSuccess, + actionUpdateExperimentMetricsFailure, } from './experiments.actions'; import { ExperimentEffects } from './experiments.effects'; import { @@ -595,6 +598,38 @@ describe('ExperimentEffects', () => { })); }); + describe('#updateExperimentMetrics$', () => { + const experiment = { id: 'test1' } as any; + const updateExperimentMetricsRequest = { experiment, metrics: [] } as any; + + it('should return an array with actionUpdateExperimentMetricsSuccess and actionFetchMetrics', fakeAsync(() => { + experimentDataService.updateExperimentMetrics = jest.fn().mockReturnValue(of(experiment)); + + service.updateExperimentMetrics$.pipe(take(2), pairwise()).subscribe((result: any) => { + const successAction = actionUpdateExperimentMetricsSuccess({ experiment }); + const fetchAction = actionFetchMetrics(); + + tick(0); + expect(result).toEqual([successAction, fetchAction]); + }); + + actions$.next(actionUpdateExperimentMetrics({ updateExperimentMetricsRequest })); + })); + + it('should throw an error with actionUpdateExperimentMetricsFailure on error', fakeAsync(() => { + experimentDataService.updateExperimentMetrics = jest.fn().mockReturnValue(throwError('testError')); + + service.updateExperimentMetrics$.subscribe((result: any) => { + const failureAction = actionUpdateExperimentMetricsFailure(); + + tick(0); + expect(result).toEqual(failureAction); + }); + + actions$.next(actionUpdateExperimentMetrics({ updateExperimentMetricsRequest })); + })); + }); + describe('#getExperimentById$', () => { const experimentId = 'testId'; const experiment = { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts index d2c8f23454..54c01446a0 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts @@ -266,9 +266,14 @@ export class ExperimentEffects { ofType(experimentAction.actionUpdateExperimentMetrics), switchMap((action) => { return this.experimentDataService.updateExperimentMetrics(action.updateExperimentMetricsRequest).pipe( - map((experiment) => { + switchMap((experiment) => { this.notificationService.showSuccess(this.translate.instant('experiments.metrics.update-success.text')); - return experimentAction.actionUpdateExperimentMetricsSuccess({ experiment }); + // A metric may have been added, edited, or removed from the experiment, which can + // change whether a catalog metric still has a query associated with it. + return [ + experimentAction.actionUpdateExperimentMetricsSuccess({ experiment }), + analysisActions.actionFetchMetrics(), + ]; }), catchError(() => { this.notificationService.showError(this.translate.instant('experiments.metrics.update-error.text')); @@ -288,9 +293,12 @@ export class ExperimentEffects { this.experimentDataService.deleteExperiment(experimentId).pipe( switchMap(() => { this.notificationService.showSuccess(this.translate.instant('global.delete-experiments.message.text')); + // Deleting an experiment also deletes its queries, which can change whether a + // catalog metric still has a query associated with it. return [ experimentAction.actionDeleteExperimentSuccess({ experimentId }), experimentAction.actionFetchAllDecisionPoints(), + analysisActions.actionFetchMetrics(), ]; }), catchError(() => [experimentAction.actionDeleteExperimentFailure()]) diff --git a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-cancel.interceptor.ts b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-cancel.interceptor.ts index d7ca81a9f8..1986975add 100644 --- a/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-cancel.interceptor.ts +++ b/packages/frontend/projects/upgrade/src/app/core/http-interceptors/http-cancel.interceptor.ts @@ -1,6 +1,6 @@ // managehttp.interceptor.ts import { Injectable } from '@angular/core'; -import { HttpRequest, HttpHandler, HttpEvent, HttpInterceptor } from '@angular/common/http'; +import { HttpRequest, HttpHandler, HttpEvent, HttpInterceptor, HttpContextToken } from '@angular/common/http'; import { Observable, Subject } from 'rxjs'; import { Router, ActivationEnd } from '@angular/router'; import { takeUntil } from 'rxjs/operators'; @@ -13,13 +13,24 @@ import { takeUntil } from 'rxjs/operators'; * The rxjs observable pipe will work like this: * * WHEN an Http request goes out, this intercept function will fire, - * IF 'ActivationEnd' event ever emitted from router (meaning user has successfully navigated away from page): + * IF 'ActivationEnd' event ever emitted from router: * THEN make 'pendingHTTPRequests$' observable emit an event (no value even matters) * * The 'takeUntil' rxjs operator will 'complete' the HTTPRequest observable when this observable emits any event. * This will cancel the request. + * + * NOTE: 'ActivationEnd' fires for ANY completed navigation, not just ones a user directly triggered + * (e.g. clicking a link). Programmatic navigations dispatched from effects (like + * `navigateOnDeleteExperiment$` calling `router.navigate(...)`) fire it too, and will cancel any + * request still pending at that moment - including ones kicked off by the very same effect chain. + * + * Requests that represent app-wide background sync (rather than data for the page that issued them) + * can opt out of this behavior by setting the SKIP_NAVIGATION_CANCEL context token to true, so they + * complete even if a navigation (user- or effect-triggered) happens before the response arrives. */ +export const SKIP_NAVIGATION_CANCEL = new HttpContextToken(() => false); + @Injectable() export class HttpCancelInterceptor implements HttpInterceptor { private pendingHTTPRequests$ = new Subject(); @@ -33,6 +44,9 @@ export class HttpCancelInterceptor implements HttpInterceptor { } intercept(req: HttpRequest, next: HttpHandler): Observable> { + if (req.context.get(SKIP_NAVIGATION_CANCEL)) { + return next.handle(req); + } return next.handle(req).pipe(takeUntil(this.onCancelPendingRequests())); } diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.html b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.html index 8f640234fb..0fc44804c1 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.html +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.html @@ -57,8 +57,16 @@ fiber_manual_record {{ node.key }} - @if (permissions.metrics.delete && keyEditMode) { - delete_outline + @if (permissions.metrics.delete && keyEditMode && node.isTopLevel) { + + delete_outline + } @@ -70,8 +78,15 @@ {{ node.key }} - @if (permissions.metrics.delete && keyEditMode) { - delete_outline + @if (permissions.metrics.delete && keyEditMode && node.isTopLevel) { + delete_outline } @if (nestedTreeControl.isExpanded(node)) { diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.scss b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.scss index 5da8047987..49a612cace 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.scss +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.scss @@ -81,6 +81,11 @@ &:hover { cursor: pointer; } + + &.disabled { + opacity: 0.4; + cursor: not-allowed; + } } .mat-mdc-cell { diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.ts index 2547462ead..000535319e 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.component.ts @@ -83,11 +83,12 @@ export class MetricsComponent implements OnInit, OnDestroy, AfterViewInit { !!node.loadChildren || (node.children && node.children.length > 0); // Process a single metric node, setting up lazy loading for its children - private insertNode(metric: IMetricUnit): LazyLoadingMetric { + private insertNode(metric: IMetricUnit, isTopLevel = false): LazyLoadingMetric { const processedMetric: LazyLoadingMetric = { ...metric, id: this.insertNodeIndex++, children: [], + isTopLevel, }; if (metric.children && metric.children.length > 0) { @@ -112,7 +113,7 @@ export class MetricsComponent implements OnInit, OnDestroy, AfterViewInit { // Process the entire metrics data to prepare for lazy loading private processMetricsData(metrics: IMetricUnit[]): LazyLoadingMetric[] { this.insertNodeIndex = 0; - return metrics.map((item) => this.insertNode(item)); + return metrics.map((item) => this.insertNode(item, true)); } openAddMetricDialog() { @@ -125,6 +126,9 @@ export class MetricsComponent implements OnInit, OnDestroy, AfterViewInit { } deleteNode(nodeToBeDeleted: LazyLoadingMetric, index: number) { + if (nodeToBeDeleted.hasQuery) { + return; + } const data = { children: [this.allMetrics.data[index]], }; diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.model.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.model.ts index 28f8d96118..4770240da2 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.model.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/profile/components/metrics/metrics.model.ts @@ -5,4 +5,5 @@ export interface LazyLoadingMetric extends IMetricUnit { id: number; children: LazyLoadingMetric[]; loadChildren?: () => Observable; + isTopLevel?: boolean; } diff --git a/packages/frontend/projects/upgrade/src/assets/i18n/en.json b/packages/frontend/projects/upgrade/src/assets/i18n/en.json index 4b015c586d..56cf358b6f 100644 --- a/packages/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/packages/frontend/projects/upgrade/src/assets/i18n/en.json @@ -844,6 +844,7 @@ "metrics.subtitle.text": "Manage metrics here", "metric.delete-metric.message.text": "Type the metric path separated by space to confirm deletion:", "metric.delete-metric.input-placeholder.text": "Metric path", + "metric.delete-metric.disabled-tooltip.text": "This metric is currently being used by an experiment", "metric.read-mode.text": "Read mode", "metric.add-metrics.text": "ADD METRICS", "query.table-id.text": "ID", diff --git a/packages/types/src/Experiment/interfaces.ts b/packages/types/src/Experiment/interfaces.ts index 288d7571a0..9e493ddadf 100644 --- a/packages/types/src/Experiment/interfaces.ts +++ b/packages/types/src/Experiment/interfaces.ts @@ -167,6 +167,7 @@ export interface IMetricUnit { children?: IMetricUnit[]; metadata?: { type: IMetricMetaData }; allowedData?: string[]; + hasQuery?: boolean; } export type ExperimentQueryComparator = '=' | '<>' | '!='; From 8ae0a5dff0fa42aeaa64bb83feab3170e61e5f88 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Wed, 26 Aug 2026 17:35:00 -0400 Subject: [PATCH 2/3] fix test to verify SKIP_NAVIGATION_CANCEL --- .../src/app/core/analysis/analysis.data.service.spec.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts index 5abeee0e1c..930a822b6e 100644 --- a/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts +++ b/packages/frontend/projects/upgrade/src/app/core/analysis/analysis.data.service.spec.ts @@ -1,8 +1,9 @@ -import { HttpClient } from '@angular/common/http'; +import { HttpClient, HttpContext } from '@angular/common/http'; import { of } from 'rxjs'; import { AnalysisDataService } from './analysis.data.service'; import { UpsertMetrics } from './store/analysis.models'; import { API_ENDPOINTS } from '../api-endpoints.constants'; +import { SKIP_NAVIGATION_CANCEL } from '../http-interceptors/http-cancel.interceptor'; class MockHTTPClient { get = jest.fn().mockReturnValue(of()); @@ -27,8 +28,11 @@ describe('AnalysisDataService', () => { expect(mockHttpClient.get).toHaveBeenCalledWith( expectedUrl, - expect.objectContaining({ context: expect.anything() }) + expect.objectContaining({ context: expect.any(HttpContext) }) ); + + const [, options] = mockHttpClient.get.mock.calls[0]; + expect(options.context.get(SKIP_NAVIGATION_CANCEL)).toBe(true); }); }); From bad614b9088d36e18237a2c1e4076990f584a751 Mon Sep 17 00:00:00 2001 From: Benjamin Blanchard Date: Thu, 27 Aug 2026 14:58:44 -0400 Subject: [PATCH 3/3] block deleting in-use metrics on the backend --- .../src/api/controllers/MetricController.ts | 2 ++ .../src/api/repositories/QueryRepository.ts | 23 ++++++++++++- .../backend/src/api/services/MetricService.ts | 8 +++++ .../backend/test/unit/mockdata/mockRepo.ts | 2 ++ .../unit/repositories/QueryRepository.test.ts | 33 +++++++++++++++++++ .../test/unit/services/MetricService.test.ts | 17 +++++++--- 6 files changed, 80 insertions(+), 5 deletions(-) diff --git a/packages/backend/src/api/controllers/MetricController.ts b/packages/backend/src/api/controllers/MetricController.ts index 40f7a019e7..48f0820e0d 100644 --- a/packages/backend/src/api/controllers/MetricController.ts +++ b/packages/backend/src/api/controllers/MetricController.ts @@ -133,6 +133,8 @@ export class MetricController { * description: Delete metric by key * '404': * description: Metrics key not found + * '409': + * description: Metric is used by one or more experiments and cannot be deleted */ @Delete('/:key') public delete( diff --git a/packages/backend/src/api/repositories/QueryRepository.ts b/packages/backend/src/api/repositories/QueryRepository.ts index a79a4f74c2..12701825e9 100644 --- a/packages/backend/src/api/repositories/QueryRepository.ts +++ b/packages/backend/src/api/repositories/QueryRepository.ts @@ -70,7 +70,8 @@ export class QueryRepository extends Repository { public async getMetricKeysWithQueries(): Promise { const queryResult = await this.createQueryBuilder('query') .innerJoin('query.metric', 'metric') - .select('DISTINCT metric.key', 'metricKey') + .distinct(true) + .select('metric.key', 'metricKey') .getRawMany() .catch((errorMsg: any) => { const errorMsgString = repositoryError('QueryRepository', 'getMetricKeysWithQueries', {}, errorMsg); @@ -79,4 +80,24 @@ export class QueryRepository extends Repository { return queryResult.map((row: { metricKey: string }) => row.metricKey); } + + public async getExperimentsUsingMetricKey( + key: string, + metricJoinText: string + ): Promise> { + const queryResult = await this.createQueryBuilder('query') + .innerJoin('query.metric', 'metric') + .innerJoin('query.experiment', 'experiment') + .where('metric.key = :keyValue OR metric.key LIKE :key', { keyValue: key, key: `${key}${metricJoinText}%` }) + .distinct(true) + .select('experiment.id', 'id') + .addSelect('experiment.name', 'name') + .getRawMany() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError('QueryRepository', 'getExperimentsUsingMetricKey', { key }, errorMsg); + throw errorMsgString; + }); + + return queryResult; + } } diff --git a/packages/backend/src/api/services/MetricService.ts b/packages/backend/src/api/services/MetricService.ts index e987b1c2a3..e21a20b18b 100644 --- a/packages/backend/src/api/services/MetricService.ts +++ b/packages/backend/src/api/services/MetricService.ts @@ -56,6 +56,14 @@ export class MetricService { public async deleteMetric(key: string, logger: UpgradeLogger): Promise { logger.info({ message: `Delete metric by key ${key}` }); + const experimentsUsingMetric = await this.queryRepository.getExperimentsUsingMetricKey(key, METRICS_JOIN_TEXT); + if (experimentsUsingMetric.length) { + const experimentNames = experimentsUsingMetric.map(({ name }) => name).join(', '); + throw new HttpError( + 409, + `Metric key ${key} cannot be deleted because it is used by the following experiment(s): ${experimentNames}` + ); + } const result = await this.metricRepository.deleteMetricsByKeys(key, METRICS_JOIN_TEXT); if (!result.length) { throw new HttpError(404, `Metric key not found: ${key}`); diff --git a/packages/backend/test/unit/mockdata/mockRepo.ts b/packages/backend/test/unit/mockdata/mockRepo.ts index 2b6a40ecde..1fa8837483 100644 --- a/packages/backend/test/unit/mockdata/mockRepo.ts +++ b/packages/backend/test/unit/mockdata/mockRepo.ts @@ -7,6 +7,7 @@ export const initializeMocks = (result) => { returning: jest.fn().mockReturnThis(), select: jest.fn().mockReturnThis(), addSelect: jest.fn().mockReturnThis(), + distinct: jest.fn().mockReturnThis(), orderBy: jest.fn().mockReturnThis(), addOrderBy: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), @@ -42,6 +43,7 @@ export const initializeMocks = (result) => { returning: mocks.returning, select: mocks.select, addSelect: mocks.addSelect, + distinct: mocks.distinct, orderBy: mocks.orderBy, addOrderBy: mocks.addOrderBy, limit: mocks.limit, diff --git a/packages/backend/test/unit/repositories/QueryRepository.test.ts b/packages/backend/test/unit/repositories/QueryRepository.test.ts index e0aea58db8..20128d380c 100644 --- a/packages/backend/test/unit/repositories/QueryRepository.test.ts +++ b/packages/backend/test/unit/repositories/QueryRepository.test.ts @@ -187,6 +187,8 @@ describe('QueryRepository Testing', () => { expect(repo.createQueryBuilder).toHaveBeenCalledTimes(1); expect(mock.innerJoin).toHaveBeenCalledTimes(1); + expect(mock.distinct).toHaveBeenCalledTimes(1); + expect(mock.distinct).toHaveBeenCalledWith(true); expect(mock.select).toHaveBeenCalledTimes(1); expect(mock.getRawMany).toHaveBeenCalledTimes(1); @@ -206,4 +208,35 @@ describe('QueryRepository Testing', () => { expect(mock.select).toHaveBeenCalledTimes(1); expect(mock.getRawMany).toHaveBeenCalledTimes(1); }); + + it('should return the experiments using a metric key', async () => { + mock.getRawMany.mockResolvedValue([{ id: 'exp1', name: 'Experiment 1' }]); + const res = await repo.getExperimentsUsingMetricKey('metric1', '@__@'); + + expect(repo.createQueryBuilder).toHaveBeenCalledTimes(1); + + expect(mock.innerJoin).toHaveBeenCalledTimes(2); + expect(mock.where).toHaveBeenCalledTimes(1); + expect(mock.distinct).toHaveBeenCalledTimes(1); + expect(mock.distinct).toHaveBeenCalledWith(true); + expect(mock.select).toHaveBeenCalledTimes(1); + expect(mock.addSelect).toHaveBeenCalledTimes(1); + expect(mock.getRawMany).toHaveBeenCalledTimes(1); + + expect(res).toEqual([{ id: 'exp1', name: 'Experiment 1' }]); + }); + + it('should throw an error when getting experiments using a metric key fails', async () => { + mock.getRawMany.mockRejectedValue(err); + + expect(async () => { + await repo.getExperimentsUsingMetricKey('metric1', '@__@'); + }).rejects.toThrow(err); + + expect(repo.createQueryBuilder).toHaveBeenCalledTimes(1); + + expect(mock.innerJoin).toHaveBeenCalledTimes(2); + expect(mock.where).toHaveBeenCalledTimes(1); + expect(mock.getRawMany).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/backend/test/unit/services/MetricService.test.ts b/packages/backend/test/unit/services/MetricService.test.ts index 49df966091..e98cb65a7b 100644 --- a/packages/backend/test/unit/services/MetricService.test.ts +++ b/packages/backend/test/unit/services/MetricService.test.ts @@ -1,5 +1,4 @@ import { MetricService, METRICS_JOIN_TEXT } from '../../../src/api/services/MetricService'; -import { Repository } from 'typeorm'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; import { IGroupMetric, IMetricMetaData, ISingleMetric } from 'upgrade_types'; @@ -13,8 +12,8 @@ import { configureLogger } from '../../utils/logger'; describe('Audit Service Testing', () => { let service: MetricService; - let repo: Repository; - let queryRepositoryMock: { getMetricKeysWithQueries: jest.Mock }; + let repo: MetricRepository; + let queryRepositoryMock: { getMetricKeysWithQueries: jest.Mock; getExperimentsUsingMetricKey: jest.Mock }; let module: TestingModule; const settingRes = [{ id: 'id', toCheckAuth: false, toFilterMetric: true }]; @@ -98,6 +97,7 @@ describe('Audit Service Testing', () => { provide: getRepositoryToken(QueryRepository), useValue: { getMetricKeysWithQueries: jest.fn().mockResolvedValue(['totalProblemsCompleted']), + getExperimentsUsingMetricKey: jest.fn().mockResolvedValue([]), }, }, { @@ -120,7 +120,7 @@ describe('Audit Service Testing', () => { }).compile(); service = module.get(MetricService); - repo = module.get>(getRepositoryToken(MetricRepository)); + repo = module.get(getRepositoryToken(MetricRepository)); queryRepositoryMock = module.get(getRepositoryToken(QueryRepository)); }); @@ -200,6 +200,15 @@ describe('Audit Service Testing', () => { expect(res).toEqual(metricResult); }); + it('should throw an error when deleting a metric used by an experiment', async () => { + queryRepositoryMock.getExperimentsUsingMetricKey.mockResolvedValueOnce([{ id: 'exp1', name: 'Experiment 1' }]); + + await expect(service.deleteMetric('totalProblemsCompleted', new UpgradeLogger())).rejects.toThrow( + 'Metric key totalProblemsCompleted cannot be deleted because it is used by the following experiment(s): Experiment 1' + ); + expect(repo.deleteMetricsByKeys).not.toHaveBeenCalled(); + }); + it('should throw an error when metrics filter not enabled', async () => { settingRes[0].toFilterMetric = false;