add visit button in joined organization filter #3218
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
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
|
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: 3
🔭 Outside diff range comments (1)
src/screens/UserPortal/Organizations/Organizations.tsx (1)
Line range hint
226-228: Fix inconsistent element ID inhandleSearchByBtnClickThe
idused indocument.getElementByIdis'searchUserOrgs', but theForm.Controlelement has anidof'searchOrganizations'. This mismatch will cause the search functionality to fail.Apply this diff to correct the
id:const handleSearchByBtnClick = (): void => { const value = - (document.getElementById('searchUserOrgs') as HTMLInputElement)?.value || ''; + (document.getElementById('searchOrganizations') as HTMLInputElement)?.value || ''; handleSearch(value); };
🧹 Nitpick comments (2)
src/components/OrganizationCard/OrganizationCard.tsx (1)
176-176: Simplify null checks using optional chainingYou can simplify the null checks by using optional chaining for better readability.
Apply this diff to use optional chaining:
-{address && address.city && ( +{address?.city && (🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/screens/UserPortal/Organizations/Organizations.tsx (1)
254-258: Review the necessity of theisJoinedpropertyThe
isJoinedproperty is added to organizations but is not defined inInterfaceOrganizationCardPropsand does not appear to be used in theOrganizationCardcomponent. If it is not required, consider removing it to simplify the code.Also applies to: 266-271, 281-286
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/OrganizationCard/OrganizationCard.spec.tsx(1 hunks)src/components/OrganizationCard/OrganizationCard.tsx(1 hunks)src/screens/UserPortal/Organizations/Organizations.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/OrganizationCard/OrganizationCard.tsx
[error] 176-176: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
| if (error.message === 'User is already a member') { | ||
| toast.error(t('AlreadyJoined') as string); | ||
| } else { | ||
| toast.error(t('errorOccured') as string); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling by using error codes instead of error messages
Comparing error messages as strings can be brittle and may not handle localization properly. Consider using error codes or custom error types to handle specific errors.
| const membershipRequest = membershipRequests.find( | ||
| (request) => request.user._id === userId, | ||
| ); | ||
|
|
||
| await cancelMembershipRequest({ | ||
| variables: { | ||
| membershipRequestId: membershipRequest?._id, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Handle case when membership request is not found
The membershipRequest may be undefined if no request is found. Add a check to handle this case to prevent potential runtime errors.
Apply this diff to handle the undefined case:
async function withdrawMembershipRequest(): Promise<void> {
const membershipRequest = membershipRequests.find(
(request) => request.user._id === userId,
);
+ if (!membershipRequest) {
+ toast.error(t('MembershipRequestNotFound') as string);
+ return;
+ }
await cancelMembershipRequest({
variables: {
membershipRequestId: membershipRequest?._id,
},
});
}📝 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 membershipRequest = membershipRequests.find( | |
| (request) => request.user._id === userId, | |
| ); | |
| await cancelMembershipRequest({ | |
| variables: { | |
| membershipRequestId: membershipRequest?._id, | |
| }, | |
| }); | |
| const membershipRequest = membershipRequests.find( | |
| (request) => request.user._id === userId, | |
| ); | |
| if (!membershipRequest) { | |
| toast.error(t('MembershipRequestNotFound') as string); | |
| return; | |
| } | |
| await cancelMembershipRequest({ | |
| variables: { | |
| membershipRequestId: membershipRequest._id, | |
| }, | |
| }); |
| describe('Organization Card', () => { | ||
| test('renders organization card with image', () => { | ||
| render( | ||
| <MockedProvider> | ||
| <BrowserRouter> | ||
| <I18nextProvider i18n={i18nForTest}> | ||
| <OrganizationCard {...props} /> | ||
| </I18nextProvider> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); | ||
|
|
||
| expect(screen.getByText(props.name)).toBeInTheDocument(); | ||
| expect(screen.getByText(/Owner:/i)).toBeInTheDocument(); | ||
| expect(screen.getByText(props.firstName)).toBeInTheDocument(); | ||
| expect(screen.getByText(props.lastName)).toBeInTheDocument(); | ||
| expect(screen.getByRole('img')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('Should render text elements when props value is not passed', () => { | ||
| const props = { | ||
| id: '123', | ||
| test('renders organization card without image', () => { | ||
| const propsWithoutImage = { | ||
| ...props, | ||
| image: '', | ||
| firstName: 'John', | ||
| lastName: 'Doe', | ||
| name: 'Sample', | ||
| }; | ||
|
|
||
| render(<OrganizationCard {...props} />); | ||
| render( | ||
| <MockedProvider> | ||
| <BrowserRouter> | ||
| <I18nextProvider i18n={i18nForTest}> | ||
| <OrganizationCard {...propsWithoutImage} /> | ||
| </I18nextProvider> | ||
| </BrowserRouter> | ||
| </MockedProvider>, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unit tests for membership status conditional rendering
The current test suite does not cover the conditional rendering based on membershipRequestStatus. To ensure the component functions correctly in all scenarios, add unit tests for cases where membershipRequestStatus is 'accepted', 'pending', and an empty string.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3218 +/- ##
=====================================================
+ Coverage 12.22% 89.91% +77.69%
=====================================================
Files 308 329 +21
Lines 7839 8555 +716
Branches 1709 1920 +211
=====================================================
+ Hits 958 7692 +6734
+ Misses 6815 628 -6187
- Partials 66 235 +169 ☔ View full report in Codecov by Sentry. |
What kind of change does this PR introduce?
Feature: Added a visit button component to the Joined Organizations section
Issue Number:
Fixes #3162
Did you add tests for your changes?
Yes
Snapshots/Videos:
issue.3162.mp4
Summary
This PR adds a visit button component to the Joined Organizations section that:
Does this PR introduce a breaking change?
No
Other information
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests