feat(admin): User Management tab for admins#9379
Open
bbonenfant wants to merge 4 commits into
Open
Conversation
Adds a paginated user list to the god-mode admin screen with deactivate/reactivate support and instance admin badges. - Backend: new GET /api/instances/users/ (paginated, searchable) and PATCH /api/instances/users/<uuid>/ endpoints behind InstanceAdminPermission; is_instance_admin annotated via EXISTS subquery - Frontend: InstanceUserService, InstanceUserStore (MobX), useInstanceUser hook, UserListItem component, and /users page following the workspace screen pattern - Sidebar entry and breadcrumb label added for the new route
…ints 13 tests covering GET /api/instances/users/ and PATCH /api/instances/users/<pk>/ — auth guards, bot exclusion, is_instance_admin annotation, search, deactivate/reactivate, self-deactivation guard, and 404 handling.
- PATCH endpoint now annotates the queryset with is_instance_admin so the
response accurately reflects admin status; previously serialized as false
for all users, stripping the badge in the UI after deactivate/reactivate
- Reject non-boolean is_active values with 400 to prevent bool("false") => True
coercion; drop redundant bool() cast
- Replace raw <button> in UserListItem with <Button> from @plane/propel/button
using error-outline/secondary variants to match the design system
- Remove getUserById from makeObservable (pure read, not a state mutation);
relabel interface section to helpers
- Wrap loader = "loaded" in finally blocks with runInAction for correct
MobX 6 async action semantics
- Add contract tests for non-boolean rejection and is_instance_admin
preservation after PATCH (15 tests total, all passing)
bbonenfant
requested review from
dheeru0198,
pablohashescobar and
sriramveeraghanta
as code owners
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds instance-user listing and activation management across the API, shared types, frontend service/store wiring, and admin UI, including a new users page, row action component, and navigation entries. ChangesInstance User Management
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant UserManagementPage
participant InstanceUserStore
participant InstanceUserService
participant InstanceUserEndpoint
Admin->>UserManagementPage: open /users/
UserManagementPage->>InstanceUserStore: fetchUsers()
InstanceUserStore->>InstanceUserService: list()
InstanceUserService->>InstanceUserEndpoint: GET /api/instances/users/
InstanceUserEndpoint-->>InstanceUserService: paginated users
InstanceUserService-->>InstanceUserStore: results and pagination
InstanceUserStore-->>UserManagementPage: userIds and loader state
sequenceDiagram
participant Admin
participant UserListItem
participant InstanceUserStore
participant InstanceUserService
participant InstanceUserEndpoint
Admin->>UserListItem: click activate/deactivate
UserListItem->>InstanceUserStore: updateUser(userId, is_active)
InstanceUserStore->>InstanceUserService: update(userId, data)
InstanceUserService->>InstanceUserEndpoint: PATCH /api/instances/users/pk/
InstanceUserEndpoint-->>InstanceUserService: updated user
InstanceUserService-->>InstanceUserStore: updated user
InstanceUserStore-->>UserListItem: refreshed row state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/admin/store/instance-user.store.ts (1)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer direct assignment over
lodash.setfor MobX observables insiderunInAction.Using
set(this.users, [user.id], user)andset(this, "paginationInfo", paginationInfo)is unnecessarily indirect. InsiderunInAction, direct assignment is idiomatic MobX and clearer. If this pattern is used elsewhere in the codebase for consistency, disregard.♻️ Suggested refactor
runInAction(() => { const { results, ...paginationInfo } = data; results.forEach((user: IUser) => { - set(this.users, [user.id], user); + this.users[user.id] = user; }); - set(this, "paginationInfo", paginationInfo); + this.paginationInfo = paginationInfo; });🤖 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 `@apps/admin/store/instance-user.store.ts` around lines 70 - 73, The `instance-user.store` update logic in `runInAction` is using `lodash.set` for MobX state writes, which is unnecessarily indirect. Update the `set(this.users, [user.id], user)` and `set(this, "paginationInfo", paginationInfo)` assignments to direct property assignment inside the same action, keeping the `users` map/object and `paginationInfo` update explicit and consistent with MobX style.apps/admin/app/(all)/(dashboard)/users/page.tsx (1)
28-28: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSurface fetch errors to the user.
useSWRcaptures fetch failures in itserrorfield, but the page doesn't destructure or render it. On API errors, the store'sfinallyblock setsloaderto"loaded", so the page shows an empty list ("• 0") with no items and no error message — users get zero feedback. As per coding guidelines, error handling should log errors appropriately and surface them to users.♻️ Suggested error state
- useSWR("INSTANCE_USERS", () => fetchUsers()); + const { error } = useSWR("INSTANCE_USERS", () => fetchUsers());And conditionally render an error state before the loader check:
header={{ title: "Users on this instance", description: "View all users and deactivate or reactivate their accounts.", }} > + {error ? ( + <div className="flex items-center justify-center py-8 text-red-500"> + Failed to load users. Please try again. + </div> + ) : loader !== "init-loader" ? ( {loader !== "init-loader" ? (🤖 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 `@apps/admin/app/`(all)/(dashboard)/users/page.tsx at line 28, The Users page is ignoring the `useSWR("INSTANCE_USERS", () => fetchUsers())` error state, so fetch failures are hidden behind the normal empty/loaded UI. Update the `useSWR` usage in the users page component to destructure `error`, log it appropriately, and render an explicit error state before the existing loader/empty-list checks so users see a clear failure message instead of “0” items.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.
Inline comments:
In `@apps/admin/store/instance-user.store.ts`:
- Around line 64-82: fetchUsers in instance-user.store.ts is merging each
refresh into this.users without removing old entries, so stale users remain
after revalidation. Update fetchUsers to clear the existing users map before
iterating over data.results and repopulating it, while keeping the current
paginationInfo update and loader handling in place.
---
Nitpick comments:
In `@apps/admin/app/`(all)/(dashboard)/users/page.tsx:
- Line 28: The Users page is ignoring the `useSWR("INSTANCE_USERS", () =>
fetchUsers())` error state, so fetch failures are hidden behind the normal
empty/loaded UI. Update the `useSWR` usage in the users page component to
destructure `error`, log it appropriately, and render an explicit error state
before the existing loader/empty-list checks so users see a clear failure
message instead of “0” items.
In `@apps/admin/store/instance-user.store.ts`:
- Around line 70-73: The `instance-user.store` update logic in `runInAction` is
using `lodash.set` for MobX state writes, which is unnecessarily indirect.
Update the `set(this.users, [user.id], user)` and `set(this, "paginationInfo",
paginationInfo)` assignments to direct property assignment inside the same
action, keeping the `users` map/object and `paginationInfo` update explicit and
consistent with MobX style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76e1d9d9-ec8d-4b4f-b9f9-673e2d299fa1
📒 Files selected for processing (19)
apps/admin/app/(all)/(dashboard)/users/page.tsxapps/admin/app/routes.tsapps/admin/components/common/header/core.tsapps/admin/components/users/list-item.tsxapps/admin/hooks/store/index.tsapps/admin/hooks/store/use-instance-user.tsxapps/admin/hooks/use-sidebar-menu/core.tsapps/admin/hooks/use-sidebar-menu/index.tsapps/admin/store/instance-user.store.tsapps/admin/store/root.store.tsapps/api/plane/license/api/serializers/__init__.pyapps/api/plane/license/api/serializers/user.pyapps/api/plane/license/api/views/__init__.pyapps/api/plane/license/api/views/user.pyapps/api/plane/license/urls.pyapps/api/plane/tests/contract/app/test_instance_user.pypackages/services/src/instance/index.tspackages/services/src/instance/instance-user.service.tspackages/types/src/users.ts
- Replace lodash.set calls with direct property assignment throughout instance-user.store; lodash-es import removed - Clear this.users before repopulating in fetchUsers so deleted or renamed users do not survive SWR revalidation (fetchNextUsers intentionally keeps its merge behaviour for pagination) - Destructure error from useSWR and render an explicit error message instead of showing an empty list when the fetch fails
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
2 participants
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.
Description
Adds a user management tab to the god-mode admin screen. I was surprised upon setting up Plane that this didn't exist. This is currently a simple interface that displays basic user information and allows the admin to deactivate/reactivate user accounts. Hopefully this can act as a launching pad to build out some more substantial user management functionality!
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Related to #3373
Summary by CodeRabbit
is_activevalues and improved handling of bot/unknown users.