From 9176ffae1e0662a81f0f3875523af10d2fc488f0 Mon Sep 17 00:00:00 2001 From: sjaanus Date: Thu, 17 Nov 2022 10:08:29 +0100 Subject: [PATCH] Change requests - add multiple reviewers (#2448) This PR adds implements the frontend and migrations part of multiple reviewers. 2 UI parts: 1. Configuration to add the count of required approvals 2. Handle multiple approvers in review page. --- .../ChangeRequestOverview.tsx | 9 +- .../ChangeRequestReviewStatus.tsx | 49 +++++- .../changeRequest/changeRequest.types.ts | 1 + .../common/GeneralSelect/GeneralSelect.tsx | 4 + .../ChangeRequestConfiguration.tsx | 142 ++++++++++++++---- .../useChangeRequestApi.ts | 23 ++- src/lib/services/project-service.ts | 10 +- .../20221115072335-add-required-approvals.js | 25 +++ 8 files changed, 216 insertions(+), 47 deletions(-) create mode 100644 src/migrations/20221115072335-add-required-approvals.js diff --git a/frontend/src/component/changeRequest/ChangeRequestOverview/ChangeRequestOverview.tsx b/frontend/src/component/changeRequest/ChangeRequestOverview/ChangeRequestOverview.tsx index c9c3383b57..709fb77df7 100644 --- a/frontend/src/component/changeRequest/ChangeRequestOverview/ChangeRequestOverview.tsx +++ b/frontend/src/component/changeRequest/ChangeRequestOverview/ChangeRequestOverview.tsx @@ -100,6 +100,9 @@ export const ChangeRequestOverview: FC = () => { changeRequest.state === 'In review' && !isAdmin; + const hasApprovedAlready = changeRequest.approvals.some( + approval => approval.createdBy.id === user?.id + ); return ( <> @@ -154,10 +157,14 @@ export const ChangeRequestOverview: FC = () => { /> } /> { + const { data } = useChangeRequestConfig(projectId); + + const getChangeRequestRequiredApprovals = React.useCallback( + (environment: string): number => { + const config = data.find(draft => { + return ( + draft.environment === environment && + draft.changeRequestEnabled + ); + }); + + return config?.requiredApprovals || 1; + }, + [data] + ); + + return { + getChangeRequestRequiredApprovals, + }; +}; + const resolveBorder = (state: ChangeRequestState, theme: Theme) => { if (state === 'Approved') { return `2px solid ${theme.palette.success.main}`; @@ -51,9 +76,8 @@ const resolveIconColors = (state: ChangeRequestState, theme: Theme) => { export const ChangeRequestReviewStatus: FC< ISuggestChangeReviewsStatusProps -> = ({ state }) => { +> = ({ state, environment }) => { const theme = useTheme(); - return ( @@ -64,7 +88,7 @@ export const ChangeRequestReviewStatus: FC< /> - + ); @@ -72,9 +96,10 @@ export const ChangeRequestReviewStatus: FC< interface IResolveComponentProps { state: ChangeRequestState; + environment: string; } -const ResolveComponent = ({ state }: IResolveComponentProps) => { +const ResolveComponent = ({ state, environment }: IResolveComponentProps) => { if (!state) { return null; } @@ -91,7 +116,7 @@ const ResolveComponent = ({ state }: IResolveComponentProps) => { return ; } - return ; + return ; }; const Approved = () => { @@ -125,8 +150,16 @@ const Approved = () => { ); }; -const ReviewRequired = () => { +interface IReviewRequiredProps { + environment: string; +} + +const ReviewRequired = ({ environment }: IReviewRequiredProps) => { const theme = useTheme(); + const projectId = useRequiredPathParam('projectId'); + const { getChangeRequestRequiredApprovals } = + useChangeRequestRequiredApprovals(projectId); + const approvals = getChangeRequestRequiredApprovals(environment); return ( <> @@ -137,7 +170,7 @@ const ReviewRequired = () => { Review required - At least 1 approving review must be submitted before + At least {approvals} approvals must be submitted before changes can be applied diff --git a/frontend/src/component/changeRequest/changeRequest.types.ts b/frontend/src/component/changeRequest/changeRequest.types.ts index 64460c7361..3eb076a88c 100644 --- a/frontend/src/component/changeRequest/changeRequest.types.ts +++ b/frontend/src/component/changeRequest/changeRequest.types.ts @@ -18,6 +18,7 @@ export interface IChangeRequestEnvironmentConfig { environment: string; type: string; changeRequestEnabled: boolean; + requiredApprovals: number; } export interface IChangeRequestFeature { diff --git a/frontend/src/component/common/GeneralSelect/GeneralSelect.tsx b/frontend/src/component/common/GeneralSelect/GeneralSelect.tsx index 55457ad96a..f4c3851f03 100644 --- a/frontend/src/component/common/GeneralSelect/GeneralSelect.tsx +++ b/frontend/src/component/common/GeneralSelect/GeneralSelect.tsx @@ -9,12 +9,15 @@ import { } from '@mui/material'; import { SELECT_ITEM_ID } from 'utils/testIds'; import { KeyboardArrowDownOutlined } from '@mui/icons-material'; +import { SxProps } from '@mui/system'; +import { Theme } from '@mui/material/styles'; export interface ISelectOption { key: string; title?: string; label?: string; disabled?: boolean; + sx?: SxProps; } export interface IGeneralSelectProps extends Omit { @@ -68,6 +71,7 @@ const GeneralSelect: React.FC = ({ > {options.map(option => ( ({ + padding: theme.spacing(1), + display: 'flex', + justifyContent: 'center', + '& .MuiInputBase-input': { + fontSize: theme.fontSizes.smallBody, + }, +})); export const ChangeRequestConfiguration: VFC = () => { const [dialogState, setDialogState] = useState<{ isOpen: boolean; - enableEnvironment?: string; + enableEnvironment: string; isEnabled: boolean; + requiredApprovals: number; }>({ isOpen: false, enableEnvironment: '', isEnabled: false, + requiredApprovals: 1, }); + const theme = useTheme(); const projectId = useRequiredPathParam('projectId'); const { data, loading, refetchChangeRequestConfig } = useChangeRequestConfig(projectId); const { updateChangeRequestEnvironmentConfig } = useChangeRequestApi(); const { setToastData, setToastApiError } = useToast(); - const onClick = (enableEnvironment: string, isEnabled: boolean) => () => { - setDialogState({ isOpen: true, enableEnvironment, isEnabled }); - }; + const onRowChange = + ( + enableEnvironment: string, + isEnabled: boolean, + requiredApprovals: number + ) => + () => { + setDialogState({ + isOpen: true, + enableEnvironment, + isEnabled, + requiredApprovals, + }); + }; const onConfirm = async () => { if (dialogState.enableEnvironment) { - try { - await updateChangeRequestEnvironmentConfig( - projectId, - dialogState.enableEnvironment, - !dialogState.isEnabled - ); - setToastData({ - type: 'success', - title: 'Updated change request status', - text: 'Successfully updated change request status.', - }); - refetchChangeRequestConfig(); - } catch (error) { - const message = formatUnknownError(error); - setToastApiError(message); - } + await updateConfiguration(); } setDialogState({ isOpen: false, enableEnvironment: '', isEnabled: false, + requiredApprovals: 1, }); }; + async function updateConfiguration(config?: IChangeRequestConfig) { + try { + await updateChangeRequestEnvironmentConfig( + config || { + project: projectId, + environment: dialogState.enableEnvironment, + enabled: !dialogState.isEnabled, + requiredApprovals: dialogState.requiredApprovals, + } + ); + setToastData({ + type: 'success', + title: 'Updated change request status', + text: 'Successfully updated change request status.', + }); + await refetchChangeRequestConfig(); + } catch (error) { + setToastApiError(formatUnknownError(error)); + } + } + + const approvalOptions = Array.from(Array(10).keys()) + .map(key => String(key + 1)) + .map(key => { + const labelText = key === '1' ? 'approval' : 'approvals'; + return { + key, + label: `${key} ${labelText}`, + sx: { 'font-size': theme.fontSizes.smallBody }, + }; + }); + + function onRequiredApprovalsChange(original: any, approvals: string) { + updateConfiguration({ + project: projectId, + environment: original.environment, + enabled: original.changeRequestEnabled, + requiredApprovals: Number(approvals), + }); + } + const columns = useMemo( () => [ { @@ -82,6 +138,34 @@ export const ChangeRequestConfiguration: VFC = () => { disableGlobalFilter: true, disableSortBy: true, }, + { + Header: 'Required approvals', + align: 'center', + Cell: ({ row: { original } }: any) => ( + + { + onRequiredApprovalsChange( + original, + approvals + ); + }} + IconComponent={KeyboardArrowDownOutlined} + fullWidth + /> + + } + /> + ), + width: 100, + disableGlobalFilter: true, + disableSortBy: true, + }, { Header: 'Status', accessor: 'changeRequestEnabled', @@ -89,22 +173,20 @@ export const ChangeRequestConfiguration: VFC = () => { align: 'center', Cell: ({ value, row: { original } }: any) => ( - + - + ), width: 100, disableGlobalFilter: true, diff --git a/frontend/src/hooks/api/actions/useChangeRequestApi/useChangeRequestApi.ts b/frontend/src/hooks/api/actions/useChangeRequestApi/useChangeRequestApi.ts index 38228ac511..3bab3270ca 100644 --- a/frontend/src/hooks/api/actions/useChangeRequestApi/useChangeRequestApi.ts +++ b/frontend/src/hooks/api/actions/useChangeRequestApi/useChangeRequestApi.ts @@ -10,6 +10,13 @@ export interface IChangeRequestsSchema { payload: string | boolean | object | number; } +export interface IChangeRequestConfig { + project: string; + environment: string; + enabled: boolean; + requiredApprovals: number; +} + export const useChangeRequestApi = () => { const { makeRequest, createRequest, errors, loading } = useAPI({ propagateErrors: true, @@ -67,15 +74,19 @@ export const useChangeRequestApi = () => { } }; - const updateChangeRequestEnvironmentConfig = async ( - project: string, - environment: string, - enabled: boolean - ) => { + const updateChangeRequestEnvironmentConfig = async ({ + project, + enabled, + environment, + requiredApprovals, + }: IChangeRequestConfig) => { const path = `api/admin/projects/${project}/environments/${environment}/change-requests/config`; const req = createRequest(path, { method: 'PUT', - body: JSON.stringify({ changeRequestsEnabled: enabled }), + body: JSON.stringify({ + changeRequestsEnabled: enabled, + requiredApprovals, + }), }); try { diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index a13bf74a8d..b11f7abb35 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -16,8 +16,7 @@ import { ProjectGroupRemovedEvent, ProjectGroupUpdateRoleEvent, } from '../types/events'; -import { IUnleashStores } from '../types'; -import { IUnleashConfig } from '../types/option'; +import { IUnleashStores, IUnleashConfig } from '../types'; import { FeatureToggle, IProject, @@ -195,6 +194,13 @@ export default class ProjectService { ); } + async addEnvironmentToProject( + project: string, + environment: string, + ): Promise { + await this.store.addEnvironmentToProject(project, environment); + } + async changeProject( newProjectId: string, featureName: string, diff --git a/src/migrations/20221115072335-add-required-approvals.js b/src/migrations/20221115072335-add-required-approvals.js new file mode 100644 index 0000000000..6744f03c8c --- /dev/null +++ b/src/migrations/20221115072335-add-required-approvals.js @@ -0,0 +1,25 @@ +'use strict'; + +exports.up = function (db, callback) { + db.runSql( + ` + ALTER TABLE change_request_settings ADD COLUMN required_approvals integer default 1; + + ALTER TABLE change_request_settings + ADD CONSTRAINT change_request_settings_project_environment_key + UNIQUE (project, environment) + `, + callback, + ); +}; + +exports.down = function (db, callback) { + db.runSql( + ` + ALTER TABLE change_request_settings DROP COLUMN IF EXISTS required_approvals; + ALTER TABLE change_request_settings + DROP CONSTRAINT IF EXISTS change_request_settings_project_environment_key; + `, + callback, + ); +};