From b07bee733d3c025e0880d4cfa6609dbd8fb74527 Mon Sep 17 00:00:00 2001 From: Aaron Plave Date: Tue, 5 Nov 2024 17:07:21 -0800 Subject: [PATCH] Handle nullable receiving and supplying plans for merge reviews (#1508) * Handle nullable receiving and supplying plans for merge reviews * Update permissions for plan merge when source plan is undefined * More handling for undefined source plans * Allow users to begin merge request even if the supplying plan has been deleted * Add e2e test --- e2e-tests/tests/plan-merge.test.ts | 61 ++++++++++++++++++ .../modals/PlanMergeRequestsModal.svelte | 39 +++++++----- src/components/plan/PlanMergeReview.svelte | 36 ++++++----- src/routes/plans/[id]/merge/+page.ts | 1 + src/types/plan.ts | 4 +- src/utilities/effects.ts | 26 ++++---- src/utilities/permissions.ts | 62 ++++++++++--------- 7 files changed, 153 insertions(+), 76 deletions(-) diff --git a/e2e-tests/tests/plan-merge.test.ts b/e2e-tests/tests/plan-merge.test.ts index 69f7794193..f6253bb238 100644 --- a/e2e-tests/tests/plan-merge.test.ts +++ b/e2e-tests/tests/plan-merge.test.ts @@ -98,3 +98,64 @@ test.describe.serial('Plan Merge', () => { await expect(page.locator('input[name="start-time"]')).toHaveValue(newActivityStartTime); }); }); + +test.describe.serial('Plan Merge with Deleted Source Plan', () => { + const newActivityStartTime: string = '2022-005T00:00:00'; + const planBranchName = uniqueNamesGenerator({ dictionaries: [adjectives, colors, animals] }); + + test('Add an activity to the parent plan', async () => { + await plan.addActivity('GrowBanana'); + }); + + test('Create a branch', async ({ baseURL }) => { + await plan.createBranch(baseURL, planBranchName); + }); + + test('Change the start time of the activity on the branch', async () => { + await page.waitForTimeout(2000); + const row = await page.getByRole('row', { name: 'GrowBanana' }); + await row.waitFor({ state: 'visible' }); + await row.first().click(); + await page.waitForSelector('.activity-header-title-edit-button:has-text("GrowBanana")', { + state: 'visible', + }); + await page.locator('input[name="start-time"]').click({ position: { x: 2, y: 2 } }); + await page.locator('input[name="start-time"]').fill(newActivityStartTime); + await page.locator('input[name="start-time"]').press('Enter'); + await plan.waitForToast('Activity Directive Updated Successfully'); + }); + + test('Create a merge request from branch to parent plan', async () => { + await page.getByText(planBranchName).first().click(); + await page.getByText('Create merge request').click(); + await page.getByRole('button', { name: 'Create Merge Request' }).click(); + await plan.waitForToast('Merge Request Created Successfully'); + }); + + test('Delete source plan', async () => { + await plans.goto(); + await plans.deletePlan(planBranchName); + }); + + test('Switch to parent plan', async () => { + await plan.goto(); + }); + + test('Start a merge review', async ({ baseURL }) => { + await page.getByRole('button', { name: '1 incoming, 0 outgoing' }).click(); + await page.getByRole('button', { name: 'Review' }).click(); + await page.waitForURL(`${baseURL}/plans/*/merge`); + await page.waitForTimeout(250); + }); + + test('Complete the merge review', async ({ baseURL }) => { + await page.getByRole('button', { name: 'Approve Changes' }).click(); + await page.waitForURL(`${baseURL}/plans/${plans.planId}/merge`); + await page.waitForTimeout(250); + }); + + test('Make sure the start time of the activity in the parent plan now equals the start time of the activity in branch', async () => { + await page.getByRole('gridcell', { name: 'GrowBanana' }).first().click(); + await expect(page.locator('input[name="start-time"]')).toHaveValue(newActivityStartTime); + }); +}); diff --git a/src/components/modals/PlanMergeRequestsModal.svelte b/src/components/modals/PlanMergeRequestsModal.svelte index 17fd335fbe..8df48bcb6b 100644 --- a/src/components/modals/PlanMergeRequestsModal.svelte +++ b/src/components/modals/PlanMergeRequestsModal.svelte @@ -77,7 +77,7 @@ } async function onReviewOrWithdraw(planMergeRequest: PlanMergeRequest) { - if (planMergeRequest.type === 'incoming') { + if (planMergeRequest.type === 'incoming' && planMergeRequest.plan_receiving_changes) { planMergeRequest.pending = true; filteredPlanMergeRequests = [...filteredPlanMergeRequests]; const success = await effects.planMergeBegin( @@ -90,7 +90,7 @@ dispatch('close'); goto(`${base}/plans/${planMergeRequest.plan_receiving_changes.id}/merge`); } - } else if (planMergeRequest.type === 'outgoing') { + } else if (planMergeRequest.type === 'outgoing' && planMergeRequest.plan_snapshot_supplying_changes.plan) { await effects.planMergeRequestWithdraw( planMergeRequest.id, planMergeRequest.plan_snapshot_supplying_changes.plan, @@ -117,19 +117,26 @@ } function hasPermission(planMergeRequest: PlanMergeRequest) { + const model = + planMergeRequest.plan_snapshot_supplying_changes.plan?.model || planMergeRequest.plan_receiving_changes?.model; + + if (!model) { + return false; + } + if (planMergeRequest.type === 'outgoing') { return featurePermissions.planBranch.canDeleteRequest( user, planMergeRequest.plan_snapshot_supplying_changes.plan, planMergeRequest.plan_receiving_changes, - planMergeRequest.plan_snapshot_supplying_changes.plan.model, + model, ); } return featurePermissions.planBranch.canReviewRequest( user, planMergeRequest.plan_snapshot_supplying_changes.plan, planMergeRequest.plan_receiving_changes, - planMergeRequest.plan_snapshot_supplying_changes.plan.model, + model, ); } @@ -190,15 +197,15 @@ use:tooltip={{ content: planMergeRequest.type === 'incoming' - ? planMergeRequest.plan_snapshot_supplying_changes.plan.name - : planMergeRequest.plan_receiving_changes.name, + ? planMergeRequest.plan_snapshot_supplying_changes.plan?.name + : planMergeRequest.plan_receiving_changes?.name || 'Deleted Plan', placement: 'top', }} > {#if planMergeRequest.type === 'incoming'} - {planMergeRequest.plan_snapshot_supplying_changes.plan.name} + {planMergeRequest.plan_snapshot_supplying_changes.plan?.name || 'Deleted Plan'} {:else if planMergeRequest.type === 'outgoing'} - {planMergeRequest.plan_receiving_changes.name} + {planMergeRequest.plan_receiving_changes?.name || 'Deleted Plan'} {/if} @@ -223,13 +230,15 @@ {planMergeRequest.type === 'outgoing' ? 'Withdraw' : 'Begin Review'} {/if} - {:else if planMergeRequest.status === 'in-progress'} - + {:else if planMergeRequest.status === 'in-progress' && planMergeRequest.plan_snapshot_supplying_changes.plan} + {#if typeof planMergeRequest.plan_receiving_changes?.id === 'number'} + + {/if} {#if planMergeRequest.type === 'outgoing'}
Merge Review: - - {initialMergeRequest?.plan_receiving_changes.name} + + {initialMergeRequest?.plan_receiving_changes?.name} from - - {initialMergeRequest?.plan_snapshot_supplying_changes.plan.name} + + {initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'} @@ -449,14 +449,14 @@
Current Branch (Target)
- {initialMergeRequest?.plan_receiving_changes.name} + {initialMergeRequest?.plan_receiving_changes?.name}
Source Branch
- {initialMergeRequest?.plan_snapshot_supplying_changes.plan.name} + {initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'}
@@ -594,7 +594,9 @@
- {initialMergeRequest?.plan_snapshot_supplying_changes.plan.name} + {initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'} Source
@@ -660,7 +662,7 @@
- {initialMergeRequest?.plan_receiving_changes.name} + {initialMergeRequest?.plan_receiving_changes?.name} Current Branch (Target)
diff --git a/src/routes/plans/[id]/merge/+page.ts b/src/routes/plans/[id]/merge/+page.ts index 05ea25b4dd..53570f177c 100644 --- a/src/routes/plans/[id]/merge/+page.ts +++ b/src/routes/plans/[id]/merge/+page.ts @@ -26,6 +26,7 @@ export const load: PageLoad = async ({ parent, params }) => { planId, user, ); + let initialConflictingActivities: PlanMergeConflictingActivity[] = []; let initialNonConflictingActivities: PlanMergeNonConflictingActivity[] = []; diff --git a/src/types/plan.ts b/src/types/plan.ts index a77f573168..58050c520d 100644 --- a/src/types/plan.ts +++ b/src/types/plan.ts @@ -59,9 +59,9 @@ export type PlanForMerging = Pick { try { - if (!queryPermissions.PLAN_MERGE_BEGIN(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_BEGIN(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('begin a merge'); } @@ -5005,12 +5005,12 @@ const effects = { async planMergeCancel( merge_request_id: number, - sourcePlan: PlanForMerging, + sourcePlan: PlanForMerging | undefined, targetPlan: PlanForMerging, user: User | null, ): Promise { try { - if (!queryPermissions.PLAN_MERGE_CANCEL(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_CANCEL(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('cancel this merge request'); } @@ -5030,12 +5030,12 @@ const effects = { async planMergeCommit( merge_request_id: number, - sourcePlan: PlanForMerging, + sourcePlan: PlanForMerging | undefined, targetPlan: PlanForMerging, user: User | null, ): Promise { try { - if (!queryPermissions.PLAN_MERGE_COMMIT(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_COMMIT(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('approve this merge request'); } @@ -5055,12 +5055,12 @@ const effects = { async planMergeDeny( merge_request_id: number, - sourcePlan: PlanForMerging, + sourcePlan: PlanForMerging | undefined, targetPlan: PlanForMerging, user: User | null, ): Promise { try { - if (!queryPermissions.PLAN_MERGE_DENY(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_DENY(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('deny this merge request'); } @@ -5081,7 +5081,7 @@ const effects = { async planMergeRequestWithdraw( merge_request_id: number, sourcePlan: PlanForMerging, - targetPlan: PlanForMerging, + targetPlan: PlanForMerging | undefined, user: User | null, ): Promise { try { @@ -5110,12 +5110,12 @@ const effects = { async planMergeResolveAllConflicts( merge_request_id: number, resolution: PlanMergeResolution, - sourcePlan: PlanForMerging, + sourcePlan: PlanForMerging | undefined, targetPlan: PlanForMerging, user: User | null, ): Promise { try { - if (!queryPermissions.PLAN_MERGE_RESOLVE_ALL_CONFLICTS(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_RESOLVE_ALL_CONFLICTS(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('resolve merge request conflicts'); } @@ -5133,12 +5133,12 @@ const effects = { merge_request_id: number, activity_id: ActivityDirectiveId, resolution: PlanMergeResolution, - sourcePlan: PlanForMerging, + sourcePlan: PlanForMerging | undefined, targetPlan: PlanForMerging, user: User | null, ): Promise { try { - if (!queryPermissions.PLAN_MERGE_RESOLVE_CONFLICT(user, sourcePlan, targetPlan, sourcePlan.model)) { + if (!queryPermissions.PLAN_MERGE_RESOLVE_CONFLICT(user, sourcePlan, targetPlan, targetPlan.model)) { throwPermissionError('resolve merge request conflicts'); } diff --git a/src/utilities/permissions.ts b/src/utilities/permissions.ts index ec037df7c2..1a0fa57992 100644 --- a/src/utilities/permissions.ts +++ b/src/utilities/permissions.ts @@ -61,7 +61,7 @@ function isUserAuthor(user: User | null, thingWithAuthor?: AssetWithAuthor | nul return false; } -function isPlanOwner(user: User | null, plan: AssetWithOwner): boolean { +function isPlanOwner(user: User | null, plan: AssetWithOwner | undefined): boolean { return isUserOwner(user, plan); } @@ -210,8 +210,8 @@ function getRolePlanPermission( function getRolePlanBranchPermission( queries: string[], user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: Pick, ): boolean { if (user && user.rolePermissions) { @@ -220,41 +220,45 @@ function getRolePlanBranchPermission( if (user.rolePermissions != null) { switch (user.rolePermissions[getFunctionPermission(queryName)]) { case 'OWNER': - permission = isPlanOwner(user, sourcePlan) && isPlanOwner(user, targetPlan); + permission = + (!sourcePlan || isPlanOwner(user, sourcePlan)) && (!targetPlan || isPlanOwner(user, targetPlan)); break; case 'MISSION_MODEL_OWNER': permission = isUserOwner(user, model); break; case 'PLAN_OWNER': - permission = isPlanOwner(user, sourcePlan) || isPlanOwner(user, targetPlan); + permission = + (!!sourcePlan && isPlanOwner(user, sourcePlan)) || (!!targetPlan && isPlanOwner(user, targetPlan)); break; case 'PLAN_COLLABORATOR': - permission = isPlanCollaborator(user, sourcePlan) || isPlanCollaborator(user, targetPlan); + permission = + (!!sourcePlan && isPlanCollaborator(user, sourcePlan)) || + (!!targetPlan && isPlanCollaborator(user, targetPlan)); break; case 'PLAN_OWNER_COLLABORATOR': permission = - isPlanOwner(user, sourcePlan) || - isPlanCollaborator(user, sourcePlan) || - isPlanOwner(user, targetPlan) || - isPlanCollaborator(user, targetPlan); + (!!sourcePlan && isPlanOwner(user, sourcePlan)) || + (!!sourcePlan && isPlanCollaborator(user, sourcePlan)) || + (!!targetPlan && isPlanOwner(user, targetPlan)) || + (!!targetPlan && isPlanCollaborator(user, targetPlan)); break; case 'PLAN_OWNER_SOURCE': - permission = isPlanOwner(user, sourcePlan); + permission = !!sourcePlan && isPlanOwner(user, sourcePlan); break; case 'PLAN_COLLABORATOR_SOURCE': - permission = isPlanCollaborator(user, sourcePlan); + permission = !!sourcePlan && isPlanCollaborator(user, sourcePlan); break; case 'PLAN_OWNER_COLLABORATOR_SOURCE': - permission = isPlanOwner(user, sourcePlan) || isPlanCollaborator(user, sourcePlan); + permission = !!sourcePlan && (isPlanOwner(user, sourcePlan) || isPlanCollaborator(user, sourcePlan)); break; case 'PLAN_OWNER_TARGET': - permission = isPlanOwner(user, targetPlan); + permission = !!targetPlan && isPlanOwner(user, targetPlan); break; case 'PLAN_COLLABORATOR_TARGET': - permission = isPlanCollaborator(user, targetPlan); + permission = !!targetPlan && isPlanCollaborator(user, targetPlan); break; case 'PLAN_OWNER_COLLABORATOR_TARGET': - permission = isPlanOwner(user, targetPlan) || isPlanCollaborator(user, targetPlan); + permission = !!targetPlan && (isPlanOwner(user, targetPlan) || isPlanCollaborator(user, targetPlan)); break; case 'NO_CHECK': default: @@ -403,7 +407,7 @@ const queryPermissions: Record b }, CREATE_PLAN_MERGE_REQUEST: ( user: User | null, - sourcePlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, targetPlan: PlanWithOwners, model: ModelWithOwner, ): boolean => { @@ -725,8 +729,8 @@ const queryPermissions: Record b }, PLAN_MERGE_BEGIN: ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ): boolean => { const queries = [Queries.BEGIN_MERGE]; @@ -737,8 +741,8 @@ const queryPermissions: Record b }, PLAN_MERGE_CANCEL: ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ): boolean => { const queries = [Queries.CANCEL_MERGE]; @@ -749,8 +753,8 @@ const queryPermissions: Record b }, PLAN_MERGE_COMMIT: ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ): boolean => { const queries = [Queries.COMMIT_MERGE]; @@ -761,8 +765,8 @@ const queryPermissions: Record b }, PLAN_MERGE_DENY: ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ): boolean => { const queries = [Queries.DENY_MERGE]; @@ -773,8 +777,8 @@ const queryPermissions: Record b }, PLAN_MERGE_REQUEST_WITHDRAW: ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ): boolean => { const queries = [Queries.WITHDRAW_MERGE_REQUEST]; @@ -1194,8 +1198,8 @@ type RolePlanPermissionCheckWithAsset = ( type RolePlanBranchPermissionCheck = ( user: User | null, - sourcePlan: PlanWithOwners, - targetPlan: PlanWithOwners, + sourcePlan: PlanWithOwners | undefined, + targetPlan: PlanWithOwners | undefined, model: ModelWithOwner, ) => boolean;