Skip to content
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.

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

fixed segments not being copied #2105

Merged
merged 17 commits into from
Oct 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmu
import { getFeatureStrategyIcon } from 'utils/strategyNames';
import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
import { CopyButton } from './CopyButton/CopyButton';
import { useSegments } from '../../../../hooks/api/getters/useSegments/useSegments';
import { IFeatureStrategyPayload } from '../../../../interfaces/strategy';

interface IFeatureStrategyEmptyProps {
projectId: string;
Expand Down Expand Up @@ -65,6 +67,7 @@ export const FeatureStrategyEmpty = ({
const { id, ...strategyCopy } = {
...strategy,
environment: environmentId,
copyOf: strategy.id,
};

return addStrategyToFeature(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
Tooltip,
} from '@mui/material';
import { AddToPhotos as CopyIcon, Lock } from '@mui/icons-material';
import { IFeatureStrategy } from 'interfaces/strategy';
import { IFeatureStrategy, IFeatureStrategyPayload } from 'interfaces/strategy';
import { useRequiredPathParam } from 'hooks/useRequiredPathParam';
import { IFeatureEnvironment } from 'interfaces/featureToggle';
import AccessContext from 'contexts/AccessContext';
Expand All @@ -19,6 +19,7 @@ import useFeatureStrategyApi from 'hooks/api/actions/useFeatureStrategyApi/useFe
import useToast from 'hooks/useToast';
import { useFeatureImmutable } from 'hooks/api/getters/useFeature/useFeatureImmutable';
import { formatUnknownError } from 'utils/formatUnknownError';
import { useSegments } from '../../../../../../../../../../hooks/api/getters/useSegments/useSegments';

interface ICopyStrategyIconMenuProps {
environments: IFeatureEnvironment['name'][];
Expand All @@ -31,6 +32,8 @@ export const CopyStrategyIconMenu: VFC<ICopyStrategyIconMenuProps> = ({
}) => {
const projectId = useRequiredPathParam('projectId');
const featureId = useRequiredPathParam('featureId');
const { segments } = useSegments(strategy.id);

const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null);
const open = Boolean(anchorEl);
const { addStrategyToFeature } = useFeatureStrategyApi();
Expand All @@ -48,6 +51,7 @@ export const CopyStrategyIconMenu: VFC<ICopyStrategyIconMenuProps> = ({
const { id, ...strategyCopy } = {
...strategy,
environment: environmentId,
copyOf: strategy.id,
};

try {
Expand Down
1 change: 1 addition & 0 deletions frontend/src/interfaces/strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface IFeatureStrategyPayload {
name?: string;
constraints: IConstraint[];
parameters: IFeatureStrategyParameters;
copyOf?: string;
}

export interface IStrategy {
Expand Down
5 changes: 4 additions & 1 deletion src/lib/db/environment-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,10 @@ export default class EnvironmentStore implements IEnvironmentStore {
async getAll(query?: Object): Promise<IEnvironment[]> {
let qB = this.db<IEnvironmentsTable>(TABLE)
.select('*')
.orderBy('sort_order', 'created_at');
.orderBy([
{ column: 'sort_order', order: 'asc' },
{ column: 'created_at', order: 'asc' },
]);
if (query) {
qB = qB.where(query);
}
Expand Down
5 changes: 4 additions & 1 deletion src/lib/db/feature-strategy-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,10 @@ class FeatureStrategiesStore implements IFeatureStrategiesStore {
feature_name: featureName,
environment,
})
.orderBy('sort_order', 'created_at');
.orderBy([
{ column: 'sort_order', order: 'asc' },
{ column: 'created_at', order: 'asc' },
]);
stopTimer();
return rows.map(mapRow);
}
Expand Down
3 changes: 3 additions & 0 deletions src/lib/openapi/spec/create-feature-strategy-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ export const createFeatureStrategySchema = {
$ref: '#/components/schemas/constraintSchema',
},
},
copyOf: {
type: 'string',
},
parameters: {
$ref: '#/components/schemas/parametersSchema',
},
Expand Down
5 changes: 5 additions & 0 deletions src/lib/openapi/spec/public-signup-token-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export const publicSignupTokenSchema = {
type: 'string',
},
url: {
description:
'The public signup link for the token. Users who follow this link will be taken to a signup page where they can create an Unleash user.',
type: 'string',
},
name: {
Expand All @@ -43,12 +45,15 @@ export const publicSignupTokenSchema = {
},
users: {
type: 'array',
description: 'Array of users that have signed up using the token',
items: {
$ref: '#/components/schemas/userSchema',
},
nullable: true,
},
role: {
description:
'Users who sign up using this token will be given this role.',
$ref: '#/components/schemas/roleSchema',
},
},
Expand Down
35 changes: 31 additions & 4 deletions src/lib/routes/admin-api/project/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { FeatureEnvironmentSchema } from '../../../openapi/spec/feature-environm
import { SetStrategySortOrderSchema } from '../../../openapi/spec/set-strategy-sort-order-schema';

import { emptyResponse } from '../../../openapi/util/standard-responses';
import { SegmentService } from '../../../services/segment-service';

interface FeatureStrategyParams {
projectId: string;
Expand Down Expand Up @@ -68,23 +69,33 @@ const PATH_STRATEGY = `${PATH_STRATEGIES}/:strategyId`;

type ProjectFeaturesServices = Pick<
IUnleashServices,
'featureToggleServiceV2' | 'projectHealthService' | 'openApiService'
| 'featureToggleServiceV2'
| 'projectHealthService'
| 'openApiService'
| 'segmentService'
>;

export default class ProjectFeaturesController extends Controller {
private featureService: FeatureToggleService;

private openApiService: OpenApiService;

private segmentService: SegmentService;

private readonly logger: Logger;

constructor(
config: IUnleashConfig,
{ featureToggleServiceV2, openApiService }: ProjectFeaturesServices,
{
featureToggleServiceV2,
openApiService,
segmentService,
}: ProjectFeaturesServices,
) {
super(config);
this.featureService = featureToggleServiceV2;
this.openApiService = openApiService;
this.segmentService = segmentService;
this.logger = config.getLogger('/admin-api/project/features.ts');

this.route({
Expand Down Expand Up @@ -555,13 +566,29 @@ export default class ProjectFeaturesController extends Controller {
res: Response<FeatureStrategySchema>,
): Promise<void> {
const { projectId, featureName, environment } = req.params;
const { copyOf, ...strategyConfig } = req.body;

const userName = extractUsername(req);
const strategy = await this.featureService.createStrategy(
req.body,
strategyConfig,
{ environment, projectId, featureName },
userName,
);
res.status(200).json(strategy);

if (copyOf) {
this.logger.info(
`Cloning segments from: strategyId=${copyOf} to: strategyId=${strategy.id} `,
);
await this.segmentService.cloneStrategySegments(
copyOf,
strategy.id,
);
}

const updatedStrategy = await this.featureService.getStrategy(
strategy.id,
);
res.status(200).json(updatedStrategy);
}

async getFeatureStrategies(
Expand Down
3 changes: 3 additions & 0 deletions src/lib/routes/public-invite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export class PublicInviteController extends Controller {
openApiService.validPath({
tags: ['Public signup tokens'],
operationId: 'validatePublicSignupToken',
summary: `Validates a public signup token exists, has not expired and is enabled`,
responses: {
200: emptyResponse,
...getStandardResponses(400),
Expand All @@ -70,6 +71,8 @@ export class PublicInviteController extends Controller {
openApiService.validPath({
tags: ['Public signup tokens'],
operationId: 'addPublicSignupTokenUser',
summary:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if the schema updates for public-invite belongs in this PR. Fine for now, but we should aim to separate PR concerns in the future

'Create a user with the "viewer" root role and link them to a signup token',
requestBody: createRequestSchema('createInvitedUserSchema'),
responses: {
200: createResponseSchema('userSchema'),
Expand Down
69 changes: 56 additions & 13 deletions src/lib/services/feature-toggle-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import {
IFeatureOverview,
IFeatureStrategy,
IFeatureToggleQuery,
ISegment,
IStrategyConfig,
IVariant,
WeightType,
Expand Down Expand Up @@ -274,12 +275,14 @@ class FeatureToggleService {

featureStrategyToPublic(
featureStrategy: IFeatureStrategy,
segments: ISegment[] = [],
): Saved<IStrategyConfig> {
return {
id: featureStrategy.id,
name: featureStrategy.strategyName,
constraints: featureStrategy.constraints || [],
parameters: featureStrategy.parameters,
segments: segments.map((segment) => segment.id) ?? [],
};
}

Expand Down Expand Up @@ -321,7 +324,13 @@ class FeatureToggleService {
});

const tags = await this.tagStore.getAllTagsForFeature(featureName);
const strategy = this.featureStrategyToPublic(newFeatureStrategy);
const segments = await this.segmentService.getByStrategy(
newFeatureStrategy.id,
);
const strategy = this.featureStrategyToPublic(
newFeatureStrategy,
segments,
);
await this.eventStore.store(
new FeatureStrategyAddEvent({
project: projectId,
Expand Down Expand Up @@ -376,10 +385,17 @@ class FeatureToggleService {
updates,
);

const segments = await this.segmentService.getByStrategy(
strategy.id,
);

// Store event!
const tags = await this.tagStore.getAllTagsForFeature(featureName);
const data = this.featureStrategyToPublic(strategy);
const preData = this.featureStrategyToPublic(existingStrategy);
const data = this.featureStrategyToPublic(strategy, segments);
const preData = this.featureStrategyToPublic(
existingStrategy,
segments,
);
await this.eventStore.store(
new FeatureStrategyUpdateEvent({
project: projectId,
Expand Down Expand Up @@ -415,8 +431,14 @@ class FeatureToggleService {
existingStrategy,
);
const tags = await this.tagStore.getAllTagsForFeature(featureName);
const data = this.featureStrategyToPublic(strategy);
const preData = this.featureStrategyToPublic(existingStrategy);
const segments = await this.segmentService.getByStrategy(
strategy.id,
);
const data = this.featureStrategyToPublic(strategy, segments);
const preData = this.featureStrategyToPublic(
existingStrategy,
segments,
);
await this.eventStore.store(
new FeatureStrategyUpdateEvent({
featureName,
Expand Down Expand Up @@ -479,6 +501,7 @@ class FeatureToggleService {
featureName: string,
environment: string = DEFAULT_ENV,
): Promise<Saved<IStrategyConfig>[]> {
this.logger.debug('getStrategiesForEnvironment');
const hasEnv = await this.featureEnvironmentStore.featureHasEnvironment(
environment,
featureName,
Expand All @@ -490,13 +513,22 @@ class FeatureToggleService {
featureName,
environment,
);
return featureStrategies.map((strat) => ({
id: strat.id,
name: strat.strategyName,
constraints: strat.constraints,
parameters: strat.parameters,
sortOrder: strat.sortOrder,
}));
const result = [];
for (const strat of featureStrategies) {
const segments =
(await this.segmentService.getByStrategy(strat.id)).map(
(segment) => segment.id,
) ?? [];
result.push({
id: strat.id,
name: strat.strategyName,
constraints: strat.constraints,
parameters: strat.parameters,
sortOrder: strat.sortOrder,
segments,
});
}
return result;
}
throw new NotFoundError(
`Feature ${featureName} does not have environment ${environment}`,
Expand Down Expand Up @@ -718,12 +750,23 @@ class FeatureToggleService {
const strategy = await this.featureStrategiesStore.getStrategyById(
strategyId,
);
return {

const segments = await this.segmentService.getByStrategy(strategyId);
let result: Saved<IStrategyConfig> = {
id: strategy.id,
name: strategy.strategyName,
constraints: strategy.constraints || [],
parameters: strategy.parameters,
segments: [],
};

if (segments && segments.length > 0) {
result = {
...result,
segments: segments.map((segment) => segment.id),
};
}
Comment on lines +763 to +768
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a test that validates that this gives the correct result?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if there are no segments defined, should we return an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreas-unleash It seems this is not reflected in the specification, at least I can't see a spec update for this data structure.

return result;
}

async getEnvironmentInfo(
Expand Down
1 change: 0 additions & 1 deletion src/lib/services/segment-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ export class SegmentService {
const sourceStrategySegments = await this.getByStrategy(
sourceStrategyId,
);

await Promise.all(
sourceStrategySegments.map((sourceStrategySegment) => {
return this.addToStrategy(
Expand Down