Add Cypress Test Suite for Facility Notice Board Functionality Verification #9045
Conversation
WalkthroughThe pull request introduces enhancements to the Cypress end-to-end test suite for the facility homepage, specifically by adding a new test case for verifying notice board functionality. It incorporates new methods in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
cypress/pageobject/Login/LoginPage.ts (1)
38-40: Add JSDoc documentation for better maintainability.The implementation looks good and aligns well with the existing code patterns. Consider adding JSDoc documentation to describe the method's purpose and behavior.
+ /** + * Clicks the sign-out button after verifying its presence + * @returns {void} + */ clickSignOutBtn(): void { cy.verifyAndClickElement("#sign-out-button", "Sign Out"); }cypress/pageobject/Facility/FacilityNotify.ts (2)
2-4: Consider using data-testid for more reliable element selection.The
#NotifyModalMessageInputselector relies on an ID that might change. Using a dedicated data-testid attribute would make the test more maintainable.- cy.get("#NotifyModalMessageInput").should("be.visible").type(message); + cy.get('[data-testid="notify-modal-message-input"]').should("be.visible").type(message);
1-36: Add JSDoc documentation and improve class structure.The class would benefit from proper documentation and a more organized structure.
+/** + * Page Object Model for facility notification functionality. + * Handles UI interactions and API verifications for the notification system. + */ export class FacilityNotify { + // Add private constants + private readonly SELECTORS = { + NOTIFY_INPUT: '[data-testid="notify-modal-message-input"]', + NOTICE_BOARD_LINK: '[data-testid="notice-board-link"]', + NOTIFICATION_BTN: '[data-testid="notification-slide-btn"]' + } as const; + + // Group methods by functionality + // UI Interactions + // API Interactions }src/components/Common/Sidebar/SidebarItem.tsx (1)
35-35: Simplify id prop access.The optional chaining operator (
?.) is unnecessary here since TypeScript already handles the optional type. You can directly useprops.id.- id={props?.id} + id={props.id}cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
147-149: Move selectors to POM class.The selectors like
#notify-facility-nameand.error-textshould be moved to theFacilityNotifyPOM class for better maintainability.
160-162: Consider extracting login flow to a shared command.The login sequence appears in multiple places. Consider creating a custom Cypress command for this common operation.
// commands.ts Cypress.Commands.add('switchUser', (role: string) => { cy.get('@loginPage').then((loginPage) => { loginPage.ensureLoggedIn(); loginPage.clickSignOutBtn(); if (role === 'nurse') { loginPage.loginManuallyAsNurse(); } // Add other roles as needed }); }); // Usage in test cy.switchUser('nurse');src/components/Facility/FacilityCard.tsx (2)
Line range hint
41-60: Consider enhancing notification handling.The notification submission logic could benefit from the following improvements:
- Add a loading state during API call to prevent multiple submissions
- Implement character limit validation
- Consider rate limiting to prevent notification spam
Here's a suggested implementation:
const [notifyMessage, setNotifyMessage] = useState(""); const [notifyError, setNotifyError] = useState(""); +const [isSubmitting, setIsSubmitting] = useState(false); +const MAX_MESSAGE_LENGTH = 500; const handleNotifySubmit = async (id: any) => { - if (notifyMessage.trim().length >= 1) { + if (isSubmitting) return; + + if (notifyMessage.trim().length === 0) { + setNotifyError("Message cannot be empty"); + return; + } + + if (notifyMessage.length > MAX_MESSAGE_LENGTH) { + setNotifyError(`Message cannot exceed ${MAX_MESSAGE_LENGTH} characters`); + return; + } + + try { + setIsSubmitting(true); setNotifyError(""); const { res } = await request(routes.sendNotificationMessages, { body: { facility: id, message: notifyMessage, }, }); if (res?.ok) { Notification.Success({ msg: "Facility Notified", }); setNotifyMessage(""); setNotifyModalFor(undefined); } else { Notification.Error({ msg: "Something went wrong..." }); } - } else { - setNotifyError("Message cannot be empty"); + } finally { + setIsSubmitting(false); } };
TextAreaFormField onChange handler is incompatible with component implementation
The component implementation shows that
TextAreaFormFieldinternally usese.target.valuein its onChange handler, but the usage in FacilityCard.tsx usese.value. This will cause the text area to not update properly.The correct usage should be:
onChange={(e) => setNotifyMessage(e.target.value)}🔗 Analysis chain
Line range hint
188-224: Verify TextAreaFormField onChange handler compatibility.The
onChangehandler now usese.valueinstead of the standarde.target.value. This suggests a change in theTextAreaFormFieldcomponent's event handling.Let's verify the TextAreaFormField component's interface:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TextAreaFormField component definition and usage echo "Searching for TextAreaFormField component definition..." ast-grep --pattern 'export $_ TextAreaFormField = $_' echo -e "\nSearching for other TextAreaFormField onChange handlers..." rg "TextAreaFormField.*onChange" --type typescript --type typescriptreactLength of output: 420
Script:
#!/bin/bash # Let's try different approaches to find the component and its usage # Find files containing TextAreaFormField echo "Files containing TextAreaFormField:" rg -l "TextAreaFormField" # Search for onChange handlers in those files echo -e "\nTextAreaFormField onChange usage:" rg -B2 -A2 "TextAreaFormField.*onChange" # Look for the component definition echo -e "\nTextAreaFormField component definition:" rg -B5 -A10 "export.*TextAreaFormField"Length of output: 3602
Script:
#!/bin/bash # Let's look at the TextAreaFormField component implementation echo "TextAreaFormField component implementation:" cat src/components/Form/FormFields/TextAreaFormField.tsx # Also search for its actual usage patterns echo -e "\nTextAreaFormField usage with onChange:" rg -B2 -A2 "<TextAreaFormField.*onChange" --type tsxLength of output: 1838
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityNotify.ts(1 hunks)cypress/pageobject/Login/LoginPage.ts(1 hunks)src/components/Common/Sidebar/SidebarItem.tsx(2 hunks)src/components/Facility/FacilityCard.tsx(1 hunks)src/components/Notifications/NoticeBoard.tsx(1 hunks)src/components/Notifications/NotificationsList.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Notifications/NoticeBoard.tsx
🧰 Additional context used
🪛 GitHub Check: cypress-run (4)
src/components/Notifications/NotificationsList.tsx
[failure] 479-479:
Type '{ text: string; id: string; do: () => void; icon: Element; badgeCount: number; handleOverflow: any; }' is not assignable to type 'IntrinsicAttributes & ((Omit<{ ref?: Ref | undefined; text: string; icon: ReactNode; onItemClick?: (() => void) | undefined; external?: true | undefined; badgeCount?: number | undefined; selected?: boolean | undefined; handleOverflow?: any; } & { ...; }, "ref"> | Omit<...>) & RefAttributes<...>)'.
🪛 GitHub Check: cypress-run (3)
src/components/Notifications/NotificationsList.tsx
[failure] 479-479:
Type '{ text: string; id: string; do: () => void; icon: Element; badgeCount: number; handleOverflow: any; }' is not assignable to type 'IntrinsicAttributes & ((Omit<{ ref?: Ref | undefined; text: string; icon: ReactNode; onItemClick?: (() => void) | undefined; external?: true | undefined; badgeCount?: number | undefined; selected?: boolean | undefined; handleOverflow?: any; } & { ...; }, "ref"> | Omit<...>) & RefAttributes<...>)'.
🪛 GitHub Check: cypress-run (2)
src/components/Notifications/NotificationsList.tsx
[failure] 479-479:
Type '{ text: string; id: string; do: () => void; icon: Element; badgeCount: number; handleOverflow: any; }' is not assignable to type 'IntrinsicAttributes & ((Omit<{ ref?: Ref | undefined; text: string; icon: ReactNode; onItemClick?: (() => void) | undefined; external?: true | undefined; badgeCount?: number | undefined; selected?: boolean | undefined; handleOverflow?: any; } & { ...; }, "ref"> | Omit<...>) & RefAttributes<...>)'.
🪛 GitHub Check: cypress-run (1)
src/components/Notifications/NotificationsList.tsx
[failure] 479-479:
Type '{ text: string; id: string; do: () => void; icon: Element; badgeCount: number; handleOverflow: any; }' is not assignable to type 'IntrinsicAttributes & ((Omit<{ ref?: Ref | undefined; text: string; icon: ReactNode; onItemClick?: (() => void) | undefined; external?: true | undefined; badgeCount?: number | undefined; selected?: boolean | undefined; handleOverflow?: any; } & { ...; }, "ref"> | Omit<...>) & RefAttributes<...>)'.
🔇 Additional comments (6)
cypress/pageobject/Facility/FacilityNotify.ts (1)
1-36: Verify integration with existing test commands.
The PR objectives mention using reusable functions from commands.ts. Let's verify this integration.
src/components/Common/Sidebar/SidebarItem.tsx (2)
24-24: LGTM! Type definition properly supports testing requirements.
The addition of the optional id prop enhances component testability while maintaining backward compatibility.
Line range hint 43-47: Verify resetHistory behavior in notification flow.
There's a comment suggesting uncertainty about resetHistory. Since this component is used in the notification system, we should verify that resetting history doesn't interfere with the notification flow, especially for the Facility Notice Board functionality.
✅ Verification successful
resetHistory usage is correct and safe for notifications
The resetHistory implementation is working as intended:
- It's properly imported from
useAppHistoryhook - It's used correctly in the click handler sequence
- The implementation in
HistoryAPIProvidershows it only resets to the first history entry - The notification flow in
NotificationsListusesSidebarItemas a navigation component, where resetting history on click is the expected behavior
The comment can be safely removed as the implementation has been verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of resetHistory to understand its impact
echo "Searching for resetHistory usage patterns..."
rg -A 3 "resetHistory" --type ts --type tsx
# Search for notification-related components using this sidebar item
echo "Checking notification-related component usage..."
rg -A 5 "SidebarItem|ShrinkedSidebarItem" --type ts --type tsx | rg -i "notif|notice"
Length of output: 393
Script:
#!/bin/bash
# Search for resetHistory without file type restrictions
echo "Searching for resetHistory usage patterns..."
rg -A 3 "resetHistory"
# Search for SidebarItem usage in notification contexts
echo "Checking notification-related component usage..."
rg -A 5 "SidebarItem" | rg -i "notif|notice"
# Check the actual implementation in SidebarItem.tsx
echo "Checking SidebarItem implementation..."
cat src/components/Common/Sidebar/SidebarItem.tsx
Length of output: 6690
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
2-3: LGTM: Import and initialization changes follow POM pattern.
The new imports and instance initialization for FacilityNotify are properly structured following the Page Object Model pattern.
Also applies to: 5-6, 13-13
140-142: LGTM: Proper API verification pattern.
Good use of the intercept-action-verify pattern for API calls, ensuring reliable test execution.
Also applies to: 155-158, 164-166, 168-170
src/components/Facility/FacilityCard.tsx (1)
188-191: LGTM! Good addition of test id.
The addition of id="notify-facility-name" improves test stability and component accessibility.
| visitNoticeBoard(): void { | ||
| cy.get("a[href='/notice_board']").should("be.visible").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and error handling for link interaction.
The notice board link interaction could be more robust with proper timeout and error handling.
- cy.get("a[href='/notice_board']").should("be.visible").click();
+ cy.get('[data-testid="notice-board-link"]', { timeout: 10000 })
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();Committable suggestion skipped: line range outside the PR's diff.
| interceptPostNotificationReq(): void { | ||
| cy.intercept("POST", "**/api/v1/notification/notify").as("notifyFacility"); | ||
| } | ||
|
|
||
| verifyPostNotificationReq(): void { | ||
| cy.wait("@notifyFacility").its("response.statusCode").should("eq", 204); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for notification POST requests.
The notification POST verification should handle error cases and validate request payload.
interceptPostNotificationReq(): void {
- cy.intercept("POST", "**/api/v1/notification/notify").as("notifyFacility");
+ cy.intercept("POST", this.API_PATHS.NOTIFICATION + '/notify').as("notifyFacility");
}
verifyPostNotificationReq(): void {
- cy.wait("@notifyFacility").its("response.statusCode").should("eq", 204);
+ cy.wait("@notifyFacility").then((interception) => {
+ // Verify request payload
+ expect(interception.request.body).to.have.property('message');
+ // Verify response
+ expect(interception.response?.statusCode).to.equal(204);
+ // Handle potential error responses
+ if (interception.response?.statusCode !== 204) {
+ throw new Error(`Notification failed: ${interception.response?.body}`);
+ }
+ });
}Committable suggestion skipped: line range outside the PR's diff.
| interceptFacilitySearchReq(): void { | ||
| cy.intercept("GET", "**/api/v1/facility/**").as("searchFacility"); | ||
| } | ||
| verifyFacilitySearchReq(): void { | ||
| cy.wait("@searchFacility").its("response.statusCode").should("eq", 200); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve API interception configuration and type safety.
The facility search API interception could be more specific and type-safe.
+ private readonly API_PATHS = {
+ FACILITY_SEARCH: '**/api/v1/facility',
+ NOTIFICATION: '**/api/v1/notification'
+ } as const;
+
+ interface FacilitySearchResponse {
+ // Add expected response type definition
+ data: Array<{ id: string; name: string }>;
+ }
+
interceptFacilitySearchReq(): void {
- cy.intercept("GET", "**/api/v1/facility/**").as("searchFacility");
+ cy.intercept("GET", this.API_PATHS.FACILITY_SEARCH + '/**').as("searchFacility");
}
verifyFacilitySearchReq(): void {
- cy.wait("@searchFacility").its("response.statusCode").should("eq", 200);
+ cy.wait("@searchFacility").then((interception) => {
+ expect(interception.response?.statusCode).to.equal(200);
+ expect(interception.response?.body).to.have.property('data');
+ });
}Committable suggestion skipped: line range outside the PR's diff.
| it("Verify Notice Board Functionality", () => { | ||
| // search facility and verify it's loaded or not | ||
| facilityNotify.interceptFacilitySearchReq(); | ||
| manageUserPage.typeFacilitySearch(facilityName); | ||
| facilityNotify.verifyFacilitySearchReq(); | ||
| // verify facility name and notify button and click it | ||
| manageUserPage.assertFacilityInCard(facilityName); | ||
| facilityHome.clickFacilityNotifyButton(); | ||
| // check visiblity of pop-up and frontend error on empty message | ||
| cy.verifyContentPresence("#notify-facility-name", [facilityName]); | ||
| cy.submitButton("Notify"); | ||
| cy.verifyContentPresence(".error-text", [notificationErrorMsg]); | ||
| // close pop-up and verify | ||
| facilityHome.verifyAndCloseNotifyModal(); | ||
| // send notification | ||
| facilityHome.clickFacilityNotifyButton(); | ||
| facilityNotify.fillNotifyText(noitificationMessage); | ||
| facilityNotify.interceptPostNotificationReq(); | ||
| cy.submitButton("Notify"); | ||
| cy.verifyNotification("Facility Notified"); | ||
| facilityNotify.verifyPostNotificationReq(); | ||
| // signout as district admin and login as a Nurse | ||
| loginPage.ensureLoggedIn(); | ||
| loginPage.clickSignOutBtn(); | ||
| loginPage.loginManuallyAsNurse(); | ||
| // Visit Notification Sidebar | ||
| facilityNotify.interceptGetNotificationReq(); | ||
| facilityNotify.visitNoticeBoard(); | ||
| facilityNotify.verifyGetNotificationReq(); | ||
| cy.verifyContentPresence("#notification-message", [noitificationMessage]); | ||
| facilityNotify.interceptGetNotificationReq(); | ||
| cy.verifyAndClickElement("#notification-slide-btn", "Notifications"); | ||
| facilityNotify.verifyGetNotificationReq(); | ||
| cy.verifyContentPresence("#notification-slide-msg", [noitificationMessage]); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding negative test cases and cleanup.
The test covers the happy path well but could be enhanced with:
- Negative test cases:
- Network failure scenarios
- Server error responses
- Invalid message formats
- Test data cleanup after test execution
- Explicit assertions for state changes
Would you like me to provide example code for these scenarios?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
138-172: Enhance test robustness with additional scenarios.Consider adding the following test cases:
- Network failure scenarios:
it("handles network failure when sending notification", () => { // ... setup test facilityNotify.interceptPostNotificationReq().networkError(); cy.submitButton("Notify"); cy.verifyNotification("Failed to send notification"); });
- API error responses:
it("handles server error when sending notification", () => { // ... setup test facilityNotify.interceptPostNotificationReq().reply(500); cy.submitButton("Notify"); cy.verifyNotification("Server error occurred"); });
- Consider extracting repeated steps into custom commands:
// commands.ts Cypress.Commands.add('sendNotification', (facility, message) => { // ... reusable notification steps });Would you like me to provide more detailed examples for these scenarios?
146-149: Extract UI text selectors and messages as constants.Consider extracting hardcoded selectors and messages into constants for better maintainability:
const SELECTORS = { facilityName: '#notify-facility-name', errorText: '.error-text', submitButton: 'Notify', successMessage: 'Facility Notified' };Also applies to: 156-157
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityNotify.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/pageobject/Facility/FacilityNotify.ts
🔇 Additional comments (2)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
2-6: LGTM! Imports and instance creation follow POM pattern.
The new imports and instance creation for FacilityNotify align well with the existing Page Object Model structure.
Also applies to: 13-13
138-172: LGTM! Well-structured test implementation.
The test case effectively covers the notice board workflow with proper API verifications and UI assertions.
| const notificationErrorMsg = "Message cannot be empty"; | ||
| const noitificationMessage = | ||
| "Reminder: The monthly report submission deadline is on 15th Nov. Ensure all entries are updated."; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move notification messages to i18n configuration.
As per PR objectives, UI text should support internationalization. Consider moving these message constants to the i18n configuration:
notificationErrorMsgnoitificationMessage
Example structure:
// i18n/en/notification.json
{
"errors": {
"emptyMessage": "Message cannot be empty"
},
"templates": {
"monthlyReport": "Reminder: The monthly report submission deadline is on 15th Nov. Ensure all entries are updated."
}
}
|
@JavidSumra your test is failing on actions |
|
Yup but it's passing on the local environment can you please provide images of that if possible @nihal467 |
|
|
|
Ok sorry I'll check it again. |
|
Hey @nihal467 as you can see below all test cases are passing in my local environment can you please guide me what should I do next? |
Proposed Changes
FacilityNotify.tsin the pageobject folder following the POM structure.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
FacilityNotifyclass to manage notifications, including methods for sending and verifying notifications.Enhancements
idattributes to various components, including notification messages and sidebar items.FacilityCardcomponent for better data management.Bug Fixes