align donation request review schema with user flows - #122
Open
brian-fu wants to merge 8 commits into
Open
Conversation
Support the admin donation-request review flow from Figma:
- Furniture gains rejection_reason / rejection_details / reviewed_at, and
FurnitureStatus gains REJECTED. Rejecting requires a reason; reason "other"
requires free-text details; approving clears any earlier rejection. The
invariant is enforced on create, PUT and the new review endpoints.
- New furniture_photos table — donors submit several ordered photos per item,
which Furniture.image_url (a single string) could not hold. image_url stays
as the thumbnail and seeds the new table on migrate.
- Pickup gains scheduled_date / note / confirmed_at, and route_id relaxes to
nullable: an admin schedules a pickup against a donation during review, long
before dispatch assigns it to a route. Moving a confirmed pickup to a new
date clears the confirmation, matching the warning in the edit dialog.
- Donation gains smoking_household / has_pets. The donor form asks these once,
but they previously only existed per-Furniture, so the donor card had no
single source to read.
review_status (pending_review / partially_reviewed / reviewed / scheduled) is
derived in the service layer rather than stored, so it cannot drift when an
item is re-reviewed or a pickup is removed.
New endpoints:
GET /donations/{id}/detail donation + donor + items + photos + pickup
GET /furniture/{id}/detail item + ordered photos
PUT /furniture/{id}/photos replace the photo set; list order = position
POST /furniture/{id}/approve
POST /furniture/{id}/reject
POST /pickups/{id}/confirm
The database is not provisioned yet, so the migration ships unapplied and is
verified only against the pytest suite (157 passing).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
It was sitting among the unrelated private helpers near the bottom of the file, far from the two functions that call it.
The rest of the codebase writes datetime.now(timezone.utc).replace(tzinfo=None) inline — 33 lambdas in models/base.py and pickups.confirm_pickup do exactly this — so a one-off helper in furniture.py was the outlier.
Every enum in enums.py is declared as a str mixin (class X(str, enum.Enum)), so members already compare equal to their raw string values -- FurnitureStatus.REJECTED == "REJECTED" is True. Converting via .value before the comparisons in _validate_rejection_fields was a no-op, so the helper and both of its call lines are gone rather than inlined.
Contributor
There was a problem hiding this comment.
⚠️ Not ready to approve
The migration currently relies on gen_random_uuid() for seeding, which is not available on default Postgres installs without enabling an extension and is likely to break alembic upgrade in real environments.
Pull request overview
Adds backend schema, persistence, and API endpoints to support the admin donation-request review flow (approve/reject with reasons, ordered item photos, donation “detail” composite including derived review_status, and pickup scheduling/confirmation).
Changes:
- Introduces review-related enums and new/extended schemas for donation detail, furniture rejection, photos, and pickup confirmation.
- Adds
furniture_photospersistence + service/API to replace an item’s ordered photo set, plus computed donationreview_status. - Extends pickup flow with scheduling fields, confirmation semantics, and corresponding tests + migration.
File summaries
| File | Description |
|---|---|
| backend/tests/test_donation_review.py | New end-to-end-ish tests spanning furniture review, photo replacement/order, donation detail aggregation, and pickup scheduling/confirmation. |
| backend/migrations/versions/20260724_donation_review_flow.py | Migration adding new donation/pickup/furniture columns and the new furniture_photos table with seed-from-thumbnail logic. |
| backend/app/services/pickups.py | Adds note length validation, confirmation logic, and confirmation-clearing when scheduled date changes. |
| backend/app/services/furniture.py | Adds approve/reject review operations, rejection invariant validation on create/update, and replace-all furniture photo set logic. |
| backend/app/services/donations.py | Adds donation detail loader, pickup selection helper, derived review_status, and response shaping helper. |
| backend/app/schemas.py | Adds schemas for furniture photos, rejection payload, furniture detail (with photos), pickup fields, and donation detail composite response. |
| backend/app/models/base.py | Adds donation household fields, pickup scheduling/confirmation fields + nullable route, furniture rejection fields, and FurniturePhoto model + relationship. |
| backend/app/models/init.py | Exports FurniturePhoto. |
| backend/app/enums.py | Adds DonationReviewStatus, FurnitureStatus.REJECTED, and FurnitureRejectionReason. |
| backend/app/api/pickups.py | Adds POST /pickups/{id}/confirm endpoint. |
| backend/app/api/furniture.py | Adds furniture detail endpoint, photo replacement endpoint, and approve/reject endpoints. |
| backend/app/api/donations.py | Adds GET /donations/{id}/detail composite endpoint. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brian-fu
changed the title
Six fixes, all reproduced against a real Postgres 15 before and after. Normalise inbound datetimes to naive UTC (schemas). The DateTime columns are TIMESTAMP WITHOUT TIME ZONE and asyncpg refuses a tz-aware value outright, so POST /pickups with the 'Z' that Date.toISOString() emits raised a DataError — a 500, since it isn't an IntegrityError. SQLite silently dropped the offset without converting, which is why the suite never saw it. Also applied to Route.date, which had the identical latent failure. Pick the scheduled pickup, not the newest one (donations service). Once dispatch created its route-assigned row, get_active_pickup returned that instead, and the donor's confirmed date and confirmed_at vanished from the review screen. Stop a pickup masking an unfinished review (donations service). Scheduling now only outranks a review that is actually complete, so partially_reviewed stays visible. Clear stale rejection_details when the reason changes (furniture service). Free text written for 'other' survived onto a different reason and kept rendering on the rejected item's card. Detach items before the lossy pickup delete (migration downgrade). The delete tripped fk_furniture_pickup_id and aborted the whole downgrade whenever an item was assigned to a review-scheduled pickup. Drop position from the photo request body and cap the set at 5 (Copilot). The endpoint always derived position from list order, so accepting one advertised a field the server ignored. Removes the unused FurniturePhotoCreate. Also orders Donation.furniture_items so the review screen's cards don't reshuffle between loads. Tests 25 -> 32, suite 157 -> 164; black clean.
Fixing Copilot's point by adding a third photo model left FurniturePhotoBase with a single subclass and url declared twice. The Base existed to be shared by FurniturePhotoCreate and FurniturePhoto; once Create was deleted as unused, the fix was simply to move position off the Base rather than add a class beside it. FurniturePhotoBase is now what a client sends (url only) and FurniturePhoto adds position alongside id and the timestamps — same shape on the wire, one fewer model, and the Base/response naming the rest of the file uses.
The migration creates ix_furniture_photos_furniture_id but the model didn't declare it, so `alembic check` reported drift and the next autogenerate run would have proposed dropping the index. The existing email indexes don't drift because those columns are unique=True, which alembic matches on its own.
Not worth it at this table's expected size, and the only query against furniture_id fetches one item's photos. Removed from the migration as well as the model — dropping just the model attribute would have left the index in the database and reintroduced the autogenerate drift it was added to fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backend schema + API for the admin donation-request review flow from Figma. No frontend changes.
Enums
FurnitureStatus.REJECTEDFurnitureRejectionReason—condition/pickup/location/other(mirrorsDEFAULT_REJECT_REASONSinRejectItemDialog.tsx)DonationReviewStatus—pending_review/partially_reviewed/reviewed/scheduled(derived, not stored)Models + migration (
20260724_donation_review)rejection_reason,rejection_details,reviewed_atfurniture_photostable — ordered, cascade-delete. Donors submit several photos per item, which the singleimage_urlstring could not hold.image_urlstays as the thumbnail and seeds the table on migrate.scheduled_date,note,confirmed_at, androute_idrelaxed to nullable — a pickup is scheduled against a donation during review, before dispatch assigns a route.smoking_household,has_pets— asked once on the donor form; previously only existed per-Furniture.Endpoints
GET/donations/{id}/detailGET/furniture/{id}/detailPUT/furniture/{id}/photosPOST/furniture/{id}/approvePOST/furniture/{id}/rejectPOST/pickups/{id}/confirmBusiness rules
otherrequires non-blank details; approving clears both. Enforced on create,PUT(validates the merged row) and the review endpoints. Changing the reason clears details written for the previous one.review_statusis computed from item statuses and the pickup, so it can't drift. A scheduled pickup only outranks a review that is actually complete.confirmed_at(matches the Edit Pickup dialog warning). Editing only the note, or re-sending the same date, keeps it.confirmis idempotent and refuses a pickup with no date. Notes are capped at 500 characters.TIMESTAMP WITHOUT TIME ZONEand asyncpg rejects tz-aware values, soDate.toISOString()output would otherwise fail on Postgres.Testing
tests/test_donation_review.py; full suite 164 passing;black --checkclean.image_urlbackfill seeds correctly,alembic checkreports no model drift, and upgrade → downgrade → upgrade round-trips. The downgrade is deliberately lossy — it drops pickups that have no route and the whole photo table.