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:
parent
911c6f7496
commit
5760ef9966
2 changed files with 10 additions and 14 deletions
|
|
@ -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")
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
|
|
|
|||
Loading…
Reference in a new issue