Facility bed capacity verification test #9145
Conversation
- Added logic to test for facility badge behavior - Added reusable function for patient and facility creation
WalkthroughThe changes involve significant enhancements to various Cypress test files and page objects related to facility and patient management. Key modifications include the introduction of structured data handling for facilities and patients, refactoring of test cases to improve readability and maintainability, and the addition of new methods for managing beds and patient data. The updates also streamline interactions with the UI, ensuring that the tests are more concise and organized, while retaining the original functionality and error handling mechanisms. Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
cypress/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: 12
🧹 Outside diff range and nitpick comments (24)
cypress/pageobject/Facility/FacilityHome.ts (2)
38-40: Good refactoring using Cypress aliases!The changes follow Cypress best practices by using aliases to cache the element and adding explicit timeouts. However, for consistency with other methods in this class that handle similar UI elements (e.g.,
clickViewFacilityDetails), consider addingscrollIntoView().Consider this minor improvement:
clickFacilityNotifyButton() { cy.get("#facility-notify").as("facilityNotify"); + cy.get("@facilityNotify").scrollIntoView(); cy.get("@facilityNotify", { timeout: 10000 }).should("be.visible"); cy.get("@facilityNotify").first().click(); }
Line range hint
82-84: Consider enhancing occupancy badge verification methodsGiven the PR's focus on bed capacity verification, consider adding methods to verify the actual occupancy status beyond just visibility. This would support the test scenarios mentioned in issue #9107.
Consider adding these methods:
verifyOccupancyBadgeText(expectedText: string) { cy.get('[data-test-id="occupancy-badge"]') .should('be.visible') .and('contain.text', expectedText); } verifyOccupancyBadgeState(state: 'full' | 'available' | 'warning') { // Assuming the badge has different classes or data attributes for different states cy.get('[data-test-id="occupancy-badge"]') .should('have.attr', 'data-state', state); }cypress/pageobject/Facility/FacilityManage.ts (1)
103-109: LGTM! Consider these enhancements for better maintainability.The implementation follows POM best practices and correctly handles API interception. Consider these improvements:
- Extract the API endpoint to a constant for reusability
- Add error handling for failed API calls
- Add JSDoc comments describing the method's purpose
+ /** API endpoint for facility patients */ + const FACILITY_API_ENDPOINT = "**/api/v1/facility/**"; + + /** + * Navigates to the view patients page and verifies the API response + * @throws {Error} If the API call fails or returns a non-200 status + */ visitViewPatients() { - cy.intercept("GET", "**/api/v1/facility/**").as("getFacilityPatients"); + cy.intercept("GET", FACILITY_API_ENDPOINT).as("getFacilityPatients"); cy.get("#view-patient-facility-list").scrollIntoView().click(); cy.wait("@getFacilityPatients") .its("response.statusCode") - .should("eq", 200); + .should("eq", 200) + .catch((error) => { + throw new Error(`Failed to fetch facility patients: ${error.message}`); + }); }cypress/pageobject/Facility/FacilityLocation.ts (2)
79-81: Enhance API response verificationWhile the status code check is good, consider adding more comprehensive API response validation:
cy.intercept("/api/v1/bed/?facility=*").as("getBeds"); cy.get("#manage-bed-button").first().click({}); -cy.wait("@getBeds").its("response.statusCode").should("eq", 200); +cy.wait("@getBeds").then((interception) => { + expect(interception.response?.statusCode).to.eq(200); + expect(interception.response?.body).to.have.property('results'); + expect(interception.response?.headers).to.have.property('content-type').contains('application/json'); +});
Line range hint
1-180: Consider implementing a data builder pattern for test dataTo improve test maintainability and reduce duplication across test files:
- Create a separate
BedBuilderclass for constructing test bed data- Implement a fluent interface for better readability
- Add factory methods for common bed configurations
Example implementation:
// cypress/support/builders/BedBuilder.ts export class BedBuilder { private data = { name: '', description: '', type: '', count: 1 }; static default(): BedBuilder { return new BedBuilder() .withName('Test Bed') .withType('Normal') .withDescription('Test Description'); } withName(name: string): BedBuilder { this.data.name = name; return this; } // ... other builder methods build(): BedData { return { ...this.data }; } }This would allow for more maintainable test data creation:
const bed = BedBuilder.default() .withName('ICU Bed') .withType('ICU') .build(); facilityLocation.addBed(bed.name, bed.description, bed.type);cypress/pageobject/Users/ManageUserPage.ts (1)
171-177: Consider adding type safety and documentation for color parameter.While the implementation is correct, it could benefit from some improvements:
Consider applying these enhancements:
- assertFacilityBadgeBackgroundColor(color: string) { + /** + * Asserts the background color of the occupancy badge + * @param color - CSS color value (e.g., 'rgb(239, 68, 68)' for red, 'rgb(34, 197, 94)' for green) + */ + assertFacilityBadgeBackgroundColor(color: `rgb(${number}, ${number}, ${number})`) { cy.get('[data-test-id="occupancy-badge"]').should( "have.css", "background-color", color, ); }cypress/pageobject/Patient/PatientHome.ts (2)
33-35: LGTM! Consider adding JSDoc documentation.The implementation follows POM patterns and aligns well with existing methods. Consider adding JSDoc documentation for better maintainability:
+/** + * Types and selects a patient name in the name input field + * @param patientName - The name of the patient to type and select + */ typePatientName(patientName: string) { cy.typeAndSelectOption("input[name='name']", patientName); }
33-35: Consider using data-testid for better selector resilience.The current selector
input[name='name']could be fragile if the name attribute changes. Consider using a data-testid attribute for better test resilience:typePatientName(patientName: string) { - cy.typeAndSelectOption("input[name='name']", patientName); + cy.typeAndSelectOption("[data-testid='patient-name-input']", patientName); }cypress/e2e/patient_spec/PatientRegistration.cy.ts (2)
70-101: Consider moving patient data to a fixture file and using enums for constants.The structured
PatientDataobject is well-organized and comprehensive, which aligns with making patient creation more reusable. However, there are a few suggestions for improvement:
Move this test data to a Cypress fixture file to:
- Separate test data from test logic
- Make it reusable across different test files
- Make it easier to maintain different test scenarios
Consider using enums for magic strings like "MIDDLE_CLASS" and "FAMILY_MEMBER" to:
- Ensure type safety
- Make maintenance easier
- Prevent typos
Example implementation:
// cypress/fixtures/patients.json { "defaultPatient": { "facility": "Dummy Facility 40", // ... rest of the patient data } } // cypress/support/enums.ts export enum SocioeconomicStatus { MIDDLE_CLASS = "MIDDLE_CLASS", // ... other statuses } export enum HealthcareSupport { FAMILY_MEMBER = "FAMILY_MEMBER", // ... other support types }Then in the test:
import { SocioeconomicStatus, HealthcareSupport } from '../support/enums'; describe('Patient Creation with consultation', () => { let newPatientData: PatientData; beforeEach(() => { cy.fixture('patients').then(({ defaultPatient }) => { newPatientData = defaultPatient; }); }); // ... rest of the test });
Line range hint
1-269: Consider implementing the Builder pattern for patient creation.While the current implementation with
PatientDatais good, the Builder pattern could further improve the test data creation by:
- Making it easier to create different patient scenarios
- Providing default values
- Making the creation process more fluent and readable
Example implementation:
// cypress/support/builders/PatientBuilder.ts export class PatientBuilder { private patient: PatientData = { // ... default values }; withName(name: string): PatientBuilder { this.patient.name = name; return this; } withGender(gender: string): PatientBuilder { this.patient.gender = gender; return this; } // ... other builder methods build(): PatientData { return { ...this.patient }; } static aDefaultPatient(): PatientBuilder { return new PatientBuilder(); } } // In test: const newPatientData = PatientBuilder.aDefaultPatient() .withName(patientOneName) .withGender(patientOneGender) .build();cypress/e2e/facility_spec/FacilityLocation.cy.ts (3)
81-91: Enhance error handling for duplicate bed creation.The duplicate bed verification could be more robust by checking the exact error message and ensuring the bed wasn't created.
Consider adding these verifications:
// After attempting to create duplicate bed facilityLocation.verifyNotification( "Name - Bed with same name already exists in location" ); // Verify the bed count hasn't increased facilityLocation.verifyBedCount(1);
Line range hint
142-165: Enhance verification for multiple bed creation and deletion.The current verification could be more comprehensive for multiple bed scenarios.
Consider adding these verifications:
// After creating multiple beds facilityLocation.verifyBedCount(numberOfBeds); facilityLocation.verifyAllBedsHaveType(bedType); facilityLocation.verifyAllBedsHaveStatus(bedStatus); // After deleting a bed facilityLocation.verifyBedCount(numberOfBeds - 1);
Line range hint
1-276: Consider implementing a data-driven test approach.The test file contains multiple similar test cases with different data. Consider implementing a data-driven approach to reduce code duplication and improve maintainability.
Example implementation:
const testCases = [ { name: 'Single bed', bedData: { name: 'Test-Bed', description: 'test description', type: 'ICU' }, expectedCount: 1 }, { name: 'Multiple beds', bedData: { name: 'Test-Bed', description: 'test description', type: 'ICU', quantity: 10 }, expectedCount: 10 } ]; testCases.forEach(({ name, bedData, expectedCount }) => { it(`Creates ${name}`, () => { facilityLocation.addBed(bedData); facilityLocation.verifyBedCount(expectedCount); }); });cypress/e2e/facility_spec/FacilityCreation.cy.ts (3)
1-3: Consider improving test data managementThe introduction of
FacilityDatatype and structured test data is a good improvement. However, consider these enhancements:
- Move the test data to a fixture file to improve maintainability
- Extract magic numbers/strings into named constants
- Create reusable location data that can be shared across tests
Example refactor:
// cypress/fixtures/facility-data.json { "testFacility": { "basic": { "name": "Cypress Facility", // ... other fields }, // ... other sections } } // cypress/support/constants.ts export const FACILITY_CONSTANTS = { OXYGEN_CAPACITY: "100", OXYGEN_EXPECTED: "80", // ... other constants } // In test file import facilityData from '../fixtures/facility-data.json'; import { FACILITY_CONSTANTS } from '../support/constants';Also applies to: 63-117
215-239: Simplify test data creationThe current approach of spreading and overriding test data is verbose. Consider creating a helper function to generate test data variants.
function createFacilityData(options: Partial<FacilityData> = {}): FacilityData { return { ...testFacilityData, ...options, basic: { ...testFacilityData.basic, ...(options.basic || {}), }, }; } // Usage const singleCapacityData = createFacilityData({ basic: { features: undefined, location: undefined, }, oxygen: undefined, beds: [/* single bed config */], doctors: [/* single doctor config */], });
Line range hint
177-214: Extract verification steps into helper methodsThe facility verification steps are repeated across test cases. Consider extracting them into reusable helper methods.
// In FacilityPage class verifyFacilityDetails(data: FacilityData) { this.getFacilityName().contains(data.basic.name).should("be.visible"); this.getAddressDetailsView() .contains(data.basic.address) .should("be.visible"); // ... other verifications } // In test facilityPage.verifyFacilityDetails(testFacilityData);src/components/Patient/PatientHome.tsx (1)
Line range hint
1-1186: Consider splitting the component for better maintainabilityThe
PatientHomecomponent is quite large and handles multiple responsibilities including:
- Patient details display
- Consultation management
- Sample test management
- Shifting management
- File management
Consider breaking it down into smaller, focused components for better maintainability and performance.
Suggested improvements:
Extract the following sections into separate components:
- Patient Details Section
- Medical History Section
- Consultation History Section
- Sample Test History Section
- Action Buttons Section
Use React.memo() for optimizing re-renders of static sections:
const PatientDetailsSection = React.memo(({ patient }: { patient: PatientModel }) => { // Extract patient details section here }); const MedicalHistorySection = React.memo(({ medicalHistory }: { medicalHistory: any[] }) => { // Extract medical history section here });
- Consider using a reducer for complex state management:
type PatientState = { patientData: PatientModel; showShifts: boolean; isShiftClicked: boolean; // ... other state }; type PatientAction = | { type: 'SET_PATIENT_DATA'; payload: PatientModel } | { type: 'TOGGLE_SHIFTS' } | { type: 'SET_SHIFT_CLICKED' }; const patientReducer = (state: PatientState, action: PatientAction) => { switch (action.type) { case 'SET_PATIENT_DATA': return { ...state, patientData: action.payload }; // ... other cases } };cypress/pageobject/Patient/PatientCreation.ts (4)
7-8: InstantiatefacilityPageandpatientMedicalHistorywithin thePatientPageclass for better encapsulationCurrently,
facilityPageandpatientMedicalHistoryare instantiated at the module level, which can lead to unintended side effects if multiple instances ofPatientPageare created. Moving these instances inside thePatientPageclass ensures better encapsulation and avoids potential issues with shared state.Apply this diff to move the instances into the class:
-export const facilityPage = new FacilityPage(); -export const patientMedicalHistory = new PatientMedicalHistory(); export class PatientPage { + private facilityPage = new FacilityPage(); + private patientMedicalHistory = new PatientMedicalHistory(); createPatient() { cy.intercept("GET", "**/api/v1/facility/**").as("getFacilities");
14-14: Use a consistent type forageIn the
PatientDatainterface,ageis typed asstring | number. For consistency and to prevent potential type errors, it's advisable to use a single type. Consideringageis a numerical value, usingnumberwould be appropriate.Apply this diff to update the type:
age: string | number; + // Change to: + age: number;Ensure that any methods using
data.agehandle it as a number.
196-198: Simplify the method name for clarityThe method name
clickCreateConsultationOnPatientPageWithNoConsultationis quite verbose. Simplifying it toclickCreateConsultationmakes it clearer and easier to read, without losing meaning.Apply this diff to rename the method:
} - clickCreateConsultationOnPatientPageWithNoConsultation() { + clickCreateConsultation() { cy.get("#create-consultation").should("be.visible").click(); }Also, update the method call in line 62:
cy.get("#create-consultation").should("be.visible"); - this.clickCreateConsultationOnPatientPageWithNoConsultation(); + this.clickCreateConsultation();
284-343: RefactorcreatePatientWithDatafor improved maintainabilityThe
createPatientWithDatamethod is lengthy and handles multiple responsibilities, which can make it harder to maintain. Breaking it into smaller helper methods or using loops for repetitive tasks can enhance readability and maintainability.Consider the following refactoring strategies:
Create helper methods: Extract sections of the method into private helper methods within the
PatientPageclass. For example, methods likefillPersonalDetails,fillAddressDetails,fillMedicalHistory, etc.Use loops for similar operations: For optional fields and medical history conditions, you can loop over arrays of field names and values.
Example of extracting a helper method:
createPatientWithData(data: PatientData) { this.createPatient(); this.selectFacility(data.facility); this.patientformvisibility(); + this.fillPersonalDetails(data); + this.fillAddressDetails(data); + this.fillAdditionalDetails(data); + this.fillMedicalHistory(data.medicalHistory); + this.clickCreatePatient(); this.verifyPatientIsCreated(); } + private fillPersonalDetails(data: PatientData) { + this.typePatientPhoneNumber(data.phoneNumber); + if (data.isEmergencyNumber) { + this.checkPhoneNumberIsEmergencyNumber(); + } + this.typePatientAge(data.age); + this.typePatientName(data.name); + this.selectPatientGender(data.gender); + this.typePatientAddress(data.address); + } + private fillAddressDetails(data: PatientData) { + this.facilityPage.fillPincode(data.pincode); + this.facilityPage.selectStateOnPincode(data.state); + this.facilityPage.selectDistrictOnPincode(data.district); + this.facilityPage.selectLocalBody(data.localBody); + this.facilityPage.selectWard(data.ward); + } + // Similarly, create methods for fillAdditionalDetails and fillMedicalHistoryThis refactoring makes the
createPatientWithDatamethod cleaner and the code easier to understand.cypress/e2e/facility_spec/FacilityHomepage.cy.ts (2)
2-6: Ensure consistent import styles for clarityThe imports mix default and named imports, which can reduce readability and maintainability. Consider adopting a consistent import style throughout the file for better clarity.
68-68: Avoid using random values in tests to ensure consistencyThe
patientIpNumberis generated using random numbers, which can lead to flaky tests due to unpredictability. Using fixed or deterministic values ensures consistent test results and easier debugging.Suggested change:
- const patientIpNumber = `${Math.floor(Math.random() * 90 + 10)}/${Math.floor(Math.random() * 9000 + 1000)}`; + const patientIpNumber = "12345/67890"; // Use a fixed or sequential valuecypress/pageobject/Facility/FacilityCreation.ts (1)
586-604: Consider refactoring repetitive code for cylinder typesIn
fillOxygenDetails, similar code blocks are used forbType,cType, anddTypecylinders. Refactoring this into a loop can reduce code duplication and enhance maintainability.Here's how you might refactor the code:
fillOxygenDetails(oxygen: NonNullable<FacilityData["oxygen"]>) { this.fillOxygenCapacity(oxygen.capacity); this.fillExpectedOxygenRequirement(oxygen.expected); + const cylinderTypes = ['b', 'c', 'd']; + cylinderTypes.forEach((type) => { + const cylinder = oxygen[`${type}Type` as keyof typeof oxygen]; + if (cylinder) { + this[`fill${type.toUpperCase()}TypeCylinderCapacity`](cylinder.capacity); + this[`fillExpected${type.toUpperCase()}TypeCylinderRequirement`](cylinder.expected); + } + }); - - if (oxygen.bType) { - this.fillBTypeCylinderCapacity(oxygen.bType.capacity); - this.fillExpectedBTypeCylinderRequirement(oxygen.bType.expected); - } - if (oxygen.cType) { - this.fillCTypeCylinderCapacity(oxygen.cType.capacity); - this.fillExpectedCTypeCylinderRequirement(oxygen.cType.expected); - } - if (oxygen.dType) { - this.fillDTypeCylinderCapacity(oxygen.dType.capacity); - this.fillExpectedDTypeCylinderRequirement(oxygen.dType.expected); - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
cypress/e2e/facility_spec/FacilityCreation.cy.ts(5 hunks)cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)cypress/e2e/facility_spec/FacilityLocation.cy.ts(5 hunks)cypress/e2e/patient_spec/PatientRegistration.cy.ts(3 hunks)cypress/pageobject/Facility/FacilityCreation.ts(3 hunks)cypress/pageobject/Facility/FacilityHome.ts(1 hunks)cypress/pageobject/Facility/FacilityLocation.ts(2 hunks)cypress/pageobject/Facility/FacilityManage.ts(1 hunks)cypress/pageobject/Patient/PatientCreation.ts(4 hunks)cypress/pageobject/Patient/PatientHome.ts(1 hunks)cypress/pageobject/Users/ManageUserPage.ts(1 hunks)src/components/Facility/FacilityCard.tsx(1 hunks)src/components/Patient/PatientHome.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/Facility/FacilityCard.tsx
🔇 Additional comments (12)
cypress/pageobject/Facility/FacilityLocation.ts (1)
22-22: Review the need for forced clicks in button interactions
The addition of { force: true } to click handlers can mask underlying UI issues. Consider investigating why these elements might not be naturally clickable:
- Are there timing issues with element rendering?
- Is there a z-index or overlay problem?
- Are elements properly visible in the viewport?
Also applies to: 85-85
cypress/pageobject/Users/ManageUserPage.ts (1)
164-169: LGTM! Well-structured badge content assertion.
The implementation correctly uses data-test-id for stable element selection and verifies the expected occupancy badge format.
cypress/e2e/patient_spec/PatientRegistration.cy.ts (2)
2-5: LGTM! Well-structured imports with proper type safety.
The addition of PatientData type import aligns with the PR objectives of creating reusable functions for patient creation.
115-115: LGTM! Good refactor to use the new structured patient creation.
The change to use createPatientWithData improves maintainability while preserving the original test functionality and verification steps.
cypress/e2e/facility_spec/FacilityLocation.cy.ts (1)
194-194: LGTM! The bed creation implementation is clean and reusable.
The implementation aligns well with the PR objectives of creating reusable functions for bed creation.
cypress/e2e/facility_spec/FacilityCreation.cy.ts (1)
Line range hint 1-348: Overall changes look good with room for improvement
The introduction of structured test data and type-safe facility creation is a significant improvement. The code is functional and maintains good test coverage. The suggested refactors would further enhance maintainability and reusability, but they're not critical for the current functionality.
src/components/Patient/PatientHome.tsx (2)
337-337: LGTM: ID addition improves testability
The addition of id="create-consultation" enhances the button's accessibility and testability, which aligns with the PR objectives.
350-353: LGTM: ID addition improves element identification
The addition of id="patient-name-age" to the header element enhances testability and follows consistent naming conventions.
cypress/pageobject/Facility/FacilityCreation.ts (4)
475-476: LGTM
The use of an alias for the district element improves code readability and avoids redundant calls to this.getDistrictElement().
527-556: createNewFacility method enhances code modularity
The createNewFacility method effectively encapsulates the facility creation workflow, promoting reusability and improving test maintainability.
558-576: fillBasicDetails method is well-structured
The fillBasicDetails method neatly organizes the steps to populate basic facility details, with appropriate handling of optional fields like features and location.
578-584: fillLocationDetails method properly fills location data
The method correctly handles the sequential selection of location fields, ensuring consistency in the facility's location information.
There was a problem hiding this comment.
Copilot reviewed 5 out of 13 changed files in this pull request and generated no suggestions.
Files not reviewed (8)
- src/components/Facility/FacilityCard.tsx: Evaluated as low risk
- cypress/e2e/patient_spec/PatientRegistration.cy.ts: Evaluated as low risk
- cypress/pageobject/Patient/PatientHome.ts: Evaluated as low risk
- cypress/pageobject/Users/ManageUserPage.ts: Evaluated as low risk
- src/components/Patient/PatientHome.tsx: Evaluated as low risk
- cypress/e2e/facility_spec/FacilityLocation.cy.ts: Evaluated as low risk
- cypress/e2e/facility_spec/FacilityCreation.cy.ts: Evaluated as low risk
- cypress/pageobject/Patient/PatientCreation.ts: Evaluated as low risk
Comments skipped due to low confidence (2)
cypress/pageobject/Facility/FacilityCreation.ts:598
- The focus().clear() pattern is repeated twice. It would be more efficient to combine these into a single statement.
cy.get("#expected_type_c_cylinders").focus().clear();
cypress/pageobject/Facility/FacilityCreation.ts:602
- The focus().clear() pattern is repeated twice. It would be more efficient to combine these into a single statement.
cy.get("#expected_type_d_cylinders").focus().clear();
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
cypress/pageobject/Patient/PatientTreatmentPlan.ts (1)
Line range hint
1-42: Consider enhancing type safety and documentation.The class implementation is clean and follows good POM practices. Consider these improvements:
- Add TypeScript interfaces for method parameters
- Add JSDoc comments for better documentation
- Consider adding input validation for critical fields
Example enhancement:
interface TreatmentPlanInput { doctor: string; instruction: string; time: string; } /** * Page Object Model for Patient Treatment Plan management */ class PatientTreatmentPlan { /** * Fills in the treating physician field * @param doctor - The name of the treating physician * @throws Will throw an error if the physician cannot be selected */ fillTreatingPhysician(doctor: string): void { if (!doctor?.trim()) { throw new Error('Doctor name cannot be empty'); } cy.typeAndSelectOption("#treating_physician", doctor); } // ... rest of the methods }cypress/pageobject/Patient/PatientCreation.ts (1)
10-33: Add JSDoc documentation to the PatientData interfaceWhile the interface is well-structured, adding JSDoc documentation would improve maintainability and help other developers understand the purpose and constraints of each field.
Example addition:
+/** + * Interface representing patient registration data + * @property facility - Name of the healthcare facility + * @property phoneNumber - Primary contact number + * @property isEmergencyNumber - Whether the phone number is for emergencies + * ... + */ export interface PatientData {cypress/e2e/facility_spec/FacilityCreation.cy.ts (2)
63-117: Consider using test data factories for better maintainability.While the structured data approach is good, consider extracting this into a test data factory pattern. This would:
- Make it easier to create different test scenarios
- Reduce duplication in test data creation
- Improve maintainability of test data
Example implementation:
// testDataFactories.ts export const createTestFacilityData = (overrides?: Partial<FacilityData>): FacilityData => ({ basic: { name: "Cypress Facility", type: "Primary Health Centres", // ... other default values }, // ... other default sections ...overrides });
246-271: Simplify test data manipulation.While using the spread operator is good, the explicit undefined assignments make the code verbose. Consider using a utility function for cleaner data manipulation.
Example implementation:
const createSingleCapacityData = (baseData: FacilityData): FacilityData => { const { features, location, oxygen, ...basicData } = baseData.basic; return { ...baseData, basic: basicData, beds: [baseData.beds[0]], doctors: [baseData.doctors[0]], oxygen: undefined }; }; // Usage const singleCapacityData = createSingleCapacityData(testFacilityData);cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
68-68: Improve readability of random number generation.Consider extracting the random number generation logic into a helper function for better maintainability and reuse.
- const patientIpNumber = `${Math.floor(Math.random() * 90 + 10)}/${Math.floor(Math.random() * 9000 + 1000)}`; + const generateRandomNumber = (min: number, max: number) => Math.floor(Math.random() * (max - min + 1)) + min; + const patientIpNumber = `${generateRandomNumber(10, 99)}/${generateRandomNumber(1000, 9999)}`;
225-227: Add error handling for login failures.Consider adding error handling and retry logic for login operations to make the tests more resilient.
- loginPage.loginManuallyAsDistrictAdmin(); - loginPage.ensureLoggedIn(); + cy.wrap(null).then(() => { + return cy.loginWithRetry(() => { + loginPage.loginManuallyAsDistrictAdmin(); + loginPage.ensureLoggedIn(); + }, { maxAttempts: 3 }); + });
270-271: Replace hardcoded selectors with data-testid attributes.Using hardcoded ID selectors makes the tests brittle. Consider using data-testid attributes for test selectors.
- cy.get("[id='manage-facility-dropdown']") + cy.get("[data-testid='manage-facility-dropdown']") - cy.get("[id=location-management]") + cy.get("[data-testid='location-management']") - cy.get("#is_asymptomatic") + cy.get("[data-testid='is-asymptomatic']")Also applies to: 301-301
cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts (2)
Line range hint
14-31: Consider using structured test data objectsThe test data constants could be better organized using a structured object or interface, aligning with the PR's approach of using structured data (as mentioned in the AI summary regarding
PatientDataandFacilityDataobjects).Example refactor:
interface ConsultationTestData { facility: { name: string; doctorName: string; }; diagnosis: { codes: string[]; }; patient: { history: string; examination: string; treatment: string; instructions: { general: string; special: string; }; vitals: { weight: string; height: string; }; }; }
Line range hint
13-14: Add dedicated test cases for bed capacity verificationTo better align with the PR objectives (issue #9107), consider adding specific test cases for bed capacity verification scenarios:
- Verify facility with no available beds
- Verify occupancy badge updates
- Test bed addition scenarios
Example test case structure:
it("should update bed capacity badge correctly", () => { // Verify initial bed capacity // Add patient and verify badge update // Add bed and verify badge update // Verify removal of "no beds available" alert });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cypress/e2e/facility_spec/FacilityCreation.cy.ts(5 hunks)cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)cypress/e2e/facility_spec/FacilityLocation.cy.ts(5 hunks)cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts(4 hunks)cypress/pageobject/Patient/PatientCreation.ts(4 hunks)cypress/pageobject/Patient/PatientTreatmentPlan.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/facility_spec/FacilityLocation.cy.ts
🔇 Additional comments (8)
cypress/pageobject/Patient/PatientTreatmentPlan.ts (1)
6-8: LGTM! Method name spelling correction.
The rename from fillTreatingPhysican to fillTreatingPhysician fixes the typo in the method name.
Let's verify that all references to this method have been updated:
✅ Verification successful
Method rename is complete and consistent across the codebase
The verification shows that:
- No instances of the old method name
fillTreatingPhysicanexist in the codebase - The new method name
fillTreatingPhysicianis properly used in:- The implementation in
PatientTreatmentPlan.ts - Multiple test files including
PatientConsultationCreation.cy.tsandFacilityHomepage.cy.ts
- The implementation in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old method name
# and verify all occurrences of the new method name
echo "Checking for any remaining instances of the old method name..."
rg -l "fillTreatingPhysican"
echo "Verifying usage of the new method name..."
rg -A 2 "fillTreatingPhysician"
Length of output: 2193
cypress/pageobject/Patient/PatientCreation.ts (1)
2-8: LGTM: Clean import structure and proper instance scoping
The imports and class-level instances are well organized, promoting code reuse across methods.
cypress/e2e/facility_spec/FacilityCreation.cy.ts (2)
1-3: LGTM! Good type safety practice.
Adding the FacilityData interface import improves type safety and code maintainability.
291-304: Refer to previous review comment about facility creation consistency.
The previous review comment about maintaining consistency in the facility creation approach is still applicable here.
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
2-6: Skip comment as the issue was already identified in a previous review.
25-29: LGTM! Page object instantiations follow the POM pattern.
The new page objects are correctly instantiated and align with the existing pattern.
44-67: Skip comment as PII concerns were already identified in a previous review.
cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts (1)
93-93: Spelling correction properly implemented
The method name fillTreatingPhysician has been consistently updated across all occurrences, improving code maintainability.
Also applies to: 257-257, 314-314
|
👋 Hi, @Jacobjeevan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
cypress/pageobject/Patient/PatientCreation.ts (3)
10-33: Add JSDoc comments to document validation constraintsThe interface is well-structured, but adding JSDoc comments would help document validation constraints (e.g., valid phone number format, age limits, valid pincode format).
export interface PatientData { + /** Facility name that must exist in the system */ facility: string; + /** Phone number in format: +91XXXXXXXXXX */ phoneNumber: string; isEmergencyNumber?: boolean; + /** Age must be between 0 and 150 */ age: string | number; + /** Patient name must not be empty */ name: string; + /** Gender must be one of: Male, Female, Other */ gender: string; address: string; + /** 6-digit pincode */ pincode: string; // ... rest of the interface
54-62: Improve method naming for better readabilityThe method name
clickCreateConsultationOnPatientPageWithNoConsultationis too verbose. Consider a more concise name that still conveys the intent.- clickCreateConsultationOnPatientPageWithNoConsultation() { + initiateNewConsultation() { cy.get("#create-consultation").should("be.visible").click(); }
283-342: Add error handling and debug loggingWhile the method is well-structured, it could benefit from error handling and debug logging to help troubleshoot test failures.
createPatientWithData(data: PatientData) { + cy.log(`Creating patient: ${data.name}`); + try { this.createPatient(); this.selectFacility(data.facility); + cy.log('Basic details completed'); // ... rest of the method this.clickCreatePatient(); this.verifyPatientIsCreated(); + cy.log(`Successfully created patient: ${data.name}`); + } catch (error) { + cy.log(`Failed to create patient: ${error.message}`); + throw error; + } }cypress/e2e/facility_spec/FacilityHomepage.cy.ts (1)
68-68: Improve random number generation for patient IP numberThe current implementation might generate duplicate IP numbers. Consider using a more robust approach.
- const patientIpNumber = `${Math.floor(Math.random() * 90 + 10)}/${Math.floor(Math.random() * 9000 + 1000)}`; + const timestamp = Date.now().toString().slice(-4); + const randomNum = Math.floor(Math.random() * 90 + 10); + const patientIpNumber = `${randomNum}/${timestamp}`;cypress/pageobject/Facility/FacilityCreation.ts (2)
3-44: Consider adding input validation for the FacilityData interfaceThe interface provides a good structure, but consider adding runtime validation to ensure data consistency, especially for numerical fields and required vs optional properties.
Consider using a validation library like Zod or io-ts:
import { z } from 'zod'; const FacilityDataSchema = z.object({ basic: z.object({ name: z.string().min(1), type: z.string().min(1), features: z.array(z.string()).optional(), address: z.string().min(1), phoneNumber: z.string().regex(/^\+?[\d\s-]+$/), location: z.string().optional(), }), // ... rest of the schema });
558-625: Well-structured modular implementationThe breakdown of facility creation into specific methods (
fillBasicDetails,fillLocationDetails, etc.) follows good separation of concerns and makes the code more maintainable.Consider further enhancing reusability by:
- Extracting common form interaction patterns into shared utilities
- Adding retry mechanisms for flaky UI interactions
- Implementing custom Cypress commands for frequently used sequences
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts(4 hunks)cypress/e2e/facility_spec/FacilityLocation.cy.ts(5 hunks)cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts(4 hunks)cypress/pageobject/Facility/FacilityCreation.ts(3 hunks)cypress/pageobject/Patient/PatientCreation.ts(4 hunks)src/components/Patient/PatientHome.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cypress/e2e/facility_spec/FacilityLocation.cy.ts
- cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts
- src/components/Patient/PatientHome.tsx
🧰 Additional context used
📓 Learnings (3)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (3)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityHomepage.cy.ts:229-317
Timestamp: 2024-11-18T10:46:56.271Z
Learning: In `cypress/e2e/facility_spec/FacilityHomepage.cy.ts`, when verifying the bed capacity badge reflection, the badge remains the same during multiple actions and only updates after a bed is attached to a patient during consultation. Therefore, it's appropriate to keep the test case consolidated rather than splitting it into smaller tests.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts:93-94
Timestamp: 2024-11-18T10:48:08.501Z
Learning: In `cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts`, bed capacity verification assertions after patient admission are already being performed elsewhere, so adding them here is unnecessary.
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityCreation.cy.ts:177-180
Timestamp: 2024-11-18T10:44:30.303Z
Learning: In `cypress/e2e/facility_spec/FacilityCreation.cy.ts`, when testing bed and staff capacity individually for additional error verification, we prefer to use separate method calls to handle bed and staff capacity, rather than using `facilityPage.createNewFacility(testFacilityData)` which also handles bed management.
cypress/pageobject/Facility/FacilityCreation.ts (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityCreation.cy.ts:177-180
Timestamp: 2024-11-18T10:44:30.303Z
Learning: In `cypress/e2e/facility_spec/FacilityCreation.cy.ts`, when testing bed and staff capacity individually for additional error verification, we prefer to use separate method calls to handle bed and staff capacity, rather than using `facilityPage.createNewFacility(testFacilityData)` which also handles bed management.
cypress/pageobject/Patient/PatientCreation.ts (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts:93-94
Timestamp: 2024-11-18T10:48:08.501Z
Learning: In `cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts`, bed capacity verification assertions after patient admission are already being performed elsewhere, so adding them here is unnecessary.
🔇 Additional comments (6)
cypress/e2e/facility_spec/FacilityHomepage.cy.ts (4)
229-317: LGTM: Test case implementation aligns with requirements
The test case correctly implements the bed capacity badge verification workflow as specified in issue #9107. The implementation:
- Verifies initial badge state
- Links doctor to facility
- Creates patient
- Adds bed to facility
- Creates consultation with bed assignment
- Verifies final badge state
This aligns with the learning that the badge updates only after bed attachment during consultation.
2-2:
Fix the AssetPage import statement
The import statement for AssetPage needs to be corrected to match its usage as a class on line 29.
- import { AssetPage } from "pageobject/Asset/AssetCreation";
+ import AssetPage from "pageobject/Asset/AssetCreation";Likely invalid or redundant comment.
273-284: 🛠️ Refactor suggestion
Improve error handling in location management flow
The current implementation lacks proper error handling and explicit waiting for elements.
- cy.get("body").then(($body) => {
- if ($body.find("#manage-bed-button").length > 0) {
- facilityLocation.clickManageBedButton();
- } else {
- facilityLocation.clickAddNewLocationButton();
- facilityPage.fillFacilityName(locationName);
- facilityLocation.selectLocationType(locationType);
- assetPage.clickassetupdatebutton();
- facilityLocation.clickNotification();
- facilityLocation.clickManageBedButton();
- }
- });
+ cy.get("body")
+ .should("exist")
+ .then(($body) => {
+ const manageBedButton = $body.find("#manage-bed-button");
+ if (manageBedButton.length > 0) {
+ cy.wrap(manageBedButton)
+ .should("be.visible")
+ .then(() => facilityLocation.clickManageBedButton());
+ } else {
+ cy.log("Creating new location as manage bed button not found");
+ facilityLocation.clickAddNewLocationButton();
+ facilityPage.fillFacilityName(locationName);
+ facilityLocation.selectLocationType(locationType);
+ assetPage.clickassetupdatebutton();
+ facilityLocation.clickNotification();
+ cy.get("#manage-bed-button")
+ .should("be.visible")
+ .then(() => facilityLocation.clickManageBedButton());
+ }
+ });Likely invalid or redundant comment.
44-67:
Use obvious placeholder values in test data
The test data contains values that could be mistaken for real personal information. Use clearly fictional data to prevent any PII concerns.
const newPatientData: PatientData = {
facility: facilityWithNoAvailableBeds,
- phoneNumber: "9898464555",
+ phoneNumber: "0000000000",
isEmergencyNumber: true,
age: "20",
- name: "Dummy Patient 42",
+ name: "TEST_PATIENT_42",
gender: "Male",
- address: "42 is the answer to everything",
+ address: "TEST_ADDRESS_42",
pincode: "682001",
// ... rest of the data
};Likely invalid or redundant comment.
cypress/pageobject/Facility/FacilityCreation.ts (2)
475-476: LGTM! Good use of Cypress aliases
The refactoring improves readability and follows Cypress best practices by using aliases and proper element visibility checks.
527-556: 🛠️ Refactor suggestion
Add error handling for API failures
The createNewFacility method should handle potential API failures and form submission errors.
Consider adding error handling:
createNewFacility(data: FacilityData) {
this.visitCreateFacilityPage();
// Fill basic details
this.fillBasicDetails(data.basic);
// Fill location details
this.fillLocationDetails(data.location);
// Fill oxygen details if provided
if (data.oxygen) {
this.fillOxygenDetails(data.oxygen);
}
// Add error handling for form submission
cy.intercept('POST', '**/api/v1/facility/').as('createFacility');
this.submitForm();
// Wait for API response and handle errors
cy.wait('@createFacility').then((interception) => {
if (interception.response?.statusCode !== 201) {
throw new Error(`Failed to create facility: ${interception.response?.statusCode}`);
}
});
cy.closeNotification();
// ... rest of the method
}⛔ Skipped due to learnings
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/facility_spec/FacilityCreation.cy.ts:177-180
Timestamp: 2024-11-18T10:44:30.303Z
Learning: In `cypress/e2e/facility_spec/FacilityCreation.cy.ts`, when testing bed and staff capacity individually for additional error verification, we prefer to use separate method calls to handle bed and staff capacity, rather than using `facilityPage.createNewFacility(testFacilityData)` which also handles bed management.
| import FacilityPage from "pageobject/Facility/FacilityCreation"; | ||
|
|
||
| import PatientMedicalHistory from "./PatientMedicalHistory"; | ||
|
|
||
| let patient_url = ""; | ||
| const facilityPage = new FacilityPage(); | ||
| const patientMedicalHistory = new PatientMedicalHistory(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving instance creation to maintain test isolation
Creating instances at the class level could lead to shared state between tests. Consider moving the instance creation to either the constructor or individual methods where they're needed.
-const facilityPage = new FacilityPage();
-const patientMedicalHistory = new PatientMedicalHistory();
+class PatientPage {
+ private facilityPage: FacilityPage;
+ private patientMedicalHistory: PatientMedicalHistory;
+
+ constructor() {
+ this.facilityPage = new FacilityPage();
+ this.patientMedicalHistory = new PatientMedicalHistory();
+ }Committable suggestion skipped: line range outside the PR's diff.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
data-test-idattributes in components.Refactor