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

feat: implement column created_by_user_id in feature_tag #5695

Merged
merged 6 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 2 additions & 0 deletions frontend/src/openapi/models/featureTagSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,6 @@ export interface FeatureTagSchema {
* @deprecated
*/
value?: string;
/** The id of the user who created this tag */
createdByUserId?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

If previously it didn't have a user, can we assume system here? I mean to avoid having it nullable, I feel we're not accomplishing our goal if this is nullable

}
23 changes: 20 additions & 3 deletions src/lib/db/feature-tag-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ interface FeatureTagTable {
feature_name: string;
tag_type: string;
tag_value: string;
created_by_user_id?: number;
}

class FeatureTagStore implements IFeatureTagStore {
Expand Down Expand Up @@ -82,6 +83,7 @@ class FeatureTagStore implements IFeatureTagStore {
featureName: row.feature_name,
tagType: row.tag_type,
tagValue: row.tag_value,
createdByUserId: row.created_by_user_id,
};
}

Expand All @@ -91,6 +93,7 @@ class FeatureTagStore implements IFeatureTagStore {
featureName: row.feature_name,
tagType: row.tag_type,
tagValue: row.tag_value,
createdByUserId: row.created_by_user_id,
}));
}

Expand Down Expand Up @@ -138,13 +141,18 @@ class FeatureTagStore implements IFeatureTagStore {
featureName: row.feature_name,
tagType: row.tag_type,
tagValue: row.tag_value,
createdByUserId: row.created_by_user_id,
}));
}

async tagFeature(featureName: string, tag: ITag): Promise<ITag> {
async tagFeature(
featureName: string,
tag: ITag,
createdByUserId: number,
): Promise<ITag> {
const stopTimer = this.timer('tagFeature');
await this.db<FeatureTagTable>(TABLE)
.insert(this.featureAndTagToRow(featureName, tag))
.insert(this.featureAndTagToRow(featureName, tag, createdByUserId))
.onConflict(COLUMNS)
.merge();
stopTimer();
Expand Down Expand Up @@ -177,6 +185,7 @@ class FeatureTagStore implements IFeatureTagStore {
featureName: row.feature_name,
tagType: row.tag_type,
tagValue: row.tag_value,
createdByUserId: row.created_by_user_id,
}));
}

Expand Down Expand Up @@ -204,7 +213,11 @@ class FeatureTagStore implements IFeatureTagStore {
const stopTimer = this.timer('untagFeature');
try {
await this.db(TABLE)
.where(this.featureAndTagToRow(featureName, tag))
.where({
feature_name: featureName,
tag_type: tag.type,
tag_value: tag.value,
})
.delete();
} catch (err) {
this.logger.error(err);
Expand Down Expand Up @@ -233,11 +246,13 @@ class FeatureTagStore implements IFeatureTagStore {
featureName,
tagType,
tagValue,
createdByUserId,
}: IFeatureTag): FeatureTagTable {
return {
feature_name: featureName,
tag_type: tagType,
tag_value: tagValue,
created_by_user_id: createdByUserId,
};
}

Expand All @@ -248,11 +263,13 @@ class FeatureTagStore implements IFeatureTagStore {
featureAndTagToRow(
featureName: string,
{ type, value }: ITag,
createdByUserId: number,
): FeatureTagTable {
return {
feature_name: featureName,
tag_type: type,
tag_value: value,
created_by_user_id: createdByUserId,
};
}
}
Expand Down
24 changes: 20 additions & 4 deletions src/lib/features/feature-toggle/tests/feature-toggles.e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ let app: IUnleashTest;
let db: ITestDb;
const sortOrderFirst = 0;
const sortOrderSecond = 10;
const TESTUSERID = 3333;

const createSegment = async (segmentName: string) => {
const segment = await app.services.segmentService.create(
Expand Down Expand Up @@ -2991,7 +2992,7 @@ test('Can filter based on tags', async () => {
await db.stores.featureToggleStore.create('default', {
name: 'not-tagged',
});
await db.stores.featureTagStore.tagFeature('to-be-tagged', tag);
await db.stores.featureTagStore.tagFeature('to-be-tagged', tag, TESTUSERID);
await app.request
.get('/api/admin/projects/default/features?tag=simple:hello-tags')
.expect((res) => {
Expand Down Expand Up @@ -3028,10 +3029,12 @@ test('Can query for features with namePrefix and tags', async () => {
await db.stores.featureTagStore.tagFeature(
'to-be-tagged-nameprefix-and-tags',
tag,
TESTUSERID,
);
await db.stores.featureTagStore.tagFeature(
'tagged-but-not-hit-nameprefix-and-tags',
tag,
TESTUSERID,
);
await app.request
.get(
Expand Down Expand Up @@ -3065,13 +3068,26 @@ test('Can query for two tags at the same time. Tags are ORed together', async ()
name: 'tagged-with-both-tags',
},
);
await db.stores.featureTagStore.tagFeature(taggedWithFirst.name, tag);
await db.stores.featureTagStore.tagFeature(
taggedWithFirst.name,
tag,
TESTUSERID,
);
await db.stores.featureTagStore.tagFeature(
taggedWithSecond.name,
secondTag,
TESTUSERID,
);
await db.stores.featureTagStore.tagFeature(
taggedWithBoth.name,
tag,
TESTUSERID,
);
await db.stores.featureTagStore.tagFeature(
taggedWithBoth.name,
secondTag,
TESTUSERID,
);
await db.stores.featureTagStore.tagFeature(taggedWithBoth.name, tag);
await db.stores.featureTagStore.tagFeature(taggedWithBoth.name, secondTag);
await app.request
.get(
`/api/admin/projects/default/features?tag=${tag.type}:${tag.value}&tag=${secondTag.type}:${secondTag.value}`,
Expand Down
6 changes: 6 additions & 0 deletions src/lib/openapi/spec/feature-tag-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,12 @@ export const featureTagSchema = {
description:
'The value of the tag. This property is deprecated and will be removed in a future version of Unleash. Superseded by the `tagValue` property.',
},
createdByUserId: {
type: 'number',
nullable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note so we don't forget: maybe we need to make this not nullable, but if this is used as input it might be a breaking change for the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is part of import/export. So I think it needs to be nullable

example: 1,
description: 'The id of the user who created this tag',
},
},
components: {},
} as const;
Expand Down
1 change: 1 addition & 0 deletions src/lib/schema/feature-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,5 @@ export const featureTagSchema = joi.object().keys({
tagValue: joi.string(),
type: nameType.optional(),
value: joi.string().optional(),
createdByUserId: joi.number().optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is used to validate input. And I believe all new input should have a createdById (but maybe it's a breaking change for the API)

});
13 changes: 9 additions & 4 deletions src/lib/services/feature-tag-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ class FeatureTagService {
const featureToggle = await this.featureToggleStore.get(featureName);
const validatedTag = await tagSchema.validateAsync(tag);
await this.createTagIfNeeded(validatedTag, userName, addedByUserId);
await this.featureTagStore.tagFeature(featureName, validatedTag);
await this.featureTagStore.tagFeature(
featureName,
validatedTag,
addedByUserId,
);

await this.eventService.storeEvent({
type: FEATURE_TAGGED,
Expand Down Expand Up @@ -94,19 +98,20 @@ class FeatureTagService {
featureName,
tagType: addedTag.type,
tagValue: addedTag.value,
createdByUserId: updatedByUserId,
})),
);

await this.featureTagStore.tagFeatures(createdFeatureTags);

const removedFeatureTags: IFeatureTag[] = featureNames.flatMap(
(featureName) =>
const removedFeatureTags: Omit<IFeatureTag, 'createdByUserId'>[] =
featureNames.flatMap((featureName) =>
removedTags.map((addedTag) => ({
featureName,
tagType: addedTag.type,
tagValue: addedTag.value,
})),
);
);

await this.featureTagStore.untagFeatures(removedFeatureTags);

Expand Down
73 changes: 49 additions & 24 deletions src/lib/services/state-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import variantsExportV3 from '../../test/examples/variantsexport_v3.json';
import EventService from './event-service';
import { SYSTEM_USER_ID } from '../types';
const oldExportExample = require('./state-service-export-v1.json');
const TESTUSERID = 3333;

function getSetup() {
const stores = createStores();
Expand Down Expand Up @@ -398,10 +399,14 @@ test('Should not import an existing tag', async () => {
};
await stores.tagTypeStore.createTagType(data.tagTypes[0]);
await stores.tagStore.createTag(data.tags[0]);
await stores.featureTagStore.tagFeature(data.featureTags[0].featureName, {
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
});
await stores.featureTagStore.tagFeature(
data.featureTags[0].featureName,
{
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
},
TESTUSERID,
);
await stateService.import({
data,
userId: SYSTEM_USER_ID,
Expand Down Expand Up @@ -466,10 +471,14 @@ test('should export tag, tagtypes but not feature tags if the feature is not exp
};
await stores.tagTypeStore.createTagType(data.tagTypes[0]);
await stores.tagStore.createTag(data.tags[0]);
await stores.featureTagStore.tagFeature(data.featureTags[0].featureName, {
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
});
await stores.featureTagStore.tagFeature(
data.featureTags[0].featureName,
{
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
},
TESTUSERID,
);

const exported = await stateService.export({
includeFeatureToggles: false,
Expand Down Expand Up @@ -504,10 +513,14 @@ test('should export tag, tagtypes, featureTags and features', async () => {
};
await stores.tagTypeStore.createTagType(data.tagTypes[0]);
await stores.tagStore.createTag(data.tags[0]);
await stores.featureTagStore.tagFeature(data.featureTags[0].featureName, {
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
});
await stores.featureTagStore.tagFeature(
data.featureTags[0].featureName,
{
type: data.featureTags[0].tagType,
value: data.featureTags[0].tagValue,
},
TESTUSERID,
);

const exported = await stateService.export({
includeFeatureToggles: true,
Expand Down Expand Up @@ -667,10 +680,14 @@ test('exporting to new format works', async () => {
parameters: {},
constraints: [],
});
await stores.featureTagStore.tagFeature('Some-feature', {
type: 'simple',
value: 'Test',
});
await stores.featureTagStore.tagFeature(
'Some-feature',
{
type: 'simple',
value: 'Test',
},
TESTUSERID,
);
const exported = await stateService.export({});
expect(exported.featureStrategies).toHaveLength(1);
});
Expand Down Expand Up @@ -725,10 +742,14 @@ test('featureStrategies can keep existing', async () => {
parameters: {},
constraints: [],
});
await stores.featureTagStore.tagFeature('Some-feature', {
type: 'simple',
value: 'Test',
});
await stores.featureTagStore.tagFeature(
'Some-feature',
{
type: 'simple',
value: 'Test',
},
TESTUSERID,
);

const exported = await stateService.export({});
await stateService.import({
Expand Down Expand Up @@ -776,10 +797,14 @@ test('featureStrategies should not keep existing if dropBeforeImport', async ()
parameters: {},
constraints: [],
});
await stores.featureTagStore.tagFeature('Some-feature', {
type: 'simple',
value: 'Test',
});
await stores.featureTagStore.tagFeature(
'Some-feature',
{
type: 'simple',
value: 'Test',
},
TESTUSERID,
);

const exported = await stateService.export({});
exported.featureStrategies = [];
Expand Down
11 changes: 9 additions & 2 deletions src/lib/types/stores/feature-tag-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export interface IFeatureTag {
featureName: string;
tagType: string;
tagValue: string;
createdByUserId?: number;
}

export interface IFeatureAndTag {
Expand All @@ -15,8 +16,14 @@ export interface IFeatureTagStore extends Store<IFeatureTag, IFeatureTag> {
getAllTagsForFeature(featureName: string): Promise<ITag[]>;
getAllFeaturesForTag(tagValue: string): Promise<string[]>;
getAllByFeatures(features: string[]): Promise<IFeatureTag[]>;
tagFeature(featureName: string, tag: ITag): Promise<ITag>;
tagFeature(
featureName: string,
tag: ITag,
createdByUserId: number,
): Promise<ITag>;
tagFeatures(featureTags: IFeatureTag[]): Promise<IFeatureAndTag[]>;
untagFeature(featureName: string, tag: ITag): Promise<void>;
untagFeatures(featureTags: IFeatureTag[]): Promise<void>;
untagFeatures(
featureTags: Omit<IFeatureTag, 'createdByUserId'>[],
): Promise<void>;
}
Loading
Loading