[WEB-2587] fix: hide log work button for guest user #5787
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/components/issues/issue-detail/issue-activity/root.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (1)
web/core/components/issues/issue-detail/issue-activity/root.tsx (1)
123-123: Conditional rendering logic is correctly updatedThe use of
isWorklogButtonEnabledensures that theIssueActivityWorklogCreateButtonis displayed only when appropriate, based on the user's permissions and the issue type. This enhances the user experience by hiding the worklog button from guest users and on intake issues.
| const { projectPermissionsByWorkspaceSlugAndProjectId } = useUserPermissions(); | ||
| const { getProjectById } = useProject(); | ||
| //derived values | ||
| const isGuest = projectPermissionsByWorkspaceSlugAndProjectId(workspaceSlug, projectId) === EUserPermissions.GUEST; | ||
| const isWorklogButtonEnabled = !isIntakeIssue && !isGuest; |
There was a problem hiding this comment.
💡 Codebase verification
Handle potential undefined permissions
It has been confirmed that projectPermissionsByWorkspaceSlugAndProjectId can return undefined. To prevent unintended behavior, please add error handling or default values where necessary.
web/core/store/user/permissions.store.tsreturnsEUserPermissions | undefined- Ensure all usages of
projectPermissionsByWorkspaceSlugAndProjectIdhandleundefinedappropriately
🔗 Analysis chain
Handle potential undefined permissions
Ensure that projectPermissionsByWorkspaceSlugAndProjectId always returns a valid permission level. If it can return undefined or null, the comparison in line 41 may cause unintended behavior. Consider adding error handling or a default value to prevent runtime errors.
Run the following script to verify that projectPermissionsByWorkspaceSlugAndProjectId always returns a valid permission:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'projectPermissionsByWorkspaceSlugAndProjectId' always returns a valid permission.
# Test: Find all return statements in 'projectPermissionsByWorkspaceSlugAndProjectId' implementations. Expect: Should not return undefined or null.
rg --type=typescript -A 5 -B 5 'function projectPermissionsByWorkspaceSlugAndProjectId'
Length of output: 124
Script:
#!/bin/bash
# Description: Verify that 'projectPermissionsByWorkspaceSlugAndProjectId' always returns a valid permission.
# Test: Find all return statements in 'projectPermissionsByWorkspaceSlugAndProjectId' implementations. Expect: Should not return undefined or null.
rg --type=ts -A 5 -B 5 'function projectPermissionsByWorkspaceSlugAndProjectId'
Length of output: 81
Script:
#!/bin/bash
# Description: Search for all declarations and exports of 'projectPermissionsByWorkspaceSlugAndProjectId' across TypeScript files.
rg --type=ts -e 'projectPermissionsByWorkspaceSlugAndProjectId' --context-separator=''
Length of output: 1892
There was a problem hiding this comment.
@sharma01ketan Thank you for addressing the issue!
(_/)
(^^)
(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
sharma01ketan
changed the title
| const { getProjectById } = useProject(); | ||
| //derived values | ||
| const isGuest = (projectPermissionsByWorkspaceSlugAndProjectId(workspaceSlug, projectId) ?? EUserPermissions.GUEST) === EUserPermissions.GUEST; |
There was a problem hiding this comment.
Use allowPermissions function here
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
web/core/components/issues/issue-detail/issue-activity/root.tsx (1)
46-47: LGTM with suggestion: New derived values for permission checks.The new derived values
isGuestandisWorklogButtonEnabledare correctly implemented and provide the necessary logic for controlling the visibility of the worklog button.However, as suggested in a previous review, consider using the
allowPermissionsfunction for consistency with the rest of the codebase:const isGuest = !allowPermissions(projectPermissionsByWorkspaceSlugAndProjectId(workspaceSlug, projectId), [EUserPermissions.MEMBER, EUserPermissions.ADMIN]);This approach would be more aligned with the existing permission checking patterns in the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- web/core/components/issues/issue-detail/issue-activity/root.tsx (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
web/core/components/issues/issue-detail/issue-activity/root.tsx (4)
14-14: LGTM: New imports for user permissions.The new imports for
useUserPermissionsandEUserPermissionsare correctly added and necessary for the permission-based logic introduced in the component.Also applies to: 19-19
43-43: LGTM: New hook usage for user permissions.The addition of
projectPermissionsByWorkspaceSlugAndProjectIdfromuseUserPermissionsis correctly placed and necessary for the new permission-based logic.
Line range hint
147-153: LGTM: Updated conditional rendering for worklog button.The conditional rendering of
IssueActivityWorklogCreateButtonnow correctly uses theisWorklogButtonEnabledderived value. This change properly implements the new permission-based logic for controlling the visibility of the worklog button.
Line range hint
1-186: Summary: Successfully implemented permission-based control for worklog button visibility.The changes in this file successfully implement the requirement to hide the log work button for guest users. The modifications include:
- Adding necessary imports for user permissions.
- Implementing permission checks using the
useUserPermissionshook.- Creating derived values to determine user status and button visibility.
- Updating the conditional rendering of the worklog button based on these checks.
These changes effectively achieve the PR objective of removing the "Log Work" button for Guest Users, enhancing the user interface by ensuring that only authorized users have access to this functionality.
To further improve the code:
- Consider using the
allowPermissionsfunction for consistency with the rest of the codebase, as suggested earlier.- Ensure that this change is properly tested, especially with different user permission scenarios.
Overall, the implementation is solid and achieves the desired outcome.
To verify the impact of these changes, you can run the following script:
[WEB-2587]
This PR aims to remove the "Log Work" button for Guest Users.
Summary by CodeRabbit
New Features
IssueActivitycomponent to conditionally enable or disable the worklog button based on user permissions.Bug Fixes