disable deleting metrics that are in use

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(

packages/backend/src/api/repositories/QueryRepository.ts

@@ -66,4 +66,38 @@ export class QueryRepository extends Repository<Query> {
 
     return queryResult.length > 0 ? true : false;
   }
+
+  public async getMetricKeysWithQueries(): Promise<string[]> {
+    const queryResult = await this.createQueryBuilder('query')
+      .innerJoin('query.metric', 'metric')
+      .distinct(true)
+      .select('metric.key', 'metricKey')
+      .getRawMany()
+      .catch((errorMsg: any) => {
+        const errorMsgString = repositoryError('QueryRepository', 'getMetricKeysWithQueries', {}, errorMsg);
+        throw errorMsgString;
+      });
+
+    return queryResult.map((row: { metricKey: string }) => row.metricKey);
+  }
+
+  public async getExperimentsUsingMetricKey(
+    key: string,
+    metricJoinText: string
+  ): Promise<Array<{ id: string; name: string }>> {
+    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;
+  }
 }

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<IMetricUnit[]> {
     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<IMetricUnit[]> {
@@ -50,6 +56,14 @@ export class MetricService {
 
   public async deleteMetric(key: string, logger: UpgradeLogger): Promise<IMetricUnit[]> {
     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}`);
@@ -139,35 +153,44 @@ export class MetricService {
     return keyArrayAndMeta;
   }
 
-  private metricDocumentToJson(metrics: Metric[]): IMetricUnit[] {
+  private metricDocumentToJson(metrics: Metric[], metricKeysWithQueries?: Set<string>): 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;
   }

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,

packages/backend/test/unit/repositories/QueryRepository.test.ts

@@ -179,4 +179,64 @@ 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.distinct).toHaveBeenCalledTimes(1);
+    expect(mock.distinct).toHaveBeenCalledWith(true);
+    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);
+  });
+
+  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);
+  });
 });

packages/backend/test/unit/services/MetricService.test.ts

@@ -1,18 +1,19 @@
-import { MetricService } from '../../../src/api/services/MetricService';
-import { Repository } from 'typeorm';
+import { MetricService, METRICS_JOIN_TEXT } from '../../../src/api/services/MetricService';
 import { Test, TestingModule } from '@nestjs/testing';
 import { getRepositoryToken } from '@nestjs/typeorm';
 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';
 
 describe('Audit Service Testing', () => {
   let service: MetricService;
-  let repo: Repository<MetricRepository>;
+  let repo: MetricRepository;
+  let queryRepositoryMock: { getMetricKeysWithQueries: jest.Mock; getExperimentsUsingMetricKey: jest.Mock };
   let module: TestingModule;
   const settingRes = [{ id: 'id', toCheckAuth: false, toFilterMetric: true }];
 
@@ -64,6 +65,13 @@ describe('Audit Service Testing', () => {
     },
   ];
 
+  const metricResultWithHasQuery = [
+    {
+      ...metricResult[0],
+      hasQuery: true,
+    },
+  ];
+
   beforeAll(() => {
     configureLogger();
   });
@@ -85,6 +93,13 @@ describe('Audit Service Testing', () => {
             save: jest.fn().mockResolvedValue(metric),
           },
         },
+        {
+          provide: getRepositoryToken(QueryRepository),
+          useValue: {
+            getMetricKeysWithQueries: jest.fn().mockResolvedValue(['totalProblemsCompleted']),
+            getExperimentsUsingMetricKey: jest.fn().mockResolvedValue([]),
+          },
+        },
         {
           provide: getRepositoryToken(SettingRepository),
           useValue: {
@@ -105,7 +120,8 @@ describe('Audit Service Testing', () => {
     }).compile();
 
     service = module.get<MetricService>(MetricService);
-    repo = module.get<Repository<MetricRepository>>(getRepositoryToken(MetricRepository));
+    repo = module.get<MetricRepository>(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 () => {
@@ -141,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;
 

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());
@@ -25,7 +26,13 @@ describe('AnalysisDataService', () => {
 
       service.fetchMetrics();
 
-      expect(mockHttpClient.get).toHaveBeenCalledWith(expectedUrl);
+      expect(mockHttpClient.get).toHaveBeenCalledWith(
+        expectedUrl,
+        expect.objectContaining({ context: expect.any(HttpContext) })
+      );
+
+      const [, options] = mockHttpClient.get.mock.calls[0];
+      expect(options.context.get(SKIP_NAVIGATION_CANCEL)).toBe(true);
     });
   });