Add discharge summary for encounters #2700
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant modifications to the discharge summary functionality within the healthcare application. The changes shift the discharge summary generation from patient consultations to encounters, enhance the PDF generation process, and introduce new template tags. Additionally, various utility functions and model methods are updated, resulting in a more streamlined and flexible approach for managing patient discharge documentation. The alterations affect database commands, viewsets, and report generation, enhancing the overall functionality. Changes
Poem
Sequence DiagramsequenceDiagram
participant User
participant EncounterViewSet
participant DischargeSummaryTask
participant FileUpload
User->>EncounterViewSet: Request Discharge Summary
EncounterViewSet->>DischargeSummaryTask: Generate Summary
DischargeSummaryTask-->>FileUpload: Create PDF
FileUpload-->>EncounterViewSet: Return File
EncounterViewSet-->>User: Provide Summary
Ah, the discharge summaries are finally getting the attention they deserve. How quaint. 😏 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (25)
docker/prod.Dockerfile (1)
1-1: Shouldn't we consider additional PDF security features for medical documents?While updating Typst is... fine, I notice we're using this for generating discharge summaries, which are sensitive medical documents. Perhaps we should consider:
- Adding PDF encryption capabilities
- Implementing digital signatures for medical documents
- Ensuring HIPAA compliance with the document generation process
Would you like me to suggest some security-focused additions to the Dockerfile?
care/templates/reports/patient_discharge_summary_pdf_template.typ (7)
3-4: Consider consolidating template tag loads.You may want to unify the newly added tag libraries into a single load statement, purely for tidiness. It's mostly an aesthetic preference, though.
46-48: Check for consistent naming in patient attribute filters.You have introduced filters like
field_name_to_labelfor gender and blood group. Make sure all patient properties use these filters (where relevant) for a consistent user experience.
62-70: Ensure admission details reflect bed info soon.Currently, the code references a TODO to show bed info instead of the facility name. Consider addressing that soon for more precise discharge documentation.
109-109: Reevaluate empty text usage.This line adds an empty text block. It might be intentional for spacing or layout, but confirm it’s needed to avoid extraneous placeholders.
113-113: Complete the TODO for comorbidity info.The comment suggests a pending feature. Remember to mark or resolve it before merging if required by your merge checklist.
145-176: Observations layout looks neat.Data is well-grouped with a subgrid. Consider adding more explicit null checks for optional fields, just in case.
210-210: Remove commented-out code if unused.The commented-out line at the end may lead to confusion in the future if not actually needed.
care/emr/reports/discharge_summary.py (3)
39-41: Lock setting logic is correct.Caching progress for 2 minutes ensures ephemeral locking. Please confirm that 2 minutes is sufficient for typical PDF generations, especially under stress conditions.
58-67: Duration formatting.Zero-padded hour:minute format is user-friendly. If multi-day stays are common, ensure partial days are displayed correctly (like “1 day and 3 hours”?).
137-178: Compile step with external call to Typst.Running
typstin a subprocess is powerful, but do keep an eye on performance and security. If you handle large documents, consider streaming the content or limiting concurrency.care/emr/api/viewsets/encounter.py (4)
1-2: New imports introduced.Everything needed for discharge summary tasks and validations is brought in. Check any linter warnings on unused imports if you see them cropping up in future.
252-273: Preview logic gracefully falls back to generating if absent.That’s a user-friendly approach. As a minor note, you might want to add more explicit messaging if no summary is found.
274-282: Email specification class.This helps to validate email input thoroughly, reducing failed email sends. Possibly expand validation if you need domain checks or additional constraints.
283-326: Solid approach to emailing discharge summaries.Just ensure that repeated attempts don’t spam if a large queue or slow generation occurs. The separate tasks are a nice asynchronous pattern.
care/emr/templatetags/data_formatting_extras.py (1)
32-37: New parse_iso_datetime filter.Shim for ISO date parsing is consistent with your server logic. Good addition, although watch out if any time zone offsets differ from ‘Z’.
care/emr/tasks/discharge_summary.py (1)
Line range hint
45-62: Emailing discharge summary task.Straightforward approach. If file retrieval fails, an error is logged, preventing silent failures. Sufficient for production usage, though you might consider more user-facing feedback.
care/emr/models/file_upload.py (1)
35-35: Consider refining the extension handling logic.
It works for most cases, but might fail for filenames containing multiple periods (e.g.my.file.upload.pdf). A small function dedicated to extracting the final extension would make this a little more future-proof.care/emr/templatetags/discharge_summary_utils.py (1)
22-35: Ensure consistent naming for summary filters.
Whiledischarge_summary_displayis clear, referencingClassChoicesmight require future maintenance if new classes emerge. A fallback for unknown classes is good, but consider extracting to a well-documented mapping for clarity.care/emr/resources/encounter/enum_display_names.py (1)
7-30: Avoid extensive match-case branches if possible.
These multiple explicit cases are helpful for clarity, but consider a dictionary-based lookup if the list of possible values grows or changes frequently.config/urls.py (1)
91-91: Double-check debug-only URL pattern.
This updated endpoint is placed under theif settings.DEBUG:block. If it's intentional for debugging, wonderful. Otherwise, ensure that production also provides the route, assuming you want everyone to have easy access.care/emr/models/patient.py (2)
5-5:pluralizeusage check.
Sincepluralizesometimes depends on Django templates, confirm that the context in which this method is called has necessary dependencies. Otherwise, consider using a small custom function for counting.
45-66: Add docstring forget_age.
The logic is excellent. A short docstring explaining the fallback onyear_of_birthand handling for deceased patients would improve readability. Also consider verifying edge cases (e.g., a bizarrely futuredate_of_birth).+ def get_age(self) -> str: + """ + Returns a human-readable age like '3 years', '2 months 7 days', '15 days', or '0 days' + depending on the difference between date_of_birth/year_of_birth + and deceased_datetime or the current time. + """ ...Makefile (1)
72-73: Careful with database resets.
Dropping and recreating the DB is straightforward, but if you ever run these commands in the wrong environment, it could be disastrous. Maybe add a small safe-guard or environment check.care/emr/migrations/0061_alter_medicationrequest_dosage_instruction.py (1)
1-18: Consider addressing the quote style inconsistencies.Not to be that reviewer, but the static analysis tool suggests using double quotes consistently. While this won't affect functionality, consistency in coding style makes the codebase just a tiny bit more pleasant to work with.
Apply this diff to maintain consistent quote style:
- dependencies = [ - ('emr', '0060_alter_medicationrequest_dosage_instruction'), - ] + dependencies = [ + ("emr", "0060_alter_medicationrequest_dosage_instruction"), + ] operations = [ migrations.AlterField( - model_name='medicationrequest', - name='dosage_instruction', + model_name="medicationrequest", + name="dosage_instruction", field=models.JSONField(blank=True, default=list, null=True), ), ]🧰 Tools
🪛 Ruff (0.8.2)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
care/static/images/logos/black-logo.svgis excluded by!**/*.svgcare/static/images/logos/light-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
Makefile(1 hunks)care/emr/api/viewsets/encounter.py(4 hunks)care/emr/migrations/0061_alter_medicationrequest_dosage_instruction.py(1 hunks)care/emr/models/file_upload.py(2 hunks)care/emr/models/patient.py(2 hunks)care/emr/reports/discharge_summary.py(1 hunks)care/emr/resources/encounter/enum_display_names.py(1 hunks)care/emr/resources/file_upload/spec.py(1 hunks)care/emr/tasks/discharge_summary.py(2 hunks)care/emr/templatetags/data_formatting_extras.py(2 hunks)care/emr/templatetags/discharge_summary_utils.py(1 hunks)care/facility/api/viewsets/legacy/patient_consultation.py(0 hunks)care/facility/templatetags/data_formatting_tags.py(0 hunks)care/facility/templatetags/prescription_tags.py(0 hunks)care/facility/tests/test_pdf_generation.py(1 hunks)care/facility/utils/reports/discharge_summary.py(0 hunks)care/templates/reports/patient_discharge_summary_pdf_template.typ(5 hunks)config/urls.py(2 hunks)docker/dev.Dockerfile(1 hunks)docker/prod.Dockerfile(1 hunks)
💤 Files with no reviewable changes (4)
- care/facility/api/viewsets/legacy/patient_consultation.py
- care/facility/utils/reports/discharge_summary.py
- care/facility/templatetags/prescription_tags.py
- care/facility/templatetags/data_formatting_tags.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0061_alter_medicationrequest_dosage_instruction.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
🔇 Additional comments (35)
docker/dev.Dockerfile (1)
3-3: Oh, updating Typst version... interesting choice.While I'm sure you have your reasons for updating to 0.12.0, it might be helpful to verify if this version is stable enough for production use, especially since it's quite new.
Let's check the release notes and known issues:
✅ Verification successful
Typst 0.12.0 seems quite stable and brings significant improvements
Based on the release notes, version 0.12.0 brings substantial improvements including:
- 2-3x performance boost through multithreading
- Up to 6x faster paragraph justification
- Highly reduced PDF file sizes
- Fixed critical printing issues with professional printers
- Fixed long-standing bugs with headings and layout
The reported bugs since release are minor and mostly cosmetic:
- Leading spaces in figure captions
- Path stroke gradient behavior
- Math equation display quirks
- URL formatting in bibliography
None of these issues affect core functionality or stability. The version appears production-ready, especially given the significant performance and stability improvements it brings... though I suppose one could always wait for 0.12.1 if they're particularly cautious.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Typst 0.12.0 release information and issues # Get release info gh api repos/typst/typst/releases/tags/v0.12.0 --jq '.body' # Check for critical issues after 0.12.0 release gh api repos/typst/typst/issues --jq '.[] | select(.labels[].name == "bug" and .state == "open" and .created_at > "2024-01-01")'Length of output: 41707
docker/prod.Dockerfile (1)
4-4: At least the version is consistent with dev.Dockerfile... that's something.While maintaining version parity between development and production environments is commendable, I notice this Dockerfile handles some rather sensitive health-related data processing. Perhaps we should consider:
- Documenting why this version update was necessary for the discharge summary feature
- Verifying that this version has been thoroughly tested with medical document generation
Let's verify the image builds successfully with the new version:
care/templates/reports/patient_discharge_summary_pdf_template.typ (7)
22-22: Verify facility name usage.Switching from
patient.facility.nametoencounter.facility.nameis correct in this new encounter-based approach, but please ensure all references align with how the facility is determined in your data model.
32-33: Nice utilization of discharge summary display filter.Using the filter to dynamically display the summary title looks good. Keep it up!
56-56: Informative heading usage.Referring to “Visit Details” with a larger font size is a helpful visual marker. No issues spotted here.
71-105: Comprehensive data capture for diagnoses, allergies, and symptoms.This block nicely captures multiple facets of the encounter. Double-check for any edge cases (e.g., no diagnoses) to avoid unexpected runtime errors.
115-142: Medication request section is well structured.Displaying medication details in a grid format provides clarity. Just confirm we handle large medication lists without layout breakage.
180-190: Discharge summary sections look correct.Applying logic to only display discharge information if
encounter.hospitalization.discharge_dispositionexists is spot on. Keep an eye on any location or billing disclaimers that might need adding.
206-206: Check file name hyphenation for potential multiline.You are hyphenating file names to handle lengthy text. Confirm that the underlying PDF engine breaks lines gracefully.
care/emr/reports/discharge_summary.py (9)
1-8: Imports look well-organized.All relevant modules are neatly imported for the new discharge summary flow. Good organizational practice here.
35-37: Straightforward lock key generator.Generating a unique cache key based on the encounter external ID is fine. Slightly passive suggestion: ensure no collisions if you ever use a similarly named key in another domain.
43-45: Simple retrieval of generation progress.No obvious issues. Just confirm you handle cases of
Nonewhen the lock is absent in consumer code.
47-49: Clear lock utility.Cleans up the cache key after completion. This is essential to avoid stale progress data.
51-56: Robust ISO datetime parsing.It’s helpful that on failure you return None. Confirm you handle None in subsequent calls (like duration calculations).
180-189: Streamlined PDF generation function.This is a succinct wrapper for the compilation routine. Logging success is beneficial for debugging.
231-242: Emailing the summary is straightforward.Attaching files as PDFs with the correct MIME type is done properly. Just be mindful of large attachments or concurrency limits in your email service.
244-257: Generating signed URL for discharge summary.Automatically creating a new summary if none exists is quite convenient. The 48-hour expiration is also a safe window. No major concerns here.
69-135:⚠️ Potential issueAggregating discharge summary data thoroughly.
This function pulls together all necessary info. The sorting for allergies by criticality is clever. Just watch for potential index errors if
diagnosesis empty while referencingdiagnoses[0].- principal_diagnosis = diagnoses[0] if diagnoses else None + principal_diagnosis = diagnoses[0] if diagnoses and len(diagnoses) > 0 else NoneLikely invalid or redundant comment.
care/emr/api/viewsets/encounter.py (2)
221-239: Internal method for generating discharge summary.Provides a nice handshake with the back-end tasks. The 406 HTTP status code usage is appropriate for “already in progress” scenarios.
240-251: Action to generate discharge summary.Publicly exposing
_generate_discharge_summarythroughgenerate_discharge_summaryis a good design. Just confirm that only authorized roles can trigger it.care/emr/templatetags/data_formatting_extras.py (2)
4-4: Additional timezone import.Looks fine. Watch that you don’t overshadow any existing imports inadvertently, though that’s unlikely here.
9-13: Helpful fallback withformat_empty_data.Returning “N/A” for missing or null data is user-friendly. Make sure “0.0” is indeed treated as empty for your use cases.
care/emr/tasks/discharge_summary.py (2)
9-10: Encounter model import.Transition from
PatientConsultationtoEncounteris consistent with the refactor. All references appear updated accordingly.
Line range hint
23-43: Rewritten generate task for encounters.Handling
Encounter.DoesNotExistensures graceful failure logs. The logging statements are helpful for debugging.care/emr/models/file_upload.py (2)
44-45: Slight caution withskip_internal_name.
Whenskip_internal_nameis passed in but set toFalse, the internal name is generated. However, you might want to double-check consumers of this method to ensure they’re aware of and properly handle the new argument.
47-48: Leverage Python 3.8+ Assignment Expressions.
The inline assignment forextensionis a welcome addition. Just make sure you verify it doesn’t break compatibility with older Python versions (if that’s still a concern). Also, consider guarding against potential corner cases ifextensionis an empty string.care/emr/templatetags/discharge_summary_utils.py (3)
17-18: No real concerns here.
Forwardingvaluetoget_admit_source_displaykeeps things consistent. The addition of a filter is well-structured.
44-54: Validate observation data keys before usage.
The logic tries different keys inobservation.value, but a double-check might prevent unwantedAttributeError. For instance, ifvalue_codeorvalue_quantityis unexpectedly missing or not a dict, we might see exceptions.
58-62: Great fallback handling for dosage instructions.
CatchingIndexError,KeyError, andTypeErrorhelps ensure no unexpected crashes. Possibly log these exceptions for better traceability, but given your context, this might be enough.care/emr/resources/file_upload/spec.py (1)
21-21: Enum member addition looks fine.
discharge_summaryenumerates a distinct category for file uploads. Just be sure your code that referencesFileCategoryChoicesis tested so the new category does not cause any surprises.care/emr/resources/encounter/enum_display_names.py (1)
33-58: Thoughtful naming for discharge dispositions.
These match statements read well, providing a direct mapping from enum to string. Just make sure to keep them updated if new enum values appear in the future.config/urls.py (1)
12-12: Reconfirm usage ofdev_preview_discharge_summary.
While importing from the newencountermodule is consistent with the rest of the PR, be sure all references and docstrings reflect this updated location so that future maintainers don’t need to guess which module it lives in.care/emr/models/patient.py (1)
1-1: Import usage clarity.
It’s appropriate to importrelativedeltahere, but you might want to confirm usage in test coverage to make sure you’re not pulling in a library without verifying its benefits.care/facility/tests/test_pdf_generation.py (1)
13-14: Ensure import references remain consistent.
Swapping the import sources is logical if the new module is indeed the permanent home for discharge summaries. Confirm that all references in the code, tests, and docs align with the new location to avoid confusion.care/emr/migrations/0061_alter_medicationrequest_dosage_instruction.py (1)
13-17: Consider the implications of using a mutable default.Using
default=listas a field default might lead to some... interesting behavior. While Django handles this correctly in migrations, it's worth noting that this pattern could be risky in model definitions. Also, the combination ofnull=Trueandblank=Truesuggests we're being quite permissive with this field. Are we absolutely sure we want both?For context, run this script to check field usage:
✅ Verification successful
Actually, the mutable default and null/blank combination is fine here
After reviewing the codebase, I can see that the
dosage_instructionfield is being used quite intentionally. The field stores a list of dosage instructions in JSON format, and Django actually handles mutable defaults correctly for JSONField (how thoughtful of them). The usage indischarge_summary_utils.pyshows it expects a list structure wheredosage_instruction[0]["text"]contains the dosage text.The null/blank combination makes perfect sense because:
- It's used in an EMR system where not all medication requests might need dosage instructions
- The field is optional in the FHIR specification (I noticed the DosageInstruction import)
- The API needs to handle both empty lists and null values consistently
I suppose I should mention that if you're creating model instances in Python code (not through Django's ORM), you might want to be extra careful with the mutable default, but that's more of a general Python gotcha than a real issue here.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how dosage_instruction is used in views and serializers ast-grep --pattern 'class $_(ViewSet|Serializer): $$$ dosage_instruction $$$'Length of output: 85
Script:
#!/bin/bash # Let's check for any usage of dosage_instruction in the codebase rg "dosage_instruction" -B 2 -A 2 # Also check model definition fd "medicationrequest.py" --exec cat {}Length of output: 9291
Script:
#!/bin/bash # Check if there are any tests related to this field rg "test.*medication.*dosage" -g "test_*.py"Length of output: 46
Script:
#!/bin/bash # Let's see if there are any serializers that might have been missed rg "class.*Serializer.*Medication" -g "*.py"Length of output: 46
Script:
#!/bin/bash # Check the migration history for this field fd "migrations" --exec rg "dosage_instruction" {} \;Length of output: 1186
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/migrations/0061_alter_medicationrequest_dosage_instruction.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
care/emr/utils/file_manager.py (1)
41-41: Sensible use ofResponseContentDisposition.Attaching the filename upon download is a user-friendly approach. A small caution though: certain client browsers might behave unpredictably if the filename is too long or contains spaces/special characters. But as far as this change goes, all good.
care/emr/api/viewsets/encounter.py (2)
221-226: Consider refining access checks.
The_check_discharge_summary_accessmethod is straightforward. However, if multiple calls rely on similar checks for clinical data access, you might centralize that logic as a shared utility or permission check. This would prevent any future drift if additional discharge-related features appear.
337-352: Small readability improvement fordev_preview_discharge_summary.
This dev-only endpoint effectively returns the PDF preview. That's great. Still, consider logging usage or gating it behind a dev setting to prevent accidental exposure in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/encounter.py(4 hunks)care/emr/utils/file_manager.py(1 hunks)
🔇 Additional comments (2)
care/emr/api/viewsets/encounter.py (2)
281-289: Good introduction of a Pydantic model for email validations.
It’s a clean approach to ensure email format correctness. In some cases, you might extend this model further to handle domain allowlists or extra validations, depending on compliance requirements.
298-335: Potential risk of concurrency inemail_discharge_summary.
If multiple requests come in quickly, there's a possibility that the lock set by_generate_discharge_summarydoesn’t apply here. You might ensure concurrency is fully handled or at least documented to avoid multiple summary generations.
Proposed Changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Improvements
Bug Fixes
Refactoring