Added the CSV Export button In Discharged Patients #9142
Conversation
WalkthroughThe pull request introduces significant 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
src/components/Facility/DischargedPatientsList.tsxOops! 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: 4
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/DischargedPatientsList.tsx (3)
203-210: UseforEachinstead ofmapfor side effectsThe
Array.prototype.mapmethod is used for transforming arrays and returns a new array. Since the returned array isn't utilized,forEachis more appropriate for iterating over the array for side effects.Apply this diff to use
forEach:- lines.map((line: any, i: number) => { + lines.forEach((line: any, i: number) => {
199-245: Correct grammatical errors in commentsThere are typographical errors and grammatical issues in the comments, such as "imforamation" and "there" instead of "their". Correcting these will improve code readability.
Example correction:
- // Generate a array which contains imforamation of duplicate patient IDs and there respective linenumbers + // Generate an array that contains information about duplicate patient IDs and their respective line numbers
455-465: Adjust scroll target to appropriate filter sectionWhen the export is not allowed, clicking the export button opens the advanced filter and scrolls to an element with the ID
bed-type-select. Since the issue is related to selecting a date range, the scroll target should be the date range selector.Apply this diff to update the scroll target:
- const element = - document.getElementById("bed-type-select"); + const element = + document.getElementById("created-date-select");Ensure that the date range selector has the corresponding ID
created-date-selector adjust the ID accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
195-196:
Reevaluate the logic for allowing export
The isExportAllowed condition may not align with the requirement that the export is permitted only when a valid seven-day period is selected. Currently, if no date ranges are selected (durations contains zeros), isExportAllowed might erroneously allow exports.
Please verify and adjust the logic to ensure that export is allowed only when at least one date range of up to seven days is selected. Consider modifying the condition as follows:
- durations.every((x) => x >= 0 && x <= 7) &&
- !durations.every((x) => x === 0);
+ durations.some((x) => x > 0 && x <= 7);| disabled={!isExportAllowed} | ||
| exportItems={[ | ||
| { | ||
| label: "Export Live patients", |
There was a problem hiding this comment.
Update export label to "Export Discharged patients"
The label for the export action is currently "Export Live patients", which does not reflect the context of the discharged patients list.
Apply this diff to correct the label:
- label: "Export Live patients",
+ label: "Export Discharged patients",📝 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.
| label: "Export Live patients", | |
| label: "Export Discharged patients", |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/DischargedPatientsList.tsx (2)
452-505: Improve configurability of export UI behaviorThe implementation has several hard-coded values that should be configurable:
- Timeout duration for scroll animation
- Scroll behavior configuration
- Magic number in tooltip text
Consider these improvements:
+ const EXPORT_CONFIG = { + SCROLL_DELAY_MS: 500, + MAX_EXPORT_DAYS: 7, + SCROLL_BEHAVIOR: 'smooth' as ScrollBehavior + }; + <div className="tooltip w-full md:w-auto" id="patient-export"> {!isExportAllowed ? ( <ButtonV2 onClick={() => { advancedFilter.setShow(true); setTimeout(() => { const element = document.getElementById("bed-type-select"); if (element) element.scrollIntoView({ - behavior: "smooth" + behavior: EXPORT_CONFIG.SCROLL_BEHAVIOR }); Notification.Warn({ - msg: "Please select a seven day period.", + msg: `Please select a ${EXPORT_CONFIG.MAX_EXPORT_DAYS} day period.`, }); - }, 500); + }, EXPORT_CONFIG.SCROLL_DELAY_MS); }}
473-497: Add loading state to export functionalityThe export action should indicate its progress to prevent multiple clicks and provide feedback to users.
Consider adding loading state:
<ExportMenu disabled={!isExportAllowed} exportItems={[ { label: "Export Discharged patients", + showLoading: true, action: async () => { + try { const query = { ...qParams, csv: true, }; const pathParams = { facility_external_id }; const { data } = await request( routes.listFacilityDischargedPatients, { query, pathParams, }, ); return data ?? null; + } catch (error) { + Notification.Error({ + msg: "Failed to export patients data", + }); + return null; + } }, parse: preventDuplicatePatientsDuetoPolicyId, }, ]} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/DischargedPatientsList.tsx(5 hunks)
🔇 Additional comments (1)
src/components/Facility/DischargedPatientsList.tsx (1)
183-192:
Fix duration calculation logic
The current implementation has a few issues:
- The duration calculation can result in negative values
- The comment about XOR is misleading as it's using logical operators
Apply this diff to fix the issues:
const durations = date_range_fields.map((field: string[]) => {
- // XOR (checks if only one of the dates is set)
+ // Check if exactly one date is set
if ((field[0] && !field[1]) || (!field[0] && field[1])) {
return -1;
}
if (field[0] && field[1]) {
- return dayjs(field[0]).diff(dayjs(field[1]), "days");
+ return Math.abs(dayjs(field[1]).diff(dayjs(field[0]), "days"));
}
return 0;
});Likely invalid or redundant comment.
| const preventDuplicatePatientsDuetoPolicyId = (data: any) => { | ||
| // Generate a array which contains imforamation of duplicate patient IDs and there respective linenumbers | ||
| const lines = data.split("\n"); // Split the data into individual lines | ||
| const idsMap = new Map(); // To store indices of lines with the same patient ID | ||
|
|
||
| lines.map((line: any, i: number) => { | ||
| const patientId = line.split(",")[0]; // Extract the patient ID from each line | ||
| if (idsMap.has(patientId)) { | ||
| idsMap.get(patientId).push(i); // Add the index to the existing array | ||
| } else { | ||
| idsMap.set(patientId, [i]); // Create a new array with the current index | ||
| } | ||
| }); | ||
|
|
||
| const linesWithSameId = Array.from(idsMap.entries()) | ||
| .filter(([_, indices]) => indices.length > 1) | ||
| .map(([patientId, indices]) => ({ | ||
| patientId, | ||
| indexSame: indices, | ||
| })); | ||
|
|
||
| // after getting the array of duplicate patient IDs and there respective linenumbers we will merge the policy IDs of the duplicate patients | ||
|
|
||
| linesWithSameId.map((lineInfo) => { | ||
| const indexes = lineInfo.indexSame; | ||
| //get policyid of all the duplicate patients and merge them by seperating them with a semicolon | ||
| const mergedPolicyId = indexes | ||
| .map((currentIndex: number) => lines[currentIndex].split(",")[5]) | ||
| .join(";"); | ||
| // replace the policy ID of the first patient with the merged policy ID | ||
| const arrayOfCurrentLine = lines[indexes[0]].split(","); | ||
| arrayOfCurrentLine[5] = mergedPolicyId; | ||
| const lineAfterMerge = arrayOfCurrentLine.join(","); | ||
| lines[indexes[0]] = `${lineAfterMerge}`; | ||
| }); | ||
|
|
||
| // after merging the policy IDs of the duplicate patients we will remove the duplicate patients from the data | ||
| const uniqueLines = []; | ||
| const ids = new Set(); // To keep track of unique patient IDs | ||
|
|
||
| for (const line of lines) { | ||
| const patientId = line.split(",")[0]; // Extract the patient ID from each line | ||
| if (!ids.has(patientId)) { | ||
| uniqueLines.push(line); | ||
| ids.add(patientId); | ||
| } | ||
| } | ||
|
|
||
| const cleanedData = uniqueLines.join("\n"); // Join the unique lines back together | ||
| return cleanedData; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of CSV processing
The current implementation has several issues that could make it fragile:
- Uses 'any' type instead of proper typing
- Assumes specific CSV column positions without validation
- No error handling for malformed data
Consider these improvements:
- const preventDuplicatePatientsDuetoPolicyId = (data: any) => {
+ interface CSVRow {
+ patientId: string;
+ policyId: string;
+ [key: string]: string;
+ }
+
+ const preventDuplicatePatientsDuetoPolicyId = (data: string) => {
+ try {
const lines = data.split("\n");
- const idsMap = new Map();
+ const idsMap = new Map<string, number[]>();
+ const PATIENT_ID_INDEX = 0;
+ const POLICY_ID_INDEX = 5;
lines.map((line: string, i: number) => {
+ if (!line.trim()) return;
+ const columns = line.split(",");
+ if (columns.length <= POLICY_ID_INDEX) {
+ throw new Error(`Invalid CSV format at line ${i + 1}`);
+ }
- const patientId = line.split(",")[0];
+ const patientId = columns[PATIENT_ID_INDEX];
// ... rest of the implementation
});
+ } catch (error) {
+ console.error("Error processing CSV:", error);
+ throw new Error("Failed to process CSV data");
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const params = { | ||
| page: qParams.page || 1, | ||
| limit: resultsPerPage, | ||
| name: qParams.name || undefined, | ||
| patient_no: qParams.patient_no || undefined, | ||
| is_active: | ||
| !qParams.last_consultation__new_discharge_reason && | ||
| (qParams.is_active || "False"), | ||
| phone_number: qParams.phone_number | ||
| ? parsePhoneNumber(qParams.phone_number) | ||
| : undefined, | ||
| emergency_phone_number: qParams.emergency_phone_number | ||
| ? parsePhoneNumber(qParams.emergency_phone_number) | ||
| : undefined, | ||
| local_body: qParams.lsgBody || undefined, | ||
| facility: qParams.facility, | ||
| facility_type: qParams.facility_type || undefined, | ||
| district: qParams.district || undefined, | ||
| offset: (qParams.page ? qParams.page - 1 : 0) * resultsPerPage, | ||
| created_date_before: qParams.created_date_before || undefined, | ||
| created_date_after: qParams.created_date_after || undefined, | ||
| modified_date_before: qParams.modified_date_before || undefined, | ||
| modified_date_after: qParams.modified_date_after || undefined, | ||
| ordering: qParams.ordering || undefined, | ||
| category: qParams.category || undefined, | ||
| gender: qParams.gender || undefined, | ||
| age_min: qParams.age_min || undefined, | ||
| age_max: qParams.age_max || undefined, | ||
| date_declared_positive_before: | ||
| qParams.date_declared_positive_before || undefined, | ||
| date_declared_positive_after: | ||
| qParams.date_declared_positive_after || undefined, | ||
| ration_card_category: qParams.ration_card_category || undefined, | ||
| last_consultation_medico_legal_case: | ||
| qParams.last_consultation_medico_legal_case || undefined, | ||
| last_consultation_encounter_date_before: | ||
| qParams.last_consultation_encounter_date_before || undefined, | ||
| last_consultation_encounter_date_after: | ||
| qParams.last_consultation_encounter_date_after || undefined, | ||
| last_consultation_discharge_date_before: | ||
| qParams.last_consultation_discharge_date_before || undefined, | ||
| last_consultation_discharge_date_after: | ||
| qParams.last_consultation_discharge_date_after || undefined, | ||
| last_consultation_admitted_bed_type_list: | ||
| qParams.last_consultation_admitted_bed_type_list || undefined, | ||
| last_consultation__consent_types: | ||
| qParams.last_consultation__consent_types || undefined, | ||
| last_consultation__new_discharge_reason: | ||
| qParams.last_consultation__new_discharge_reason || undefined, | ||
| last_consultation_current_bed__location: | ||
| qParams.last_consultation_current_bed__location || undefined, | ||
| number_of_doses: qParams.number_of_doses || undefined, | ||
| covin_id: qParams.covin_id || undefined, | ||
| is_kasp: qParams.is_kasp || undefined, | ||
| is_declared_positive: qParams.is_declared_positive || undefined, | ||
| last_vaccinated_date_before: | ||
| qParams.last_vaccinated_date_before || undefined, | ||
| last_vaccinated_date_after: qParams.last_vaccinated_date_after || undefined, | ||
| last_consultation_is_telemedicine: | ||
| qParams.last_consultation_is_telemedicine || undefined, | ||
| is_antenatal: qParams.is_antenatal || undefined, | ||
| last_menstruation_start_date_after: | ||
| (qParams.is_antenatal === "true" && | ||
| dayjs().subtract(9, "month").format("YYYY-MM-DD")) || | ||
| undefined, | ||
| ventilator_interface: qParams.ventilator_interface || undefined, | ||
| diagnoses: qParams.diagnoses || undefined, | ||
| diagnoses_confirmed: qParams.diagnoses_confirmed || undefined, | ||
| diagnoses_provisional: qParams.diagnoses_provisional || undefined, | ||
| diagnoses_unconfirmed: qParams.diagnoses_unconfirmed || undefined, | ||
| diagnoses_differential: qParams.diagnoses_differential || undefined, | ||
| review_missed: qParams.review_missed || undefined, | ||
| }; |
There was a problem hiding this comment.
are you sure these filters are present in the discharged patient list?
| const preventDuplicatePatientsDuetoPolicyId = (data: any) => { | ||
| // Generate a array which contains imforamation of duplicate patient IDs and there respective linenumbers | ||
| const lines = data.split("\n"); // Split the data into individual lines | ||
| const idsMap = new Map(); // To store indices of lines with the same patient ID | ||
|
|
||
| lines.map((line: any, i: number) => { | ||
| const patientId = line.split(",")[0]; // Extract the patient ID from each line | ||
| if (idsMap.has(patientId)) { | ||
| idsMap.get(patientId).push(i); // Add the index to the existing array | ||
| } else { | ||
| idsMap.set(patientId, [i]); // Create a new array with the current index | ||
| } | ||
| }); | ||
|
|
||
| const linesWithSameId = Array.from(idsMap.entries()) | ||
| .filter(([_, indices]) => indices.length > 1) | ||
| .map(([patientId, indices]) => ({ | ||
| patientId, | ||
| indexSame: indices, | ||
| })); | ||
|
|
||
| // after getting the array of duplicate patient IDs and there respective linenumbers we will merge the policy IDs of the duplicate patients | ||
|
|
||
| linesWithSameId.map((lineInfo) => { | ||
| const indexes = lineInfo.indexSame; | ||
| //get policyid of all the duplicate patients and merge them by seperating them with a semicolon | ||
| const mergedPolicyId = indexes | ||
| .map((currentIndex: number) => lines[currentIndex].split(",")[5]) | ||
| .join(";"); | ||
| // replace the policy ID of the first patient with the merged policy ID | ||
| const arrayOfCurrentLine = lines[indexes[0]].split(","); | ||
| arrayOfCurrentLine[5] = mergedPolicyId; | ||
| const lineAfterMerge = arrayOfCurrentLine.join(","); | ||
| lines[indexes[0]] = `${lineAfterMerge}`; | ||
| }); | ||
|
|
||
| // after merging the policy IDs of the duplicate patients we will remove the duplicate patients from the data | ||
| const uniqueLines = []; | ||
| const ids = new Set(); // To keep track of unique patient IDs | ||
|
|
||
| for (const line of lines) { | ||
| const patientId = line.split(",")[0]; // Extract the patient ID from each line | ||
| if (!ids.has(patientId)) { | ||
| uniqueLines.push(line); | ||
| ids.add(patientId); |
There was a problem hiding this comment.
this is duplicate code, same as present in normal patients list page. do create a utility function out of it and reuse it.
Proposed Changes
added export button with restriction based on seven-day period selection.
Introduced an ExportMenu component to handle export of live patients when export is allowed.
Implemented tooltip guidance for when export is not allowed, prompting the user to select a valid seven-day period.
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
ExportMenufor exporting patient data, with notifications for invalid selections.Bug Fixes