407 update updates#454
Conversation
Introduce an optional shouldUpdate parameter to Group.setMemberRoles and Group.removeMemberRoles to control whether changes are immediately persisted. These methods now skip calling update() when shouldUpdate is false, allowing callers to batch multiple role mutations and perform a single update. Call sites updated: removed a redundant update in Project.removeMember flow and adjusted memberRouter to pass shouldUpdate=false for intermediate role changes, then call group.update() once after all changes. This reduces redundant DB writes and improves efficiency.
thehabes
left a comment
There was a problem hiding this comment.
Survived manual stress testing of the endpoints. No regressions or errors discovered.
/static-review did not find anything of note.
Static Review Comments
Branch: 407-update-updates
PR: #454 — 407 update updates
Review Date: 2026-02-24
Reviewer: Claude Code assisted by Bryan - @thehabes
Bryan and Claude make mistakes. Verify all issues and suggestions. Avoid unnecessary scope creep.
Summary
The change is well-scoped (3 files, +28/-18 lines) and all existing behavior is preserved via shouldUpdate = true defaults. Manual API testing against a live instance confirmed all endpoints work correctly, and npm run allTests passes cleanly (188/188 tests pass).
| Category | Issues Found |
|---|---|
| 🔴 Critical | 0 |
| 🟠 Major | 0 |
| 🟡 Minor | 0 |
| 🔵 Suggestions | 0 |
Critical Issues 🔴
None.
Major Issues 🟠
None.
Minor Issues 🟡
None.
Suggestions 🔵
None.
Manual API Testing Report
All Group management endpoints were tested against a live instance (localhost:3012) with authenticated requests using a valid Bearer token.
| # | Test | Expected | Actual | Status |
|---|---|---|---|---|
| 1 | GET / health check |
200 | 200 | ✅ |
| 2 | GET /project/:id with auth |
200 + project JSON | 200 | ✅ |
| 3a-g | All 7 endpoints without auth | 401 or 400 | 400 (auth library behavior) | ✅ |
| 4 | GET /project/:id/customRoles |
200 + custom roles | 200 {TEAM MANAGER: [...]} |
✅ |
| 5 | POST addRoles — CONTRIBUTOR to VIEWER member |
200 | 200, roles [VIEWER, CONTRIBUTOR] |
✅ |
| 6 | Verify addRoles persisted | [VIEWER, CONTRIBUTOR] |
[VIEWER, CONTRIBUTOR] |
✅ |
| 7 | POST removeRoles — remove CONTRIBUTOR |
204 | 204, roles [VIEWER] |
✅ |
| 8 | PUT setRoles — replace all roles with CONTRIBUTOR |
200 | 200, roles [CONTRIBUTOR] |
✅ |
| 9 | POST addRoles — OWNER role blocked |
400 | 400 Cannot assign OWNER role |
✅ |
| 10 | POST removeRoles — last role blocked |
400 | 400 Cannot remove the last role |
✅ |
| 11 | POST removeRoles — OWNER blocked at router |
400 | 400 The OWNER role cannot be removed |
✅ |
| 12 | PUT setRoles — restore VIEWER |
200 | 200 | ✅ |
| 13 | POST addRoles — non-existent member |
404 | 404 Member not found |
✅ |
| 14 | POST switch/owner — self-transfer |
400 | 400 Cannot transfer ownership to the current owner |
✅ |
| 15 | POST switch/owner — missing newOwnerId |
400 | 400 Provide the ID of the new owner |
✅ |
| 16 | POST switch/owner — empty body |
400 | 400 Request body is required |
✅ |
| 17 | POST invite-member — missing email |
400 | 400 Invitee's email is required |
✅ |
| 18 | POST invite-member — invalid email |
400 | 400 Invitee email is invalid |
✅ |
| 19 | POST remove-member — missing userId |
400 | 400 User ID is required |
✅ |
| 20 | POST switch/owner — transfer to collaborator |
200 | 200 Ownership successfully transferred |
✅ |
| 20b | Verify post-transfer roles | Bryan: [LEADER], Collab: [VIEWER, OWNER] |
Matched | ✅ |
| 21b | Non-owner tries switch/owner |
403 | 403 You do not have permission |
✅ |
| 22 | POST addRoles — array body format ["CONTRIBUTOR"] |
200 | 200 | ✅ |
| 24 | PUT setRoles — space-delimited string "VIEWER CONTRIBUTOR" |
200 | 200, both roles set | ✅ |
| 25 | POST addRoles — case insensitivity "leader" |
200, uppercased | 200, roles include LEADER |
✅ |
| 26 | POST addRoles — fake project ID |
error | 500 Cannot read properties of null |
|
| 27 | POST addRoles — empty body {} |
400 | 400 Invalid roles |
✅ |
| 28 | PUT setRoles — empty body {} |
400 | 400 Invalid roles |
✅ |
| 29 | POST addRoles — duplicate role (idempotency) |
200, no duplicates | 200, [VIEWER] (no dup) |
✅ |
| 30 | POST addCustomRoles |
201 | 201 Custom roles added |
✅ |
| 31 | Verify custom role added | TEST_ROLE present |
Present | ✅ |
| 32 | DELETE removeCustomRoles |
200 | 200 Custom roles removed |
✅ |
| 33 | Verify custom role removed | TEST_ROLE gone |
Gone | ✅ |
| — | validateGroup() safety net |
Auto-restores OWNER to creator | Confirmed working | ✅ |
| — | Data restored to original state | Bryan: [OWNER, LEADER], Collab: [VIEWER] |
Matched | ✅ |
| — | npm run allTests |
All pass | 188 passed, 0 failed | ✅ |
Result: 33/33 tests passing. 0 regressions. 1 pre-existing issue noted (test 26).
This pull request refactors the role management methods in the
Groupclass to allow more granular control over when changes are persisted to the database. It introduces a newshouldUpdateparameter to key methods, enabling callers to decide whether to immediately persist changes or defer updates. This change reduces redundant database writes and simplifies related router and project logic.Role management API improvements
shouldUpdateparameter (defaulting totrue) tosetMemberRoles,addMemberRoles,removeMemberRoles, andremoveMembermethods inGroup.js, allowing control over whether changes are persisted immediately. [1] [2] [3] [4] [5] [6]Group.jsto conditionally callupdate()based onshouldUpdate, reducing unnecessary database operations. [1] [2] [3]Integration and usage updates
addMemberRolesinGroup.jsto passshouldUpdate = falsewhen used in initialization logic, preventing redundant updates.update()calls inProject.jsandproject/memberRouter.jssince updates are now handled within the role management methods when appropriate. [1] [2]shouldUpdateparameter, deferring updates until all role changes are complete.