[US-14.2 / OBT-247] Derived user role + platform-admin role management endpoint - #101
Conversation
The existing services in app/services/user carry no docstrings; the three new helpers introduced for the derived role feature should follow suit.
…romotion set_user_role deduplicates project_ids before upserting access rows; without coverage a regression would surface as an IntegrityError on the (project_id, user_id) unique constraint.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
📝 WalkthroughWalkthroughUser responses now expose derived roles based on platform-admin status and manager project access. A protected role-update endpoint and service support platform-admin promotion, manager project assignment, demotion, validation, and persistence. ChangesUser role management
Sequence Diagram(s)sequenceDiagram
participant PlatformAdmin
participant UsersAPI
participant UserService
participant ProjectUserAccess
PlatformAdmin->>UsersAPI: PUT /{user_id}/role
UsersAPI->>UserService: set_user_role(role, project_ids)
UserService->>ProjectUserAccess: update manager or member access
UserService-->>UsersAPI: updated UserListResponse
UsersAPI-->>PlatformAdmin: role-aware user response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
app/services/user/build_user_list_response.py-5-5 (1)
5-5: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a concise docstring for this public service function.
Proposed fix
def build_user_list_response(user: User, *, is_manager: bool) -> UserListResponse: + """Build a user response with its derived platform role.""" role: UserRole = (As per coding guidelines, public service functions require “concise docstrings on public service functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/user/build_user_list_response.py` at line 5, Add a concise docstring to the public build_user_list_response function describing that it builds and returns a user list response, while preserving its existing parameters and behavior.Source: Coding guidelines
app/services/user/set_user_role.py-13-19 (1)
13-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required concise docstrings to the new public service API. Both newly introduced public service functions omit the required documentation.
app/services/user/set_user_role.py#L13-L19: add a concise docstring describing role mutation, validation, and the returned role-aware response.app/services/user/get_manager_user_ids.py#L7-L14: add a concise docstring describing manager-ID derivation and optional user filtering.As per coding guidelines, “Keep service functions function-oriented and composable, with concise docstrings on public service functions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/user/set_user_role.py` around lines 13 - 19, Add concise docstrings to the public functions set_user_role in app/services/user/set_user_role.py (lines 13-19) and get_manager_user_ids in app/services/user/get_manager_user_ids.py (lines 7-14); describe role mutation, validation, and the role-aware response for set_user_role, and manager-ID derivation with optional user filtering for get_manager_user_ids.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@app/services/user/build_user_list_response.py`:
- Line 5: Add a concise docstring to the public build_user_list_response
function describing that it builds and returns a user list response, while
preserving its existing parameters and behavior.
In `@app/services/user/set_user_role.py`:
- Around line 13-19: Add concise docstrings to the public functions
set_user_role in app/services/user/set_user_role.py (lines 13-19) and
get_manager_user_ids in app/services/user/get_manager_user_ids.py (lines 7-14);
describe role mutation, validation, and the role-aware response for
set_user_role, and manager-ID derivation with optional user filtering for
get_manager_user_ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: fcc9a2bb-31af-4740-aa4f-126964757daa
📒 Files selected for processing (9)
app/api/users.pyapp/models/user.pyapp/services/user/__init__.pyapp/services/user/build_user_list_response.pyapp/services/user/get_manager_user_ids.pyapp/services/user/set_user_role.pytests/baker.pytests/test_set_user_role.pytests/test_user_service.py
|
@coderabbitai — decisions on this review:
|
|
Tip For best results, initiate chat on the files or code changes.
I won't flag either of these again. ✏️ Learnings added
|
| @@ -53,7 +56,24 @@ async def update_user( | |||
| is_platform_admin=payload.is_platform_admin, | |||
There was a problem hiding this comment.
Just make sure here: the new PUT /{user_id}/role refuses a self role change, but this PATCH still accepts is_platform_admin with no guard, so the admin can demote himself anyway by this route. The lockout is really effective then? Could you explore that @levigtri — if touching the PATCH is out of scope here that's ok, but then maybe the guard is only giving a false sense of protection.
…rmanagerplatform_admin-role
2 participants
[US-14.2 / OBT-247] Derived user role + platform-admin role management endpoint
Summary
This PR exposes a derived platform role for every user returned by the users API and adds a dedicated endpoint for platform admins to manage that role. The role is not stored anywhere new — it is derived on read: a user is a
platform_adminwhenusers.is_platform_adminis set, amanagerwhen they hold manager access on at least one project inproject_user_access, and amemberotherwise. Because the role is fully derived from existing columns, no Alembic migration is needed.The new
PUT /api/users/{user_id}/roleendpoint lets a platform admin promote or demote any other user. Promotion to manager can grant manager access to specific projects (upsertingproject_user_accessrows without duplicates), demotion to member downgrades all of the user's manager accesses, and a lockout guard prevents an admin from changing their own role. The manager-id lookup is done with a single distinct query so listing endpoints stay free of N+1 queries.Changes
rolefield on the user DTO —app/models/user.py:UserListResponsegainsrole: Literal["member", "manager", "platform_admin"](via theUserRolealias) and the newUserRoleUpdaterequest model (role+ optionalproject_ids).app/services/user/get_manager_user_ids.py: returns the set of user ids holding manager access on at least one project in a singleSELECT DISTINCT user_id FROM project_user_access WHERE role = 'manager'query, optionally filtered by a list of user ids. This keepsGET /api/usersat one extra query for the whole page instead of one per user.app/services/user/build_user_list_response.py: buildsUserListResponsefrom the ORMUserplus anis_managerflag, applying the precedenceplatform_admin>manager>member.app/services/user/set_user_role.py: validates the target user (NotFoundError), blocks self-change by the acting admin (AuthorizationError), and applies the role:platform_adminsets the flag and leaves project access untouched;managerclears the flag and upserts manager access for the givenproject_ids(each project validated, existing rows upgraded in place, no duplicate rows) or requires the user to already manage a project whenproject_idsis absent (ValidationError);memberclears the flag and downgrades only that user's manager rows to member. Returns the updatedUserListResponsewith the derived role.app/api/users.py:GET /api/users,GET /api/users/search,GET /api/users/{id}andPATCH /api/users/{id}now compose the manager-id helper with the DTO builder to populaterole; newPUT /api/users/{user_id}/roleendpoint guarded byrequire_platform_admin. Services keep returning ORM users, so existing callers (app/api/auth.py) are unaffected; the router does no direct data access.app/services/user/__init__.py: exposesget_manager_user_ids,build_user_list_responseandset_user_role.tests/baker.py:make_project_user_accessaccepts a keyword-onlyrole(default"member").tests/test_user_service.py: member without access, member with non-manager access, manager with manager access, platform_admin precedence over manager, and a mixed list deriving all three roles.tests/test_set_user_role.py: promotion to platform_admin (access untouched), promotion to manager withproject_ids(new row created and existing member row upgraded, no duplicates), manager withoutproject_idsfor a user who already manages (ok) and for one who doesn't (ValidationError), demotion to member (other users' rows and non-manager rows intact), missing user/project (NotFoundError), and self-change (AuthorizationError).Type of Change
Testing
Manual verification of the new endpoint:
Summary by CodeRabbit
New Features
Tests