feat(projects): make admins implicit members of every project - #379
Merged
Merged
Conversation
Admins already reach every project through is_admin, so a membership row for one was a weaker second copy of that flag. They are now composed into a project's roster instead of stored in it: listRoster returns every admin first, labelled with the display-only ADMIN_MEMBER_ROLE, then the stored memberships with any admin among them dropped rather than shown twice. Because they are never stored, they are not assignable either. The staff picker roster excludes them, so no admin gets a role dropdown, and POST/PUT reject an admin id outright rather than writing a row the roster would ignore. PROJECT_ROLES is untouched -- it is still what may be written, and the CHECK still rejects Admin. ADMIN_MEMBER_ROLE sits beside it as a label only. Headcounts follow the roster: the rollup counts stored rows, so both the list card and the dashboard add the admins and subtract any admin that still holds a row. That keeps the count right without a migration, which is why leftover admin memberships are fine to leave in place. Seed gains three non-admin staff and hands them the memberships -- every seeded user was an admin, so an admin-filtered picker had nobody to offer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four distinct causes, not one: - rollups.e2e asserted the trigger's counter through GET /projects, which now composes admins in. Its two membership cases read project_rollup directly instead, so they test the trigger they are named for rather than the API's headcount. - project-page assigned seed users 1-3, who are admins and are now rejected. Moved to the new non-admin seed ids, and its roster assertions filter the implicit admins out rather than hard-coding a total that moves whenever the admin count does. - projects.e2e hard-coded ids 4 and 5 for the two non-admin users it inserts, which the three new seed users displaced -- so "non-member" was silently a seeded Director and got a 200. The insert now RETURNINGs the ids. - dashboard.unit feeds selectFrom a fixed queue of mocked chains; the admin headcount query needed entries, and staff_count now includes the admin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nourshoreibah
marked this pull request as ready for review
August 25, 2026 03:17
The three new non-admin seed users broke four more suites, all for the same underlying reason: fixtures named "director" or "student" were seed users 1-3, who held those memberships only because there was nobody else to hold them. Those rows moved to users 4-6, so the fixtures now point there and once again mean what they are named. - donors: `authenticatedUser` was user 1 and reached the roster by directing project 1. Every donors:view test 403'd. Its `studentUser` was passing but vacuously -- user 3 now directs nothing *and* belongs to nothing, so it no longer showed "a member who directs nothing". Both repointed. - expenditures: same three fixtures. Two DELETE cases flipped 403 -> 404 rather than failing outright, because a non-member cannot see the row at all. - Seeded expenditures moved to entered_by 4-6 to match, so "the submitter" is still a member of the project they filed against. - users: the roster count is 6, and `user_id` 4 is a real user now, so the PATCH-404 case uses an id that does not exist. - auth: the seeded-invitation contract is about admins, so it selects them rather than asserting the whole table is three rows. Verified: projects 167, expenditures 128, reports 112, users 68 all pass; donors 59/60 and auth 82/85, the remainder being the cases that need the dev-server on :3000 that CI starts and a local run does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Change
Admins already reach every project through
is_admin, so aproject_membershipsrow for one was a weaker second copy of that flag. They are now composed into a project's roster rather than stored in it.listRoster()returns every admin first, labelledADMIN_MEMBER_ROLE, then the stored memberships with any admin among them dropped rather than listed twice. Used by bothGET /projects/{id}/overviewandGET /projects/{id}/members.StaffCardalready renderstitleas<Name>, <title>, so "Ada Lovelace, Admin" falls out with no frontend change.GET /projects/assignable-stafffilters admins out (is_admin IS NOT TRUE, matching the codebase's strict=== true), so no admin ever reaches the picker and none gets a role select.POST /projectsandPUT /projects/{id}reject an admin id with a 400 instead of writing a row the roster would ignore.ProjectFormModaldrops admins when it seeds from the overview roster, so an edit cannot post one back.PROJECT_ROLESis unchanged — stillDirector | Student, still what the CHECK accepts.ADMIN_MEMBER_ROLEsits beside it as a display label only.No migration
You said leftover admin membership rows are fine, so there is none. The roster dedupes them, and the headcounts subtract them:
member_counton the list card andstaff_counton the dashboard arerollup − storedAdmins + admins. That is correct whether or not those rows exist, so the card and the roster below it always agree.Seed
Every seeded user was an admin, so an admin-filtered picker had nobody to offer and the dev UI could not assign anyone. Added three non-admin staff (Sam Okafor, Priya Raman, Diego Alvarez) and moved the seeded memberships onto them.
Verified
apps/frontend:tsc --noEmitclean, 38/38 suites and 367 tests pass.apps/backend/lambdas/projects:tsc --noEmitclean.shared/rbacbuilds.What still needs doing
apps/backend/lambdas/projects— 4 suites, 49 tests failing. I ran them but had not diagnosed them before pushing. Known causes from the change itself:project-page.unit.test.ts— assigns admin ids 1/2/3, which now 400. Needs the new non-admin ids 4/5/6. Alsoexpect(staff.length).toBeGreaterThan(0)onassignable-staff, and member-count assertions that now include the admins.projects.e2e.test.ts— hardcodes user ids created after the seed; the three new seed users shift them. Plusstaff_countassertions.rollups.e2e.test.ts— its shared invariant comparesproject_rollup.member_countagainstCOUNT(*)on the table. My change does not touch the rollup, so this one I have not explained yet and it needs looking at first.dashboard.unit.test.ts— mocked rows with fixedstaff_count.Also not done: no test yet covering admin-first ordering, the 400 on assigning an admin, or the "Name, Admin" rendering.
🤖 Generated with Claude Code