Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a cypress test to assign a volunteer to a patient #9167

Closed
wants to merge 14 commits into from
Closed

Added a cypress test to assign a volunteer to a patient #9167

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c5c6b0b
Added a cypress test to assign a volunteer to a patient
sidpg123 Nov 20, 2024
48cb8d9
Merge branch 'develop' into issues/8913/test-to-assign-a-volunteer
sidpg123 Nov 20, 2024
8ed99ed
Merge branch 'ohcnetwork:develop' into issues/8913/test-to-assign-a-v…
sidpg123 Nov 23, 2024
5be77e1
Refactored volunteer assignment logic and improved test cases
sidpg123 Nov 23, 2024
9654198
added edge case for volunteer not found
sidpg123 Nov 24, 2024
e99203b
improved searchVolunteer method
sidpg123 Nov 24, 2024
9cc5379
resolved merge conflicts
sidpg123 Nov 28, 2024
95d7dc6
Merge branch 'ohcnetwork:develop' into issues/8913/test-to-assign-a-v…
sidpg123 Dec 4, 2024
c0cc83a
Merge branch 'develop' into issues/8913/test-to-assign-a-volunteer
nihal467 Dec 10, 2024
60e3890
Merge branch 'ohcnetwork:develop' into issues/8913/test-to-assign-a-v…
sidpg123 Dec 16, 2024
3d30a92
Merge branch 'ohcnetwork:develop' into issues/8913/test-to-assign-a-v…
sidpg123 Dec 17, 2024
0dbf1e2
updated tests according to changes in frontend & used existing comman…
sidpg123 Dec 17, 2024
045f375
made suggested changes
sidpg123 Dec 17, 2024
b65f952
added a function for edge case - searching non existing name
sidpg123 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions cypress/e2e/patient_spec/PatientVolunteer.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import LoginPage from "../../pageobject/Login/LoginPage";
import { PatientConsultationPage } from "../../pageobject/Patient/PatientConsultation";
import { PatientPage } from "../../pageobject/Patient/PatientCreation";
import { PatientDetailsPage } from "../../pageobject/Patient/PatientDetails";

describe("Assign a volunteer to a patient", () => {
const loginPage = new LoginPage();
const patientPage = new PatientPage();
const patientConsultationPage = new PatientConsultationPage();
const patientDetailsPage = new PatientDetailsPage();
const patient = "Dummy Patient 16";
const volunteerName = "dummy volunteer";
const anotherVolunteerName = "Abhi Patil";

before(() => {
cy.log("Logging in as district admin");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cy.log("Logging in as district admin");
cy.log("Logging in as district admin");

remove unwanted logging

loginPage.loginAsDistrictAdmin();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
loginPage.loginAsDistrictAdmin();
loginPage.loginByRole("districtAdmin");

cy.saveLocalStorage();
});

beforeEach(() => {
cy.restoreLocalStorage();
cy.clearLocalStorage(/filters--.+/);
});

it("assigns a volunteer to a patient and checks the banner that shows the volunteer's name", () => {
cy.visit("/patients");

patientPage.visitPatient(patient);
patientConsultationPage.clickPatientDetails();

patientDetailsPage.clickAssignToAVounteer();

patientDetailsPage.selectAndAssignVolunteer(volunteerName);

patientDetailsPage.verifyVolunteerBannerIsUpdated(volunteerName);

patientDetailsPage.clickAssignToAVounteer();

patientDetailsPage.selectAndAssignVolunteer(anotherVolunteerName);

patientDetailsPage.verifyVolunteerBannerIsUpdated(anotherVolunteerName);

patientDetailsPage.clickAssignToAVounteer();

patientDetailsPage.unassignVolunteer();

patientDetailsPage.verifyBannerIsRemovedAfterUnassign();
});
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance test organization and assertions

While the test covers all required scenarios, consider these improvements:

  1. Split into smaller, focused test cases using nested describes
  2. Add explicit assertions for navigation success
  3. Add error handling for critical steps
describe("Assign a volunteer to a patient", () => {
  // ... existing setup ...

  describe("volunteer assignment workflow", () => {
    beforeEach(() => {
      cy.visit("/patients").then(() => {
        cy.log("Successfully navigated to patients page");
      });
      
      patientPage.visitPatient(patient);
      patientConsultationPage.clickPatientDetails();
    });

    it("should assign a new volunteer successfully", () => {
      patientDetailsPage.clickAssignToAVounteer();
      patientDetailsPage.selectAndAssignVolunteer(volunteerName);
      patientDetailsPage.verifyVolunteerBannerIsUpdated(volunteerName);
    });

    it("should replace existing volunteer successfully", () => {
      patientDetailsPage.clickAssignToAVounteer();
      patientDetailsPage.selectAndAssignVolunteer(anotherVolunteerName);
      patientDetailsPage.verifyVolunteerBannerIsUpdated(anotherVolunteerName);
    });

    it("should unassign volunteer successfully", () => {
      patientDetailsPage.clickAssignToAVounteer();
      patientDetailsPage.unassignVolunteer();
      patientDetailsPage.verifyBannerIsRemovedAfterUnassign();
    });
  });
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the suggestions are worth doing, like this one (break it up into smaller tests). Go ahead and make the changes accordingly @sidpg123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

});
29 changes: 29 additions & 0 deletions cypress/pageobject/Patient/PatientDetails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
export class PatientDetailsPage {
clickAssignToAVounteer() {
cy.get('button:contains("Assign to a volunteer")').click({ force: true });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo and avoid force clicks

  1. The method name contains a typo: "Vounteer" should be "Volunteer"
  2. Using force: true for clicks can make tests brittle and mask real issues. Consider:
    • Waiting for the button to be actionable
    • Using proper selectors that ensure visibility

Here's the suggested fix:

-  clickAssignToAVounteer() {
-    cy.get('button:contains("Assign to a volunteer")').click({ force: true });
+  clickAssignToVolunteer() {
+    cy.contains('button', 'Assign to a volunteer')
+      .should('be.visible')
+      .should('be.enabled')
+      .click();
📝 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.

Suggested change
export class PatientDetailsPage {
clickAssignToAVounteer() {
cy.get('button:contains("Assign to a volunteer")').click({ force: true });
}
export class PatientDetailsPage {
clickAssignToVolunteer() {
cy.contains('button', 'Assign to a volunteer')
.should('be.visible')
.should('be.enabled')
.click();
}


selectAndAssignVolunteer(volunteerName: string) {
cy.clickAndSelectOption("#assign_volunteer", volunteerName);
cy.clickSubmitButton("Assign");
cy.wait(2000);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace hardcoded wait with explicit conditions

The hardcoded cy.wait(2000) is an anti-pattern that could make tests flaky. Instead, wait for specific conditions that indicate the assignment is complete.

   selectAndAssignVolunteer(volunteerName: string) {
     cy.clickAndSelectOption("#assign_volunteer", volunteerName);
     cy.clickSubmitButton("Assign");
-    cy.wait(2000);
+    // Wait for the assignment to complete
+    cy.get('#assigned-volunteer', { timeout: 10000 })
+      .should('be.visible')
+      .should('contain.text', volunteerName);
   }
📝 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.

Suggested change
selectAndAssignVolunteer(volunteerName: string) {
cy.clickAndSelectOption("#assign_volunteer", volunteerName);
cy.clickSubmitButton("Assign");
cy.wait(2000);
}
selectAndAssignVolunteer(volunteerName: string) {
cy.clickAndSelectOption("#assign_volunteer", volunteerName);
cy.clickSubmitButton("Assign");
// Wait for the assignment to complete
cy.get('#assigned-volunteer', { timeout: 10000 })
.should('be.visible')
.should('contain.text', volunteerName);
}


verifyVolunteerBannerIsUpdated(volunteerName: string) {
cy.get("#assigned-volunteer")
.scrollIntoView()
.should("contain.text", `Assigned Volunteer:${volunteerName}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix banner text verification and remove redundant scroll

The banner text verification has formatting issues and unnecessary scrolling.

 verifyVolunteerBannerIsUpdated(volunteerName: string) {
   cy.get("#assigned-volunteer")
-    .scrollIntoView()
     .should("contain.text", 
-      `Assigned Volunteer:${volunteerName}`
+      `Assigned Volunteer: ${volunteerName}`
     );
 }
📝 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.

Suggested change
verifyVolunteerBannerIsUpdated(volunteerName: string) {
cy.get("#assigned-volunteer")
.scrollIntoView()
.should("contain.text", `Assigned Volunteer:${volunteerName}`);
}
verifyVolunteerBannerIsUpdated(volunteerName: string) {
cy.get("#assigned-volunteer")
.should("contain.text",
`Assigned Volunteer: ${volunteerName}`
);
}


unassignVolunteer() {
cy.get("#clear-button").should("be.visible").find("svg").click();
// Close the dropdown
cy.get('button[id^="headlessui-combobox-button-"]').click(); // Click the dropdown close button

cy.clickSubmitButton("Assign");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve method clarity and selector robustness

  1. The method name doesn't reflect that it also performs a reassignment
  2. The headlessui selector is brittle and might break with library updates
-  unassignVolunteer() {
+  unassignAndPrepareForReassignment() {
     cy.get("#clear-button")
       .should("be.visible")
       .find("svg")
       .click();
-    // Close the dropdown
-    cy.get('button[id^="headlessui-combobox-button-"]').click();
+    // Use a more reliable selector for the dropdown
+    cy.get('[data-testid="volunteer-dropdown"]').click();
 
     cy.clickSubmitButton("Assign");
   }

Committable suggestion skipped: line range outside the PR's diff.


verifyBannerIsRemovedAfterUnassign() {
cy.get("#assigned-volunteer").should("not.exist"); // Ensure the banner does not exist
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add timeout for banner removal verification

The verification should account for potential delays in DOM updates.

 verifyBannerIsRemovedAfterUnassign() {
-  cy.get("#assigned-volunteer").should("not.exist"); // Ensure the banner does not exist
+  cy.get("#assigned-volunteer", { timeout: 10000 }).should("not.exist");
 }
📝 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.

Suggested change
verifyBannerIsRemovedAfterUnassign() {
cy.get("#assigned-volunteer").should("not.exist"); // Ensure the banner does not exist
}
verifyBannerIsRemovedAfterUnassign() {
cy.get("#assigned-volunteer", { timeout: 10000 }).should("not.exist");
}

}
5 changes: 4 additions & 1 deletion src/components/Patient/PatientHome.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,10 @@ export const PatientHome = (props: any) => {
</p>
)}
{patientData.assigned_to_object && (
<p className="mx-2 flex-1 rounded-lg bg-primary-200 p-3 text-center font-bold text-primary-800 shadow">
<p
id="assigned-volunteer"
className="mx-2 flex-1 rounded-lg bg-primary-200 p-3 text-center font-bold text-primary-800 shadow"
>
<span className="inline">
Assigned Volunteer:
{formatName(patientData.assigned_to_object)}
Expand Down
Loading