Merge pull request #283 from Sea-Haven-Industries/fix/ab/sh-406-no-revoke-auto-approved

Hide Revoke from admins on auto-approved uplifts
This commit is contained in:
Alexandre Brandizzi 2026-09-25 22:17:01 +00:00 • committed by GitHub
commit 810fa5d5b0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 62 additions and 7 deletions

View file

@ -12,6 +12,7 @@ type WorkOrderUpliftListItemProps = {
uplift: WorkOrderUplift; uplift: WorkOrderUplift;
readOnly: boolean; readOnly: boolean;
currentUserId: string | number | null | undefined; currentUserId: string | number | null | undefined;
currentUserIsAdmin: boolean;
pendingAction?: boolean; pendingAction?: boolean;
onCancel?: () => void; onCancel?: () => void;
onRevoke?: () => void; onRevoke?: () => void;
@ -21,6 +22,7 @@ export function WorkOrderUpliftListItem({
uplift, uplift,
readOnly, readOnly,
currentUserId, currentUserId,
currentUserIsAdmin,
pendingAction = false, pendingAction = false,
onCancel, onCancel,
onRevoke, onRevoke,
@ -28,7 +30,9 @@ export function WorkOrderUpliftListItem({
const pillStyle = getUpliftStatusPillStyle(uplift.status); const pillStyle = getUpliftStatusPillStyle(uplift.status);
const showCancel = !readOnly && uplift.status === "pending" && Boolean(onCancel); const showCancel = !readOnly && uplift.status === "pending" && Boolean(onCancel);
const showRevoke = const showRevoke =
!readOnly && canRevokeWorkOrderUplift(uplift, currentUserId) && Boolean(onRevoke); !readOnly &&
canRevokeWorkOrderUplift(uplift, currentUserId, currentUserIsAdmin) &&
Boolean(onRevoke);
return ( return (
<div className="rounded-lg border border-[var(--color-border)] p-3"> <div className="rounded-lg border border-[var(--color-border)] p-3">

View file

@ -12,6 +12,7 @@ import { hasOpenWorkOrderUplift } from "@/domain/work-orders/utils/uplift-displa
type WorkOrderUpliftsDialogContentProps = { type WorkOrderUpliftsDialogContentProps = {
readOnly: boolean; readOnly: boolean;
currentUserId: string | number | null | undefined; currentUserId: string | number | null | undefined;
currentUserIsAdmin: boolean;
readOnlyStatusLabel?: string; readOnlyStatusLabel?: string;
isLoading: boolean; isLoading: boolean;
error: Error | null; error: Error | null;
@ -28,6 +29,7 @@ type WorkOrderUpliftsDialogContentProps = {
export function WorkOrderUpliftsDialogContent({ export function WorkOrderUpliftsDialogContent({
readOnly, readOnly,
currentUserId, currentUserId,
currentUserIsAdmin,
readOnlyStatusLabel, readOnlyStatusLabel,
isLoading, isLoading,
error, error,
@ -77,6 +79,7 @@ export function WorkOrderUpliftsDialogContent({
uplift={uplift} uplift={uplift}
readOnly={readOnly} readOnly={readOnly}
currentUserId={currentUserId} currentUserId={currentUserId}
currentUserIsAdmin={currentUserIsAdmin}
pendingAction={actionPending} pendingAction={actionPending}
onCancel={readOnly ? undefined : () => onCancelUplift(uplift.id)} onCancel={readOnly ? undefined : () => onCancelUplift(uplift.id)}
onRevoke={readOnly ? undefined : () => onRevokeUplift(uplift.id)} onRevoke={readOnly ? undefined : () => onRevokeUplift(uplift.id)}

View file

@ -15,6 +15,7 @@ import {
isWorkOrderUpliftsReadOnly, isWorkOrderUpliftsReadOnly,
upliftRevokeRequiresReason, upliftRevokeRequiresReason,
} from "@/domain/work-orders/utils/uplift-display-utils"; } from "@/domain/work-orders/utils/uplift-display-utils";
import { isAdminUser } from "@/lib/auth/user-utils";
import { useAuthContext } from "@/providers/auth-context"; import { useAuthContext } from "@/providers/auth-context";
type RevokeTarget = { type RevokeTarget = {
@ -78,6 +79,7 @@ export function WorkOrderUpliftsDialog({ row, open, onClose }: WorkOrderUpliftsD
<WorkOrderUpliftsDialogContent <WorkOrderUpliftsDialogContent
readOnly={readOnly} readOnly={readOnly}
currentUserId={user?.id} currentUserId={user?.id}
currentUserIsAdmin={isAdminUser(user?.userRoles)}
readOnlyStatusLabel={row?.status} readOnlyStatusLabel={row?.status}
isLoading={isLoading} isLoading={isLoading}
error={error} error={error}

View file

@ -87,11 +87,16 @@ export function canOpenUpliftsDialog(
export function canRevokeWorkOrderUplift( export function canRevokeWorkOrderUplift(
uplift: Pick<WorkOrderUplift, "status" | "requestedById">, uplift: Pick<WorkOrderUplift, "status" | "requestedById">,
currentUserId: string | number | null | undefined, currentUserId: string | number | null | undefined,
currentUserIsAdmin: boolean,
): boolean { ): boolean {
if (uplift.status !== "auto_approved") { if (uplift.status !== "auto_approved") {
// SH-214/SH-212: admin-approved revoke lives on Uplift Approvals, not the WO dialog. // SH-214/SH-212: admin-approved revoke lives on Uplift Approvals, not the WO dialog.
return false; return false;
} }
if (currentUserIsAdmin) {
// Admins revoke only admin-approved uplifts; an auto-approval is the requester's to revoke.
return false;
}
return ( return (
currentUserId != null && currentUserId != null &&
currentUserId !== "" && currentUserId !== "" &&

View file

@ -22,6 +22,12 @@ const pendingUplift: WorkOrderUplift = {
const mockState = vi.hoisted(() => ({ const mockState = vi.hoisted(() => ({
uplifts: [] as WorkOrderUplift[], uplifts: [] as WorkOrderUplift[],
authUser: { id: "u1", userRoles: "Dispatcher" },
}));
vi.mock("@/providers/auth-context", async (importOriginal) => ({
...(await importOriginal<typeof import("@/providers/auth-context")>()),
useAuthContext: () => ({ user: mockState.authUser }),
})); }));
const baseRow: WorkOrderTableRow = { const baseRow: WorkOrderTableRow = {
@ -145,6 +151,31 @@ describe("WorkOrderUpliftsDialog affordances", () => {
expect(screen.queryByRole("button", { name: /create uplift/i })).not.toBeInTheDocument(); expect(screen.queryByRole("button", { name: /create uplift/i })).not.toBeInTheDocument();
expect(screen.queryByRole("button", { name: /cancel pending/i })).not.toBeInTheDocument(); expect(screen.queryByRole("button", { name: /cancel pending/i })).not.toBeInTheDocument();
}); });
it("shows no Revoke to an admin on the auto-approved uplift they requested", () => {
mockState.authUser = { id: "admin-1", userRoles: "Admin" };
mockState.uplifts = [{ ...pendingUplift, status: "auto_approved", requestedById: "admin-1" }];
renderWithProviders(<WorkOrderUpliftsDialog row={baseRow} open onClose={vi.fn()} />, {
withAuth: true,
});
expect(screen.getByText("Auto")).toBeInTheDocument();
expect(screen.queryByRole("button", { name: /revoke/i })).not.toBeInTheDocument();
});
it("shows Revoke to the dispatcher on the auto-approved uplift they requested", () => {
mockState.authUser = { id: "dispatcher-1", userRoles: "Dispatcher" };
mockState.uplifts = [
{ ...pendingUplift, status: "auto_approved", requestedById: "dispatcher-1" },
];
renderWithProviders(<WorkOrderUpliftsDialog row={baseRow} open onClose={vi.fn()} />, {
withAuth: true,
});
expect(screen.getByRole("button", { name: /revoke/i })).toBeInTheDocument();
});
}); });
describe("WorkOrderUpliftListItem revoke affordances", () => { describe("WorkOrderUpliftListItem revoke affordances", () => {
@ -154,6 +185,7 @@ describe("WorkOrderUpliftListItem revoke affordances", () => {
uplift={{ ...pendingUplift, status: "approved" }} uplift={{ ...pendingUplift, status: "approved" }}
readOnly={false} readOnly={false}
currentUserId="admin-1" currentUserId="admin-1"
currentUserIsAdmin
onRevoke={vi.fn()} onRevoke={vi.fn()}
/>, />,
{ withAuth: false }, { withAuth: false },
@ -168,6 +200,7 @@ describe("WorkOrderUpliftListItem revoke affordances", () => {
uplift={{ ...pendingUplift, status: "auto_approved", requestedById: "dispatcher-1" }} uplift={{ ...pendingUplift, status: "auto_approved", requestedById: "dispatcher-1" }}
readOnly={false} readOnly={false}
currentUserId="dispatcher-1" currentUserId="dispatcher-1"
currentUserIsAdmin={false}
onRevoke={vi.fn()} onRevoke={vi.fn()}
/>, />,
{ withAuth: false }, { withAuth: false },

View file

@ -12,6 +12,7 @@ describe("canRevokeWorkOrderUplift", () => {
canRevokeWorkOrderUplift( canRevokeWorkOrderUplift(
{ status: "auto_approved", requestedById: "dispatcher-1" }, { status: "auto_approved", requestedById: "dispatcher-1" },
"dispatcher-1", "dispatcher-1",
false,
), ),
).toBe(true); ).toBe(true);
}); });
@ -21,6 +22,7 @@ describe("canRevokeWorkOrderUplift", () => {
canRevokeWorkOrderUplift( canRevokeWorkOrderUplift(
{ status: "auto_approved", requestedById: "other-dispatcher" }, { status: "auto_approved", requestedById: "other-dispatcher" },
"dispatcher-1", "dispatcher-1",
false,
), ),
).toBe(false); ).toBe(false);
}); });
@ -30,28 +32,34 @@ describe("canRevokeWorkOrderUplift", () => {
canRevokeWorkOrderUplift( canRevokeWorkOrderUplift(
{ status: "auto_approved", requestedById: "dispatcher-1" }, { status: "auto_approved", requestedById: "dispatcher-1" },
"admin-user", "admin-user",
true,
), ),
).toBe(false); ).toBe(false);
}); });
it("allows admin to revoke their own auto-approved uplift as the requester", () => { it("rejects admin revoking an auto-approved uplift even as its requester", () => {
expect( expect(
canRevokeWorkOrderUplift( canRevokeWorkOrderUplift(
{ status: "auto_approved", requestedById: "admin-user" }, { status: "auto_approved", requestedById: "admin-user" },
"admin-user", "admin-user",
true,
), ),
).toBe(true); ).toBe(false);
}); });
it("rejects approved revoke on the Work Order surface (SH-214)", () => { it("rejects approved revoke on the Work Order surface (SH-214)", () => {
const uplift = { status: "approved" as const, requestedById: "dispatcher-1" }; const uplift = { status: "approved" as const, requestedById: "dispatcher-1" };
expect(canRevokeWorkOrderUplift(uplift, "dispatcher-1")).toBe(false); expect(canRevokeWorkOrderUplift(uplift, "dispatcher-1", false)).toBe(false);
expect(canRevokeWorkOrderUplift(uplift, "admin-user")).toBe(false); expect(canRevokeWorkOrderUplift(uplift, "admin-user", true)).toBe(false);
}); });
it("rejects revoke for other statuses", () => { it("rejects revoke for other statuses", () => {
expect(canRevokeWorkOrderUplift({ status: "pending", requestedById: "u1" }, "u1")).toBe(false); expect(canRevokeWorkOrderUplift({ status: "pending", requestedById: "u1" }, "u1", false)).toBe(
expect(canRevokeWorkOrderUplift({ status: "rejected", requestedById: "u1" }, "u1")).toBe(false); false,
);
expect(canRevokeWorkOrderUplift({ status: "rejected", requestedById: "u1" }, "u1", false)).toBe(
false,
);
}); });
}); });