feat(expenses): admin approve/deny review flow with receipt upload - #313
Closed
nourshoreibah wants to merge 3 commits into
Closed
feat(expenses): admin approve/deny review flow with receipt upload#313nourshoreibah wants to merge 3 commits into
nourshoreibah wants to merge 3 commits into
Conversation
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>
Contributor
This PR contains a database migration
It will be applied to the production database automatically when this PR merges, before the new lambda code is deployed. Please confirm before requesting review:
This PR also changes 31 files outside |
- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
1 similar comment
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Contributor
Terraform Plan 📖
|
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Collaborator
Author
|
Superseded by a stacked pair, split so the migration and IAM land before the code that depends on them:
Rebuilding on current |
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).
Status vocabulary
The design is internally inconsistent — the table pill reads Needs Info, the base status-bar component variant is named
rejected, and the filter menu says "Need Info". Confirmed with the requester: the set is Pending / Approved / Needs Info.No migration was needed for this: the baseline check constraint already allows
needs_more_info. The legacydeniedvalue stays valid in the DB and still renders (greyed) 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.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.next buildfailure (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. Build was 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.
Migration
20260812012951_add_expenditure_admin_notes.sqladds a nullableadmin_notes TEXT(additive, forward-only).Infra
The shared lambda role had no S3 permissions at all, so a presigned PUT would have failed
AccessDenied. Addss3:PutObject/s3:GetObjecton the reports bucket.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. PR #310 makes that bucket private and is not merged yet — this PR deliberately does not touch the bucket policy to avoid conflicting with it, and reads go through presigned GETs so nothing here depends on public access. #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