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

@@ -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);
+  });
+});

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,
     );
   }
 </script>
@@ -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}
                   </div>
                   <PlanMergeRequestStatusBadge status={planMergeRequest.status} />
@@ -223,13 +230,15 @@
                       {planMergeRequest.type === 'outgoing' ? 'Withdraw' : 'Begin Review'}
                     {/if}
                   </button>
-                {:else if planMergeRequest.status === 'in-progress'}
-                  <button
-                    on:click={() => goto(`${base}/plans/${planMergeRequest.plan_receiving_changes.id}/merge`)}
-                    class="st-button secondary"
-                  >
-                    {planMergeRequest.type === 'incoming' ? 'Review' : 'View Merge Request'}
-                  </button>
+                {:else if planMergeRequest.status === 'in-progress' && planMergeRequest.plan_snapshot_supplying_changes.plan}
+                  {#if typeof planMergeRequest.plan_receiving_changes?.id === 'number'}
+                    <button
+                      on:click={() => goto(`${base}/plans/${planMergeRequest.plan_receiving_changes?.id}/merge`)}
+                      class="st-button secondary"
+                    >
+                      {planMergeRequest.type === 'incoming' ? 'Review' : 'View Merge Request'}
+                    </button>
+                  {/if}
 
                   {#if planMergeRequest.type === 'outgoing'}
                     <div

src/components/plan/PlanMergeReview.svelte

@@ -80,14 +80,14 @@
   let selectedNonConflictingActivity: PlanMergeNonConflictingActivity | null;
   let unresolvedConflictsCount: number = 0;
   let userInitiatedMergeRequestResolution: boolean = false;
-  let sourcePlan: PlanForMerging;
+  let sourcePlan: PlanForMerging | undefined;
   let targetPlan: PlanForMerging;
 
-  $: if (initialPlan && initialMergeRequest) {
-    sourcePlan = initialMergeRequest.plan_snapshot_supplying_changes.plan;
+  $: if (initialPlan && initialMergeRequest && initialMergeRequest.plan_receiving_changes) {
+    sourcePlan = initialMergeRequest.plan_snapshot_supplying_changes?.plan;
     targetPlan = initialMergeRequest.plan_receiving_changes;
 
-    const { id: supplyingPlanId } = sourcePlan;
+    let supplyingPlanId = sourcePlan?.id ?? -1;
 
     hasReviewPermission = featurePermissions.planBranch.canReviewRequest(
       user,
@@ -294,7 +294,7 @@
   }
 
   async function onApproveChanges() {
-    if (initialMergeRequest !== null) {
+    if (initialMergeRequest !== null && initialMergeRequest.plan_receiving_changes) {
       const success = await effects.planMergeCommit(
         initialMergeRequest.id,
         initialMergeRequest.plan_snapshot_supplying_changes.plan,
@@ -310,7 +310,7 @@
   }
 
   async function onDenyChanges() {
-    if (initialMergeRequest !== null) {
+    if (initialMergeRequest !== null && initialMergeRequest.plan_receiving_changes) {
       const success = await effects.planMergeDeny(
         initialMergeRequest.id,
         initialMergeRequest.plan_snapshot_supplying_changes.plan,
@@ -326,7 +326,7 @@
   }
 
   async function onCancel() {
-    if (initialMergeRequest !== null) {
+    if (initialMergeRequest !== null && initialMergeRequest.plan_receiving_changes) {
       const success = await effects.planMergeCancel(
         initialMergeRequest.id,
         initialMergeRequest.plan_snapshot_supplying_changes.plan,
@@ -344,7 +344,7 @@
   function onResolveAll(e: Event) {
     const { value } = getTarget(e);
     const resolution = value as PlanMergeResolution;
-    if (initialMergeRequest !== null) {
+    if (initialMergeRequest !== null && initialMergeRequest.plan_receiving_changes) {
       effects.planMergeResolveAllConflicts(
         initialMergeRequest.id,
         resolution,
@@ -397,7 +397,7 @@
   }
 
   async function resolveConflict(activityId: number, resolution: PlanMergeResolution) {
-    if (initialMergeRequest !== null) {
+    if (initialMergeRequest !== null && initialMergeRequest.plan_receiving_changes) {
       await effects.planMergeResolveConflict(
         initialMergeRequest.id,
         activityId,
@@ -423,12 +423,12 @@
   <Nav {user}>
     <span class="" slot="title"
       >Merge Review:
-      <a href={`${base}/plans/${initialMergeRequest?.plan_receiving_changes.id}`} class="link">
-        {initialMergeRequest?.plan_receiving_changes.name}
+      <a href={`${base}/plans/${initialMergeRequest?.plan_receiving_changes?.id}`} class="link">
+        {initialMergeRequest?.plan_receiving_changes?.name}
       </a>
       from
-      <a href={`${base}/plans/${initialMergeRequest?.plan_snapshot_supplying_changes.plan.id}`} class="link">
-        {initialMergeRequest?.plan_snapshot_supplying_changes.plan.name}
+      <a href={`${base}/plans/${initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.id}`} class="link">
+        {initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'}
       </a>
     </span>
   </Nav>
@@ -449,14 +449,14 @@
           <div class="st-typography-medium">Current Branch (Target)</div>
           <div class="merge-review-branch-metadata-content st-typography-body">
             <MergeIcon />
-            {initialMergeRequest?.plan_receiving_changes.name}
+            {initialMergeRequest?.plan_receiving_changes?.name}
           </div>
         </div>
         <div class="merge-review-branch-metadata">
           <div class="st-typography-medium">Source Branch</div>
           <div class="merge-review-branch-metadata-content st-typography-body">
             <PlanWithUpArrow />
-            {initialMergeRequest?.plan_snapshot_supplying_changes.plan.name}
+            {initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'}
           </div>
         </div>
         <div class="merge-review-stats">
@@ -594,7 +594,9 @@
         <div class="merge-review-subheader">
           <span style="gap: 8px">
             <PlanWithUpArrow />
-            <span class="st-typography-medium">{initialMergeRequest?.plan_snapshot_supplying_changes.plan.name}</span>
+            <span class="st-typography-medium"
+              >{initialMergeRequest?.plan_snapshot_supplying_changes?.plan?.name ?? 'Deleted Plan'}</span
+            >
           </span>
           <span class="section-title st-typography-medium">Source</span>
         </div>
@@ -660,7 +662,7 @@
         <div class="merge-review-subheader">
           <span style="gap: 8px">
             <MergeIcon />
-            <span class="st-typography-medium">{initialMergeRequest?.plan_receiving_changes.name}</span>
+            <span class="st-typography-medium">{initialMergeRequest?.plan_receiving_changes?.name}</span>
           </span>
           <span class="section-title st-typography-medium">Current Branch (Target)</span>
         </div>

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[] = [];
 

src/types/plan.ts

@@ -59,9 +59,9 @@ export type PlanForMerging = Pick<PlanSchema, 'id' | 'name' | 'owner' | 'collabo
 
 export type PlanMergeRequestSchema = {
   id: number;
-  plan_receiving_changes: PlanForMerging;
+  plan_receiving_changes?: PlanForMerging;
   plan_snapshot_supplying_changes: {
-    plan: PlanForMerging;
+    plan?: PlanForMerging;
     snapshot_id: number;
   };
   requester_username: string;

src/utilities/effects.ts

@@ -4981,12 +4981,12 @@ const effects = {
 
   async planMergeBegin(
     merge_request_id: number,
-    sourcePlan: PlanForMerging,
+    sourcePlan: PlanForMerging | undefined,
     targetPlan: PlanForMerging,
     user: User | null,
   ): Promise<boolean> {
     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<boolean> {
     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<boolean> {
     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<boolean> {
     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<boolean> {
     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<void> {
     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<void> {
     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');
       }