From a228f54344e468085f5b8d4e2ab982b4613d924f Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Wed, 6 Dec 2023 15:39:17 +0100 Subject: [PATCH] feat: show scheduled CRs using strategies when removing it (#5560) Show a warning about how deleting a strategy might mess up scheduled change requests. If there are change requests, list them. If there are no conflicts, show nothing. If we don't know (because of no successful response from the API), say that it might cause issues. ![image](https://github.com/Unleash/unleash/assets/17786332/2c6a4257-69f5-458a-ab6f-9b2ea2f5d550) --- .../StrategyDraggableItem.tsx | 8 +- .../DialogStrategyRemove.test.tsx | 141 ++++++++++++++++++ .../DialogStrategyRemove.tsx | 139 ++++++++++++++--- .../useScheduledChangeRequestsWithStrategy.ts | 30 ++++ 4 files changed, 297 insertions(+), 21 deletions(-) create mode 100644 frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.test.tsx create mode 100644 frontend/src/hooks/api/getters/useScheduledChangeRequestsWithStrategy/useScheduledChangeRequestsWithStrategy.ts diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem.tsx index c11b69bcae..58135ddbf9 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyDraggableItem.tsx @@ -117,7 +117,12 @@ const renderHeaderChildren = ( ); if (draftChange) { - badges.push(); + badges.push( + , + ); } const scheduledChanges = changes.filter( @@ -127,6 +132,7 @@ const renderHeaderChildren = ( if (scheduledChanges.length > 0) { badges.push( scheduledChange.changeRequestId, )} diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.test.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.test.tsx new file mode 100644 index 0000000000..b84b425b91 --- /dev/null +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.test.tsx @@ -0,0 +1,141 @@ +import React from 'react'; +import { render } from 'utils/testRenderer'; +import { screen } from '@testing-library/react'; +import { + FeatureStrategyRemoveDialogue, + SuggestFeatureStrategyRemoveDialogue, +} from './DialogStrategyRemove'; +import { + ChangeRequestState, + IChangeRequestFeature, + IFeatureChange, +} from 'component/changeRequest/changeRequest.types'; + +const strategyId = 'c81e3a1d-e91c-4083-bd0f-75bb8a9e32a2'; +const projectId = 'default'; +const environment = 'development'; +const featureId = 'bb1d79e0-95b0-4393-b248-64d1e0294ee3'; + +describe('Use in scheduled change requests', () => { + it.each(['enabled', 'disabled'])( + 'should show usage in scheduled change requests with change requests %s for the project', + async (changeRequestsEnabled) => { + const changeRequestWithTitle = { id: 1, title: 'My CR' }; + const changeRequestWithoutTitle = { id: 2 }; + const scheduledChangeRequests = [ + changeRequestWithTitle, + changeRequestWithoutTitle, + ]; + + if (changeRequestsEnabled === 'enabled') { + render( + {}} + onClose={() => {}} + isOpen={true} + scheduledChangeRequestsForStrategy={{ + projectId, + changeRequests: scheduledChangeRequests, + }} + />, + ); + } else { + render( + {}} + onClose={() => {}} + isOpen={true} + scheduledChangeRequestsForStrategy={{ + projectId, + changeRequests: scheduledChangeRequests, + }} + />, + ); + } + + const alerts = await screen.findAllByRole('alert'); + + expect( + alerts.find((alert) => + alert.textContent!.startsWith('This strategy'), + ), + ).toBeTruthy(); + + const links = await screen.findAllByRole('link'); + + expect(links).toHaveLength(scheduledChangeRequests.length); + + const [link1, link2] = links; + + expect(link1).toHaveTextContent('#1 (My CR)'); + expect(link1).toHaveAccessibleDescription('Change request 1'); + expect(link1).toHaveAttribute( + 'href', + `/projects/default/change-requests/1`, + ); + + expect(link2).toHaveTextContent('#2'); + expect(link2).toHaveAccessibleDescription('Change request 2'); + expect(link2).toHaveAttribute( + 'href', + `/projects/default/change-requests/2`, + ); + }, + ); + + it('should not render scheduled change requests warning when there are no scheduled change requests', async () => { + render( + {}} + onClose={() => {}} + isOpen={true} + scheduledChangeRequestsForStrategy={{ + projectId, + changeRequests: [], + }} + />, + ); + + const alerts = await screen.findAllByRole('alert'); + + expect( + alerts.find((alert) => + alert.textContent!.startsWith('This strategy'), + ), + ).toBeFalsy(); + + expect(alerts).toHaveLength(1); + + const link = screen.queryByRole('link'); + + expect(link).toBe(null); + }); + + it("It should render a warning saying there might be scheduled change requests if it doesn't get a successful API response", async () => { + render( + {}} + onClose={() => {}} + isOpen={true} + scheduledChangeRequestsForStrategy={{ + projectId, + changeRequests: undefined, + }} + />, + ); + + const alerts = await screen.findAllByRole('alert'); + + expect( + alerts.find((alert) => + alert.textContent!.startsWith('This strategy'), + ), + ).toBeTruthy(); + + expect(alerts).toHaveLength(2); + + const link = screen.queryByRole('link'); + + expect(link).toBe(null); + }); +}); diff --git a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.tsx b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.tsx index 0394023418..3f25d59534 100644 --- a/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.tsx +++ b/frontend/src/component/feature/FeatureView/FeatureOverview/FeatureOverviewEnvironments/FeatureOverviewEnvironment/EnvironmentAccordionBody/StrategyDraggableItem/StrategyItem/MenuStrategyRemove/DialogStrategyRemove.tsx @@ -1,7 +1,7 @@ import React, { FC } from 'react'; import useFeatureStrategyApi from 'hooks/api/actions/useFeatureStrategyApi/useFeatureStrategyApi'; import { formatUnknownError } from 'utils/formatUnknownError'; -import { useNavigate } from 'react-router-dom'; +import { Link, useNavigate } from 'react-router-dom'; import useToast from 'hooks/useToast'; import { formatFeaturePath } from '../../../../../../../../FeatureStrategy/FeatureStrategyEdit/FeatureStrategyEdit'; import { Dialogue } from 'component/common/Dialogue/Dialogue'; @@ -10,7 +10,8 @@ import { useFeature } from 'hooks/api/getters/useFeature/useFeature'; import { useChangeRequestApi } from 'hooks/api/actions/useChangeRequestApi/useChangeRequestApi'; import { useChangeRequestsEnabled } from 'hooks/useChangeRequestsEnabled'; import { usePendingChangeRequests } from 'hooks/api/getters/usePendingChangeRequests/usePendingChangeRequests'; - +import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; +import { useScheduledChangeRequestsWithStrategy } from 'hooks/api/getters/useScheduledChangeRequestsWithStrategy/useScheduledChangeRequestsWithStrategy'; interface IFeatureStrategyRemoveProps { projectId: string; featureId: string; @@ -18,13 +19,23 @@ interface IFeatureStrategyRemoveProps { strategyId: string; disabled?: boolean; icon?: boolean; - text?: boolean; } +type ChangeRequest = { + id: number; + title?: string; +}; + +type ScheduledChangeRequestData = { + changeRequests?: ChangeRequest[]; + projectId: string; +}; + interface IFeatureStrategyRemoveDialogueProps { onRemove: (event: React.FormEvent) => Promise; onClose: () => void; isOpen: boolean; + scheduledChangeRequestsForStrategy: ScheduledChangeRequestData; } const RemoveAlert: FC = () => ( @@ -34,30 +45,103 @@ const RemoveAlert: FC = () => ( ); -const FeatureStrategyRemoveDialogue: FC = - ({ onRemove, onClose, isOpen }) => { +const AlertContainer = styled('div')(({ theme }) => ({ + '> * + *': { + marginTop: theme.spacing(1), + }, +})); + +const StrategyInScheduledChangeRequestsWarning: FC<{ + changeRequests?: ChangeRequest[]; + projectId: string; +}> = ({ changeRequests, projectId }) => { + if (changeRequests && changeRequests.length > 0) { return ( - - - + +

+ This strategy is in use by at least one scheduled change + request. If you remove it, those change requests can no + longer be applied. +

+

+ The following scheduled change requests use this strategy: +

+
    + {changeRequests.map(({ id, title }) => { + const text = title ? `#${id} (${title})` : `#${id}`; + return ( +
  • + + {text} + +
    • + ); + })} +
+ ); - }; + } else if (changeRequests === undefined) { + return ( + +

+ This strategy may be in use by one or more scheduled change + requests. If you remove it, those change requests can no + longer be applied. +

+ + ); + } + + // all good, we have nothing to show + return null; +}; + +const Alerts: FC<{ + scheduledChangeRequestsForStrategy: ScheduledChangeRequestData; +}> = ({ scheduledChangeRequestsForStrategy }) => ( + + + + +); + +export const FeatureStrategyRemoveDialogue: FC< + IFeatureStrategyRemoveDialogueProps +> = ({ onRemove, onClose, isOpen, scheduledChangeRequestsForStrategy }) => { + return ( + + + + ); +}; const MsgContainer = styled('div')(({ theme }) => ({ marginTop: theme.spacing(3), marginBottom: theme.spacing(1), })); -const SuggestFeatureStrategyRemoveDialogue: FC< +export const SuggestFeatureStrategyRemoveDialogue: FC< IFeatureStrategyRemoveDialogueProps -> = ({ onRemove, onClose, isOpen }) => { +> = ({ onRemove, onClose, isOpen, scheduledChangeRequestsForStrategy }) => { return ( - + Your suggestion: @@ -155,7 +243,6 @@ export const DialogStrategyRemove = ({ featureId, environmentId, strategyId, - text, isOpen, onClose, }: IFeatureStrategyRemoveProps & { @@ -164,6 +251,16 @@ export const DialogStrategyRemove = ({ }) => { const { isChangeRequestConfigured } = useChangeRequestsEnabled(projectId); + const { changeRequests } = useScheduledChangeRequestsWithStrategy( + projectId, + strategyId, + ); + + const changeRequestData = { + changeRequests, + projectId, + }; + const onRemove = useOnRemove({ featureId, projectId, @@ -186,6 +283,7 @@ export const DialogStrategyRemove = ({ await onSuggestRemove(e); onClose(); }} + scheduledChangeRequestsForStrategy={changeRequestData} /> ); } @@ -195,6 +293,7 @@ export const DialogStrategyRemove = ({ isOpen={isOpen} onClose={() => onClose()} onRemove={onRemove} + scheduledChangeRequestsForStrategy={changeRequestData} /> ); }; diff --git a/frontend/src/hooks/api/getters/useScheduledChangeRequestsWithStrategy/useScheduledChangeRequestsWithStrategy.ts b/frontend/src/hooks/api/getters/useScheduledChangeRequestsWithStrategy/useScheduledChangeRequestsWithStrategy.ts new file mode 100644 index 0000000000..ad99deb816 --- /dev/null +++ b/frontend/src/hooks/api/getters/useScheduledChangeRequestsWithStrategy/useScheduledChangeRequestsWithStrategy.ts @@ -0,0 +1,30 @@ +import { formatApiPath } from 'utils/formatPath'; +import handleErrorResponses from '../httpErrorResponseHandler'; +import { IChangeRequest } from 'component/changeRequest/changeRequest.types'; +import { useEnterpriseSWR } from '../useEnterpriseSWR/useEnterpriseSWR'; + +const fetcher = (path: string) => { + return fetch(path) + .then(handleErrorResponses('ChangeRequest')) + .then((res) => res.json()); +}; + +export const useScheduledChangeRequestsWithStrategy = ( + project: string, + strategyId: string, +) => { + const { data, error, mutate } = useEnterpriseSWR( + [], + formatApiPath( + `api/admin/projects/${project}/change-requests/scheduled/with-strategy/${strategyId}`, + ), + fetcher, + ); + + return { + changeRequests: data, + loading: !error && !data, + refetch: mutate, + error, + }; +};