1
0
mirror of https://github.com/Unleash/unleash.git synced 2025-04-15 01:16:22 +02:00

fix: hide warnings that you'll overwrite changes on CRs that are already applied (#6214)

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
This commit is contained in:
Thomas Heartman 2024-02-16 12:41:14 +08:00 committed by GitHub
parent 64a6af2858
commit a8fa1ae347
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 171 additions and 108 deletions

View File

@ -34,6 +34,7 @@ export const ChangeRequest: VFC<IChangeRequestProps> = ({
key={segmentChange.payload.id} key={segmentChange.payload.id}
segmentChange={segmentChange} segmentChange={segmentChange}
onNavigate={onNavigate} onNavigate={onNavigate}
changeRequestState={changeRequest.state}
actions={ actions={
<ChangeActions <ChangeActions
changeRequest={changeRequest} changeRequest={changeRequest}

View File

@ -0,0 +1,73 @@
import {
ChangeRequestState,
IChangeRequestPatchVariant,
IChangeRequestUpdateSegment,
IChangeRequestUpdateStrategy,
} from 'component/changeRequest/changeRequest.types';
import { useChangeRequestPlausibleContext } from 'component/changeRequest/ChangeRequestContext';
import { useUiFlag } from 'hooks/useUiFlag';
import { IFeatureVariant } from 'interfaces/featureToggle';
import { ISegment } from 'interfaces/segment';
import { IFeatureStrategy } from 'interfaces/strategy';
import { useEffect } from 'react';
import { OverwriteWarning } from './OverwriteWarning';
import {
getEnvVariantChangesThatWouldBeOverwritten,
getSegmentChangesThatWouldBeOverwritten,
getStrategyChangesThatWouldBeOverwritten,
} from './strategy-change-diff-calculation';
type ChangeData =
| {
changeType: 'environment variant configuration';
current?: IFeatureVariant[];
change: IChangeRequestPatchVariant;
}
| {
changeType: 'segment';
current?: ISegment;
change: IChangeRequestUpdateSegment;
}
| {
changeType: 'strategy';
current?: IFeatureStrategy;
change: IChangeRequestUpdateStrategy;
};
export const ChangeOverwriteWarning: React.FC<{
data: ChangeData;
changeRequestState: ChangeRequestState;
}> = ({ 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 (
<OverwriteWarning
changeRequestState={changeRequestState}
changeType={data.changeType}
changesThatWouldBeOverwritten={getChangesThatWouldBeOverwritten()}
/>
);
};

View File

@ -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(
<OverwriteWarning
changesThatWouldBeOverwritten={[
{ property: 'some-prop', oldValue: 'old', newValue: 'new' },
]}
changeType={'strategy'}
changeRequestState={status as ChangeRequestState}
/>,
);
const hasRendered = screen.queryByText('Heads up!') !== null;
expect(hasRendered).toBe(showWarning);
});

View File

@ -1,21 +1,6 @@
import { Box, styled } from '@mui/material'; import { Box, styled } from '@mui/material';
import { useChangeRequestPlausibleContext } from 'component/changeRequest/ChangeRequestContext'; import { ChangeRequestState } from 'component/changeRequest/changeRequest.types';
import { import { ChangesThatWouldBeOverwritten } from './strategy-change-diff-calculation';
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';
const ChangesToOverwriteContainer = styled(Box)(({ theme }) => ({ const ChangesToOverwriteContainer = styled(Box)(({ theme }) => ({
color: theme.palette.warning.dark, 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'; changeType: 'segment' | 'strategy' | 'environment variant configuration';
changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten; changesThatWouldBeOverwritten: ChangesThatWouldBeOverwritten | null;
}> = ({ changeType, changesThatWouldBeOverwritten }) => { changeRequestState: ChangeRequestState;
}> = ({ changeType, changesThatWouldBeOverwritten, changeRequestState }) => {
const changeRequestIsClosed = ['Applied', 'Cancelled', 'Rejected'].includes(
changeRequestState,
);
if (!changesThatWouldBeOverwritten || changeRequestIsClosed) {
return null;
}
return ( return (
<ChangesToOverwriteContainer> <ChangesToOverwriteContainer>
<p> <p>
@ -159,74 +153,3 @@ const OverwriteWarning: React.FC<{
</ChangesToOverwriteContainer> </ChangesToOverwriteContainer>
); );
}; };
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 (
<OverwriteWarning
changeType='environment variant configuration'
changesThatWouldBeOverwritten={changesThatWouldBeOverwritten}
/>
);
};
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 (
<OverwriteWarning
changeType='segment'
changesThatWouldBeOverwritten={changesThatWouldBeOverwritten}
/>
);
};
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 (
<OverwriteWarning
changeType='strategy'
changesThatWouldBeOverwritten={changesThatWouldBeOverwritten}
/>
);
};

View File

@ -170,12 +170,14 @@ export const FeatureChange: FC<{
featureName={feature.name} featureName={feature.name}
environmentName={changeRequest.environment} environmentName={changeRequest.environment}
projectId={changeRequest.project} projectId={changeRequest.project}
changeRequestState={changeRequest.state}
/> />
) : null} ) : null}
{change.action === 'patchVariant' && ( {change.action === 'patchVariant' && (
<VariantPatch <VariantPatch
feature={feature.name} feature={feature.name}
project={changeRequest.project} project={changeRequest.project}
changeRequestState={changeRequest.state}
environment={changeRequest.environment} environment={changeRequest.environment}
change={change} change={change}
actions={actions} actions={actions}

View File

@ -1,7 +1,10 @@
import { FC, ReactNode } from 'react'; import { FC, ReactNode } from 'react';
import { Link as RouterLink } from 'react-router-dom'; import { Link as RouterLink } from 'react-router-dom';
import { Box, Card, Typography, Link } from '@mui/material'; import { Box, Card, Typography, Link } from '@mui/material';
import { ISegmentChange } from '../../../changeRequest.types'; import {
ChangeRequestState,
ISegmentChange,
} from '../../../changeRequest.types';
import { SegmentChangeDetails } from './SegmentChangeDetails'; import { SegmentChangeDetails } from './SegmentChangeDetails';
import { ConflictWarning } from './ConflictWarning'; import { ConflictWarning } from './ConflictWarning';
@ -9,12 +12,14 @@ interface ISegmentChangeProps {
segmentChange: ISegmentChange; segmentChange: ISegmentChange;
onNavigate?: () => void; onNavigate?: () => void;
actions: ReactNode; actions: ReactNode;
changeRequestState: ChangeRequestState;
} }
export const SegmentChange: FC<ISegmentChangeProps> = ({ export const SegmentChange: FC<ISegmentChangeProps> = ({
segmentChange, segmentChange,
onNavigate, onNavigate,
actions, actions,
changeRequestState,
}) => ( }) => (
<Card <Card
elevation={0} elevation={0}
@ -66,6 +71,10 @@ export const SegmentChange: FC<ISegmentChangeProps> = ({
</Link> </Link>
</Box> </Box>
</Box> </Box>
<SegmentChangeDetails change={segmentChange} actions={actions} /> <SegmentChangeDetails
change={segmentChange}
actions={actions}
changeRequestState={changeRequestState}
/>
</Card> </Card>
); );

View File

@ -1,13 +1,14 @@
import { VFC, FC, ReactNode } from 'react'; import { VFC, FC, ReactNode } from 'react';
import { Box, styled, Typography } from '@mui/material'; import { Box, styled, Typography } from '@mui/material';
import { import {
ChangeRequestState,
IChangeRequestDeleteSegment, IChangeRequestDeleteSegment,
IChangeRequestUpdateSegment, IChangeRequestUpdateSegment,
} from 'component/changeRequest/changeRequest.types'; } from 'component/changeRequest/changeRequest.types';
import { useSegment } from 'hooks/api/getters/useSegment/useSegment'; import { useSegment } from 'hooks/api/getters/useSegment/useSegment';
import { SegmentDiff, SegmentTooltipLink } from '../../SegmentTooltipLink'; import { SegmentDiff, SegmentTooltipLink } from '../../SegmentTooltipLink';
import { ConstraintAccordionList } from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList'; import { ConstraintAccordionList } from 'component/common/ConstraintAccordion/ConstraintAccordionList/ConstraintAccordionList';
import { SegmentChangesToOverwrite } from './StrategyChangeOverwriteWarning'; import { ChangeOverwriteWarning } from './ChangeOverwriteWarning/ChangeOverwriteWarning';
const ChangeItemCreateEditWrapper = styled(Box)(({ theme }) => ({ const ChangeItemCreateEditWrapper = styled(Box)(({ theme }) => ({
display: 'grid', display: 'grid',
@ -51,7 +52,8 @@ const SegmentContainer = styled(Box, {
export const SegmentChangeDetails: VFC<{ export const SegmentChangeDetails: VFC<{
actions?: ReactNode; actions?: ReactNode;
change: IChangeRequestUpdateSegment | IChangeRequestDeleteSegment; change: IChangeRequestUpdateSegment | IChangeRequestDeleteSegment;
}> = ({ actions, change }) => { changeRequestState: ChangeRequestState;
}> = ({ actions, change, changeRequestState }) => {
const { segment: currentSegment } = useSegment(change.payload.id); const { segment: currentSegment } = useSegment(change.payload.id);
return ( return (
@ -78,9 +80,13 @@ export const SegmentChangeDetails: VFC<{
)} )}
{change.action === 'updateSegment' && ( {change.action === 'updateSegment' && (
<> <>
<SegmentChangesToOverwrite <ChangeOverwriteWarning
currentSegment={currentSegment} data={{
change={change} current: currentSegment,
change,
changeType: 'segment',
}}
changeRequestState={changeRequestState}
/> />
<ChangeItemCreateEditWrapper> <ChangeItemCreateEditWrapper>
<ChangeItemInfo> <ChangeItemInfo>

View File

@ -8,6 +8,7 @@ import {
} from '../../StrategyTooltipLink/StrategyTooltipLink'; } from '../../StrategyTooltipLink/StrategyTooltipLink';
import { StrategyExecution } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution'; import { StrategyExecution } from 'component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/StrategyExecution/StrategyExecution';
import { import {
ChangeRequestState,
IChangeRequestAddStrategy, IChangeRequestAddStrategy,
IChangeRequestDeleteStrategy, IChangeRequestDeleteStrategy,
IChangeRequestUpdateStrategy, IChangeRequestUpdateStrategy,
@ -17,7 +18,7 @@ import { Badge } from 'component/common/Badge/Badge';
import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
import { flexRow } from 'themes/themeStyles'; import { flexRow } from 'themes/themeStyles';
import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable'; 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)({ export const ChangeItemWrapper = styled(Box)({
display: 'flex', display: 'flex',
@ -120,7 +121,15 @@ export const StrategyChange: VFC<{
environmentName: string; environmentName: string;
featureName: string; featureName: string;
projectId: string; projectId: string;
}> = ({ actions, change, featureName, environmentName, projectId }) => { changeRequestState: ChangeRequestState;
}> = ({
actions,
change,
featureName,
environmentName,
projectId,
changeRequestState,
}) => {
const currentStrategy = useCurrentStrategy( const currentStrategy = useCurrentStrategy(
change, change,
projectId, projectId,
@ -224,9 +233,13 @@ export const StrategyChange: VFC<{
)} )}
{change.action === 'updateStrategy' && ( {change.action === 'updateStrategy' && (
<> <>
<StrategyChangesToOverwrite <ChangeOverwriteWarning
currentStrategy={currentStrategy} data={{
change={change} current: currentStrategy,
change,
changeType: 'strategy',
}}
changeRequestState={changeRequestState}
/> />
<ChangeItemCreateEditDeleteWrapper> <ChangeItemCreateEditDeleteWrapper>
<ChangeItemInfo> <ChangeItemInfo>

View File

@ -1,12 +1,15 @@
import { Box, styled } from '@mui/material'; 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 { Badge } from 'component/common/Badge/Badge';
import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender';
import { TooltipLink } from 'component/common/TooltipLink/TooltipLink'; import { TooltipLink } from 'component/common/TooltipLink/TooltipLink';
import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable'; import { EnvironmentVariantsTable } from 'component/feature/FeatureView/FeatureVariants/FeatureEnvironmentVariants/EnvironmentVariantsCard/EnvironmentVariantsTable/EnvironmentVariantsTable';
import { useFeature } from 'hooks/api/getters/useFeature/useFeature'; import { useFeature } from 'hooks/api/getters/useFeature/useFeature';
import { ReactNode } from 'react'; import { ReactNode } from 'react';
import { EnvVariantChangesToOverwrite } from '../StrategyChangeOverwriteWarning'; import { ChangeOverwriteWarning } from '../ChangeOverwriteWarning/ChangeOverwriteWarning';
import { VariantDiff } from './VariantDiff'; import { VariantDiff } from './VariantDiff';
const ChangeItemInfo = styled(Box)({ const ChangeItemInfo = styled(Box)({
@ -37,6 +40,7 @@ interface IVariantPatchProps {
environment: string; environment: string;
change: IChangeRequestPatchVariant; change: IChangeRequestPatchVariant;
actions?: ReactNode; actions?: ReactNode;
changeRequestState: ChangeRequestState;
} }
export const VariantPatch = ({ export const VariantPatch = ({
@ -45,6 +49,7 @@ export const VariantPatch = ({
environment, environment,
change, change,
actions, actions,
changeRequestState,
}: IVariantPatchProps) => { }: IVariantPatchProps) => {
const { feature: featureData } = useFeature(project, feature); const { feature: featureData } = useFeature(project, feature);
@ -54,9 +59,13 @@ export const VariantPatch = ({
return ( return (
<ChangeItemInfo> <ChangeItemInfo>
<EnvVariantChangesToOverwrite <ChangeOverwriteWarning
currentVariants={preData} data={{
change={change} current: preData,
change,
changeType: 'environment variant configuration',
}}
changeRequestState={changeRequestState}
/> />
<StyledChangeHeader> <StyledChangeHeader>
<TooltipLink <TooltipLink