feat(expenses): admin approve/deny review flow with receipt upload - #315
Merged
Conversation
This was referenced Aug 12, 2026
Implements the Figma "Expenses Page" section (node 3545:34605).
Frontend
- Status column and pill: Approved / Pending / Needs Info, using the
design's exact fills. `needs_more_info` was already permitted by the
DB check constraint, so no enum change was needed.
- New ReviewExpenseModal. Everyone sees the expense read-only; the
Admin Decision pills and Admin Notes are rendered only for admins,
and Save Changes is admin-only.
- Table now matches the design: Expense ID, Date, Type of Expense,
Project, Amount, Receipt, Status. Description was dropped; the
project detail page hides Project via `showProject`.
- Filters consolidated into one "Filter By" nested menu (Month /
Project / Type / Status) plus "Clear Filters (n)".
- The receipt is now actually uploaded. FileUpload previously ran a
fake setInterval progress bar and the File was never sent anywhere,
so receipt_url was always null. It now presigns, PUTs to S3 with
real XHR progress, and passes the object URL through to the POST.
- resetForm did not clear the selected file, so a cancelled modal
reopened holding the previous receipt.
Backend
- GET /expenditures/upload-url presigns a PDF PUT under receipts/.
- GET /expenditures/{id}/receipt presigns a short-lived GET, so the
receipt does not depend on the bucket being publicly readable.
- PATCH /expenditures/{id}/status accepts and persists adminNotes.
- GET /expenditures/{id} returns the submitter and project names for
the modal's "Submitted By".
- validateExpenditureInput read body.receipt_url while every other
field was camelCase; it now accepts receiptUrl and keeps the old
key working.
Infra
- The shared lambda role had no S3 permissions, so a presigned PUT
would have failed AccessDenied. Adds PutObject/GetObject.
Also fixes a pre-existing `next build` failure: page modules may not
have non-page exports, and both accounts/page.tsx and
expenses/page.tsx did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pill was pinned to Figma's 81x29, which clips longer labels such as Needs Info and the legacy denied fallback. 81px is now a min-width and the label drives the real width. Review modal now shrinks below its 485px Figma width, the field labels flex instead of sitting at a fixed 120px, and a long receipt filename truncates rather than pushing the actions off the row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…load route The presigned upload route was written against the old PI/Accountant/Admin allow-list and git merged it cleanly over #311, so it silently kept roles that no longer exist. Any non-global-admin would have been refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nourshoreibah
force-pushed
the
expense-approvals-app
branch
from
August 12, 2026 02:27
fcfae46 to
7a97212
Compare
nourshoreibah
added
the
test-environment
Contributor
|
🌿 ⏳ Creating preview environment… (logs) |
Contributor
🌿 Preview environment — ready ✅Open: https://d3nmtjoh6ir9ym.cloudfront.net/pr-315/ Shared RDS + Cognito (prod data); DB migrations are not applied here — if this PR adds a migration, endpoints using the new columns will fail until it merges. New commits update this environment in place — a note is posted here on each update. Remove the |
Contributor
|
🌿 Preview environment torn down 🧹 — the stack for this PR has been destroyed. |
nourshoreibah
added a commit
that referenced
this pull request
Aug 12, 2026
Resolves conflicts between the admin-only dashboard and main's expense approval flow (#315), project role rename (#311) and audit fixes (#310): - routes: /dashboard is admin-gated, /expenses is not. Main opened /expenses to non-admins because they submit and read their own expenses there; only the review modal's approve/deny is admin-gated. - accounts: both sides moved the staff roster out of page.tsx to satisfy the Next.js page-export rule. Kept main's mockUsers.ts and dropped the duplicate staff.ts. - Navbar/routes tests follow the same split. Co-authored-by: Cursor <cursoragent@cursor.com>
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
1 participant
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.
Implements the Figma "Expenses Page" section (node 3545:34605).
Note
Based on #314 (migration + IAM), not
main. Review/merge that one first — the review modal writesadmin_notes, and the receipt upload needs the S3 permissions. Retarget tomainonce #314 lands. Supersedes #313, which mixed both layers.Status vocabulary
The design is internally inconsistent — the table pill reads Needs Info, the base component variant is named
rejected, and the filter menu says "Need Info". Confirmed with the requester: Pending / Approved / Needs Info.The legacy
deniedvalue stays valid in the DB and still renders (greyed, un-clipped) if an old row has it, but it is not offered in the UI.Frontend
StatusBadge) using the design's fills —#B5D99D/#FFD167/#E17070.ReviewExpenseModal— everyone sees the expense read-only; Admin Decision and Admin Notes render only whenisAdmin, and so does Save Changes.showProject./expensesis no longer admin-gated. Non-admins submit expenses and read their own submissions there; only the review controls are gated. The backend already let any authenticated user list expenditures, so this aligns the frontend with the existing API rather than exposing anything new.Sizes are content-driven, not pinned to the Figma pixel values: the pill treats 81px as a
min-widthso longer labels cannot clip, the modal shrinks below its 485px design width, field labels flex, and a long receipt filename truncates instead of pushing the actions off the row. Table columns are percentage-based.Bugs fixed along the way
FileUploadran a fakesetIntervalprogress bar (markedTODO) and theFilewas only used as a required-field gate —receipt_urlwas always null. It now presigns, PUTs to S3 with real XHR progress, and threads the object URL into the POST body.resetForm()did not clear the selected file, so a cancelled modal reopened still holding the previous receipt.validateExpenditureInputreadbody.receipt_urlwhile every other field was camelCase. Now acceptsreceiptUrl, old key still honoured.PI/Accountant/Adminallow-list; git merged that cleanly over feat(roles): project roles are Admin, Director, Student #311, which had renamed the roles. Silent, and it would have refused every non-global-admin. NowDirector/Admin, matching the sibling routes.next buildfailure (present onmain, unrelated to this work): Next.js forbids non-page exports from a page module, and bothaccounts/page.tsx(facilitationTeam,teamMembers) andexpenses/page.tsx(EXPENSE_CATEGORIES) had them. The build was already broken before this branch; it passes now.Backend
GET /expenditures/upload-urlreceipts/{projectId}/GET /expenditures/{id}/receiptPATCH /expenditures/{id}/statusadminNotesGET /expenditures/{id}Receipts are read through a presigned GET rather than their object URL, so they do not depend on the bucket being publicly readable.
Warning
Receipts land in
aws_s3_bucket.reports_bucket, which onmainis still public-read (block_public_*allfalseplus aPrincipal: "*"s3:GetObjectpolicy). Receipts are financial documents at predictable keys. #310 makes that bucket private and is not merged yet. Nothing here depends on public access, but #310 should land before this is deployed.Verification
npx tsc --noEmitclean (frontend + expenditures lambda)next lint— no warnings or errorsnext build— passesNew tests cover the admin-vs-non-admin gating of the review modal, the table's Status/Receipt columns, real upload success/failure, and the three new backend route behaviours.
Not implemented
deniedis retained in the DB constraint but has no UI, per the confirmed status set.🤖 Generated with Claude Code