From b62fa443d35e225150ba34317b41b9a85b2eafe1 Mon Sep 17 00:00:00 2001
From: Aaron Plave <aaron.plave@jpl.nasa.gov>
Date: Thu, 24 Oct 2024 17:07:38 -0700
Subject: [PATCH] Allow users to begin merge request even if the supplying plan
 has been deleted

---
 .../modals/PlanMergeRequestsModal.svelte      | 19 ++++----
 src/utilities/effects.ts                      |  6 +--
 src/utilities/permissions.ts                  | 44 ++++++++++---------
 3 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/src/components/modals/PlanMergeRequestsModal.svelte b/src/components/modals/PlanMergeRequestsModal.svelte
index a8b65a9052..d9227d4cc4 100644
--- a/src/components/modals/PlanMergeRequestsModal.svelte
+++ b/src/components/modals/PlanMergeRequestsModal.svelte
@@ -77,11 +77,7 @@
   }
 
   async function onReviewOrWithdraw(planMergeRequest: PlanMergeRequest) {
-    if (!planMergeRequest.plan_snapshot_supplying_changes.plan || !planMergeRequest.plan_receiving_changes) {
-      return;
-    }
-
-    if (planMergeRequest.type === 'incoming') {
+    if (planMergeRequest.type === 'incoming' && planMergeRequest.plan_receiving_changes) {
       planMergeRequest.pending = true;
       filteredPlanMergeRequests = [...filteredPlanMergeRequests];
       const success = await effects.planMergeBegin(
@@ -94,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,
@@ -121,7 +117,10 @@
   }
 
   function hasPermission(planMergeRequest: PlanMergeRequest) {
-    if (!planMergeRequest.plan_snapshot_supplying_changes.plan || !planMergeRequest.plan_receiving_changes) {
+    const model =
+      planMergeRequest.plan_snapshot_supplying_changes.plan?.model || planMergeRequest.plan_receiving_changes?.model;
+
+    if (!model) {
       return false;
     }
 
@@ -130,14 +129,14 @@
         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>
@@ -212,7 +211,7 @@
                   <PlanMergeRequestStatusBadge status={planMergeRequest.status} />
                 </div>
 
-                {#if planMergeRequest.status === 'pending' && planMergeRequest.plan_snapshot_supplying_changes.plan}
+                {#if planMergeRequest.status === 'pending'}
                   <button
                     on:click={() => onReviewOrWithdraw(planMergeRequest)}
                     class="st-button secondary"
diff --git a/src/utilities/effects.ts b/src/utilities/effects.ts
index 4302a30732..e6ca931767 100644
--- a/src/utilities/effects.ts
+++ b/src/utilities/effects.ts
@@ -4980,12 +4980,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');
       }
 
@@ -5080,7 +5080,7 @@ const effects = {
   async planMergeRequestWithdraw(
     merge_request_id: number,
     sourcePlan: PlanForMerging,
-    targetPlan: PlanForMerging,
+    targetPlan: PlanForMerging | undefined,
     user: User | null,
   ): Promise<boolean> {
     try {
diff --git a/src/utilities/permissions.ts b/src/utilities/permissions.ts
index a2dd03c49c..1a0fa57992 100644
--- a/src/utilities/permissions.ts
+++ b/src/utilities/permissions.ts
@@ -211,7 +211,7 @@ function getRolePlanBranchPermission(
   queries: string[],
   user: User | null,
   sourcePlan: PlanWithOwners | undefined,
-  targetPlan: PlanWithOwners,
+  targetPlan: PlanWithOwners | undefined,
   model: Pick<Model, 'owner'>,
 ): boolean {
   if (user && user.rolePermissions) {
@@ -220,23 +220,27 @@ function getRolePlanBranchPermission(
       if (user.rolePermissions != null) {
         switch (user.rolePermissions[getFunctionPermission(queryName)]) {
           case 'OWNER':
-            permission = (!sourcePlan || 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 = (sourcePlan && isPlanOwner(user, sourcePlan)) || isPlanOwner(user, targetPlan);
+            permission =
+              (!!sourcePlan && isPlanOwner(user, sourcePlan)) || (!!targetPlan && isPlanOwner(user, targetPlan));
             break;
           case 'PLAN_COLLABORATOR':
-            permission = (sourcePlan && isPlanCollaborator(user, sourcePlan)) || isPlanCollaborator(user, targetPlan);
+            permission =
+              (!!sourcePlan && isPlanCollaborator(user, sourcePlan)) ||
+              (!!targetPlan && isPlanCollaborator(user, targetPlan));
             break;
           case 'PLAN_OWNER_COLLABORATOR':
             permission =
-              (sourcePlan && isPlanOwner(user, sourcePlan)) ||
-              (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 = !!sourcePlan && isPlanOwner(user, sourcePlan);
@@ -248,13 +252,13 @@ function getRolePlanBranchPermission(
             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:
@@ -725,8 +729,8 @@ const queryPermissions: Record<GQLKeys, (user: User | null, ...args: any[]) => 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];
@@ -738,7 +742,7 @@ const queryPermissions: Record<GQLKeys, (user: User | null, ...args: any[]) => b
   PLAN_MERGE_CANCEL: (
     user: User | null,
     sourcePlan: PlanWithOwners | undefined,
-    targetPlan: PlanWithOwners,
+    targetPlan: PlanWithOwners | undefined,
     model: ModelWithOwner,
   ): boolean => {
     const queries = [Queries.CANCEL_MERGE];
@@ -750,7 +754,7 @@ const queryPermissions: Record<GQLKeys, (user: User | null, ...args: any[]) => b
   PLAN_MERGE_COMMIT: (
     user: User | null,
     sourcePlan: PlanWithOwners | undefined,
-    targetPlan: PlanWithOwners,
+    targetPlan: PlanWithOwners | undefined,
     model: ModelWithOwner,
   ): boolean => {
     const queries = [Queries.COMMIT_MERGE];
@@ -761,8 +765,8 @@ const queryPermissions: Record<GQLKeys, (user: User | null, ...args: any[]) => 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<GQLKeys, (user: User | null, ...args: any[]) => 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];
@@ -1195,7 +1199,7 @@ type RolePlanPermissionCheckWithAsset<T = AssetWithOwner> = (
 type RolePlanBranchPermissionCheck = (
   user: User | null,
   sourcePlan: PlanWithOwners | undefined,
-  targetPlan: PlanWithOwners,
+  targetPlan: PlanWithOwners | undefined,
   model: ModelWithOwner,
 ) => boolean;