From 32a110e0ca8487946ee240a24461014778ae868d Mon Sep 17 00:00:00 2001
From: Daniel Hougaard <62331820+DanielHougaard@users.noreply.github.com>
Date: Thu, 4 Apr 2024 12:44:49 -0700
Subject: [PATCH] Fix: Multiple approvers acceptance bug

---
 .../access-approval-request-service.ts        | 40 +++++++++++--------
 .../AccessApprovalRequest.tsx                 | 32 ++++++++++-----
 2 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts
index 0c170c40f..b7b61fd8e 100644
--- a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts
+++ b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts
@@ -274,8 +274,8 @@ export const accessApprovalRequestServiceFactory = ({
 
         const approvedReviews = allReviews.filter((r) => r.status === ApprovalStatus.APPROVED);
 
-        // If all approvers have approved the request, update the privilege to approved
-        if (approvedReviews.length === policy.approvers.length) {
+        // approvals is the required number of approvals. If the number of approved reviews is equal to the number of required approvals, then the request is approved.
+        if (approvedReviews.length === policy.approvals) {
           if (accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
             throw new BadRequestError({ message: "Temporary range is required for temporary access" });
           }
@@ -284,27 +284,33 @@ export const accessApprovalRequestServiceFactory = ({
 
           if (!accessApprovalRequest.isTemporary && !accessApprovalRequest.temporaryRange) {
             // Permanent access
-            const privilege = await additionalPrivilegeDAL.create({
-              projectMembershipId: accessApprovalRequest.requestedBy,
-              slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
-              permissions: JSON.stringify(accessApprovalRequest.permissions)
-            });
+            const privilege = await additionalPrivilegeDAL.create(
+              {
+                projectMembershipId: accessApprovalRequest.requestedBy,
+                slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
+                permissions: JSON.stringify(accessApprovalRequest.permissions)
+              },
+              tx
+            );
             privilegeId = privilege.id;
           } else {
             // Temporary access
             const relativeTempAllocatedTimeInMs = ms(accessApprovalRequest.temporaryRange!);
             const startTime = new Date();
 
-            const privilege = await additionalPrivilegeDAL.create({
-              projectMembershipId: accessApprovalRequest.requestedBy,
-              slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
-              permissions: JSON.stringify(accessApprovalRequest.permissions),
-              isTemporary: true,
-              temporaryMode: ProjectUserAdditionalPrivilegeTemporaryMode.Relative,
-              temporaryRange: accessApprovalRequest.temporaryRange!,
-              temporaryAccessStartTime: startTime,
-              temporaryAccessEndTime: new Date(new Date(startTime).getTime() + relativeTempAllocatedTimeInMs)
-            });
+            const privilege = await additionalPrivilegeDAL.create(
+              {
+                projectMembershipId: accessApprovalRequest.requestedBy,
+                slug: `requested-privilege-${slugify(alphaNumericNanoId(12))}`,
+                permissions: JSON.stringify(accessApprovalRequest.permissions),
+                isTemporary: true,
+                temporaryMode: ProjectUserAdditionalPrivilegeTemporaryMode.Relative,
+                temporaryRange: accessApprovalRequest.temporaryRange!,
+                temporaryAccessStartTime: startTime,
+                temporaryAccessEndTime: new Date(new Date(startTime).getTime() + relativeTempAllocatedTimeInMs)
+              },
+              tx
+            );
             privilegeId = privilege.id;
           }
 
diff --git a/frontend/src/views/SecretApprovalPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx b/frontend/src/views/SecretApprovalPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
index b072513dc..2421a629d 100644
--- a/frontend/src/views/SecretApprovalPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
+++ b/frontend/src/views/SecretApprovalPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
@@ -342,12 +342,14 @@ export const AccessApprovalRequest = ({
       displayData = { label: "Access Granted", colorClass: "bg-green/20 text-green" };
     else if (isRejectedByAnyone)
       displayData = { label: "Rejected", colorClass: "bg-red/20 text-red" };
-    else if (userReviewStatus === ApprovalStatus.APPROVED)
+    else if (userReviewStatus === ApprovalStatus.APPROVED) {
       displayData = {
-        label: `Pending ${request.policy.approvals - request.reviewers.length} reviews`,
+        label: `Pending ${request.policy.approvals - request.reviewers.length} review${
+          request.policy.approvals - request.reviewers.length > 1 ? "s" : ""
+        }`,
         colorClass: "bg-yellow/20 text-yellow"
       };
-    else if (!isReviewedByUser)
+    } else if (!isReviewedByUser)
       displayData = {
         label: "Review Required",
         colorClass: "bg-yellow/20 text-yellow"
@@ -499,14 +501,21 @@ export const AccessApprovalRequest = ({
 
                 return (
                   <div
-                    aria-disabled={details.isReviewedByUser || details.isRejectedByAnyone}
+                    aria-disabled={
+                      details.isReviewedByUser || details.isRejectedByAnyone || details.isAccepted
+                    }
                     key={request.id}
                     className="flex w-full cursor-pointer px-8 py-4 hover:bg-mineshaft-700 aria-disabled:opacity-80"
                     role="button"
                     tabIndex={0}
                     onClick={() => {
-                      if (!details.isApprover) return;
-                      if (details.isReviewedByUser || details.isRejectedByAnyone) return;
+                      if (
+                        !details.isApprover ||
+                        details.isReviewedByUser ||
+                        details.isRejectedByAnyone ||
+                        details.isAccepted
+                      )
+                        return;
 
                       setSelectedRequest({
                         ...request,
@@ -515,8 +524,13 @@ export const AccessApprovalRequest = ({
                       handlePopUpOpen("reviewRequest");
                     }}
                     onKeyDown={(evt) => {
-                      if (!details.isApprover) return;
-                      if (details.isReviewedByUser || details.isRejectedByAnyone) return;
+                      if (
+                        !details.isApprover ||
+                        details.isAccepted ||
+                        details.isReviewedByUser ||
+                        details.isRejectedByAnyone
+                      )
+                        return;
                       if (evt.key === "Enter") {
                         setSelectedRequest({
                           ...request,
@@ -543,7 +557,7 @@ export const AccessApprovalRequest = ({
                               </>
                             )}
                           </div>
-                          <div className="">
+                          <div>
                             {details.isApprover && (
                               <DisplayBadge
                                 text={details.displayData.label}