-
Notifications
You must be signed in to change notification settings - Fork 516
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.
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
Fixed Multi-selection functionality. #9345
Conversation
WalkthroughThe pull request introduces changes to the investigation-related components in the patient management system. The modifications focus on improving the multi-select functionality for investigations, enhancing the user interface, and refactoring the component logic. Key changes include adding a function to clear selected investigations, updating the AutocompleteMultiSelectFormField, and modifying the Cypress test suite to reflect new filtering and interaction scenarios. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, but do revert changes made to unrelated files
.gitattributes
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert unrelated changes
package-lock.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert unrelated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rithviknishad I added " * text=auto " to the .gitattributes file. Should I remove it? do I need to update any packages in the package-lock.json file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/Investigations/Reports/index.tsx (3)
111-113
: Consider adding a confirmation dialog for clearing selections.The clear functionality works correctly, but since it's a bulk operation that clears all selections, consider adding a confirmation dialog to prevent accidental clearing of multiple selections.
const clearSelectedInvestigations = () => { + if (window.confirm(t("clear_selections_confirmation"))) { dispatch({ type: "set_selected_investigations", payload: [] }); + } };
352-372
: Consider adding ARIA labels for accessibility.The button layout improvements look good, but consider adding ARIA labels to improve accessibility.
<ButtonV2 onClick={clearSelectedInvestigations} disabled={!selectedInvestigations.length} variant="secondary" className="my-2.5" + aria-label={t("clear_selected_investigations")} > {t("clear")} </ButtonV2>
Line range hint
111-372
: Add tests for the new multi-selection functionality.While the implementation looks good, please ensure adequate test coverage for:
- Multi-selection behavior
- Clear functionality
- Edge cases with empty/invalid selections
Would you like me to help generate test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
.gitattributes
(1 hunks)src/components/Facility/Investigations/Reports/index.tsx
(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🔇 Additional comments (2)
src/components/Facility/Investigations/Reports/index.tsx (2)
57-82
: LGTM! Clean reducer case formatting.
The reducer cases have been reformatted for better readability while maintaining the same functionality.
346-349
: Verify the multi-selection fix.
The changes correctly implement multi-selection by using external_id
as the unique identifier and enabling the selectAll
functionality.
✅ Verification successful
Multi-selection implementation verified successfully
The verification confirms that:
- The
external_id
is consistently used for investigation identification across API endpoints - The
AutocompleteMultiSelect
component is properly implemented in the Investigations module - The selection logic is correctly implemented with appropriate IDs and naming conventions
- The translation key "select_investigation" indicates proper UI messaging for the multi-select functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that external_id is consistently used across the codebase for investigation identification
# Search for investigation identification patterns
rg -A 2 "external_id.*investigation" --type typescript
# Search for potential places where the old selection logic might still be used
rg "investigation.*select" --type typescript
Length of output: 173
Script:
#!/bin/bash
# Let's try without typescript file type specification
# Search for external_id usage patterns in investigation context
rg -A 2 "external_id.*investigation"
# Search for investigation selection patterns
rg "investigation.*select"
# Also search for AutocompleteMultiSelect component usage
rg -A 3 "AutocompleteMultiSelect"
Length of output: 8612
@@ -185,7 +183,7 @@ const InvestigationReports = ({ id }: any) => { | |||
.flatten() | |||
.map((i) => ({ | |||
...i, | |||
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name} `, | |||
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix investigation name formatting logic.
The current implementation has a logical error in the string concatenation that could lead to incorrect formatting.
- name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`,
+ name: `${i.name}${i.groups[0]?.name ? ` | ${i.groups[0].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.
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`, | |
name: `${i.name}${i.groups[0]?.name ? ` | ${i.groups[0].name}` : ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/Investigations/Reports/index.tsx (2)
Line range hint
130-143
: Improve error message clarityThe current error message "No more reports to load" might be confusing when no investigations are selected. Consider providing more specific messages for different scenarios:
- When no investigations are selected
- When selected investigations have no reports
- When all reports have been loaded
if (investigationsParams.length === 0) { Notification.Error({ - msg: "No more reports to load", + msg: selectedInvestigations.length === 0 + ? "Please select at least one investigation" + : "No more reports to load", });
349-349
: Consider using the prop value directlyThe
selectAll
prop is hardcoded as a boolean literal. Consider using justselectAll
instead ofselectAll={true}
for better readability.- selectAll={true} + selectAll
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
.gitattributes
(1 hunks)src/components/Facility/Investigations/Reports/index.tsx
(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
🔇 Additional comments (5)
src/components/Facility/Investigations/Reports/index.tsx (5)
57-82
: LGTM! Clean reducer implementation.
The simplified reducer cases improve readability while maintaining the same functionality.
111-113
: LGTM! Clear function implementation.
The clearSelectedInvestigations
function properly implements the Clear functionality requested in the PR objectives.
346-349
: LGTM! Fixed multi-selection functionality.
The optionValue
now correctly uses external_id
, which should resolve the multi-selection issues mentioned in issue #9144.
352-372
: LGTM! Clear button implementation.
The Clear button is properly implemented with:
- Correct positioning using flex layout
- Appropriate disabled state
- Consistent styling with other buttons
346-372
: Verify fix for all reported issues
Please verify that these changes resolve all the issues mentioned in #9144:
- Users can now select multiple investigations
- Previously filled input forms remain visible
- Form submission works correctly with multiple selections
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient evidence to verify the review comment's concerns:
All reported issues have been properly addressed
The code changes successfully implement both requirements:
- Multi-selection functionality:
- Proper state management using
selectedInvestigations
array in reducer - Correct implementation of
AutocompleteMultiSelectFormField
with value binding - Working onChange handler that dispatches selected values
- Clear button functionality:
clearSelectedInvestigations
function properly resets selections- Button is correctly disabled when no investigations are selected
- Clear action properly integrated with state management
The implementation also maintains proper form state and handles report generation correctly through handleGenerateReports
.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining TODO comments or references to the multi-selection issue
rg -i "multi.*select|todo.*select" --type ts --type tsx
# Search for any error handling related to form submission
rg "handleSubmit|onSubmit|formData" --type ts --type tsx -A 5
Length of output: 177
Script:
#!/bin/bash
# Search for any remaining TODO comments or references to the multi-selection issue
rg -i "multi.*select|todo.*select"
# Search for form submission and state management related code in the Investigations component
rg "handleSubmit|onSubmit|formData|handleGenerateReports" -A 5
# Look for the investigation selection related code
ast-grep --pattern 'const handleGenerateReports = $_'
Length of output: 65961
Script:
#!/bin/bash
# Search for handleGenerateReports implementation in the Reports component
rg -t tsx "const handleGenerateReports" src/components/Facility/Investigations/Reports/ -A 10
# Look for state management related to investigation selection
rg -t tsx "selectedInvestigations|setSelectedInvestigations" src/components/Facility/Investigations/Reports/ -A 5
# Search for clearSelectedInvestigations implementation
rg -t tsx "const clearSelectedInvestigations" src/components/Facility/Investigations/Reports/ -A 5
Length of output: 396
Script:
#!/bin/bash
# Search for handleGenerateReports implementation in the Reports component
rg "const handleGenerateReports" src/components/Facility/Investigations/Reports/ -A 10
# Look for state management related to investigation selection
rg "selectedInvestigations|setSelectedInvestigations" src/components/Facility/Investigations/Reports/ -A 5
# Search for clearSelectedInvestigations implementation
rg "const clearSelectedInvestigations" src/components/Facility/Investigations/Reports/ -A 5
# Look for the AutocompleteMultiSelectFormField usage for investigations
rg "AutocompleteMultiSelectFormField.*investigations" src/components/Facility/Investigations/Reports/ -A 5
Length of output: 6977
@@ -185,7 +183,7 @@ const InvestigationReports = ({ id }: any) => { | |||
.flatten() | |||
.map((i) => ({ | |||
...i, | |||
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name} `, | |||
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix investigation name formatting logic
The current implementation has potential issues with empty groups and unnecessary separators.
- name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`,
+ name: `${i.name}${i.groups?.[0]?.name ? ` | ${i.groups[0].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.
name: `${i.name} ${i.groups[0].name && " | " + i.groups[0].name}`, | |
name: `${i.name}${i.groups?.[0]?.name ? ` | ${i.groups[0].name}` : ''}`, |
👋 Hi, @Jeffrin2005, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
{t("generate_report")} | ||
</ButtonV2> | ||
<div className="flex space-x-2"> | ||
<ButtonV2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch these out for Shadcn's Button component under ui/
(Switch out all the ButtonV2s in this file while you are at it)
package.json
Outdated
@@ -60,10 +60,10 @@ | |||
"@pnotify/mobile": "^5.2.0", | |||
"@radix-ui/react-dialog": "^1.1.2", | |||
"@radix-ui/react-dropdown-menu": "^2.1.2", | |||
"@radix-ui/react-icons": "^1.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave package.json out of the commit.
👋 Hi, @Jeffrin2005, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
request for review only once all previously requested changes are addressed |
👋 Hi, @Jeffrin2005, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts (2)
28-28
: Consider also verifying visibility.
You're currently checking that the element with#consultation_tab_nav
exists. If you want to ensure it's also visible and ready for user interaction, you could add a.should("be.visible")
assertion.
31-31
: Extend the assertion beyond existence if needed.
Verifying that#log-lab-results
exists is good. For more comprehensive coverage, consider adding an assertion to confirm it's enabled or clickable, ensuring a fully functioning button.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts
(1 hunks)cypress/pageobject/Patient/PatientInvestigation.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/pageobject/Patient/PatientInvestigation.ts
🧰 Additional context used
📓 Learnings (1)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9145
File: cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts:93-94
Timestamp: 2024-11-18T10:48:08.501Z
Learning: In `cypress/e2e/patient_spec/PatientConsultationCreation.cy.ts`, bed capacity verification assertions after patient admission are already being performed elsewhere, so adding them here is unnecessary.
🔇 Additional comments (3)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts (3)
25-26
: Good verification of the patient route.
Ensuring the URL includes /patient
confirms that we are on the correct page. This helps to prevent false positives if the test unexpectedly loads a different route.
32-32
: Validate multi-selection state.
The call to selectInvestigationOption(["Haematology", "Urine Test"])
is a solid approach to test multiple selections. Confirm that both options are actually selected in the UI to validate that the multi-select logic works end-to-end.
38-41
: Ensure user feedback for invalid submissions is confirmed.
Checking the visibility of the "Save Investigation" button, clicking it, and then verifying the "Please Enter at least one value" notification ensures that the validation flow for empty values is covered. If relevant, consider confirming that the button is disabled until valid inputs are provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cypress/pageobject/Patient/PatientInvestigation.ts (3)
2-11
: Consider more specific ID selectors to prevent conflicts.While using IDs for selectors is good practice, some IDs like
#investigation
and#investigation-select
are quite generic. Consider prefixing them with a more specific context (e.g.,#patient-investigation-add
,#patient-investigation-select
).
90-104
: Optimize wait strategies in UI interaction methods.The
clickLogLabResults
method has multiple fixed waits and redundant checks. Consider optimizing this and similar methods.Here's a more efficient implementation:
clickLogLabResults() { cy.log("Clicking Log Lab Results"); - this.elements.logLabResults().scrollIntoView().should("be.visible"); - cy.wait(1000); - - this.elements - .logLabResults() - .contains("Log Lab Results") - .should("be.visible") - .should("be.enabled") - .click(); - - cy.wait(2000); + this.elements + .logLabResults() + .scrollIntoView() + .should("be.visible") + .and("be.enabled") + .and("contain", "Log Lab Results") + .click() + // Wait for the action to complete + .then(() => { + // Add appropriate assertion based on what should happen after click + cy.get("#results-form").should("exist"); + }); }
117-123
: Enhance utility methods for better reusability.The utility methods are good additions but could be more flexible and robust.
Consider these improvements:
-verifyElementPresent(selector: string, timeout = 10000) { - return cy.get(selector, { timeout }).should("exist").should("be.visible"); +verifyElementPresent( + selector: string, + options: { timeout?: number; assertion?: string } = {} +) { + const { timeout = 10000, assertion = "be.visible" } = options; + return cy.get(selector, { timeout }).should("exist").should(assertion); } waitForLoading() { - cy.get("#loading-indicator", { timeout: 10000 }).should("not.exist"); + // Handle both spinner and skeleton loading states + cy.get("#loading-indicator, .skeleton-loader", { timeout: 10000 }).should( + "not.exist" + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts
(1 hunks)cypress/pageobject/Patient/PatientInvestigation.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/patient_spec/PatientInvestigation.cy.ts
selectInvestigationOption(options: string[]) { | ||
cy.clickAndMultiSelectOption("#investigations", options); | ||
cy.log("Selecting Investigation Options:", options); | ||
|
||
this.elements | ||
.investigationSelect() | ||
.should("exist") | ||
.should("be.visible") | ||
.click(); | ||
|
||
options.forEach((option) => { | ||
cy.get("[role='option']") | ||
.contains(option) | ||
.should("be.visible") | ||
.then(($el) => { | ||
if ($el.length > 0) { | ||
cy.wrap($el).click(); | ||
} else { | ||
throw new Error(`Option "${option}" not found in dropdown`); | ||
} | ||
}); | ||
}); | ||
|
||
cy.get("body").click(0, 0); | ||
cy.wait(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve robustness of multi-selection handling.
The current implementation has several areas for improvement:
- Clicking at (0,0) to close the dropdown is fragile and may interfere with other elements
- No verification that selections were actually successful
- Using fixed waits instead of waiting for state changes
Consider this improved implementation:
selectInvestigationOption(options: string[]) {
cy.log("Selecting Investigation Options:", options);
this.elements
.investigationSelect()
.should("exist")
.should("be.visible")
.click();
options.forEach((option) => {
cy.get("[role='option']")
.contains(option)
- .should("be.visible")
+ .should("be.visible", { timeout: 5000 })
.then(($el) => {
if ($el.length > 0) {
cy.wrap($el).click();
+ // Verify selection was successful
+ cy.get(".selected-option").should("contain", option);
} else {
- throw new Error(`Option "${option}" not found in dropdown`);
+ throw new Error(
+ `Option "${option}" not found in dropdown. Available options: ${
+ $el.parent().text()
+ }`
+ );
}
});
});
- cy.get("body").click(0, 0);
- cy.wait(500);
+ // Close dropdown using the dropdown trigger itself
+ this.elements.investigationSelect().click();
+ // Verify dropdown is closed
+ cy.get("[role='option']").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.
selectInvestigationOption(options: string[]) { | |
cy.clickAndMultiSelectOption("#investigations", options); | |
cy.log("Selecting Investigation Options:", options); | |
this.elements | |
.investigationSelect() | |
.should("exist") | |
.should("be.visible") | |
.click(); | |
options.forEach((option) => { | |
cy.get("[role='option']") | |
.contains(option) | |
.should("be.visible") | |
.then(($el) => { | |
if ($el.length > 0) { | |
cy.wrap($el).click(); | |
} else { | |
throw new Error(`Option "${option}" not found in dropdown`); | |
} | |
}); | |
}); | |
cy.get("body").click(0, 0); | |
cy.wait(500); | |
selectInvestigationOption(options: string[]) { | |
cy.log("Selecting Investigation Options:", options); | |
this.elements | |
.investigationSelect() | |
.should("exist") | |
.should("be.visible") | |
.click(); | |
options.forEach((option) => { | |
cy.get("[role='option']") | |
.contains(option) | |
.should("be.visible", { timeout: 5000 }) | |
.then(($el) => { | |
if ($el.length > 0) { | |
cy.wrap($el).click(); | |
// Verify selection was successful | |
cy.get(".selected-option").should("contain", option); | |
} else { | |
throw new Error( | |
`Option "${option}" not found in dropdown. Available options: ${ | |
$el.parent().text() | |
}` | |
); | |
} | |
}); | |
}); | |
// Close dropdown using the dropdown trigger itself | |
this.elements.investigationSelect().click(); | |
// Verify dropdown is closed | |
cy.get("[role='option']").should("not.exist"); | |
} |
selectInvestigation(investigation: string) { | ||
cy.get("#search-patient-investigation").type(investigation); | ||
cy.verifyAndClickElement("#investigation-group", investigation); | ||
cy.verifyAndClickElement("#investigation", "Investigation No. 1"); | ||
cy.log(`Selecting Investigation: ${investigation}`); | ||
|
||
this.elements | ||
.searchPatientInvestigation() | ||
.should("be.visible") | ||
.clear() | ||
.type(investigation, { delay: 100 }); | ||
|
||
this.elements | ||
.investigationGroup() | ||
.contains(investigation) | ||
.should("be.visible") | ||
.click(); | ||
|
||
this.elements | ||
.addInvestigation() | ||
.contains("Investigation No. 1") | ||
.should("be.visible") | ||
.click(); | ||
} | ||
|
||
clickInvestigationCheckbox() { | ||
cy.get("#investigation-checkbox").click(); | ||
cy.log("Clicking Investigation Checkbox"); | ||
this.elements | ||
.investigationCheckbox() | ||
.should("be.visible") | ||
.should("be.enabled") | ||
.click(); | ||
} | ||
|
||
selectInvestigationOption(options: string[]) { | ||
cy.clickAndMultiSelectOption("#investigations", options); | ||
cy.log("Selecting Investigation Options:", options); | ||
|
||
this.elements | ||
.investigationSelect() | ||
.should("exist") | ||
.should("be.visible") | ||
.click(); | ||
|
||
options.forEach((option) => { | ||
cy.get("[role='option']") | ||
.contains(option) | ||
.should("be.visible") | ||
.then(($el) => { | ||
if ($el.length > 0) { | ||
cy.wrap($el).click(); | ||
} else { | ||
throw new Error(`Option "${option}" not found in dropdown`); | ||
} | ||
}); | ||
}); | ||
|
||
cy.get("body").click(0, 0); | ||
cy.wait(500); | ||
} | ||
|
||
clickLogLabResults() { | ||
cy.verifyAndClickElement("#log-lab-results", "Log Lab Results"); | ||
cy.log("Clicking Log Lab Results"); | ||
|
||
this.elements.logLabResults().scrollIntoView().should("be.visible"); | ||
|
||
cy.wait(1000); | ||
|
||
this.elements | ||
.logLabResults() | ||
.contains("Log Lab Results") | ||
.should("be.visible") | ||
.should("be.enabled") | ||
.click(); | ||
|
||
cy.wait(2000); | ||
} | ||
|
||
selectInvestigationFrequency(frequency: string) { | ||
cy.get("#investigation-frequency").click(); | ||
cy.log(`Selecting Investigation Frequency: ${frequency}`); | ||
|
||
this.elements.investigationFrequency().should("be.visible").click(); | ||
|
||
cy.contains("button", frequency).should("be.visible").click(); | ||
|
||
cy.wait(500); | ||
} | ||
|
||
verifyElementPresent(selector: string, timeout = 10000) { | ||
return cy.get(selector, { timeout }).should("exist").should("be.visible"); | ||
} | ||
|
||
waitForLoading() { | ||
cy.get("#loading-indicator", { timeout: 10000 }).should("not.exist"); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Test coverage is incomplete for multi-selection scenarios
The current test implementation only partially covers the required scenarios. Here are the gaps identified:
- The test only verifies multi-selection in the log lab results section with two options ("Haematology", "Urine Test")
- Missing test coverage for:
- Multi-selection in the create investigation page
- Clear button functionality
- Edge cases like selecting/deselecting all options
- Persistence of selected options
The page object has the necessary methods (selectInvestigationOption
, clear
), but they aren't fully utilized in the test scenarios. The underlying components (AutoCompleteAsync.tsx
) support multiple selection, but the test coverage doesn't exercise all these capabilities.
🔗 Analysis chain
Verify coverage of all multi-selection scenarios.
While the test file has been updated to handle multi-selection, please ensure it covers all scenarios mentioned in issue #9144, including:
- Multi-selection in the Patient log lab results section
- Multi-selection in the create investigation page
- The new Clear button functionality
Let's verify the test coverage:
Let me gather more information about the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test cases in the codebase
echo "Searching for related test files..."
fd -e cy.ts -e spec.ts "Investigation"
echo "Checking for multi-select testing patterns..."
rg -l "selectInvestigationOption.*\[.*\]" -t ts
echo "Checking for Clear button test coverage..."
rg -l "clear.*button|clear.*selection" -t ts
Length of output: 746
Script:
#!/bin/bash
# Check the actual test implementation
echo "Checking PatientInvestigation test implementation..."
cat cypress/e2e/patient_spec/PatientInvestigation.cy.ts
# Look for clear functionality in the page object
echo -e "\nChecking clear functionality in page object..."
rg "clear" cypress/pageobject/Patient/PatientInvestigation.ts
# Check for any multi-select related code in the component
echo -e "\nChecking multi-select implementation in components..."
rg -A 5 "multiSelect|multiple" src/components/Form/AutoCompleteAsync.tsx src/components/Form/FormFields/Autocomplete.tsx
Length of output: 5394
@nihal467 I tried to fix it, but it's been not resolved , Could you help me to resolve it ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cypress/pageobject/Patient/PatientInvestigation.ts (3)
3-7
: LGTM! Consider adding timeout configuration.The improved element checks enhance test reliability. However, consider configuring a custom timeout for better error handling.
cy.get("#investigation") .should("exist") - .and("be.visible") + .and("be.visible", { timeout: 10000 }) .contains("Add Investigation") .click();
11-21
: Consider extracting common patterns into custom commands.The implementation is robust with network interception and proper waits. However, this pattern of waiting for network requests and checking element visibility could be reused across tests.
Create a custom command in
cypress/support/commands.ts
:Cypress.Commands.add('clickTabAndWaitForData', (tabSelector: string, interceptUrl: string) => { cy.intercept('GET', interceptUrl).as('dataLoad'); cy.get(tabSelector) .should('exist') .and('be.visible') .click(); cy.wait('@dataLoad', { timeout: 10000 }); });Then simplify the method to:
clickInvestigationTab() { cy.clickTabAndWaitForData( '#consultation_tab_nav', '**/investigations/**' ); cy.get('#investigations').should('exist').and('be.visible'); }
25-41
: Parameterize hard-coded text for better maintainability.While the implementation is robust with proper element checks and typing behavior, the hard-coded text "Investigation No. 1" could make tests fragile.
- cy.get("#investigation") - .should("exist") - .and("be.visible") - .contains("Investigation No. 1") - .click(); + cy.get("#investigation") + .should("exist") + .and("be.visible") + .contains(new RegExp(`Investigation.*${investigation}`)) + .click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts
(1 hunks)cypress/pageobject/Patient/PatientInvestigation.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cypress/e2e/patient_spec/PatientInvestigation.cy.ts
🔇 Additional comments (2)
cypress/pageobject/Patient/PatientInvestigation.ts (2)
58-76
: LGTM! Well-structured methods with proper checks.
The methods follow consistent patterns for element verification and provide good utility functions for test scenarios.
49-54
: Verify the custom command implementation for multi-select.
The implementation uses a custom command clickAndMultiSelectOption
which is an improvement over the previous version. However, we should verify its implementation.
class PatientInvestigation { | ||
clickAddInvestigation() { | ||
cy.verifyAndClickElement("#investigation", "Add Investigation"); | ||
cy.get("#investigation") | ||
.should("exist") | ||
.and("be.visible") | ||
.contains("Add Investigation") | ||
.click(); | ||
} | ||
|
||
clickInvestigationTab() { | ||
cy.verifyAndClickElement("#consultation_tab_nav", "Investigations"); | ||
cy.intercept("GET", "**/investigations/**").as("investigationsLoad"); | ||
|
||
cy.get("#consultation_tab_nav") | ||
.should("exist") | ||
.and("be.visible") | ||
.contains("Investigations") | ||
.click(); | ||
|
||
cy.wait("@investigationsLoad", { timeout: 10000 }); | ||
|
||
cy.get("#investigations").should("exist").and("be.visible"); | ||
} | ||
|
||
selectInvestigation(investigation: string) { | ||
cy.get("#search-patient-investigation").type(investigation); | ||
cy.verifyAndClickElement("#investigation-group", investigation); | ||
cy.verifyAndClickElement("#investigation", "Investigation No. 1"); | ||
cy.get("#search-patient-investigation") | ||
.should("exist") | ||
.and("be.visible") | ||
.clear() | ||
.type(investigation, { delay: 100 }); | ||
|
||
cy.get("#investigation-group") | ||
.should("exist") | ||
.and("be.visible") | ||
.contains(investigation) | ||
.click(); | ||
|
||
cy.get("#investigation") | ||
.should("exist") | ||
.and("be.visible") | ||
.contains("Investigation No. 1") | ||
.click(); | ||
} | ||
|
||
clickInvestigationCheckbox() { | ||
cy.get("#investigation-checkbox").click(); | ||
cy.get("#investigation-checkbox").should("exist").and("be.visible").click(); | ||
} | ||
|
||
selectInvestigationOption(options: string[]) { | ||
cy.clickAndMultiSelectOption("#investigations", options); | ||
cy.get("#investigations") | ||
.should("exist") | ||
.and("be.visible") | ||
.then(() => { | ||
cy.clickAndMultiSelectOption("#investigations", options); | ||
}); | ||
} | ||
|
||
clickLogLabResults() { | ||
cy.verifyAndClickElement("#log-lab-results", "Log Lab Results"); | ||
cy.get("#log-lab-results") | ||
.should("exist") | ||
.and("be.visible") | ||
.contains("Log Lab Results") | ||
.click(); | ||
} | ||
|
||
selectInvestigationFrequency(frequency: string) { | ||
cy.get("#investigation-frequency").click(); | ||
cy.contains("button", frequency).should("be.visible").click(); | ||
cy.get("#investigation-frequency") | ||
.should("exist") | ||
.and("be.visible") | ||
.click(); | ||
|
||
cy.contains("button", frequency).should("exist").and("be.visible").click(); | ||
} | ||
|
||
verifyPageLoaded() { | ||
return cy.get("#investigations").should("exist").and("be.visible"); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Test coverage needs to be expanded for multi-selection scenarios
Based on the code analysis, the current test coverage is incomplete for the multi-selection functionality mentioned in issue #9144:
- The investigation test file only covers a basic multi-selection scenario with two options ("Haematology", "Urine Test") in one test case.
- The clear button functionality is not tested.
- Multi-selection in create investigation page is not covered.
Key areas that need test coverage:
- Test selecting multiple investigations with more than two options
- Test clearing selected investigations
- Test multi-selection in both log lab results and create investigation pages
- Test edge cases (selecting all options, deselecting options)
🔗 Analysis chain
Ensure comprehensive test coverage for multi-selection scenarios.
While the page object methods have been improved, we need to ensure all scenarios from issue #9144 are covered:
- Multi-selection in Patient log lab results section
- Multi-selection in create investigation page
- Clear button functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for multi-selection scenarios
echo "Checking test coverage..."
# Look for test cases covering multi-selection
echo "Multi-selection test cases:"
rg -B 2 -A 2 "selectInvestigationOption.*\[.*\]" cypress/e2e/
# Look for clear button test cases
echo -e "\nClear button test cases:"
rg -B 2 -A 2 "clear.*button|clearSelected" cypress/e2e/
# Look for create investigation page test cases
echo -e "\nCreate investigation page test cases:"
rg -B 2 -A 2 "create.*investigation" cypress/e2e/
Length of output: 928
Script:
#!/bin/bash
# Let's try a different approach to find test coverage
# Look for test files that might contain investigation-related tests
echo "Investigation test files:"
fd ".*investigation.*\.cy\.ts$" cypress/e2e/
# Search for investigation-related test descriptions
echo -e "\nInvestigation test cases:"
rg -B 2 -A 2 "it\(.*investigation.*" cypress/e2e/
# Look for usage of PatientInvestigation page object
echo -e "\nPatientInvestigation usage:"
rg -B 2 -A 2 "new PatientInvestigation\(\)" cypress/e2e/
# Search for multi-select related code
echo -e "\nMulti-select related code:"
rg -B 2 -A 2 "multiSelect|multiple.*select" cypress/e2e/
Length of output: 2941
Script:
#!/bin/bash
# Let's examine the actual test file content to understand coverage
echo "Content of PatientInvestigation.cy.ts:"
cat cypress/e2e/patient_spec/PatientInvestigation.cy.ts
echo -e "\nChecking for clickAndMultiSelectOption implementation:"
rg -B 2 -A 2 "clickAndMultiSelectOption" cypress/support/
echo -e "\nChecking PatientLogUpdate.cy.ts for lab results tests:"
cat cypress/e2e/patient_spec/PatientLogUpdate.cy.ts
Length of output: 19730
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts (3)
7-27
: Move test data to fixturesConsider moving the test data (e.g.,
patientGender
,patientCategory
, dates, facility details) to a Cypress fixture file. This would:
- Improve maintainability
- Allow reuse across different test files
- Make it easier to update test data
Example structure:
// cypress/fixtures/patient-home.json { "patient": { "gender": "Male", "category": "Moderate", "ageRange": { "min": "18", "max": "24" } // ... other patient data }, "facility": { "name": "Dummy Facility 40", "type": "Private Hospital" // ... other facility data } }
40-57
: Extract common filter operations into custom commandsThe date-based filter test (and others) repeat similar patterns. Consider creating custom commands for common operations:
// cypress/support/commands.ts Cypress.Commands.add('applyDateFilters', (fromDate: string, toDate: string) => { cy.get('[data-cy="advanced-filters-button"]').click() cy.get('[data-cy="created-before-date"]').type(fromDate) // ... other date filter operations })
144-154
: Enhance export test robustnessThe export test could be more robust:
- Add verification for the exported file content
- Test different date ranges
- Verify error cases (e.g., invalid date ranges)
Example enhancement:
it("Export the live patient list based on a date range", () => { // Test invalid date range patientHome.typePatientModifiedBeforeDate("07122023"); patientHome.typePatientModifiedAfterDate("01122023"); patientHome.clickPatientFilterApply(); patientHome.clickPatientExport(); cy.verifyNotification("Invalid date range"); // Test valid date range // ... existing code ... // Verify exported file cy.readFile('cypress/downloads/patient-list.csv').should('exist') .and('contain', 'Patient Name,Gender,Age'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts
(2 hunks)
🔇 Additional comments (3)
cypress/e2e/patient_spec/PatientInvestigation.cy.ts (3)
Line range hint 29-39
: LGTM! Test hooks follow best practices
The before/beforeEach hooks are well-structured:
- Proper login setup
- Appropriate localStorage handling
- Clean filter state before each test
Line range hint 175-177
: LGTM! Proper test cleanup
The afterEach hook appropriately saves localStorage state, maintaining consistency with the beforeEach hook.
Line range hint 1-177
: Add tests for multi-selection functionality
The PR objectives mention fixing multi-selection functionality in the Patient log lab results section, but there are no tests covering this functionality. Consider adding test cases to verify:
- Multiple selection capability
- Clear button functionality
- Edge cases (e.g., selecting all options, deselecting all)
Example test structure:
it("supports multi-selection in lab results", () => {
// Navigate to lab results
// Select multiple options
// Verify selections
// Test clear button
});
import { advanceFilters } from "pageobject/utils/advanceFilterHelpers"; | ||
import { pageNavigation } from "pageobject/utils/paginationHelpers"; | ||
|
||
import LoginPage from "../../pageobject/Login/LoginPage"; | ||
import PatientHome from "../../pageobject/Patient/PatientHome"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Rename the file to match its contents
The filename PatientInvestigation.cy.ts
suggests tests for patient investigations, but the actual content tests patient homepage functionalities. Consider renaming the file to PatientHome.cy.ts
or PatientHomepage.cy.ts
for better clarity.
Fixed cypress test |
no longer relevant in new architecture |
Proposed Changes
multiselection1.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements