Skip to content

[WIP] Add list details page for managing include/exclude list values #3264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
zackcl wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

[WIP] Add list details page for managing include/exclude list values #3264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
392 changes: 255 additions & 137 deletions packages/backend/src/api/repositories/ExperimentRepository.ts
View file
Open in desktop

Large diffs are not rendered by default.

108 changes: 65 additions & 43 deletions packages/backend/test/unit/repositories/ExperimentRepository.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,11 @@ describe('ExperimentRepository Testing', () => {

const res = await repo.findAllExperiments();

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(5);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(23);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(5);

// queries are ordered by `order` ASC (NULLS LAST) then `createdAt` ASC as a stable fallback
expect(mock.addOrderBy).toHaveBeenCalledTimes(2);
Expand All @@ -176,11 +176,11 @@ describe('ExperimentRepository Testing', () => {
await repo.findAllExperiments();
}).rejects.toThrow(err);

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(5);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(23);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(5);
});

it('should find all experiments by name', async () => {
Expand Down Expand Up @@ -213,12 +213,12 @@ describe('ExperimentRepository Testing', () => {

const res = await repo.getValidExperiments('context');

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(20);
expect(mock.where).toHaveBeenCalledTimes(3);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(3);
expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(4);

expect(res).toEqual(result);
});
Expand All @@ -230,12 +230,12 @@ describe('ExperimentRepository Testing', () => {
await repo.getValidExperiments('context');
}).rejects.toThrow(err);

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(20);
expect(mock.where).toHaveBeenCalledTimes(3);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(3);
expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(4);
});

it('should get valid experiments with preview', async () => {
Expand All @@ -244,12 +244,12 @@ describe('ExperimentRepository Testing', () => {

const res = await repo.getValidExperimentsWithPreview('context');

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(20);
expect(mock.where).toHaveBeenCalledTimes(3);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(3);
expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(4);

expect(res).toEqual(result);
});
Expand All @@ -261,12 +261,12 @@ describe('ExperimentRepository Testing', () => {
await repo.getValidExperimentsWithPreview('context');
}).rejects.toThrow(err);

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);

expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(20);
expect(mock.where).toHaveBeenCalledTimes(3);
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.getMany).toHaveBeenCalledTimes(3);
expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.getMany).toHaveBeenCalledTimes(4);
});

it('should update experiment state', async () => {
Expand Down Expand Up @@ -369,16 +369,16 @@ describe('ExperimentRepository Testing', () => {
it('should find one experiment ordered by queries.order then createdAt', async () => {
const res = await repo.findOneExperiment(experiment.id);

// 4 parallel queries: conditionLevelPayload, factorDecisionPointPayload, metric, segment
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);
// 5 parallel queries: conditionLevelPayload, factorDecisionPointPayload, metric, inclusion, exclusion
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(5);

// conditions(1) + partitions+factors+levels(3) + queries.order+createdAt(2) = 6 addOrderBy calls
expect(mock.addOrderBy).toHaveBeenCalledTimes(6);
expect(mock.addOrderBy).toHaveBeenCalledWith('queries.order', 'ASC', 'NULLS LAST');
expect(mock.addOrderBy).toHaveBeenCalledWith('queries.createdAt', 'ASC');

expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.getOne).toHaveBeenCalledTimes(4);
expect(mock.where).toHaveBeenCalledTimes(5);
expect(mock.getOne).toHaveBeenCalledTimes(5);

expect(res).toEqual(experiment);
});
Expand Down Expand Up @@ -412,29 +412,33 @@ describe('ExperimentRepository Testing', () => {
});

describe('getValidExperimentsForContextAndDecisionPoint', () => {
it('should build three queries and add a leftJoin on partitions (decision points) for the condition and segment queries', async () => {
it('should build four queries and add a leftJoin on partitions for the condition and segment queries', async () => {
const result = [experiment];
mock.getMany.mockResolvedValue(result);

const res = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
// 4 (conditionLevel) + 6 (factorDecisionPoint) + 10 (segment) = 20
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);
// 4 (conditionLevel) + 6 (factorDecisionPoint) + 5 (inclusion) + 5 (exclusion) = 20
expect(mock.leftJoinAndSelect).toHaveBeenCalledTimes(20);
// conditionLevelPayloadQuery and segmentQuery each add a non-selecting leftJoin for partition filtering
expect(mock.leftJoin).toHaveBeenCalledTimes(2);
// conditionLevelPayloadQuery and both segment queries add a non-selecting leftJoin for partition filtering
expect(mock.leftJoin).toHaveBeenCalledTimes(3);
expect(mock.leftJoin).toHaveBeenCalledWith('experiment.partitions', 'partitions');
// buildSegmentQuery calls .select('experiment.id')
expect(mock.select).toHaveBeenCalledTimes(1);
expect(mock.where).toHaveBeenCalledTimes(3);
expect(mock.getMany).toHaveBeenCalledTimes(3);
// Both segment queries call .select('experiment.id')
expect(mock.select).toHaveBeenCalledTimes(2);
expect(mock.where).toHaveBeenCalledTimes(4);
expect(mock.getMany).toHaveBeenCalledTimes(4);

expect(res).toEqual(result);
});

it('should return empty array when no experiments match the site/target', async () => {
// conditionLevel and segment find experiments, but factorDecisionPoint finds none at this site/target
mock.getMany.mockResolvedValueOnce([experiment]).mockResolvedValueOnce([]).mockResolvedValueOnce([experiment]);
mock.getMany
.mockResolvedValueOnce([experiment])
.mockResolvedValueOnce([])
.mockResolvedValueOnce([experiment])
.mockResolvedValueOnce([experiment]);

const res = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');

Expand All @@ -451,6 +455,7 @@ describe('ExperimentRepository Testing', () => {
mock.getMany
.mockResolvedValueOnce([expA, expB])
.mockResolvedValueOnce([expA])
.mockResolvedValueOnce([expA, expB])
.mockResolvedValueOnce([expA, expB]);

const res = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');
Expand All @@ -462,12 +467,14 @@ describe('ExperimentRepository Testing', () => {
it('should merge condition, partition, and segment data onto each result experiment', async () => {
const condData = { id: 'exp-a', conditions: ['cond1'] } as any;
const factorData = { id: 'exp-a', partitions: ['part1'] } as any;
const segData = { id: 'exp-a', experimentSegmentInclusion: ['seg1'] } as any;
const inclusionData = { id: 'exp-a', experimentSegmentInclusion: ['seg1'] } as any;
const exclusionData = { id: 'exp-a', experimentSegmentExclusion: ['seg2'] } as any;

mock.getMany
.mockResolvedValueOnce([condData])
.mockResolvedValueOnce([factorData])
.mockResolvedValueOnce([segData]);
.mockResolvedValueOnce([inclusionData])
.mockResolvedValueOnce([exclusionData]);

const [result] = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');

Expand All @@ -476,14 +483,19 @@ describe('ExperimentRepository Testing', () => {
conditions: ['cond1'],
partitions: ['part1'],
experimentSegmentInclusion: ['seg1'],
experimentSegmentExclusion: ['seg2'],
});
});

it('should return experiment without segment data when segment query returns no match', async () => {
const condData = { id: 'exp-a', conditions: ['cond1'] } as any;
const factorData = { id: 'exp-a', partitions: ['part1'] } as any;

mock.getMany.mockResolvedValueOnce([condData]).mockResolvedValueOnce([factorData]).mockResolvedValueOnce([]);
mock.getMany
.mockResolvedValueOnce([condData])
.mockResolvedValueOnce([factorData])
.mockResolvedValueOnce([])
.mockResolvedValueOnce([]);

const [result] = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');

Expand All @@ -492,21 +504,31 @@ describe('ExperimentRepository Testing', () => {

it('should return factorDecisionPoint data even when conditionLevel query returns no match', async () => {
const factorData = { id: 'exp-a', partitions: ['part1'] } as any;
const segData = { id: 'exp-a', experimentSegmentInclusion: ['seg1'] } as any;
const inclusionData = { id: 'exp-a', experimentSegmentInclusion: ['seg1'] } as any;
const exclusionData = { id: 'exp-a', experimentSegmentExclusion: ['seg2'] } as any;

mock.getMany.mockResolvedValueOnce([]).mockResolvedValueOnce([factorData]).mockResolvedValueOnce([segData]);
mock.getMany
.mockResolvedValueOnce([])
.mockResolvedValueOnce([factorData])
.mockResolvedValueOnce([inclusionData])
.mockResolvedValueOnce([exclusionData]);

const [result] = await repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1');

expect(result).toMatchObject({ id: 'exp-a', partitions: ['part1'], experimentSegmentInclusion: ['seg1'] });
expect(result).toMatchObject({
id: 'exp-a',
partitions: ['part1'],
experimentSegmentInclusion: ['seg1'],
experimentSegmentExclusion: ['seg2'],
});
});

it('should throw an error when a sub-query fails', async () => {
mock.getMany.mockRejectedValue(err);

await expect(repo.getValidExperimentsForContextAndDecisionPoint('context', 'site1', 'target1')).rejects.toThrow();

expect(repo.createQueryBuilder).toHaveBeenCalledTimes(3);
expect(repo.createQueryBuilder).toHaveBeenCalledTimes(4);
});
});

Expand Down
22 changes: 22 additions & 0 deletions packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import JSZip from 'jszip';
import { TranslateService } from '@ngx-translate/core';
import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service';
import { CommonExportHelpersService } from '../../../shared/services/common-export-helpers.service';
import { LIST_FILTER_MODE } from 'upgrade_types';
import { LIST_OPTION_TYPE } from '../../segments/store/segments.model';
@Injectable()
export class ExperimentEffects {
constructor(
Expand Down Expand Up @@ -560,6 +562,16 @@ export class ExperimentEffects {
map((listResponse) => {
this.notificationService.showSuccess(this.translate.instant('experiments.inclusions.add-success.text'));
this.commonModalEvents.forceCloseModal();
if (action.list.list.listType?.toLowerCase() !== LIST_OPTION_TYPE.SEGMENT.toLowerCase()) {
this.router.navigate([
'/home',
'detail',
action.list.experimentId,
'list',
LIST_FILTER_MODE.INCLUSION,
listResponse.segment.id,
]);
}
return experimentAction.actionAddExperimentInclusionListSuccess({ listResponse });
}),
catchError((error) => {
Expand Down Expand Up @@ -617,6 +629,16 @@ export class ExperimentEffects {
map((listResponse) => {
this.notificationService.showSuccess(this.translate.instant('experiments.exclusions.add-success.text'));
this.commonModalEvents.forceCloseModal();
if (action.list.list.listType?.toLowerCase() !== LIST_OPTION_TYPE.SEGMENT.toLowerCase()) {
this.router.navigate([
'/home',
'detail',
action.list.experimentId,
'list',
LIST_FILTER_MODE.EXCLUSION,
listResponse.segment.id,
]);
}
return experimentAction.actionAddExperimentExclusionListSuccess({ listResponse });
}),
catchError((error) => {
Expand Down
23 changes: 22 additions & 1 deletion packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import { selectSearchString, selectFeatureFlagPaginationParams } from './feature
import { selectCurrentUser } from '../../auth/store/auth.selectors';
import { CommonExportHelpersService } from '../../../shared/services/common-export-helpers.service';
import { of } from 'rxjs';
import { SERVER_ERROR } from 'upgrade_types';
import { LIST_FILTER_MODE, SERVER_ERROR } from 'upgrade_types';
import { LIST_OPTION_TYPE } from '../../segments/store/segments.model';
import { CommonModalEventsService } from '../../../shared/services/common-modal-event.service';

@Injectable()
Expand Down Expand Up @@ -189,6 +190,16 @@ export class FeatureFlagsEffects {
map((listResponse) => {
this.notificationService.showSuccess(this.translate.instant('feature-flags.inclusions.add-success.text'));
this.commonModalEvents.forceCloseModal();
if (action.list.listType?.toLowerCase() !== LIST_OPTION_TYPE.SEGMENT.toLowerCase()) {
this.router.navigate([
'/featureflags',
'detail',
action.list.id,
'list',
LIST_FILTER_MODE.INCLUSION,
listResponse.segment.id,
]);
}
return FeatureFlagsActions.actionAddFeatureFlagInclusionListSuccess({ listResponse });
}),
catchError((error) => {
Expand Down Expand Up @@ -270,6 +281,16 @@ export class FeatureFlagsEffects {
map((listResponse) => {
this.notificationService.showSuccess(this.translate.instant('feature-flags.exclusions.add-success.text'));
this.commonModalEvents.forceCloseModal();
if (action.list.listType?.toLowerCase() !== LIST_OPTION_TYPE.SEGMENT.toLowerCase()) {
this.router.navigate([
'/featureflags',
'detail',
action.list.id,
'list',
LIST_FILTER_MODE.EXCLUSION,
listResponse.segment.id,
]);
}
return FeatureFlagsActions.actionAddFeatureFlagExclusionListSuccess({ listResponse });
}),
catchError((error) => {
Expand Down
Loading