Feature Added: An option for users to leave the Organization (Fixes #1873) #2629
Conversation
WalkthroughThe pull request introduces a feature that enables users to leave an organization. This is accomplished through the addition of a Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx (3)
8-8: Remove commented-out import statementThe commented import statement should be removed as it's not being used.
-// import useLocalStorage from 'utils/useLocalstorage';
11-21: Consider extracting mock data to a separate fileThe mock implementation of useLocalStorage could be moved to a separate test utils file for reusability across other test files.
55-143: Test coverage looks comprehensive but could be enhancedThe test suite covers the main scenarios well. Consider adding these additional test cases:
- Network error handling
- Loading state verification
- Edge cases with empty/null organization data
- Modal cancellation flow
src/components/IconComponent/IconComponent.tsx (2)
22-22: Group similar imports togetherMove the ExitToAppIcon import to be grouped with other @mui/icons-material imports at the top.
import { QuestionMarkOutlined, ContactPageOutlined, NewspaperOutlined, + ExitToAppIcon, } from '@mui/icons-material'; -import ExitToAppIcon from '@mui/icons-material/ExitToApp';
138-144: Follow consistent pattern for icon propsThe ExitToAppIcon implementation should follow the same pattern as other Material-UI icons in the file by spreading props.
case 'Leave Organization': return ( <ExitToAppIcon data-testid="Icon-Component-Leave-Organization" - stroke={props.fill} + {...props} /> );src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (1)
77-162: Consider implementing i18n for text strings.The component contains hardcoded English text strings. Consider using the i18next translation system that's already set up in the project for better internationalization support.
Example implementation:
- <Modal.Title>Leave Organization</Modal.Title> + <Modal.Title>{t('leaveOrganization.modalTitle')}</Modal.Title>package.json (1)
Conflicting React and React Types versions detected in dependencies
Multiple versions of React-related packages are specified in different sections:
- Dependencies section has React v18.3.1 and @types/react v18.3.12
- Resolutions section pins @types/react to v17.0.2 and @types/react-dom to v17.0.2
- @apollo/client also has conflicting versions: ^3.11.8 vs ^3.4.0-beta.19
This version mismatch between React 18 dependencies and React 17 type definitions can lead to type checking errors and potential runtime issues.
🔗 Analysis chain
Line range hint
164-167: Review package version resolutions.The resolutions block fixes specific versions of React-related packages which might cause compatibility issues with other dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential conflicts in package versions rg -A 1 '"react":|"@types/react":|"@apollo/client":' package.jsonLength of output: 395
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/assets/svgs/leave.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
package.json(1 hunks)public/locales/en/translation.json(1 hunks)public/locales/fr/translation.json(1 hunks)public/locales/hi/translation.json(1 hunks)public/locales/sp/translation.json(1 hunks)public/locales/zh/translation.json(1 hunks)src/App.tsx(2 hunks)src/components/EventDashboardScreen/EventDashboardScreen.tsx(1 hunks)src/components/IconComponent/IconComponent.tsx(2 hunks)src/screens/UserPortal/LeaveOrganization/LeaveOrganization.module.css(1 hunks)src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx(1 hunks)src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx(1 hunks)src/screens/UserPortal/UserScreen/UserScreen.tsx(1 hunks)src/state/reducers/userRoutesReducer.ts(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/screens/UserPortal/LeaveOrganization/LeaveOrganization.module.css
🔇 Additional comments (11)
tsconfig.json (1)
3-3: LGTM! Addition of vitest types supports test implementation
The addition of "vitest" to the types array is appropriate for enabling TypeScript type checking in test files.
src/state/reducers/userRoutesReducer.ts (1)
66-70: LGTM! Component structure follows existing pattern
The new "Leave Organization" component entry follows the established pattern and integrates well with the existing routing structure.
Verify security measures for the leave organization feature
Since leaving an organization is a sensitive operation, ensure proper security measures are in place.
✅ Verification successful
Security measures are properly implemented in the Leave Organization feature
The component implementation shows robust security measures:
- Uses a two-step verification process with email confirmation
- Implements a confirmation modal with clear warnings
- Utilizes user authentication data from local storage
- Proper error handling and loading states
- Uses GraphQL mutation with proper error handling
- Includes navigation guards through user session data
Additional security features:
- User must verify their email before proceeding
- Clear user feedback and error messages
- Proper session management using
Talawa-adminstorage - Immediate session handling post-organization leave
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for authentication/authorization checks in the LeaveOrganization component
# Search for authentication checks
echo "Checking for authentication guards..."
rg -l "useAuth|isAuthenticated|requireAuth" "src/screens/UserPortal/LeaveOrganization"
# Search for confirmation dialogs
echo "Checking for confirmation dialogs..."
rg -l "confirm|dialog|modal|prompt" "src/screens/UserPortal/LeaveOrganization"
# Search for API endpoint security
echo "Checking API endpoint security..."
rg -l "mutation.*leaveOrganization|DELETE.*organization" "src/graphql"
Length of output: 734
Script:
#!/bin/bash
# Let's check the actual implementation of LeaveOrganization component
echo "Checking LeaveOrganization component implementation..."
cat src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx
# Let's also check for GraphQL mutations in the entire codebase
echo "Checking for leave organization mutations..."
rg "mutation.*leaveOrganization|mutation.*LeaveOrganization" -A 5
# Check for auth guards in routing setup
echo "Checking for auth guards in routing..."
rg "LeaveOrganization.*auth|auth.*LeaveOrganization" -A 3
Length of output: 5253
src/screens/UserPortal/UserScreen/UserScreen.tsx (1)
23-23: LGTM! Mapping addition follows existing pattern
The new mapping for the leave organization feature is correctly integrated into the existing structure.
Verify translation integration for the leave organization feature
The AI summary mentions translation additions across multiple languages. Let's verify their implementation.
✅ Verification successful
Translation integration for leave organization feature is properly implemented
The verification confirms that:
- Translation keys for 'leaveOrganization' are present in all language files (en, fr, hi, sp, zh)
- Each language has the correct title translation:
- English: "Leave Organization"
- French: "Quitter l'organisation"
- Hindi: "संगठन छोड़ें"
- Spanish: "Dejar la organización"
- Chinese: "离开组织"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify translation keys for leave organization feature
# Check for leaveOrganization translation key across all language files
echo "Checking translation files for 'leaveOrganization' key..."
rg -l "leaveOrganization" "public/locales"
# Verify translation content
echo "Verifying translation content..."
for lang in en fr hi sp zh; do
echo "Checking $lang translation..."
cat "public/locales/$lang/translation.json" | jq '.leaveOrganization'
done
Length of output: 1373
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (1)
1-21: LGTM! Clean setup with appropriate dependencies.
The component's initialization is well-structured with proper state management and data retrieval.
package.json (1)
159-159: Verify vitest version compatibility.
The vitest version update should be tested to ensure it doesn't introduce any breaking changes in the test suite.
✅ Verification successful
Let me try a different approach to verify the vitest version compatibility.
Let me check for test files and their patterns to ensure compatibility.
Vitest version update appears safe to proceed
The update to vitest v2.1.8 is compatible with the current test setup:
- Test files use standard vitest imports and patterns
- Configuration in vitest.config.ts follows v2.x syntax
- Setup file uses compatible testing-library integration
- No deprecated or removed features are being used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes in vitest
curl -s https://raw.githubusercontent.com/vitest-dev/vitest/main/CHANGELOG.md | head -n 20
Length of output: 107
Script:
#!/bin/bash
# Check for vitest usage in test files
fd -e test.ts -e test.js -e spec.ts -e spec.js | xargs rg "from 'vitest'" -l
# Check package-lock.json for current vitest version
fd package-lock.json -x cat {} | jq '.packages["node_modules/vitest"].version' 2>/dev/null
# Check for vitest configuration
fd vitest.config -e ts -e js -x cat {}
Length of output: 1108
Script:
#!/bin/bash
# Find and examine test files for vitest patterns
fd -e spec.ts -e spec.tsx | xargs rg "^import .* from 'vitest'" -A 2
# Check vitest setup file
cat vitest.setup.ts 2>/dev/null
# Look for any test-related scripts in package.json
fd package.json -x cat {} | jq '.scripts | with_entries(select(.key | contains("test")))'
Length of output: 1453
src/App.tsx (1)
202-205: LGTM! Route implementation is secure and consistent.
The new route is properly implemented within the SecuredRouteForUser component, maintaining security and following the existing routing patterns.
public/locales/zh/translation.json (1)
1408-1410: LGTM! The Chinese translation is accurate and clear.
The translation "离开组织" appropriately conveys "Leave Organization" in Chinese, maintaining both semantic accuracy and natural language flow.
public/locales/en/translation.json (1)
1408-1410: LGTM! The English text is clear and consistent.
The text "Leave Organization" is appropriately concise and clearly conveys the action to users.
public/locales/hi/translation.json (1)
1408-1410: LGTM! The Hindi translation is accurate and natural.
The translation "संगठन छोड़ें" correctly conveys "Leave Organization" in Hindi, using appropriate terminology and grammatical structure.
public/locales/fr/translation.json (1)
1408-1410: LGTM! French translation is accurate.
The translation "Quitter l'organisation" is grammatically correct and appropriately conveys the meaning of "Leave Organization".
public/locales/sp/translation.json (1)
1410-1412: LGTM! Spanish translation is accurate.
The translation "Dejar la organización" is grammatically correct and appropriately conveys the meaning of "Leave Organization".
| const mocks = [ | ||
| { | ||
| request: { | ||
| query: ORGANIZATIONS_LIST, | ||
| variables: { id: 'test-org-id' }, | ||
| }, | ||
| result: { | ||
| data: { | ||
| organizations: [ | ||
| { | ||
| name: 'Test Organization', | ||
| description: 'This is a test organization.', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| request: { | ||
| query: REMOVE_MEMBER_MUTATION, | ||
| variables: { orgid: 'test-org-id', userid: 'test-user-id' }, | ||
| }, | ||
| result: { | ||
| data: { | ||
| removeMember: { | ||
| success: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add GraphQL error scenarios to test cases
The component has proper error handling implemented (error state, error display, and mutation error handling), but the test mocks don't cover GraphQL error scenarios. Add the following test cases:
- Test case for mutation error when leaving organization
- Test case for query error when loading organization details
Example mock for mutation error:
{
request: {
query: REMOVE_MEMBER_MUTATION,
variables: { orgid: 'test-org-id', userid: 'test-user-id' },
},
error: new Error('Failed to leave organization'),
}🔗 Analysis chain
Add error scenarios to GraphQL mocks
The current mocks only cover successful responses. Consider adding error scenarios to test error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if error handling exists in the component
ast-grep --pattern 'catch ($_) {
$$$
}'
Length of output: 43
Script:
#!/bin/bash
# Search for error handling patterns in the test file and component
rg -A 5 "error|catch|throw" "src/screens/UserPortal/LeaveOrganization/"
# Check for any error states or error messages in the component
ast-grep --pattern 'const [$_] = useState<$_Error'
# Look for error related props or interfaces
rg -A 2 "interface.*Error|type.*Error" "src/screens/UserPortal/LeaveOrganization/"
# Check for error handling in mutation hooks
ast-grep --pattern 'useMutation<$_, $_>($_, {
$$$
onError: $_
$$$
})'
Length of output: 2607
Script:
#!/bin/bash
# Check the implementation of the test file to see all test cases
cat "src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx"
# Check mutation implementation for error handling
ast-grep --pattern 'useMutation($_, {
$$$
})'
Length of output: 10055
There was a problem hiding this comment.
@raggettii, thanks for addressing the issue! The error scenarios you've added to the tests will help ensure the component handles GraphQL errors properly. Great work!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -43,7 +43,7 @@ const EventDashboardScreen = (): JSX.Element => { | |||
| className={`d-flex flex-row justify-content-between flex-wrap ${styles.gap}`} | |||
| > | |||
| <div style={{ flex: 1 }}> | |||
| <h1>{t('title')}</h1> | |||
| <h1>{t('Not title ')}</h1> | |||
There was a problem hiding this comment.
Fix incorrect title translation key
The title translation key appears to be incorrectly changed to 'Not title ' which seems unintentional and inconsistent with other title renderings in the file.
- <h1>{t('Not title ')}</h1>
+ <h1>{t('title')}</h1>📝 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.
| <h1>{t('Not title ')}</h1> | |
| <h1>{t('title')}</h1> |
There was a problem hiding this comment.
@raggettii, thank you for making the changes!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { | ||
| onCompleted: () => { | ||
| alert('You have successfully left the organization!'); | ||
| setShowModal(false); | ||
| navigate(`/user/organizations`); | ||
| window.location.reload(); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the page reload after navigation.
The window.location.reload() call after navigation is unnecessary and could cause a jarring user experience. The Apollo cache update should handle the UI refresh.
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, {
onCompleted: () => {
alert('You have successfully left the organization!');
setShowModal(false);
- navigate(`/user/organizations`);
- window.location.reload();
+ navigate('/user/organizations');
},📝 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.
| const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { | |
| onCompleted: () => { | |
| alert('You have successfully left the organization!'); | |
| setShowModal(false); | |
| navigate(`/user/organizations`); | |
| window.location.reload(); | |
| }, | |
| const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { | |
| onCompleted: () => { | |
| alert('You have successfully left the organization!'); | |
| setShowModal(false); | |
| navigate('/user/organizations'); | |
| }, |
There was a problem hiding this comment.
After leaving the organization it is navigated to user/organizations and then the org join button still says visit instead of join that is the reason i put a reload there should remove it or if you suggest something else that would be really helpful
There was a problem hiding this comment.
@raggettii, thank you for explaining the issue. Instead of using window.location.reload(), you can refresh the organization data by utilizing Apollo's refetchQueries option in your useMutation call. This will update the UI to reflect the correct button state without requiring a full page reload. Here's how you might adjust your mutation:
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, {
onCompleted: () => {
alert('You have successfully left the organization!');
setShowModal(false);
navigate('/user/organizations');
},
refetchQueries: [{ query: ORGANIZATIONS_LIST }],
});Alternatively, you can use the update function to directly modify the Apollo cache and remove the organization from the user's list. This approach ensures the UI is consistent with the latest data and provides a smoother user experience.
✏️ Learnings added
Learnt from: raggettii
PR: PalisadoesFoundation/talawa-admin#2629
File: src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx:30-36
Timestamp: 2024-12-09T21:12:47.806Z
Learning: When modifying organization membership, prefer using Apollo's `refetchQueries` or cache updates to refresh the UI instead of using `window.location.reload()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
i have used the Apollo's refetchQueries option but it didn't work
There was a problem hiding this comment.
@raggettii, I understand that using Apollo's refetchQueries option didn't resolve the issue. Let's try another approach to update the UI without a full page reload.
You can use the update function in your useMutation to modify the Apollo cache directly after the mutation completes. This ensures that the UI reflects the latest data. Here's how you might implement it:
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, {
variables: {
// your mutation variables here
},
onCompleted: () => {
alert('You have successfully left the organization!');
setShowModal(false);
navigate('/user/organizations');
},
update: (cache, { data: { removeMember } }) => {
// Remove the organization from the cache
cache.modify({
fields: {
organizations(existingOrganizations = [], { readField }) {
return existingOrganizations.filter(
org => readField('id', org) !== organizationId
);
},
},
});
},
});In this example:
- The
updatefunction accesses the Apollo cache and modifies it after the mutation. - We filter out the organization that the user has left from the
organizationsfield in the cache. - This should update the UI components that rely on the
organizationsdata without needing a page reload.
Ensure that the field names in cache.modify match those in your GraphQL schema. You may need to adjust organizations and any other identifiers accordingly.
Additionally, verify that the component displaying the "Visit" or "Join" button is correctly reading from the Apollo cache and reacting to changes. If it relies on local state or props, make sure those are updated when the mutation completes.
If this doesn't resolve the issue, please share more details or relevant code snippets of how the button label is determined. That way, I can assist you further in troubleshooting the problem.
There was a problem hiding this comment.
I tried again and refetchQueries works fine :)
There was a problem hiding this comment.
@raggettii, that's great news! I'm glad that refetchQueries is working fine now. This will help keep the UI updated without needing a full page reload.
(\ (\
(^‿^)
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| const handleVerifyAndLeave = (): void => { | ||
| if (email === userEmail) { | ||
| handleLeaveOrganization(); | ||
| } else { | ||
| setError('Verification failed: Email does not match.'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance email verification security.
The email comparison should be case-insensitive to prevent user friction, and consider trimming whitespace.
const handleVerifyAndLeave = (): void => {
- if (email === userEmail) {
+ if (email.trim().toLowerCase() === userEmail.toLowerCase()) {
handleLeaveOrganization();
} else {
setError('Verification failed: Email does not match.');
}
};📝 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.
| const handleVerifyAndLeave = (): void => { | |
| if (email === userEmail) { | |
| handleLeaveOrganization(); | |
| } else { | |
| setError('Verification failed: Email does not match.'); | |
| } | |
| }; | |
| const handleVerifyAndLeave = (): void => { | |
| if (email.trim().toLowerCase() === userEmail.toLowerCase()) { | |
| handleLeaveOrganization(); | |
| } else { | |
| setError('Verification failed: Email does not match.'); | |
| } | |
| }; |
There was a problem hiding this comment.
Required changes have been made
There was a problem hiding this comment.
@raggettii, thank you for making the changes!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
palisadoes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (3)
51-57: Consider adding rate limiting for verification attempts.To prevent brute force attempts, consider implementing a maximum number of verification attempts before temporarily disabling the feature.
+ const [attempts, setAttempts] = useState(0); + const MAX_ATTEMPTS = 3; const handleVerifyAndLeave = (): void => { + if (attempts >= MAX_ATTEMPTS) { + setError('Too many attempts. Please try again later.'); + return; + } if (email.trim().toLowerCase() === userEmail.toLowerCase()) { handleLeaveOrganization(); } else { + setAttempts(prev => prev + 1); setError('Verification failed: Email does not match.'); } };
32-32: Replace native alert with Bootstrap Alert component.Using native alert() is not consistent with the UI design and can be blocked by browsers.
- alert('You have successfully left the organization!'); + const Toast = useToast(); + Toast.success('You have successfully left the organization!');
87-160: Enhance modal accessibility.The modal component should have proper ARIA labels and keyboard navigation support.
<Modal show={showModal} + aria-labelledby="leave-organization-modal" + aria-describedby="leave-organization-description" onHide={() => { setShowModal(false); setVerificationStep(false); setEmail(''); setError(''); }} > - <Modal.Header closeButton> + <Modal.Header closeButton id="leave-organization-modal"> <Modal.Title>Leave Organization</Modal.Title> </Modal.Header> - <Modal.Body> + <Modal.Body id="leave-organization-description">
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (1)
Learnt from: raggettii
PR: PalisadoesFoundation/talawa-admin#2629
File: src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx:30-36
Timestamp: 2024-12-09T21:12:47.916Z
Learning: When modifying organization membership, prefer using Apollo's `refetchQueries` or cache updates to refresh the UI instead of using `window.location.reload()`.
🔇 Additional comments (3)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (3)
1-8: LGTM! Imports are well-organized and appropriate.
All necessary dependencies are properly imported, and there's a clear separation between external libraries and internal utilities.
30-41: Remove window.location.reload() as previously discussed.
As mentioned in the previous review, using window.location.reload() creates a jarring user experience. Use Apollo's cache management instead.
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, {
onCompleted: () => {
alert('You have successfully left the organization!');
setShowModal(false);
navigate(`/user/organizations`);
- window.location.reload();
},
+ refetchQueries: [{ query: ORGANIZATIONS_LIST }],
onError: (err) => {
setError(err.message);
setLoading(false);
},
});13-14: Consider adding type checking for localStorage values.
The values retrieved from localStorage should be validated to ensure they exist and are of the correct type, as they are critical for the component's functionality.
- const userEmail = getItem('Talawa-admin', 'email');
- const userId = getItem('Talawa-admin', 'userId');
+ const userEmail = getItem('Talawa-admin', 'email') || '';
+ const userId = getItem('Talawa-admin', 'userId');
+ if (!userId || !userEmail) {
+ navigate('/login');
+ return;
+ }| if (orgLoading) return <Spinner animation="border" />; | ||
| if (orgError) return <p>Error: {orgError.message}</p>; | ||
|
|
||
| const organization = orgData?.organizations[0]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for organization data.
The organization data access could be unsafe if the query returns no results.
if (orgLoading) return <Spinner animation="border" />;
if (orgError) return <p>Error: {orgError.message}</p>;
+ if (!orgData?.organizations?.length) {
+ return <p>Organization not found</p>;
+ }
const organization = orgData?.organizations[0];📝 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.
| if (orgLoading) return <Spinner animation="border" />; | |
| if (orgError) return <p>Error: {orgError.message}</p>; | |
| const organization = orgData?.organizations[0]; | |
| if (orgLoading) return <Spinner animation="border" />; | |
| if (orgError) return <p>Error: {orgError.message}</p>; | |
| if (!orgData?.organizations?.length) { | |
| return <p>Organization not found</p>; | |
| } | |
| const organization = orgData?.organizations[0]; |
There was a problem hiding this comment.
Required changes have been made
There was a problem hiding this comment.
@raggettii, thank you for making the required changes!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
Please fix the failing tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx (2)
22-68: Consider adding more error scenarios to improve coverage.The current mocks provide good coverage of basic success and error cases. Consider adding these additional scenarios:
- Network errors
- Validation errors (e.g., user not found)
- Permission errors
Example mock for a permission error:
{ request: { query: REMOVE_MEMBER_MUTATION, variables: { orgid: 'test-org-id', userid: 'test-user-id' }, }, result: { errors: [{ message: "Permission denied: User cannot leave this organization", extensions: { code: 'FORBIDDEN' } }] } }
70-203: Add cleanup and improve test maintainability.While the test coverage is comprehensive, consider these improvements:
- Extract test data and expected messages into constants
- Add snapshot tests for complex UI states
- Consider using
screen.findBy*instead ofwaitFor+getBy*for simpler async assertionsExample refactor:
const TEST_DATA = { orgName: 'Test Organization', orgDescription: 'This is a test organization.', errorMessages: { loadFailed: 'Failed to load organization details', verificationFailed: 'Verification failed', } }; // Replace magic strings with constants expect(screen.getByText(TEST_DATA.orgName)).toBeInTheDocument();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx(1 hunks)
🔇 Additional comments (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx (1)
1-20: LGTM! Clean and comprehensive test setup.
The imports and mock setup are well-structured, with proper mocking of the useLocalStorage hook and its methods.
| }); | ||
| }); | ||
|
|
||
| test('opens modal on leave button click', async () => { | ||
| render( | ||
| <MockedProvider mocks={mocks.slice(0, 2)} addTypename={false}> | ||
| <BrowserRouter> | ||
| <LeaveOrganization /> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| await waitFor(() => screen.getByText('Leave Organization')); | ||
| fireEvent.click(screen.getByText('Leave Organization')); | ||
|
|
||
| expect( | ||
| screen.getByText(/Are you sure you want to leave this organization?/i), | ||
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('verifies email and submits', async () => { | ||
| render( | ||
| <MockedProvider mocks={mocks.slice(0, 2)} addTypename={false}> | ||
| <BrowserRouter> | ||
| <LeaveOrganization /> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| await waitFor(() => screen.getByText('Leave Organization')); | ||
| fireEvent.click(screen.getByText('Leave Organization')); | ||
|
|
||
| await waitFor(() => screen.getByText('Continue')); | ||
| fireEvent.click(screen.getByText('Continue')); | ||
|
|
||
| fireEvent.change(screen.getByPlaceholderText(/Enter your email/i), { | ||
| target: { value: '[email protected]' }, | ||
| }); | ||
|
|
||
| fireEvent.click(screen.getByText('Confirm')); | ||
|
|
||
| await waitFor(() => | ||
| expect(screen.queryByText('Leave Organization')).not.toBeInTheDocument(), | ||
| ); | ||
| }); | ||
|
|
||
| test('shows error for incorrect email', async () => { | ||
| render( | ||
| <MockedProvider mocks={mocks.slice(0, 2)} addTypename={false}> | ||
| <BrowserRouter> | ||
| <LeaveOrganization /> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| await waitFor(() => screen.getByText('Leave Organization')); | ||
| fireEvent.click(screen.getByText('Leave Organization')); | ||
|
|
||
| await waitFor(() => screen.getByText('Continue')); | ||
| fireEvent.click(screen.getByText('Continue')); | ||
|
|
||
| fireEvent.change(screen.getByPlaceholderText(/Enter your email/i), { | ||
| target: { value: '[email protected]' }, | ||
| }); | ||
|
|
||
| fireEvent.click(screen.getByText('Confirm')); | ||
|
|
||
| await waitFor(() => | ||
| expect(screen.getByText(/Verification failed/i)).toBeInTheDocument(), | ||
| ); | ||
| }); | ||
|
|
||
| test('shows error message when mutation fails', async () => { | ||
| render( | ||
| <MockedProvider mocks={mocks.slice(3, 4)} addTypename={false}> | ||
| <BrowserRouter> | ||
| <LeaveOrganization /> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| await waitFor(() => screen.getByText('Leave Organization')); | ||
| fireEvent.click(screen.getByText('Leave Organization')); | ||
|
|
||
| await waitFor(() => screen.getByText('Continue')); | ||
| fireEvent.click(screen.getByText('Continue')); | ||
|
|
||
| fireEvent.change(screen.getByPlaceholderText(/Enter your email/i), { | ||
| target: { value: '[email protected]' }, | ||
| }); | ||
|
|
||
| fireEvent.click(screen.getByText('Confirm')); | ||
|
|
||
| await waitFor(() => | ||
| expect( | ||
| screen.getByText(/Failed to leave organization/i), | ||
| ).toBeInTheDocument(), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add test cleanup to ensure isolation.
The tests may interfere with each other as there's no cleanup between tests. This could lead to flaky tests.
Add this at the start of the describe block:
beforeEach(() => {
jest.clearAllMocks();
});
afterEach(() => {
jest.resetModules();
});There was a problem hiding this comment.
I have made the changes that you suggested still you are showing error please check again
|
I have made the changes suggested by coderabbitai in the recent commit it is still giving me the same error @palisadoes
|
|
Do we need a email to enter while leaving organisation which I think a bad UX for user. cc: @palisadoes |
|
@raggettii Please keep working on fixing the tests. You can also ask for help in slack if you are getting a specific issues. @varshith257 I have not seen the email request for leaving in other apps, I don't think its needed, but I'd like to know what you think @palisadoes |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (2)
26-30: Consider combining related state variables.The modal-related states (
showModal,verificationStep,error) could be combined into a single state object for better maintainability.- const [email, setEmail] = useState(''); - const [error, setError] = useState(''); - const [loading, setLoading] = useState(false); - const [showModal, setShowModal] = useState(false); - const [verificationStep, setVerificationStep] = useState(false); + const [modalState, setModalState] = useState({ + email: '', + error: '', + loading: false, + showModal: false, + verificationStep: false, + }); + + const updateModalState = (updates: Partial<typeof modalState>) => { + setModalState(prev => ({ ...prev, ...updates })); + };
112-117: Enhance loading and error states.The current implementation could be more user-friendly with better loading states and error messages.
- if (orgLoading) return <Spinner animation="border" />; - if (orgError) return <p>Error: {orgError.message}</p>; + if (orgLoading) return ( + <div className="text-center p-4"> + <Spinner animation="border" /> + <p className="mt-2">Loading organization details...</p> + </div> + ); + if (orgError) return ( + <Alert variant="danger"> + <Alert.Heading>Unable to load organization</Alert.Heading> + <p> + We encountered an error while loading the organization details. + Please try refreshing the page. + </p> + <hr /> + <p className="mb-0">Error details: {orgError.message}</p> + </Alert> + ); if (!orgData?.organizations?.length) { - return <p>Organization not found</p>; + return ( + <Alert variant="warning"> + <Alert.Heading>Organization Not Found</Alert.Heading> + <p>The organization you're looking for doesn't exist or has been removed.</p> + <Button variant="primary" onClick={() => navigate('/user/organizations')}> + View All Organizations + </Button> + </Alert> + ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (1)
Learnt from: raggettii
PR: PalisadoesFoundation/talawa-admin#2629
File: src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx:30-36
Timestamp: 2024-12-09T21:12:47.916Z
Learning: When modifying organization membership, prefer using Apollo's `refetchQueries` or cache updates to refresh the UI instead of using `window.location.reload()`.
| const handleLeaveOrganization = (): void => { | ||
| setError(''); | ||
| setLoading(true); | ||
| removeMember({ | ||
| variables: { orgid: organizationId, userid: userId }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Add validation before mutation.
The handleLeaveOrganization function should validate required parameters before proceeding.
const handleLeaveOrganization = (): void => {
+ if (!organizationId || !userId) {
+ setError('Unable to process request: Missing required information.');
+ setLoading(false);
+ return;
+ }
setError('');
setLoading(true);
removeMember({
variables: { orgid: organizationId, userid: userId },
});
};📝 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.
| const handleLeaveOrganization = (): void => { | |
| setError(''); | |
| setLoading(true); | |
| removeMember({ | |
| variables: { orgid: organizationId, userid: userId }, | |
| }); | |
| }; | |
| const handleLeaveOrganization = (): void => { | |
| if (!organizationId || !userId) { | |
| setError('Unable to process request: Missing required information.'); | |
| setLoading(false); | |
| return; | |
| } | |
| setError(''); | |
| setLoading(true); | |
| removeMember({ | |
| variables: { orgid: organizationId, userid: userId }, | |
| }); | |
| }; |
| const userEmail = getItem('Talawa-admin', 'email'); | ||
| const userId = getItem('Talawa-admin', 'userId'); |
There was a problem hiding this comment.
Add error handling for localStorage access.
The getItem calls could fail if localStorage is unavailable or corrupted.
- const userEmail = getItem('Talawa-admin', 'email');
- const userId = getItem('Talawa-admin', 'userId');
+ const userEmail = (() => {
+ try {
+ return getItem('Talawa-admin', 'email') ?? '';
+ } catch (e) {
+ console.error('Failed to access localStorage:', e);
+ return '';
+ }
+ })();
+ const userId = (() => {
+ try {
+ return getItem('Talawa-admin', 'userId') ?? '';
+ } catch (e) {
+ console.error('Failed to access localStorage:', e);
+ return '';
+ }
+ })();Committable suggestion skipped: line range outside the PR's diff.
| const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { | ||
| refetchQueries: [ | ||
| { | ||
| query: USER_ORGANIZATION_CONNECTION, | ||
| variables: { id: organizationId }, | ||
| }, | ||
| ], | ||
| onCompleted: () => { | ||
| alert('You have successfully left the organization!'); | ||
| setShowModal(false); | ||
| navigate(`/user/organizations`); | ||
| }, | ||
| onError: (err) => { | ||
| setError(err.message); | ||
| setLoading(false); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance user feedback and error handling.
The current implementation has several areas for improvement:
- Using
alert()is not accessibility-friendly - Error messages could be more user-friendly
- Network errors should be handled separately
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, {
refetchQueries: [
{
query: USER_ORGANIZATION_CONNECTION,
variables: { id: organizationId },
},
],
onCompleted: () => {
- alert('You have successfully left the organization!');
+ // Use a toast notification or in-app message
+ setModalState(prev => ({
+ ...prev,
+ showModal: false,
+ successMessage: 'You have successfully left the organization!'
+ }));
setShowModal(false);
navigate(`/user/organizations`);
},
onError: (err) => {
- setError(err.message);
+ const isNetworkError = err.networkError !== null;
+ setError(
+ isNetworkError
+ ? 'Unable to process your request. Please check your connection.'
+ : 'Failed to leave organization. Please try again.'
+ );
setLoading(false);
},
});Committable suggestion skipped: line range outside the PR's diff.
| <Modal | ||
| show={showModal} | ||
| onHide={() => { | ||
| setShowModal(false); | ||
| setVerificationStep(false); | ||
| setEmail(''); | ||
| setError(''); | ||
| }} | ||
| > | ||
| <Modal.Header closeButton> | ||
| <Modal.Title>Leave Organization</Modal.Title> | ||
| </Modal.Header> | ||
| <Modal.Body> | ||
| {!verificationStep ? ( | ||
| <> | ||
| <p>Are you sure you want to leave this organization?</p> | ||
| <p> | ||
| This action cannot be undone, and you may need to request access | ||
| again if you reconsider. | ||
| </p> | ||
| </> | ||
| ) : ( | ||
| <Form | ||
| onSubmit={(e) => { | ||
| e.preventDefault(); | ||
| handleVerifyAndLeave(); | ||
| }} | ||
| > | ||
| <Form.Group> | ||
| <Form.Label>Enter your email to confirm:</Form.Label> | ||
| <Form.Control | ||
| type="email" | ||
| placeholder="Enter your email" | ||
| value={email} | ||
| onChange={(e) => setEmail(e.target.value)} | ||
| onKeyDown={handleKeyPress} | ||
| /> | ||
| </Form.Group> | ||
| {error && <Alert variant="danger">{error}</Alert>} | ||
| </Form> | ||
| )} | ||
| </Modal.Body> | ||
| <Modal.Footer> | ||
| {!verificationStep ? ( | ||
| <> | ||
| <Button variant="secondary" onClick={() => setShowModal(false)}> | ||
| Cancel | ||
| </Button> | ||
| <Button | ||
| variant="danger" | ||
| onClick={() => setVerificationStep(true)} | ||
| > | ||
| Continue | ||
| </Button> | ||
| </> | ||
| ) : ( | ||
| <> | ||
| <Button | ||
| variant="secondary" | ||
| onClick={() => setVerificationStep(false)} | ||
| > | ||
| Back | ||
| </Button> | ||
| <Button | ||
| variant="danger" | ||
| disabled={loading} | ||
| onClick={handleVerifyAndLeave} | ||
| > | ||
| {loading ? <Spinner animation="border" size="sm" /> : 'Confirm'} | ||
| </Button> | ||
| </> | ||
| )} | ||
| </Modal.Footer> | ||
| </Modal> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve modal accessibility and form handling.
The modal implementation could benefit from improved accessibility and form handling.
<Modal
show={showModal}
+ aria-labelledby="leave-organization-modal"
onHide={() => {
setShowModal(false);
setVerificationStep(false);
setEmail('');
setError('');
}}
>
- <Modal.Header closeButton>
- <Modal.Title>Leave Organization</Modal.Title>
+ <Modal.Header closeButton>
+ <Modal.Title id="leave-organization-modal">Leave Organization</Modal.Title>
</Modal.Header>
<Modal.Body>
{!verificationStep ? (
<>
<p>Are you sure you want to leave this organization?</p>
<p>
This action cannot be undone, and you may need to request access
again if you reconsider.
</p>
</>
) : (
<Form
onSubmit={(e) => {
e.preventDefault();
+ if (!email.trim()) {
+ setError('Please enter your email address.');
+ return;
+ }
handleVerifyAndLeave();
}}
+ noValidate
>
<Form.Group>
- <Form.Label>Enter your email to confirm:</Form.Label>
+ <Form.Label htmlFor="confirm-email">
+ Enter your email to confirm:
+ </Form.Label>
<Form.Control
+ id="confirm-email"
type="email"
placeholder="Enter your email"
value={email}
+ required
+ aria-describedby={error ? "email-error" : undefined}
onChange={(e) => setEmail(e.target.value)}
onKeyDown={handleKeyPress}
/>
</Form.Group>
- {error && <Alert variant="danger">{error}</Alert>}
+ {error && (
+ <Alert variant="danger" id="email-error" role="alert">
+ {error}
+ </Alert>
+ )}
</Form>
)}
</Modal.Body>📝 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.
| <Modal | |
| show={showModal} | |
| onHide={() => { | |
| setShowModal(false); | |
| setVerificationStep(false); | |
| setEmail(''); | |
| setError(''); | |
| }} | |
| > | |
| <Modal.Header closeButton> | |
| <Modal.Title>Leave Organization</Modal.Title> | |
| </Modal.Header> | |
| <Modal.Body> | |
| {!verificationStep ? ( | |
| <> | |
| <p>Are you sure you want to leave this organization?</p> | |
| <p> | |
| This action cannot be undone, and you may need to request access | |
| again if you reconsider. | |
| </p> | |
| </> | |
| ) : ( | |
| <Form | |
| onSubmit={(e) => { | |
| e.preventDefault(); | |
| handleVerifyAndLeave(); | |
| }} | |
| > | |
| <Form.Group> | |
| <Form.Label>Enter your email to confirm:</Form.Label> | |
| <Form.Control | |
| type="email" | |
| placeholder="Enter your email" | |
| value={email} | |
| onChange={(e) => setEmail(e.target.value)} | |
| onKeyDown={handleKeyPress} | |
| /> | |
| </Form.Group> | |
| {error && <Alert variant="danger">{error}</Alert>} | |
| </Form> | |
| )} | |
| </Modal.Body> | |
| <Modal.Footer> | |
| {!verificationStep ? ( | |
| <> | |
| <Button variant="secondary" onClick={() => setShowModal(false)}> | |
| Cancel | |
| </Button> | |
| <Button | |
| variant="danger" | |
| onClick={() => setVerificationStep(true)} | |
| > | |
| Continue | |
| </Button> | |
| </> | |
| ) : ( | |
| <> | |
| <Button | |
| variant="secondary" | |
| onClick={() => setVerificationStep(false)} | |
| > | |
| Back | |
| </Button> | |
| <Button | |
| variant="danger" | |
| disabled={loading} | |
| onClick={handleVerifyAndLeave} | |
| > | |
| {loading ? <Spinner animation="border" size="sm" /> : 'Confirm'} | |
| </Button> | |
| </> | |
| )} | |
| </Modal.Footer> | |
| </Modal> | |
| <Modal | |
| show={showModal} | |
| aria-labelledby="leave-organization-modal" | |
| onHide={() => { | |
| setShowModal(false); | |
| setVerificationStep(false); | |
| setEmail(''); | |
| setError(''); | |
| }} | |
| > | |
| <Modal.Header closeButton> | |
| <Modal.Title id="leave-organization-modal">Leave Organization</Modal.Title> | |
| </Modal.Header> | |
| <Modal.Body> | |
| {!verificationStep ? ( | |
| <> | |
| <p>Are you sure you want to leave this organization?</p> | |
| <p> | |
| This action cannot be undone, and you may need to request access | |
| again if you reconsider. | |
| </p> | |
| </> | |
| ) : ( | |
| <Form | |
| onSubmit={(e) => { | |
| e.preventDefault(); | |
| if (!email.trim()) { | |
| setError('Please enter your email address.'); | |
| return; | |
| } | |
| handleVerifyAndLeave(); | |
| }} | |
| noValidate | |
| > | |
| <Form.Group> | |
| <Form.Label htmlFor="confirm-email"> | |
| Enter your email to confirm: | |
| </Form.Label> | |
| <Form.Control | |
| id="confirm-email" | |
| type="email" | |
| placeholder="Enter your email" | |
| value={email} | |
| required | |
| aria-describedby={error ? "email-error" : undefined} | |
| onChange={(e) => setEmail(e.target.value)} | |
| onKeyDown={handleKeyPress} | |
| /> | |
| </Form.Group> | |
| {error && ( | |
| <Alert variant="danger" id="email-error" role="alert"> | |
| {error} | |
| </Alert> | |
| )} | |
| </Form> | |
| )} | |
| </Modal.Body> | |
| <Modal.Footer> | |
| {!verificationStep ? ( | |
| <> | |
| <Button variant="secondary" onClick={() => setShowModal(false)}> | |
| Cancel | |
| </Button> | |
| <Button | |
| variant="danger" | |
| onClick={() => setVerificationStep(true)} | |
| > | |
| Continue | |
| </Button> | |
| </> | |
| ) : ( | |
| <> | |
| <Button | |
| variant="secondary" | |
| onClick={() => setVerificationStep(false)} | |
| > | |
| Back | |
| </Button> | |
| <Button | |
| variant="danger" | |
| disabled={loading} | |
| onClick={handleVerifyAndLeave} | |
| > | |
| {loading ? <Spinner animation="border" size="sm" /> : 'Confirm'} | |
| </Button> | |
| </> | |
| )} | |
| </Modal.Footer> | |
| </Modal> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
src/state/reducers/userRoutersReducer.test.ts (1)
21-21: Consider extracting repeated configuration into constants.The component and target configurations for "Leave Organization" are duplicated between the initial state and UPDATE_TARGETS test cases. This could lead to maintenance issues if the configuration needs to change.
Consider extracting the configuration into constants:
+const LEAVE_ORG_CONFIG = { + name: 'Leave Organization', + comp_id: 'leaveorg', + component: 'LeaveOrganization', +}; +const LEAVE_ORG_TARGET = (orgId = 'undefined') => ({ + name: 'Leave Organization', + url: `user/leaveorg/${orgId}`, +}); it('should return the initial state', () => { expect( reducer(undefined, { type: '', payload: undefined, }), ).toEqual({ targets: [ // ... other targets - { name: 'Leave Organization', url: 'user/leaveorg/undefined' }, + LEAVE_ORG_TARGET(), ], components: [ // ... other components - { - name: 'Leave Organization', - comp_id: 'leaveorg', - component: 'LeaveOrganization', - }, + LEAVE_ORG_CONFIG, ], }); });Also applies to: 48-52, 73-73, 100-104
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (5)
32-34: Consider adding fetchPolicy to the query.The organization query might benefit from a specific fetch policy to ensure data freshness.
const { data: orgData, loading: orgLoading, error: orgError, } = useQuery(ORGANIZATIONS_LIST, { variables: { id: organizationId }, + fetchPolicy: 'network-only', // Ensure fresh data });
39-55: Consider implementing optimistic updates.The mutation could benefit from optimistic updates to provide a more responsive user experience.
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { + optimisticResponse: { + removeMember: { + __typename: 'Organization', + id: organizationId, + }, + }, refetchQueries: [ { query: USER_ORGANIZATION_CONNECTION, variables: { id: organizationId }, }, ], // ... rest of the configuration });
95-108: Enhance loading and error states with more context.The loading and error states could provide more specific information to users.
if (orgLoading) { return ( <div className="text-center mt-4" role="status"> <Spinner animation="border" /> - <p>Loading organization details...</p> + <p>Loading organization details for leaving process...</p> </div> ); } -if (orgError) - return <Alert variant="danger">Error: {orgError.message}</Alert>; +if (orgError) { + return ( + <Alert variant="danger"> + <h4>Unable to load organization details</h4> + <p>{orgError.message}</p> + <p>Please try refreshing the page or contact support if the issue persists.</p> + </Alert> + ); +}
147-166: Add form validation feedback.The email verification form could benefit from built-in validation feedback.
<Form onSubmit={(e) => { e.preventDefault(); + if (!email.trim()) { + setError('Please enter your email address.'); + return; + } handleVerifyAndLeave(); }} + noValidate > <Form.Group> <Form.Label>Enter your email to confirm:</Form.Label> <Form.Control type="email" placeholder="Enter your email" value={email} + required + isInvalid={!!error} onChange={(e) => setEmail(e.target.value)} onKeyDown={handleKeyPress} aria-label="confirm-email-input" /> + <Form.Control.Feedback type="invalid"> + {error} + </Form.Control.Feedback> </Form.Group> - {error && <Alert variant="danger">{error}</Alert>} </Form>
189-203: Add confirmation dialog for irreversible action.Since leaving an organization is irreversible, consider adding an additional confirmation dialog.
Consider implementing a confirmation dialog pattern that:
- Clearly states the consequences
- Requires explicit confirmation
- Has a delay before enabling the confirm button
- Shows organization name in the confirmation message
This would help prevent accidental submissions and align with best practices for destructive actions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx(1 hunks)src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx(1 hunks)src/state/reducers/userRoutersReducer.test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2629 +/- ##
=====================================================
- Coverage 95.73% 83.50% -12.23%
=====================================================
Files 295 313 +18
Lines 7036 8191 +1155
Branches 1516 1789 +273
=====================================================
+ Hits 6736 6840 +104
- Misses 99 1207 +1108
+ Partials 201 144 -57 ☔ View full report in Codecov by Sentry. |
What kind of change does this PR introduce?
The PR introduces a feature that allows users to leave an organization with an added two-step verification process for enhanced confirmation.
Issue Number:
Fixes #1873
Did you add tests for your changes?
Yes, unit tests have been added to ensure the functionality works as expected.
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
This PR introduces a feature that allows users to leave an organization. It ensures that the process is deliberate and secure by adding a two-step verification mechanism. The user must confirm their email address before leaving the organization. This helps prevent accidental or unauthorized removals.
Does this PR introduce a breaking change?
No, this PR does not introduce a breaking change. The feature is self-contained and does not impact existing functionalities.
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests