Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/backend/src/api/controllers/ExperimentController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,9 @@ interface ExperimentListImportValidation {
* description:
* type: string
* minLength: 1
* usedByCount:
* type: integer
* description: Number of non-archived experiments using this decision point
* order: {}
* queries:
* type: array
Expand Down
40 changes: 40 additions & 0 deletions packages/backend/src/api/repositories/DecisionPointRepository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ import { DecisionPoint } from '../models/DecisionPoint';
import { Repository, EntityManager } from 'typeorm';
import { EntityRepository } from '../../typeorm-typedi-extensions';
import repositoryError from './utils/repositoryError';
import { EXPERIMENT_STATE } from 'upgrade_types';

export interface DecisionPointUsageCount {
decisionPointId: string;
usedByCount: number;
}

@EntityRepository(DecisionPoint)
export class DecisionPointRepository extends Repository<DecisionPoint> {
Expand Down Expand Up @@ -106,6 +112,40 @@ export class DecisionPointRepository extends Repository<DecisionPoint> {
});
}

public async getUsageCountsForExperiment(
experimentId: string,
entityManager?: EntityManager
): Promise<DecisionPointUsageCount[]> {
// Treat site-target pairs as case-insensitive and normalize missing targets to empty.
const repository = entityManager ? entityManager.getRepository(DecisionPoint) : this;

return await repository
.createQueryBuilder('sourceDecisionPoint')
.select('sourceDecisionPoint.id', 'decisionPointId')
.addSelect('COUNT(DISTINCT usedByExperiment.id)::int', 'usedByCount')
.leftJoin(
DecisionPoint,
'usedByDecisionPoint',
`LOWER(usedByDecisionPoint.site) = LOWER(sourceDecisionPoint.site)
AND LOWER(COALESCE(usedByDecisionPoint.target, '')) = LOWER(COALESCE(sourceDecisionPoint.target, ''))`
)
Comment thread
zackcl marked this conversation as resolved.
.leftJoin('usedByDecisionPoint.experiment', 'usedByExperiment', 'usedByExperiment.state != :archivedState', {
archivedState: EXPERIMENT_STATE.ARCHIVED,
})
.where('"sourceDecisionPoint"."experimentId" = :experimentId', { experimentId })
.groupBy('sourceDecisionPoint.id')
.getRawMany()
.catch((errorMsg: any) => {
const errorMsgString = repositoryError(
this.constructor.name,
'getUsageCountsForExperiment',
{ experimentId },
errorMsg
);
throw errorMsgString;
});
}

public async setAllPendingActivationFalse(experimentId: string, entityManager?: EntityManager): Promise<void> {
const that = entityManager ? entityManager : this;
await that
Expand Down
48 changes: 43 additions & 5 deletions packages/backend/src/api/services/ExperimentService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
} from '../models/Experiment';

import { ExperimentConditionRepository } from '../repositories/ExperimentConditionRepository';
import { DecisionPointRepository } from '../repositories/DecisionPointRepository';
import { DecisionPointRepository, DecisionPointUsageCount } from '../repositories/DecisionPointRepository';
import { ExperimentCondition } from '../models/ExperimentCondition';
import { DecisionPoint } from '../models/DecisionPoint';
import { ExperimentSchedulerService } from './ExperimentSchedulerService';
Expand Down Expand Up @@ -232,14 +232,38 @@ export class ExperimentService {
}

public async getSingleExperiment(id: string, logger?: UpgradeLogger): Promise<ExperimentDTO | undefined> {
const experiment = await this.findOne(id, logger);
const [experiment, decisionPointUsageCounts] = await Promise.all([
this.findOne(id, logger),
this.decisionPointRepository.getUsageCountsForExperiment(id),
]);

if (experiment) {
return this.reducedConditionPayload(this.formattingPayload(experiment));
return this.attachDecisionPointUsageCounts(
this.reducedConditionPayload(this.formattingPayload(experiment)),
decisionPointUsageCounts
);
} else {
return undefined;
}
}

private attachDecisionPointUsageCounts(
experiment: ExperimentDTO,
decisionPointUsageCounts: DecisionPointUsageCount[]
): ExperimentDTO {
const decisionPointUsageCountById = new Map(
decisionPointUsageCounts.map(({ decisionPointId, usedByCount }) => [decisionPointId, Number(usedByCount)])
);

return {
...experiment,
partitions: experiment.partitions.map((decisionPoint) => ({
...decisionPoint,
usedByCount: decisionPointUsageCountById.get(decisionPoint.id) ?? 0,
})),
};
}

public async findOne(id: string, logger?: UpgradeLogger): Promise<Experiment | undefined> {
if (logger) {
logger.info({ message: `Find experiment by id => ${id}` });
Expand Down Expand Up @@ -420,9 +444,15 @@ export class ExperimentService {
if (logger) {
logger.info({ message: `Update the experiment`, details: experiment });
}
return this.reducedConditionPayload(
const updatedExperiment = this.reducedConditionPayload(
await this.updateExperimentInDB(experiment, currentUser, logger, entityManager)
);
const decisionPointUsageCounts = await this.decisionPointRepository.getUsageCountsForExperiment(
updatedExperiment.id,
entityManager
);

return this.attachDecisionPointUsageCounts(updatedExperiment, decisionPointUsageCounts);
}

public async getExperimentalConditions(experimentId: string, logger: UpgradeLogger): Promise<ExperimentCondition[]> {
Expand Down Expand Up @@ -542,7 +572,15 @@ export class ExperimentService {
this.transformStateTimeLogs([responseStateTimeLog])[0],
];

return this.reducedConditionPayload(this.formattingPayload(this.formattingConditionPayload(oldExperiment)));
const updatedExperiment = this.reducedConditionPayload(
this.formattingPayload(this.formattingConditionPayload(oldExperiment))
);
const decisionPointUsageCounts = await this.decisionPointRepository.getUsageCountsForExperiment(
experimentId,
entityManager
);

return this.attachDecisionPointUsageCounts(updatedExperiment, decisionPointUsageCounts);
}

public async verifyExperiments(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { DecisionPointRepository } from '../../../src/api/repositories/DecisionP
import { DecisionPoint } from '../../../src/api/models/DecisionPoint';
import { Container } from '../../../src/typeorm-typedi-extensions';
import { initializeMocks } from '../mockdata/mockRepo';
import { EXPERIMENT_STATE } from 'upgrade_types';

let mock;
let manager;
Expand Down Expand Up @@ -38,6 +39,7 @@ beforeEach(() => {

manager = {
createQueryBuilder: repo.createQueryBuilder,
getRepository: jest.fn().mockReturnValue({ createQueryBuilder: repo.createQueryBuilder }),
};
});

Expand Down Expand Up @@ -196,6 +198,51 @@ describe('DecisionPointRepository Testing', () => {
expect(mock.getMany).toHaveBeenCalledTimes(1);
});

it('should get distinct non-archived experiment usage counts for each decision point in an experiment', async () => {
const usageCounts = [{ decisionPointId: decisionPoint.id, usedByCount: 2 }];
mock.getRawMany.mockResolvedValue(usageCounts);

const res = await repo.getUsageCountsForExperiment('experiment-1');

expect(repo.createQueryBuilder).toHaveBeenCalledWith('sourceDecisionPoint');
expect(mock.select).toHaveBeenCalledWith('sourceDecisionPoint.id', 'decisionPointId');
expect(mock.addSelect).toHaveBeenCalledWith('COUNT(DISTINCT usedByExperiment.id)::int', 'usedByCount');
expect(mock.leftJoin).toHaveBeenNthCalledWith(
1,
DecisionPoint,
'usedByDecisionPoint',
expect.stringContaining('COALESCE(usedByDecisionPoint.target')
);
expect(mock.leftJoin).toHaveBeenNthCalledWith(
2,
'usedByDecisionPoint.experiment',
'usedByExperiment',
'usedByExperiment.state != :archivedState',
{ archivedState: EXPERIMENT_STATE.ARCHIVED }
);
expect(mock.where).toHaveBeenCalledWith('"sourceDecisionPoint"."experimentId" = :experimentId', {
experimentId: 'experiment-1',
});
expect(mock.groupBy).toHaveBeenCalledWith('sourceDecisionPoint.id');
expect(mock.getRawMany).toHaveBeenCalledTimes(1);
expect(res).toEqual(usageCounts);
});

it('should use the provided entity manager to get decision point usage counts within a transaction', async () => {
mock.getRawMany.mockResolvedValue([]);

await repo.getUsageCountsForExperiment('experiment-1', manager);

expect(manager.getRepository).toHaveBeenCalledWith(DecisionPoint);
expect(repo.createQueryBuilder).toHaveBeenCalledWith('sourceDecisionPoint');
});

it('should throw an error when getting decision point usage counts fails', async () => {
mock.getRawMany.mockRejectedValue(err);

await expect(repo.getUsageCountsForExperiment('experiment-1')).rejects.toThrow(err);
});

describe('setAllPendingActivationFalse', () => {
it('should set pendingActivation to false for all DPs using entityManager', async () => {
await repo.setAllPendingActivationFalse('experiment-1', manager);
Expand Down
64 changes: 64 additions & 0 deletions packages/backend/test/unit/services/ExperimentService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ describe('ExperimentService Testing', () => {
return {
save: jest.fn().mockResolvedValue(mockExperiment),
findOne: jest.fn().mockResolvedValue(mockExperiment),
findBy: jest.fn().mockResolvedValue([mockExperiment]),
};
}
if (entity === StateTimeLog) {
Expand Down Expand Up @@ -289,6 +290,7 @@ describe('ExperimentService Testing', () => {
find: jest.fn().mockResolvedValue([mockDecisionPoint1]),
save: jest.fn().mockResolvedValue(mockDecisionPoint1),
findOne: jest.fn().mockResolvedValue(null),
getUsageCountsForExperiment: jest.fn().mockResolvedValue([]),
upsertDecisionPoint: jest.fn().mockImplementation((value) => Promise.resolve(value)),
deleteDecisionPoint: jest.fn().mockResolvedValue({ affected: 1 }),
deleteByIds: jest.fn().mockResolvedValue({ affected: 1 }),
Expand Down Expand Up @@ -468,6 +470,19 @@ describe('ExperimentService Testing', () => {
);
});

it('should include current decision point usage counts in the updated experiment response', async () => {
decisionPointRepo.getUsageCountsForExperiment = jest
.fn()
.mockResolvedValue([{ decisionPointId: mockDecisionPoint1.id, usedByCount: 2 }]);

const result = await service.update(mockExperimentDTO, mockUser, logger);

expect(decisionPointRepo.getUsageCountsForExperiment).toHaveBeenCalledWith(mockExperimentDTO.id, undefined);
expect(result.partitions).toEqual(
expect.arrayContaining([expect.objectContaining({ id: mockDecisionPoint1.id, usedByCount: 2 })])
);
});

it('should update conditions when they are modified', async () => {
const result = await service.update(mockExperimentDTO, mockUser, logger);

Expand Down Expand Up @@ -983,6 +998,55 @@ describe('ExperimentService Testing', () => {

expect(decisionPointRepo.setAllPendingActivationFalse).not.toHaveBeenCalled();
});

it('should include current decision point usage counts in the updated state response', async () => {
decisionPointRepo.getUsageCountsForExperiment = jest
.fn()
.mockResolvedValue([{ decisionPointId: mockDecisionPoint1.id, usedByCount: 2 }]);

const result = await service.updateState(
mockExperiment.id,
EXPERIMENT_STATE.PAUSED,
mockUser,
logger,
undefined,
entityManager
);

expect(decisionPointRepo.getUsageCountsForExperiment).toHaveBeenCalledWith(mockExperiment.id, entityManager);
expect(result.partitions).toEqual(
expect.arrayContaining([expect.objectContaining({ id: mockDecisionPoint1.id, usedByCount: 2 })])
);
});
});

describe('getSingleExperiment()', () => {
it('should attach decision point usage counts to the single experiment response', async () => {
const secondDecisionPoint = {
...createMockDecisionPoint1(),
id: 'partition-2',
site: 'another-site',
} as DecisionPoint;
const experiment = {
...mockExperiment,
partitions: [mockDecisionPoint1 as DecisionPoint, secondDecisionPoint],
} as Experiment;

jest.spyOn(service, 'findOne').mockResolvedValue(experiment);
decisionPointRepo.getUsageCountsForExperiment = jest
.fn()
.mockResolvedValue([{ decisionPointId: mockDecisionPoint1.id, usedByCount: 2 }]);

const result = await service.getSingleExperiment(mockExperiment.id);

expect(decisionPointRepo.getUsageCountsForExperiment).toHaveBeenCalledWith(mockExperiment.id);
expect(result.partitions).toEqual(
expect.arrayContaining([
expect.objectContaining({ id: mockDecisionPoint1.id, usedByCount: 2 }),
expect.objectContaining({ id: secondDecisionPoint.id, usedByCount: 0 }),
])
);
});
});

describe('paginatedSearchString()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,32 @@ describe('ExperimentDataService', () => {

expect(mockHttpClient.put).toHaveBeenCalledWith(expectedUrl, { ...experiment });
});

it('should strip decision point usage counts from the update request', () => {
const experiment = {
...mockExperiment,
partitions: [
{
id: 'decision-point-1',
site: 'lesson-stream',
target: 'question-hint',
description: '',
order: 1,
createdAt: 'time',
updatedAt: 'time',
versionNumber: 1,
excludeIfReached: false,
usedByCount: 2,
},
],
} as Experiment;

service.updateExperiment(experiment);

const requestBody = mockHttpClient.put.mock.calls[0][1];
expect(requestBody.partitions[0]).not.toHaveProperty('usedByCount');
expect(requestBody.partitions[0]).toEqual(expect.objectContaining({ id: 'decision-point-1' }));
});
});

describe('#updateExperimentState', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable } from '@angular/core';
import {
Experiment,
ExperimentDecisionPoint,
ExperimentStateInfo,
ExperimentPaginationParams,
UpdateExperimentFilterModeRequest,
Expand Down Expand Up @@ -57,7 +58,14 @@ export class ExperimentDataService {
private stripVMProperties(experiment: Experiment): Experiment {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { stat, weightingMethod, ...experimentData } = experiment as any;
return experimentData;
return {
...experimentData,
partitions: experimentData.partitions.map((decisionPoint: ExperimentDecisionPoint) => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { usedByCount, ...decisionPointData } = decisionPoint;
return decisionPointData;
}),
};
}

updateExperimentState(experimentId: string, experimentStateInfo: ExperimentStateInfo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export interface ExperimentDecisionPoint {
versionNumber: number;
excludeIfReached: boolean;
pendingActivation?: boolean;
usedByCount?: number;
}

export interface ExperimentFactor {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@
</td>
</ng-container>

<!-- Used By Column -->
<ng-container matColumnDef="usedBy">
<th mat-header-cell *matHeaderCellDef class="used-by-column ft-14-600">
{{ DECISION_POINT_TRANSLATION_KEYS.USED_BY | translate }}
</th>
<td mat-cell *matCellDef="let decisionPoint" class="used-by-column ft-14-400">
{{ getUsedByCountText(decisionPoint) }}
</td>
</ng-container>

<!-- Exclude If Reached Column -->
<ng-container matColumnDef="excludeIfReached">
<th mat-header-cell *matHeaderCellDef class="exclude-if-reached-column ft-14-600">
Expand Down
Loading