Add Cypress Test Suite for Facility Notice Board Functionality Verification #9045
Conversation
WalkthroughThe changes introduced in this pull request enhance the Cypress end-to-end testing suite for the facility homepage, focusing on the functionality of the notice board. A new test case for verifying notice board operations is added, along with a new class Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running the following tools: 🔧 eslintcypress/e2e/facility_spec/FacilityHomepage.cy.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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.
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.
|
@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? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (5)
28-29: Fix typo in constant name.The constant name has a typo:
noitificationMessageshould benotificationMessage.- const noitificationMessage = + const notificationMessage =
138-138: Add test documentation.Consider adding a descriptive comment block to document the test's purpose, prerequisites, and workflow steps.
+ /** + * Test case to verify the facility notice board functionality: + * 1. Search and verify facility + * 2. Test empty message validation + * 3. Send notification + * 4. Verify notification as a Nurse + * + * Prerequisites: + * - District admin and nurse users exist + * - Facility "Dummy Facility 40" exists + */ it("Verify Notice Board Functionality", () => {
145-147: Remove redundant logging.The
cy.logstatements around clicking the notify button are redundant as Cypress already logs command execution.- cy.log("Clicking facility notify button"); facilityHome.clickFacilityNotifyButton(); - cy.log("Clicked facility notify button");
149-151: Make assertions more specific.The content verification could be more specific about what's being tested.
- cy.verifyContentPresence("#notify-facility-name", [facilityName]); - cy.submitButton("Notify"); - cy.verifyContentPresence(".error-text", [notificationErrorMsg]); + cy.get("#notify-facility-name") + .should("be.visible") + .and("contain.text", facilityName); + cy.submitButton("Notify") + .should("not.be.disabled"); + cy.get(".error-text") + .should("be.visible") + .and("have.text", notificationErrorMsg);
163-166: Add cleanup after user switching.Consider adding cleanup steps after switching users to ensure test isolation.
loginPage.ensureLoggedIn(); loginPage.clickSignOutBtn(); loginPage.loginManuallyAsNurse(); + cy.task('db:cleanup', { user: 'district-admin' }); // Add if cleanup task is available // Visit Notification Sidebar
📜 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/FacilityHome.ts(1 hunks)
🔇 Additional comments (1)
cypress/pageobject/Facility/FacilityHome.ts (1)
38-38: 🛠️ Refactor suggestion
Consider investigating the need for such a long timeout.
The 20-second timeout seems excessive and might be masking underlying issues. This could be related to the test failures reported in the PR comments.
Consider these alternatives:
- Investigate why the element takes so long to appear
- Use Cypress's built-in retry-ability with explicit assertions:
-cy.get("#facility-notify", { timeout: 20000 }).first().click();
+cy.get("#facility-notify")
+ .should("exist")
+ .and("be.visible")
+ .first()
+ .click();Let's verify the element's presence in other tests:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
153-153: Remove commented out code.Remove the commented line as it's redundant with line 154.
- // facilityHome.clickFacilityNotifyButton();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityHome.ts(1 hunks)cypress/pageobject/Facility/FacilityNotify.ts(1 hunks)src/components/Facility/FacilityCard.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/pageobject/Facility/FacilityHome.ts
- src/components/Facility/FacilityCard.tsx
🔇 Additional comments (5)
cypress/pageobject/Facility/FacilityNotify.ts (2)
22-43: 🛠️ Refactor suggestion
Improve API interception with proper typing and validation.
The API interception methods need better organization, typing, and validation:
- Centralize API paths
- Add TypeScript interfaces for request/response payloads
- Add proper error handling
- Validate response payloads
+interface NotificationPayload {
+ message: string;
+ facilityIds?: string[];
+}
+
+interface FacilitySearchResponse {
+ data: Array<{
+ id: string;
+ name: string;
+ // Add other facility properties
+ }>;
+}
+
+private readonly API_PATHS = {
+ FACILITY: '**/api/v1/facility',
+ NOTIFICATION: '**/api/v1/notification'
+} as const;
interceptFacilitySearchReq(): void {
- cy.intercept("GET", "**/api/v1/facility/**").as("searchFacility");
+ cy.intercept("GET", `${this.API_PATHS.FACILITY}/**`).as("searchFacility");
}
verifyFacilitySearchReq(): void {
- cy.wait("@searchFacility").its("response.statusCode").should("eq", 200);
+ cy.wait("@searchFacility").then((interception) => {
+ const response = interception.response;
+ expect(response?.statusCode).to.equal(200);
+
+ // Validate response body
+ const body = response?.body as FacilitySearchResponse;
+ expect(body).to.have.property('data');
+ expect(body.data).to.be.an('array');
+
+ // Validate each facility in the response
+ body.data.forEach(facility => {
+ expect(facility).to.have.property('id');
+ expect(facility).to.have.property('name');
+ });
+ });
}
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) => {
+ // Validate request payload
+ const request = interception.request;
+ const payload = request.body as NotificationPayload;
+ expect(payload).to.have.property('message');
+
+ // Validate response
+ const response = interception.response;
+ if (response?.statusCode !== 204) {
+ throw new Error(`Notification failed: ${JSON.stringify(response?.body)}`);
+ }
+ });
}
interceptGetNotificationReq(): void {
- cy.intercept("GET", "**/api/v1/notification/**").as("getNotifications");
+ cy.intercept("GET", `${this.API_PATHS.NOTIFICATION}/**`).as("getNotifications");
}
verifyGetNotificationReq(): void {
- cy.wait("@getNotifications").its("response.statusCode").should("eq", 200);
+ cy.wait("@getNotifications").then((interception) => {
+ const response = interception.response;
+ expect(response?.statusCode).to.equal(200);
+
+ // Add response payload validation based on your API contract
+ const body = response?.body;
+ expect(body).to.have.property('data');
+ });
}#!/bin/bash
# Description: Verify API endpoints and response structures
# Search for API endpoint definitions
echo "Checking API endpoint definitions..."
rg -A 5 "api/v1/(facility|notification)" --type ts --type tsx
# Search for response type definitions
echo "Checking response type definitions..."
rg -A 10 "interface.*Response" --type ts2-16: 🛠️ Refactor suggestion
Enhance test reliability with data-testid selectors and proper error handling.
The UI interaction methods should use data-testid attributes instead of IDs/href for better maintainability. Also, add proper timeout and error handling.
fillNotifyText(message: string): void {
- cy.get("#NotifyModalMessageInput").should("be.visible").type(message);
+ cy.get('[data-testid="notify-modal-input"]', { timeout: 10000 })
+ .should("be.visible")
+ .should("be.enabled")
+ .type(message, { force: true });
}
openNotificationSlide(): void {
- cy.get("#notification-slide-btn").should("be.visible").click();
+ cy.get('[data-testid="notification-slide-btn"]', { timeout: 10000 })
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();
}
clickNotifyButton(): void {
- cy.get("#facility-notify-btn").should("be.visible").click();
+ cy.get('[data-testid="facility-notify-btn"]', { timeout: 10000 })
+ .should("be.visible")
+ .should("not.be.disabled")
+ .click();
}
visitNoticeBoard(): void {
- 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();
}cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
2-6: LGTM: Import and setup changes are well-structured.
The new import and instance creation for FacilityNotify follow the established patterns in the test suite.
Also applies to: 13-13
138-173: Improve test structure and add error scenarios.
While the test covers the happy path well, consider these improvements:
- Group related assertions using
withinfor better readability:
cy.get('#notification-section').within(() => {
cy.verifyContentPresence("#notification-message", [notificationMessage]);
cy.verifyContentPresence("#notification-slide-msg", [notificationMessage]);
});- Add test cleanup to reset notification state after test completion
27-29:
Fix typo and move notification messages to i18n.
Issues found:
- Typo in constant name:
noitificationMessageshould benotificationMessage - UI text should be moved to i18n configuration as per PR objectives
Apply this diff to fix the typo:
- const noitificationMessage =
+ const notificationMessage =
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
145-145: Remove commented code.The following lines should be removed as they are commented out and appear to be redundant:
- // facilityHome.clickFacilityNotifyButton(); - // facilityHome.clickFacilityNotifyButton();Also applies to: 154-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityNotify.ts(1 hunks)src/components/Facility/FacilityCard.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/pageobject/Facility/FacilityNotify.ts
- src/components/Facility/FacilityCard.tsx
🔇 Additional comments (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
2-6: LGTM! Setup follows POM pattern.
The imports and instance creation follow the established Page Object Model pattern, promoting code reusability and maintainability.
Also applies to: 13-13
27-29: Past review comment about moving text to i18n configuration is still applicable.
138-174: Past review comment about adding negative test cases is still applicable.
|
Hey @nihal467, could you help me debug this issue? I'm new to this and would really appreciate your guidance. I've tried multiple solutions, but it's still not passing in GitHub CI. Thank you! |
…041/test-verify-notice-board
…idSumra/care_fe into issues/9041/test-verify-notice-board
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
cypress/pageobject/Facility/FacilityNotify.ts (1)
1-1: Enhance class structure with TypeScript features.Consider improving type safety and maintainability by:
- Adding interfaces for API responses
- Defining constants for API paths and selectors
- Grouping related methods (UI interactions, API interceptors, verifications)
+interface NotificationResponse { + message: string; + status: string; +} + +interface FacilitySearchResponse { + data: Array<{ id: string; name: string }>; +} + export class FacilityNotify { + private readonly SELECTORS = { + NOTIFY_INPUT: '#NotifyModalMessageInput', + NOTIFICATION_SLIDE: '#notification-slide-btn', + NOTICE_BOARD_LINK: "a[href='/notice_board']" + } as const; + + private readonly API_PATHS = { + FACILITY: '**/api/v1/facility', + NOTIFICATION: '**/api/v1/notification' + } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityHome.ts(1 hunks)cypress/pageobject/Facility/FacilityNotify.ts(1 hunks)
🔇 Additional comments (6)
cypress/pageobject/Facility/FacilityNotify.ts (3)
14-16:
Remove duplicate method.
The visitNotificationSideBar method is identical to openNotificationSlide. Remove this duplicate to maintain DRY principles.
- visitNotificationSideBar(): void {
- cy.get("#notification-slide-btn").should("be.visible").click();
- }Likely invalid or redundant comment.
18-23: 🛠️ Refactor suggestion
Improve API interception and response validation.
The facility search API methods need better type checking and error handling.
interceptFacilitySearchReq(): void {
- cy.intercept("GET", "**/api/v1/facility/**").as("searchFacility");
+ cy.intercept("GET", `${this.API_PATHS.FACILITY}/**`).as("searchFacility");
}
verifyFacilitySearchReq(): void {
- cy.wait("@searchFacility").its("response.statusCode").should("eq", 200);
+ cy.wait("@searchFacility").then((interception) => {
+ const response = interception.response;
+ expect(response?.statusCode).to.equal(200);
+
+ const body = response?.body as FacilitySearchResponse;
+ expect(body).to.have.property('data');
+ expect(Array.isArray(body.data)).to.be.true;
+ });
}Likely invalid or redundant comment.
25-31: 🛠️ Refactor suggestion
Add proper error handling for notification requests.
The notification API methods should handle errors and validate the response 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) => {
+ const response = interception.response;
+
+ // Verify request
+ expect(interception.request.body).to.have.property('message');
+
+ // Verify response
+ if (response?.statusCode !== 204) {
+ throw new Error(`Notification failed: ${JSON.stringify(response?.body)}`);
+ }
+ });
}Likely invalid or redundant comment.
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
2-6: LGTM: Imports and class instantiation are well-organized.
The new imports and class instantiation follow the established patterns in the codebase.
Also applies to: 13-13
138-174: Consider breaking down the test into smaller, focused test cases.
The current test case covers multiple scenarios in a single test. Consider splitting it into smaller, focused test cases:
- Verify empty message validation
- Verify successful notification sending
- Verify notification visibility for nurse
This would improve test maintainability and make failures easier to diagnose.
it("should show error for empty notification message", () => {
// Setup facility search
facilityNotify.interceptFacilitySearchReq();
manageUserPage.typeFacilitySearch(facilityName);
facilityNotify.verifyFacilitySearchReq();
// Verify empty message error
facilityHome.clickFacilityNotifyButton();
cy.verifyContentPresence("#notify-facility-name", [facilityName]);
cy.submitButton("Notify");
cy.verifyContentPresence(".error-text", [notificationErrorMsg]);
});
it("should successfully send notification", () => {
// Setup and send notification
facilityHome.clickFacilityNotifyButton();
facilityNotify.fillNotifyText(notificationMessage);
facilityNotify.interceptPostNotificationReq();
cy.submitButton("Notify");
// Verify notification sent
cy.verifyNotification("Facility Notified");
facilityNotify.verifyPostNotificationReq();
});
it("should display notification for nurse", () => {
// Login as nurse
loginPage.loginManuallyAsNurse();
// Verify notification in board
facilityNotify.interceptGetNotificationReq();
facilityNotify.visitNoticeBoard();
facilityNotify.verifyGetNotificationReq();
cy.verifyContentPresence("#notification-message", [notificationMessage]);
// Verify notification in slide
facilityNotify.interceptGetNotificationReq();
facilityNotify.openNotificationSlide();
facilityNotify.verifyGetNotificationReq();
cy.verifyContentPresence("#notification-slide-msg", [notificationMessage]);
});The previous review comments about adding error handling test cases and cleanup steps are still valid.
27-29:
Fix typo in variable name.
The variable name noitificationMessage contains a typo and should be notificationMessage.
The previous review comment about moving these messages to i18n configuration is still valid.
| interceptGetNotificationReq(): void { | ||
| cy.intercept("GET", "**/api/v1/notification/**").as("getNotifications"); | ||
| } | ||
|
|
||
| verifyGetNotificationReq(): void { | ||
| cy.wait("@getNotifications").its("response.statusCode").should("eq", 200); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance GET notification verification.
Add proper response validation for the get notifications endpoint.
interceptGetNotificationReq(): void {
- cy.intercept("GET", "**/api/v1/notification/**").as("getNotifications");
+ cy.intercept("GET", `${this.API_PATHS.NOTIFICATION}/**`).as("getNotifications");
}
verifyGetNotificationReq(): void {
- cy.wait("@getNotifications").its("response.statusCode").should("eq", 200);
+ cy.wait("@getNotifications").then((interception) => {
+ const response = interception.response;
+ expect(response?.statusCode).to.equal(200);
+
+ const body = response?.body as NotificationResponse;
+ expect(body).to.have.property('message');
+ expect(body).to.have.property('status');
+ });
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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)
🔇 Additional comments (3)
cypress/pageobject/Facility/FacilityNotify.ts (2)
18-20:
Remove duplicate method.
The visitNotificationSideBar method is identical to openNotificationSlide. Remove this duplicate to maintain DRY principles.
1-44: Address test reliability concerns.
The test failures reported in the PR comments might be related to timing and assertion issues. The improvements suggested above should help make the tests more reliable by:
- Adding appropriate timeouts for UI interactions
- Including additional assertions to verify element states
- Adding proper error handling for API responses
- Using constants for selectors and API paths
Please implement these changes and verify if they resolve the test failures in the reviewer's environment.
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
2-6: LGTM: Import and setup changes are well-structured.
The new imports and instance creation follow the established patterns in the codebase.
Also applies to: 13-13
| fillNotifyText(message: string): void { | ||
| cy.get("#NotifyModalMessageInput").should("be.visible").type(message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve UI interaction reliability.
Add proper error handling, timeouts, and assertions for UI interactions.
fillNotifyText(message: string): void {
- cy.get("#NotifyModalMessageInput").should("be.visible").type(message);
+ cy.get(this.SELECTORS.NOTIFY_INPUT, { timeout: 10000 })
+ .should('be.visible')
+ .should('be.enabled')
+ .clear()
+ .type(message, { delay: 100 })
+ .should('have.value', message);
}
openNotificationSlide(): void {
- cy.get("#notification-slide-btn").should("be.visible").click();
+ cy.get(this.SELECTORS.NOTIFICATION_SLIDE_BTN, { timeout: 10000 })
+ .should('be.visible')
+ .should('be.enabled')
+ .click()
+ .should('not.be.enabled');
}
closeNotificationSlide(): void {
- cy.get("#close-slide-over").should("be.visible").click();
+ cy.get(this.SELECTORS.CLOSE_SLIDE_BTN, { timeout: 10000 })
+ .should('be.visible')
+ .should('be.enabled')
+ .click()
+ .should('not.exist');
}
visitNoticeBoard(): void {
- cy.get("a[href='/notice_board']").should("be.visible").click();
+ cy.get(this.SELECTORS.NOTICE_BOARD_LINK, { timeout: 10000 })
+ .should('be.visible')
+ .should('not.be.disabled')
+ .click();
}Also applies to: 6-8, 10-12, 14-16
| 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
Enhance API interception and verification.
Add proper response validation, error handling, and use API path constants.
interceptFacilitySearchReq(): void {
- cy.intercept("GET", "**/api/v1/facility/**").as("searchFacility");
+ cy.intercept("GET", `${this.API_PATHS.FACILITY}/**`).as("searchFacility");
}
verifyFacilitySearchReq(): void {
- cy.wait("@searchFacility").its("response.statusCode").should("eq", 200);
+ cy.wait("@searchFacility").then((interception) => {
+ if (!interception.response) {
+ throw new Error('No response received from facility search');
+ }
+ expect(interception.response.statusCode).to.equal(200);
+ expect(interception.response.body).to.have.property('data');
+ });
}
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) => {
+ if (!interception.response) {
+ throw new Error('No response received from notification post');
+ }
+ expect(interception.request.body).to.have.property('message');
+ expect(interception.response.statusCode).to.equal(204);
+ });
}
interceptGetNotificationReq(): void {
- cy.intercept("GET", "**/api/v1/notification/**").as("getNotifications");
+ cy.intercept("GET", `${this.API_PATHS.NOTIFICATION}/**`).as("getNotifications");
}
verifyGetNotificationReq(): void {
- cy.wait("@getNotifications").its("response.statusCode").should("eq", 200);
+ cy.wait("@getNotifications").then((interception) => {
+ if (!interception.response) {
+ throw new Error('No response received from get notifications');
+ }
+ const response = interception.response;
+ expect(response.statusCode).to.equal(200);
+
+ const body = response.body as NotificationResponse;
+ expect(body).to.have.property('message');
+ expect(body).to.have.property('status');
+ });
}Also applies to: 29-35, 37-43
| @@ -0,0 +1,44 @@ | |||
| export class FacilityNotify { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance class structure with TypeScript best practices.
Add type definitions and constants to improve maintainability and type safety.
+interface NotificationResponse {
+ message: string;
+ status: string;
+}
+
export class FacilityNotify {
+ private readonly SELECTORS = {
+ NOTIFY_INPUT: '#NotifyModalMessageInput',
+ NOTIFICATION_SLIDE_BTN: '#notification-slide-btn',
+ CLOSE_SLIDE_BTN: '#close-slide-over',
+ NOTICE_BOARD_LINK: 'a[href="/notice_board"]'
+ } as const;
+
+ private readonly API_PATHS = {
+ FACILITY: '**/api/v1/facility',
+ NOTIFICATION: '**/api/v1/notification'
+ } as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class FacilityNotify { | |
| interface NotificationResponse { | |
| message: string; | |
| status: string; | |
| } | |
| export class FacilityNotify { | |
| private readonly SELECTORS = { | |
| NOTIFY_INPUT: '#NotifyModalMessageInput', | |
| NOTIFICATION_SLIDE_BTN: '#notification-slide-btn', | |
| CLOSE_SLIDE_BTN: '#close-slide-over', | |
| NOTICE_BOARD_LINK: 'a[href="/notice_board"]' | |
| } as const; | |
| private readonly API_PATHS = { | |
| FACILITY: '**/api/v1/facility', | |
| NOTIFICATION: '**/api/v1/notification' | |
| } as const; |
…041/test-verify-notice-board
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
5-5: Consider using a more descriptive test message.The notification message is too simple for testing real-world scenarios. Consider using a more comprehensive message that includes:
- Dynamic dates
- Special characters
- Multi-line content
- Maximum length edge cases
- const notificationMessage = "Test Notification"; + const notificationMessage = `Facility Update: Monthly review meeting scheduled for ${Cypress.dayjs().add(7, 'days').format('Do MMM, YYYY')} at 10:00 AM. + +Please ensure to: +- Update patient records +- Prepare monthly statistics +- Review inventory status + +Contact support@example.com for any queries.`;Also applies to: 13-13, 27-28
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)
🔇 Additional comments (1)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
37-37: LGTM! Good practice clearing filters.
The addition of filter clearing in beforeEach hook is a good practice to ensure a clean state for each test.
| it("Verify Notice Board Functionality", () => { | ||
| // search facility and verify it's loaded or not | ||
| manageUserPage.interceptFacilitySearchReq(); | ||
| manageUserPage.typeFacilitySearch(facilityName); | ||
| manageUserPage.verifyFacilitySearchReq(); | ||
| // verify facility name and card reflection | ||
| facilityNotify.verifyUrlContains("Dummy+Facility+40"); | ||
| facilityPage.verifyFacilityBadgeContent(facilityName); | ||
| manageUserPage.assertFacilityInCard(facilityName); | ||
| // send notification to a facility | ||
| facilityHome.clickFacilityNotifyButton(); | ||
| facilityNotify.verifyFacilityName(facilityName); | ||
| facilityNotify.fillNotifyText(notificationMessage); | ||
| facilityNotify.interceptPostNotificationReq(); | ||
| cy.submitButton("Notify"); | ||
| facilityNotify.verifyPostNotificationReq(); | ||
| cy.verifyNotification("Facility Notified"); | ||
| cy.closeNotification(); | ||
| cy.wait(2000); | ||
| // Verify the frontend error on empty message | ||
| facilityHome.clickFacilityNotifyButton(); | ||
| facilityNotify.verifyFacilityName(facilityName); | ||
| cy.submitButton("Notify"); | ||
| facilityNotify.verifyErrorMessage(notificationErrorMsg); | ||
| // close pop-up and verify | ||
| facilityHome.verifyAndCloseNotifyModal(); | ||
| // signout as district admin and login as a Nurse | ||
| loginPage.ensureLoggedIn(); | ||
| loginPage.clickSignOutBtn(); | ||
| loginPage.loginManuallyAsNurse(); | ||
| // Verify Notice Board Reflection | ||
| facilityNotify.interceptGetNotificationReq("MESSAGE"); | ||
| facilityNotify.visitNoticeBoard(); | ||
| facilityNotify.verifyGetNotificationReq(); | ||
| facilityNotify.verifyFacilityNoticeBoardMessage(notificationMessage); | ||
| facilityNotify.interceptGetNotificationReq(); | ||
| // Verify Sidebar Notification Reflection | ||
| facilityNotify.openNotificationSlide(); | ||
| facilityNotify.verifyGetNotificationReq(); | ||
| cy.verifyContentPresence("#notification-slide-msg", [notificationMessage]); | ||
| facilityNotify.closeNotificationSlide(); | ||
| loginPage.ensureLoggedIn(); | ||
| loginPage.clickSignOutBtn(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test case needs to be split into smaller, focused tests.
The current test case is doing too much and violates the Single Responsibility Principle. It should be split into smaller, focused tests following the AAA (Arrange-Act-Assert) pattern.
Split into these focused test cases:
describe('Notice Board Functionality', () => {
beforeEach(() => {
cy.restoreLocalStorage();
// Common setup
manageUserPage.interceptFacilitySearchReq();
manageUserPage.typeFacilitySearch(facilityName);
manageUserPage.verifyFacilitySearchReq();
});
it('should validate empty notification message', () => {
facilityHome.clickFacilityNotifyButton();
cy.submitButton("Notify");
facilityNotify.verifyErrorMessage(notificationErrorMsg);
});
it('should successfully send notification as district admin', () => {
facilityHome.clickFacilityNotifyButton();
facilityNotify.fillNotifyText(notificationMessage);
facilityNotify.interceptPostNotificationReq();
cy.submitButton("Notify");
facilityNotify.verifyPostNotificationReq();
cy.verifyNotification("Facility Notified");
});
it('should display notification to nurse', () => {
loginPage.loginManuallyAsNurse();
facilityNotify.interceptGetNotificationReq();
facilityNotify.visitNoticeBoard();
facilityNotify.verifyGetNotificationReq();
cy.verifyContentPresence("#notification-message", [notificationMessage]);
facilityNotify.openNotificationSlide();
cy.verifyContentPresence("#notification-slide-msg", [notificationMessage]);
});
});🛠️ Refactor suggestion
Add error handling test cases.
The test suite lacks error scenarios that could occur in production.
Add these test cases:
it('should handle network failure when sending notification', () => {
facilityHome.clickFacilityNotifyButton();
facilityNotify.fillNotifyText(notificationMessage);
cy.intercept('POST', '**/api/v1/notification/notify', {
forceNetworkError: true
}).as('networkError');
cy.submitButton("Notify");
cy.wait('@networkError');
cy.verifyNotification("Failed to send notification");
});
it('should handle server error when sending notification', () => {
facilityHome.clickFacilityNotifyButton();
facilityNotify.fillNotifyText(notificationMessage);
cy.intercept('POST', '**/api/v1/notification/notify', {
statusCode: 500,
body: { error: 'Internal Server Error' }
}).as('serverError');
cy.submitButton("Notify");
cy.wait('@serverError');
cy.verifyNotification("Server error occurred");
});🛠️ Refactor suggestion
Add cleanup in afterEach hook.
The test modifies application state but doesn't clean up afterwards.
afterEach(() => {
cy.saveLocalStorage();
+ // Clean up notifications created during tests
+ if (cy.state('test').title.includes('Notice Board')) {
+ facilityNotify.interceptDeleteNotificationReq();
+ facilityNotify.deleteAllNotifications();
+ facilityNotify.verifyDeleteNotificationReq();
+ }
});Committable suggestion skipped: line range outside the PR's diff.
| facilityNotify.verifyPostNotificationReq(); | ||
| cy.verifyNotification("Facility Notified"); | ||
| cy.closeNotification(); | ||
| cy.wait(2000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded wait with explicit wait conditions.
Using cy.wait(2000) and manual login verification makes the test flaky. Instead:
- Wait for specific elements or state changes
- Use Cypress's session management
- cy.wait(2000);
+ cy.get('[data-testid="notification-popup"]')
+ .should('not.exist');
- loginPage.ensureLoggedIn();
- loginPage.clickSignOutBtn();
+ cy.session('nurse', () => {
+ loginPage.loginManuallyAsNurse();
+ });Also applies to: 180-181
|
@nihal467 @JavidSumra Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
|
Hey @rithviknishad can you please assign 2 story points for this. |
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
Improvements
idproperties to various components for better accessibility and targeting.Bug Fixes