feat: Listing the Assigned Tags on the MemberDetail screen #2425
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing user tag management functionalities. Key updates include modifying the GitHub Actions workflow to exclude a specific file from line counting, adding new translation keys related to user tags in various language files, and updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. 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
|
meetulr
added
the
ignore-sensitive-files-pr
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2425 +/- ##
========================================
Coverage 98.25% 98.26%
========================================
Files 296 297 +1
Lines 8598 8639 +41
Branches 2500 2486 -14
========================================
+ Hits 8448 8489 +41
Misses 139 139
Partials 11 11 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/screens/ManageTag/UnassignUserTagModal.tsx (1)
Line range hint
16-69: Consider these improvements for better maintainability and user experienceWhile the implementation is solid, consider these enhancements:
- Extract the submit button's async handler to a separate function for better readability
- Add error handling feedback in the UI
- Memoize the component to prevent unnecessary re-renders
Here's a suggested implementation:
-const UnassignUserTagModal: React.FC<InterfaceUnassignUserTagModalProps> = ({ +const UnassignUserTagModal: React.FC<InterfaceUnassignUserTagModalProps> = React.memo(({ unassignUserTagModalIsOpen, toggleUnassignUserTagModal, handleUnassignUserTag, t, tCommon, -}) => { +}) => { + const [error, setError] = React.useState<string | null>(null); + + const handleSubmit = React.useCallback(async (e: React.MouseEvent<HTMLButtonElement>) => { + const btn = e.currentTarget; + btn.disabled = true; + setError(null); + + try { + await handleUnassignUserTag(); + } catch (err) { + setError(err instanceof Error ? err.message : 'An error occurred'); + } finally { + btn.disabled = false; + } + }, [handleUnassignUserTag]); return ( <> <Modal // ... existing Modal props ... > {/* ... existing Modal.Header ... */} <Modal.Body> {t('unassignUserTagMessage')} + {error && <div className="text-danger mt-2">{error}</div>} </Modal.Body> <Modal.Footer> {/* ... existing close button ... */} <Button type="button" className="btn btn-success" - onClick={async (e) => { - const btn = e.currentTarget; - btn.disabled = true; - try { - await handleUnassignUserTag(); - } finally { - btn.disabled = false; - } - }} + onClick={handleSubmit} data-testid="unassignTagModalSubmitBtn" aria-label={tCommon('yes')} > {tCommon('yes')} </Button> </Modal.Footer> </Modal> </> ); -}; +}); + +UnassignUserTagModal.displayName = 'UnassignUserTagModal';src/screens/MemberDetail/MemberDetailMocks.ts (1)
5-335: Consider extracting common user data into a shared fixture.The user profile data is duplicated across mock responses. Consider refactoring to:
- Extract common user data into a shared fixture
- Merge with specific test case data using spread operator
Example refactor:
const baseUserProfile = { _id: '1', email: '[email protected]', firstName: 'Aditya', lastName: 'Agarwal', // ... other common fields }; export const MOCKS1 = [ { request: {/*...*/}, result: { data: { user: { ...baseUserProfile, tagsAssignedWith: { // specific test case data } } } } } ];src/screens/MemberDetail/MemberDetail.test.tsx (4)
33-45: Simplify deep copying of translation objectsThe current method of deep copying translations using
JSON.parse(JSON.stringify(...))is inefficient and unnecessary. You can use the spread operator directly unless a deep clone is specifically required.Simplify the creation of the
translationsobject:-const translations = { - ...JSON.parse( - JSON.stringify( - i18nForTest.getDataByLanguage('en')?.translation.memberDetail ?? {}, - ), - ), - ...JSON.parse( - JSON.stringify(i18nForTest.getDataByLanguage('en')?.common ?? {}), - ), - ...JSON.parse( - JSON.stringify(i18nForTest.getDataByLanguage('en')?.errors ?? {}), - ), -}; +const translations = { + ...(i18nForTest.getDataByLanguage('en')?.translation.memberDetail ?? {}), + ...(i18nForTest.getDataByLanguage('en')?.common ?? {}), + ...(i18nForTest.getDataByLanguage('en')?.errors ?? {}), +};
371-374: Simplify asynchronous element querying in testsWhen using
findByTestId, you can directlyawaitthe result instead of wrapping it withwaitForandexpect(...).resolves. This simplifies the code and improves readability.Apply this change to the test:
-await waitFor(() => { - return expect( - screen.findByTestId('unassignTagModalCloseBtn'), - ).resolves.toBeInTheDocument(); -}); +const closeButton = await screen.findByTestId('unassignTagModalCloseBtn'); +expect(closeButton).toBeInTheDocument();
100-100: UsewaitForElementToBeRemovedto wait for the loading indicatorImmediately checking for the absence of the loading indicator may lead to flaky tests. Instead, use
waitForElementToBeRemovedto ensure that the loading state has completed before proceeding.Replace the immediate check with:
-expect(screen.queryByText('Loading data...')).not.toBeInTheDocument(); +await waitForElementToBeRemoved(() => screen.queryByText('Loading data...'));Also applies to: 151-151, 216-216, 223-223
159-189: Refactor repetitive input interactions into a helper functionThe test contains repetitive code for clearing and typing into input fields. Consider refactoring this into a helper function to improve readability and maintainability.
Refactor the input interactions:
const fillInputField = (placeholder, value) => { const input = screen.getByPlaceholderText(new RegExp(placeholder, 'i')); userEvent.clear(input); userEvent.type(input, value); }; fillInputField('First Name', formData.firstName); fillInputField('Last Name', formData.lastName); fillInputField('Address', formData.address); fillInputField('Country Code', formData.countryCode); fillInputField('State', formData.state); fillInputField('City', formData.city); fillInputField('Email', formData.email); fillInputField('Phone', formData.phoneNumber);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
.github/workflows/pull-request.yml(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/GraphQl/Queries/Queries.ts(2 hunks)src/screens/ManageTag/UnassignUserTagModal.tsx(1 hunks)src/screens/MemberDetail/MemberDetail.module.css(2 hunks)src/screens/MemberDetail/MemberDetail.test.tsx(9 hunks)src/screens/MemberDetail/MemberDetail.tsx(7 hunks)src/screens/MemberDetail/MemberDetailMocks.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/screens/MemberDetail/MemberDetailMocks.ts (2)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2355
File: src/screens/ManageTag/ManageTagMocks.ts:187-269
Timestamp: 2024-10-25T19:31:37.405Z
Learning: In `src/screens/ManageTag/ManageTagMocks.ts`, when mocking data for `USER_TAGS_ASSIGNED_MEMBERS` and `USER_TAGS_MEMBERS_TO_ASSIGN_TO`, it's acceptable for user IDs to overlap because the queries serve different purposes and the overlapping IDs can represent realistic scenarios in tests.
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/components/AddPeopleToTag/AddPeopleToTagsMocks.ts:16-16
Timestamp: 2024-10-30T12:37:36.856Z
Learning: When renaming GraphQL fields in this codebase, aliases are used to map old field names to new ones. It's acceptable for `getUserTag` to appear as long as it's properly aliased.
🔇 Additional comments (22)
src/screens/ManageTag/UnassignUserTagModal.tsx (1)
13-13: LGTM: Type definition properly expanded for translation function
The type definition change correctly extends the translation namespace to include 'memberDetail', which aligns with the component's usage in the member detail screen.
src/screens/MemberDetail/MemberDetail.module.css (2)
120-120: Verify content overflow behavior with fixed height
The change from max-height to a fixed height of 35vh might cause content overflow issues if there are many tags. Please verify that:
- The scroll behavior works correctly with different amounts of content
- The height is sufficient for typical use cases
- The container remains usable on different screen sizes
Consider using max-height instead to allow the container to shrink when there's less content:
- height: 35vh;
+ max-height: 35vh;676-686: LGTM! Well-structured tag link styles
The .tagLink class implementation is clean and follows best practices:
- Uses CSS variables for consistent colors
- Provides clear visual feedback on hover
- Ensures accessibility with proper cursor and text decoration
src/GraphQl/Queries/Queries.ts (2)
513-519: LGTM: Well-structured query parameters for pagination
The pagination parameters follow the Relay specification for cursor-based pagination, which is the recommended approach for GraphQL APIs. The use of PositiveInt for size limits provides good input validation.
548-570: LGTM: Comprehensive implementation of paginated tags query
The tagsAssignedWith field implementation is well-structured with:
- Proper cursor-based pagination parameters
- Complete tag data including parent tags for hierarchy support
- Standard Relay pagination metadata (pageInfo)
- Total count for UI pagination controls
Let's verify the query usage:
✅ Verification successful
Pagination and tag hierarchy properly implemented and utilized
The verification confirms that the tagsAssignedWith query implementation is properly utilized in the codebase:
-
In MemberDetail.tsx, cursor-based pagination is correctly implemented:
- Uses
InfiniteScrollcomponent withhasMorebased onpageInfo.hasNextPage - Implements
loadMoreAssignedTagsfunction that properly merges new edges - Handles parent tag relationships with conditional rendering
- Uses
-
In EventAttendance.tsx, the query results are properly rendered:
- Displays tag names from the edges
- Handles empty state with "None" fallback
The implementation matches the GraphQL schema and follows Relay pagination patterns correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the component using this query properly handles pagination
# and parent tag relationships
# Check for proper pagination handling in components
rg -l "tagsAssignedWith.*pageInfo" --type tsx
# Check for parent tag relationship handling
rg -l "parentTag.*_id" --type tsx
Length of output: 139
Script:
#!/bin/bash
# Let's try different approaches to find the query usage
# Search for tagsAssignedWith query usage across all files
rg -l "tagsAssignedWith"
# Search for files containing pageInfo structure
rg -l "pageInfo.*startCursor.*endCursor"
# Look for parent tag handling
rg -l "parentTag"
# Find GraphQL related files
fd -e graphql -e ts -e tsx -e js -e jsx
Length of output: 27940
Script:
#!/bin/bash
# Let's examine the implementation and usage of tagsAssignedWith query
# Check the implementation in EventAttendance component
rg -A 10 -B 10 "tagsAssignedWith" "src/components/EventManagement/EventAttendance/EventAttendance.tsx"
# Check the implementation in MemberDetail
rg -A 10 -B 10 "tagsAssignedWith" "src/screens/MemberDetail/MemberDetail.tsx"
# Look for pagination handling
rg "pageInfo.*hasNextPage" "src/components/EventManagement/EventAttendance/EventAttendance.tsx" "src/screens/MemberDetail/MemberDetail.tsx"
# Check for parent tag relationship usage
rg "parentTag" "src/components/EventManagement/EventAttendance/EventAttendance.tsx" "src/screens/MemberDetail/MemberDetail.tsx"
Length of output: 4055
src/screens/MemberDetail/MemberDetailMocks.ts (5)
1-4: LGTM! Imports are well-structured.
The necessary GraphQL operations and constants are properly imported.
5-335: LGTM! Comprehensive mock data for paginated tag listing and unassignment.
The mock data effectively covers:
- Initial tag loading
- Pagination with proper cursor handling
- Tag unassignment mutation
- Mix of regular tags and subtags
337-436: LGTM! Good coverage of edge cases.
The mock effectively tests:
- Empty tags scenario with proper pageInfo structure
- UI rendering with user image
- Different event attendance states
437-536: LGTM! Good coverage of role-based scenarios.
The mock effectively tests the super admin role with proper data variations.
1-536: Verify GraphQL schema compatibility.
Let's ensure the mock data structure matches the actual GraphQL schema.
✅ Verification successful
Mock data structure aligns with GraphQL schema
The mock data in MemberDetailMocks.ts correctly implements the GraphQL schema structure:
UserDatatype withappUserProfileanduserfieldsAppUserProfilewith all required fields includingadminFor,isSuperAdmin, etc.Usertype with correct fields including the paginatedtagsAssignedWithconnection- Connection types (
UserTagsConnection) with proper edge and pageInfo structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mock data structure matches the GraphQL schema
# Expected: Find the type definitions for User, UserData, and AppUserProfile
# Search for type definitions in schema files
echo "Searching for GraphQL type definitions..."
rg -t graphql "type (User|UserData|AppUserProfile)" --type-add 'graphql:*.{graphql,gql}'
# Search for type definitions in code (TypeScript/JavaScript)
echo "Searching for TypeScript/JavaScript type definitions..."
ast-grep --pattern 'type $type = {
$$$
}'
ast-grep --pattern 'interface $type {
$$$
}'
Length of output: 930
Script:
#!/bin/bash
# Let's examine the actual schema definitions to verify the mock structure
echo "Checking User, UserData, and AppUserProfile schema definitions..."
cat schema.graphql | awk '/type (User|UserData|AppUserProfile)/{p=1;print;next} p&&/^}/{p=0;print} p{print}'
Length of output: 2410
public/locales/zh/translation.json (1)
958-963: LGTM! The translations for tag management are well-structured.
The new translations for tag management functionality in the MemberDetail section are clear, consistent, and properly localized in Chinese:
- "取消分配标签" for unassigning tags
- "已分配标签" for showing assigned tags
- "未分配标签" for indicating no tags are assigned
Let's verify the consistency of these translations across other locale files:
✅ Verification successful
Chinese translations for tag management are consistent with other locales
The translations are well-implemented and all the required keys are present across all locale files (en, fr, hi, sp, zh). The Chinese translations maintain semantic equivalence with other languages while following proper Chinese language conventions:
- "取消分配标签" (Unassign Tag) aligns with English "Unassign Tag", French "Désassigner l'étiquette", etc.
- "已分配标签" (Tags Assigned) matches the meaning in other locales
- "未分配标签" (No Tags Assigned) correctly conveys the same concept as other translations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the same keys exist in other locale files
# Expected: The keys should exist in all locale files (en, fr, hi, sp)
for key in "unassignUserTag" "unassignUserTagMessage" "successfullyUnassigned" "tagsAssigned" "noTagsAssigned"; do
echo "Checking key: $key"
rg -l "\"$key\":" "public/locales/{en,fr,hi,sp}/translation.json"
done
Length of output: 1754
Script:
#!/bin/bash
# First, let's find all translation files
echo "Available translation files:"
fd "translation.json" public/locales
# Then check for these keys in each translation file
echo -e "\nChecking translation keys:"
for key in "unassignUserTag" "unassignUserTagMessage" "successfullyUnassigned" "tagsAssigned" "noTagsAssigned"; do
echo -e "\nKey: $key"
fd "translation.json" public/locales -x rg "\"$key\":" {}
done
Length of output: 3728
public/locales/en/translation.json (1)
987-992: LGTM! Translation keys added for tag management.
The new translation keys are well-structured and align with the PR objectives for tag management functionality.
Let's verify if these keys are present in other language files:
✅ Verification successful
Translation keys are properly synchronized across all language files
The verification confirms that all new translation keys (talawaApiUnavailable, unassignUserTag, unassignUserTagMessage, successfullyUnassigned, tagsAssigned, noTagsAssigned) are present in all language files (fr, hi, sp, zh). The translations are well-maintained across the internationalization system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new translation keys exist in other language files
# Expected: Keys should be present in fr/hi/sp/zh translation files
# Define the keys to check
keys=(
"talawaApiUnavailable"
"unassignUserTag"
"unassignUserTagMessage"
"successfullyUnassigned"
"tagsAssigned"
"noTagsAssigned"
)
# Check each key in other language files
for key in "${keys[@]}"; do
echo "Checking key: $key"
fd -e json -p "public/locales/(fr|hi|sp|zh)" --exec rg -l "\"$key\":"
done
Length of output: 2248
public/locales/hi/translation.json (1)
958-963: LGTM! The translations are well-formatted and consistent.
The new translations for tag-related functionality are properly formatted and maintain consistency with the existing translations in the file.
Let's verify that these translations are present in other language files:
✅ Verification successful
All tag-related translations are present and consistent across language files
The verification confirms that all the new tag-related keys (unassignUserTag, unassignUserTagMessage, successfullyUnassigned, tagsAssigned, noTagsAssigned) are properly translated in all language files (en, fr, sp, zh). Each key has appropriate translations that maintain semantic meaning across languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new tag-related translations exist in all language files
# Expected: The keys should be present in en, fr, sp, and zh translation files
for key in "unassignUserTag" "unassignUserTagMessage" "successfullyUnassigned" "tagsAssigned" "noTagsAssigned"; do
echo "Checking translations for key: $key"
for lang in "en" "fr" "sp" "zh"; do
rg -U "\"$key\":" "public/locales/$lang/translation.json"
done
done
Length of output: 4808
public/locales/sp/translation.json (1)
959-964: LGTM! The Spanish translations for tag management are complete and accurate.
The new translation keys provide a complete set of Spanish translations needed for the tag management functionality on the MemberDetail screen, including actions, messages, and empty states.
src/screens/MemberDetail/MemberDetail.tsx (8)
5-5: Imports updated appropriately
The necessary imports for useLocation, useNavigate, and useParams from react-router-dom have been correctly added.
31-36: New dependencies imported correctly
Imports for InterfaceTagData, InfiniteScroll, InfiniteScrollLoader, UnassignUserTagModal, UNASSIGN_USER_TAG, and TAGS_QUERY_DATA_CHUNK_SIZE are properly added to support the new tagging functionality.
63-68: State and navigation hooks initialized correctly
Initialization of orgId using useParams, navigate using useNavigate, and unassignUserTagModalIsOpen state are appropriate for managing routing and modal state.
103-109: Ensure correct variable usage in useQuery
While setting up the useQuery hook for USER_DETAILS, consider verifying the variable names and ensuring consistency. The usage of currentUrl as the user ID is acceptable if it aligns with the GraphQL query requirements.
180-209: Ensure proper error handling and state management
The handleUnassignUserTag and toggleUnassignUserTagModal functions are implemented correctly, with appropriate error handling and state updates.
731-737: Pass correct props to UnassignUserTagModal component
Ensure that the props passed to UnassignUserTagModal correspond to its expected interface, especially after correcting state variable names and functions.
143-147:
Correct redundant user property access
The userData variable is already assigned as user?.user. Accessing userData?.user introduces an extra layer that could lead to undefined values. Update the property access to remove the redundant user.
Apply this diff to fix the property access:
- const tagsAssigned =
- userData?.user?.tagsAssignedWith.edges.map(
+ const tagsAssigned =
+ userData?.tagsAssignedWith.edges.map(
(edge: { node: InterfaceTagData; cursor: string }) => edge.node,
) ?? /* istanbul ignore next */ [];Likely invalid or redundant comment.
686-727:
Confirm event data availability in rendering events attended
Ensure that userData?.user.eventsAttended is correctly accessed. Based on previous corrections, it might need to be userData?.eventsAttended.
Run the following script to verify the property access:
If necessary, apply this diff to correct the property access:
- {!userData?.user.eventsAttended?.length ? (
+ {!userData?.eventsAttended?.length ? (
// ...
) : (
- userData.user.eventsAttended.map(
+ userData.eventsAttended.map(
// ...
)
)}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
public/locales/fr/translation.json (1)
958-963: Translations look good with a minor capitalization suggestion.The translations are grammatically correct with proper gender agreement for "étiquette" (feminine). However, there's a minor improvement that could be made:
Consider capitalizing the first letter of "étiquettes" in the
tagsAssignedtranslation:- "tagsAssigned": "étiquettes assignées", + "tagsAssigned": "Étiquettes assignées",
What kind of change does this PR introduce?
Lists the tags assigned to a user. Adds to functionality to navigate to that tag and unassign it.
Issue Number:
Fixes #2424
Did you add tests for your changes?
Yes
Demo
tagsAssignedWith.mp4
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests
Chores