From 8ffc92af5ba410431ce3040b7389a5ddc52aab35 Mon Sep 17 00:00:00 2001 From: andreas-unleash Date: Wed, 22 Nov 2023 09:50:03 +0200 Subject: [PATCH] fix: Only show strategy variant changes if there is a diff in the variants (#5353) What it says on the box Closes # [1-1652](https://linear.app/unleash/issue/1-1652/remove-the-variants-from-change-request-page-when-not-modified) ![Screenshot 2023-11-16 at 11 26 05](https://github.com/Unleash/unleash/assets/104830839/8f25b82c-4dbc-46fb-bdd6-0e0049659c72) ![Screenshot 2023-11-16 at 11 25 46](https://github.com/Unleash/unleash/assets/104830839/e6366622-3a50-4a0e-bba2-6c1d34e64077) --------- Signed-off-by: andreas-unleash --- .../ChangeRequest/ChangeRequest.test.tsx | 199 +++++++++++++++++- .../Changes/Change/StrategyChange.tsx | 45 ++-- .../Change/hooks/useCurrentStrategy.ts | 4 +- 3 files changed, 227 insertions(+), 21 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.test.tsx index 8f1e058b27..78b1c16224 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.test.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.test.tsx @@ -1,6 +1,6 @@ import { screen } from '@testing-library/react'; -import { Routes, Route } from 'react-router-dom'; +import { Route, Routes } from 'react-router-dom'; import { render } from 'utils/testRenderer'; import { ChangeRequest } from './ChangeRequest'; @@ -9,6 +9,91 @@ import { IChangeRequestAddStrategy, IChangeRequestEnabled, } from '../changeRequest.types'; +import { testServerRoute, testServerSetup } from 'utils/testServer'; +import { StrategyVariantSchema } from 'openapi'; + +const server = testServerSetup(); +const uiConfigForEnterprise = () => + testServerRoute(server, '/api/admin/ui-config', { + versionInfo: { + current: { oss: 'version', enterprise: 'version' }, + }, + flags: { + scheduledConfigurationChanges: true, + }, + }); + +const featureWithStrategyVariants = () => + testServerRoute(server, `/api/admin/projects/default/features/feature1`, { + name: 'feature1', + impressionData: false, + description: '', + project: 'default', + stale: false, + variants: [], + createdAt: '2022-11-14T08:16:33.338Z', + lastSeenAt: null, + type: 'release', + archived: false, + children: [], + dependencies: [], + environments: [ + { + name: 'development', + enabled: false, + type: 'production', + sortOrder: 1, + strategies: [ + { + id: '2e4f0555-518b-45b3-b0cd-a32cca388a92', + variants: [ + { + name: 'variant1', + stickiness: 'default', + weight: 500, + weightType: 'fix', + }, + { + name: 'variant2', + stickiness: 'default', + weight: 500, + weightType: 'fix', + }, + ], + }, + ], + }, + ], + }); + +const feature = () => + testServerRoute(server, `/api/admin/projects/default/features/feature1`, { + name: 'feature1', + impressionData: false, + description: '', + project: 'default', + stale: false, + variants: [], + createdAt: '2022-11-14T08:16:33.338Z', + lastSeenAt: null, + type: 'release', + archived: false, + children: [], + dependencies: [], + environments: [ + { + name: 'development', + enabled: false, + type: 'production', + sortOrder: 1, + strategies: [ + { + id: '2e4f0555-518b-45b3-b0cd-a32cca388a92', + }, + ], + }, + ], + }); const changeRequestWithDefaultChange = ( defaultChange: IChangeRequestEnabled | IChangeRequestAddStrategy, @@ -26,7 +111,7 @@ const changeRequestWithDefaultChange = ( segments: [], features: [ { - name: 'Feature Toggle Name', + name: 'feature1', changes: [ { id: 67, @@ -50,7 +135,51 @@ const changeRequestWithDefaultChange = ( minApprovals: 1, state: 'Draft', title: 'My change request', - project: 'project', + project: 'default', + environment: 'production', + }; + return changeRequest; +}; + +const changeRequest = (variants: StrategyVariantSchema[]) => { + const changeRequest: IChangeRequest = { + approvals: [], + rejections: [], + comments: [], + createdAt: new Date(), + createdBy: { + id: 1, + username: 'author', + imageUrl: '', + }, + segments: [], + features: [ + { + name: 'feature1', + changes: [ + { + id: 0, + action: 'updateStrategy', + payload: { + id: '2e4f0555-518b-45b3-b0cd-a32cca388a92', + name: 'flexibleRollout', + constraints: [], + parameters: { + rollout: '100', + stickiness: 'default', + groupId: 'test123', + }, + variants: variants, + }, + }, + ], + }, + ], + id: 27, + minApprovals: 1, + state: 'Draft', + title: 'My change request', + project: 'default', environment: 'production', }; return changeRequest; @@ -75,7 +204,7 @@ test('Display default add strategy', async () => { />, ); - expect(screen.getByText('Feature Toggle Name')).toBeInTheDocument(); + expect(screen.getByText('feature1')).toBeInTheDocument(); expect(screen.getByText('Enabled')).toBeInTheDocument(); expect( screen.getByText('Default strategy will be added'), @@ -95,12 +224,12 @@ test('Display default disable feature', async () => { />, ); - expect(screen.getByText('Feature Toggle Name')).toBeInTheDocument(); + expect(screen.getByText('feature1')).toBeInTheDocument(); expect(screen.getByText('Disabled')).toBeInTheDocument(); expect(screen.getByText('Feature status will change')).toBeInTheDocument(); }); -test('Displays feature strategy variants table', async () => { +test('Displays feature strategy variants table when addStrategy action with variants', async () => { render( { /> , { - route: 'projects/default/features/colors/strategies/edit?environmentId=development&strategyId=2e4f0555-518b-45b3-b0cd-a32cca388a92', + route: '/projects/default/features/feature1/strategies/edit', }, ); @@ -147,3 +276,59 @@ test('Displays feature strategy variants table', async () => { screen.getByText('Updating feature variants to:'), ).toBeInTheDocument(); }); + +test('Displays feature strategy variants table when there is a change in the variants array', async () => { + uiConfigForEnterprise(); + featureWithStrategyVariants(); + render( + + + } + /> + , + { + route: '/projects/default/change-requests/27', + }, + ); + await screen.findByText('Updating feature variants to:'); +}); + +test('Displays feature strategy variants table when existing strategy does not have variants and change does', async () => { + uiConfigForEnterprise(); + feature(); + render( + + + } + /> + , + { + route: '/projects/default/change-requests/27', + }, + ); + await screen.findByText('Updating feature variants to:'); +}); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index a031607009..fb314931b2 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -127,20 +127,15 @@ export const StrategyChange: VFC<{ environmentName, ); + const hasDiff = (object: unknown, objectToCompare: unknown) => + JSON.stringify(object) !== JSON.stringify(objectToCompare); + const isStrategyAction = change.action === 'addStrategy' || change.action === 'updateStrategy'; - const featureStrategyVariantsDisplay = + const hasVariantDiff = isStrategyAction && - change.payload.variants && - change.payload.variants.length > 0 ? ( - - - Updating feature variants to: - - - - ) : null; + hasDiff(currentStrategy?.variants || [], change.payload.variants || []); return ( <> @@ -173,7 +168,21 @@ export const StrategyChange: VFC<{
{actions}
- {featureStrategyVariantsDisplay} + + + Updating feature variants to: + + + + ) + } + /> )} {change.action === 'deleteStrategy' && ( @@ -240,7 +249,19 @@ export const StrategyChange: VFC<{ } /> - {featureStrategyVariantsDisplay} + + + Updating feature variants to: + + + + } + /> )} diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/hooks/useCurrentStrategy.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/hooks/useCurrentStrategy.ts index 2addd0eaf5..be90227f4a 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/hooks/useCurrentStrategy.ts +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/hooks/useCurrentStrategy.ts @@ -14,8 +14,8 @@ export const useCurrentStrategy = ( feature: string, environmentName: string, ) => { - const currentFeature = useFeature(project, feature); - const currentStrategy = currentFeature.feature?.environments + const { feature: currentFeature } = useFeature(project, feature); + const currentStrategy = currentFeature?.environments .find((environment) => environment.name === environmentName) ?.strategies.find( (strategy) =>