From 8c1e678d7a6404390f29cccb8ca4d7a01f9aec67 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Thu, 25 Jul 2024 11:13:59 +0530 Subject: [PATCH 01/16] showing duplicate key error on UI --- .../api/controllers/FeatureFlagController.ts | 40 +++++++++++++++++++ .../Upgrade/src/api/models/FeatureFlag.ts | 5 ++- .../src/api/services/FeatureFlagService.ts | 21 ++++++++++ .../feature-flags.data.service.ts | 6 +++ .../upsert-feature-flag-modal.component.html | 3 ++ .../upsert-feature-flag-modal.component.ts | 18 +++++++-- .../projects/upgrade/src/assets/i18n/en.json | 1 + 7 files changed, 89 insertions(+), 5 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index 889d655682..5808f41beb 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -16,6 +16,14 @@ interface FeatureFlagsPaginationInfo extends PaginationResponse { nodes: FeatureFlag[]; } +export interface FeatureFlagFormData { + name: string; + key: string; + description: string; + appContext: string; + tags: string[]; +} + /** * @swagger * definitions: @@ -300,6 +308,38 @@ export class FeatureFlagsController { return this.featureFlagService.create(flag, request.logger); } + /** + * @swagger + * /flags/validation: + * post: + * description: Check for duplicate feature flags name or key + * consumes: + * - application/json + * parameters: + * - in: body + * name: flag + * required: true + * schema: + * type: object + * $ref: '#/definitions/FeatureFlag' + * description: Feature flag structure + * tags: + * - Feature Flags + * produces: + * - application/json + * responses: + * '200': + * description: Feature flag name or key is unique + * '409': + * description: Feature flag name or key already exists + */ + @Post('/validation') + public validateFeatureFlags( + @Body({ validate: false }) flag: FeatureFlagFormData, + @Req() request: AppRequest + ): Promise { + return this.featureFlagService.validate(flag, request.logger); + } /** * @swagger * /flags/status: diff --git a/backend/packages/Upgrade/src/api/models/FeatureFlag.ts b/backend/packages/Upgrade/src/api/models/FeatureFlag.ts index aa7b6489cd..c52cda0502 100644 --- a/backend/packages/Upgrade/src/api/models/FeatureFlag.ts +++ b/backend/packages/Upgrade/src/api/models/FeatureFlag.ts @@ -1,4 +1,4 @@ -import { Column, Entity, PrimaryColumn, OneToMany } from 'typeorm'; +import { Column, Entity, PrimaryColumn, OneToMany, Unique } from 'typeorm'; import { IsNotEmpty } from 'class-validator'; import { BaseModel } from './base/BaseModel'; import { Type } from 'class-transformer'; @@ -6,6 +6,7 @@ import { FEATURE_FLAG_STATUS, FILTER_MODE } from 'upgrade_types'; import { FeatureFlagSegmentInclusion } from './FeatureFlagSegmentInclusion'; import { FeatureFlagSegmentExclusion } from './FeatureFlagSegmentExclusion'; @Entity() +@Unique(['key', 'context']) export class FeatureFlag extends BaseModel { @PrimaryColumn('uuid') public id: string; @@ -14,7 +15,7 @@ export class FeatureFlag extends BaseModel { @Column() public name: string; - @Column('text', { unique: true }) + @Column('text') public key: string; @Column() diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 7865548fe6..5665668a6a 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -22,6 +22,7 @@ import { ExperimentUser } from '../models/ExperimentUser'; import { ExperimentAssignmentService } from './ExperimentAssignmentService'; import { SegmentService } from './SegmentService'; import { ErrorWithType } from '../errors/ErrorWithType'; +import { FeatureFlagFormData } from '../controllers/FeatureFlagController'; @Service() export class FeatureFlagService { @@ -204,6 +205,26 @@ export class FeatureFlagService { return this.segmentService.deleteSegment(segmentId, logger); } + public async validate(flagDTO: FeatureFlagFormData, logger: UpgradeLogger): Promise { + logger.info({ message: `Validating featureFlags` }); + const result = await this.featureFlagRepository + .createQueryBuilder('feature_flag') + .where('feature_flag.key = :key', { key: flagDTO.key }) + .andWhere('feature_flag.context = :context', { context: [flagDTO.appContext] }) + .getOne(); + + if (result) { + return 'Feature Flag with this key already exists'; + // const error = new Error('Feature Flag with this key already exists');{ + // error.httpCode = 409; + const error = new Error(`A flag with this key already exists for this app-context`); + (error as any).type = SERVER_ERROR.UNSUPPORTED_CALIPER; + (error as any).httpCode = 409; + throw error; + } + return ''; + } + public async addList( listInput: FeatureFlagListValidator, filterType: string, diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts index c5a6fed5fe..d4fc1f4f44 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts @@ -4,6 +4,7 @@ import { HttpClient } from '@angular/common/http'; import { AddFeatureFlagRequest, FeatureFlag, + FeatureFlagFormData, FeatureFlagsPaginationInfo, FeatureFlagsPaginationParams, UpdateFeatureFlagRequest, @@ -40,6 +41,11 @@ export class FeatureFlagsDataService { return this.http.put(url, flag); } + validateFeatureFlag(flag: FeatureFlagFormData) { + const url = `${this.environment.api.featureFlag}/validation`; + return this.http.post(url, flag); + } + deleteFeatureFlag(id: string) { const url = `${this.environment.api.featureFlag}/${id}`; return this.http.delete(url); diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html index 7c6d91ec3e..b08b70a0d1 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html @@ -19,6 +19,9 @@ Key + + {{ 'feature-flags.duplicate-key-error-text' | translate }} + {{'feature-flags.upsert-flag-modal.key-hint.text' | translate}} Learn more diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 23ea5877aa..efeb1a9221 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -36,6 +36,7 @@ import { BehaviorSubject, combineLatestWith, map, Observable, startWith, Subscri import { TranslateModule } from '@ngx-translate/core'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; import { CommonTextHelpersService } from '../../../../../shared/services/common-text-helpers.service'; +import { FeatureFlagsDataService } from '../../../../../core/feature-flags/feature-flags.data.service'; import isEqual from 'lodash.isequal'; @Component({ @@ -76,6 +77,7 @@ export class UpsertFeatureFlagModalComponent { initialFormValues$ = new BehaviorSubject(null); featureFlagForm: FormGroup; + validationError: boolean; constructor( @Inject(MAT_DIALOG_DATA) @@ -85,6 +87,7 @@ export class UpsertFeatureFlagModalComponent { private featureFlagsService: FeatureFlagsService, private experimentService: ExperimentService, private formHelpersService: CommonFormHelpersService, + private featureFlagDataService: FeatureFlagsDataService, public dialogRef: MatDialogRef ) {} @@ -154,16 +157,25 @@ export class UpsertFeatureFlagModalComponent { this.subscriptions.add(this.isSelectedFeatureFlagUpdated$.subscribe(() => this.closeModal())); } - onPrimaryActionBtnClicked(): void { + async onPrimaryActionBtnClicked(): Promise { if (this.featureFlagForm.valid) { - // Handle extra frontend form validation logic here? - this.sendRequest(this.config.params.action, this.config.params.sourceFlag); + this.validationError = await this.sendValidateRequest(); + if (!this.validationError) { + this.sendRequest(this.config.params.action, this.config.params.sourceFlag); + } } else { // If the form is invalid, manually mark all form controls as touched this.formHelpersService.triggerTouchedToDisplayErrors(this.featureFlagForm); } } + async sendValidateRequest() { + const formData: FeatureFlagFormData = this.featureFlagForm.value; + const result = (await this.featureFlagDataService.validateFeatureFlag(formData).toPromise()) as string; + + return !!result; + } + sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void { const formData: FeatureFlagFormData = this.featureFlagForm.value; diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index cc9e8f50c5..b14e72118a 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -389,6 +389,7 @@ "feature-flags.import-feature-flag.text": "Import Feature Flag", "feature-flags.import-feature-flag.message.text": "The Feature Flag JSON file should include the required properties for it to be imported.", "feature-flags.export-all-feature-flags.text": "Export All Feature Flags", + "feature-flags.duplicate-key-error-text": "Feature flag with this key already exists for this app-context.", "segments.title.text": "Segments", "segments.subtitle.text": "Define new segments to include or exclude from any experiment", "segments.no-segments.text": "Welcome!
Let's start by creating a new segment!", From 84f9d1ed006c7b7074fe6012acf3415165b63f5c Mon Sep 17 00:00:00 2001 From: Yagnik Date: Thu, 25 Jul 2024 15:00:47 +0530 Subject: [PATCH 02/16] error display for duplicate key and added cdr --- .../upsert-feature-flag-modal.component.html | 16 ++++++++---- .../upsert-feature-flag-modal.component.scss | 3 +++ .../upsert-feature-flag-modal.component.ts | 26 ++++++++++--------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html index b08b70a0d1..32742154f4 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html @@ -19,11 +19,17 @@ Key - - {{ 'feature-flags.duplicate-key-error-text' | translate }} - - {{'feature-flags.upsert-flag-modal.key-hint.text' | translate}} - Learn more + + + {{'feature-flags.upsert-flag-modal.key-hint.text' | translate}} + Learn more + + + + + {{ 'feature-flags.duplicate-key-error-text' | translate }} + + diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.scss b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.scss index e69de29bb2..ef4395adac 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.scss +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.scss @@ -0,0 +1,3 @@ +.duplicate-error-message { + color: var(--red); +} \ No newline at end of file diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index efeb1a9221..9a25da916a 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, Component, Inject } from '@angular/core'; +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject } from '@angular/core'; import { CommonModalComponent, CommonTagsInputComponent, @@ -77,7 +77,7 @@ export class UpsertFeatureFlagModalComponent { initialFormValues$ = new BehaviorSubject(null); featureFlagForm: FormGroup; - validationError: boolean; + validationError = false; constructor( @Inject(MAT_DIALOG_DATA) @@ -88,7 +88,8 @@ export class UpsertFeatureFlagModalComponent { private experimentService: ExperimentService, private formHelpersService: CommonFormHelpersService, private featureFlagDataService: FeatureFlagsDataService, - public dialogRef: MatDialogRef + public dialogRef: MatDialogRef, + private cdr: ChangeDetectorRef, ) {} ngOnInit(): void { @@ -157,23 +158,24 @@ export class UpsertFeatureFlagModalComponent { this.subscriptions.add(this.isSelectedFeatureFlagUpdated$.subscribe(() => this.closeModal())); } - async onPrimaryActionBtnClicked(): Promise { + onPrimaryActionBtnClicked() { if (this.featureFlagForm.valid) { - this.validationError = await this.sendValidateRequest(); - if (!this.validationError) { - this.sendRequest(this.config.params.action, this.config.params.sourceFlag); - } + this.sendValidateRequest().then(validationError => { + this.validationError = validationError; + this.cdr.detectChanges(); + if (!this.validationError) { + this.sendRequest(this.config.params.action, this.config.params.sourceFlag); + } + }); } else { // If the form is invalid, manually mark all form controls as touched this.formHelpersService.triggerTouchedToDisplayErrors(this.featureFlagForm); } } - async sendValidateRequest() { + sendValidateRequest() { const formData: FeatureFlagFormData = this.featureFlagForm.value; - const result = (await this.featureFlagDataService.validateFeatureFlag(formData).toPromise()) as string; - - return !!result; + return this.featureFlagDataService.validateFeatureFlag(formData).toPromise().then((result: string) => !!result); } sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void { From 7e1dd7e77c1c5e04694f34b29ebd4802be9869d4 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 26 Jul 2024 10:19:21 +0530 Subject: [PATCH 03/16] added 409 error in backend for unique-pair error --- .../api/middlewares/ErrorHandlerMiddleware.ts | 4 ++ .../src/api/services/FeatureFlagService.ts | 5 +-- .../1721969267641-UniquePairFlag.ts | 43 +++++++++++++++++++ .../upsert-feature-flag-modal.component.ts | 13 ++++-- types/src/Experiment/enums.ts | 1 + 5 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 backend/packages/Upgrade/src/database/migrations/1721969267641-UniquePairFlag.ts diff --git a/backend/packages/Upgrade/src/api/middlewares/ErrorHandlerMiddleware.ts b/backend/packages/Upgrade/src/api/middlewares/ErrorHandlerMiddleware.ts index 522312d553..a5c5756166 100644 --- a/backend/packages/Upgrade/src/api/middlewares/ErrorHandlerMiddleware.ts +++ b/backend/packages/Upgrade/src/api/middlewares/ErrorHandlerMiddleware.ts @@ -97,6 +97,10 @@ export class ErrorHandlerMiddleware implements ExpressErrorMiddlewareInterface { message = error.message; type = SERVER_ERROR.QUERY_FAILED; break; + case 409: + message = error.message; + type = SERVER_ERROR.DUPLICATE_KEY; + break; case 422: message = error.message; type = SERVER_ERROR.UNSUPPORTED_CALIPER; diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 5665668a6a..e89bf61057 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -214,11 +214,8 @@ export class FeatureFlagService { .getOne(); if (result) { - return 'Feature Flag with this key already exists'; - // const error = new Error('Feature Flag with this key already exists');{ - // error.httpCode = 409; const error = new Error(`A flag with this key already exists for this app-context`); - (error as any).type = SERVER_ERROR.UNSUPPORTED_CALIPER; + (error as any).type = SERVER_ERROR.DUPLICATE_KEY; (error as any).httpCode = 409; throw error; } diff --git a/backend/packages/Upgrade/src/database/migrations/1721969267641-UniquePairFlag.ts b/backend/packages/Upgrade/src/database/migrations/1721969267641-UniquePairFlag.ts new file mode 100644 index 0000000000..4f7587e8a8 --- /dev/null +++ b/backend/packages/Upgrade/src/database/migrations/1721969267641-UniquePairFlag.ts @@ -0,0 +1,43 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class UniquePairFlag1721969267641 implements MigrationInterface { + name = 'UniquePairFlag1721969267641'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "feature_flag_segment_exclusion" ALTER COLUMN "enabled" SET DEFAULT false`); + await queryRunner.query(`ALTER TABLE "feature_flag" DROP CONSTRAINT "UQ_960310efa932f7a29eec57350b3"`); + await queryRunner.query(`ALTER TABLE "feature_flag_segment_inclusion" ALTER COLUMN "enabled" SET DEFAULT false`); + await queryRunner.query( + `ALTER TYPE "public"."experiment_error_type_enum" RENAME TO "experiment_error_type_enum_old"` + ); + await queryRunner.query( + `CREATE TYPE "public"."experiment_error_type_enum" AS ENUM('Database not reachable', 'Database auth fail', 'Error in the assignment algorithm', 'Parameter missing in the client request', 'Parameter not in the correct format', 'User ID not found', 'Query Failed', 'Error reported from client', 'Experiment user not defined', 'Experiment user group not defined', 'Working group is not a subset of user group', 'Invalid token', 'Token is not present in request', 'JWT Token validation failed', 'Error in migration', 'Email send error', 'Condition not found', 'Experiment ID not provided for shared Decision Point', 'Experiment ID provided is invalid for shared Decision Point', 'Caliper profile or event not supported', 'Feature Flag with same key already exists for this app-context')` + ); + await queryRunner.query( + `ALTER TABLE "experiment_error" ALTER COLUMN "type" TYPE "public"."experiment_error_type_enum" USING "type"::"text"::"public"."experiment_error_type_enum"` + ); + await queryRunner.query(`DROP TYPE "public"."experiment_error_type_enum_old"`); + await queryRunner.query( + `ALTER TABLE "feature_flag" ADD CONSTRAINT "UQ_653d98ebae725596b4052f80a65" UNIQUE ("key", "context")` + ); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`ALTER TABLE "feature_flag" DROP CONSTRAINT "UQ_653d98ebae725596b4052f80a65"`); + await queryRunner.query( + `CREATE TYPE "public"."experiment_error_type_enum_old" AS ENUM('Caliper profile or event not supported', 'Condition not found', 'Database auth fail', 'Database not reachable', 'Email send error', 'Error in migration', 'Error in the assignment algorithm', 'Error reported from client', 'Experiment ID not provided for shared Decision Point', 'Experiment ID provided is invalid for shared Decision Point', 'Experiment user group not defined', 'Experiment user not defined', 'Invalid token', 'JWT Token validation failed', 'Parameter missing in the client request', 'Parameter not in the correct format', 'Query Failed', 'Token is not present in request', 'User ID not found', 'Working group is not a subset of user group')` + ); + await queryRunner.query( + `ALTER TABLE "experiment_error" ALTER COLUMN "type" TYPE "public"."experiment_error_type_enum_old" USING "type"::"text"::"public"."experiment_error_type_enum_old"` + ); + await queryRunner.query(`DROP TYPE "public"."experiment_error_type_enum"`); + await queryRunner.query( + `ALTER TYPE "public"."experiment_error_type_enum_old" RENAME TO "experiment_error_type_enum"` + ); + await queryRunner.query(`ALTER TABLE "feature_flag_segment_inclusion" ALTER COLUMN "enabled" DROP DEFAULT`); + await queryRunner.query( + `ALTER TABLE "feature_flag" ADD CONSTRAINT "UQ_960310efa932f7a29eec57350b3" UNIQUE ("key")` + ); + await queryRunner.query(`ALTER TABLE "feature_flag_segment_exclusion" ALTER COLUMN "enabled" DROP DEFAULT`); + } +} diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 9a25da916a..5ed0ac04ae 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -88,8 +88,8 @@ export class UpsertFeatureFlagModalComponent { private experimentService: ExperimentService, private formHelpersService: CommonFormHelpersService, private featureFlagDataService: FeatureFlagsDataService, - public dialogRef: MatDialogRef, private cdr: ChangeDetectorRef, + public dialogRef: MatDialogRef ) {} ngOnInit(): void { @@ -160,7 +160,7 @@ export class UpsertFeatureFlagModalComponent { onPrimaryActionBtnClicked() { if (this.featureFlagForm.valid) { - this.sendValidateRequest().then(validationError => { + this.sendValidateRequest().then((validationError) => { this.validationError = validationError; this.cdr.detectChanges(); if (!this.validationError) { @@ -173,9 +173,14 @@ export class UpsertFeatureFlagModalComponent { } } - sendValidateRequest() { + async sendValidateRequest() { const formData: FeatureFlagFormData = this.featureFlagForm.value; - return this.featureFlagDataService.validateFeatureFlag(formData).toPromise().then((result: string) => !!result); + try { + const result = (await this.featureFlagDataService.validateFeatureFlag(formData).toPromise()) as string; + return !!result; + } catch (error) { + return true; + } } sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void { diff --git a/types/src/Experiment/enums.ts b/types/src/Experiment/enums.ts index d7a4228b5c..ee32e15c7f 100644 --- a/types/src/Experiment/enums.ts +++ b/types/src/Experiment/enums.ts @@ -65,6 +65,7 @@ export enum SERVER_ERROR { EXPERIMENT_ID_MISSING_FOR_SHARED_DECISIONPOINT = 'Experiment ID not provided for shared Decision Point', INVALID_EXPERIMENT_ID_FOR_SHARED_DECISIONPOINT = 'Experiment ID provided is invalid for shared Decision Point', UNSUPPORTED_CALIPER = 'Caliper profile or event not supported', + DUPLICATE_KEY = 'Feature Flag with same key already exists for this app-context', } export enum MARKED_DECISION_POINT_STATUS { From edc4668940b5c3eec0d91501e0a0c255005e2c0d Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 29 Jul 2024 12:20:15 +0530 Subject: [PATCH 04/16] remove validate endpoint and return 409 on create/update --- .../src/api/controllers/FeatureFlagController.ts | 5 +++-- .../src/api/services/FeatureFlagService.ts | 13 +++++++------ .../http-interceptors/http-error.interceptor.ts | 5 ++++- .../upsert-feature-flag-modal.component.ts | 15 ++++++++------- 4 files changed, 22 insertions(+), 16 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index 5808f41beb..0800914c98 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -333,13 +333,14 @@ export class FeatureFlagsController { * '409': * description: Feature flag name or key already exists */ - @Post('/validation') + /*@Post('/validation') public validateFeatureFlags( @Body({ validate: false }) flag: FeatureFlagFormData, @Req() request: AppRequest ): Promise { return this.featureFlagService.validate(flag, request.logger); - } + }*/ + /** * @swagger * /flags/status: diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index e89bf61057..fd74e7bc8f 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -22,7 +22,6 @@ import { ExperimentUser } from '../models/ExperimentUser'; import { ExperimentAssignmentService } from './ExperimentAssignmentService'; import { SegmentService } from './SegmentService'; import { ErrorWithType } from '../errors/ErrorWithType'; -import { FeatureFlagFormData } from '../controllers/FeatureFlagController'; @Service() export class FeatureFlagService { @@ -71,8 +70,9 @@ export class FeatureFlagService { return featureFlag; } - public create(flagDTO: FeatureFlagValidation, logger: UpgradeLogger): Promise { + public async create(flagDTO: FeatureFlagValidation, logger: UpgradeLogger): Promise { logger.info({ message: 'Create a new feature flag', details: flagDTO }); + await this.validate(flagDTO, logger); return this.addFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), logger); } @@ -150,8 +150,9 @@ export class FeatureFlagService { return updatedState; } - public update(flagDTO: FeatureFlagValidation, logger: UpgradeLogger): Promise { + public async update(flagDTO: FeatureFlagValidation, logger: UpgradeLogger): Promise { logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); + await this.validate(flagDTO, logger); // TODO add entry in log of updating feature flag return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), logger); } @@ -205,12 +206,12 @@ export class FeatureFlagService { return this.segmentService.deleteSegment(segmentId, logger); } - public async validate(flagDTO: FeatureFlagFormData, logger: UpgradeLogger): Promise { + public async validate(flagDTO: FeatureFlagValidation, logger: UpgradeLogger) { logger.info({ message: `Validating featureFlags` }); const result = await this.featureFlagRepository .createQueryBuilder('feature_flag') .where('feature_flag.key = :key', { key: flagDTO.key }) - .andWhere('feature_flag.context = :context', { context: [flagDTO.appContext] }) + .andWhere('feature_flag.context = :context', { context: flagDTO.context }) .getOne(); if (result) { @@ -219,7 +220,7 @@ export class FeatureFlagService { (error as any).httpCode = 409; throw error; } - return ''; + return; } public async addList( diff --git a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts index 1c3a9644d9..34ec3a013b 100755 --- a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts +++ b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts @@ -17,7 +17,10 @@ export class HttpErrorInterceptor implements HttpInterceptor { openPopup(error) { const temp = { type: NotificationType.Error, - title: 'Network call failed. See console for details.', + title: + error.status === 409 + ? 'Flag with this key already exists for this app-context.' + : 'Network call failed. See console for details.', content: error.url, animate: 'fromRight', }; diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 5ed0ac04ae..baa856f0ea 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -160,13 +160,14 @@ export class UpsertFeatureFlagModalComponent { onPrimaryActionBtnClicked() { if (this.featureFlagForm.valid) { - this.sendValidateRequest().then((validationError) => { - this.validationError = validationError; - this.cdr.detectChanges(); - if (!this.validationError) { - this.sendRequest(this.config.params.action, this.config.params.sourceFlag); - } - }); + this.sendRequest(this.config.params.action, this.config.params.sourceFlag); + // this.sendValidateRequest().then((validationError) => { + // this.validationError = validationError; + // this.cdr.detectChanges(); + // if (!this.validationError) { + // this.sendRequest(this.config.params.action, this.config.params.sourceFlag); + // } + // }); } else { // If the form is invalid, manually mark all form controls as touched this.formHelpersService.triggerTouchedToDisplayErrors(this.featureFlagForm); From f08f723e51e4fd172e630cb7934297ca91b564a3 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 29 Jul 2024 13:14:16 +0530 Subject: [PATCH 05/16] fix test-cases --- .../Upgrade/test/unit/services/FeatureFlagService.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index adc106e8e7..abd65743d3 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -133,6 +133,7 @@ describe('Feature Flag Service Testing', () => { addOrderBy: addOrderBySpy, setParameter: setParamaterSpy, where: jest.fn().mockReturnThis(), + andWhere: jest.fn().mockReturnThis(), offset: offsetSpy, limit: limitSpy, innerJoinAndSelect: jest.fn().mockReturnThis(), @@ -169,6 +170,7 @@ describe('Feature Flag Service Testing', () => { .compile(); service = module.get(FeatureFlagService); + service.validate = jest.fn().mockResolvedValue(null); flagRepo = module.get(getRepositoryToken(FeatureFlagRepository)); }); From ea4ef83c165f5f0f12fec0040532ee3cd34b0786 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Wed, 18 Sep 2024 18:06:21 +0530 Subject: [PATCH 06/16] solve git comments --- .../api/controllers/FeatureFlagController.ts | 41 ------------------- .../src/api/services/FeatureFlagService.ts | 6 +-- .../unit/services/FeatureFlagService.test.ts | 2 +- .../upsert-feature-flag-modal.component.ts | 7 ---- 4 files changed, 4 insertions(+), 52 deletions(-) diff --git a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts index b9506342dd..abfa6a78b2 100644 --- a/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts +++ b/backend/packages/Upgrade/src/api/controllers/FeatureFlagController.ts @@ -35,14 +35,6 @@ interface FeatureFlagsPaginationInfo extends PaginationResponse { nodes: FeatureFlag[]; } -export interface FeatureFlagFormData { - name: string; - key: string; - description: string; - appContext: string; - tags: string[]; -} - /** * @swagger * definitions: @@ -316,39 +308,6 @@ export class FeatureFlagsController { return this.featureFlagService.create(flag, currentUser, request.logger); } - /** - * @swagger - * /flags/validation: - * post: - * description: Check for duplicate feature flags name or key - * consumes: - * - application/json - * parameters: - * - in: body - * name: flag - * required: true - * schema: - * type: object - * $ref: '#/definitions/FeatureFlag' - * description: Feature flag structure - * tags: - * - Feature Flags - * produces: - * - application/json - * responses: - * '200': - * description: Feature flag name or key is unique - * '409': - * description: Feature flag name or key already exists - */ - /*@Post('/validation') - public validateFeatureFlags( - @Body({ validate: false }) flag: FeatureFlagFormData, - @Req() request: AppRequest - ): Promise { - return this.featureFlagService.validate(flag, request.logger); - }*/ - /** * @swagger * /flags/status: diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index ce39bf5fbe..1b9cbfaf8f 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -135,7 +135,7 @@ export class FeatureFlagService { public async create(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: 'Create a new feature flag', details: flagDTO }); - await this.validateForm(flagDTO, logger); + await this.validateUniqueKey(flagDTO, logger); return this.addFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } @@ -287,7 +287,7 @@ export class FeatureFlagService { public async update(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); - await this.validateForm(flagDTO, logger); + await this.validateUniqueKey(flagDTO, logger); // TODO add entry in log of updating feature flag return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } @@ -409,7 +409,7 @@ export class FeatureFlagService { return this.segmentService.deleteSegment(segmentId, logger); } - public async validateForm(flagDTO: FeatureFlagValidation, logger: UpgradeLogger) { + public async validateUniqueKey(flagDTO: FeatureFlagValidation, logger: UpgradeLogger) { logger.info({ message: `Validating featureFlags` }); const result = await this.featureFlagRepository .createQueryBuilder('feature_flag') diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index 5532d0db9c..b361bd8d55 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -242,7 +242,7 @@ describe('Feature Flag Service Testing', () => { .compile(); service = module.get(FeatureFlagService); - service.validateForm = jest.fn().mockResolvedValue(null); + service.validateUniqueKey = jest.fn().mockResolvedValue(null); flagRepo = module.get(getRepositoryToken(FeatureFlagRepository)); }); diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index c4df60eefc..57b2a89df1 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -163,13 +163,6 @@ export class UpsertFeatureFlagModalComponent { onPrimaryActionBtnClicked() { if (this.featureFlagForm.valid) { this.sendRequest(this.config.params.action, this.config.params.sourceFlag); - // this.sendValidateRequest().then((validationError) => { - // this.validationError = validationError; - // this.cdr.detectChanges(); - // if (!this.validationError) { - // this.sendRequest(this.config.params.action, this.config.params.sourceFlag); - // } - // }); } else { // If the form is invalid, manually mark all form controls as touched CommonFormHelpersService.triggerTouchedToDisplayErrors(this.featureFlagForm); From 1bca6168385c845a4caffe9cb7384f5467d0ffd8 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 20 Sep 2024 09:27:46 +0530 Subject: [PATCH 07/16] Added duplicateKey related changes in the store --- .../feature-flags.data.service.ts | 6 ----- .../feature-flags/feature-flags.service.ts | 2 ++ .../store/feature-flags.actions.ts | 2 ++ .../store/feature-flags.effects.ts | 17 ++++++++++---- .../store/feature-flags.model.ts | 1 + .../store/feature-flags.reducer.ts | 2 ++ .../store/feature-flags.selectors.ts | 2 ++ .../http-error.interceptor.ts | 10 ++++---- .../upsert-feature-flag-modal.component.ts | 23 +++++++++---------- 9 files changed, 38 insertions(+), 27 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts index b4577e2383..bf06dadc19 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts @@ -4,7 +4,6 @@ import { HttpClient, HttpParams } from '@angular/common/http'; import { AddFeatureFlagRequest, FeatureFlag, - FeatureFlagFormData, FeatureFlagSegmentListDetails, FeatureFlagsPaginationInfo, FeatureFlagsPaginationParams, @@ -46,11 +45,6 @@ export class FeatureFlagsDataService { return this.http.put(url, flag); } - validateFeatureFlagForm(flag: FeatureFlagFormData) { - const url = `${this.environment.api.featureFlag}/validation`; - return this.http.post(url, flag); - } - validateFeatureFlag(featureFlag: { files: IFeatureFlagFile[] }) { const url = this.environment.api.validateFeatureFlag; return this.http.post(url, featureFlag); diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index d0fe315758..ccbb27a769 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -26,6 +26,7 @@ import { selectFeatureFlagIds, selectShouldShowWarningForSelectedFlag, selectWarningStatusForAllFlags, + selectDuplicateKeyFound, } from './store/feature-flags.selectors'; import * as FeatureFlagsActions from './store/feature-flags.actions'; import { actionFetchContextMetaData } from '../experiments/store/experiments.actions'; @@ -51,6 +52,7 @@ export class FeatureFlagsService { isLoadingFeatureFlags$ = this.store$.pipe(select(selectIsLoadingFeatureFlags)); isLoadingSelectedFeatureFlag$ = this.store$.pipe(select(selectIsLoadingSelectedFeatureFlag)); isLoadingUpsertFeatureFlag$ = this.store$.pipe(select(selectIsLoadingUpsertFeatureFlag)); + isDuplicateKeyFound$ = this.store$.pipe(select(selectDuplicateKeyFound)); isLoadingFeatureFlagDelete$ = this.store$.pipe(select(selectIsLoadingFeatureFlagDelete)); isLoadingImportFeatureFlag$ = this.store$.pipe(select(selectIsLoadingImportFeatureFlag)); isLoadingUpdateFeatureFlagStatus$ = this.store$.pipe(select(selectIsLoadingUpdateFeatureFlagStatus)); diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts index ce4d86cee1..96f7bc1179 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts @@ -45,6 +45,8 @@ export const actionAddFeatureFlagSuccess = createAction( export const actionAddFeatureFlagFailure = createAction('[Feature Flags] Add Feature Flag Failure'); +export const actionDuplicateKey = createAction('[Feature Flags] Upsert Feature Flag Failure, Duplicate Key found'); + export const actionDeleteFeatureFlag = createAction('[Feature Flags] Delete Feature Flag', props<{ flagId: string }>()); export const actionDeleteFeatureFlagSuccess = createAction( diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 38ad6f96ee..1a794bd498 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -15,12 +15,11 @@ import { selectSortAs, selectSearchString, selectIsAllFlagsFetched, - selectSelectedFeatureFlag, } from './feature-flags.selectors'; 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'; @Injectable() export class FeatureFlagsEffects { constructor( @@ -103,7 +102,12 @@ export class FeatureFlagsEffects { tap(({ response }) => { this.router.navigate(['/featureflags', 'detail', response.id]); }), - catchError(() => [FeatureFlagsActions.actionAddFeatureFlagFailure()]) + catchError((res) => { + if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) { + return [FeatureFlagsActions.actionDuplicateKey()]; + } + return [FeatureFlagsActions.actionAddFeatureFlagFailure()]; + }) ); }) ) @@ -117,7 +121,12 @@ export class FeatureFlagsEffects { map((response) => { return FeatureFlagsActions.actionUpdateFeatureFlagSuccess({ response }); }), - catchError(() => [FeatureFlagsActions.actionUpdateFeatureFlagFailure()]) + catchError((res) => { + if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) { + return [FeatureFlagsActions.actionDuplicateKey()]; + } + return [FeatureFlagsActions.actionAddFeatureFlagFailure()]; + }) ); }) ) diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts index 03de3b2436..88529df35d 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.model.ts @@ -186,6 +186,7 @@ export interface FeatureFlagState extends EntityState { isLoadingFeatureFlagDelete: boolean; isLoadingUpsertPrivateSegmentList: boolean; hasInitialFeatureFlagsDataLoaded: boolean; + duplicateKeyFound: boolean; activeDetailsTabIndex: number; skipFlags: number; totalFlags: number; diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index ec79707993..cc51da626b 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts @@ -20,6 +20,7 @@ export const initialState: FeatureFlagState = adapter.getInitialState({ isLoadingSelectedFeatureFlag: false, isLoadingUpsertPrivateSegmentList: false, hasInitialFeatureFlagsDataLoaded: false, + duplicateKeyFound: false, activeDetailsTabIndex: 0, skipFlags: 0, totalFlags: null, @@ -75,6 +76,7 @@ const reducer = createReducer( ...state, isLoadingUpsertFeatureFlag: false, })), + on(FeatureFlagsActions.actionDuplicateKey, (state) => ({ ...state, duplicateKeyFound: true })), // Feature Flag Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlag, (state) => ({ ...state, isLoadingFeatureFlagDelete: true })), diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts index c5f51fb1bd..770cb9e0ed 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts @@ -47,6 +47,8 @@ export const selectIsLoadingUpsertFeatureFlag = createSelector( (state) => state.isLoadingUpsertFeatureFlag ); +export const selectDuplicateKeyFound = createSelector(selectFeatureFlagsState, (state) => state.duplicateKeyFound); + export const selectSelectedFeatureFlag = createSelector( selectRouterState, selectFeatureFlagsState, diff --git a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts index 34ec3a013b..4b1fe5999c 100755 --- a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts +++ b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts @@ -5,6 +5,7 @@ import { Observable, throwError } from 'rxjs'; import { catchError } from 'rxjs/operators'; import { ENV, Environment } from '../../../environments/environment-types'; import { AuthService } from '../auth/auth.service'; +import { SERVER_ERROR } from 'upgrade_types'; @Injectable() export class HttpErrorInterceptor implements HttpInterceptor { @@ -17,14 +18,13 @@ export class HttpErrorInterceptor implements HttpInterceptor { openPopup(error) { const temp = { type: NotificationType.Error, - title: - error.status === 409 - ? 'Flag with this key already exists for this app-context.' - : 'Network call failed. See console for details.', + title: 'Network call failed. See console for details.', content: error.url, animate: 'fromRight', }; - this._notifications.create(temp.title, temp.content, temp.type, temp); + if (error.error.type !== SERVER_ERROR.DUPLICATE_KEY) { + this._notifications.create(temp.title, temp.content, temp.type, temp); + } } intercept(request: HttpRequest, next: HttpHandler): Observable> { diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 57b2a89df1..ad951c1cfd 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -70,6 +70,7 @@ export class UpsertFeatureFlagModalComponent { isSelectedFeatureFlagUpdated$ = this.featureFlagsService.isSelectedFeatureFlagUpdated$; selectedFlag$ = this.featureFlagsService.selectedFeatureFlag$; appContexts$ = this.featureFlagsService.appContexts$; + isDuplicateKeyFound$ = this.featureFlagsService.isDuplicateKeyFound$; subscriptions = new Subscription(); isInitialFormValueChanged$: Observable; @@ -88,8 +89,6 @@ export class UpsertFeatureFlagModalComponent { private formBuilder: FormBuilder, private featureFlagsService: FeatureFlagsService, private experimentService: ExperimentService, - private formHelpersService: CommonFormHelpersService, - private featureFlagDataService: FeatureFlagsDataService, private cdr: ChangeDetectorRef, public dialogRef: MatDialogRef ) {} @@ -101,6 +100,7 @@ export class UpsertFeatureFlagModalComponent { this.listenOnNameChangesToUpdateKey(); this.listenForIsInitialFormValueChanged(); this.listenForPrimaryButtonDisabled(); + this.listenForDuplicateKey(); } createFeatureFlagForm(): void { @@ -160,6 +160,15 @@ export class UpsertFeatureFlagModalComponent { this.subscriptions.add(this.isSelectedFeatureFlagUpdated$.subscribe(() => this.closeModal())); } + listenForDuplicateKey() { + this.subscriptions.add( + this.isDuplicateKeyFound$.subscribe((isDuplicate) => { + this.validationError = isDuplicate; + this.cdr.detectChanges(); + }) + ); + } + onPrimaryActionBtnClicked() { if (this.featureFlagForm.valid) { this.sendRequest(this.config.params.action, this.config.params.sourceFlag); @@ -169,16 +178,6 @@ export class UpsertFeatureFlagModalComponent { } } - async sendValidateRequest() { - const formData: FeatureFlagFormData = this.featureFlagForm.value; - try { - const result = (await this.featureFlagDataService.validateFeatureFlagForm(formData).toPromise()) as string; - return !!result; - } catch (error) { - return true; - } - } - sendRequest(action: UPSERT_FEATURE_FLAG_ACTION, sourceFlag?: FeatureFlag): void { const formData: FeatureFlagFormData = this.featureFlagForm.value; From db2b029aafb402c13cdea8e1dc639f9d0c2400f2 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 20 Sep 2024 09:57:39 +0530 Subject: [PATCH 08/16] moved validationKey function to repository --- .../api/repositories/FeatureFlagRepository.ts | 18 +++++++++++++++- .../src/api/services/FeatureFlagService.ts | 21 ++----------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts index 73e82d91a1..4e10129886 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts @@ -2,7 +2,8 @@ import { Repository, EntityManager } from 'typeorm'; import { EntityRepository } from '../../typeorm-typedi-extensions'; import { FeatureFlag } from '../models/FeatureFlag'; import repositoryError from './utils/repositoryError'; -import { FEATURE_FLAG_STATUS, FILTER_MODE } from 'upgrade_types'; +import { FEATURE_FLAG_STATUS, FILTER_MODE, SERVER_ERROR } from 'upgrade_types'; +import { FeatureFlagValidation } from '../controllers/validators/FeatureFlagValidator'; @EntityRepository(FeatureFlag) export class FeatureFlagRepository extends Repository { @@ -111,4 +112,19 @@ export class FeatureFlagRepository extends Repository { return result; } + + public async validateUniqueKey(flagDTO: FeatureFlagValidation) { + const result = await this.createQueryBuilder('feature_flag') + .where('feature_flag.key = :key', { key: flagDTO.key }) + .andWhere('feature_flag.context = :context', { context: flagDTO.context }) + .getOne(); + + if (result) { + const error = new Error(`A flag with this key already exists for this app-context`); + (error as any).type = SERVER_ERROR.DUPLICATE_KEY; + (error as any).httpCode = 409; + throw error; + } + return; + } } diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 1b9cbfaf8f..c2233e56ee 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -135,7 +135,7 @@ export class FeatureFlagService { public async create(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: 'Create a new feature flag', details: flagDTO }); - await this.validateUniqueKey(flagDTO, logger); + await this.featureFlagRepository.validateUniqueKey(flagDTO); return this.addFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } @@ -287,7 +287,7 @@ export class FeatureFlagService { public async update(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); - await this.validateUniqueKey(flagDTO, logger); + await this.featureFlagRepository.validateUniqueKey(flagDTO); // TODO add entry in log of updating feature flag return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } @@ -409,23 +409,6 @@ export class FeatureFlagService { return this.segmentService.deleteSegment(segmentId, logger); } - public async validateUniqueKey(flagDTO: FeatureFlagValidation, logger: UpgradeLogger) { - logger.info({ message: `Validating featureFlags` }); - const result = await this.featureFlagRepository - .createQueryBuilder('feature_flag') - .where('feature_flag.key = :key', { key: flagDTO.key }) - .andWhere('feature_flag.context = :context', { context: flagDTO.context }) - .getOne(); - - if (result) { - const error = new Error(`A flag with this key already exists for this app-context`); - (error as any).type = SERVER_ERROR.DUPLICATE_KEY; - (error as any).httpCode = 409; - throw error; - } - return; - } - public async addList( listsInput: FeatureFlagListValidator[], filterType: FEATURE_FLAG_LIST_FILTER_MODE, From a3d14080b87c9b8e0c3dfd319d66b1a402d01dd0 Mon Sep 17 00:00:00 2001 From: Yagnik Date: Fri, 20 Sep 2024 12:16:45 +0530 Subject: [PATCH 09/16] show duplicate key error message and addressed UI bugs related to it --- .../feature-flags/feature-flags.service.ts | 4 +++ .../store/feature-flags.actions.ts | 5 +++- .../store/feature-flags.effects.ts | 4 +-- .../store/feature-flags.reducer.ts | 6 +++- .../upsert-feature-flag-modal.component.html | 28 +++++++++++-------- .../upsert-feature-flag-modal.component.ts | 22 +++++++++++---- .../shared/services/common-dialog.service.ts | 3 +- .../projects/upgrade/src/assets/i18n/en.json | 1 + 8 files changed, 51 insertions(+), 22 deletions(-) diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index ccbb27a769..2db3dec2ce 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -142,6 +142,10 @@ export class FeatureFlagsService { this.store$.dispatch(FeatureFlagsActions.actionUpdateFilterMode({ updateFilterModeRequest })); } + setIsDuplicateKey(duplicateKeyFound: boolean) { + this.store$.dispatch(FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound })); + } + deleteFeatureFlag(flagId: string) { this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlag({ flagId })); } diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts index 96f7bc1179..ab00140bbb 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts @@ -45,7 +45,10 @@ export const actionAddFeatureFlagSuccess = createAction( export const actionAddFeatureFlagFailure = createAction('[Feature Flags] Add Feature Flag Failure'); -export const actionDuplicateKey = createAction('[Feature Flags] Upsert Feature Flag Failure, Duplicate Key found'); +export const actionSetIsDuplicateKey = createAction( + '[Feature Flags] Upsert Feature Flag Failure, Duplicate Key found', + props<{ duplicateKeyFound: boolean }>() +); export const actionDeleteFeatureFlag = createAction('[Feature Flags] Delete Feature Flag', props<{ flagId: string }>()); diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 1a794bd498..a0894d9ee8 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -104,7 +104,7 @@ export class FeatureFlagsEffects { }), catchError((res) => { if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) { - return [FeatureFlagsActions.actionDuplicateKey()]; + return [FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound: true })]; } return [FeatureFlagsActions.actionAddFeatureFlagFailure()]; }) @@ -123,7 +123,7 @@ export class FeatureFlagsEffects { }), catchError((res) => { if (res.error.type == SERVER_ERROR.DUPLICATE_KEY) { - return [FeatureFlagsActions.actionDuplicateKey()]; + return [FeatureFlagsActions.actionSetIsDuplicateKey({ duplicateKeyFound: true })]; } return [FeatureFlagsActions.actionAddFeatureFlagFailure()]; }) diff --git a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index cc51da626b..570258b1c0 100644 --- a/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts +++ b/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts @@ -76,7 +76,11 @@ const reducer = createReducer( ...state, isLoadingUpsertFeatureFlag: false, })), - on(FeatureFlagsActions.actionDuplicateKey, (state) => ({ ...state, duplicateKeyFound: true })), + on(FeatureFlagsActions.actionSetIsDuplicateKey, (state, { duplicateKeyFound }) => ({ + ...state, + duplicateKeyFound, + isLoadingUpsertFeatureFlag: false, + })), // Feature Flag Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlag, (state) => ({ ...state, isLoadingFeatureFlagDelete: true })), diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html index dc0e539670..f1a94431a7 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.html @@ -5,29 +5,31 @@ [primaryActionBtnColor]="config.primaryActionBtnColor" [primaryActionBtnDisabled]="isPrimaryButtonDisabled$ | async" (primaryActionBtnClicked)="onPrimaryActionBtnClicked()" - > +>
Name - + - {{'feature-flags.upsert-flag-modal.name-hint.text' | translate}} + {{ 'feature-flags.upsert-flag-modal.name-hint.text' | translate }} Key - + - {{'feature-flags.upsert-flag-modal.key-hint.text' | translate}} - Learn more + {{ 'feature-flags.upsert-flag-modal.key-hint.text' | translate }} + Learn more - {{ 'feature-flags.duplicate-key-error-text' | translate }} + {{ 'feature-flags.upsert-flag-modal.duplicate-key-error.text' | translate }} @@ -35,17 +37,21 @@ Description (optional) - + App Context - {{ context }} + {{ + context + }} - {{ 'feature-flags.upsert-flag-modal.app-context-hint.text' | translate}} - Learn more + {{ 'feature-flags.upsert-flag-modal.app-context-hint.text' | translate }} + Learn more diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index ad951c1cfd..6d6099546c 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject } from '@angular/core'; +import { ChangeDetectionStrategy, Component, Inject } from '@angular/core'; import { CommonModalComponent, CommonTagsInputComponent, @@ -36,7 +36,6 @@ import { BehaviorSubject, combineLatestWith, map, Observable, startWith, Subscri import { TranslateModule } from '@ngx-translate/core'; import { ExperimentService } from '../../../../../core/experiments/experiments.service'; import { CommonTextHelpersService } from '../../../../../shared/services/common-text-helpers.service'; -import { FeatureFlagsDataService } from '../../../../../core/feature-flags/feature-flags.data.service'; import isEqual from 'lodash.isequal'; import { CommonModalConfig } from '../../../../../shared-standalone-component-lib/components/common-modal/common-modal.types'; @@ -89,13 +88,14 @@ export class UpsertFeatureFlagModalComponent { private formBuilder: FormBuilder, private featureFlagsService: FeatureFlagsService, private experimentService: ExperimentService, - private cdr: ChangeDetectorRef, public dialogRef: MatDialogRef ) {} ngOnInit(): void { + this.featureFlagsService.setIsDuplicateKey(false); this.experimentService.fetchContextMetaData(); this.createFeatureFlagForm(); + this.listenOnKeyChangesToRemoveWarning(); this.listenForFeatureFlagGetUpdated(); this.listenOnNameChangesToUpdateKey(); this.listenForIsInitialFormValueChanged(); @@ -139,6 +139,15 @@ export class UpsertFeatureFlagModalComponent { ); } + listenOnKeyChangesToRemoveWarning(): void { + this.subscriptions.add( + this.featureFlagForm.get('key')?.valueChanges.subscribe(() => { + this.validationError = this.validationError ? false : this.validationError; + this.featureFlagsService.setIsDuplicateKey(false); + }) + ); + } + listenForIsInitialFormValueChanged() { this.isInitialFormValueChanged$ = this.featureFlagForm.valueChanges.pipe( startWith(this.featureFlagForm.value), @@ -157,14 +166,17 @@ export class UpsertFeatureFlagModalComponent { // Close the modal once the feature flag list length changes, as that indicates actual success listenForFeatureFlagGetUpdated(): void { - this.subscriptions.add(this.isSelectedFeatureFlagUpdated$.subscribe(() => this.closeModal())); + this.subscriptions.add( + this.isSelectedFeatureFlagUpdated$.subscribe(() => { + this.closeModal(); + }) + ); } listenForDuplicateKey() { this.subscriptions.add( this.isDuplicateKeyFound$.subscribe((isDuplicate) => { this.validationError = isDuplicate; - this.cdr.detectChanges(); }) ); } diff --git a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts index 51eff86d72..11a9c5fa0f 100644 --- a/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts +++ b/frontend/projects/upgrade/src/app/shared/services/common-dialog.service.ts @@ -143,8 +143,7 @@ export class DialogService { cancelBtnLabel: 'Cancel', params: { message: 'Are you sure you want to disable "Include All"?', - subMessage: - '* Disabling this will revert to the previously defined include lists, if any.', + subMessage: '* Disabling this will revert to the previously defined include lists, if any.', subMessageClass: 'warn', }, }; diff --git a/frontend/projects/upgrade/src/assets/i18n/en.json b/frontend/projects/upgrade/src/assets/i18n/en.json index aecdcc301e..2f5e47cdc5 100644 --- a/frontend/projects/upgrade/src/assets/i18n/en.json +++ b/frontend/projects/upgrade/src/assets/i18n/en.json @@ -373,6 +373,7 @@ "feature-flags.import-flag-modal.compatibility-description.warning.text": "This JSON file can be imported, but it may contain outdated or missing properties/features. Please review the feature flag details post-import.", "feature-flags.upsert-flag-modal.name-hint.text": "The name for this feature flag.", "feature-flags.upsert-flag-modal.key-hint.text": "A unique key used to retrieve this feature flag from the client application.", + "feature-flags.upsert-flag-modal.duplicate-key-error.text": "Feature flag with this key already exists for this app-context.", "feature-flags.upsert-flag-modal.app-context-hint.text": "The App Context indicates where the feature flag will run, known to UpGrade.", "feature-flags.upsert-flag-modal.tags-label.text": "Tags (optional)", "feature-flags.upsert-flag-modal.tags-placeholder.text": "Tags separated by commas", From f0f19b4a5dd179ece3e3dabc87306ef402d017d1 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Fri, 20 Sep 2024 18:56:44 +0530 Subject: [PATCH 10/16] solve failing testcases --- .../Upgrade/test/unit/services/FeatureFlagService.test.ts | 2 +- .../src/app/core/http-interceptors/http-error.interceptor.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts index b361bd8d55..f3d6fb2bfe 100644 --- a/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts +++ b/backend/packages/Upgrade/test/unit/services/FeatureFlagService.test.ts @@ -202,6 +202,7 @@ describe('Feature Flag Service Testing', () => { getMany: jest.fn().mockResolvedValue(mockFlagArr), getOne: jest.fn().mockResolvedValue(mockFlag1), })), + validateUniqueKey: jest.fn().mockResolvedValue(null), }, }, { @@ -242,7 +243,6 @@ describe('Feature Flag Service Testing', () => { .compile(); service = module.get(FeatureFlagService); - service.validateUniqueKey = jest.fn().mockResolvedValue(null); flagRepo = module.get(getRepositoryToken(FeatureFlagRepository)); }); diff --git a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts index 4b1fe5999c..0b8a0c16e4 100755 --- a/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts +++ b/frontend/projects/upgrade/src/app/core/http-interceptors/http-error.interceptor.ts @@ -22,7 +22,7 @@ export class HttpErrorInterceptor implements HttpInterceptor { content: error.url, animate: 'fromRight', }; - if (error.error.type !== SERVER_ERROR.DUPLICATE_KEY) { + if (error.error?.type !== SERVER_ERROR.DUPLICATE_KEY) { this._notifications.create(temp.title, temp.content, temp.type, temp); } } From 5a71ea8ff7c21f27f113a6979c04a7a2926c70b7 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 23 Sep 2024 10:36:10 +0530 Subject: [PATCH 11/16] moved error throwing code from repo to service --- .../api/repositories/FeatureFlagRepository.ts | 10 ++-------- .../src/api/services/FeatureFlagService.ts | 19 +++++++++++++++++-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts index 4e10129886..0c41b52a19 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts @@ -2,7 +2,7 @@ import { Repository, EntityManager } from 'typeorm'; import { EntityRepository } from '../../typeorm-typedi-extensions'; import { FeatureFlag } from '../models/FeatureFlag'; import repositoryError from './utils/repositoryError'; -import { FEATURE_FLAG_STATUS, FILTER_MODE, SERVER_ERROR } from 'upgrade_types'; +import { FEATURE_FLAG_STATUS, FILTER_MODE } from 'upgrade_types'; import { FeatureFlagValidation } from '../controllers/validators/FeatureFlagValidator'; @EntityRepository(FeatureFlag) @@ -119,12 +119,6 @@ export class FeatureFlagRepository extends Repository { .andWhere('feature_flag.context = :context', { context: flagDTO.context }) .getOne(); - if (result) { - const error = new Error(`A flag with this key already exists for this app-context`); - (error as any).type = SERVER_ERROR.DUPLICATE_KEY; - (error as any).httpCode = 409; - throw error; - } - return; + return result; } } diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index bb130f0746..9869d42276 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -135,7 +135,15 @@ export class FeatureFlagService { public async create(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: 'Create a new feature flag', details: flagDTO }); - await this.featureFlagRepository.validateUniqueKey(flagDTO); + const result = await this.featureFlagRepository.validateUniqueKey(flagDTO); + + if (result) { + const error = new Error(`A flag with this key already exists for this app-context`); + (error as any).type = SERVER_ERROR.DUPLICATE_KEY; + (error as any).httpCode = 409; + throw error; + } + return this.addFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } @@ -287,7 +295,14 @@ export class FeatureFlagService { public async update(flagDTO: FeatureFlagValidation, currentUser: User, logger: UpgradeLogger): Promise { logger.info({ message: `Update a Feature Flag => ${flagDTO.toString()}` }); - await this.featureFlagRepository.validateUniqueKey(flagDTO); + const result = await this.featureFlagRepository.validateUniqueKey(flagDTO); + + if (result) { + const error = new Error(`A flag with this key already exists for this app-context`); + (error as any).type = SERVER_ERROR.DUPLICATE_KEY; + (error as any).httpCode = 409; + throw error; + } // TODO add entry in log of updating feature flag return this.updateFeatureFlagInDB(this.featureFlagValidatorToFlag(flagDTO), currentUser, logger); } From 77b157a47245f68dfa435daf358e29bcdd432821 Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Mon, 23 Sep 2024 14:30:31 +0530 Subject: [PATCH 12/16] update import for key & context unique pair --- .../packages/Upgrade/src/api/services/FeatureFlagService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts index 9869d42276..a8ab2a2a70 100644 --- a/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts +++ b/backend/packages/Upgrade/src/api/services/FeatureFlagService.ts @@ -908,7 +908,9 @@ export class FeatureFlagService { } if (compatibilityType === FF_COMPATIBILITY_TYPE.COMPATIBLE) { - const keyExists = existingFeatureFlags.find((existingFlag) => existingFlag.key === flag.key); + const keyExists = existingFeatureFlags?.find( + (existingFlag) => existingFlag.key === flag.key && existingFlag.context === flag.context + ); if (keyExists) { compatibilityType = FF_COMPATIBILITY_TYPE.INCOMPATIBLE; From 71b050041c4743e937785aae17fbde7c4e6b1e6a Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Tue, 24 Sep 2024 18:45:21 +0530 Subject: [PATCH 13/16] Key modal outline turns red on duplicate error --- .../upsert-feature-flag-modal.component.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 6d6099546c..1c737c7c3a 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -144,6 +144,7 @@ export class UpsertFeatureFlagModalComponent { this.featureFlagForm.get('key')?.valueChanges.subscribe(() => { this.validationError = this.validationError ? false : this.validationError; this.featureFlagsService.setIsDuplicateKey(false); + this.featureFlagForm.get('key').setErrors(null); }) ); } @@ -177,6 +178,7 @@ export class UpsertFeatureFlagModalComponent { this.subscriptions.add( this.isDuplicateKeyFound$.subscribe((isDuplicate) => { this.validationError = isDuplicate; + this.featureFlagForm.get('key').setErrors({ duplicateKey: isDuplicate }); }) ); } From dc9a823935f265afc2e98675c7bd562d73383c3c Mon Sep 17 00:00:00 2001 From: RidhamShah Date: Wed, 25 Sep 2024 18:47:47 +0530 Subject: [PATCH 14/16] fix red box not showing around key --- .../upsert-feature-flag-modal.component.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 1c737c7c3a..71dd3667a3 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -141,10 +141,10 @@ export class UpsertFeatureFlagModalComponent { listenOnKeyChangesToRemoveWarning(): void { this.subscriptions.add( - this.featureFlagForm.get('key')?.valueChanges.subscribe(() => { + this.featureFlagForm.get('key')?.valueChanges.subscribe((key) => { this.validationError = this.validationError ? false : this.validationError; this.featureFlagsService.setIsDuplicateKey(false); - this.featureFlagForm.get('key').setErrors(null); + key ? this.featureFlagForm.get('key').setErrors(null) : null; }) ); } @@ -179,6 +179,7 @@ export class UpsertFeatureFlagModalComponent { this.isDuplicateKeyFound$.subscribe((isDuplicate) => { this.validationError = isDuplicate; this.featureFlagForm.get('key').setErrors({ duplicateKey: isDuplicate }); + isDuplicate ? this.featureFlagForm.get('key').markAllAsTouched() : null; }) ); } From 17e4f901134a8d3c48447d6af08315ae3600b656 Mon Sep 17 00:00:00 2001 From: Yagnik Date: Thu, 26 Sep 2024 11:25:54 +0530 Subject: [PATCH 15/16] addressed review comment about not updating ff with same key --- .../Upgrade/src/api/repositories/FeatureFlagRepository.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts index 0c41b52a19..f335945fcf 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts @@ -117,6 +117,7 @@ export class FeatureFlagRepository extends Repository { const result = await this.createQueryBuilder('feature_flag') .where('feature_flag.key = :key', { key: flagDTO.key }) .andWhere('feature_flag.context = :context', { context: flagDTO.context }) + .andWhere('feature_flag.id != :id', { id: flagDTO.id }) .getOne(); return result; From 8924eeabd365752080d8d1b041b0cd0f21a74803 Mon Sep 17 00:00:00 2001 From: Yagnik Date: Thu, 26 Sep 2024 15:21:08 +0530 Subject: [PATCH 16/16] addressed bug not allowing to create featureFlag --- .../src/api/repositories/FeatureFlagRepository.ts | 11 +++++++---- .../upsert-feature-flag-modal.component.ts | 9 +++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts index f335945fcf..0f6da24de1 100644 --- a/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts +++ b/backend/packages/Upgrade/src/api/repositories/FeatureFlagRepository.ts @@ -114,12 +114,15 @@ export class FeatureFlagRepository extends Repository { } public async validateUniqueKey(flagDTO: FeatureFlagValidation) { - const result = await this.createQueryBuilder('feature_flag') + const queryBuilder = this.createQueryBuilder('feature_flag') .where('feature_flag.key = :key', { key: flagDTO.key }) - .andWhere('feature_flag.context = :context', { context: flagDTO.context }) - .andWhere('feature_flag.id != :id', { id: flagDTO.id }) - .getOne(); + .andWhere('feature_flag.context = :context', { context: flagDTO.context }); + + if (flagDTO.id) { + queryBuilder.andWhere('feature_flag.id != :id', { id: flagDTO.id }); + } + const result = await queryBuilder.getOne(); return result; } } diff --git a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts index 71dd3667a3..a1a2c09399 100644 --- a/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts +++ b/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/modals/upsert-feature-flag-modal/upsert-feature-flag-modal.component.ts @@ -101,6 +101,7 @@ export class UpsertFeatureFlagModalComponent { this.listenForIsInitialFormValueChanged(); this.listenForPrimaryButtonDisabled(); this.listenForDuplicateKey(); + this.listenOnContext(); } createFeatureFlagForm(): void { @@ -149,6 +150,14 @@ export class UpsertFeatureFlagModalComponent { ); } + listenOnContext(): void { + this.subscriptions.add( + this.featureFlagForm.get('appContext')?.valueChanges.subscribe(() => { + this.featureFlagForm.get('key') ? this.featureFlagForm.get('key').setErrors(null) : null; + }) + ); + } + listenForIsInitialFormValueChanged() { this.isInitialFormValueChanged$ = this.featureFlagForm.valueChanges.pipe( startWith(this.featureFlagForm.value),