chore!: remove deprecated setAdminStatus
#37388
Merged
Conversation
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
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 46300c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the deprecated server method Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Client
participant Server
rect rgba(200,230,255,0.4)
note over Client,Server: Before — single Meteor method
Admin->>Client: toggle admin status
Client->>Server: setAdminStatus(userId, isAdmin)
Server->>Server: validate, authorize, user lookup
Server->>Server: add/remove 'admin' role
Server-->>Client: ack
end
rect rgba(255,235,220,0.4)
note over Client,Server: After — endpoint-based flow (username)
Admin->>Client: toggle admin status
alt adding admin
Client->>Server: roles.addUserToRole(username, 'admin')
else removing admin
Client->>Server: roles.removeUserFromRole(username, 'admin')
end
Server->>Server: validate & authorize, manage role
Server-->>Client: ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37388 +/- ##
=================================================
- Coverage 72.30% 70.95% -1.35%
=================================================
Files 1494 3036 +1542
Lines 77102 104621 +27519
Branches 10935 18432 +7497
=================================================
+ Hits 55746 74236 +18490
- Misses 20940 28438 +7498
- Partials 416 1947 +1531
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/gentle-dingos-retire.md(1 hunks)apps/meteor/app/lib/server/index.ts(0 hunks)apps/meteor/app/lib/server/methods/setAdminStatus.ts(0 hunks)apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx(1 hunks)apps/meteor/client/views/admin/users/hooks/useAdminUserInfoActions.ts(1 hunks)apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/meteor/app/lib/server/methods/setAdminStatus.ts
- apps/meteor/app/lib/server/index.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
.changeset/gentle-dingos-retire.md
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/users/hooks/useAdminUserInfoActions.ts (1)
apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts (1)
useChangeAdminStatusAction(8-43)
apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/client/views/admin/users/hooks/useAdminUserInfoActions.ts (1)
AdminUserAction(15-22)
apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx (1)
apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts (1)
useChangeAdminStatusAction(8-43)
🔇 Additional comments (4)
.changeset/gentle-dingos-retire.md (1)
1-5: LGTM! Changeset properly documents the breaking change.The changeset correctly marks this as a major version bump and clearly describes the removal of the deprecated method.
apps/meteor/client/views/admin/users/hooks/useAdminUserInfoActions.ts (1)
92-92: LGTM! Updated to pass username instead of userId.The change correctly aligns with the new signature of
useChangeAdminStatusActionthat now uses username-based endpoints.apps/meteor/client/views/admin/users/UsersTable/UsersTableRow.tsx (1)
92-92: LGTM! Correctly updated to use username.The change properly passes
usernameto align with the refactored hook that now uses role endpoints.apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts (1)
15-16: LGTM! Correct endpoints for role management.The endpoints properly replace the deprecated
setAdminStatusmethod with the standard role management API.
Comment on lines
+8
to
+12
| export const useChangeAdminStatusAction = ( | ||
| username: IUser['username'] = '', | ||
| isAdmin: boolean, | ||
| onChange: () => void, | ||
| ): AdminUserAction | undefined => { |
Contributor
There was a problem hiding this comment.
Add validation for username parameter.
The username parameter is typed as IUser['username'], which is string | undefined based on the IUser type definition. While it defaults to an empty string, the function should validate that username is non-empty before attempting to call the role endpoints.
Apply this diff to add validation:
export const useChangeAdminStatusAction = (
username: IUser['username'] = '',
isAdmin: boolean,
onChange: () => void,
): AdminUserAction | undefined => {
const { t } = useTranslation();
const dispatchToastMessage = useToastMessageDispatch();
const addUserToRoleEndpoint = useEndpoint('POST', '/v1/roles.addUserToRole');
const removeUserFromRoleEndpoint = useEndpoint('POST', '/v1/roles.removeUserFromRole');
const canAssignAdminRole = usePermission('assign-admin-role');
const changeAdminStatus = useCallback(async () => {
+ if (!username) {
+ dispatchToastMessage({ type: 'error', message: t('error-invalid-user') });
+ return;
+ }
+
try {
if (isAdmin) {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts
around lines 8–12, the function accepts username typed as IUser['username']
(string | undefined) and defaults to '' but does not validate it; add an
explicit guard that treats empty string or undefined username as invalid and
return undefined (no action) before constructing or calling any role endpoints,
and update any internal usage to rely on that guard so no network calls are
attempted with a falsy username.
Comment on lines
13
to
+34
| const changeAdminStatus = useCallback(async () => { | ||
| try { | ||
| await setAdminStatus(userId, !isAdmin); | ||
| const message = isAdmin ? 'User_is_no_longer_an_admin' : 'User_is_now_an_admin'; | ||
| dispatchToastMessage({ type: 'success', message: t(message) }); | ||
| if (isAdmin) { | ||
| await removeUserFromRoleEndpoint({ roleId: 'admin', username }); | ||
| dispatchToastMessage({ type: 'info', message: t('User_is_no_longer_an_admin') }); | ||
| onChange(); | ||
| return; | ||
| } | ||
| await addUserToRoleEndpoint({ roleId: 'admin', username }); | ||
| dispatchToastMessage({ type: 'success', message: t('User_is_now_an_admin') }); | ||
| onChange(); | ||
| } catch (error) { | ||
| dispatchToastMessage({ type: 'error', message: error }); | ||
| } | ||
| }, [userId, dispatchToastMessage, isAdmin, onChange, setAdminStatus, t]); | ||
| }, [isAdmin, removeUserFromRoleEndpoint, username, dispatchToastMessage, addUserToRoleEndpoint, t, onChange]); |
Contributor
There was a problem hiding this comment.
Fix error message handling in catch block.
At line 32, the error object is passed directly to dispatchToastMessage as the message. Toast messages expect a string, but error might be an Error object or have a different structure.
Apply this diff to properly extract the error message:
} catch (error) {
- dispatchToastMessage({ type: 'error', message: error });
+ dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const changeAdminStatus = useCallback(async () => { | |
| try { | |
| await setAdminStatus(userId, !isAdmin); | |
| const message = isAdmin ? 'User_is_no_longer_an_admin' : 'User_is_now_an_admin'; | |
| dispatchToastMessage({ type: 'success', message: t(message) }); | |
| if (isAdmin) { | |
| await removeUserFromRoleEndpoint({ roleId: 'admin', username }); | |
| dispatchToastMessage({ type: 'info', message: t('User_is_no_longer_an_admin') }); | |
| onChange(); | |
| return; | |
| } | |
| await addUserToRoleEndpoint({ roleId: 'admin', username }); | |
| dispatchToastMessage({ type: 'success', message: t('User_is_now_an_admin') }); | |
| onChange(); | |
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error }); | |
| } | |
| }, [userId, dispatchToastMessage, isAdmin, onChange, setAdminStatus, t]); | |
| }, [isAdmin, removeUserFromRoleEndpoint, username, dispatchToastMessage, addUserToRoleEndpoint, t, onChange]); | |
| const changeAdminStatus = useCallback(async () => { | |
| try { | |
| if (isAdmin) { | |
| await removeUserFromRoleEndpoint({ roleId: 'admin', username }); | |
| dispatchToastMessage({ type: 'info', message: t('User_is_no_longer_an_admin') }); | |
| onChange(); | |
| return; | |
| } | |
| await addUserToRoleEndpoint({ roleId: 'admin', username }); | |
| dispatchToastMessage({ type: 'success', message: t('User_is_now_an_admin') }); | |
| onChange(); | |
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); | |
| } | |
| }, [isAdmin, removeUserFromRoleEndpoint, username, dispatchToastMessage, addUserToRoleEndpoint, t, onChange]); |
🤖 Prompt for AI Agents
In apps/meteor/client/views/admin/users/hooks/useChangeAdminStatusAction.ts
around lines 20 to 34, the catch block passes the raw error object to
dispatchToastMessage which expects a string; change it to extract a safe message
string (for example use error?.message ?? String(error) ??
t('Something_went_wrong')) and pass that as the message value in
dispatchToastMessage({ type: 'error', message: <extracted string> }) so the
toast always receives a string; keep the rest of the catch logic unchanged.
setAdminStatussetAdminStatus
ae7b8da to
8728dd6
Compare
e92059f to
46300c7
Compare
ggazzo
pushed a commit
that referenced
this pull request
Nov 6, 2025
ggazzo
pushed a commit
that referenced
this pull request
Nov 12, 2025
ggazzo
pushed a commit
that referenced
this pull request
Nov 12, 2025
ggazzo
pushed a commit
that referenced
this pull request
Nov 13, 2025
ggazzo
pushed a commit
that referenced
this pull request
Nov 18, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 2, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 2, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 2, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 2, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 2, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 9, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 10, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 15, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 17, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 17, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 17, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 18, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 18, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 18, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 18, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 19, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 19, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 19, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 20, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 20, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 20, 2025
ggazzo
pushed a commit
that referenced
this pull request
Dec 20, 2025
dougfabris
pushed a commit
that referenced
this pull request
Dec 20, 2025
Merged
gaolin1
pushed a commit
to gaolin1/medsense.webchat
that referenced
this pull request
Jan 6, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ARCH-1857
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Breaking Changes
Improvements