Rewriting UsersCreation.cy.ts according to POM approach #8930
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
- We use POM approach in cypress, so changes everything into a re-usable function Refer: PatientDoctorConnect.cy.ts test file
For example cy.get("#user-profile-name").click(); should be a function which is named clickUserProfileName()
Similarly, convert the entire test file to the POM approach
|
@nihal467 Declare such indiviual functions in pageobejct of user creation.ts |
|
@AnveshNalimela i have modified the issue, can you clear the rest of them ? |
|
@nihal467 made the changes accordingly by refering patientDoctorConnect.cy.ts .just have look at latest commit |
AnveshNalimela
changed the title
|
👋 Hi, @AnveshNalimela, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@AnveshNalimela as per the updated issue , you need to rewrite the entire file, read it carefully, so which means that, we need to give individual function name to each operation.
Replace all the usage of :
inshort: rewrite the entire file with new individual proper function, proper usage of command.ts functions, and keep it clean |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
cypress/e2e/users_spec/UsersManage.cy.ts (1)
Line range hint
1-168: Consider enhancing test maintainability with these suggestions.While the overall test structure is good, here are some suggestions to improve maintainability:
- Consider moving test data (usernames, facility names) to a separate fixture file
- Group related constants using interfaces/types
- Add more descriptive comments for each test case's purpose
Example structure for test data:
// cypress/fixtures/users.json { "doctors": { "linkFacility": { "doc1": "dummydoctor4", "doc2": "dummydoctor5", "doc3": "dummydoctor6" }, "linkSkill": { "username": "devdoctor", "firstName": "Dev", "lastName": "Doctor" } }, "facilities": { "shiftingCenter": "Dummy Shifting Center", "skillFacility": "Dummy Facility 40" } }cypress/e2e/users_spec/UsersCreation.cy.ts (3)
Line range hint
25-33: Replace Math.random() with a cryptographically secure alternative.Using
Math.random()for generating usernames could be predictable and potentially exploitable. Consider usingcrypto.getRandomValues()for better security.const makeId = (length: number) => { let result = ""; const characters = "abcdefghijklmnopqrstuvwxyz0123456789"; - const charactersLength = characters.length; - for (let i = 0; i < length; i++) { - result += characters.charAt(Math.floor(Math.random() * charactersLength)); - } + const array = new Uint32Array(length); + crypto.getRandomValues(array); + for (let i = 0; i < length; i++) { + result += characters.charAt(array[i] % characters.length); + } return result; };🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 17-17: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
[failure] 18-18: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
62-62: Fix typo in firstName constant value.The word "Editted" is misspelled. It should be "Edited".
-const firstName = "District Editted"; +const firstName = "District Edited";
176-179: Improve error message verification reliability.The error message verification could be more reliable by adding an explicit wait for the error messages to be visible before verifying them.
userCreationPage.clickAddUserButton(); userCreationPage.clickSaveUserButton(); -cy.get(".error-text", { timeout: 10000 }).should("be.visible"); +// Wait for all error messages to be visible +cy.get(".error-text").should("have.length", EXPECTED_ERROR_MESSAGES.length).and("be.visible"); userCreationPage.verifyErrorMessages(EXPECTED_ERROR_MESSAGES);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cypress/e2e/users_spec/UsersCreation.cy.ts(6 hunks)cypress/e2e/users_spec/UsersManage.cy.ts(1 hunks)cypress/e2e/users_spec/UsersProfile.cy.ts(1 hunks)cypress/pageobject/Users/UserCreation.ts(1 hunks)cypress/pageobject/Users/UserProfilePage.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cypress/e2e/users_spec/UsersProfile.cy.ts
- cypress/pageobject/Users/UserProfilePage.ts
🧰 Additional context used
📓 Learnings (3)
cypress/e2e/users_spec/UsersCreation.cy.ts (3)
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:16-59
Timestamp: 2024-11-12T10:23:10.322Z
Learning: In the `cypress/pageobject/Users/UserCreation.ts` file, method parameter names may use a mix of `snake_case` and `camelCase` naming conventions, and this is acceptable in the codebase.
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:31-42
Timestamp: 2024-11-12T10:23:10.322Z
Learning: In `UserCreationPage` of `UserCreation.ts`, methods like `typePhoneNumber` and `typeUserPhoneNumber` target different input fields and are both necessary.
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:3-5
Timestamp: 2024-11-15T05:22:27.913Z
Learning: In the `clickProfileName()` method of the `UserCreationPage` class, the profile name is dynamic and cannot be verified using a static text. Therefore, `cy.get("#user-profile-name").click();` should be used instead of `cy.verifyAndClickElement`.
cypress/e2e/users_spec/UsersManage.cy.ts (2)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9080
File: cypress/pageobject/Users/ManageUserPage.ts:67-97
Timestamp: 2024-11-14T08:09:58.453Z
Learning: In Cypress test files (`cypress/pageobject/Users/ManageUserPage.ts`), when methods are called to check input provided earlier in the test, adding TypeScript interfaces, input validation, and custom error messages is unnecessary.
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:3-5
Timestamp: 2024-11-15T05:22:27.913Z
Learning: In the `clickProfileName()` method of the `UserCreationPage` class, the profile name is dynamic and cannot be verified using a static text. Therefore, `cy.get("#user-profile-name").click();` should be used instead of `cy.verifyAndClickElement`.
cypress/pageobject/Users/UserCreation.ts (3)
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:31-42
Timestamp: 2024-11-12T10:23:10.322Z
Learning: In `UserCreationPage` of `UserCreation.ts`, methods like `typePhoneNumber` and `typeUserPhoneNumber` target different input fields and are both necessary.
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:3-5
Timestamp: 2024-11-15T05:22:27.913Z
Learning: In the `clickProfileName()` method of the `UserCreationPage` class, the profile name is dynamic and cannot be verified using a static text. Therefore, `cy.get("#user-profile-name").click();` should be used instead of `cy.verifyAndClickElement`.
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#8930
File: cypress/pageobject/Users/UserCreation.ts:16-59
Timestamp: 2024-11-12T10:23:10.322Z
Learning: In the `cypress/pageobject/Users/UserCreation.ts` file, method parameter names may use a mix of `snake_case` and `camelCase` naming conventions, and this is acceptable in the codebase.
🪛 GitHub Check: CodeQL
cypress/e2e/users_spec/UsersCreation.cy.ts
[failure] 17-17: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
[failure] 18-18: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
🔇 Additional comments (8)
cypress/pageobject/Users/UserCreation.ts (2)
3-4: LGTM! Click methods use appropriate reusable commands.
The click methods correctly utilize reusable commands from commands.ts:
verifyAndClickElementfor Add User buttonclickSubmitButtonfor Save User button
Also applies to: 48-49
21-25: 🛠️ Refactor suggestion
Use cy.clearInput from commands.ts for consistency.
The clear methods should use the reusable command for better maintainability.
Apply this pattern to both clear methods:
clearFirstName() {
- cy.get("#firstName").click().clear();
+ cy.clearInput("#firstName");
}Likely invalid or redundant comment.
cypress/e2e/users_spec/UsersManage.cy.ts (2)
49-51: LGTM! Good use of reusable component.
The change from userCreationPage.verifyElementContainsText to cy.verifyContentPresence aligns well with the PR's objective of utilizing reusable components from commands.ts.
49-51: Verify the custom command implementation.
Let's ensure the verifyContentPresence command is properly implemented in commands.ts.
✅ Verification successful
Custom command verifyContentPresence is properly implemented
The command is correctly implemented in cypress/support/commands.ts with proper TypeScript support. It:
- Takes a selector and an array of texts as parameters
- Uses
cy.get()to find elements by selector - Verifies each text in the array is present within the found element
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if verifyContentPresence is properly implemented in commands.ts
# Expected: Should find the command definition with proper TypeScript types
# Search for verifyContentPresence command definition
echo "Searching for verifyContentPresence command definition..."
rg -A 5 "verifyContentPresence" "cypress/support/commands.ts"
# Search for any direct usage of cy.get with username-profile-details
echo "Checking for any direct DOM queries that could be consolidated..."
rg "cy\.get.*username-profile-details"
Length of output: 558
cypress/e2e/users_spec/UsersCreation.cy.ts (4)
Line range hint 1-21: LGTM! Clean setup with proper page object instantiation.
The imports are well-organized and the page objects are correctly instantiated following the Page Object Model pattern.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 17-17: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
[failure] 18-18: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
Line range hint 81-92: LGTM! Well-structured test lifecycle hooks.
The hooks properly handle authentication and local storage management.
110-112: LGTM! Proper API interception implementation.
The API calls are properly intercepted and verified, following Cypress best practices for handling asynchronous operations.
142-172: LGTM! Well-structured user creation test.
The test follows a clear arrangement of actions and assertions, with proper API interception and comprehensive verification of the created user's details.
| selectFacility(name: string) { | ||
| cy.typeAndSelectOption("input[name='facilities']", name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use typeAndMultiSelectOption for facility selection.
As suggested in the past reviews, facility selection should use the multi-select option command.
selectFacility(name: string) {
- cy.typeAndSelectOption("input[name='facilities']", name);
+ cy.typeAndMultiSelectOption("input[name='facilities']", name);
}📝 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.
| selectFacility(name: string) { | |
| cy.typeAndSelectOption("input[name='facilities']", name); | |
| selectFacility(name: string) { | |
| cy.typeAndMultiSelectOption("input[name='facilities']", name); |
| selectState(state: string) { | ||
| cy.clickAndSelectOption("#state", state); | ||
| } | ||
| selectDistrict(district: string) { | ||
| cy.clickAndSelectOption("#district", district); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can generate the final response:
Update selectors to match the actual DOM elements
The current selectors #state and #district need to be updated. From the codebase search results, particularly in cypress/pageobject/Users/UserSearch.ts, the correct selectors are:
#state inputfor state selection#district inputfor district selection
These elements appear to be autocomplete inputs rather than simple dropdowns.
🔗 Analysis chain
Verify state and district element IDs.
The selectors #state and #district might need to be updated to match the actual element IDs in the DOM.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the correct element IDs for state and district dropdowns
echo "Searching for state and district element IDs..."
rg -A 2 'id=["\'](state|district)["\']' src/
Length of output: 332
Script:
#!/bin/bash
# Search for state and district element IDs in all files
rg -A 2 'id=["\'']?(state|district)["\'']?'
# Search for any React/JSX components that might contain state or district dropdowns
ast-grep --pattern 'Select id="$_" $$$'
# Search for any form fields or components with state or district in their name
rg -i "(state|district).*(select|dropdown|field|input)"
Length of output: 14898
| typeUserName(username: string) { | ||
| cy.get("#username").click().type(username); | ||
| } | ||
|
|
||
| typeIntoElementByIdPostClear(elementId: string, value: string) { | ||
| cy.get("#" + elementId) | ||
| .click() | ||
| .clear() | ||
| .click() | ||
| .type(value); | ||
| typeFirstName(firstName: string) { | ||
| cy.get("#firstName").click().type(firstName); | ||
| } | ||
| typeIntoElementByIdPostClearDob(elementId: string, value: string) { | ||
| cy.clickAndTypeDate("#" + elementId, value); | ||
| typeLastName(lastName: string) { | ||
| cy.get("#lastName").click().type(lastName); | ||
| } | ||
| clearIntoElementById(elementId: string) { | ||
| cy.get("#" + elementId) | ||
| .click() | ||
| .clear(); | ||
| typePassword(password: string) { | ||
| cy.get("#password").click().type(password); | ||
| } | ||
|
|
||
| typeIntoInputByName(inputName: string, value: string) { | ||
| cy.get("input[name='" + inputName + "']") | ||
| .click() | ||
| .type(value); | ||
| typeConfirmPassword(password: string) { | ||
| cy.get("#c_password").click().type(password); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use cy.typeIntoInput from commands.ts for consistency.
The type methods should use the reusable command for better maintainability.
Apply this pattern to all type methods:
typeUserName(username: string) {
- cy.get("#username").click().type(username);
+ cy.typeIntoInput("#username", username);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cypress/pageobject/utils/constants.ts(1 hunks)
🔇 Additional comments (1)
cypress/pageobject/utils/constants.ts (1)
8-9: Consider the necessity of a separate emergency phone function
The current implementation doesn't differentiate emergency numbers from regular phone numbers. Consider either:
- Adding distinct validation/formatting for emergency contacts, or
- Removing this wrapper function if no special handling is needed
| export function generatePhoneNumber(): string { | ||
| const array = new Uint32Array(1); | ||
| window.crypto.getRandomValues(array); | ||
| const randomNum = (array[0] % 900000000) + 100000000; | ||
| return "9" + randomNum.toString(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve code clarity
While the switch to crypto.getRandomValues() is a good security improvement, consider these enhancements:
+const PHONE_PREFIX = '9';
+const MIN_PHONE_LENGTH = 9;
+
export function generatePhoneNumber(): string {
+ try {
+ if (!window.crypto || !window.crypto.getRandomValues) {
+ throw new Error('Crypto API not supported');
+ }
const array = new Uint32Array(1);
window.crypto.getRandomValues(array);
- const randomNum = (array[0] % 900000000) + 100000000;
- return "9" + randomNum.toString();
+ // Ensure exactly 9 digits (excluding prefix)
+ const randomNum = (array[0] % 900000000) + 100000000;
+ return PHONE_PREFIX + randomNum.toString();
+ } catch (error) {
+ Cypress.log({
+ name: 'Phone Generation Error',
+ message: error.message
+ });
+ throw error;
+ }
}📝 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 function generatePhoneNumber(): string { | |
| const array = new Uint32Array(1); | |
| window.crypto.getRandomValues(array); | |
| const randomNum = (array[0] % 900000000) + 100000000; | |
| return "9" + randomNum.toString(); | |
| const PHONE_PREFIX = '9'; | |
| const MIN_PHONE_LENGTH = 9; | |
| export function generatePhoneNumber(): string { | |
| try { | |
| if (!window.crypto || !window.crypto.getRandomValues) { | |
| throw new Error('Crypto API not supported'); | |
| } | |
| const array = new Uint32Array(1); | |
| window.crypto.getRandomValues(array); | |
| // Ensure exactly 9 digits (excluding prefix) | |
| const randomNum = (array[0] % 900000000) + 100000000; | |
| return PHONE_PREFIX + randomNum.toString(); | |
| } catch (error) { | |
| Cypress.log({ | |
| name: 'Phone Generation Error', | |
| message: error.message | |
| }); | |
| throw error; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cypress/e2e/facility_spec/FacilityLocation.cy.ts (2)
39-39: Consider using a more descriptive constant nameThe constant name
duplicateBedNameimplies it's used for testing duplicate bed scenarios, but it's actually used for bed modification testing. Consider renaming it tomodifiedBedNameorupdatedBedNameto better reflect its purpose.
Line range hint
71-109: Consider breaking down the large test case into smaller, focused testsThe test case "Add a Bed to facility location along with duplication and deleting a bed" combines multiple scenarios:
- Mandatory field validation
- Single bed creation
- Duplicate bed validation
- Bed modification
Consider splitting these into separate test cases for better maintainability and clearer failure isolation. This aligns with testing best practices where each test should verify a single piece of functionality.
Example structure:
it("should validate mandatory fields for bed creation") it("should successfully create a single bed") it("should prevent creation of duplicate beds") it("should successfully modify existing bed details")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
cypress/e2e/facility_spec/FacilityLocation.cy.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
cypress/e2e/facility_spec/FacilityLocation.cy.ts (2)
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/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.
🔇 Additional comments (2)
cypress/e2e/facility_spec/FacilityLocation.cy.ts (2)
Line range hint 1-277: Implementation looks good overall!
The changes maintain the existing test coverage while updating the bed modification scenario. The code follows good testing practices with proper use of Page Object Model, appropriate assertions, and comprehensive error handling.
100-107: Verify the test coverage for bed name modification
The test case modifies a bed's name to "ICCU" but doesn't explicitly verify that this isn't a duplicate name in the system. Consider adding a verification step to ensure the bed name modification succeeds without any duplicate name conflicts.
Additionally, consider adding the following test scenarios:
- Attempt to modify a bed name to an existing name
- Verify appropriate error messages for duplicate bed names during modification
nihal467
added
tested
reviewed
|
@AnveshNalimela 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! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation