From a8fa1ae347ee9bebff72bd77850e3a6c018520bf Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 16 Feb 2024 12:41:14 +0800 Subject: [PATCH] fix: hide warnings that you'll overwrite changes on CRs that are already applied (#6214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current approach uses adds an extra parameter to the components and passes it through from the parent components. It's never a lot of levels, so it feels alright, but it's feels like a bit of a code smell. I wonder if it would make sense to use a context for each change request? 🤔 Supersedes: https://github.com/Unleash/unleash/pull/6181 --- .../ChangeRequest/ChangeRequest.tsx | 1 + .../ChangeOverwriteWarning.tsx | 73 ++++++++++++ .../OverwriteWarning.test.tsx | 27 +++++ .../OverwriteWarning.tsx} | 105 +++--------------- .../strategy-change-diff-calculation.test.ts | 0 .../strategy-change-diff-calculation.ts | 0 .../Changes/Change/FeatureChange.tsx | 2 + .../Changes/Change/SegmentChange.tsx | 13 ++- .../Changes/Change/SegmentChangeDetails.tsx | 16 ++- .../Changes/Change/StrategyChange.tsx | 23 +++- .../Change/VariantPatch/VariantPatch.tsx | 19 +++- 11 files changed, 171 insertions(+), 108 deletions(-) create mode 100644 frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/ChangeOverwriteWarning.tsx create mode 100644 frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.test.tsx rename frontend/src/component/changeRequest/ChangeRequest/Changes/Change/{StrategyChangeOverwriteWarning.tsx => ChangeOverwriteWarning/OverwriteWarning.tsx} (62%) rename frontend/src/component/changeRequest/ChangeRequest/Changes/Change/{ => ChangeOverwriteWarning}/strategy-change-diff-calculation.test.ts (100%) rename frontend/src/component/changeRequest/ChangeRequest/Changes/Change/{ => ChangeOverwriteWarning}/strategy-change-diff-calculation.ts (100%) diff --git a/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.tsx b/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.tsx index 4833ddfc88..8aec8a0bbb 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/ChangeRequest.tsx @@ -34,6 +34,7 @@ export const ChangeRequest: VFC = ({ key={segmentChange.payload.id} segmentChange={segmentChange} onNavigate={onNavigate} + changeRequestState={changeRequest.state} actions={ = ({ data, changeRequestState }) => { + const checkForChanges = useUiFlag('changeRequestConflictHandling'); + if (!checkForChanges) { + return null; + } + + const getChangesThatWouldBeOverwritten = () => { + switch (data.changeType) { + case 'segment': + return getSegmentChangesThatWouldBeOverwritten( + data.current, + data.change, + ); + case 'strategy': + return getStrategyChangesThatWouldBeOverwritten( + data.current, + data.change, + ); + case 'environment variant configuration': + return getEnvVariantChangesThatWouldBeOverwritten( + data.current, + data.change, + ); + } + }; + + return ( + + ); +}; diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.test.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.test.tsx new file mode 100644 index 0000000000..adf38b89e4 --- /dev/null +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.test.tsx @@ -0,0 +1,27 @@ +import { render } from 'utils/testRenderer'; +import { screen } from '@testing-library/react'; +import { OverwriteWarning } from './OverwriteWarning'; +import { ChangeRequestState } from 'component/changeRequest/changeRequest.types'; + +test.each([ + ['Draft', true], + ['Approved', true], + ['In review', true], + ['Applied', false], + ['Scheduled', true], + ['Cancelled', false], + ['Rejected', false], +])('Shows warnings for CRs in the "%s" state: %s', (status, showWarning) => { + render( + , + ); + + const hasRendered = screen.queryByText('Heads up!') !== null; + expect(hasRendered).toBe(showWarning); +}); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.tsx similarity index 62% rename from frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx rename to frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.tsx index 7387f22b6c..7a33b58fd9 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChangeOverwriteWarning.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/OverwriteWarning.tsx @@ -1,21 +1,6 @@ import { Box, styled } from '@mui/material'; -import { useChangeRequestPlausibleContext } from 'component/changeRequest/ChangeRequestContext'; -import { - IChangeRequestPatchVariant, - IChangeRequestUpdateSegment, - IChangeRequestUpdateStrategy, -} from 'component/changeRequest/changeRequest.types'; -import { useUiFlag } from 'hooks/useUiFlag'; -import { ISegment } from 'interfaces/segment'; -import { IFeatureStrategy } from 'interfaces/strategy'; -import { - ChangesThatWouldBeOverwritten, - getEnvVariantChangesThatWouldBeOverwritten, - getStrategyChangesThatWouldBeOverwritten, - getSegmentChangesThatWouldBeOverwritten, -} from './strategy-change-diff-calculation'; -import { useEffect } from 'react'; -import { IFeatureVariant } from 'interfaces/featureToggle'; +import { ChangeRequestState } from 'component/changeRequest/changeRequest.types'; +import { ChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation'; const ChangesToOverwriteContainer = styled(Box)(({ theme }) => ({ color: theme.palette.warning.dark, @@ -137,10 +122,19 @@ const DetailsTable: React.FC<{ ); }; -const OverwriteWarning: React.FC<{ +export const OverwriteWarning: React.FC<{ changeType: 'segment' | 'strategy' | 'environment variant configuration'; - changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten; -}> = ({ changeType, changesThatWouldBeOverwritten }) => { + changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten | null; + changeRequestState: ChangeRequestState; +}> = ({ changeType, changesThatWouldBeOverwritten, changeRequestState }) => { + const changeRequestIsClosed = ['Applied', 'Cancelled', 'Rejected'].includes( + changeRequestState, + ); + + if (!changesThatWouldBeOverwritten || changeRequestIsClosed) { + return null; + } + return (

@@ -159,74 +153,3 @@ const OverwriteWarning: React.FC<{ ); }; - -export const EnvVariantChangesToOverwrite: React.FC<{ - currentVariants?: IFeatureVariant[]; - change: IChangeRequestPatchVariant; -}> = ({ change, currentVariants }) => { - const checkForChanges = useUiFlag('changeRequestConflictHandling'); - const changesThatWouldBeOverwritten = checkForChanges - ? getEnvVariantChangesThatWouldBeOverwritten(currentVariants, change) - : null; - - if (!changesThatWouldBeOverwritten) { - return null; - } - - return ( - - ); -}; - -export const SegmentChangesToOverwrite: React.FC<{ - currentSegment?: ISegment; - change: IChangeRequestUpdateSegment; -}> = ({ change, currentSegment }) => { - const checkForChanges = useUiFlag('changeRequestConflictHandling'); - const changesThatWouldBeOverwritten = checkForChanges - ? getSegmentChangesThatWouldBeOverwritten(currentSegment, change) - : null; - - if (!changesThatWouldBeOverwritten) { - return null; - } - - return ( - - ); -}; - -export const StrategyChangesToOverwrite: React.FC<{ - currentStrategy?: IFeatureStrategy; - change: IChangeRequestUpdateStrategy; -}> = ({ change, currentStrategy }) => { - const checkForChanges = useUiFlag('changeRequestConflictHandling'); - const changesThatWouldBeOverwritten = checkForChanges - ? getStrategyChangesThatWouldBeOverwritten(currentStrategy, change) - : null; - const { registerWillOverwriteStrategyChanges } = - useChangeRequestPlausibleContext(); - - useEffect(() => { - if (changesThatWouldBeOverwritten) { - registerWillOverwriteStrategyChanges(); - } - }, [changesThatWouldBeOverwritten]); - - if (!changesThatWouldBeOverwritten) { - return null; - } - - return ( - - ); -}; diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts similarity index 100% rename from frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.test.ts rename to frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.test.ts diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts similarity index 100% rename from frontend/src/component/changeRequest/ChangeRequest/Changes/Change/strategy-change-diff-calculation.ts rename to frontend/src/component/changeRequest/ChangeRequest/Changes/Change/ChangeOverwriteWarning/strategy-change-diff-calculation.ts diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/FeatureChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/FeatureChange.tsx index 5aae905dc7..435d2ef3a1 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/FeatureChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/FeatureChange.tsx @@ -170,12 +170,14 @@ export const FeatureChange: FC<{ featureName={feature.name} environmentName={changeRequest.environment} projectId={changeRequest.project} + changeRequestState={changeRequest.state} /> ) : null} {change.action === 'patchVariant' && ( void; actions: ReactNode; + changeRequestState: ChangeRequestState; } export const SegmentChange: FC = ({ segmentChange, onNavigate, actions, + changeRequestState, }) => ( = ({ - + ); diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx index 5a3281d1c7..3d56c69b7f 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/SegmentChangeDetails.tsx @@ -1,13 +1,14 @@ import { VFC, FC, ReactNode } from 'react'; import { Box, styled, Typography } from '@mui/material'; import { + ChangeRequestState, IChangeRequestDeleteSegment, IChangeRequestUpdateSegment, } from 'component/changeRequest/changeRequest.types'; import { useSegment } from 'hooks/api/getters/useSegment/useSegment'; import { SegmentDiff, SegmentTooltipLink } from '../../SegmentTooltipLink'; import { ConstraintAccordionList } from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList'; -import { SegmentChangesToOverwrite } from './StrategyChangeOverwriteWarning'; +import { ChangeOverwriteWarning } from './ChangeOverwriteWarning/ChangeOverwriteWarning'; const ChangeItemCreateEditWrapper = styled(Box)(({ theme }) => ({ display: 'grid', @@ -51,7 +52,8 @@ const SegmentContainer = styled(Box, { export const SegmentChangeDetails: VFC<{ actions?: ReactNode; change: IChangeRequestUpdateSegment | IChangeRequestDeleteSegment; -}> = ({ actions, change }) => { + changeRequestState: ChangeRequestState; +}> = ({ actions, change, changeRequestState }) => { const { segment: currentSegment } = useSegment(change.payload.id); return ( @@ -78,9 +80,13 @@ export const SegmentChangeDetails: VFC<{ )} {change.action === 'updateSegment' && ( <> - diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx index 34963b07e8..ed83b64a28 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/StrategyChange.tsx @@ -8,6 +8,7 @@ import { } from '../../StrategyTooltipLink/StrategyTooltipLink'; import { StrategyExecution } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution'; import { + ChangeRequestState, IChangeRequestAddStrategy, IChangeRequestDeleteStrategy, IChangeRequestUpdateStrategy, @@ -17,7 +18,7 @@ import { Badge } from 'component/common/Badge/Badge'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { flexRow } from 'themes/themeStyles'; import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable'; -import { StrategyChangesToOverwrite } from './StrategyChangeOverwriteWarning'; +import { ChangeOverwriteWarning } from './ChangeOverwriteWarning/ChangeOverwriteWarning'; export const ChangeItemWrapper = styled(Box)({ display: 'flex', @@ -120,7 +121,15 @@ export const StrategyChange: VFC<{ environmentName: string; featureName: string; projectId: string; -}> = ({ actions, change, featureName, environmentName, projectId }) => { + changeRequestState: ChangeRequestState; +}> = ({ + actions, + change, + featureName, + environmentName, + projectId, + changeRequestState, +}) => { const currentStrategy = useCurrentStrategy( change, projectId, @@ -224,9 +233,13 @@ export const StrategyChange: VFC<{ )} {change.action === 'updateStrategy' && ( <> - diff --git a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/VariantPatch/VariantPatch.tsx b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/VariantPatch/VariantPatch.tsx index d216752d6e..f463b6af3e 100644 --- a/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/VariantPatch/VariantPatch.tsx +++ b/frontend/src/component/changeRequest/ChangeRequest/Changes/Change/VariantPatch/VariantPatch.tsx @@ -1,12 +1,15 @@ import { Box, styled } from '@mui/material'; -import { IChangeRequestPatchVariant } from 'component/changeRequest/changeRequest.types'; +import { + ChangeRequestState, + IChangeRequestPatchVariant, +} from 'component/changeRequest/changeRequest.types'; import { Badge } from 'component/common/Badge/Badge'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { TooltipLink } from 'component/common/TooltipLink/TooltipLink'; import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable'; import { useFeature } from 'hooks/api/getters/useFeature/useFeature'; import { ReactNode } from 'react'; -import { EnvVariantChangesToOverwrite } from '../StrategyChangeOverwriteWarning'; +import { ChangeOverwriteWarning } from '../ChangeOverwriteWarning/ChangeOverwriteWarning'; import { VariantDiff } from './VariantDiff'; const ChangeItemInfo = styled(Box)({ @@ -37,6 +40,7 @@ interface IVariantPatchProps { environment: string; change: IChangeRequestPatchVariant; actions?: ReactNode; + changeRequestState: ChangeRequestState; } export const VariantPatch = ({ @@ -45,6 +49,7 @@ export const VariantPatch = ({ environment, change, actions, + changeRequestState, }: IVariantPatchProps) => { const { feature: featureData } = useFeature(project, feature); @@ -54,9 +59,13 @@ export const VariantPatch = ({ return ( -