[WEB-2055] fix: enable admins to change role of other admins and add missing observers#5236
Conversation
WalkthroughThe recent updates enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccountTypeColumn
participant MobXStore
User->>AccountTypeColumn: Requests member data
AccountTypeColumn->>MobXStore: Fetches user roles
MobXStore-->>AccountTypeColumn: Returns user roles
AccountTypeColumn->>AccountTypeColumn: Checks isCurrentUser and isRoleEditable
alt Role is editable
AccountTypeColumn->>User: Displays role selection
else Role is not editable
AccountTypeColumn->>User: Displays role
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
anmolsinghbhatia
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- web/core/components/project/settings/member-columns.tsx (3 hunks)
- web/core/components/workspace/settings/member-columns.tsx (3 hunks)
Additional comments not posted (10)
web/core/components/project/settings/member-columns.tsx (5)
Line range hint
1-10: Imports look good!The import statements for
observeranduseUserare correct and necessary for the new functionality.
84-84: Wrapping withobserverlooks good!The
AccountTypeColumncomponent is now correctly wrapped with theobserverfunction to enable reactivity.
95-95:useUserhook integration looks good!The
useUserhook is correctly integrated to retrieve the current user's data.
98-100: Conditional rendering logic is well-implemented!The derived values
isCurrentUser,isAdminRole, andisRoleEditableare correctly calculated and used for conditional rendering.
104-107: Simplified role display for non-editable roles.The role display logic for non-editable roles is correctly simplified.
web/core/components/workspace/settings/member-columns.tsx (5)
Line range hint
1-10: Imports look good!The import statements for
observeranduseUserare correct and necessary for the new functionality.
83-83: Wrapping withobserverlooks good!The
AccountTypeColumncomponent is now correctly wrapped with theobserverfunction to enable reactivity.
94-94:useUserhook integration looks good!The
useUserhook is correctly integrated to retrieve the current user's data.
97-99: Conditional rendering logic is well-implemented!The derived values
isCurrentUser,isAdminRole, andisRoleEditableare correctly calculated and used for conditional rendering.
103-106: Simplified role display for non-editable roles.The role display logic for non-editable roles is correctly simplified.
| <Controller | ||
| name="role" | ||
| control={control} | ||
| rules={{ required: "Role is required." }} | ||
| render={({ field: { value } }) => ( | ||
| <CustomSelect | ||
| value={value} | ||
| onChange={(value: EUserProjectRoles) => { | ||
| if (!workspaceSlug) return; | ||
|
|
||
| updateMember(workspaceSlug.toString(), projectId.toString(), rowData.member.id, { | ||
| role: value as unknown as EUserProjectRoles, // Cast value to unknown first, then to EUserWorkspaceRoles | ||
| }).catch((err) => { | ||
| console.log(err, "err"); | ||
| const error = err.error; | ||
| const errorString = Array.isArray(error) ? error[0] : error; | ||
|
|
||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: errorString ?? "An error occurred while updating member role. Please try again.", | ||
| }); | ||
| }); | ||
| }} | ||
| label={ | ||
| <div className="flex "> | ||
| <span>{ROLE[rowData.role as keyof typeof ROLE]}</span> | ||
| </div> | ||
| } | ||
| buttonClassName={`!px-0 !justify-start hover:bg-custom-background-100 ${errors.role ? "border-red-500" : "border-none"}`} | ||
| className="rounded-md p-0 w-32" | ||
| optionsClassName="w-full" | ||
| input | ||
| > | ||
| {Object.keys(ROLE).map((item) => ( | ||
| <CustomSelect.Option key={item} value={item as unknown as EUserProjectRoles}> | ||
| {ROLE[item as unknown as keyof typeof ROLE]} | ||
| </CustomSelect.Option> | ||
| ))} | ||
| </CustomSelect> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Role selection logic is well-implemented but improve error handling.
The role selection logic using Controller and CustomSelect is well-implemented. However, consider improving error handling by logging errors using a logging library instead of console.log.
- console.log(err, "err");
+ // Consider using a logging library for better error tracking
+ logger.error("Error updating member role", err);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.
| <Controller | |
| name="role" | |
| control={control} | |
| rules={{ required: "Role is required." }} | |
| render={({ field: { value } }) => ( | |
| <CustomSelect | |
| value={value} | |
| onChange={(value: EUserProjectRoles) => { | |
| if (!workspaceSlug) return; | |
| updateMember(workspaceSlug.toString(), projectId.toString(), rowData.member.id, { | |
| role: value as unknown as EUserProjectRoles, // Cast value to unknown first, then to EUserWorkspaceRoles | |
| }).catch((err) => { | |
| console.log(err, "err"); | |
| const error = err.error; | |
| const errorString = Array.isArray(error) ? error[0] : error; | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: errorString ?? "An error occurred while updating member role. Please try again.", | |
| }); | |
| }); | |
| }} | |
| label={ | |
| <div className="flex "> | |
| <span>{ROLE[rowData.role as keyof typeof ROLE]}</span> | |
| </div> | |
| } | |
| buttonClassName={`!px-0 !justify-start hover:bg-custom-background-100 ${errors.role ? "border-red-500" : "border-none"}`} | |
| className="rounded-md p-0 w-32" | |
| optionsClassName="w-full" | |
| input | |
| > | |
| {Object.keys(ROLE).map((item) => ( | |
| <CustomSelect.Option key={item} value={item as unknown as EUserProjectRoles}> | |
| {ROLE[item as unknown as keyof typeof ROLE]} | |
| </CustomSelect.Option> | |
| ))} | |
| </CustomSelect> | |
| )} | |
| /> | |
| }).catch((err) => { | |
| // Consider using a logging library for better error tracking | |
| logger.error("Error updating member role", err); | |
| const error = err.error; | |
| const errorString = Array.isArray(error) ? error[0] : error; | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: errorString ?? "An error occurred while updating member role. Please try again.", | |
| }); | |
| }); |
| <Controller | ||
| name="role" | ||
| control={control} | ||
| rules={{ required: "Role is required." }} | ||
| render={({ field: { value } }) => ( | ||
| <CustomSelect | ||
| value={value} | ||
| onChange={(value: EUserProjectRoles) => { | ||
| console.log({ value, workspaceSlug }, "onChange"); | ||
| if (!workspaceSlug) return; | ||
|
|
||
| updateMember(workspaceSlug.toString(), rowData.member.id, { | ||
| role: value as unknown as EUserWorkspaceRoles, // Cast value to unknown first, then to EUserWorkspaceRoles | ||
| }).catch((err) => { | ||
| console.log(err, "err"); | ||
| const error = err.error; | ||
| const errorString = Array.isArray(error) ? error[0] : error; | ||
|
|
||
| setToast({ | ||
| type: TOAST_TYPE.ERROR, | ||
| title: "Error!", | ||
| message: errorString ?? "An error occurred while updating member role. Please try again.", | ||
| }); | ||
| }); | ||
| }} | ||
| label={ | ||
| <div className="flex "> | ||
| <span>{ROLE[rowData.role as keyof typeof ROLE]}</span> | ||
| </div> | ||
| } | ||
| buttonClassName={`!px-0 !justify-start hover:bg-custom-background-100 ${errors.role ? "border-red-500" : "border-none"}`} | ||
| className="rounded-md p-0 w-32" | ||
| optionsClassName="w-full" | ||
| input | ||
| > | ||
| {Object.keys(ROLE).map((item) => ( | ||
| <CustomSelect.Option key={item} value={item as unknown as EUserProjectRoles}> | ||
| {ROLE[item as unknown as keyof typeof ROLE]} | ||
| </CustomSelect.Option> | ||
| ))} | ||
| </CustomSelect> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Role selection logic is well-implemented but improve error handling.
The role selection logic using Controller and CustomSelect is well-implemented. However, consider improving error handling by logging errors using a logging library instead of console.log.
- console.log(err, "err");
+ // Consider using a logging library for better error tracking
+ logger.error("Error updating member role", err);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.
| <Controller | |
| name="role" | |
| control={control} | |
| rules={{ required: "Role is required." }} | |
| render={({ field: { value } }) => ( | |
| <CustomSelect | |
| value={value} | |
| onChange={(value: EUserProjectRoles) => { | |
| console.log({ value, workspaceSlug }, "onChange"); | |
| if (!workspaceSlug) return; | |
| updateMember(workspaceSlug.toString(), rowData.member.id, { | |
| role: value as unknown as EUserWorkspaceRoles, // Cast value to unknown first, then to EUserWorkspaceRoles | |
| }).catch((err) => { | |
| console.log(err, "err"); | |
| const error = err.error; | |
| const errorString = Array.isArray(error) ? error[0] : error; | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: errorString ?? "An error occurred while updating member role. Please try again.", | |
| }); | |
| }); | |
| }} | |
| label={ | |
| <div className="flex "> | |
| <span>{ROLE[rowData.role as keyof typeof ROLE]}</span> | |
| </div> | |
| } | |
| buttonClassName={`!px-0 !justify-start hover:bg-custom-background-100 ${errors.role ? "border-red-500" : "border-none"}`} | |
| className="rounded-md p-0 w-32" | |
| optionsClassName="w-full" | |
| input | |
| > | |
| {Object.keys(ROLE).map((item) => ( | |
| <CustomSelect.Option key={item} value={item as unknown as EUserProjectRoles}> | |
| {ROLE[item as unknown as keyof typeof ROLE]} | |
| </CustomSelect.Option> | |
| ))} | |
| </CustomSelect> | |
| )} | |
| /> | |
| }).catch((err) => { | |
| // Consider using a logging library for better error tracking | |
| logger.error("Error updating member role", err); | |
| const error = err.error; | |
| const errorString = Array.isArray(error) ? error[0] : error; | |
| setToast({ | |
| type: TOAST_TYPE.ERROR, | |
| title: "Error!", | |
| message: errorString ?? "An error occurred while updating member role. Please try again.", | |
| }); | |
| }); |
SatishGandham
changed the title
2 participants
Problem:
Solution:
Reference:
[WEB-2055]
Media:
Summary by CodeRabbit
New Features
Bug Fixes