From dc08f1dadd7e81db938bdb656879c48eab86ef0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Thu, 17 Nov 2022 14:05:57 +0100 Subject: [PATCH] fix: broken UI after import (#2447) fix: broken UI when importing features into environments which are not linked to the feature's project ## Related to - PR: https://github.com/Unleash/unleash/pull/2209 - Issue: https://github.com/Unleash/unleash/issues/2186 - Issue: https://github.com/Unleash/unleash/issues/2193 ## Expected behaviour: After importing we should see: ![image](https://user-images.githubusercontent.com/455064/202149719-fa74b3b7-3936-443b-9d0e-8f1ca2e779f4.png) ## About the changes **The problem:** when we import we have projects, features and environments. Each feature belongs to a project (this is by default and the imported file enforces that). The links between projects and features, or projects and environments, depend on us creating those relationships. When we add a feature to an environment we're not validating that the project and the environment are connected. Because of that, in some situations (like in this test), we can end up with a project with features but no environment. This breaks a weak constraint we had which is that all projects should have at least one environment. **This PR makes the following assumption when importing**: _if a feature is added to an environment, and that environment is still not linked to the project that feature belongs to, then the project and environments have to be linked_. The rationale behind this is that the user couldn't have generated this export file without the project and environment being linked together. --- src/lib/db/feature-environment-store.ts | 11 ++++- src/lib/services/state-service.ts | 49 +++++++++++++++++-- .../types/stores/feature-environment-store.ts | 7 ++- src/test/e2e/api/admin/state.e2e.test.ts | 14 +++--- src/test/examples/import-state.json | 16 +++--- .../fake-feature-environment-store.ts | 2 +- 6 files changed, 77 insertions(+), 22 deletions(-) diff --git a/src/lib/db/feature-environment-store.ts b/src/lib/db/feature-environment-store.ts index 99333b7547..9450dde876 100644 --- a/src/lib/db/feature-environment-store.ts +++ b/src/lib/db/feature-environment-store.ts @@ -217,11 +217,17 @@ export class FeatureEnvironmentStore implements IFeatureEnvironmentStore { async connectProject( environment: string, projectId: string, + idempotent?: boolean, // default false to respect old behavior ): Promise { - await this.db('project_environments').insert({ + const query = this.db('project_environments').insert({ environment_name: environment, project_id: projectId, }); + if (idempotent) { + await query.onConflict(['environment_name', 'project_id']).ignore(); + } else { + await query; + } } async connectFeatures( @@ -258,6 +264,7 @@ export class FeatureEnvironmentStore implements IFeatureEnvironmentStore { async connectFeatureToEnvironmentsForProject( featureName: string, projectId: string, + enabledIn: { [environment: string]: boolean } = {}, ): Promise { const environmentsToEnable = await this.db('project_environments') .select('environment_name') @@ -268,7 +275,7 @@ export class FeatureEnvironmentStore implements IFeatureEnvironmentStore { .insert({ environment: env.environment_name, feature_name: featureName, - enabled: false, + enabled: enabledIn[env.environment_name] || false, }) .onConflict(['environment', 'feature_name']) .ignore(); diff --git a/src/lib/services/state-service.ts b/src/lib/services/state-service.ts index 4737c549f7..d4e56f4ca4 100644 --- a/src/lib/services/state-service.ts +++ b/src/lib/services/state-service.ts @@ -200,9 +200,19 @@ export default class StateService { dropBeforeImport, keepExisting, }); - await this.importFeatureEnvironments({ - featureEnvironments, - }); + + if (featureEnvironments) { + // make sure the project and environment are connected + // before importing featureEnvironments + await this.linkFeatureEnvironments({ + features, + featureEnvironments, + }); + await this.importFeatureEnvironments({ + featureEnvironments, + }); + } + await this.importFeatureStrategies({ featureStrategies, dropBeforeImport, @@ -256,6 +266,35 @@ export default class StateService { } } + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types + async linkFeatureEnvironments({ + features, + featureEnvironments, + }): Promise { + const linkTasks = featureEnvironments.map(async (fe) => { + const project = features.find( + (f) => f.project && f.name === fe.featureName, + ).project; + if (project) { + return this.featureEnvironmentStore.connectProject( + fe.environment, + project, + true, // make it idempotent + ); + } + }); + await Promise.all(linkTasks); + } + + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types + enabledInConfiguration(feature: string, env) { + const config = {}; + env.filter((e) => e.featureName === feature).forEach((e) => { + config[e.environment] = e.enabled || false; + }); + return config; + } + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types async importFeatureEnvironments({ featureEnvironments }): Promise { await Promise.all( @@ -266,6 +305,10 @@ export default class StateService { this.featureEnvironmentStore.connectFeatureToEnvironmentsForProject( env.featureName, id, + this.enabledInConfiguration( + env.featureName, + featureEnvironments, + ), ), ), ), diff --git a/src/lib/types/stores/feature-environment-store.ts b/src/lib/types/stores/feature-environment-store.ts index 0df15eede4..cdfc2c0f89 100644 --- a/src/lib/types/stores/feature-environment-store.ts +++ b/src/lib/types/stores/feature-environment-store.ts @@ -44,9 +44,14 @@ export interface IFeatureEnvironmentStore connectFeatureToEnvironmentsForProject( featureName: string, projectId: string, + enabledIn?: { [environment: string]: boolean }, ): Promise; - connectProject(environment: string, projectId: string): Promise; + connectProject( + environment: string, + projectId: string, + idempotent?: boolean, + ): Promise; disconnectProject(environment: string, projectId: string): Promise; copyEnvironmentFeaturesByProjects( sourceEnvironment: string, diff --git a/src/test/e2e/api/admin/state.e2e.test.ts b/src/test/e2e/api/admin/state.e2e.test.ts index e743d47dfe..b0e31d794c 100644 --- a/src/test/e2e/api/admin/state.e2e.test.ts +++ b/src/test/e2e/api/admin/state.e2e.test.ts @@ -452,15 +452,15 @@ test(`should not show environment on feature toggle, when environment is disable .attach('file', 'src/test/examples/import-state.json') .expect(202); - await app.request - .post('/api/admin/projects/default/environments') - .send({ environment: 'state-visible-environment' }) - .expect(200); - const { body } = await app.request .get('/api/admin/projects/default/features/my-feature') .expect(200); - expect(body.environments).toHaveLength(1); - expect(body.environments[0].name).toBe('state-visible-environment'); + // sort to have predictable test results + const result = body.environments.sort((e1, e2) => e1.name < e2.name); + expect(result).toHaveLength(2); + expect(result[0].name).toBe('development'); + expect(result[0].enabled).toBeTruthy(); + expect(result[1].name).toBe('production'); + expect(result[1].enabled).toBeFalsy(); }); diff --git a/src/test/examples/import-state.json b/src/test/examples/import-state.json index 0360445bc5..1afa9f3394 100644 --- a/src/test/examples/import-state.json +++ b/src/test/examples/import-state.json @@ -17,7 +17,7 @@ "id": "2ea91298-4565-4db2-8a23-50757001a076", "featureName": "my-feature", "projectId": "default", - "environment": "state-visible-environment", + "environment": "development", "strategyName": "gradualRolloutRandom", "parameters": { "percentage": "100" @@ -28,26 +28,26 @@ ], "environments": [ { - "name": "state-visible-environment", - "type": "production", - "displayName": "Visible" + "name": "development", + "type": "development", + "displayName": "Dev" }, { - "name": "state-hidden-environment", + "name": "production", "type": "production", - "displayName": "Hidden" + "displayName": "Prod" } ], "featureEnvironments": [ { "enabled": true, "featureName": "my-feature", - "environment": "state-visible-environment" + "environment": "development" }, { "enabled": false, "featureName": "my-feature", - "environment": "state-hidden-environment" + "environment": "production" } ] } diff --git a/src/test/fixtures/fake-feature-environment-store.ts b/src/test/fixtures/fake-feature-environment-store.ts index a0592e7988..9b8be77f30 100644 --- a/src/test/fixtures/fake-feature-environment-store.ts +++ b/src/test/fixtures/fake-feature-environment-store.ts @@ -123,7 +123,7 @@ export default class FakeFeatureEnvironmentStore // eslint-disable-next-line @typescript-eslint/no-unused-vars projectId: string, ): Promise { - return Promise.reject(new Error('Not implemented')); + return Promise.resolve(undefined); } async connectFeatures(