Skip to content

feat(roles): project roles are Admin, Director, Student - #311

Merged
nourshoreibah merged 1 commit into
mainfrom
worktree-project-roles-director-student
Aug 12, 2026
Merged

feat(roles): project roles are Admin, Director, Student#311
nourshoreibah merged 1 commit into
mainfrom
worktree-project-roles-director-student

Conversation

@nourshoreibah

Copy link
Copy Markdown
Collaborator

Project membership roles become Admin, Director, Student.

Mapping

Old New
Admin Admin
PI Director
Accountant Director
Staff Student

Permissions are unchanged

PI and Accountant could both edit; Staff could not. So editableRoles collapses from ['PI', 'Accountant', 'Admin'] to ['Director', 'Admin'] and every existing authorization outcome is preserved. Student is read-only on expenditures, matching Staff today. The global users.is_admin flag and the frontend nav roles (admin/standard/limited) are a separate mechanism and are untouched.

The migration is expand-only

20260812011405_rename_project_roles.sql widens the CHECK to old ∪ new before backfilling, so the currently deployed code can keep writing 'PI'/'Accountant'/'Staff' during the window between the migration and the lambda deploy. A follow-up migration drops the three old names once this is live — it is the contract half and needs -- allow-destructive:.

One transient effect during that window: the still-deployed code checks ['PI','Accountant','Admin'], so a backfilled Director row is denied edit (read is membership-only, so reads are unaffected) until the new lambdas land. Self-healing, no data impact.

Verification

  • expenditures — 107 tests pass (unit + e2e)
  • projectsprojects.e2e 28 pass, delete-authz.unit + dashboard.unit 13 pass
  • donors — 50 pass; health test 🌞 fails, but it fails identically on unmodified main (it fetches localhost:3000/donors/health, which needs the lambda container up)
  • apps/frontendtsc --noEmit clean
  • Post-migration DB state: constraint is old ∪ new, rows are 2 Director / 1 Student

shared/types/db-types.d.ts is unchanged — role is still VARCHAR(30)string, so the generated types can't drift.

Left alone deliberately

The Staff headings in ProjectCard.tsx and ProjectDetailClient.tsx are generic labels for the whole member list, not the role — renaming them to "Students" would mislabel Directors.

🤖 Generated with Claude Code

PI and Accountant both become Director, Staff becomes Student, Admin is
unchanged. Edit rights are unaffected: PI and Accountant could both edit,
Staff could not, so editableRoles collapses from ['PI','Accountant','Admin']
to ['Director','Admin'].

The migration is expand-only. It widens the CHECK to old ∪ new so the
currently deployed code can keep writing 'PI'/'Accountant'/'Staff' during the
window between the migration and the lambda deploy, then backfills the rows.
A follow-up migration drops the three old names once this is live.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

This PR contains a database migration

  • apps/backend/db/migrations/20260812011405_rename_project_roles.sql

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:

  • Applied and tested locally. cd apps/backend && make migrate, then run the affected lambda's tests (cd apps/backend/lambdas/<name> && npm test). make show-migrations shows what applied.
  • Safe for the code that is live right now. During the deploy window -- and indefinitely if the deploy fails -- the currently deployed lambdas run against your new schema. Additive changes (CREATE TABLE, nullable ADD COLUMN, CREATE INDEX) are fine in one PR. DROP COLUMN, renames, ADD COLUMN NOT NULL with no default, and new UNIQUE/CHECK/FOREIGN KEY constraints need two merged PRs -- see the expand/contract rules in apps/backend/db/README.md.
  • Kept separate from unrelated changes. A migration PR should ideally contain the migration, the code that needs it, and nothing else. It changes production state, it is the one thing here that redeploying cannot roll back, and a reviewer should be able to see the whole schema change without scrolling past unrelated work.
  • No already-merged migration was edited. Fix an old migration by adding a new one; there is no down.

shared/types/db-types.d.ts is regenerated and pushed to this branch automatically -- don't hand-edit it. Expect one red migrations-fresh check before that commit lands.

@github-actions

Copy link
Copy Markdown
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.

@nourshoreibah nourshoreibah added the no-review The PR review bot won't run label Aug 12, 2026
@nourshoreibah
nourshoreibah marked this pull request as ready for review August 12, 2026 01:38
@nourshoreibah
nourshoreibah merged commit c8dfcfb into main Aug 12, 2026
22 checks passed
@nourshoreibah
nourshoreibah deleted the worktree-project-roles-director-student branch August 12, 2026 01:38
nourshoreibah added a commit that referenced this pull request Aug 12, 2026
…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 added a commit that referenced this pull request Aug 12, 2026
)

* feat(expenses): admin approve/deny review flow with receipt upload

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>

* fix(expenses): size the status pill and review modal from content

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>

* fix(expenses): use the Director/Admin project roles on the receipt upload 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>

* chore: regenerate lambda READMEs

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-review The PR review bot won't run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant