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. Two test cases are implemented: one for district admin error handling when creating a facility outside their district 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 issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@nihal467 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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/pageobject/Facility/FacilityCreation.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'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. |
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)
| 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);
});
});There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cypress/e2e/facility_spec/FacilityCreation.cy.ts(1 hunks)cypress/pageobject/Facility/FacilityCreation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/pageobject/Facility/FacilityCreation.ts
🧰 Additional context used
📓 Learnings (1)
cypress/e2e/facility_spec/FacilityCreation.cy.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.
| it("Access Restriction for Non-Admin Users to facility creation page", () => { | ||
| const nonAdminLoginMethods = [ | ||
| loginPage.loginAsDevDoctor.bind(loginPage), | ||
| loginPage.loginAsStaff.bind(loginPage), | ||
| ]; | ||
|
|
||
| nonAdminLoginMethods.forEach((loginMethod) => { | ||
| loginMethod(); | ||
| cy.visit("/facility/create"); | ||
| facilityPage.verifyErrorNotification( | ||
| "You don't have permission to perform this action. Contact the admin", | ||
| ); | ||
| cy.clearCookies(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test structure and API verification
While the test case handles session cleanup well, it needs improvements in several areas:
- Add API request verification
- Decouple login methods
- Verify HTTP status code
- Align error message with PR objectives
Apply these improvements:
it("Access Restriction for Non-Admin Users to facility creation page", () => {
+ cy.intercept('GET', '**/api/v1/facility/create/').as('accessFacility');
+
- const nonAdminLoginMethods = [
- loginPage.loginAsDevDoctor.bind(loginPage),
- loginPage.loginAsStaff.bind(loginPage),
+ // Load test users from fixture
+ cy.fixture('users').then((data) => {
+ const { nonAdminUsers } = data;
+
+ nonAdminUsers.forEach(({ username, password }) => {
+ loginPage.login(username, password);
+ cy.visit("/facility/create");
+
+ // Verify API response
+ 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();
+ });
});
-
- nonAdminLoginMethods.forEach((loginMethod) => {
- loginMethod();
- cy.visit("/facility/create");
- facilityPage.verifyErrorNotification(
- "You don't have permission to perform this action. Contact the admin",
- );
- cy.clearCookies();
- });
});Create a fixture file cypress/fixtures/users.json:
{
"nonAdminUsers": [
{
"username": "dummynurse1",
"password": "Coronasafe@123",
"role": "Nurse"
},
{
"username": "devdoctor",
"password": "Coronasafe@123",
"role": "Doctor"
}
]
}| 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
Enhance test reliability with API verification and data management
The test case needs improvements to meet the PR objectives and enhance reliability:
- Add API request verification
- Add test data cleanup
- Move test data to fixtures
Apply these improvements:
it("Should display error when district admin tries to create facility in a different district", () => {
+ cy.intercept('POST', '**/api/v1/facility/').as('createFacility');
+
+ // Load test data from fixture
+ cy.fixture('facility_data').then((data) => {
+ const { facilityName, facilityType, differentDistrict } = data;
+
facilityPage.visitCreateFacilityPage();
facilityPage.fillFacilityName(facilityName);
facilityPage.selectFacilityType(facilityType);
facilityPage.fillPincode("682001");
facilityPage.selectStateOnPincode("Kerala");
- facilityPage.selectDistrictOnPincode("Kottayam");
- facilityPage.selectLocalBody("Arpookara");
+ facilityPage.selectDistrictOnPincode(differentDistrict.name);
+ facilityPage.selectLocalBody(differentDistrict.localBody);
facilityPage.selectWard("5");
facilityPage.fillAddress(facilityAddress);
facilityPage.fillPhoneNumber(facilityNumber);
facilityPage.submitForm();
+
+ // Verify API response
+ cy.wait('@createFacility').then((interception) => {
+ expect(interception.response.statusCode).to.equal(403);
+ expect(interception.response.body).to.have.property('detail',
+ 'You do not have permission to perform this action.');
+ });
+
facilityPage.verifyErrorNotification(
"You do not have permission to perform this action.",
);
+
+ // Cleanup
+ cy.clearCookies();
+ cy.clearLocalStorage();
});
});Committable suggestion skipped: line range outside the PR's diff.
|
@Alokih 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