chore: replace deprecated methods in 8.0.0#6644
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Warning Rate limit exceeded@OtavioStasiak has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds IRoleUser and a new REST endpoint roles.getUsersInPublicRoles. Refactors restApi.getUsersRoles to be version-gated (returns boolean or IRoleUser[]) and removes exports saveUserProfileMethod and setUserPassword. Updates ChangePasswordView and ProfileView to call saveUserProfile with revised parameters and remove two-factor argument passing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as UI
participant Svc as restApi.getUsersRoles
participant SDK as sdk
participant Store as reduxStore
UI->>Svc: getUsersRoles()
Svc->>Store: read serverVersion
alt serverVersion >= 7.10.0
Svc->>SDK: GET roles.getUsersInPublicRoles
SDK-->>Svc: { success, users: IRoleUser[] }
alt success
Svc-->>UI: IRoleUser[]
else failure
Svc-->>UI: false
end
else legacy
Svc->>SDK: methodCall('getUserRoles')
SDK-->>Svc: boolean
Svc-->>UI: boolean
end
sequenceDiagram
autonumber
actor User
participant View as ChangePasswordView / ProfileView
participant Svc as Services.saveUserProfile
participant API as Server
User->>View: Submit (currentPassword, newPassword, ...)
View->>Svc: saveUserProfile(IProfileParams, customFields?)
Svc->>API: POST saveUserProfile
alt success
API-->>Svc: 200 OK
Svc-->>View: resolve
View-->>User: Success
else requires 2FA / error
API-->>Svc: error (e.g., 2FA required)
Svc-->>View: reject(error)
View-->>User: Show error / 2FA prompt
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/views/ProfileView/index.tsx (1)
196-213: Duplicate/contradictory setUser dispatch and shadoweduservariable
- You dispatch setUser up to twice and the second dispatch uses the outer
user(selector) while the innerconst user = { ... }is block-scoped and ignored afterward.- This can overwrite state with stale data and causes unnecessary renders.
Unify to a single dispatch and avoid shadowing.
- if (result) { + if (result) { logEvent(events.PROFILE_SAVE_CHANGES); if ('realname' in params) { params.name = params.realname; delete params.realname; } - if (customFields) { - dispatch(setUser({ customFields, ...params })); - setCustomFields(customFields); - } else { - dispatch(setUser({ ...params })); - const user = { ...getValues(), ...params }; - Object.entries(user).forEach(([key, value]) => setValue(key as any, value)); - } - dispatch(setUser({ ...user, ...params, customFields })); + const updated = { ...user, ...params, ...(customFields ? { customFields } : {}) }; + dispatch(setUser(updated)); + if (customFields) { + setCustomFields(customFields); + } else { + const formValues = { ...getValues(), ...params }; + Object.entries(formValues).forEach(([key, value]) => setValue(key as any, value)); + } EventEmitter.emit(LISTENER, { message: I18n.t('Profile_saved_successfully') }); }app/views/ChangePasswordView/index.tsx (1)
68-75: Broken validation: confirm password compares against a non-existent field
yup.ref('password')should referencenewPassword. As-is, validation can never pass.- .oneOf([yup.ref('password'), null], I18n.t('Passwords_do_not_match')) + .oneOf([yup.ref('newPassword')], I18n.t('Passwords_do_not_match'))
🧹 Nitpick comments (7)
app/definitions/IUser.ts (1)
152-156: Prefer deriving from IUser to prevent type driftIRoleUser duplicates IUser fields. Derive from IUser to keep types in sync if IUser changes.
-export interface IRoleUser { - _id: string; - username: string; - roles: string[]; -} +export interface IRoleUser extends Pick<IUser, '_id' | 'username'> { + roles: string[]; +}app/definitions/rest/v1/roles.ts (1)
77-82: Confirm response shape/pagination for roles.getUsersInPublicRolesIf the server can return large datasets, lack of params (offset/count) could be heavy. Also ensure the response is exactly
{ users: IRoleUser[]; success: boolean }on all supported servers.Would you like me to add optional
offset/countparams and type them behind a version check?app/views/ProfileView/index.tsx (1)
228-236: 2FA code is stored but never sent with the requestGiven
saveUserProfileno longer accepts a 2FA arg, confirmtwoFactor()configures the SDK to include the code (e.g., via headers) on the subsequent request. If not, this retry path won’t succeed.If SDK is already handling it globally, consider removing
twoFactorCodestate to simplify.app/views/ChangePasswordView/index.tsx (1)
134-137: Type casting hides contract issuesCasting to
IProfileParamsworks but masks that many fields are optional in some flows. Consider introducing a dedicated type for password changes (e.g.,IChangePasswordParams) or allowingPartial<IProfileParams>insaveUserProfile.If you want, I can raise a follow-up PR to introduce the dedicated type and adjust
saveUserProfileaccordingly.app/lib/services/restApi.ts (3)
1081-1093: Unclear return contract for getUsersRoles; consider a stable shapeReturning
boolean | IRoleUser[]forces callers to discriminate types and is easy to misuse. Also, legacygetUserRoleslikely doesn’t return a boolean; verify its actual payload.Proposed stable contract:
-export const getUsersRoles = async (): Promise<boolean | IRoleUser[]> => { +export const getUsersRoles = async (): Promise<{ supported: boolean; users: IRoleUser[] }> => { const serverVersion = reduxStore.getState().server.version; if (compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.0.0')) { - const response = await sdk.get('roles.getUsersInPublicRoles'); - if (response.success) { - return response.users; - } - return false; + const response = await sdk.get('roles.getUsersInPublicRoles'); + return { supported: !!response.success, users: response.success ? response.users : [] }; } - return sdk.methodCall('getUserRoles'); + // Back-compat: return "unsupported" with empty users + return { supported: false, users: [] }; };And update call sites accordingly. If you prefer not to change the signature, at least document and enforce type guards at usage.
1085-1088: Type the REST response for safetyIf
sdk.getsupports generics, add a typed response to ensureusers/successare validated at compile time.Example:
const response: { users: IRoleUser[]; success: boolean } = await sdk.get('roles.getUsersInPublicRoles');
631-637: Consider accepting Partial
users.updateOwnBasicInfoaccepts partial data. AllowingPartial<IProfileParams>would remove the need for casts across callers.-export const saveUserProfile = ( - data: IProfileParams | Pick<IProfileParams, 'username' | 'name'>, +export const saveUserProfile = ( + data: Partial<IProfileParams>, customFields?: { [key: string | number]: string } ) => sdk.post('users.updateOwnBasicInfo', { data, customFields });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/definitions/IUser.ts(1 hunks)app/definitions/rest/v1/roles.ts(2 hunks)app/lib/services/restApi.ts(2 hunks)app/views/ChangePasswordView/index.tsx(3 hunks)app/views/ProfileView/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/definitions/rest/v1/roles.ts (1)
app/definitions/IUser.ts (1)
IRoleUser(152-156)
app/lib/services/restApi.ts (2)
app/definitions/IUser.ts (1)
IRoleUser(152-156)app/lib/methods/helpers/compareServerVersion.ts (1)
compareServerVersion(10-15)
app/views/ChangePasswordView/index.tsx (2)
app/lib/services/index.ts (1)
Services(4-7)app/definitions/IProfile.ts (1)
IProfileParams(3-12)
app/views/ProfileView/index.tsx (1)
app/lib/services/index.ts (1)
Services(4-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (2)
app/definitions/rest/v1/roles.ts (1)
3-3: Import change looks goodapp/lib/services/restApi.ts (1)
8-8: Import looks good
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/views/ChangePasswordView/index.tsx (1)
108-112: Address inconsistent currentPassword hashing.The
currentPasswordis sent in plain text here, while the profile view path (line 134) hashes it withsha256. This inconsistency may cause server-side validation issues or security concerns.Apply this diff to hash the currentPassword consistently:
- const { newPassword, currentPassword } = inputValues; + const { newPassword, currentPassword } = inputValues; try { setValue('saving', true); - await Services.saveUserProfile({ newPassword, currentPassword: sha256(currentPassword) } as IProfileParams); + await Services.saveUserProfile({ newPassword, currentPassword: sha256(currentPassword) } as IProfileParams);
🧹 Nitpick comments (1)
app/views/ChangePasswordView/index.tsx (1)
135-135: Consider removing the unnecessary empty line.There's an empty line that doesn't add value to code readability.
Apply this diff to remove the unnecessary empty line:
const params = { currentPassword: sha256(currentPassword), newPassword, username, email: emails?.[0].address || '' }; - const result = await Services.saveUserProfile(params);
📜 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 (1)
app/views/ChangePasswordView/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/views/ChangePasswordView/index.tsx (2)
app/lib/services/index.ts (1)
Services(4-7)app/definitions/IProfile.ts (1)
IProfileParams(3-12)
🔇 Additional comments (2)
app/views/ChangePasswordView/index.tsx (2)
10-10: LGTM! Added necessary type import.The import of
IProfileParamsis correctly added to support the new typing requirements for the password change operations.
134-136: LGTM! Proper parameter construction and API call.The parameters are correctly constructed with hashed
currentPassword, and the simplifiedsaveUserProfilecall aligns with the deprecated method removal objectives. The type casting toIProfileParamsis appropriate.
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
* feat: deprecate getUserRoles * feat: remove deprecated method setUserPassword and setUserProfileMethod * fix: comments * fix: encrypt currentPassword * fix: change serverVersion to compare in getUserRoles
2 participants
Proposed changes
replace deprecated methods in 8.0.0, related to RocketChat/Rocket.Chat#36146.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-942
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Refactor