fix(approvals): address Task 2 code quality findings + test mock update

Four code-quality issues from review + one test-mock follow-up:

1. CRITICAL: isPlaceholderCustomer was called with a string arg but
   expects { id, customer, email } - TS error, and at runtime the
   function would always return true. Variable was dead (computed but
   never read) so remove the call, the explanatory comment, and the
   import entirely. The Option A behaviour (server handles placeholder
   via its 422) is preserved by simply not having the dead variable.

2. CRITICAL: toast(..., { type: 'info' }) doesn't compile under
   ExternalToast and silently drops the type option at runtime, so
   the invalid_status race toast renders with default styling instead
   of info (blue). Switch to toast.info(...). Updates the test
   assertion to check toast.info directly (the test mock defines
   toast.info as an independent vi.fn(), not a wrapper around toast,
   so the prior expect(toast).toHaveBeenCalledWith assertion would
   no longer match).

3. IMPORTANT: Spec asks for a dedicated 404 branch ('This quote no
   longer exists. Refreshing.') but the implementation fell through
   to the generic catch-all. Add the 404 branch between the mail-fail
   branch and the generic fallback.

4. NICE-TO-HAVE: formatCurrency was imported but never used. Drop.

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Dave 2026-08-27 01:00:43 +02:00
parent 911c6f7496
commit 5760ef9966
2 changed files with 10 additions and 14 deletions

View file

@ -213,9 +213,8 @@ describe("ApprovalsPage — Approve & send from list", () => {
});
const { toast } = await import("sonner");
await waitFor(() => {
expect(toast).toHaveBeenCalledWith(
expect.stringContaining("already actioned"),
expect.anything()
expect(toast.info).toHaveBeenCalledWith(
expect.stringContaining("already actioned")
);
});
});

View file

@ -8,14 +8,13 @@ import { Button } from "@/components/ui/button";
import { Textarea } from "@/components/ui/textarea";
import { toast } from "sonner";
import getPocketBase from "@/lib/pocketbase";
import { formatDate, formatCurrency } from "@/lib/pb-helpers";
import { formatDate } from "@/lib/pb-helpers";
import { quoteStatusClassName, formatQuoteStatus } from "@/lib/quote-status";
import { ArrowLeft, CheckCircle, XCircle, Loader2, FileText, ListChecks } from "lucide-react";
import { getClientIdentity } from "@/lib/dev-identity";
import { cn } from "@/lib/utils";
import type { Quote, QuoteItem, QuoteAttachment } from "@/lib/types";
import { SendQuoteModal } from "@/components/quotes/send-quote-modal";
import { isPlaceholderCustomer } from "@/lib/placeholder-customers";
/**
* /quotes/approvals — queue of quotes awaiting the rep's eyes.
@ -208,7 +207,7 @@ export default function ApprovalsPage() {
// 3) Anything else (placeholder customer, validation, 5xx) —
// generic error toast.
if (body.code === "invalid_status") {
toast("This quote was already actioned — refreshing.", { type: "info" });
toast.info("This quote was already actioned — refreshing.");
router.refresh();
setSendModalQuoteId(null);
return;
@ -221,6 +220,12 @@ export default function ApprovalsPage() {
router.refresh();
return;
}
if (res.status === 404) {
toast.info("This quote no longer exists. Refreshing.");
setSendModalQuoteId(null);
router.refresh();
return;
}
toast.error(body.error ?? `Approve failed (HTTP ${res.status})`);
return;
}
@ -543,17 +548,9 @@ export default function ApprovalsPage() {
</div>
)}
{/* Placeholder-customer UX (disable submit) is delegated to the
server's 422 response. The detail page passes a customerIsPlaceholder
prop to SendQuoteModal; the list page deliberately doesn't bother —
placeholder customers shouldn't reach the approval queue in normal
flow, and the existing 422 handler shows the failure cleanly. */}
{(() => {
const openQuote = quotes.find((q) => q.id === sendModalQuoteId);
if (!openQuote) return null;
const customerIsPlaceholder = isPlaceholderCustomer(
openQuote.expand?.customer?.customer ?? ""
);
return (
<SendQuoteModal
open={true}