Added Event Registrants Tab under Event Management Dashboard #2804
Conversation
WalkthroughThis pull request introduces comprehensive changes to the event registrant management functionality in the Talawa Admin portal. The modifications include adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![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
🧹 Nitpick comments (6)
src/components/CheckIn/CheckInWrapper.tsx (1)
31-37: Refine thealttext for accessibility.
Using"Sort"as thealttext in this context might be misleading since the button label is "Check In Registrants." Providing a more descriptive alt text improves accessibility.<img src="/images/svg/options-outline.svg" - width={30.63} - height={30.63} - alt="Sort" - className={styles.sortImg} /> + width={30.63} + height={30.63} + alt="Check in icon" + className={styles.sortImg} +/>src/components/EventRegistrantsModal/EventRegistrantsWrapper.tsx (2)
27-33: InvokeonUpdateafter state changes.
CallingonUpdate()after closing the modal ensures external components can refresh or handle post-close logic. This approach is correct.Consider adding error handling or a safety check around
onUpdatein case the callback logic fails or is asynchronous.
39-40: Testing attribute for consistent naming.
Usingdata-testid="filter-button"is fine, but the naming might be more descriptive, for instancedata-testid="add-registrants-button". This makes tests more explicit.<Button - data-testid="filter-button" + data-testid="add-registrants-button" className={`border-1 mx-4 ${style.createButton}`} ... >src/components/EventManagement/EventRegistrant/EventRegistrants.test.tsx (1)
52-65: Leverage built-in async utilities or set up wait helpers for better reliability.Your approach ensures that table headers appear after the queries resolve. This is correct, but consider using React Testing Library’s recommended patterns (
waitFor) extensively to reduce potential timing-related flakiness.src/components/EventManagement/EventRegistrant/EventRegistrants.tsx (2)
63-71: Verify concurrency of queries.Using
useLazyQueryfor bothgetEventRegistrantsandgetEventAttendeesis fine, but carefully ensure that both queries resolve in tandem. Consider error handling logic for each query to maintain a robust user experience.
210-216: Provide feedback or user confirmation after adding a registrant.You refresh the data upon
onUpdate, which is good. Consider offering user-friendly feedback, such as a toast notification, acknowledging the successful addition of a new registrant. This will enhance the user experience.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/images/svg/options-outline.svgis excluded by!**/*.svgpublic/images/svg/organization.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
package.json(1 hunks)public/locales/en/translation.json(1 hunks)src/GraphQl/Queries/Queries.ts(1 hunks)src/components/CheckIn/CheckInWrapper.tsx(2 hunks)src/components/CheckIn/tagTemplate.ts(2 hunks)src/components/EventManagement/EventRegistrant/EventRegistrants.module.css(1 hunks)src/components/EventManagement/EventRegistrant/EventRegistrants.test.tsx(1 hunks)src/components/EventManagement/EventRegistrant/EventRegistrants.tsx(1 hunks)src/components/EventManagement/EventRegistrant/Registrations.mocks.ts(1 hunks)src/components/EventRegistrantsModal/EventRegistrantsWrapper.test.tsx(1 hunks)src/components/EventRegistrantsModal/EventRegistrantsWrapper.tsx(2 hunks)src/screens/EventManagement/EventManagement.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/EventManagement/EventRegistrant/EventRegistrants.module.css
🔇 Additional comments (17)
src/components/CheckIn/CheckInWrapper.tsx (2)
5-5: Style import looks good.
The newly imported style object from app.module.css aligns well with the existing approach of modular CSS usage.
24-25: Testing attribute and combined styling.
Adding data-testid="stats-modal" is helpful for test reliability. The combined classes from local styles and app.module.css is consistent with the rest of the codebase.
src/components/EventRegistrantsModal/EventRegistrantsWrapper.tsx (5)
4-4: Consistent styling import.
Importing style from the global module is a good step for unified styling across components.
10-10: Optional callback prop is a good pattern.
Introducing onUpdate as an optional callback offers flexibility to handle updates after the modal is closed.
23-23: Destructure onUpdate properly.
Destructuring the new onUpdate prop is clear and consistent with the existing pattern.
46-46: Button label change is appropriate.
Renaming "Show Registrants" to "Add Registrants" clarifies the immediate action.
53-53: Explicitly referencing handleClose.
Passing handleClose directly is concise and ensures a clear flow of control when the modal closes.
src/components/EventManagement/EventRegistrant/Registrations.mocks.ts (1)
1-67: Well-structured mock data.
The mock objects for successful and error responses are well-defined, covering normal and failing scenarios for EVENT_REGISTRANTS and EVENT_ATTENDEES. This is a good practice to ensure robust testing coverage.
src/components/EventRegistrantsModal/EventRegistrantsWrapper.test.tsx (1)
80-80: Updated test reference is correct.
Changing the button label to "Add Registrants" aligns with the updated UI text and ensures the test remains accurate.
src/components/EventManagement/EventRegistrant/EventRegistrants.test.tsx (2)
1-13: Use consistent imports and remove unused ones if any.
All looks good regarding imports, especially React, Apollo, and react-testing-library. Just be cautious of any libraries or types that might not be used in the final implementation.
39-45: Avoid overshadowing or mocking react-router-dom incorrectly.
The jest.mock('react-router-dom') pattern is acceptable, but ensure it doesn't conflict with other tests or lead to unpredictable behavior if your test suite grows. Consider a specialized mock file for useParams if needed.
src/screens/EventManagement/EventManagement.tsx (1)
235-237: Great integration of the new EventRegistrants tab.
You are appropriately rendering <EventRegistrants /> under the registrants tab. Ensure that navigation to this tab is tested in a higher-level integration test so that the overall user flow is validated.
src/components/EventManagement/EventRegistrant/EventRegistrants.tsx (2)
45-60: Safeguard your type checks and handle potential undefined data.
The onCompleted callback sets registrants only if data?.getEventAttendeesByEventId is defined. Ensure that null or empty edge cases (e.g., no event attendees) are gracefully handled.
81-104: Check how you handle missing or partial attendee data.
Merging logic that depends on createdAt could break if a partial match or incomplete data is encountered. Consider a fallback for matchedAttendee to ensure no unhandled exceptions.
src/GraphQl/Queries/Queries.ts (1)
331-339: EVENT_REGISTRANTS query is well-defined.
The query is structured properly to retrieve minimal fields (userId, isRegistered, _id). Confirm if any additional fields are needed, such as user creation date, to avoid multiple queries.
public/locales/en/translation.json (1)
775-785: LGTM! Translation keys are well-structured
The added eventRegistrant translations are clear, concise and follow the established naming conventions. All necessary UI text elements for the event registrants feature are covered.
src/components/CheckIn/tagTemplate.ts (1)
Line range hint 5-19: LGTM! Schema structure simplified while maintaining functionality
The schema restructuring reduces unnecessary nesting while preserving the same name field configuration. The PDF template settings look correct.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2804 +/- ##
=====================================================
+ Coverage 59.89% 87.92% +28.02%
=====================================================
Files 296 315 +19
Lines 7373 8264 +891
Branches 1610 1866 +256
=====================================================
+ Hits 4416 7266 +2850
+ Misses 2711 788 -1923
+ Partials 246 210 -36 ☔ View full report in Codecov by Sentry. |
|
Should I raise an issue regarding the test coverage of this component as it would also require to be migrated into vitest. |
|
Please fix the test coverage in this PR |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx (3)
1-16: Use consistent import statements and group them logically.
The imports are comprehensive but mixing test utilities and library imports in a single block can reduce readability. Consider grouping them logically (e.g., React & library imports, custom utilities, mocks).
27-39: Isolate MockedProvider usage for repeated tests.
TherenderEventRegistrantsfunction is well-structured; however, if multiple tests use the same setup, consider extracting shared code to a test utility function to reduce repetition and potential maintenance overhead.
41-57: Mocking react-router-dom for each test could be optimized.
Re-mockingreact-router-dominside eachdescribeor test may lead to overhead if used frequently across test files. IfuseParamsis consistently returning the same values, consider a centralized mock approach for better maintainability.public/locales/sp/translation.json (1)
1087-1098: LGTM! The Spanish translations for event registrants look good.The translations are grammatically correct and use appropriate Spanish terminology for the event registration context. The section is well-structured and provides all necessary translations for the feature.
Consider adding a period at the end of "No se encontraron registrados" to match the punctuation style used in other similar messages in the file, like "No se encontraron voluntarios!" on line 1087:
- "noRegistrantsFound": "No se encontraron registrados" + "noRegistrantsFound": "No se encontraron registrados."
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx(1 hunks)
🔇 Additional comments (7)
src/components/EventManagement/EventRegistrant/EventRegistrants.spec.tsx (4)
17-25: Validate unused helper function usage or rename it.
The wait function is an extra layer around waitFor but does not appear to be used beyond line 21. Decide if this helper is necessary to maintain readability or remove it to keep the test suite lean.
58-126: Ensure coverage for success/error handling.
The test scenarios appear comprehensive for table headers, empty scenario, and button presence, but confirm that potential error states (e.g., failed network requests) are also covered or will be covered in future tests.
128-197: Thorough scenario coverage for data merging.
The test for combining registrant and attendee data is nicely handled. Please ensure special cases, like partial merges or repeated user entries, are also tested to prevent edge-case regressions.
198-264: Fallback logic testing is well-structured.
The test validates missing attendee data rendering. This robust approach ensures consistent UX. Keeping the fallback logic minimal and clear is advisable to avoid confusion when scaling up the component features.
public/locales/zh/translation.json (1)
1085-1095: Translations align with established naming.
The newly added eventRegistrant keys mirror the structure used elsewhere, promoting consistency. Double-check for typos or missing context in these newly added translations for "注册者" or "未找到注册者".
public/locales/hi/translation.json (1)
1085-1095: Maintain continuous localization parity.
These Hindi translations for eventRegistrant appear correct. Ensure that similar keys have no mismatch in other locales, preserving the same terminology (“पंजीकृत व्यक्ति”) across the UI.
public/locales/fr/translation.json (1)
1085-1095: Check for accent consistency and spacing.
The French strings for eventRegistrant, such as “Enregistré le”, match the format used throughout the file. Just verify accents and punctuation in the UI for complete fluidity.
|
@palisadoes increased the test coverage. |
|
@palisadoes, I am already using all the CSS properties from that file only. The CSS file for this component is only present if in case an individual file is required in the future. |
|
In that case please delete all the CSS files in the directories where the files you have edited reside. Make sure there are no references to the local CSS files |
palisadoes
added
the
ignore-sensitive-files-pr
palisadoes
merged commit 3209a18
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #1750
Did you add tests for your changes?
Yes
Snapshots/Videos:
1734901891254134.mp4
If relevant, did you update the documentation?
No
Summary
Added the event registrants tab where admin can check in and add registrants of an event.
Does this PR introduce a breaking change?
No
Other information
Added a new package : @pdfme/common and changed its template schema according to the package. (This might give a formatting/linting error).
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores