feat: Add filtering and sorting on userTag screens (GSoC) #2398
Conversation
WalkthroughThe changes in this pull request primarily involve enhancements to the translation files for multiple languages, adding a new key for user prompts related to tag management. Additionally, modifications to various GraphQL queries have been made to support filtering and sorting of user tags. The UI components have been updated to include search functionality and improved layouts for tag management. CSS adjustments have been made to enhance loading states and user interface elements. Overall, these changes aim to improve user interaction and experience within the tag management features. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (33)
src/utils/organizationTagsUtils.ts (1)
47-48: Enhance the type documentation.The type implementation looks good and aligns with the PR objectives. Consider improving the comment to be more descriptive.
-// the sortedByType +// Defines the sorting order for tag-related operationssrc/GraphQl/Queries/userTagQueries.ts (2)
67-68: LGTM: Comprehensive filtering and sorting for child tagsThe implementation properly handles pagination, filtering, and sorting for child tags.
Consider implementing field-level permissions or using GraphQL aliases if different components need different subsets of this data to optimize response size.
Also applies to: 72-79
Line range hint
1-152: Update query documentation to reflect new parametersThe queries have good basic documentation, but it should be updated to include information about the new filtering and sorting parameters.
Example documentation update for the first query:
/** * GraphQL query to retrieve organization members assigned a certain tag. * * @param id - The ID of the tag that is assigned. + * @param where - Filter criteria for assigned users + * @param sortedBy - Sorting criteria for assigned users * @returns The list of organization members. */src/components/AddPeopleToTag/AddPeopleToTagsMocks.ts (2)
165-258: Consider adding more comprehensive test cases.While the current test cases cover basic filtering scenarios, consider adding:
- Combined firstName AND lastName filters
- Edge cases with special characters
- Case sensitivity scenarios
Example addition:
{ request: { query: USER_TAGS_MEMBERS_TO_ASSIGN_TO, variables: { id: '1', first: TAGS_QUERY_DATA_CHUNK_SIZE, where: { firstName: { starts_with: 'John' }, lastName: { starts_with: 'Doe' }, }, }, }, result: { data: { getUsersToAssignTo: { // ... result structure } } } }
Line range hint
1-294: Consider implementing a mock data generator utility.The mock data follows a consistent pattern but requires manual maintenance. Consider creating a utility function to generate mock data, which would:
- Reduce duplication
- Make it easier to add new test cases
- Ensure consistency in mock data structure
Example:
function generateMockResponse(filters: { firstName?: string; lastName?: string; }, pagination?: { after?: string; hasNextPage: boolean; }) { // Generate mock response based on filters and pagination }src/screens/ManageTag/ManageTagMocks.ts (2)
172-221: Consider adding edge case search scenarios.The mock effectively tests basic search functionality. Consider adding test cases for:
- Case sensitivity
- Special characters
- Empty result sets
Mock structure needs adjustment to match the GraphQL schema
The mock structure has some discrepancies with the actual GraphQL query:
- The query name in the schema is
UserTagDetails, but the mock uses a different structure- The query uses
getUserTag(id: $id)as the resolver, but the mock showsgetAssignedUsers- The
ancestorTagsfield in the mock is not present in the actual query definition🔗 Analysis chain
Line range hint
9-121: LGTM! Verify query structure matches the API schema.The mock correctly implements filtering and sorting with appropriate default values. The pagination structure is well-maintained.
The previous pattern might have been too strict. Let's try a broader search to find the query definition and related schema types to verify the mock structure.
The actual query shows different input types than initially assumed. Let's verify the complete structure including the input types to ensure the mock aligns with the schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the mock structure matches the actual GraphQL schema # Look for the query definition to ensure alignment ast-grep --pattern 'export const USER_TAGS_ASSIGNED_MEMBERS = gql` query $_($where: $_) { getAssignedUsers(where: $where) { $$$ } } `'Length of output: 154
Script:
#!/bin/bash # Search for the GraphQL query definition with a more flexible pattern rg -U "USER_TAGS_ASSIGNED_MEMBERS.*=.*gql" -A 20 # Also search for any GraphQL type definitions that might help verify the schema rg -U "input.*UserFilter|type.*User.*{|interface.*User.*{" -A 10Length of output: 1593
Script:
#!/bin/bash # Let's fix the regex and search for type definitions rg -U "input.*UserTagUsersAssignedToWhereInput|input.*UserTagUsersAssignedToSortedByInput" -A 5 # Also get the complete query definition to verify the response structure rg -U "USER_TAGS_ASSIGNED_MEMBERS.*=.*gql" -A 30Length of output: 2021
src/GraphQl/Queries/OrganizationQueries.ts (1)
92-103: Add JSDoc documentation for the new parameters.Other queries in this file have detailed JSDoc documentation. Please maintain consistency by documenting the new
whereandsortedByparameters.Add documentation like this:
/** * GraphQL query to retrieve the list of user tags belonging to an organization. * * @param id - ID of the organization. * @param first - Number of tags to retrieve "after" (if provided) a certain tag. * @param after - Id of the last tag on the current page. * @param last - Number of tags to retrieve "before" (if provided) a certain tag. * @param before - Id of the first tag on the current page. * @param where - Filter conditions for user tags. * @param sortedBy - Sorting criteria for user tags. * @returns The list of organizations based on the applied filters. */src/components/AddPeopleToTag/AddPeopleToTag.test.tsx (4)
66-91: LGTM! Consider adding documentation for the merge logic.The cache configuration with type policies is well-implemented. The merge function correctly handles the concatenation of edges while preserving other fields.
Consider adding comments to explain the merge logic:
const cache = new InMemoryCache({ typePolicies: { Query: { fields: { getUserTag: { + // Merge function concatenates edges from existing and incoming data + // while preserving other fields to support infinite scroll merge(existing = {}, incoming) { const merged = { ...existing, ...incoming, usersToAssignTo: {
177-208: Fix typo and improve test description.The test implementation is good, but there are some minor improvements needed:
- test('searchs for tags where the firstName matches the provided firstName search input', async () => { + test('searches and filters members by firstName input', async () => {
210-241: Fix typo and improve test description.Similar improvement needed for the lastName search test:
- test('searchs for tags where the lastName matches the provided lastName search input', async () => { + test('searches and filters members by lastName input', async () => {
Line range hint
1-24: Consider adding more edge case tests.The test coverage is good, but consider adding these test cases to improve robustness:
- Test search with special characters in names
- Test search with empty/whitespace input
- Test infinite scroll when there are no more results
- Test error handling for network failures during search
- Test concurrent searches (typing fast in both firstName and lastName fields)
Would you like me to help implement any of these test cases?
src/screens/SubTags/SubTags.test.tsx (1)
239-258: Consider adding edge cases to search test.The current test effectively verifies basic search functionality. Consider adding test cases for:
- Empty search results
- Special characters in search
- Case sensitivity
src/components/TagActions/TagActions.test.tsx (1)
202-221: Fix typo in test name and consider adding edge cases.The test effectively verifies the search functionality, but has a few areas for improvement:
- Fix the grammatical error in the test name from "searchs" to "searches"
- Consider adding edge cases to strengthen test coverage:
- Empty search input
- Search with no matching results
Apply this diff to fix the typo:
- test('searchs for tags where the name matches the provided search input', async () => { + test('searches for tags where the name matches the provided search input', async () => {src/utils/interfaces.ts (2)
215-215: LGTM! Consider adding JSDoc comments.The addition of
parentTagandancestorTagsestablishes a proper hierarchical structure for tags. This will effectively support the filtering and sorting functionality mentioned in the PR objectives.Consider adding JSDoc comments to document the hierarchical relationship:
/** * Represents a user tag with hierarchical structure. * @property parentTag - Direct parent tag reference * @property ancestorTags - Array of all ancestor tags in hierarchical order */Also applies to: 222-225
275-278: LGTM! Consider extracting common types.The addition of
ancestorTagsmaintains consistency across tag-related interfaces.Consider extracting the common
ancestorTagstype to reduce duplication:interface AncestorTag { _id: string; name: string; } interface BaseTagInterface { ancestorTags: AncestorTag[]; } interface InterfaceQueryUserTagsAssignedMembers extends BaseTagInterface { name: string; usersAssignedTo: InterfaceTagMembersData; }src/screens/OrganizationTags/OrganizationTagsMocks.ts (2)
12-13: Enhance test coverage with diverse mock data scenarios.While the mock data structure is consistent, consider adding more diverse test cases:
- Tags with different parent-child relationships
- Tags with varying depths of ancestor hierarchies
- Edge cases for filtering (e.g., special characters, empty names)
Also applies to: 26-33, 41-48, 56-63, 71-78, 86-93, 101-108, 116-123, 131-138, 146-153, 161-168, 193-194, 207-214, 222-229
Line range hint
1-426: Consider implementing a mock data generator.Given the repetitive nature of the mock data and the need to maintain consistency across different scenarios, consider implementing a mock data generator function. This would:
- Reduce duplication and make updates easier
- Ensure consistency in test data
- Make it easier to add new test cases
- Help maintain the relationship between parent and child tags
Example structure:
interface TagGeneratorOptions { prefix?: string; count?: number; sortOrder?: 'ASCENDING' | 'DESCENDING'; parentTagId?: string; } function generateTagMocks(options: TagGeneratorOptions) { // Generate mock data based on options }Would you like me to help create this generator function?
src/components/TagActions/TagActions.tsx (1)
310-321: Enhance search input accessibility and semanticsA few suggestions to improve the search implementation:
- Add
aria-labelto the search input for better screen reader support- Update the input's
idto reflect its purpose (currently "userName" for tag search)- Consider using a button element for the search icon to make it keyboard accessible
<div className="mt-3 position-relative"> - <i className="fa fa-search position-absolute text-body-tertiary end-0 top-50 translate-middle" /> + <button + className="fa fa-search position-absolute text-body-tertiary end-0 top-50 translate-middle border-0 bg-transparent" + aria-label={t('search')} + onClick={() => {/* Optional: implement search on click */}} + /> <Form.Control type="text" - id="userName" + id="tagSearch" className="bg-light" placeholder={tCommon('searchByName')} onChange={(e) => setTagSearchName(e.target.value.trim())} data-testid="searchByName" autoComplete="off" + aria-label={tCommon('searchByName')} /> </div>src/screens/SubTags/SubTagsMocks.ts (3)
Line range hint
1-13: Consider adding TypeScript interfaces for mock data structure.The mock setup would benefit from TypeScript interfaces defining the expected shape of the query variables and responses. This would make the mocks more maintainable and catch potential structure mismatches early.
Line range hint
14-221: Consider extracting repeated mock data structures into helper functions.The mock data contains repeated structures for nodes that could be generated using helper functions. This would make the mocks more maintainable and reduce the chance of inconsistencies.
Example helper function:
const createMockSubTag = (id: string, name: string, options?: { usersCount?: number, childTagsCount?: number, ancestors?: Array<{_id: string, name: string}> }) => ({ node: { _id: id, name, usersAssignedTo: { totalCount: options?.usersCount ?? 0 }, childTags: { totalCount: options?.childTagsCount ?? 0 }, ancestorTags: options?.ancestors ?? [] }, cursor: id });
Line range hint
489-502: Consider adding more error scenarios.The error mocks could be expanded to cover:
- Network errors
- Validation errors with specific error messages
- Error cases for mutations
- Error cases for pagination
src/components/AddPeopleToTag/AddPeopleToTag.tsx (2)
57-87: LGTM! Consider adding type annotations for useState.The search state implementation and reset logic look good. For better type safety, consider explicitly typing the state variables.
- const [memberToAssignToSearchFirstName, setMemberToAssignToSearchFirstName] = - useState(''); + const [memberToAssignToSearchFirstName, setMemberToAssignToSearchFirstName] = + useState<string>(''); - const [memberToAssignToSearchLastName, setMemberToAssignToSearchLastName] = - useState(''); + const [memberToAssignToSearchLastName, setMemberToAssignToSearchLastName] = + useState<string>('');
274-296: Enhance accessibility for the remove member button.While the UI implementation looks good, the remove member button could be more accessible.
- <i - className={`${styles.removeFilterIcon} fa fa-times ms-2 text-body-tertiary`} - onClick={() => removeMember(member._id)} - data-testid="clearSelectedMember" - /> + <button + type="button" + className={`${styles.removeFilterIcon} btn btn-link border-0 p-0 ms-2`} + onClick={() => removeMember(member._id)} + data-testid="clearSelectedMember" + aria-label={`Remove ${member.firstName} ${member.lastName}`} + > + <i className="fa fa-times text-body-tertiary" /> + </button>src/screens/OrganizationTags/OrganizationTags.tsx (2)
124-126: Consider adding dependency array conditions to useEffect.The current implementation will trigger a refetch on every component mount. Consider adding relevant dependencies to prevent unnecessary refetches.
useEffect(() => { orgUserTagsRefetch(); - }, []); + }, [orgUserTagsRefetch]);
134-137: Consider enhancing tag name validation.While checking for empty strings is good, consider adding:
- Maximum length validation
- Special character restrictions
- Duplicate tag name validation
src/screens/ManageTag/ManageTag.test.tsx (2)
327-346: Enhance search test coverage and fix grammatical error.
- The test name contains a grammatical error: "searchs" should be "searches"
- Consider adding negative test cases:
- Search with no matches
- Search with empty string
- Search with special characters
Apply this diff to fix the grammar and improve the test description:
- test('searchs for tags where the name matches the provided search input', async () => { + test('searches for tags where the name matches the provided search input', async () => {
327-403: Consider restructuring tests for better maintainability.While the tests cover the core functionality well, consider these architectural improvements:
Create a
test-utils.tsfile to store common helper functions for:
- Sort button interactions
- Search input interactions
- Common assertions
Group related tests using
describeblocks for:
- Search functionality
- Sort functionality
- Combined search and sort scenarios
This will improve test maintainability and make it easier to add more test cases in the future.
src/screens/SubTags/SubTags.tsx (2)
310-322: Add aria-labels to dropdown items for better accessibility<Dropdown.Item data-testid="latest" + aria-label={tCommon('Latest')} onClick={() => setTagSortOrder('DESCENDING')} > {tCommon('Latest')} </Dropdown.Item> <Dropdown.Item data-testid="oldest" + aria-label={tCommon('Oldest')} onClick={() => setTagSortOrder('ASCENDING')} > {tCommon('Oldest')} </Dropdown.Item>
Line range hint
144-154: Use translation key for error messageThe error message should use the translation system for consistency and internationalization support.
<h6 className="fw-bold text-danger text-center"> - Error occured while loading sub tags + {t('errorLoadingSubTags')} </h6>Don't forget to add the translation key to your translation files.
src/components/TagActions/TagActionsMocks.ts (1)
249-321: LGTM: Search mock provides good test coverage.The new mock entry effectively demonstrates tag hierarchy and relationships. However, consider adding a test case for a search term that yields no results to ensure proper handling of empty states.
Add another mock entry with a search term that returns no results:
{ request: { query: ORGANIZATION_USER_TAGS_LIST, variables: { id: '123', first: TAGS_QUERY_DATA_CHUNK_SIZE, where: { name: { starts_with: 'nonexistent' } }, }, }, result: { data: { organizations: [{ userTags: { edges: [], pageInfo: { hasNextPage: false, hasPreviousPage: false, startCursor: null, endCursor: null }, totalCount: 0 } }] } } }src/screens/ManageTag/ManageTag.tsx (2)
251-251: Internationalize the error messageThe error message "Error occured while loading assigned users" is hardcoded in English. To support internationalization, consider using the
tfunction fromuseTranslation.Update the code as follows:
<h6 className="fw-bold text-danger text-center"> - Error occured while loading assigned users + {t('errorLoadingAssignedUsers')} </h6>Ensure the translation key
errorLoadingAssignedUsersis added to your localization files.
365-366: Avoid redundant trimming of search inputCurrently, the search input is trimmed both in the
onChangehandler and in theuseEffect. Trimming in both places is unnecessary.Choose one place to trim the input. For example, you could remove the
.trim()in theonChangehandler:onChange={(e) => - setAssignedMemberSearchInput(e.target.value.trim()) + setAssignedMemberSearchInput(e.target.value) }Alternatively, if you prefer to trim in the
onChangehandler, you can remove the.trim()in theuseEffect.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (27)
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/OrganizationQueries.ts(1 hunks)src/GraphQl/Queries/userTagQueries.ts(8 hunks)src/components/AddPeopleToTag/AddPeopleToTag.module.css(2 hunks)src/components/AddPeopleToTag/AddPeopleToTag.test.tsx(6 hunks)src/components/AddPeopleToTag/AddPeopleToTag.tsx(6 hunks)src/components/AddPeopleToTag/AddPeopleToTagsMocks.ts(4 hunks)src/components/TagActions/TagActions.module.css(2 hunks)src/components/TagActions/TagActions.test.tsx(2 hunks)src/components/TagActions/TagActions.tsx(8 hunks)src/components/TagActions/TagActionsMocks.ts(21 hunks)src/components/TagActions/TagNode.tsx(1 hunks)src/screens/ManageTag/ManageTag.test.tsx(2 hunks)src/screens/ManageTag/ManageTag.tsx(13 hunks)src/screens/ManageTag/ManageTagMocks.ts(6 hunks)src/screens/OrganizationTags/OrganizationTags.test.tsx(3 hunks)src/screens/OrganizationTags/OrganizationTags.tsx(11 hunks)src/screens/OrganizationTags/OrganizationTagsMocks.ts(6 hunks)src/screens/SubTags/SubTags.test.tsx(4 hunks)src/screens/SubTags/SubTags.tsx(10 hunks)src/screens/SubTags/SubTagsMocks.ts(18 hunks)src/utils/interfaces.ts(2 hunks)src/utils/organizationTagsUtils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/components/AddPeopleToTag/AddPeopleToTag.module.css
- src/components/TagActions/TagActions.module.css
- src/components/TagActions/TagNode.tsx
🧰 Additional context used
📓 Learnings (6)
src/components/AddPeopleToTag/AddPeopleToTag.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/components/AddPeopleToTag/AddPeopleToTag.tsx:78-104
Timestamp: 2024-10-30T15:46:05.784Z
Learning: In `src/components/AddPeopleToTag/AddPeopleToTag.tsx`, and similar components, the team prefers to keep the current implementation of the `updateQuery` logic in infinite scroll functionality consistent across all screens.
src/components/AddPeopleToTag/AddPeopleToTagsMocks.ts (2)
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.
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.
src/components/TagActions/TagActions.test.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2362
File: src/components/TagActions/TagActions.test.tsx:251-274
Timestamp: 2024-10-28T06:33:09.726Z
Learning: In `TagActions.test.tsx`, negative test cases for tag operations can be deferred and added later if necessary, according to the team's plan.
src/screens/ManageTag/ManageTag.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/ManageTag/ManageTag.tsx:113-147
Timestamp: 2024-10-30T13:29:28.310Z
Learning: In the `src/screens/ManageTag/ManageTag.tsx` file, the `InfiniteScroll` component uses the `loadMoreAssignedMembers` function efficiently, and additional error handling is unnecessary.
src/screens/ManageTag/ManageTagMocks.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.
src/screens/OrganizationTags/OrganizationTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/OrganizationTags/OrganizationTags.tsx:350-376
Timestamp: 2024-10-30T13:18:56.627Z
Learning: In the `src/screens/OrganizationTags/OrganizationTags.tsx` file, the `OrganizationTags` component uses chunk size with the `InfiniteScroll` component and an outer parent div for handling data loading and scrolling.
🪛 Biome
src/components/TagActions/TagActions.tsx
[error] 367-367: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
[error] 378-378: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (38)
src/utils/organizationTagsUtils.ts (1)
47-49: Verify consistent usage of SortedByType.
Let's ensure the new type is being used consistently across components.
✅ Verification successful
SortedByType is consistently used across the codebase
The type is properly imported and utilized in all relevant components:
OrganizationTags.tsx: Used for tagSortOrder stateSubTags.tsx: Used for tagSortOrder stateManageTag.tsx: Used for assignedMemberSortOrder state
All literal string usages of 'ASCENDING' and 'DESCENDING' are properly typed with SortedByType, including mock data in respective *Mocks.ts files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of SortedByType across the codebase
# Expected: Find references in tag-related components and queries
# Search for SortedByType usage
echo "=== Checking SortedByType usage ==="
rg "SortedByType" -A 2
# Search for literal string usage to catch any inconsistencies
echo -e "\n=== Checking for potential inconsistent sort type usage ==="
rg "'ASCENDING'|'DESCENDING'" --type ts
Length of output: 4193
src/GraphQl/Queries/userTagQueries.ts (1)
17-18: LGTM: Well-structured filtering and sorting implementation
The addition of where and sortedBy parameters enhances the query's functionality, aligning perfectly with the PR objectives for implementing filtering and sorting on tag screens.
Also applies to: 27-28
src/components/AddPeopleToTag/AddPeopleToTagsMocks.ts (3)
12-15: LGTM! Well-structured default filters.
The empty string filters provide a good default state for unfiltered results.
124-127: LGTM! Consistent filter handling with pagination.
The filters are correctly maintained across paginated requests, ensuring consistent results.
284-287: LGTM! Error case properly updated.
The error mock correctly includes the new filtering structure, maintaining consistency with successful cases.
src/screens/ManageTag/ManageTagMocks.ts (4)
Line range hint 1-8: LGTM! Import changes align with new filtering functionality.
The removal of USER_TAG_ANCESTORS and retention of necessary mutation imports properly reflect the updated functionality.
Line range hint 122-171: LGTM! Pagination mock maintains consistency.
The mock correctly implements cursor-based pagination while maintaining the new filtering and sorting structure.
222-270: LGTM! Sort order mock effectively tests both orderings.
The mock properly demonstrates ascending and descending sort functionality with consistent data.
Line range hint 326-336: LGTM! Error mock updated with new parameters.
The error mock correctly maintains consistency with the success mocks by including the new filtering and sorting parameters.
src/GraphQl/Queries/OrganizationQueries.ts (3)
92-93: LGTM: Well-structured query parameters for filtering and sorting.
The addition of UserTagWhereInput and UserTagSortedByInput parameters provides a clean interface for implementing the filtering and sorting functionality.
96-103: LGTM: Clean integration of filtering and sorting with pagination.
The parameters are well-organized and maintain compatibility with existing pagination functionality.
108-110: Consider paginating ancestorTags for deep hierarchies.
While the addition of parentTag and ancestorTags fields is good for supporting hierarchical relationships, consider the performance implications of fetching all ancestor tags without pagination.
Let's check the potential depth of tag hierarchies:
Consider adding pagination to ancestorTags if deep hierarchies are expected:
ancestorTags(first: $first, last: $last) {
edges {
node {
_id
name
}
}
pageInfo {
hasNextPage
hasPreviousPage
}
}Also applies to: 117-120
src/screens/OrganizationTags/OrganizationTags.test.tsx (1)
280-284: LGTM: Good error handling test coverage.
The added validation test for empty tag name is well-implemented and verifies the correct error message.
src/screens/SubTags/SubTags.test.tsx (2)
54-79: Well-implemented cache merge strategy!
The cache configuration properly handles merging of existing and incoming tag data, with good handling of edge cases for childTags. This is crucial for maintaining cache consistency during pagination and updates.
83-84: Good test setup with proper cache management!
The addition of cache reset in beforeEach and passing the cache to MockedProvider ensures clean test state and proper cache behavior.
Also applies to: 114-114
src/components/TagActions/TagActions.test.tsx (1)
346-355: LGTM! Good addition of error handling test.
The new test case effectively validates the error feedback when attempting to submit without selecting any tags. This improves the test coverage and ensures a better user experience by verifying proper error messaging.
src/screens/OrganizationTags/OrganizationTagsMocks.ts (1)
420-421: LGTM!
The error mock data is properly updated to include the new query parameters, maintaining consistency with the successful query structure.
src/components/TagActions/TagActions.tsx (2)
53-54: LGTM: Well-implemented search functionality!
The search integration with GraphQL is properly implemented, with appropriate state management and query filtering.
Also applies to: 64-64
327-329: LGTM: Improved loading state visualization!
The switch to InfiniteScrollLoader provides better visual feedback and maintains consistency across components.
src/screens/SubTags/SubTagsMocks.ts (4)
Line range hint 222-271: LGTM! Pagination implementation follows GraphQL best practices.
The mock correctly implements cursor-based pagination with proper page info structure.
336-466: LGTM! Comprehensive test coverage for search and sort functionality.
The mock data properly covers:
- Search filtering with prefix matching
- Both ascending and descending sort orders
- Consistent data structure with proper cursors
472-486: LGTM! Create tag mutation mock is correctly structured.
The mock provides the necessary response structure for tag creation.
Line range hint 272-329: Verify ancestor chain consistency in nested tags.
While the mock data structure looks correct, consider adding tests to verify that:
- The ancestor chain is always ordered from root to immediate parent
- The ancestor IDs match the actual parent tags
✅ Verification successful
Let me search for the actual ancestor chain implementations to better understand the structure.
Let me try a different approach to find the ancestor chain implementations.
Based on the search results, I can now generate the final response:
Ancestor chain implementation is consistent and correct
The mock data correctly implements the ancestor chain hierarchy:
- First-level subtags (e.g., 'subTag1', 'subTag2') have a single ancestor: userTag1
- Second-level subtag (subTag1.1) has the complete chain: [userTag1, subTag1]
- The order is consistently maintained from root to immediate parent
- IDs and names match across the hierarchy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for inconsistencies in ancestor chains across test files
rg -U "ancestorTags.*?\[.*?\]" --multiline
Length of output: 3181
Script:
#!/bin/bash
# Search for ancestor chain implementations with context
rg -U "ancestorTags.*?\[.*?\{.*?\}.*?\]" --multiline -A 5
Length of output: 59
Script:
#!/bin/bash
# Search for ancestor chain implementations with more specific pattern
rg -U 'ancestorTags:\s*\[\s*\{\s*_id:' --multiline -A 10 src/screens/SubTags/SubTagsMocks.ts
Length of output: 6096
src/components/AddPeopleToTag/AddPeopleToTag.tsx (2)
157-160: LGTM! Good validation with user feedback.
The empty selection check with internationalized error message is well implemented.
Line range hint 330-352: LGTM! Clean implementation of infinite scroll with loading states.
The loading states and infinite scroll implementation are well-handled with proper component separation.
src/screens/OrganizationTags/OrganizationTags.tsx (3)
79-80: LGTM! Query parameters properly implemented.
The filtering and sorting parameters are correctly integrated into the GraphQL query, aligning with the PR objectives.
211-221: LGTM! Well-implemented breadcrumb navigation.
The breadcrumb implementation for ancestor tags provides good hierarchical context and navigation.
375-413: LGTM! Robust implementation of infinite scrolling with DataGrid.
The implementation:
- Properly handles loading states
- Includes error boundaries
- Follows the chunk size pattern from previous implementations
- Has appropriate fallback UI for empty states
src/screens/SubTags/SubTags.tsx (2)
19-22: LGTM: Well-structured state management for search and sort functionality
The implementation uses proper TypeScript types and follows React best practices for state management.
Also applies to: 55-57
383-422: LGTM: Well-implemented infinite scroll with DataGrid
The implementation of infinite scroll with DataGrid is well-structured and includes:
- Proper loading states
- Error handling
- Row customization
- Empty state handling
src/components/TagActions/TagActionsMocks.ts (3)
16-16: LGTM: Mock data structure updated correctly for filtering.
The changes consistently add the new filtering capability and tag relationship fields (parentTag and ancestorTags) across all mock entries. The empty string filter (starts_with: '') is appropriate for showing all tags by default.
Also applies to: 29-29, 36-36, 44-44, 51-51, 59-59, 66-66, 74-74, 81-81, 89-89, 96-96, 104-104, 111-111, 119-119, 126-126, 134-134, 141-141, 149-149, 156-156, 164-164, 171-171, 196-196, 209-209, 216-216, 224-224, 231-231
346-351: LGTM: Subtag hierarchy correctly represented.
The ancestorTags field is consistently added to all subtags, correctly representing their relationship to the parent tag.
Also applies to: 365-370, 384-389, 403-408, 422-427, 441-446, 460-465, 479-484, 498-503, 517-522, 535-535, 565-570, 583-583
629-629: LGTM: Error mocks updated consistently.
The error mocks have been properly updated to include the new filtering capability and tag relationship fields, maintaining consistency with the successful mocks.
Also applies to: 643-643, 656-656, 663-663, 671-671, 678-678
public/locales/zh/translation.json (1)
323-324: LGTM! The translation changes look good.
The changes enhance the tag management features by:
- Adding a trailing comma after
addChildTagfor better maintainability - Adding an accurate Chinese translation for the tag name input prompt
The translation is natural and consistent with the existing style.
public/locales/en/translation.json (1)
323-324: LGTM! Translation key added for tag management.
The new translation key enterTagName is well-placed in the organizationTags section and follows the existing naming conventions. The translation text is clear and consistent with other similar prompts in the file.
public/locales/hi/translation.json (1)
323-324: LGTM! The translation addition is well-structured.
The new translation key "enterTagName" with Hindi translation "टैग का नाम दर्ज करें" is properly implemented and maintains consistency with the existing translations.
Let's verify that this translation key exists across other locale files:
✅ Verification successful
Translation key "enterTagName" is consistently present across all locale files
The verification confirms that the "enterTagName" key exists with appropriate translations in all locale files:
- English (en): "Enter Tag Name"
- French (fr): "Entrez le nom du étiquette"
- Spanish (sp): "Ingrese el nombre de la etiqueta"
- Chinese (zh): "输入标签名称"
- Hindi (hi): "टैग का नाम दर्ज करें"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if enterTagName exists in all locale files
# Expected: The key should exist in en, fr, sp, zh locale files
for locale in en fr sp zh; do
echo "Checking $locale translation..."
rg -A 1 '"enterTagName"' "public/locales/$locale/translation.json"
done
Length of output: 801
public/locales/sp/translation.json (1)
323-324: LGTM! The Spanish translation for the tag name input prompt is correctly implemented.
The translation is grammatically correct and consistent with Spanish language conventions and similar prompts in the file.
src/screens/ManageTag/ManageTag.tsx (1)
386-395:
Verify the field used for sorting assigned members
The sorting functionality uses the id field to order assigned members by 'Latest' and 'Oldest'. However, the id may not accurately reflect the creation time or assignment time of members.
Please verify if sorting by id provides the intended ordering. If not, consider using a timestamp field like createdAt or assignedAt for accurate sorting.
Also applies to: 108-112
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2398 +/- ##
===========================================
+ Coverage 98.06% 98.08% +0.02%
===========================================
Files 255 255
Lines 7169 7208 +39
Branches 2087 2086 -1
===========================================
+ Hits 7030 7070 +40
+ Misses 129 128 -1
Partials 10 10 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/AddPeopleToTag/AddPeopleToTag.test.tsx (1)
66-91: LGTM! Consider adding type safety.The cache configuration with merge policy for
getUserTagis well implemented. It correctly handles the merging of existing and incoming data while maintaining immutability.Consider adding TypeScript interfaces for the merged data structure:
interface UsersToAssignTo { edges: unknown[]; // add other properties } interface UserTagData { usersToAssignTo?: UsersToAssignTo; // add other properties }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/pull-request.yml(1 hunks)public/locales/fr/translation.json(1 hunks)src/components/AddPeopleToTag/AddPeopleToTag.test.tsx(6 hunks)src/components/AddPeopleToTag/AddPeopleToTag.tsx(6 hunks)src/components/TagActions/TagActions.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- public/locales/fr/translation.json
- src/components/AddPeopleToTag/AddPeopleToTag.tsx
- src/components/TagActions/TagActions.test.tsx
🔇 Additional comments (3)
.github/workflows/pull-request.yml (1)
41-41: LGTM! Verify the excluded file exists.
The addition of src/components/TagActions/TagActionsMocks.ts to the list of excluded files follows the established pattern.
Let's verify that the excluded file exists:
✅ Verification successful
File exists and exclusion is valid
The file src/components/TagActions/TagActionsMocks.ts exists in the codebase, confirming that the exclusion in the line counting workflow is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the excluded file exists in the codebase
fd "TagActionsMocks.ts" --type f
Length of output: 77
src/components/AddPeopleToTag/AddPeopleToTag.test.tsx (2)
121-121: LGTM! Good test isolation practice.
Adding cache.reset() in beforeEach ensures proper test isolation by clearing any cached data between tests.
272-285: LGTM! Good test separation.
Excellent work on extracting the error handling into a separate test case. This improves test clarity and maintainability by following the single responsibility principle.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/screens/SubTags/SubTags.tsx (2)
Line range hint
55-78: Consider optimizing query variablesThe GraphQL query variables could be optimized to prevent unnecessary re-renders by using
useMemofor the variables object.+ const queryVariables = useMemo( + () => ({ + id: parentTagId, + first: TAGS_QUERY_DATA_CHUNK_SIZE, + where: { name: { starts_with: tagSearchName } }, + sortedBy: { id: tagSortOrder }, + }), + [parentTagId, tagSearchName, tagSortOrder] + ); const { data: subTagsData, error: subTagsError, loading: subTagsLoading, refetch: subTagsRefetch, fetchMore: fetchMoreSubTags, }: InterfaceOrganizationSubTagsQuery = useQuery(USER_TAG_SUB_TAGS, { - variables: { - id: parentTagId, - first: TAGS_QUERY_DATA_CHUNK_SIZE, - where: { name: { starts_with: tagSearchName } }, - sortedBy: { id: tagSortOrder }, - }, + variables: queryVariables, });Don't forget to add the following import:
import { useMemo } from 'react';
Line range hint
144-154: Use translation key for error messageThe error message is hardcoded. For consistency with i18n practices, use a translation key.
if (subTagsError) { return ( <div className={`${styles.errorContainer} bg-white rounded-4 my-3`}> <div className={styles.errorMessage}> <WarningAmberRounded className={styles.errorIcon} fontSize="large" /> <h6 className="fw-bold text-danger text-center"> - Error occured while loading sub tags + {t('errorLoadingSubTags')} </h6> </div> </div> ); }src/screens/ManageTag/ManageTag.tsx (2)
443-443: Simplify theclassNameassignment for readabilityThe
classNamefor the breadcrumb items can be simplified for better readability by avoiding nested template literals.Consider refactoring the
classNameas follows:- className={`ms-2 my-1 ${tag._id === currentTagId ? `fs-4 fw-semibold text-secondary` : `${styles.tagsBreadCrumbs} fs-6`}`} + className={`ms-2 my-1 ${ + tag._id === currentTagId + ? 'fs-4 fw-semibold text-secondary' + : `${styles.tagsBreadCrumbs} fs-6` + }`}
359-367: Ensure the search icon is accessibleThe search icon added lacks accessibility features such as
aria-labelorrole. This may affect users who rely on assistive technologies.Consider adding an
aria-hiddenattribute if the icon is purely decorative, or provide an accessible name:-<i className="fa fa-search position-absolute text-body-tertiary end-0 top-50 translate-middle" /> +<i + className="fa fa-search position-absolute text-body-tertiary end-0 top-50 translate-middle" + aria-hidden="true" +/>If the icon conveys important information, use
aria-label:+ aria-label="Search"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
public/locales/en/translation.json(2 hunks)public/locales/fr/translation.json(2 hunks)public/locales/hi/translation.json(2 hunks)public/locales/sp/translation.json(2 hunks)public/locales/zh/translation.json(2 hunks)src/components/AddPeopleToTag/AddPeopleToTag.tsx(7 hunks)src/screens/ManageTag/ManageTag.tsx(13 hunks)src/screens/OrganizationTags/OrganizationTags.tsx(11 hunks)src/screens/SubTags/SubTags.tsx(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- public/locales/en/translation.json
- public/locales/fr/translation.json
- public/locales/hi/translation.json
- public/locales/sp/translation.json
- public/locales/zh/translation.json
- src/components/AddPeopleToTag/AddPeopleToTag.tsx
🧰 Additional context used
📓 Learnings (3)
src/screens/ManageTag/ManageTag.tsx (3)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2398
File: src/screens/ManageTag/ManageTag.tsx:263-0
Timestamp: 2024-11-02T07:38:14.044Z
Learning: In `src/screens/ManageTag/ManageTag.tsx`, `currentTagName` is guaranteed to be non-empty when constructing the `orgUserTagAncestors` array for breadcrumbs.
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2398
File: src/screens/ManageTag/ManageTag.tsx:108-0
Timestamp: 2024-11-02T07:39:51.950Z
Learning: In `src/screens/ManageTag/ManageTag.tsx`, when `assignedMemberSearchFirstName` or `assignedMemberSearchLastName` are empty strings, the query in the `useQuery` for `USER_TAGS_ASSIGNED_MEMBERS` intentionally includes filters with empty strings to match all entries.
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/ManageTag/ManageTag.tsx:113-147
Timestamp: 2024-10-30T13:29:28.310Z
Learning: In the `src/screens/ManageTag/ManageTag.tsx` file, the `InfiniteScroll` component uses the `loadMoreAssignedMembers` function efficiently, and additional error handling is unnecessary.
src/screens/OrganizationTags/OrganizationTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2387
File: src/screens/OrganizationTags/OrganizationTags.tsx:350-376
Timestamp: 2024-10-30T13:18:56.627Z
Learning: In the `src/screens/OrganizationTags/OrganizationTags.tsx` file, the `OrganizationTags` component uses chunk size with the `InfiniteScroll` component and an outer parent div for handling data loading and scrolling.
src/screens/SubTags/SubTags.tsx (1)
Learnt from: meetulr
PR: PalisadoesFoundation/talawa-admin#2398
File: src/screens/SubTags/SubTags.tsx:77-0
Timestamp: 2024-11-02T07:36:11.553Z
Learning: In the `SubTags` component (`src/screens/SubTags/SubTags.tsx`), when filtering tag names in GraphQL queries, use `starts_with` as per the API's supported types. The `contains` filter is not currently supported by the API, and additional filtering criteria may be added later.
🔇 Additional comments (8)
src/screens/OrganizationTags/OrganizationTags.tsx (4)
52-53: LGTM: Well-structured state management for filtering and sorting
The new state variables are properly typed and initialized with sensible default values.
79-80: LGTM: GraphQL query properly enhanced with filtering and sorting
The query parameters are correctly structured to support the new filtering and sorting functionality.
305-313: LGTM: Search functionality properly implemented
The search implementation is clean and well-integrated with the UI.
327-341: LGTM: Sort functionality well-implemented with clear UX
The sorting implementation provides clear visual feedback and proper internationalization support.
src/screens/SubTags/SubTags.tsx (1)
347-424: Well-implemented infinite scroll with proper loading states
The implementation includes:
- Proper loading states
- Infinite scroll with error handling
- Clear data grid implementation with proper test IDs
src/screens/ManageTag/ManageTag.tsx (3)
135-135: Confirm the placement of istanbul ignore next comment
The /* istanbul ignore next */ comment is placed after the condition. For accurate test coverage reporting, ensure it's correctly positioned.
Please verify that the comment is ignoring the intended line. The comment should precede the line you wish to exclude from coverage.
-if (!fetchMoreResult) /* istanbul ignore next */ return prevResult;
+/* istanbul ignore next */
if (!fetchMoreResult) return prevResult;Would you like assistance in adjusting the placement to ensure proper coverage reporting?
153-160: Handle cases where the search input is empty
When assignedMemberSearchInput is empty, the firstName and lastName variables may be set to empty strings. Ensure that this behavior aligns with the intended functionality, considering that empty strings are used to match all entries.
64-71: Initialization of state variables for search and sorting
The addition of state variables for managing search inputs and sorting order is appropriate and enhances the component's functionality.
rishav-jha-mech
merged commit 7707708
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Adds filtering and sorting for userTag, refactors and addresses minor issues.
Issue Number:
Fixes #2395
Did you add tests for your changes?
Yes
Demo
filterAndSort.mp4
Additional Context
talawa-api issue: PalisadoesFoundation/talawa-api#2630
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation