Implemented correct user roles Fixes #3106 #3111
Conversation
WalkthroughThe pull request enhances user role management in the People component by updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![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
🧹 Nitpick comments (2)
src/screens/UserPortal/People/People.spec.tsx (1)
14-14: Remove or clarify the commented import.
TheInterfaceMemberimport is no longer used. Consider removing it altogether to avoid confusion and maintain a clean codebase.src/screens/UserPortal/People/People.tsx (1)
25-31: Consider using a more specific type foruserType.
Currently,userTypeis a free-form string. Since it seems to only accept'Admin'or'Member', using a string-literal union or anenumwould help prevent accidental misspellings or other invalid values.-interface InterfaceMember { +type UserType = 'Admin' | 'Member'; -interface InterfaceMember { firstName: string; lastName: string; image: string; _id: string; email: string; - userType: string; + userType: UserType; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/UserPortal/People/People.spec.tsx(1 hunks)src/screens/UserPortal/People/People.tsx(4 hunks)
🔇 Additional comments (6)
src/screens/UserPortal/People/People.tsx (6)
53-55: Good separation of concerns with multiple state variables.
Having separate states for members, allMembers, and admins can improve clarity. Make sure to keep them in sync and avoid redundant data transformations.
137-146: Verify multiple admins scenario.
This logic currently handles only one admin (data2.organizations[0].admins[0]). If your application must handle multiple admins, consider mapping over each admin rather than storing a single one.
Would you like a script to search for references to this admin array usage in other files, confirming whether multi-admin scenarios need to be addressed elsewhere?
150-159: User type assignment seems correct.
The code updates each member’s userType to 'Member' by default unless already 'Admin'. This appears consistent with the PR’s objective of clarifying roles.
183-183: All members list assignment is correct.
When the mode is 0 (All Members), assigning allMembers to members correctly reflects the entire list.
187-187: Admin assignment logic is consistent with the selected mode.
When the mode is 1 (Admins), assigning admins to members ensures the screen only shows admin users. Keep in mind the multi-admin suggestion above.
279-279: Role property usage shows clarity of user type.
Using member.userType for the role field ensures that the UI reflects the newly introduced userType. This aligns with the PR’s goal of displaying correct roles.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3111 +/- ##
=====================================================
+ Coverage 25.37% 89.32% +63.95%
=====================================================
Files 301 322 +21
Lines 7623 8451 +828
Branches 1667 1843 +176
=====================================================
+ Hits 1934 7549 +5615
+ Misses 5562 669 -4893
- Partials 127 233 +106 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/App.tsx (2)
202-205: Route for leaving an organization.
Adding a dedicated route to leave an organization is good for clarity, but confirm you have proper role checks. For instance, a user should presumably not be able to leave if they are the only admin in an org.
218-218: Commented code for chat route.
The chat feature appears to be commented out. If it’s not intended for near-future release, consider removing or move it to a separate WIP branch. This will help maintain a cleaner codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsx(5 hunks)src/screens/UserPortal/People/People.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/People/People.tsx
🔇 Additional comments (5)
src/App.tsx (5)
33-33: Good use of useEffect import.
The addition of useEffect from react is consistent with the usage below and follows React best practices.
42-43: GraphQL queries are now integrated properly.
The useQuery hook and CHECK_AUTH import appear correct; this is a clean way to fetch authentication data via GraphQL. Confirm that the environment and server endpoints are correctly configured to avoid runtime errors.
53-53: New import for organization exit functionality.
LeaveOrganization is introduced here. Ensure that the logic in this component adequately handles edge cases such as a user leaving an org when they're the sole admin, or membership is mandatory under certain conditions.
55-55: Initialization of useLocalStorage.
Consider verifying that getItem calls (if any) happen in a safe manner. Currently, only setItem usage is shown. This looks straightforward, but keep an eye on potential stale or missing data issues.
102-113: Data-driven authentication setup.
Storing user info in local storage is a reasonable approach for quick session management. However, ensure session tokens or other sensitive credentials, if present, are not inadvertently exposed. If a token is being used, consider using HTTP-only cookies for stronger security.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/People/People.tsx(4 hunks)
🔇 Additional comments (5)
src/screens/UserPortal/People/People.tsx (5)
25-32: LGTM: Interface changes align with role management requirements
The InterfaceMember interface has been correctly updated to use userType for role management.
50-52: LGTM: Well-structured state management
The state variables are properly typed and follow a clear separation of concerns between all members, filtered members, and admins.
126-136: LGTM: Proper member data handling
The member data processing correctly preserves existing userType while mapping the data.
248-248: LGTM: UI correctly displays user roles
The PeopleCard component now properly uses the userType property for role display.
114-123:
Fix admin data handling to support multiple admins
The current implementation assumes only one admin exists by accessing data2.organizations[0].admins[0]. This could lead to missing other admins in the organization.
Consider updating the code to handle multiple admins:
useEffect(() => {
if (data2) {
- const admin = data2.organizations[0].admins[0];
- const updatedAdmin: InterfaceMember = {
- ...admin,
- userType: 'Admin',
- };
- setAdmins([updatedAdmin]);
+ const updatedAdmins = data2.organizations[0].admins.map((admin: any) => ({
+ ...admin,
+ userType: 'Admin',
+ }));
+ setAdmins(updatedAdmins);
}
}, [data2]);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/App.tsx(4 hunks)src/screens/UserPortal/People/People.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/People/People.tsx
🔇 Additional comments (3)
src/App.tsx (3)
100-100: Make sure to handle GraphQL errors.
The useQuery(CHECK_AUTH) currently destructures { data, loading }. Consider also handling the error property to ensure fallback logic or user notifications are invoked if the query fails or returns an error.
104-111: Validate security of user data in local storage.
While useLocalStorage is convenient, consider encrypting or carefully handling sensitive user data (e.g., email, ID) to mitigate potential security vulnerabilities.
202-205: Verify LeaveOrganization component integration.
The route for leaving an organization is correctly added and secured. However, please ensure:
- The LeaveOrganization component handles the orgId parameter correctly
- There are proper confirmations before leaving an organization
- The component updates the user's organization list after successful operation
Run the following script to verify the component implementation:
✅ Verification successful
LeaveOrganization component is well-implemented with all required safeguards
The component implementation has been verified and includes all the necessary features:
- ✓ Correctly handles orgId parameter using
useParamshook - ✓ Implements a two-step confirmation process:
- Initial confirmation dialog
- Email verification step
- ✓ Proper error handling with:
- Network error handling
- Email verification validation
- Loading states
- ✓ Updates user's organization list after leaving via
refetchQueries - ✓ Navigates to organizations page after successful operation
- ✓ Uses toast notifications for success feedback
The implementation is robust and includes additional safety features like keyboard support, loading states, and accessibility attributes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LeaveOrganization component implementation
# Check if the component handles orgId parameter
ast-grep --pattern 'const { orgId } = useParams()' 'screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx'
# Check for confirmation dialog
ast-grep --pattern 'confirm($$$)' 'screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx'
# Check for proper error handling
ast-grep --pattern 'catch($$$)' 'screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx'
# Check for navigation after leaving
ast-grep --pattern 'navigate($$$)' 'screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx'
Length of output: 819
Script:
#!/bin/bash
# Let's first find the correct location of the LeaveOrganization component
fd -e tsx -e ts -e js -e jsx LeaveOrganization
# Once we find the file, let's see its content
fd -e tsx -e ts -e js -e jsx LeaveOrganization --exec cat {}
Length of output: 22869
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
@palisadoes all checks passed please review |
palisadoes
merged commit cbba422
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
It introduces correct role for admin and user.
Issue Number:
Fixes #3106
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
__typenamewith a more descriptiveuserTypeproperty.Tests