Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-31 00:16:47 +01:00
Code Issues Projects Releases Wiki Activity

feat: return CR uses of segments when flag is active (#5378)

Browse Source 
This PR changes the payload of the strategiesBySegment endpoint when the
flag is active. In addition to returning just the strategies, the object
will also contain a new property, called `changeRequestStrategies`
containing the strategies that are used in change requests.

This PR does not update the schema. That can be done later when the
changes go into beta. This also allows us some time to iterate on the
payload without changing the public API.

## Discussion points:

Should `strategies` and `changeRequestStrategies` ever contain
duplicates? Take this scenario:
- Strategy S uses segment T.
- There is an open change request that updates the list of segments for
S to T and a new segment U.
- In this case, strategy S would show up both in `strategies` _and_ in
`changeRequestStrategies`.

We have two options: 
1. Filter the list of change request strategies, so that they don't
contain any duplicates (this is currently how it's implemented)
2. Ignore the duplicates and just send both lists as is.

We're doing option 2 for now.
This commit is contained in:
Thomas Heartman 2023-11-22 07:51:04 +01:00 committed by GitHub
parent 7ddcceed8a
commit 8337885e47
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 286 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
src/lib/features/segment/segment-controller.ts
View File

@ -347,25 +347,47 @@ export class SegmentsController extends Controller {
req: IAuthRequest<{ id: number }>,
res: Response<SegmentStrategiesSchema>,
): Promise<void> {
const { id } = req.params;
const id = Number(req.params.id);
const { user } = req;
const strategies = await this.segmentService.getVisibleStrategies(
id,
user.id,
);
// Remove unnecessary IFeatureStrategy fields from the response.
const segmentStrategies = strategies.map((strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
}));
if (this.flagResolver.isEnabled('detectSegmentUsageInChangeRequests')) {
const mapStrategies = (strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
});
res.json({
strategies: segmentStrategies,
});
const mapChangeRequestStrategies = (strategy) => ({
...(strategy.id ? { id: strategy.id } : {}),
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
});
res.json({
strategies: strategies.strategies.map(mapStrategies),
changeRequestStrategies: strategies.changeRequestStrategies.map(
mapChangeRequestStrategies,
),
});
} else {
const segmentStrategies = strategies.strategies.map((strategy) => ({
id: strategy.id,
projectId: strategy.projectId,
featureName: strategy.featureName,
strategyName: strategy.strategyName,
environment: strategy.environment,
}));
res.json({ strategies: segmentStrategies });
}
}
async removeSegment(
@ -379,7 +401,7 @@ export class SegmentsController extends Controller {
segmentIsInUse = await this.segmentService.isInUse(id);
} else {
const strategies = await this.segmentService.getAllStrategies(id);
segmentIsInUse = strategies.length > 0;
segmentIsInUse = strategies.strategies.length > 0;
}
if (segmentIsInUse) {

10
src/lib/segments/segment-service-interface.ts
View File

@ -1,6 +1,12 @@
import { ChangeRequestStrategy } from 'lib/features/change-request-segment-usage-service/change-request-segment-usage-read-model';
import { UpsertSegmentSchema } from 'lib/openapi';
import { IClientSegment, IFeatureStrategy, ISegment, IUser } from 'lib/types';
export type StrategiesUsingSegment = {
strategies: IFeatureStrategy[];
changeRequestStrategies: ChangeRequestStrategy[];
};
export interface ISegmentService {
updateStrategySegments: (
strategyId: string,
@ -18,12 +24,12 @@ export interface ISegmentService {
* This is NOT considering the private projects
* For most use cases, use `getVisibleStrategies`
*/
getAllStrategies(id: number): Promise<IFeatureStrategy[]>;
getAllStrategies(id: number): Promise<StrategiesUsingSegment>;
getVisibleStrategies(
id: number,
userId: number,
): Promise<IFeatureStrategy[]>;
): Promise<StrategiesUsingSegment>;
validateName(name: string): Promise<void>;

56
src/lib/services/segment-service.ts
View File

@ -18,7 +18,10 @@ import {
import User from '../types/user';
import { IFeatureStrategiesStore } from '../features/feature-toggle/types/feature-toggle-strategies-store-type';
import BadDataError from '../error/bad-data-error';
import { ISegmentService } from '../segments/segment-service-interface';
import {
ISegmentService,
StrategiesUsingSegment,
} from '../segments/segment-service-interface';
import { PermissionError } from '../error';
import { IChangeRequestAccessReadModel } from '../features/change-request-access-service/change-request-access-read-model';
import { IPrivateProjectChecker } from '../features/private-project/privateProjectCheckerType';
@ -90,39 +93,50 @@ export class SegmentService implements ISegmentService {
async getVisibleStrategies(
id: number,
userId: number,
): Promise<IFeatureStrategy[]> {
const strategies = await this.getAllStrategies(id);
): Promise<StrategiesUsingSegment> {
const allStrategies = await this.getAllStrategies(id);
if (this.flagResolver.isEnabled('privateProjects')) {
const accessibleProjects =
await this.privateProjectChecker.getUserAccessibleProjects(
userId,
);
if (accessibleProjects.mode === 'all') {
return strategies;
return allStrategies;
} else {
return strategies.filter((strategy) =>
accessibleProjects.projects.includes(strategy.projectId),
);
const filter = (strategy) =>
accessibleProjects.projects.includes(strategy.projectId);
return {
strategies: allStrategies.strategies.filter(filter),
changeRequestStrategies:
allStrategies.changeRequestStrategies.filter(filter),
};
}
}
return strategies;
return allStrategies;
}
async getAllStrategies(id: number): Promise<IFeatureStrategy[]> {
async getAllStrategies(id: number): Promise<StrategiesUsingSegment> {
const strategies =
await this.featureStrategiesStore.getStrategiesBySegment(id);
return strategies;
const strategyIds = new Set(strategies.map((s) => s.id));
const changeRequestStrategies = (
await this.changeRequestSegmentUsageReadModel.getStrategiesUsedInActiveChangeRequests(
id,
)
).filter(
(strategy) => !('id' in strategy && strategyIds.has(strategy.id)),
);
return { strategies, changeRequestStrategies };
}
async isInUse(id: number): Promise<boolean> {
const strategies = await this.getAllStrategies(id);
if (strategies.length > 0) {
return true;
}
const { strategies, changeRequestStrategies } =
await this.getAllStrategies(id);
return await this.changeRequestSegmentUsageReadModel.isSegmentUsedInActiveChangeRequests(
id,
);
return strategies.length > 0 || changeRequestStrategies.length > 0;
}
async create(
@ -300,11 +314,13 @@ export class SegmentService implements ISegmentService {
id: number,
segment: Omit<ISegment, 'id'>,
): Promise<void> {
const strategies =
await this.featureStrategiesStore.getStrategiesBySegment(id);
const { strategies, changeRequestStrategies } =
await this.getAllStrategies(id);
const projectsUsed = new Set(
strategies.map((strategy) => strategy.projectId),
[strategies, changeRequestStrategies].flatMap((strats) =>
strats.map((strategy) => strategy.projectId),
),
);
if (

263
src/test/e2e/api/admin/segment.e2e.test.ts
View File

@ -15,6 +15,7 @@ import {
IUnleashTest,
setupAppWithCustomConfig,
} from '../../helpers/test-helper';
import { StrategiesUsingSegment } from 'lib/segments/segment-service-interface';
let app: IUnleashTest;
let db: ITestDb;
@ -49,11 +50,11 @@ const fetchFeatures = (): Promise<IFeatureToggleClient[]> =>
const fetchSegmentStrategies = (
segmentId: number,
): Promise<IFeatureStrategy[]> =>
): Promise<StrategiesUsingSegment> =>
app.request
.get(`${SEGMENTS_BASE_PATH}/${segmentId}/strategies`)
.expect(200)
.then((res) => res.body.strategies);
.then((res) => res.body);
const createSegment = (
postData: object,
@ -234,57 +235,6 @@ test('should not delete segments used by strategies', async () => {
expect((await fetchSegments()).length).toEqual(1);
});
test('should not delete segments used by strategies in CRs', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
const CR_ID = 54321;
const user = await db.stores.userStore.insert({
username: 'test',
});
await db.rawDatabase.table('change_requests').insert({
id: CR_ID,
environment: 'default',
state: 'In review',
project: 'default',
created_by: user.id,
created_at: '2023-01-01 00:00:00',
min_approvals: 1,
title: 'My change request',
});
await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
action: 'addStrategy',
payload: {
name: 'flexibleRollout',
title: '',
disabled: false,
segments: [segment.id],
variants: [],
parameters: {
groupId: toggle.name,
rollout: '100',
stickiness: 'default',
},
constraints: [],
},
created_at: '2023-01-01 00:01:00',
change_request_id: CR_ID,
created_by: user.id,
});
expect((await fetchSegments()).length).toEqual(1);
await app.request.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`).expect(409);
expect((await fetchSegments()).length).toEqual(1);
});
test('should list strategies by segment', async () => {
await createSegment({ name: 'S1', constraints: [] });
await createSegment({ name: 'S2', constraints: [] });
@ -335,15 +285,15 @@ test('should list strategies by segment', async () => {
const segmentStrategies2 = await fetchSegmentStrategies(segment2.id);
const segmentStrategies3 = await fetchSegmentStrategies(segment3.id);
expect(collectIds(segmentStrategies1)).toEqual(
expect(collectIds(segmentStrategies1.strategies)).toEqual(
collectIds(feature1.strategies),
);
expect(collectIds(segmentStrategies2)).toEqual(
expect(collectIds(segmentStrategies2.strategies)).toEqual(
collectIds([...feature1.strategies, ...feature2.strategies]),
);
expect(collectIds(segmentStrategies3)).toEqual(
expect(collectIds(segmentStrategies3.strategies)).toEqual(
collectIds([
...feature1.strategies,
...feature2.strategies,
@ -477,3 +427,204 @@ test('Should show usage in features and projects', async () => {
const segments = await fetchSegments();
expect(segments).toMatchObject([{ usedInFeatures: 1, usedInProjects: 1 }]);
});
describe('detect strategy usage in change requests', () => {
const CR_ID = 54321;
let user;
beforeAll(async () => {
user = await db.stores.userStore.insert({
username: 'test',
});
await db.rawDatabase.table('change_requests').insert({
id: CR_ID,
environment: 'default',
state: 'In review',
project: 'default',
created_by: user.id,
created_at: '2023-01-01 00:00:00',
min_approvals: 1,
title: 'My change request',
});
});
afterAll(async () => {
user = await db.stores.userStore.delete(user.id);
await db.rawDatabase.table('change_requests').delete();
});
afterEach(async () => {
await db.rawDatabase.table('change_request_events').delete();
});
test('should not delete segments used by strategies in CRs', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
action: 'addStrategy',
payload: {
name: 'flexibleRollout',
title: '',
disabled: false,
segments: [segment.id],
variants: [],
parameters: {
groupId: toggle.name,
rollout: '100',
stickiness: 'default',
},
constraints: [],
},
created_at: '2023-01-01 00:01:00',
change_request_id: CR_ID,
created_by: user.id,
});
expect((await fetchSegments()).length).toEqual(1);
await app.request
.delete(`${SEGMENTS_BASE_PATH}/${segment.id}`)
.expect(409);
expect((await fetchSegments()).length).toEqual(1);
});
test('Should show segment usage in addStrategy events', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
action: 'addStrategy',
payload: {
name: 'flexibleRollout',
title: '',
disabled: false,
segments: [segment.id],
variants: [],
parameters: {
groupId: toggle.name,
rollout: '100',
stickiness: 'default',
},
constraints: [],
},
created_at: '2023-01-01 00:01:00',
change_request_id: CR_ID,
created_by: user.id,
});
const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
expect(changeRequestStrategies).toMatchObject([
{
environment: 'default',
featureName: toggle.name,
projectId: 'default',
strategyName: 'flexibleRollout',
},
]);
expect(strategies).toStrictEqual([]);
});
test('Should show segment usage in updateStrategy events', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await addStrategyToFeatureEnv(
app,
{ ...toggle.strategies[0] },
'default',
toggle.name,
);
const [feature] = await fetchFeatures();
const strategyId = feature.strategies[0].id;
await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
action: 'updateStrategy',
payload: {
id: strategyId,
name: 'flexibleRollout',
title: '',
disabled: false,
segments: [segment.id],
variants: [],
parameters: {
groupId: toggle.name,
rollout: '100',
stickiness: 'default',
},
constraints: [],
},
created_at: '2023-01-01 00:01:00',
change_request_id: CR_ID,
created_by: user.id,
});
const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
expect(changeRequestStrategies).toMatchObject([{ id: strategyId }]);
expect(strategies).toStrictEqual([]);
});
test('If a segment is used in an existing strategy and in a CR for the same strategy, the strategy should only be listed once', async () => {
await createSegment({ name: 'a', constraints: [] });
const toggle = mockFeatureToggle();
await createFeatureToggle(app, toggle);
const [segment] = await fetchSegments();
await addStrategyToFeatureEnv(
app,
{ ...toggle.strategies[0] },
'default',
toggle.name,
);
const [feature] = await fetchFeatures();
const strategyId = feature.strategies[0].id;
await addSegmentsToStrategy([segment.id], strategyId!);
await db.rawDatabase.table('change_request_events').insert({
feature: toggle.name,
action: 'updateStrategy',
payload: {
id: strategyId,
name: 'flexibleRollout',
title: '',
disabled: false,
segments: [segment.id],
variants: [],
parameters: {
groupId: toggle.name,
rollout: '100',
stickiness: 'default',
},
constraints: [],
},
created_at: '2023-01-01 00:01:00',
change_request_id: CR_ID,
created_by: user.id,
});
const { strategies, changeRequestStrategies } =
await fetchSegmentStrategies(segment.id);
expect(strategies).toMatchObject([{ id: strategyId }]);
expect(changeRequestStrategies).toStrictEqual([]);
});
});