Skip to content

Commit 093274a

Browse files
committed
frontend: address review findings on the donation review screen
Schema alignment with the review-schema PR - Renames `items` to `furniture_items`, matching DonationDetail, so wiring the live API stays a fetch swap. - Widens ItemReviewStatus to all seven FurnitureStatus values. Only the first three are reachable from this screen, but an item can arrive from the API in a downstream state and was previously mistyped. - Documents the deliberate divergences at the top of types.ts — DonorInformation is a display shape, `condition` holds UI copy rather than FurnitureConditionEnum, `request_id`/`submitted_at` are presentation fields, and `pickup` is pre-resolved to the active one — so the next reader can tell intent from oversight. Review-status precedence - deriveReviewStatus checked for a scheduled pickup first, so a pickup booked while items were still outstanding reported "scheduled" and hid them. The backend explicitly orders this the other way: scheduling only outranks a finished review. Reordered to match, and the test that asserted the old behaviour ("regardless of item states") is replaced with cases covering both directions. Dates - The header used a local formatShortDate carrying the same UTC off-by-one that was just fixed in common; it now uses the shared helper. Other - Drops the decorative calendar icon from the pickup date field: type="date" draws its own picker indicator, so the two rendered side by side. - Donor card actions use Figma's borderless Cancel + outlined Save, distinct from the dialogs where Save is the filled primary action. - Removes the unused DONATION_REQUESTS / donationRequestPath constants. Nothing referenced them, and dropping them leaves this branch touching only app/donation-request/. - Adds donationRequestStore tests — the confirmation-invalidation rule the Edit dialog warns about had no coverage at all.
1 parent 082bc2e commit 093274a

12 files changed

Lines changed: 202 additions & 51 deletions

File tree

frontend/app/donation-request/[id]/page.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ export default function DonationRequestPage() {
6565
request={request}
6666
reviewStatus={reviewStatus}
6767
approvedCount={approvedCount}
68-
totalItems={request.items.length}
68+
totalItems={request.furniture_items.length}
6969
onSchedulePickup={openSchedule}
7070
/>
7171

@@ -81,9 +81,9 @@ export default function DonationRequestPage() {
8181

8282
<section className="flex flex-col gap-lg">
8383
<h2 className="text-heading-3 font-semibold text-foreground">
84-
{request.items.length} Items Donated
84+
{request.furniture_items.length} Items Donated
8585
</h2>
86-
{request.items.map((item) => (
86+
{request.furniture_items.map((item) => (
8787
<DonationItemCard
8888
key={item.id}
8989
item={item}

frontend/app/donation-request/components/ApproveItemDialog.tsx

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,7 @@ function ApproveItemDialog({
4444
<DonationItemPreview {...item} />
4545
</DialogBody>
4646
<DialogFooter>
47-
<DialogClose
48-
render={<Button variant="outline" onClick={onCancel} />}
49-
>
47+
<DialogClose render={<Button variant="outline" onClick={onCancel} />}>
5048
Cancel
5149
</DialogClose>
5250
<DialogClose render={<Button onClick={onConfirm} />}>

frontend/app/donation-request/components/DonationRequestHeader.tsx

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import {
99
PendingReviewBadge,
1010
ScheduledBadge,
1111
} from "@/common/components/status-labels";
12-
import { formatDate } from "@/common/utils/DateUtils";
12+
import { formatDate, formatShortDate } from "@/common/utils/DateUtils";
1313
import type { DonationRequest, ReviewStatus } from "./types";
1414

1515
interface DonationRequestHeaderProps {
@@ -55,13 +55,6 @@ function ReviewStatusIndicator({
5555
}
5656
}
5757

58-
/** "March 26, 2026" → "Mar 26" for the compact scheduled badge. */
59-
function formatShortDate(iso: string): string {
60-
const date = new Date(iso);
61-
if (Number.isNaN(date.getTime())) return iso;
62-
return date.toLocaleDateString("en-US", { month: "short", day: "numeric" });
63-
}
64-
6558
function DonationRequestHeader(props: DonationRequestHeaderProps) {
6659
const { request } = props;
6760
return (

frontend/app/donation-request/components/DonorInformationCard.tsx

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -232,11 +232,15 @@ function DonorInformationForm({
232232
/>
233233
</FieldColumn>
234234

235+
{/* Figma's donor card uses a borderless Cancel next to an outlined Save —
236+
distinct from the dialogs, where Save is the filled primary action. */}
235237
<div className="flex justify-end gap-xs">
236-
<Button variant="outline" onClick={onCancel}>
238+
<Button variant="ghost" onClick={onCancel}>
237239
Cancel
238240
</Button>
239-
<Button onClick={onSave}>Save</Button>
241+
<Button variant="outline" onClick={onSave}>
242+
Save
243+
</Button>
240244
</div>
241245
</div>
242246
);

frontend/app/donation-request/components/SchedulePickupDialog.tsx

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
"use client";
22

33
import { useEffect, useState } from "react";
4-
import { CalendarDays } from "lucide-react";
54

65
import {
76
Dialog,
@@ -79,16 +78,14 @@ function SchedulePickupDialog({
7978
<Label htmlFor="pickup-date">
8079
Date<span className="text-destructive"> *</span>
8180
</Label>
82-
<div className="relative">
83-
<CalendarDays className="pointer-events-none absolute left-sm top-1/2 size-4 -translate-y-1/2 text-muted-foreground" />
84-
<Input
85-
id="pickup-date"
86-
type="date"
87-
value={date}
88-
onChange={(event) => setDate(event.target.value)}
89-
className="pl-8"
90-
/>
91-
</div>
81+
{/* No decorative calendar icon here: type="date" draws its own
82+
picker indicator, so adding one renders two side by side. */}
83+
<Input
84+
id="pickup-date"
85+
type="date"
86+
value={date}
87+
onChange={(event) => setDate(event.target.value)}
88+
/>
9289
</div>
9390

9491
<div className="flex flex-col gap-xs">

frontend/app/donation-request/components/fixture.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ export const DONATION_REQUEST_FIXTURE: DonationRequest = {
2626
has_pets: false,
2727
pickup_address: "210 Drake Ave, NL, CA A2V 1K5",
2828
},
29-
items: [
29+
furniture_items: [
3030
{
3131
id: "item-dining",
3232
name: "Dining table & chairs / set",

frontend/app/donation-request/components/reviewStatus.test.ts

Lines changed: 39 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,18 @@ function makeRequest(
3434
has_pets: false,
3535
pickup_address: "210 Drake Ave",
3636
},
37-
items,
37+
furniture_items: items,
3838
pickup,
3939
};
4040
}
4141

42+
const SCHEDULED_PICKUP: DonationRequest["pickup"] = {
43+
id: "p1",
44+
scheduled_date: "2026-03-26",
45+
note: null,
46+
confirmed_at: null,
47+
};
48+
4249
describe("deriveReviewStatus", () => {
4350
it("is pending_review when a donation has no items", () => {
4451
expect(deriveReviewStatus(makeRequest([]))).toBe("pending_review");
@@ -68,16 +75,40 @@ describe("deriveReviewStatus", () => {
6875
expect(deriveReviewStatus(request)).toBe("reviewed");
6976
});
7077

71-
it("is scheduled once a pickup has a date, regardless of item states", () => {
72-
const request = makeRequest([makeItem("a", "PICKUP_PENDING")], {
73-
id: "p1",
74-
scheduled_date: "2026-03-26",
75-
note: null,
76-
confirmed_at: null,
77-
});
78+
it("is scheduled once every item is reviewed and a pickup has a date", () => {
79+
const request = makeRequest(
80+
[makeItem("a", "APPROVED"), makeItem("b", "REJECTED")],
81+
SCHEDULED_PICKUP
82+
);
7883
expect(deriveReviewStatus(request)).toBe("scheduled");
7984
});
8085

86+
// Mirrors compute_review_status: "a pickup booked while items are still
87+
// outstanding must not hide that they need attention."
88+
it("does not let a scheduled pickup mask items still awaiting review", () => {
89+
const request = makeRequest(
90+
[makeItem("a", "APPROVED"), makeItem("b", "PICKUP_PENDING")],
91+
SCHEDULED_PICKUP
92+
);
93+
expect(deriveReviewStatus(request)).toBe("partially_reviewed");
94+
});
95+
96+
it("does not let a scheduled pickup mask a wholly unreviewed donation", () => {
97+
const request = makeRequest(
98+
[makeItem("a", "PICKUP_PENDING")],
99+
SCHEDULED_PICKUP
100+
);
101+
expect(deriveReviewStatus(request)).toBe("pending_review");
102+
});
103+
104+
it("treats downstream statuses as not-yet-reviewed, as the backend does", () => {
105+
const request = makeRequest([
106+
makeItem("a", "APPROVED"),
107+
makeItem("b", "OFFERED"),
108+
]);
109+
expect(deriveReviewStatus(request)).toBe("partially_reviewed");
110+
});
111+
81112
it("ignores a pickup with no scheduled date", () => {
82113
const request = makeRequest([makeItem("a", "APPROVED")], {
83114
id: "p1",
Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,39 @@
11
import type { DonationRequest, ReviewStatus } from "./types";
22

3-
/** Item statuses that count as "an admin has finished reviewing this item". */
3+
/**
4+
* Item statuses that count as "an admin has finished reviewing this item".
5+
* Mirrors services.donations.REVIEWED_FURNITURE_STATUSES — the downstream
6+
* states (OFFERED, SCHEDULED, …) deliberately do not count as reviewed.
7+
*/
48
const REVIEWED_ITEM_STATUSES = new Set(["APPROVED", "REJECTED"]);
59

610
/**
711
* Derive how far along the admin's review is. Mirrors the backend's
8-
* services.donations.compute_review_status so the badge stays consistent:
9-
* a scheduled pickup wins, otherwise it's driven by item review progress.
12+
* services.donations.compute_review_status, including its ordering: scheduling
13+
* only outranks a *finished* review, so a pickup booked while items are still
14+
* outstanding must not hide that they need attention.
1015
*/
1116
export function deriveReviewStatus(request: DonationRequest): ReviewStatus {
12-
if (request.pickup?.scheduled_date) {
13-
return "scheduled";
14-
}
15-
16-
const items = request.items;
17+
const items = request.furniture_items;
1718
const reviewed = items.filter((item) =>
1819
REVIEWED_ITEM_STATUSES.has(item.status)
1920
);
2021

22+
// A donation with no items yet has nothing to review.
2123
if (items.length === 0 || reviewed.length === 0) {
2224
return "pending_review";
2325
}
2426
if (reviewed.length < items.length) {
2527
return "partially_reviewed";
2628
}
29+
if (request.pickup?.scheduled_date) {
30+
return "scheduled";
31+
}
2732
return "reviewed";
2833
}
2934

3035
/** Count of approved items — drives the "n/m Approved" header label. */
3136
export function countApproved(request: DonationRequest): number {
32-
return request.items.filter((item) => item.status === "APPROVED").length;
37+
return request.furniture_items.filter((item) => item.status === "APPROVED")
38+
.length;
3339
}

frontend/app/donation-request/components/types.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,32 @@
55
* (see backend/app/schemas.py: DonationDetail, FurnitureDetail, Pickup) using
66
* the same field names, so wiring to the live API later is a fetch swap rather
77
* than a reshape. The page currently reads them from a local store — no API.
8+
*
9+
* Deliberate divergences from those schemas, so the next reader can tell intent
10+
* from oversight:
11+
* - `DonorInformation` is a display shape, not backend `Donor`. It flattens the
12+
* donation-level `smoking_household` / `has_pets` onto the donor, and carries
13+
* `pickup_address` as one preformatted line where the backend stores
14+
* `address_line_1` / `address_line_2` / `city` / `postal_code` separately.
15+
* - `DonationRequestItem.condition` holds display copy ("No Stains"), NOT the
16+
* backend `FurnitureConditionEnum` (excellent | good | fair | poor).
17+
* - `request_id` and `submitted_at` are presentation fields; the backend
18+
* `Donation` exposes `id` and `created_at`.
19+
* - `pickup` is already resolved to the single active pickup — the backend
20+
* returns a list and picks one via `services.donations.get_active_pickup`.
21+
* Everything else matches the schema field-for-field.
822
*/
923

24+
/** Mirrors backend FurnitureStatus. Only the first three are reachable from this
25+
* screen; the rest are downstream states an item can arrive in from the API. */
1026
export type ItemReviewStatus =
1127
| "PICKUP_PENDING" // awaiting review
1228
| "APPROVED"
13-
| "REJECTED";
29+
| "REJECTED"
30+
| "OFFERED"
31+
| "SCHEDULED"
32+
| "DELIVERED"
33+
| "CLOSED";
1434

1535
export type ReviewStatus =
1636
| "pending_review"
@@ -64,6 +84,6 @@ export interface DonationRequest {
6484
/** ISO date the request was submitted. */
6585
submitted_at: string;
6686
donor: DonorInformation;
67-
items: DonationRequestItem[];
87+
furniture_items: DonationRequestItem[];
6888
pickup: DonationPickup | null;
6989
}
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
import { useDonationRequestStore } from "./donationRequestStore";
2+
3+
const store = () => useDonationRequestStore.getState();
4+
const pickup = () => store().request.pickup;
5+
const itemIds = () => store().request.furniture_items.map((item) => item.id);
6+
7+
beforeEach(() => {
8+
store().reset();
9+
});
10+
11+
describe("item review", () => {
12+
it("approves an item and clears any previous rejection", () => {
13+
const [id] = itemIds();
14+
store().rejectItem(id, "other", "Too large for the van");
15+
store().approveItem(id);
16+
17+
const item = store().request.furniture_items.find((i) => i.id === id)!;
18+
expect(item.status).toBe("APPROVED");
19+
expect(item.rejection_reason).toBeNull();
20+
expect(item.rejection_details).toBeNull();
21+
});
22+
23+
it("records the reason and free-text details on rejection", () => {
24+
const [id] = itemIds();
25+
store().rejectItem(id, "other", "Too large for the van");
26+
27+
const item = store().request.furniture_items.find((i) => i.id === id)!;
28+
expect(item.status).toBe("REJECTED");
29+
expect(item.rejection_reason).toBe("other");
30+
expect(item.rejection_details).toBe("Too large for the van");
31+
});
32+
33+
it("leaves other items untouched", () => {
34+
const [first, second] = itemIds();
35+
store().approveItem(first);
36+
37+
const other = store().request.furniture_items.find((i) => i.id === second)!;
38+
expect(other.status).toBe("PICKUP_PENDING");
39+
});
40+
});
41+
42+
describe("pickup scheduling", () => {
43+
it("schedules an unconfirmed pickup", () => {
44+
store().schedulePickup("2026-03-26", "Ring the buzzer");
45+
46+
expect(pickup()?.scheduled_date).toBe("2026-03-26");
47+
expect(pickup()?.note).toBe("Ring the buzzer");
48+
expect(pickup()?.confirmed_at).toBeNull();
49+
});
50+
51+
it("confirms a scheduled pickup", () => {
52+
store().schedulePickup("2026-03-26");
53+
store().confirmPickup();
54+
55+
expect(pickup()?.confirmed_at).not.toBeNull();
56+
});
57+
58+
it("will not confirm when nothing is scheduled", () => {
59+
store().confirmPickup();
60+
expect(pickup()).toBeNull();
61+
});
62+
63+
// The subtlest rule in the flow, and the one the Edit dialog warns about.
64+
it("clears the confirmation when a confirmed pickup moves to a new date", () => {
65+
store().schedulePickup("2026-03-26");
66+
store().confirmPickup();
67+
expect(pickup()?.confirmed_at).not.toBeNull();
68+
69+
store().updatePickup("2026-03-27");
70+
71+
expect(pickup()?.scheduled_date).toBe("2026-03-27");
72+
expect(pickup()?.confirmed_at).toBeNull();
73+
});
74+
75+
it("keeps the confirmation when only the note changes", () => {
76+
store().schedulePickup("2026-03-26", "Ring the buzzer");
77+
store().confirmPickup();
78+
const confirmedAt = pickup()!.confirmed_at;
79+
80+
store().updatePickup("2026-03-26", "Use the side door");
81+
82+
expect(pickup()?.note).toBe("Use the side door");
83+
expect(pickup()?.confirmed_at).toBe(confirmedAt);
84+
});
85+
86+
it("keeps the same pickup id across edits", () => {
87+
store().schedulePickup("2026-03-26");
88+
const id = pickup()!.id;
89+
90+
store().updatePickup("2026-03-27");
91+
92+
expect(pickup()?.id).toBe(id);
93+
});
94+
});
95+
96+
describe("donor edits", () => {
97+
it("patches only the supplied fields", () => {
98+
const before = store().request.donor;
99+
store().updateDonor({ email: "new@example.com" });
100+
101+
expect(store().request.donor.email).toBe("new@example.com");
102+
expect(store().request.donor.first_name).toBe(before.first_name);
103+
});
104+
});

0 commit comments

Comments
 (0)