Tests to verify error handling and access restrictions during Facility Creation #9119
Conversation
WalkthroughThe changes introduced in this pull request enhance the Cypress end-to-end tests for facility creation by adding new test cases and methods to improve error handling. A constant for non-admin user credentials is introduced, and two test cases are implemented: one for district admin error handling and another for access restrictions for non-admin users. Additionally, a new method to verify error notifications is added 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
cypress/e2e/facility_spec/FacilityCreation.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: 3
🧹 Outside diff range and nitpick comments (2)
cypress/pageobject/Facility/FacilityCreation.ts (2)
305-307: LGTM with suggestions for enhancementThe implementation is consistent with existing patterns and serves the PR's objectives. Consider these enhancements:
- Add TypeScript type information:
- verifyErrorNotification(message: string) { + verifyErrorNotification(message: string): void {
- Consider consolidating notification verification to reduce duplication:
+ private verifyNotification(message: string, type: 'success' | 'error'): void { + cy.verifyNotification(message); + } + - verifySuccessNotification(message: string) { - cy.verifyNotification(message); - } + verifySuccessNotification(message: string): void { + this.verifyNotification(message, 'success'); + } + - verifyErrorNotification(message: string) { - cy.verifyNotification(message); - } + verifyErrorNotification(message: string): void { + this.verifyNotification(message, 'error'); + }
- Add error message constants to improve maintainability:
export const ERROR_MESSAGES = { DISTRICT_ACCESS_DENIED: 'You are not authorized to create facility outside your district', NON_ADMIN_ACCESS_DENIED: 'You do not have permission to access this page' } as const;
305-307: Consider enhancing error handling architectureTo better support the PR's focus on error handling and access control, consider these architectural improvements:
- Create a dedicated error handling section in the class:
// Error Handling Methods /** * Verifies access control and error notifications */ interface IAccessControl { verifyErrorNotification(message: string): void; verifyAccessDenied(): void; verifyDistrictAccessDenied(): void; } // Implementation examples: verifyAccessDenied(): void { cy.url().should('include', '/403'); this.verifyErrorNotification(ERROR_MESSAGES.NON_ADMIN_ACCESS_DENIED); } verifyDistrictAccessDenied(): void { this.verifyErrorNotification(ERROR_MESSAGES.DISTRICT_ACCESS_DENIED); }
- Add JSDoc documentation for error scenarios:
/** * Verifies error notifications during facility creation * @param message - The expected error message * @example * // Verify district access error * facilityPage.verifyErrorNotification(ERROR_MESSAGES.DISTRICT_ACCESS_DENIED); */ verifyErrorNotification(message: string): void { cy.verifyNotification(message); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/e2e/facility_spec/FacilityCreation.cy.ts(2 hunks)cypress/pageobject/Facility/FacilityCreation.ts(1 hunks)
| const nonAdminUsers = [ | ||
| { username: "dummynurse1", password: "Coronasafe@123" }, | ||
| { username: "devdoctor", password: "Coronasafe@123" }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving credentials to environment variables or test fixtures
While the constant is well-structured, storing credentials directly in test files is not recommended. Consider moving these credentials to:
- Cypress environment variables (
cypress.env.json) - Test fixtures (
cypress/fixtures/users.json)
This approach improves security and makes credential management more maintainable.
- const nonAdminUsers = [
- { username: "dummynurse1", password: "Coronasafe@123" },
- { username: "devdoctor", password: "Coronasafe@123" },
- ];And create a fixture file cypress/fixtures/users.json:
{
"nonAdminUsers": [
{ "username": "dummynurse1", "password": "Coronasafe@123" },
{ "username": "devdoctor", "password": "Coronasafe@123" }
]
}Then use it in the test:
let nonAdminUsers: Array<{username: string, password: string}>;
before(() => {
cy.fixture('users').then((users) => {
nonAdminUsers = users.nonAdminUsers;
});
});| it("Access Restriction for Non-Admin Users to facility creation page", () => { | ||
| nonAdminUsers.forEach((user) => { | ||
| loginPage.login(user.username, user.password); | ||
| cy.visit("/facility/create"); | ||
|
|
||
| facilityPage.verifyErrorNotification( | ||
| "You don't have permission to perform this action. Contact the admin", | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test reliability and API verification
The test efficiently checks multiple users but could be improved in several ways:
- Add session cleanup between users
- Verify API requests
- Create a custom command for the repetitive login/verification flow
it("Access Restriction for Non-Admin Users to facility creation page", () => {
+ cy.intercept('GET', '**/api/v1/facility/create/').as('accessFacility');
nonAdminUsers.forEach((user) => {
loginPage.login(user.username, user.password);
cy.visit("/facility/create");
+ cy.wait('@accessFacility').then((interception) => {
+ expect(interception.response.statusCode).to.equal(403);
+ });
facilityPage.verifyErrorNotification(
"You don't have permission to perform this action. Contact the admin",
);
+ cy.clearCookies(); // Clean up session
+ cy.clearLocalStorage();
});
});Consider creating a custom command for better reusability:
// In cypress/support/commands.ts
Cypress.Commands.add('verifyNoFacilityAccess', (username: string, password: string) => {
cy.intercept('GET', '**/api/v1/facility/create/').as('accessFacility');
loginPage.login(username, password);
cy.visit("/facility/create");
cy.wait('@accessFacility').its('response.statusCode').should('eq', 403);
facilityPage.verifyErrorNotification(
"You don't have permission to perform this action. Contact the admin"
);
cy.clearCookies();
cy.clearLocalStorage();
});
// In test file
it("Access Restriction for Non-Admin Users to facility creation page", () => {
nonAdminUsers.forEach((user) => {
cy.verifyNoFacilityAccess(user.username, user.password);
});
});| it("Should display error when district admin tries to create facility in a different district", () => { | ||
| facilityPage.visitCreateFacilityPage(); | ||
| facilityPage.fillFacilityName(facilityName); | ||
| facilityPage.selectFacilityType(facilityType); | ||
| facilityPage.fillPincode("682001"); | ||
| facilityPage.selectStateOnPincode("Kerala"); | ||
| facilityPage.selectDistrictOnPincode("Kottayam"); | ||
| facilityPage.selectLocalBody("Arpookara"); | ||
| facilityPage.selectWard("5"); | ||
| facilityPage.fillAddress(facilityAddress); | ||
| facilityPage.fillPhoneNumber(facilityNumber); | ||
| facilityPage.submitForm(); | ||
| facilityPage.verifyErrorNotification( | ||
| "You do not have permission to perform this action.", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API request verification using cy.intercept()
The test effectively verifies the UI behavior, but according to the PR objectives, it should also verify the API request. Consider adding cy.intercept() to ensure the backend request is made with the correct payload and the error originates from the API.
it("Should display error when district admin tries to create facility in a different district", () => {
+ cy.intercept('POST', '**/api/v1/facility/create/').as('createFacility');
facilityPage.visitCreateFacilityPage();
facilityPage.fillFacilityName(facilityName);
// ... existing code ...
facilityPage.submitForm();
+ cy.wait('@createFacility').then((interception) => {
+ expect(interception.response.statusCode).to.equal(403);
+ expect(interception.response.body.detail).to.include('You do not have permission');
+ });
facilityPage.verifyErrorNotification(
"You do not have permission to perform this action.",
);
});📝 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.
| it("Should display error when district admin tries to create facility in a different district", () => { | |
| facilityPage.visitCreateFacilityPage(); | |
| facilityPage.fillFacilityName(facilityName); | |
| facilityPage.selectFacilityType(facilityType); | |
| facilityPage.fillPincode("682001"); | |
| facilityPage.selectStateOnPincode("Kerala"); | |
| facilityPage.selectDistrictOnPincode("Kottayam"); | |
| facilityPage.selectLocalBody("Arpookara"); | |
| facilityPage.selectWard("5"); | |
| facilityPage.fillAddress(facilityAddress); | |
| facilityPage.fillPhoneNumber(facilityNumber); | |
| facilityPage.submitForm(); | |
| facilityPage.verifyErrorNotification( | |
| "You do not have permission to perform this action.", | |
| ); | |
| }); | |
| it("Should display error when district admin tries to create facility in a different district", () => { | |
| cy.intercept('POST', '**/api/v1/facility/create/').as('createFacility'); | |
| facilityPage.visitCreateFacilityPage(); | |
| facilityPage.fillFacilityName(facilityName); | |
| facilityPage.selectFacilityType(facilityType); | |
| facilityPage.fillPincode("682001"); | |
| facilityPage.selectStateOnPincode("Kerala"); | |
| facilityPage.selectDistrictOnPincode("Kottayam"); | |
| facilityPage.selectLocalBody("Arpookara"); | |
| facilityPage.selectWard("5"); | |
| facilityPage.fillAddress(facilityAddress); | |
| facilityPage.fillPhoneNumber(facilityNumber); | |
| facilityPage.submitForm(); | |
| cy.wait('@createFacility').then((interception) => { | |
| expect(interception.response.statusCode).to.equal(403); | |
| expect(interception.response.body.detail).to.include('You do not have permission'); | |
| }); | |
| facilityPage.verifyErrorNotification( | |
| "You do not have permission to perform this action.", | |
| ); | |
| }); |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes