From 1ec783d3c0b3b9a966ce52df7bf7005a2b923910 Mon Sep 17 00:00:00 2001 From: kwasniew Date: Mon, 25 Nov 2024 15:58:42 +0100 Subject: [PATCH 1/6] feat: view diff in change requests --- .../StrategyTooltipLink/StrategyTooltipLink.tsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx index d4aab1a497f2..f05c393e862b 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx @@ -69,6 +69,11 @@ const StyledContainer: FC<{ children?: React.ReactNode }> = styled('div')( }), ); +const StyledViewDiff = styled('span')(({ theme }) => ({ + color: theme.palette.primary.main, + marginLeft: theme.spacing(1), +})); + const Truncated = styled('div')(() => ({ ...textTruncated, maxWidth: 500, @@ -82,6 +87,9 @@ export const StrategyTooltipLink: FC = ({ + + {formatStrategyName(change.payload.name)} + = ({ maxHeight: 600, }} > - - {formatStrategyName(change.payload.name)} - + View Diff Date: Mon, 25 Nov 2024 16:00:42 +0100 Subject: [PATCH 2/6] feat: view diff in change requests --- .../ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx index f05c393e862b..7295dcf614e2 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx @@ -69,7 +69,7 @@ const StyledContainer: FC<{ children?: React.ReactNode }> = styled('div')( }), ); -const StyledViewDiff = styled('span')(({ theme }) => ({ +const ViewDiff = styled('span')(({ theme }) => ({ color: theme.palette.primary.main, marginLeft: theme.spacing(1), })); @@ -97,7 +97,7 @@ export const StrategyTooltipLink: FC = ({ maxHeight: 600, }} > - View Diff + View Diff Date: Tue, 26 Nov 2024 10:56:07 +0100 Subject: [PATCH 3/6] feat: change request applied diff --- .../Changes/Change/StrategyChange.test.tsx | 130 ++++++++++++++++++ .../Changes/Change/StrategyChange.tsx | 12 +- .../changeRequest/changeRequest.types.ts | 2 +- 3 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx new file mode 100644 index 000000000000..0cda657f5574 --- /dev/null +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx @@ -0,0 +1,130 @@ +import { render } from 'utils/testRenderer'; +import { StrategyChange } from './StrategyChange'; +import { testServerRoute, testServerSetup } from 'utils/testServer'; +import { screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +const server = testServerSetup(); + +const feature = 'my_feature'; +const projectId = 'default'; +const environmentName = 'production'; +const snapshotRollout = '70'; +const currentRollout = '80'; +const changeRequestRollout = '90'; +const strategy = { + name: 'flexibleRollout', + constraints: [], + variants: [], + parameters: { + groupId: 'child_1', + stickiness: 'default', + }, + segments: [], + id: '8e25e369-6424-4dad-b17f-5d32cceb2fbe', + disabled: false, +}; + +testServerRoute( + server, + `/api/admin/projects/${projectId}/features/${feature}`, + { + environments: [ + { + name: environmentName, + strategies: [ + { + ...strategy, + title: 'current_title', + parameters: { + ...strategy.parameters, + rollout: currentRollout, + }, + }, + ], + }, + ], + }, +); + +test('Editing strategy before change request is applied', async () => { + render( + , + ); + + await screen.findByText('Editing strategy:'); + await screen.findByText('change_request_title'); + await screen.findByText('current_title'); + expect(screen.queryByText('snapshot_title')).not.toBeInTheDocument(); + + const viewDiff = await screen.findByText('View Diff'); + userEvent.hover(viewDiff); + await screen.findByText(`- parameters.rollout: "${currentRollout}"`); + await screen.findByText(`+ parameters.rollout: "${changeRequestRollout}"`); +}); + +test('Editing strategy after change request is applied', async () => { + render( + , + ); + + await screen.findByText('Editing strategy:'); + await screen.findByText('change_request_title'); + await screen.findByText('snapshot_title'); + expect(screen.queryByText('current_title')).not.toBeInTheDocument(); + + const viewDiff = await screen.findByText('View Diff'); + userEvent.hover(viewDiff); + await screen.findByText(`- parameters.rollout: "${snapshotRollout}"`); + await screen.findByText(`+ parameters.rollout: "${changeRequestRollout}"`); +}); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index 03321f4f4984..aa5e7ea72d7a 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -271,11 +271,19 @@ export const StrategyChange: VFC<{ /> diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index 61ccaeeecd64..c0ef4f99fbf7 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/frontend/src/component/changeRequest/changeRequest.types.ts @@ -241,7 +241,7 @@ export type ChangeRequestAddStrategy = Pick< export type ChangeRequestEditStrategy = ChangeRequestAddStrategy & { id: string; - snapshot?: Omit & { title?: string | null }; + snapshot?: Omit & { title?: string }; }; type ChangeRequestDeleteStrategy = { From cb7b8352b36015832f0eb8acff2890a3dc108ff9 Mon Sep 17 00:00:00 2001 From: kwasniew Date: Tue, 26 Nov 2024 11:09:28 +0100 Subject: [PATCH 4/6] feat: change request applied diff --- .../ChangeRequest/Changes/Change/StrategyChange.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx index 0cda657f5574..945d914b8705 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx @@ -47,7 +47,7 @@ testServerRoute( }, ); -test('Editing strategy before change request is applied', async () => { +test('Editing strategy before change request is applied diffs against current strategy', async () => { render( { await screen.findByText(`+ parameters.rollout: "${changeRequestRollout}"`); }); -test('Editing strategy after change request is applied', async () => { +test('Editing strategy after change request is applied diffs against the snapshot', async () => { render( Date: Wed, 27 Nov 2024 11:52:10 +0100 Subject: [PATCH 5/6] feat: delete diff --- .../Changes/Change/StrategyChange.test.tsx | 111 ++++-- .../Changes/Change/StrategyChange.tsx | 329 ++++++++++-------- .../StrategyTooltipLink.tsx | 57 +-- .../changeRequest/changeRequest.types.ts | 5 +- .../StrategyExecution/StrategyExecution.tsx | 6 +- 5 files changed, 303 insertions(+), 205 deletions(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx index 945d914b8705..1e8fc5c4eb4f 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx @@ -25,27 +25,31 @@ const strategy = { disabled: false, }; -testServerRoute( - server, - `/api/admin/projects/${projectId}/features/${feature}`, - { - environments: [ - { - name: environmentName, - strategies: [ - { - ...strategy, - title: 'current_title', - parameters: { - ...strategy.parameters, - rollout: currentRollout, +const setupApi = () => { + testServerRoute( + server, + `/api/admin/projects/${projectId}/features/${feature}`, + { + environments: [ + { + name: environmentName, + strategies: [ + { + ...strategy, + title: 'current_title', + parameters: { + ...strategy.parameters, + rollout: currentRollout, + }, }, - }, - ], - }, - ], - }, -); + ], + }, + ], + }, + ); +}; + +beforeEach(setupApi); test('Editing strategy before change request is applied diffs against current strategy', async () => { render( @@ -83,7 +87,7 @@ test('Editing strategy before change request is applied diffs against current st expect(screen.queryByText('snapshot_title')).not.toBeInTheDocument(); const viewDiff = await screen.findByText('View Diff'); - userEvent.hover(viewDiff); + await userEvent.hover(viewDiff); await screen.findByText(`- parameters.rollout: "${currentRollout}"`); await screen.findByText(`+ parameters.rollout: "${changeRequestRollout}"`); }); @@ -124,7 +128,70 @@ test('Editing strategy after change request is applied diffs against the snapsho expect(screen.queryByText('current_title')).not.toBeInTheDocument(); const viewDiff = await screen.findByText('View Diff'); - userEvent.hover(viewDiff); + await userEvent.hover(viewDiff); await screen.findByText(`- parameters.rollout: "${snapshotRollout}"`); await screen.findByText(`+ parameters.rollout: "${changeRequestRollout}"`); }); + +test('Deleting strategy before change request is applied diffs against current strategy', async () => { + render( + , + ); + + await screen.findByText('- Deleting strategy:'); + await screen.findByText('Gradual rollout'); + await screen.findByText('current_title'); + + const viewDiff = await screen.findByText('View Diff'); + await userEvent.hover(viewDiff); + await screen.findByText('- constraints (deleted)'); +}); + +test('Deleting strategy before change request is applied diffs against the snapshot', async () => { + render( + , + ); + + await screen.findByText('- Deleting strategy:'); + await screen.findByText('Gradual rollout'); + await screen.findByText('snapshot_title'); + expect(screen.queryByText('current_title')).not.toBeInTheDocument(); + + const viewDiff = await screen.findByText('View Diff'); + await userEvent.hover(viewDiff); + await screen.findByText('- constraints (deleted)'); +}); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index aa5e7ea72d7a..fd03701a168b 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -1,5 +1,5 @@ import type React from 'react'; -import type { VFC, FC, ReactNode } from 'react'; +import type { FC, ReactNode } from 'react'; import { Box, styled, Tooltip, Typography } from '@mui/material'; import BlockIcon from '@mui/icons-material/Block'; import TrackChangesIcon from '@mui/icons-material/TrackChanges'; @@ -20,6 +20,7 @@ import { ConditionallyRender } from 'component/common/ConditionallyRender/Condit import { flexRow } from 'themes/themeStyles'; import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable'; import { ChangeOverwriteWarning } from './ChangeOverwriteWarning/ChangeOverwriteWarning'; +import type { IFeatureStrategy } from 'interfaces/strategy'; export const ChangeItemWrapper = styled(Box)({ display: 'flex', @@ -60,10 +61,7 @@ const StyledTypography: FC<{ children?: React.ReactNode }> = styled(Typography)( }), ); -const hasNameField = (payload: unknown): payload is { name: string } => - typeof payload === 'object' && payload !== null && 'name' in payload; - -const DisabledEnabledState: VFC<{ show?: boolean; disabled: boolean }> = ({ +const DisabledEnabledState: FC<{ show?: boolean; disabled: boolean }> = ({ show = true, disabled, }) => { @@ -98,7 +96,7 @@ const DisabledEnabledState: VFC<{ show?: boolean; disabled: boolean }> = ({ ); }; -const EditHeader: VFC<{ +const EditHeader: FC<{ wasDisabled?: boolean; willBeDisabled?: boolean; }> = ({ wasDisabled = false, willBeDisabled = false }) => { @@ -119,7 +117,157 @@ const EditHeader: VFC<{ return Editing strategy:; }; -export const StrategyChange: VFC<{ +const hasDiff = (object: unknown, objectToCompare: unknown) => + JSON.stringify(object) !== JSON.stringify(objectToCompare); + +const DeleteStrategy: FC<{ + change: IChangeRequestDeleteStrategy; + changeRequestState: ChangeRequestState; + currentStrategy: IFeatureStrategy | undefined; + actions?: ReactNode; +}> = ({ change, changeRequestState, currentStrategy, actions }) => { + const name = + changeRequestState === 'Applied' + ? change.payload?.snapshot?.name + : currentStrategy?.name; + const title = + changeRequestState === 'Applied' + ? change.payload?.snapshot?.title + : currentStrategy?.title; + const referenceStrategy = + changeRequestState === 'Applied' + ? change.payload.snapshot + : currentStrategy; + + return ( + <> + + + ({ + color: theme.palette.error.main, + })} + > + - Deleting strategy: + + + + + +
{actions}
+
+ {referenceStrategy && ( + + )} + + + Deleting strategy variants: + + + + ) + } + /> + + ); +}; + +const UpdateStrategy: FC<{ + change: IChangeRequestUpdateStrategy; + changeRequestState: ChangeRequestState; + currentStrategy: IFeatureStrategy | undefined; + actions?: ReactNode; +}> = ({ change, changeRequestState, currentStrategy, actions }) => { + const hasVariantDiff = hasDiff( + currentStrategy?.variants || [], + change.payload.variants || [], + ); + const previousTitle = + changeRequestState === 'Applied' + ? change.payload.snapshot?.title + : currentStrategy?.title; + const referenceStrategy = + changeRequestState === 'Applied' + ? change.payload.snapshot + : currentStrategy; + + return ( + <> + + + + + + + + +
{actions}
+
+ theme.spacing(2), + marginBottom: (theme) => theme.spacing(2), + ...flexRow, + gap: (theme) => theme.spacing(1), + }} + > + This strategy will be{' '} + + + } + /> + + + + Updating feature variants to: + + + + } + /> + + ); +}; + +export const StrategyChange: FC<{ actions?: ReactNode; change: | IChangeRequestAddStrategy @@ -144,16 +292,6 @@ 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 hasVariantDiff = - isStrategyAction && - hasDiff(currentStrategy?.variants || [], change.payload.variants || []); - return ( <> {change.action === 'addStrategy' && ( @@ -169,7 +307,10 @@ export const StrategyChange: VFC<{ > + Adding strategy: - + {actions} - - - Updating feature variants to: - - - - ) - } - /> - - )} - {change.action === 'deleteStrategy' && ( - <> - - - ({ - color: theme.palette.error.main, - })} - > - - Deleting strategy: - - {hasNameField(change.payload) && ( - - - - )} - -
{actions}
-
- - { - - } - - } - /> - - - Deleting strategy variants: - - - - ) - } - /> - - )} - {change.action === 'updateStrategy' && ( - <> - - - - - - - - -
{actions}
-
- theme.spacing(2), - marginBottom: (theme) => theme.spacing(2), - ...flexRow, - gap: (theme) => theme.spacing(1), - }} - > - This strategy will be{' '} - - - } - /> - - 0 && ( Updating feature variants to: - } - /> + )} )} + {change.action === 'deleteStrategy' && ( + + )} + {change.action === 'updateStrategy' && ( + + )} ); }; diff --git a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx index 7295dcf614e2..8719d91e2470 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/StrategyTooltipLink/StrategyTooltipLink.tsx @@ -51,10 +51,8 @@ export const StrategyDiff: FC<{ }; interface IStrategyTooltipLinkProps { - change: - | IChangeRequestAddStrategy - | IChangeRequestUpdateStrategy - | IChangeRequestDeleteStrategy; + name: string; + title?: string; previousTitle?: string; children?: React.ReactNode; } @@ -80,29 +78,32 @@ const Truncated = styled('div')(() => ({ })); export const StrategyTooltipLink: FC = ({ - change, + name, + title, previousTitle, children, -}) => ( - - - - - {formatStrategyName(change.payload.name)} - - - View Diff - - - - -); +}) => { + return ( + + + + + {formatStrategyName(name)} + + + View Diff + + + + + ); +}; diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index c0ef4f99fbf7..e2e742b8027a 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/frontend/src/component/changeRequest/changeRequest.types.ts @@ -241,14 +241,15 @@ export type ChangeRequestAddStrategy = Pick< export type ChangeRequestEditStrategy = ChangeRequestAddStrategy & { id: string; - snapshot?: Omit & { title?: string }; + snapshot?: IFeatureStrategy; }; type ChangeRequestDeleteStrategy = { id: string; - name: string; + name?: string; title?: string; disabled?: boolean; + snapshot?: IFeatureStrategy; }; export type ChangeRequestAction = diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution.tsx index 9f7bf3dbe801..38dbc9bca533 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution.tsx @@ -1,4 +1,4 @@ -import { Fragment, useMemo, type VFC } from 'react'; +import { type FC, Fragment, useMemo } from 'react'; import { Alert, Box, Chip, Link, styled } from '@mui/material'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import PercentageCircle from 'component/common/PercentageCircle/PercentageCircle'; @@ -56,7 +56,7 @@ const CustomStrategyDeprecationWarning = () => ( ); -const NoItems: VFC = () => ( +const NoItems: FC = () => ( This strategy does not have constraints or parameters. @@ -73,7 +73,7 @@ const StyledValueSeparator = styled('span')(({ theme }) => ({ color: theme.palette.neutral.main, })); -export const StrategyExecution: VFC = ({ +export const StrategyExecution: FC = ({ strategy, }) => { const { parameters, constraints = [] } = strategy; From 8bb29657c46d0b307e0fc5b17a18df5f5cf8ab7f Mon Sep 17 00:00:00 2001 From: kwasniew Date: Wed, 27 Nov 2024 11:55:00 +0100 Subject: [PATCH 6/6] feat: delete diff --- .../ChangeRequest/Changes/Change/StrategyChange.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx index 1e8fc5c4eb4f..90c7f8a4565f 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.test.tsx @@ -160,7 +160,7 @@ test('Deleting strategy before change request is applied diffs against current s await screen.findByText('- constraints (deleted)'); }); -test('Deleting strategy before change request is applied diffs against the snapshot', async () => { +test('Deleting strategy after change request is applied diffs against the snapshot', async () => { render(