fixed:Postgres Migration Support for User Portal #3730
Conversation
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
|
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
✨ Finishing Touches
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
|
![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
🔭 Outside diff range comments (2)
src/screens/UserPortal/Organizations/Organizations.tsx (2)
193-199:⚠️ Potential issueUpdate search functionality to match new pagination model.
The search functionality still uses the old filter-based approach (
refetch({ filter: value })), but the query has been updated to use cursor-based pagination withidandfirstparameters. This mismatch could cause issues.Update the search handler to match the new query structure:
const handleSearch = (value: string): void => { setFilterName(value); refetch({ - filter: value, + id: userId, + first: rowsPerPage, + searchText: value, }); };
228-251: 🛠️ Refactor suggestionRefactor duplicate membership status logic.
The membership status checking logic is duplicated between the two
useEffecthooks. This violates the DRY principle and makes maintenance harder.Extract the common logic into a reusable function:
const getMembershipStatus = (organization: InterfaceOrganization, userId: string): string => { if (Array.isArray(organization.members) && organization.members.some(member => member._id === userId)) { return 'accepted'; } if (organization.membershipRequests?.some( request => request.user._id === userId)) { return 'pending'; } return ''; };Then use this function in both effects:
// In first useEffect return { ...organization, membershipRequestStatus: getMembershipStatus(organization, userId) }; // In second useEffect return { ...organization, membershipRequestStatus: getMembershipStatus(organization, userId), isJoined: false, };Also applies to: 256-315
🧹 Nitpick comments (1)
src/screens/UserPortal/Organizations/Organizations.tsx (1)
287-314: Simplify nested conditionals in mode handling.The mode handling logic uses deeply nested conditionals which make the code harder to read and maintain.
Consider using early returns or a switch statement:
const getOrganizationsByMode = (mode: number) => { switch (mode) { case 1: return joinedOrganizationsData?.users?.[0]?.user?.joinedOrganizations?.map( (org: InterfaceOrganization) => ({ ...org, membershipRequestStatus: 'accepted', isJoined: true, }) ) || []; case 2: return createdOrganizationsData?.users?.[0]?.appUserProfile?.createdOrganizations?.map( (org: InterfaceOrganization) => ({ ...org, membershipRequestStatus: 'accepted', isJoined: true, }) ) || []; default: return data?.user?.organizationsWhereMember?.edges?.map( (edge: { node: InterfaceOrganization }) => ({ ...edge.node, membershipRequestStatus: getMembershipStatus(edge.node, userId), isJoined: false, }) ) || []; } }; // In useEffect setOrganizations(getOrganizationsByMode(mode));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Organizations/Organizations.tsx(7 hunks)
🔇 Additional comments (1)
src/screens/UserPortal/Organizations/Organizations.tsx (1)
6-8: LGTM! Import changes align with Postgres migration.The addition of
USER_JOINED_ORGANIZATIONS_PGquery aligns with the PR objective of implementing Postgres migration support for the user portal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (1)
543-577: Add loading state verification.The search test is thorough but could be improved by verifying the loading state during search operations.
Add loading state verification after the search action:
await userEvent.click(searchBtn); +// Verify loading state +expect(screen.getByText('Loading...')).toBeInTheDocument(); + // Wait for search results to update await wait();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/UserPortal/Organizations/Organizations.spec.tsx(5 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/screens/UserPortal/Organizations/Organizations.tsx (3)
7-7: LGTM!The import statement for the new GraphQL query is correctly added.
230-231: LGTM!The data mapping is correctly updated to handle the new query structure.
265-267: LGTM! Good improvements.The changes improve the code in two ways:
- Using
someinstead offindis more efficient as it stops iteration once a match is found.- Adding optional chaining for
membershipRequestsadds null safety.Also applies to: 272-273
src/screens/UserPortal/Organizations/Organizations.spec.tsx (2)
321-479: LGTM! Comprehensive test coverage.The mock data is well-structured and covers essential scenarios:
- Initial load with multiple organizations
- Search functionality with filters
- Empty search case
650-659: LGTM! Good test improvements.The test is well-structured with:
- Proper async handling using
waitFor- Specific assertions for the number of join buttons
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3730 +/- ##
====================================================
- Coverage 86.09% 85.96% -0.13%
====================================================
Files 371 371
Lines 9138 9139 +1
Branches 1925 1925
====================================================
- Hits 7867 7856 -11
- Misses 906 918 +12
Partials 365 365 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai full review |
|
No description provided. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/screens/UserPortal/Organizations/Organizations.tsx (2)
128-128: Remove unused state or use it.You're only using
setFilterNamewithout reading its current state. If that's intentional, consider renaming the variable to_filterNameor removing it altogether for clarity.- const [, setFilterName] = React.useState(''); + const [filterName, setFilterName] = React.useState('');
143-144: Initialize filter in the query variables.Since the handleSearch function refetches with a
filterargument, consider adding a defaultfilter: ''to the initial query variables for consistency.- variables: { id: userId, first: rowsPerPage }, + variables: { id: userId, first: rowsPerPage, filter: '' },src/screens/UserPortal/Organizations/Organizations.spec.tsx (1)
543-577: Consider more granular checks in your search test.The search test verifies the presence of organizations after typing “2” and clearing the input, but it doesn’t confirm which organizations disappear or reappear. You may want to add assertions ensuring “anyOrganization1” is absent after searching.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/screens/UserPortal/Organizations/Organizations.spec.tsx(5 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(7 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/screens/UserPortal/Organizations/Organizations.tsx
[warning] 259-259: src/screens/UserPortal/Organizations/Organizations.tsx#L259
Added line #L259 was not covered by tests
[warning] 261-261: src/screens/UserPortal/Organizations/Organizations.tsx#L261
Added line #L261 was not covered by tests
🔇 Additional comments (4)
src/screens/UserPortal/Organizations/Organizations.tsx (2)
7-7: Looks good!Importing the new
USER_JOINED_ORGANIZATIONS_PGquery seems correct.
229-251: Possible duplication in data handling logic.Lines 229-251 form one approach to building the
organizationsarray, while lines 259-285 contain a separate approach formode === 0. Verify if both are needed or if any references toUserJoinedOrganizationsvs.user.organizationsWhereMemberare unintentional.src/screens/UserPortal/Organizations/Organizations.spec.tsx (2)
321-419: Good addition of mocks for the new query.The newly introduced mocks effectively cover the
USER_JOINED_ORGANIZATIONS_PGscenarios, including the default and filtered responses.
637-660: Test approach looks solid.Confirming that two "Join Now" buttons appear for two organizations is straightforward and clear. This effectively validates your UI for multiple organizations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (1)
588-635: Enhance search test robustness.While the search test covers basic functionality, it could be more robust:
- Missing negative test cases (e.g., no results found)
- No validation of loading states during search
- No error handling test cases
Consider adding these test cases:
test('Search works properly', async () => { + // Test no results found + await userEvent.type(searchInput, 'nonexistent'); + await userEvent.click(searchBtn); + await wait(); + expect(screen.getByText('No results found')).toBeInTheDocument(); + + // Test loading state + const searchPromise = userEvent.type(searchInput, '2'); + expect(screen.getByTestId('loading-indicator')).toBeInTheDocument(); + await searchPromise; + + // Test error handling + // Add error mock and verify error message
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (3)
9-9: LGTM! Comprehensive mock data for the new query.The mock data for
USER_JOINED_ORGANIZATIONS_PGis well-structured and covers multiple scenarios:
- Default query with pagination
- Search functionality
- Empty search case
Also applies to: 321-479
695-718: LGTM! Improved join button test.The test now properly verifies the number of join buttons based on the organizations loaded.
825-904: LGTM! Comprehensive edge case coverage.Excellent addition of tests for:
- Handling organizations with edges data structure
- Empty edges case
- Membership status verification
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx(7 hunks)
🧰 Additional context used
🪛 ESLint
src/screens/UserPortal/Organizations/Organizations.spec.tsx
[error] 22-22: 'mocks' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/screens/UserPortal/Organizations/Organizations.spec.tsx
[failure] 22-22:
'mocks' is defined but never used
🪛 GitHub Actions: PR Workflow
src/screens/UserPortal/Organizations/Organizations.spec.tsx
[error] 22-22: 'mocks' is defined but never used @typescript-eslint/no-unused-vars
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (4)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (4)
322-480: LGTM! Well-structured mock data for different scenarios.The mock data for
USER_JOINED_ORGANIZATIONS_PGis comprehensive and covers multiple scenarios:
- Default query with pagination
- Search functionality with filter
- Empty search case
588-635: LGTM! Thorough testing of search functionality.The search test is well-implemented with clear steps:
- Initial state verification
- Search execution
- Clear search
- Re-search with keyboard event
695-718: LGTM! Improved test assertion for Join Now buttons.The test now properly verifies the exact number of join buttons based on the number of organizations.
846-940: LGTM! Comprehensive test suite for edge/node data structure.The new test suite thoroughly validates:
- Loading state
- Mode change interaction
- Data structure processing
- UI updates
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (1)
915-920: Consider reducing the waitFor timeout.A timeout of 2000ms is quite long for unit tests. Consider reducing it to improve test execution time.
- { timeout: 2000 }, + { timeout: 1000 },Also applies to: 927-932
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/screens/UserPortal/Organizations/Organizations.spec.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (4)
src/screens/UserPortal/Organizations/Organizations.spec.tsx (4)
321-479: LGTM! Well-structured mock data.The mock data is comprehensive and covers all necessary test scenarios including pagination, filtering, and empty search cases.
595-629: LGTM! Comprehensive search functionality test.The test covers all essential scenarios:
- Initial state verification
- Search input and button interaction
- Keyboard events
- Search clearing
702-711: LGTM! Improved join button verification.The test now correctly waits for the organizations to load and verifies the exact number of join buttons, making it more reliable.
837-837: Remove duplicate link declaration.The
linkconstant is already declared at line 544.
palisadoes
merged commit f6067ff
into
PalisadoesFoundation:develop-postgres
|
@gkbishnoi07 can you please confirm why you introduced this one ?? |
|
which one i introduce?? the organizationCard.tsx is in both src/Components/Userportal and src/Components |
The one you introduced |
where i inroduce? in which PR |
https://github.com/PalisadoesFoundation/talawa-admin/pull/3278/files this one |
|
@gkbishnoi07 i wanted to know if ita has some speacial functionality other than the userPortal one ? |
I updated this component to match the develop branch, but it's not currently in use anywhere. Right now, src/components/userportal is working since it's imported in organization.tsx. If I find a use for this component in the future, I'll update you. Hope that makes sense! so right now focus on src/components/userportal |
What kind of change does this PR introduce?
feature
Issue Number:
Fixes #3565
Snapshots/Videos:
before
after
Summary
used userJoinedOrganizations to fetch organizations in the user portal
Does this PR introduce a breaking change?
no
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Bug Fixes
Tests