improvements: address feedback and fix bugs

2026-09-22 13:39:35 +00:00 · 2025-06-20 12:25:28 -07:00
parent 7e9743b4c2
commit 7c7af347fc
10 changed files with 146 additions and 86 deletions
--- a/backend/src/ee/routes/v1/access-approval-request-router.ts
+++ b/backend/src/ee/routes/v1/access-approval-request-router.ts
@@ -89,7 +89,7 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv
    schema: {
      querystring: z.object({
        projectSlug: z.string().trim(),
-        authorProjectMembershipId: z.string().trim().optional(),
+        authorUserId: z.string().trim().optional(),
        envSlug: z.string().trim().optional()
      }),
      response: {
@@ -143,7 +143,7 @@ export const registerAccessApprovalRequestRouter = async (server: FastifyZodProv
    handler: async (req) => {
      const { requests } = await server.services.accessApprovalRequest.listApprovalRequests({
        projectSlug: req.query.projectSlug,
-        authorProjectMembershipId: req.query.authorProjectMembershipId,
+        authorUserId: req.query.authorUserId,
        envSlug: req.query.envSlug,
        actor: req.permission.type,
        actorId: req.permission.id,
--- a/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts
+++ b/backend/src/ee/services/access-approval-request/access-approval-request-dal.ts
@@ -725,16 +725,17 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR
        )

        .where(`${TableName.Environment}.projectId`, projectId)
-        .where(`${TableName.AccessApprovalPolicy}.deletedAt`, null)
        .select(selectAllTableCols(TableName.AccessApprovalRequest))
        .select(db.ref("status").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerStatus"))
-        .select(db.ref("reviewerUserId").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerUserId"));
+        .select(db.ref("reviewerUserId").withSchema(TableName.AccessApprovalRequestReviewer).as("reviewerUserId"))
+        .select(db.ref("deletedAt").withSchema(TableName.AccessApprovalPolicy).as("policyDeletedAt"));

      const formattedRequests = sqlNestRelationships({
        data: accessRequests,
        key: "id",
        parentMapper: (doc) => ({
-          ...AccessApprovalRequestsSchema.parse(doc)
+          ...AccessApprovalRequestsSchema.parse(doc),
+          isPolicyDeleted: Boolean(doc.policyDeletedAt)
        }),
        childrenMapper: [
          {
@@ -751,7 +752,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR
        (req) =>
          !req.privilegeId &&
          !req.reviewers.some((r) => r.status === ApprovalStatus.REJECTED) &&
-          req.status === ApprovalStatus.PENDING
+          req.status === ApprovalStatus.PENDING &&
+          !req.isPolicyDeleted
      );

      // an approval is finalized if there are any rejections, a privilege ID is set or the number of approvals is equal to the number of approvals required.
@@ -759,7 +761,8 @@ export const accessApprovalRequestDALFactory = (db: TDbClient): TAccessApprovalR
        (req) =>
          req.privilegeId ||
          req.reviewers.some((r) => r.status === ApprovalStatus.REJECTED) ||
-          req.status !== ApprovalStatus.PENDING
+          req.status !== ApprovalStatus.PENDING ||
+          req.isPolicyDeleted
      );

      return { pendingCount: pendingApprovals.length, finalizedCount: finalizedApprovals.length };
--- 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
@@ -275,7 +275,7 @@ export const accessApprovalRequestServiceFactory = ({

  const listApprovalRequests: TAccessApprovalRequestServiceFactory["listApprovalRequests"] = async ({
    projectSlug,
-    authorProjectMembershipId,
+    authorUserId,
    envSlug,
    actor,
    actorOrgId,
@@ -300,8 +300,8 @@ export const accessApprovalRequestServiceFactory = ({
    const policies = await accessApprovalPolicyDAL.find({ projectId: project.id });
    let requests = await accessApprovalRequestDAL.findRequestsWithPrivilegeByPolicyIds(policies.map((p) => p.id));

-    if (authorProjectMembershipId) {
-      requests = requests.filter((request) => request.requestedByUserId === actorId);
+    if (authorUserId) {
+      requests = requests.filter((request) => request.requestedByUserId === authorUserId);
    }

    if (envSlug) {
--- a/backend/src/ee/services/access-approval-request/access-approval-request-types.ts
+++ b/backend/src/ee/services/access-approval-request/access-approval-request-types.ts
@@ -31,7 +31,7 @@ export type TCreateAccessApprovalRequestDTO = {

 export type TListApprovalRequestsDTO = {
  projectSlug: string;
-  authorProjectMembershipId?: string;
+  authorUserId?: string;
  envSlug?: string;
 } & Omit<TProjectPermission, "projectId">;

--- a/backend/src/ee/services/license/license-fns.ts
+++ b/backend/src/ee/services/license/license-fns.ts
@@ -40,7 +40,7 @@ export const getDefaultOnPremFeatures = (): TFeatureSet => ({
  status: null,
  trial_end: null,
  has_used_trial: true,
-  secretApproval: false,
+  secretApproval: true,
  secretRotation: false,
  caCrl: false,
  instanceUserManagement: false,
--- a/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts
+++ b/backend/src/ee/services/secret-approval-request/secret-approval-request-dal.ts
@@ -315,7 +315,6 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
                  .where(`${TableName.SecretApprovalPolicyApprover}.approverUserId`, userId)
                  .orWhere(`${TableName.SecretApprovalRequest}.committerUserId`, userId)
            )
-            .andWhere((bd) => void bd.where(`${TableName.SecretApprovalPolicy}.deletedAt`, null))
            .select("status", `${TableName.SecretApprovalRequest}.id`)
            .groupBy(`${TableName.SecretApprovalRequest}.id`, "status")
            .count("status")
@@ -347,7 +346,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
    try {
      // akhilmhdh: If ever u wanted a 1 to so many relationship connected with pagination
      // this is the place u wanna look at.
-      const query = (tx || db.replicaNode())(TableName.SecretApprovalRequest)
+      const innerQuery = (tx || db.replicaNode())(TableName.SecretApprovalRequest)
        .join(TableName.SecretFolder, `${TableName.SecretApprovalRequest}.folderId`, `${TableName.SecretFolder}.id`)
        .join(TableName.Environment, `${TableName.SecretFolder}.envId`, `${TableName.Environment}.id`)
        .join(
@@ -434,24 +433,31 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
          db.ref("email").withSchema("committerUser").as("committerUserEmail"),
          db.ref("username").withSchema("committerUser").as("committerUserUsername"),
          db.ref("firstName").withSchema("committerUser").as("committerUserFirstName"),
-          db.ref("lastName").withSchema("committerUser").as("committerUserLastName"),
-
-          db.raw(`count(*) OVER() as total_count`)
+          db.ref("lastName").withSchema("committerUser").as("committerUserLastName")
        )
-        .orderBy("createdAt", "desc");
+        .distinctOn(`${TableName.SecretApprovalRequest}.id`)
+        .as("inner");
+
+      const query = (tx || db)
+        .select("*")
+        .select(db.raw("count(*) OVER() as total_count"))
+        .from(innerQuery)
+        .orderBy("createdAt", "desc") as typeof innerQuery;

      if (search) {
-        void query
-          .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [
-            db.ref("firstName").withSchema("committerUser"),
-            db.ref("lastName").withSchema("committerUser"),
-            `%${search}%`
-          ])
-          .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`])
-          .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`])
-          .orWhereILike(`${TableName.Environment}.name`, `%${search}%`)
-          .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`)
-          .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`);
+        void query.where((qb) => {
+          void qb
+            .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [
+              db.ref("firstName").withSchema("committerUser"),
+              db.ref("lastName").withSchema("committerUser"),
+              `%${search}%`
+            ])
+            .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`])
+            .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`])
+            .orWhereILike(`${TableName.Environment}.name`, `%${search}%`)
+            .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`)
+            .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`);
+        });
      }

      const docs = await (tx || db)
@@ -544,7 +550,7 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
    try {
      // akhilmhdh: If ever u wanted a 1 to so many relationship connected with pagination
      // this is the place u wanna look at.
-      const query = (tx || db.replicaNode())(TableName.SecretApprovalRequest)
+      const innerQuery = (tx || db.replicaNode())(TableName.SecretApprovalRequest)
        .join(TableName.SecretFolder, `${TableName.SecretApprovalRequest}.folderId`, `${TableName.SecretFolder}.id`)
        .join(TableName.Environment, `${TableName.SecretFolder}.envId`, `${TableName.Environment}.id`)
        .join(
@@ -631,24 +637,31 @@ export const secretApprovalRequestDALFactory = (db: TDbClient) => {
          db.ref("email").withSchema("committerUser").as("committerUserEmail"),
          db.ref("username").withSchema("committerUser").as("committerUserUsername"),
          db.ref("firstName").withSchema("committerUser").as("committerUserFirstName"),
-          db.ref("lastName").withSchema("committerUser").as("committerUserLastName"),
-
-          db.raw(`count(*) OVER() as total_count`)
+          db.ref("lastName").withSchema("committerUser").as("committerUserLastName")
        )
-        .orderBy("createdAt", "desc");
+        .distinctOn(`${TableName.SecretApprovalRequest}.id`)
+        .as("inner");
+
+      const query = (tx || db)
+        .select("*")
+        .select(db.raw("count(*) OVER() as total_count"))
+        .from(innerQuery)
+        .orderBy("createdAt", "desc") as typeof innerQuery;

      if (search) {
-        void query
-          .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [
-            db.ref("firstName").withSchema("committerUser"),
-            db.ref("lastName").withSchema("committerUser"),
-            `%${search}%`
-          ])
-          .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`])
-          .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`])
-          .orWhereILike(`${TableName.Environment}.name`, `%${search}%`)
-          .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`)
-          .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`);
+        void query.where((qb) => {
+          void qb
+            .whereRaw(`CONCAT_WS(' ', ??, ??) ilike ?`, [
+              db.ref("firstName").withSchema("committerUser"),
+              db.ref("lastName").withSchema("committerUser"),
+              `%${search}%`
+            ])
+            .orWhereRaw(`?? ilike ?`, [db.ref("username").withSchema("committerUser"), `%${search}%`])
+            .orWhereRaw(`?? ilike ?`, [db.ref("email").withSchema("committerUser"), `%${search}%`])
+            .orWhereILike(`${TableName.Environment}.name`, `%${search}%`)
+            .orWhereILike(`${TableName.Environment}.slug`, `%${search}%`)
+            .orWhereILike(`${TableName.SecretApprovalPolicy}.secretPath`, `%${search}%`);
+        });
      }

      const rankOffset = offset + 1;
--- a/frontend/src/hooks/api/accessApproval/queries.tsx
+++ b/frontend/src/hooks/api/accessApproval/queries.tsx
@@ -65,11 +65,11 @@ const fetchApprovalPolicies = async ({ projectSlug }: TGetAccessApprovalRequests
 const fetchApprovalRequests = async ({
  projectSlug,
  envSlug,
-  authorProjectMembershipId
+  authorUserId
 }: TGetAccessApprovalRequestsDTO) => {
  const { data } = await apiRequest.get<{ requests: TAccessApprovalRequest[] }>(
    "/api/v1/access-approvals/requests",
-    { params: { projectSlug, envSlug, authorProjectMembershipId } }
+    { params: { projectSlug, envSlug, authorUserId } }
  );

  return data.requests.map((request) => ({
@@ -109,12 +109,12 @@ export const useGetAccessRequestsCount = ({
 export const useGetAccessApprovalPolicies = ({
  projectSlug,
  envSlug,
-  authorProjectMembershipId,
+  authorUserId,
  options = {}
 }: TGetAccessApprovalRequestsDTO & TReactQueryOptions) =>
  useQuery({
    queryKey: accessApprovalKeys.getAccessApprovalPolicies(projectSlug),
-    queryFn: () => fetchApprovalPolicies({ projectSlug, envSlug, authorProjectMembershipId }),
+    queryFn: () => fetchApprovalPolicies({ projectSlug, envSlug, authorUserId }),
    ...options,
    enabled: Boolean(projectSlug) && (options?.enabled ?? true)
  });
@@ -122,16 +122,13 @@ export const useGetAccessApprovalPolicies = ({
 export const useGetAccessApprovalRequests = ({
  projectSlug,
  envSlug,
-  authorProjectMembershipId,
+  authorUserId,
  options = {}
 }: TGetAccessApprovalRequestsDTO & TReactQueryOptions) =>
  useQuery({
-    queryKey: accessApprovalKeys.getAccessApprovalRequests(
-      projectSlug,
-      envSlug,
-      authorProjectMembershipId
-    ),
-    queryFn: () => fetchApprovalRequests({ projectSlug, envSlug, authorProjectMembershipId }),
+    queryKey: accessApprovalKeys.getAccessApprovalRequests(projectSlug, envSlug, authorUserId),
+    queryFn: () => fetchApprovalRequests({ projectSlug, envSlug, authorUserId }),
    ...options,
-    enabled: Boolean(projectSlug) && (options?.enabled ?? true)
+    enabled: Boolean(projectSlug) && (options?.enabled ?? true),
+    placeholderData: (previousData) => previousData
  });
--- a/frontend/src/hooks/api/accessApproval/types.ts
+++ b/frontend/src/hooks/api/accessApproval/types.ts
@@ -148,7 +148,7 @@ export type TCreateAccessRequestDTO = {
 export type TGetAccessApprovalRequestsDTO = {
  projectSlug: string;
  envSlug?: string;
-  authorProjectMembershipId?: string;
+  authorUserId?: string;
 };

 export type TGetAccessPolicyApprovalCountDTO = {
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/AccessApprovalRequest/AccessApprovalRequest.tsx
@@ -3,6 +3,7 @@
 import { useCallback, useMemo, useState } from "react";
 import {
  faArrowUpRightFromSquare,
+  faBan,
  faBookOpen,
  faCheck,
  faCheckCircle,
@@ -12,10 +13,12 @@ import {
  faMagnifyingGlass,
  faPlus,
  faSearch,
-  faUser
+  faStopwatch,
+  faUser,
+  IconDefinition
 } from "@fortawesome/free-solid-svg-icons";
 import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
-import { formatDistance } from "date-fns";
+import { format, formatDistance } from "date-fns";
 import { AnimatePresence, motion } from "framer-motion";
 import { twMerge } from "tailwind-merge";

@@ -133,7 +136,7 @@ export const AccessApprovalRequest = ({
    isPending: areRequestsPending
  } = useGetAccessApprovalRequests({
    projectSlug,
-    authorProjectMembershipId: requestedByFilter,
+    authorUserId: requestedByFilter,
    envSlug: envFilter
  });

@@ -203,9 +206,15 @@ export const AccessApprovalRequest = ({
      const canBypass =
        !request.policy.bypassers.length || request.policy.bypassers.includes(user.id);

-      let displayData: { label: string; type: "primary" | "danger" | "success" } = {
+      let displayData: {
+        label: string;
+        type: "primary" | "danger" | "success";
+        tooltipContent?: string;
+        icon: IconDefinition | null;
+      } = {
        label: "",
-        type: "primary"
+        type: "primary",
+        icon: null
      };

      const isExpired =
@@ -213,20 +222,42 @@ export const AccessApprovalRequest = ({
        request.isApproved &&
        new Date() > new Date(request.privilege.temporaryAccessEndTime || ("" as string));

-      if (isExpired) displayData = { label: "Access Expired", type: "danger" };
-      else if (isAccepted) displayData = { label: "Access Granted", type: "success" };
-      else if (isRejectedByAnyone) displayData = { label: "Rejected", type: "danger" };
+      if (isExpired)
+        displayData = {
+          label: "Access Expired",
+          type: "danger",
+          icon: faStopwatch,
+          tooltipContent: request.privilege?.temporaryAccessEndTime
+            ? `Expired ${format(request.privilege.temporaryAccessEndTime, "M/d/yyyy h:mm aa")}`
+            : undefined
+        };
+      else if (isAccepted)
+        displayData = {
+          label: "Access Granted",
+          type: "success",
+          icon: faCheck,
+          tooltipContent: `Granted ${format(request.updatedAt, "M/d/yyyy h:mm aa")}`
+        };
+      else if (isRejectedByAnyone)
+        displayData = {
+          label: "Rejected",
+          type: "danger",
+          icon: faBan,
+          tooltipContent: `Rejected ${format(request.updatedAt, "M/d/yyyy h:mm aa")}`
+        };
      else if (userReviewStatus === ApprovalStatus.APPROVED) {
        displayData = {
          label: `Pending ${request.policy.approvals - request.reviewers.length} review${
            request.policy.approvals - request.reviewers.length > 1 ? "s" : ""
          }`,
-          type: "primary"
+          type: "primary",
+          icon: faClipboardCheck
        };
      } else if (!isReviewedByUser)
        displayData = {
          label: "Review Required",
-          type: "primary"
+          type: "primary",
+          icon: faClipboardCheck
        };

      return {
@@ -266,6 +297,8 @@ export const AccessApprovalRequest = ({
    [generateRequestDetails, membersGroupById, user, setSelectedRequest, handlePopUpOpen]
  );

+  const isFiltered = Boolean(search || envFilter || requestedByFilter);
+
  return (
    <AnimatePresence mode="wait">
      <motion.div
@@ -357,7 +390,7 @@ export const AccessApprovalRequest = ({
              }}
            >
              <FontAwesomeIcon icon={faCheck} className="mr-2" />
-              {!!requestCount && requestCount.finalizedCount} Completed
+              {!!requestCount && requestCount.finalizedCount} Closed
            </div>
            <div className="flex flex-grow justify-end space-x-8">
              <DropdownMenu>
@@ -365,7 +398,7 @@ export const AccessApprovalRequest = ({
                  <Button
                    variant="plain"
                    colorSchema="secondary"
-                    className="text-bunker-300"
+                    className={envFilter ? "text-white" : "text-bunker-300"}
                    rightIcon={<FontAwesomeIcon icon={faChevronDown} size="sm" className="ml-2" />}
                  >
                    Environments
@@ -419,10 +452,16 @@ export const AccessApprovalRequest = ({
                    {members?.map(({ user: membershipUser, id }) => (
                      <DropdownMenuItem
                        onClick={() =>
-                          setRequestedByFilter((state) => (state === id ? undefined : id))
+                          setRequestedByFilter((state) =>
+                            state === membershipUser.id ? undefined : membershipUser.id
+                          )
                        }
                        key={`request-filter-member-${id}`}
-                        icon={requestedByFilter === id && <FontAwesomeIcon icon={faCheckCircle} />}
+                        icon={
+                          requestedByFilter === membershipUser.id && (
+                            <FontAwesomeIcon icon={faCheckCircle} />
+                          )
+                        }
                        iconPos="right"
                      >
                        {membershipUser.username}
@@ -434,16 +473,16 @@ export const AccessApprovalRequest = ({
            </div>
          </div>
          <div className="flex flex-col rounded-b-md border-x border-b border-t border-mineshaft-600 bg-mineshaft-800">
-            {filteredRequests?.length === 0 && !search && (
+            {filteredRequests?.length === 0 && !isFiltered && (
              <div className="py-12">
                <EmptyState
-                  title={`No ${statusFilter === "open" ? "Pending" : "Completed"} Access Requests`}
+                  title={`No ${statusFilter === "open" ? "Pending" : "Closed"} Access Requests`}
                />
              </div>
            )}
-            {Boolean(!filteredRequests?.length && search && !areRequestsPending) && (
+            {Boolean(!filteredRequests?.length && isFiltered && !areRequestsPending) && (
              <div className="py-12">
-                <EmptyState title="No Requests Match Search" icon={faSearch} />
+                <EmptyState title="No Requests Match Filters" icon={faSearch} />
              </div>
            )}
            {!!filteredRequests?.length &&
@@ -494,13 +533,19 @@ export const AccessApprovalRequest = ({
                            <span>Requested By You</span>
                          </div>
                        )}
-                        <Badge
-                          className="flex items-center gap-1.5 whitespace-nowrap"
-                          variant={details.displayData.type}
-                        >
-                          <FontAwesomeIcon icon={faClipboardCheck} />
-                          <span>{details.displayData.label}</span>
-                        </Badge>
+                        <Tooltip content={details.displayData.tooltipContent}>
+                          <div>
+                            <Badge
+                              className="flex items-center gap-1.5 whitespace-nowrap"
+                              variant={details.displayData.type}
+                            >
+                              {details.displayData.icon && (
+                                <FontAwesomeIcon icon={details.displayData.icon} />
+                              )}
+                              <span>{details.displayData.label}</span>
+                            </Badge>
+                          </div>
+                        </Tooltip>
                      </div>
                    </div>
                  </div>
--- a/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx
+++ b/frontend/src/pages/secret-manager/SecretApprovalsPage/components/SecretApprovalRequest/SecretApprovalRequest.tsx
@@ -126,6 +126,8 @@ export const SecretApprovalRequest = () => {

  const isRequestListEmpty = !isApprovalRequestLoading && secretApprovalRequests?.length === 0;

+  const isFiltered = Boolean(searchFilter || envFilter || committerFilter);
+
  return (
    <AnimatePresence mode="wait">
      {isSecretApprovalScreen ? (
@@ -292,7 +294,7 @@ export const SecretApprovalRequest = () => {
              </div>
            </div>
            <div className="flex flex-col rounded-b-md border-x border-b border-t border-mineshaft-600 bg-mineshaft-800">
-              {isRequestListEmpty && !searchFilter && (
+              {isRequestListEmpty && !isFiltered && (
                <div className="py-12">
                  <EmptyState
                    title={`No ${statusFilter === "open" ? "Open" : "Closed"} Change Requests`}
@@ -344,10 +346,10 @@ export const SecretApprovalRequest = () => {
                );
              })}
              {Boolean(
-                !secretApprovalRequests.length && searchFilter && !isApprovalRequestLoading
+                !secretApprovalRequests.length && isFiltered && !isApprovalRequestLoading
              ) && (
                <div className="py-12">
-                  <EmptyState title="No Requests Match Search" icon={faSearch} />
+                  <EmptyState title="No Requests Match Filters" icon={faSearch} />
                </div>
              )}
              {Boolean(totalApprovalCount) && (