Feature Added: An option for users to leave the Organization (Fixes #1873) #2629
Conversation
WalkthroughThis pull request introduces a new feature allowing users to leave an organization. The changes span multiple files across localization, routing, and component implementation. The feature adds a new route 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
|
![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".
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx
Outdated
Show resolved
Hide resolved
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;
+ }
|
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.
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx
Outdated
Show resolved
Hide resolved
|
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()`.
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 82.81% 86.20% +3.38%
====================================================
Files 295 313 +18
Lines 7274 8204 +930
Branches 1592 1800 +208
====================================================
+ Hits 6024 7072 +1048
+ Misses 1012 956 -56
+ Partials 238 176 -62 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (3)
35-39: Consider combining related state variables.The modal-related states (
showModal,verificationStep,error) could be combined into a single state object for better maintainability and to prevent potential state synchronization issues.- 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 + });
55-77: Enhance UX with optimistic updates.Consider implementing optimistic updates in the mutation to provide instant feedback to users. This would make the UI feel more responsive, especially in cases of slow network connections.
const [removeMember] = useMutation(REMOVE_MEMBER_MUTATION, { + optimisticResponse: { + removeMember: { + __typename: 'Organization', + id: organizationId, + members: members?.filter(member => member.id !== userId) + } + }, refetchQueries: [ { query: USER_ORGANIZATION_CONNECTION, variables: { id: organizationId }, }, ], onCompleted: () => {
153-159: Add warning for unsaved changes.When closing the modal with unsaved changes (entered email), consider showing a confirmation dialog to prevent accidental data loss.
onHide={() => { + if (email.trim() && !loading) { + if (window.confirm('Are you sure you want to cancel? Your entered information will be lost.')) { + setShowModal(false); + setVerificationStep(false); + setEmail(''); + setError(''); + } + return; + } setShowModal(false); setVerificationStep(false); setEmail(''); setError(''); }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx(1 hunks)src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/screens/UserPortal/LeaveOrganization/LeaveOrganization.test.tsx
🧰 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 (2)
src/screens/UserPortal/LeaveOrganization/LeaveOrganization.tsx (2)
175-197:
Add form submission handler.
The Form component lacks an onSubmit handler, which means pressing Enter might trigger unexpected behavior. Add proper form submission handling to ensure a consistent user experience.
- <Form>
+ <Form onSubmit={(e) => {
+ e.preventDefault();
+ if (!email.trim()) {
+ setError('Please enter your email address.');
+ return;
+ }
+ handleVerifyAndLeave();
+ }}>Likely invalid or redundant comment.
13-30:
Consider moving sensitive user data to a secure context.
The userEmail and userId are still being exported, which could expose sensitive user information. Consider:
- Moving these to a secure context provider
- Using React's Context API for sharing this data
- Keeping these as internal constants
-const userEmail = (() => {
+const getUserEmail = (() => {
try {
return getItem('Talawa-admin', 'email') ?? '';
} catch (e) {
console.error('Failed to access localStorage:', e);
return '';
}
})();
-const userId = (() => {
+const getUserId = (() => {
try {
return getItem('Talawa-admin', 'userId') ?? '';
} catch (e) {
console.error('Failed to access localStorage:', e);
return '';
}
})();
-export { userEmail, userId };Likely invalid or redundant comment.
|
@palisadoes review |
|
|
@palisadoes all checks are passed |
|
Thanks. Is this supported in the API? |
|
@palisadoes i have created a pr for the same but it got closed due to inactivity PalisadoesFoundation/talawa-api#2753 |
|
|
@palisadoes, can you please merge this PR? Also, you have merged the API PR, i.e., |
palisadoes
merged commit 007bcb0
into
PalisadoesFoundation:develop-postgres
|
@palisadoes As the changes are made in this PR to access leave organzation page ie.(in App.tsx changes are not reflecting and other thing is fine) but in the develop-postgres branch it is not there even though the PR is merged, it can be conflicting issue can you please look into this. |
|
Please look at the for history to see wether any PRs afterward updated the file. Should we add the file to the PR sensitive files check? |
|
@palisadoes The changes are made in this PR causing the issue |
|
Please open a PR against the issue below to fix it: |
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
Tests