Added cypress tests for Facility cover image button functionality #9134
Conversation
WalkthroughThis pull request introduces a new Cypress test suite for the "Facility Cover Image Functionality," enhancing the testing framework for facility management. It adds a structured approach to test various user roles and their interactions with the cover image modal, including uploading, editing, and deleting images. Additionally, it removes a previously non-functional test case related to the cover image button. Updates to the Changes
Assessment against linked issues
Possibly related PRs
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 (1.23.1)
cypress/e2e/facility_spec/FacilityCoverImage.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. |
|
@rohitlingarker since the PR is under draft removing the review PR |
|
We should consider making each test atomic i.e., 1 test shouldn't influence the other, as this is making debugging and developing harder. We should clean up after or during each test and create the necessary resources before or during the test. There is no need to refactor anything for now, maybe we should enforce this for newer tests. @nihal467 thoughts? |
this way like @khavinshankar mention they will be independent test |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (4)
cypress/e2e/facility_spec/FacilityCoverImage.cy.ts (1)
4-7: Consider improving test data managementThe hard-coded facility names could make tests brittle and harder to maintain. Consider:
- Moving test data to a separate fixture file
- Adding TypeScript interfaces for better type safety
+interface UserFacility { + username: string; + facilityName: string; +} + -const roleUserFacilities = [ +const roleUserFacilities: UserFacility[] = [ { username: "devdistrictadmin", facilityName: "Dummy Facility 40" }, { username: "devdoctor", facilityName: "Dummy Facility 4" }, ];cypress/pageobject/Facility/FacilityManage.ts (1)
1-1: Consider adding test data management methodsBased on the PR objectives emphasizing test atomicity and proper test data management, consider adding methods to this page object for:
- Creating a new facility with cover image for testing
- Cleaning up test data after each test
- Verifying the state of cover image data
This will help ensure tests are truly independent and maintain their own test data.
src/components/Common/AvatarEditModal.tsx (2)
Line range hint
291-303: Consider adding test IDs to other interactive elements.While the save and delete buttons now have IDs, other interactive elements could benefit from similar identifiers to maintain consistent testability. Consider adding test IDs to:
- Upload image label/input
- Camera open button
- Cancel button
- Switch camera button
- Capture button
Example additions:
<label - id="upload-cover-image" + id="upload-cover-image-label" className="button-size-default button-shape-square button-primary-default inline-flex h-min w-full cursor-pointer items-center justify-center gap-2 whitespace-pre font-medium shadow outline-offset-1 transition-all duration-200 ease-in-out enabled:hover:shadow-md disabled:cursor-not-allowed disabled:bg-secondary-200 disabled:text-secondary-500" > <input + id="upload-cover-image-input" title="changeFile" type="file" accept="image/*" className="hidden" onChange={onSelectFile} /> </label>
Line range hint
1-450: Enhance accessibility attributes for better user experience.While reviewing the component for testing improvements, I noticed some accessibility enhancements that could be made:
- Add aria-labels to buttons and inputs
- Add role attributes where appropriate
- Add aria-live regions for status updates
Example improvements:
<ButtonV2 id="delete-cover-image" + aria-label={`${t("delete")} ${t("cover_image")}`} onClick={deleteAvatar} disabled={isProcessing} > {t("delete")} </ButtonV2> <div + role="status" + aria-live="polite" className="flex flex-col gap-2 pt-4 sm:flex-row" > {isProcessing ? `${t("uploading")}...` : null} </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
cypress/fixtures/facility-cover-image.jpgis excluded by!**/*.jpgcypress/fixtures/facility-cover-image1.jpgis excluded by!**/*.jpgcypress/fixtures/new-facility-cover-image.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
cypress/e2e/facility_spec/FacilityCoverImage.cy.ts(1 hunks)cypress/e2e/facility_spec/FacilityManage.cy.ts(0 hunks)cypress/pageobject/Facility/FacilityManage.ts(2 hunks)src/components/Common/AvatarEditModal.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- cypress/e2e/facility_spec/FacilityManage.cy.ts
🔇 Additional comments (3)
cypress/e2e/facility_spec/FacilityCoverImage.cy.ts (1)
1-56: Verify alignment with PR objectives
The implementation aligns with the comment suggestions to create a new test file, but some requirements from the PR objectives need attention:
- API intercepts are missing (addressed in previous comments)
- Test atomicity could be improved with better cleanup
- Consider adding more specific test cases for different user roles beyond just verifying the same workflow
cypress/pageobject/Facility/FacilityManage.ts (1)
17-17: LGTM!
Good addition of scrollIntoView() to ensure the element is in the viewport before checking visibility.
src/components/Common/AvatarEditModal.tsx (1)
Line range hint 286-290: LGTM! The added button ID enhances testability.
The addition of id="delete-cover-image" aligns with the PR objectives and facilitates Cypress test selection.
| before(() => { | ||
| cy.loginByApi(username, "Coronasafe@123"); | ||
| cy.saveLocalStorage(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for login failures
The login step should handle potential failures gracefully.
before(() => {
- cy.loginByApi(username, "Coronasafe@123");
+ cy.loginByApi(username, "Coronasafe@123").then((response) => {
+ expect(response.status).to.eq(200);
+ });
cy.saveLocalStorage();
});📝 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.
| before(() => { | |
| cy.loginByApi(username, "Coronasafe@123"); | |
| cy.saveLocalStorage(); | |
| }); | |
| before(() => { | |
| cy.loginByApi(username, "Coronasafe@123").then((response) => { | |
| expect(response.status).to.eq(200); | |
| }); | |
| cy.saveLocalStorage(); | |
| }); |
| it("Facility Cover Image button functionality", () => { | ||
| // It's only button functionality because we can't access S3 bucket in local | ||
| // Check: Cancel button functionality | ||
| facilityManage.clickCoverImage(); | ||
| facilityManage.clickCancelButton(); | ||
| facilityManage.verifyCoverImageModalClosed(); | ||
| // Check: upload an image, verifying the API response. | ||
| facilityManage.clickCoverImage(); | ||
| facilityManage.verifyUploadButtonVisible(); | ||
| facilityManage.uploadCoverImage("facility-cover-image.jpg"); | ||
| facilityManage.clickSaveCoverImage(); | ||
| facilityManage.verifyCoverImageModalClosed(); | ||
| // //Check: Edit the current cover image by uploading a new one, and verify the API response. | ||
| facilityManage.clickCoverImage(); | ||
| facilityManage.verifyUploadButtonVisible(); | ||
| facilityManage.uploadCoverImage("new-facility-cover-image.jpg"); | ||
| facilityManage.clickSaveCoverImage(); | ||
| facilityManage.verifyCoverImageModalClosed(); | ||
| //Check: Delete the existing cover image, confirming the API response. | ||
| facilityManage.clickCoverImage(); | ||
| facilityManage.clickDeleteCoverImage(); | ||
| }); |
There was a problem hiding this comment.
Enhance test assertions and API verifications
The test case lacks proper assertions after API calls and UI state verifications.
Add assertions and API verifications:
it("Facility Cover Image button functionality", () => {
- // It's only button functionality because we can't access S3 bucket in local
+ // Note: This test verifies UI interactions and API calls without actual S3 bucket operations
+ // as S3 access is not available in local development environment
// Check: Cancel button functionality
facilityManage.clickCoverImage();
facilityManage.clickCancelButton();
facilityManage.verifyCoverImageModalClosed();
// Check: upload an image, verifying the API response.
facilityManage.clickCoverImage();
facilityManage.verifyUploadButtonVisible();
facilityManage.uploadCoverImage("facility-cover-image.jpg");
facilityManage.clickSaveCoverImage();
+ cy.wait('@uploadCoverImage').its('response.statusCode').should('eq', 200);
facilityManage.verifyCoverImageModalClosed();
+ facilityManage.verifyCoverImageVisible();
// Check: Edit the current cover image
facilityManage.clickCoverImage();
facilityManage.verifyUploadButtonVisible();
facilityManage.uploadCoverImage("new-facility-cover-image.jpg");
facilityManage.clickSaveCoverImage();
+ cy.wait('@uploadCoverImage').its('response.statusCode').should('eq', 200);
facilityManage.verifyCoverImageModalClosed();
+ facilityManage.verifyCoverImageVisible();
// Check: Delete the existing cover image
facilityManage.clickCoverImage();
facilityManage.clickDeleteCoverImage();
+ cy.wait('@deleteCoverImage').its('response.statusCode').should('eq', 200);
+ facilityManage.verifyCoverImageNotVisible();
});Committable suggestion skipped: line range outside the PR's diff.
| afterEach(() => { | ||
| cy.saveLocalStorage(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper test cleanup
The test should clean up any resources it creates to maintain test isolation.
afterEach(() => {
cy.saveLocalStorage();
+ // Clean up any uploaded images to maintain test isolation
+ cy.get('@deleteCoverImage').then((interception) => {
+ if (interception) {
+ facilityManage.clickCoverImage();
+ facilityManage.clickDeleteCoverImage();
+ cy.wait('@deleteCoverImage');
+ }
+ });
});Committable suggestion skipped: line range outside the PR's diff.
| beforeEach(() => { | ||
| cy.viewport(1280, 720); | ||
| cy.restoreLocalStorage(); | ||
| cy.clearLocalStorage(/filters--.+/); | ||
| cy.awaitUrl("/"); | ||
| facilityPage.typeFacilitySearch(facilityName); | ||
| facilityPage.verifyFacilityBadgeContent(facilityName); | ||
| facilityPage.visitAlreadyCreatedFacility(); | ||
| }); |
There was a problem hiding this comment.
Implement API intercepts as specified in PR objectives
The PR objectives mention using cy.intercept() for API verification, but these are missing in the implementation.
Add API intercepts before navigation:
beforeEach(() => {
cy.viewport(1280, 720);
cy.restoreLocalStorage();
cy.clearLocalStorage(/filters--.+/);
+ cy.intercept('GET', '/api/v1/facility/*').as('getFacility');
+ cy.intercept('POST', '/api/v1/facility/*/cover_image').as('uploadCoverImage');
+ cy.intercept('DELETE', '/api/v1/facility/*/cover_image').as('deleteCoverImage');
cy.awaitUrl("/");
facilityPage.typeFacilitySearch(facilityName);
facilityPage.verifyFacilityBadgeContent(facilityName);
facilityPage.visitAlreadyCreatedFacility();
+ cy.wait('@getFacility');
});📝 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.
| beforeEach(() => { | |
| cy.viewport(1280, 720); | |
| cy.restoreLocalStorage(); | |
| cy.clearLocalStorage(/filters--.+/); | |
| cy.awaitUrl("/"); | |
| facilityPage.typeFacilitySearch(facilityName); | |
| facilityPage.verifyFacilityBadgeContent(facilityName); | |
| facilityPage.visitAlreadyCreatedFacility(); | |
| }); | |
| beforeEach(() => { | |
| cy.viewport(1280, 720); | |
| cy.restoreLocalStorage(); | |
| cy.clearLocalStorage(/filters--.+/); | |
| cy.intercept('GET', '/api/v1/facility/*').as('getFacility'); | |
| cy.intercept('POST', '/api/v1/facility/*/cover_image').as('uploadCoverImage'); | |
| cy.intercept('DELETE', '/api/v1/facility/*/cover_image').as('deleteCoverImage'); | |
| cy.awaitUrl("/"); | |
| facilityPage.typeFacilitySearch(facilityName); | |
| facilityPage.verifyFacilityBadgeContent(facilityName); | |
| facilityPage.visitAlreadyCreatedFacility(); | |
| cy.wait('@getFacility'); | |
| }); |
| clickCancelButton() { | ||
| cy.get("#cancel").scrollIntoView().should("be.visible").click(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more specific selector for the cancel button
The current selector '#cancel' is too generic and could match unintended elements on the page. Consider using a more specific selector that includes the context of the cover image modal.
- cy.get("#cancel").scrollIntoView().should("be.visible").click();
+ cy.get("[data-testid='cover-image-modal-cancel']").scrollIntoView().should("be.visible").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.
| clickCancelButton() { | |
| cy.get("#cancel").scrollIntoView().should("be.visible").click(); | |
| } | |
| clickCancelButton() { | |
| cy.get("[data-testid='cover-image-modal-cancel']").scrollIntoView().should("be.visible").click(); | |
| } |
| verifyCoverImageModalClosed() { | ||
| cy.get("#upload-cover-image").should("not.exist"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify modal closure using the modal container
The current implementation checks for the upload button's non-existence, which might not accurately represent whether the modal is closed. Consider checking the modal container element instead.
- cy.get("#upload-cover-image").should("not.exist");
+ cy.get("[data-testid='cover-image-modal']").should("not.exist");Committable suggestion skipped: line range outside the PR's diff.
| cy.intercept("POST", "**/api/v1/facility/**").as("uploadCoverImage"); | ||
| cy.get("#save-cover-image").scrollIntoView().click(); | ||
| cy.wait("@uploadCoverImage").its("response.statusCode").should("eq", 200); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve API intercept and error handling
The current implementation has several areas for improvement:
- The API route wildcard could match unintended requests
- No handling of failed uploads
- Multiple responsibilities in a single method
- cy.intercept("POST", "**/api/v1/facility/**").as("uploadCoverImage");
- cy.get("#save-cover-image").scrollIntoView().click();
- cy.wait("@uploadCoverImage").its("response.statusCode").should("eq", 200);
+ cy.intercept("POST", "**/api/v1/facility/*/cover_image").as("uploadCoverImage");
+ cy.get("#save-cover-image").scrollIntoView().should("be.visible").click();
+ cy.wait("@uploadCoverImage").then((interception) => {
+ if (interception.response?.statusCode !== 200) {
+ throw new Error(`Failed to upload cover image: ${interception.response?.statusCode}`);
+ }
+ });📝 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.
| cy.intercept("POST", "**/api/v1/facility/**").as("uploadCoverImage"); | |
| cy.get("#save-cover-image").scrollIntoView().click(); | |
| cy.wait("@uploadCoverImage").its("response.statusCode").should("eq", 200); | |
| } | |
| cy.intercept("POST", "**/api/v1/facility/*/cover_image").as("uploadCoverImage"); | |
| cy.get("#save-cover-image").scrollIntoView().should("be.visible").click(); | |
| cy.wait("@uploadCoverImage").then((interception) => { | |
| if (interception.response?.statusCode !== 200) { | |
| throw new Error(`Failed to upload cover image: ${interception.response?.statusCode}`); | |
| } | |
| }); |
| clickDeleteCoverImage() { | ||
| cy.intercept("DELETE", "**/api/v1/facility/**").as("deleteCoverImage"); | ||
| cy.get("#delete-cover-image").scrollIntoView(); | ||
| cy.get("#delete-cover-image").click(); | ||
| cy.wait("@deleteCoverImage").its("response.statusCode").should("eq", 204); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix duplicate scrollIntoView and improve error handling
The current implementation has several issues:
- Duplicate
scrollIntoViewcalls - The API route wildcard could match unintended requests
- No handling of failed deletions
- cy.intercept("DELETE", "**/api/v1/facility/**").as("deleteCoverImage");
- cy.get("#delete-cover-image").scrollIntoView();
- cy.get("#delete-cover-image").click();
- cy.wait("@deleteCoverImage").its("response.statusCode").should("eq", 204);
+ cy.intercept("DELETE", "**/api/v1/facility/*/cover_image").as("deleteCoverImage");
+ cy.get("#delete-cover-image").scrollIntoView().should("be.visible").click();
+ cy.wait("@deleteCoverImage").then((interception) => {
+ if (interception.response?.statusCode !== 204) {
+ throw new Error(`Failed to delete cover image: ${interception.response?.statusCode}`);
+ }
+ });📝 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.
| clickDeleteCoverImage() { | |
| cy.intercept("DELETE", "**/api/v1/facility/**").as("deleteCoverImage"); | |
| cy.get("#delete-cover-image").scrollIntoView(); | |
| cy.get("#delete-cover-image").click(); | |
| cy.wait("@deleteCoverImage").its("response.statusCode").should("eq", 204); | |
| clickDeleteCoverImage() { | |
| cy.intercept("DELETE", "**/api/v1/facility/*/cover_image").as("deleteCoverImage"); | |
| cy.get("#delete-cover-image").scrollIntoView().should("be.visible").click(); | |
| cy.wait("@deleteCoverImage").then((interception) => { | |
| if (interception.response?.statusCode !== 204) { | |
| throw new Error(`Failed to delete cover image: ${interception.response?.statusCode}`); | |
| } | |
| }); |
|
@rohitlingarker look into the code rabbit review,
|
|
@rohitlingarker any update |
|
@rohitlingarker Closing the PR due to inactivity and unassigning you from the issue. Feel free to reopen the PR and get reassigned if you are still working on it. |
Proposed Changes
id = "delete-cover-page"to the delete cover page button element.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
idattributes for buttons, improving integration with testing frameworks.