Data points for patient level report builder #3419
Conversation
📝 WalkthroughWalkthroughThis PR adds invoice filter cleanup, introduces authorization and context builder infrastructure for patient and account reports, registers two new report types (patient summary and account report), and implements comprehensive data point context builders for accounts, invoices, charge items, payment reconciliations, and monetary components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
care/emr/reports/authorizers/patient.py (1)
8-8: Add type hints for theuserparameter.The
userparameter lacks type annotations, which would improve code clarity and enable better IDE support.If using Django's built-in User model:
+from django.contrib.auth.models import User + class PatientReportAuthorizer(BaseReportAuthorizer): - def authorize_read(self, user, associating_id: str) -> bool: + def authorize_read(self, user: User, associating_id: str) -> bool: patient_obj = get_object_or_404(Patient, external_id=associating_id) return AuthorizationController.call("can_view_clinical_data", user, patient_obj) - def authorize_write(self, user, associating_id: str) -> bool: + def authorize_write(self, user: User, associating_id: str) -> bool: patient_obj = get_object_or_404(Patient, external_id=associating_id) return AuthorizationController.call("can_write_patient_obj", user, patient_obj)Also applies to: 12-12
care/emr/reports/context_builder/data_points/payment_reconcilation.py (1)
56-58: Annotate the mutable class attribute withClassVar.The
__filterset_backends__class attribute should be annotated withtyping.ClassVarto make it clear this is a class-level attribute and prevent potential mutation issues.Based on static analysis tools.
Apply this diff:
+from typing import ClassVar + class PaymentReconciliationContextBuilder(QuerysetContextBuilder): filterset_class = PaymentReconciliationReportFilter - __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__: ClassVar = [filters.DjangoFilterBackend]
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
care/emr/api/viewsets/invoice.py(0 hunks)care/emr/reports/authorizers/__init__.py(1 hunks)care/emr/reports/authorizers/discharge_summary.py(1 hunks)care/emr/reports/authorizers/patient.py(1 hunks)care/emr/reports/context_builder/__init__.py(1 hunks)care/emr/reports/context_builder/data_points/account.py(1 hunks)care/emr/reports/context_builder/data_points/charge_items.py(1 hunks)care/emr/reports/context_builder/data_points/invoice.py(1 hunks)care/emr/reports/context_builder/data_points/patient.py(1 hunks)care/emr/reports/context_builder/data_points/payment_reconcilation.py(1 hunks)care/emr/reports/report_types.py(2 hunks)
💤 Files with no reviewable changes (1)
- care/emr/api/viewsets/invoice.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/reports/context_builder/__init__.pycare/emr/reports/authorizers/__init__.pycare/emr/reports/context_builder/data_points/patient.pycare/emr/reports/context_builder/data_points/payment_reconcilation.pycare/emr/reports/authorizers/patient.pycare/emr/reports/authorizers/discharge_summary.pycare/emr/reports/context_builder/data_points/invoice.pycare/emr/reports/context_builder/data_points/charge_items.pycare/emr/reports/report_types.pycare/emr/reports/context_builder/data_points/account.py
🧠 Learnings (1)
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/reports/report_types.py
🧬 Code graph analysis (8)
care/emr/reports/authorizers/__init__.py (3)
care/emr/reports/authorizers/discharge_summary.py (2)
DischargeBillReportAuthorizer(9-10)DischargeSummaryReportAuthorizer(5-6)care/emr/reports/authorizers/encounter.py (1)
EncounterReportAuthorizer(7-18)care/emr/reports/authorizers/utils.py (1)
report_authorizer(6-37)
care/emr/reports/context_builder/data_points/patient.py (4)
care/emr/models/patient.py (2)
age(77-81)get_age(54-74)care/emr/reports/context_builder/data_point_registry.py (1)
DataPointRegistry(1-31)care/emr/reports/context_builder/data_points/account.py (1)
AccountContextBuilder(42-125)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)SingleObjectContextBuilder(128-129)
care/emr/reports/context_builder/data_points/payment_reconcilation.py (5)
care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)care/emr/reports/context_builder/data_points/charge_items.py (2)
get_context(77-78)get_context(82-83)care/emr/reports/context_builder/data_points/invoice.py (2)
get_context(57-58)get_context(62-63)
care/emr/reports/authorizers/patient.py (3)
care/emr/reports/authorizers/base.py (1)
BaseReportAuthorizer(4-11)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)care/utils/shortcuts.py (1)
get_object_or_404(6-15)
care/emr/reports/authorizers/discharge_summary.py (2)
care/emr/reports/authorizers/patient.py (1)
PatientReportAuthorizer(7-14)care/emr/reports/authorizers/encounter.py (1)
EncounterReportAuthorizer(7-18)
care/emr/reports/context_builder/data_points/invoice.py (5)
care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)care/emr/reports/context_builder/data_points/payment_reconcilation.py (1)
get_context(133-134)care/emr/reports/context_builder/data_points/charge_items.py (2)
get_context(77-78)get_context(82-83)
care/emr/reports/context_builder/data_points/charge_items.py (5)
care/emr/models/charge_item.py (1)
ChargeItem(7-48)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)care/emr/reports/context_builder/data_points/payment_reconcilation.py (1)
get_context(133-134)care/emr/reports/context_builder/data_points/invoice.py (2)
get_context(57-58)get_context(62-63)
care/emr/reports/context_builder/data_points/account.py (4)
care/emr/models/account.py (1)
Account(7-28)care/emr/reports/context_builder/data_points/charge_items.py (3)
AccountChargeItemContextBuilder(81-83)get_context(77-78)get_context(82-83)care/emr/reports/context_builder/data_points/invoice.py (3)
AccountInvoiceContextBuilder(61-63)get_context(57-58)get_context(62-63)care/emr/reports/context_builder/data_points/payment_reconcilation.py (2)
PaymentReconciliationContextBuilder(56-134)get_context(133-134)
🪛 Ruff (0.14.8)
care/emr/reports/context_builder/data_points/payment_reconcilation.py
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/reports/context_builder/data_points/invoice.py
26-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/reports/context_builder/data_points/charge_items.py
34-34: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/reports/context_builder/data_points/account.py
44-44: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (14)
care/emr/reports/context_builder/data_points/patient.py (1)
20-42: Field definitions are well-structured.The field definitions for patient attributes are clear, include appropriate preview values, and the age field correctly uses the
get_age()method from the Patient model.care/emr/reports/context_builder/data_points/charge_items.py (2)
9-23: Display mappings implemented correctly.The display mappings for charge item resources and statuses provide appropriate fallback behavior using
replace("_", " ").title()when keys aren't found in the dictionaries.
32-83: Context builder implementation follows established patterns.The
ChargeItemContextBuilderandAccountChargeItemContextBuilderare well-structured with appropriate field definitions, filtering, and context resolution methods. The use of lambda mappings with None-checking ensures graceful handling of missing values.care/emr/reports/context_builder/data_points/invoice.py (1)
9-63: Invoice context builder implementation is consistent and correct.The implementation follows the same well-established pattern as other context builders in this PR. Field definitions include appropriate display metadata, the status mapping handles missing values gracefully, and both patient-level and account-level context resolution are properly implemented.
care/emr/reports/context_builder/data_points/account.py (1)
42-121: Comprehensive field definitions for account reporting.The field definitions cover all relevant account attributes with appropriate display names, preview values, and human-readable mappings for status fields. The nested context fields for invoices, charge items, and payment reconciliations properly establish relationships with related entities.
care/emr/reports/context_builder/__init__.py (1)
2-2: Wildcard import follows established pattern.Adding the patient data points export is consistent with the existing encounter imports and appropriately exposes the patient context builder to the module's public API.
care/emr/reports/authorizers/discharge_summary.py (1)
9-10: Authorizer correctly inherits patient-level permissions.The
DischargeBillReportAuthorizerappropriately extendsPatientReportAuthorizerto inherit authorization logic for reading and writing patient-related reports. The empty implementation is consistent with the existingDischargeSummaryReportAuthorizerpattern.care/emr/reports/authorizers/__init__.py (1)
3-6: Module exports updated correctly.The new
DischargeBillReportAuthorizeris properly imported and added to the__all__list, making it available to consumers of this module.Also applies to: 12-12
care/emr/reports/report_types.py (1)
16-22: Report type registration is properly structured.The new
discharge_billreport type registration correctly associates the Patient model with the DischargeBillReportAuthorizer and includes an appropriate description.care/emr/reports/authorizers/patient.py (2)
1-4: LGTM!Imports are well-organized and follow Django conventions.
10-10: No action required. The authorization actionscan_view_clinical_dataandcan_write_patient_objare already properly defined in the registered patient authorization handler.Likely an incorrect or invalid review comment.
care/emr/reports/context_builder/data_points/payment_reconcilation.py (3)
48-53: LGTM!The FilterSet implementation correctly uses UUID filters for foreign key relationships and case-insensitive matching for string fields, following established patterns in the codebase.
60-131: Field definitions follow established patterns.The Field definitions correctly use lambda mappings with fallback logic (
dict.get(key, default_value)), which is consistent with other context builders in the codebase. Just remember to update the dictionary references in lines 63, 71, 91, 105, 115, and 125 when you fix the spelling of those mapping dictionaries mentioned earlier.
133-134: LGTM!The
get_contextmethod correctly filters PaymentReconciliation by account, following the same pattern as other account-scoped context builders (InvoiceContextBuilder, ChargeItemContextBuilder).
care/emr/reports/context_builder/data_points/payment_reconcilation.py
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3419 +/- ##
===========================================
+ Coverage 74.60% 74.72% +0.12%
===========================================
Files 449 457 +8
Lines 20646 20819 +173
Branches 2141 2141
===========================================
+ Hits 15402 15558 +156
- Misses 4782 4799 +17
Partials 462 462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nandkishorr
changed the title
…into datapoint/patient
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
care/emr/reports/context_builder/data_points/account.py (1)
51-67: Callable mappings will fail if account is None.The
get_context()method returns.first()which yieldsNonewhen no accounts exist. The lambda mappings forstatusandbilling_statuswill raiseAttributeErrorwhen attempting to accessa.statusora.billing_statuson aNoneobject.
🧹 Nitpick comments (3)
care/emr/reports/context_builder/data_points/payment_reconciliation.py (2)
88-95: Preview value doesn't match any mapped value.The
preview_valueis "Credit" but thePAYMENT_RECONCILIATION_KINDmapping contains "Deposit", "Periodic Payment", "Online", and "Kiosk". Using a value that doesn't appear in the actual data might be slightly confusing for users previewing the report.kind = Field( display="Kind", - preview_value="Credit", + preview_value="Deposit", mapping=lambda p: PAYMENT_RECONCILIATION_KIND.get(p.kind, p.kind.title()) if p.kind else "", description="Kind of payment reconciliation", )
112-121: Preview value doesn't match any mapped value.The
preview_valueis "Success" butPAYMENT_RECONCILIATION_OUTCOMEmaps to "Queued", "Complete", "Error", and "Partial". I'm sure "Success" makes perfect sense somehow.outcome = Field( display="Outcome", - preview_value="Success", + preview_value="Complete", mapping=lambda p: PAYMENT_RECONCILIATION_OUTCOME.get( p.outcome, p.outcome.title() ) if p.outcome else "", description="Outcome of the payment reconciliation process", )care/emr/reports/context_builder/data_points/account.py (1)
123-125: Consider documenting the assumption about parent_context.This implementation assumes
parent_contextis always aPatientinstance. While this may be guaranteed by the report infrastructure, a brief comment clarifying this expectation might help future maintainers understand the contract. Just a thought.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/reports/context_builder/data_points/account.py(1 hunks)care/emr/reports/context_builder/data_points/payment_reconciliation.py(1 hunks)care/emr/reports/report_types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/reports/context_builder/data_points/payment_reconciliation.pycare/emr/reports/report_types.pycare/emr/reports/context_builder/data_points/account.py
🧠 Learnings (1)
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/reports/report_types.py
🧬 Code graph analysis (3)
care/emr/reports/context_builder/data_points/payment_reconciliation.py (3)
care/emr/models/payment_reconciliation.py (1)
PaymentReconciliation(6-29)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)
care/emr/reports/report_types.py (5)
care/emr/models/patient.py (1)
Patient(16-133)care/emr/reports/authorizers/discharge_summary.py (1)
DischargeBillReportAuthorizer(9-10)care/emr/reports/report_type_registry.py (2)
ReportTypeRegistry(23-87)register(27-50)care/emr/reports/context_builder/data_point_registry.py (1)
register(6-7)care/emr/reports/context_builder/type_registry.py (1)
register(5-6)
care/emr/reports/context_builder/data_points/account.py (5)
care/emr/models/account.py (1)
Account(7-28)care/emr/reports/context_builder/data_points/base.py (1)
Field(7-54)care/emr/reports/context_builder/data_points/charge_items.py (3)
AccountChargeItemContextBuilder(81-83)get_context(77-78)get_context(82-83)care/emr/reports/context_builder/data_points/invoice.py (3)
AccountInvoiceContextBuilder(61-63)get_context(57-58)get_context(62-63)care/emr/reports/context_builder/data_points/payment_reconciliation.py (2)
PaymentReconciliationContextBuilder(56-134)get_context(133-134)
🪛 Ruff (0.14.8)
care/emr/reports/context_builder/data_points/payment_reconciliation.py
58-58: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/reports/context_builder/data_points/account.py
44-44: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (5)
care/emr/reports/report_types.py (1)
16-22: LGTM!The new report type registration follows the established pattern and correctly associates the patient bill summary with the
Patientmodel andDischargeBillReportAuthorizer. All required parameters are properly provided.care/emr/reports/context_builder/data_points/payment_reconciliation.py (1)
133-133: LGTM!The
get_contextmethod correctly returns a queryset filtered by the parent account context, consistent with the pattern used byAccountInvoiceContextBuilderandAccountChargeItemContextBuilder.care/emr/reports/context_builder/data_points/account.py (3)
1-16: LGTM!Imports are well-organized and all imported context builders are properly utilized in the Field definitions below.
18-32: LGTM!Status mapping constants are well-defined with clear, human-readable display values.
94-111: LGTM!The related context fields for invoices, charge items, and payment reconciliations are correctly wired to their respective context builders using
target_context.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
care/emr/reports/context_builder/data_points/patient.py (1)
12-42: Missingget_context()method - addressing prior feedback.This class extends
SingleObjectContextBuilderbut doesn't implementget_context(). The previous review correctly identified this gap. Without this method, the builder won't know how to retrieve the patient instance when rendering the report context.🔎 Suggested implementation
accounts = Field( display="Patient Accounts", target_context=PatientAccountContextBuilder, preview_value="", description="Accounts associated with the patient", ) + + def get_context(self): + return self.parent_contextcare/emr/reports/context_builder/data_points/account.py (2)
43-60: Lambda mappings will fail when context is None.I notice the previous review comment about None handling hasn't been addressed yet. The lambda mappings in
statusandbilling_statusfields access attributes directly on the context object without checking if it's None. SincePatientAccountContextBuilder.get_context()uses.first()(Line 125), it can return None when no account exists for the patient, causing anAttributeErrorat runtime.You'll need to either:
- Add None validation in
PatientAccountContextBuilder.get_context()to raise a clear exception, or- Update these lambda mappings to handle None gracefully
🔎 Proposed fix for None-safe lambda mappings
status = Field( display="Account Status", preview_value="Active", - mapping=lambda a: STATUS_DISPLAY.get(a.status, a.status.title()) - if a.status - else "", + mapping=lambda a: ( + STATUS_DISPLAY.get(a.status, a.status.title()) + if a and a.status + else "" + ), description="Current status of the account", ) billing_status = Field( display="Account Billing Status", preview_value="Billed", - mapping=lambda a: BILLING_STATUS_DISPLAY.get( - a.billing_status, a.billing_status.title() - ) - if a.billing_status - else "", + mapping=lambda a: ( + BILLING_STATUS_DISPLAY.get(a.billing_status, a.billing_status.title()) + if a and a.billing_status + else "" + ), description="Billing status of the account", )
122-125: Still returning None from get_context without validation.This method uses
.first()which returns None when no account exists for the patient. As previously noted, this will cause downstream AttributeErrors in the field lambda mappings.Consider one of these approaches:
- Validate and raise a clear exception if no account exists
- Return a sentinel/empty context object with safe defaults
- Ensure all upstream code paths validate account existence before rendering
🔎 Proposed fix - validate account existence
def get_context(self): accounts = Account.objects.filter(patient=self.parent_context) - return accounts.first() + account = accounts.first() + if account is None: + raise ValueError( + f"No account found for patient {self.parent_context.id}" + ) + return account
🧹 Nitpick comments (3)
care/emr/reports/context_builder/data_points/charge_items.py (1)
36-38: Annotate mutable class attribute withClassVar.The static analysis tool (Ruff RUF012) correctly identifies that
__filterset_backends__is a mutable class attribute. While this works at runtime, annotating it withClassVarimproves type-checking clarity and signals that this attribute is shared across instances.🔎 Suggested fix
+from typing import ClassVar + from django_filters import rest_framework as filters ... class ChargeItemContextBuilder(QuerysetContextBuilder): filterset_class = ChargeItemReportFilter - __filterset_backends__ = [filters.DjangoFilterBackend] + __filterset_backends__: ClassVar[list] = [filters.DjangoFilterBackend]care/emr/reports/context_builder/data_points/invoice.py (2)
21-24: Consider adding Meta class to specify the model.While the FilterSet defines appropriate filters, it's missing a
Metaclass with themodelattribute. Django FilterSet best practices recommend explicitly declaring the model being filtered.🔎 Proposed enhancement
class InvoiceReportFilter(filters.FilterSet): status = filters.CharFilter(lookup_expr="iexact") title = filters.CharFilter(lookup_expr="icontains") number = filters.CharFilter(lookup_expr="icontains") + + class Meta: + model = Invoice + fields = ["status", "title", "number"]
36-42: Consider adding defensive None check in status mapping.While the lambda checks
if i.status, it doesn't verify thatiitself is not None. Though unlikely with QuerysetContextBuilder, defensive coding would make this safer.🔎 Proposed enhancement
status = Field( display="Invoice Status", preview_value="Issued", - mapping=lambda i: STATUS_DISPLAY.get(i.status, i.status.title()) - if i.status - else "", + mapping=lambda i: ( + STATUS_DISPLAY.get(i.status, i.status.title()) + if i and i.status + else "" + ), description="Current status of the invoice", )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
care/emr/reports/authorizers/__init__.pycare/emr/reports/authorizers/account.pycare/emr/reports/context_builder/__init__.pycare/emr/reports/context_builder/data_points/account.pycare/emr/reports/context_builder/data_points/charge_items.pycare/emr/reports/context_builder/data_points/invoice.pycare/emr/reports/context_builder/data_points/monetary_component.pycare/emr/reports/context_builder/data_points/patient.pycare/emr/reports/report_types.py
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/reports/authorizers/init.py
- care/emr/reports/context_builder/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/reports/authorizers/account.pycare/emr/reports/context_builder/data_points/charge_items.pycare/emr/reports/report_types.pycare/emr/reports/context_builder/data_points/patient.pycare/emr/reports/context_builder/data_points/invoice.pycare/emr/reports/context_builder/data_points/account.pycare/emr/reports/context_builder/data_points/monetary_component.py
🧠 Learnings (1)
📚 Learning: 2025-07-15T13:39:29.540Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3148
File: care/emr/api/viewsets/consent.py:22-24
Timestamp: 2025-07-15T13:39:29.540Z
Learning: In the care codebase, BaseModel (in care/utils/models/base.py) contains an external_id field which is inherited by all EMR models including Encounter. This allows filtering using field paths like `encounter__external_id` in Django filters.
Applied to files:
care/emr/reports/report_types.py
🧬 Code graph analysis (6)
care/emr/reports/authorizers/account.py (4)
care/emr/models/account.py (1)
Account(7-28)care/emr/reports/authorizers/base.py (1)
BaseReportAuthorizer(4-11)care/security/authorization/base.py (2)
AuthorizationController(72-113)call(97-108)care/utils/shortcuts.py (1)
get_object_or_404(6-15)
care/emr/reports/context_builder/data_points/charge_items.py (5)
care/emr/models/charge_item.py (1)
ChargeItem(7-48)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/monetary_component.py (3)
MonetaryComponentContextBuilder(15-45)get_context(44-45)get_context(49-50)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)care/emr/reports/context_builder/data_points/invoice.py (2)
get_context(66-67)get_context(71-72)
care/emr/reports/report_types.py (5)
care/emr/models/account.py (1)
Account(7-28)care/emr/models/patient.py (1)
Patient(16-133)care/emr/reports/authorizers/account.py (1)
AccountReportAuthorizer(7-18)care/emr/reports/authorizers/patient.py (1)
PatientReportAuthorizer(7-14)care/emr/reports/report_type_registry.py (2)
ReportTypeRegistry(23-87)register(27-50)
care/emr/reports/context_builder/data_points/invoice.py (5)
care/emr/models/invoice.py (1)
Invoice(7-31)care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)care/emr/reports/context_builder/data_points/monetary_component.py (3)
MonetaryComponentContextBuilder(15-45)get_context(44-45)get_context(49-50)care/emr/reports/context_builder/data_points/account.py (1)
get_context(123-125)care/emr/reports/context_builder/data_points/charge_items.py (2)
get_context(94-95)get_context(99-100)
care/emr/reports/context_builder/data_points/account.py (5)
care/emr/models/account.py (1)
Account(7-28)care/emr/reports/context_builder/data_point_registry.py (1)
DataPointRegistry(1-31)care/emr/reports/context_builder/data_points/charge_items.py (3)
AccountChargeItemContextBuilder(98-100)get_context(94-95)get_context(99-100)care/emr/reports/context_builder/data_points/invoice.py (3)
AccountInvoiceContextBuilder(70-72)get_context(66-67)get_context(71-72)care/emr/reports/context_builder/data_points/monetary_component.py (2)
get_context(44-45)get_context(49-50)
care/emr/reports/context_builder/data_points/monetary_component.py (1)
care/emr/reports/context_builder/data_points/base.py (2)
Field(7-54)QuerysetContextBuilder(99-125)
🪛 Ruff (0.14.8)
care/emr/reports/context_builder/data_points/charge_items.py
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
care/emr/reports/context_builder/data_points/invoice.py
29-29: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (18)
care/emr/reports/authorizers/account.py (1)
7-18: LGTM!The
AccountReportAuthorizercorrectly implements theBaseReportAuthorizerinterface, delegating toAuthorizationControllerwith appropriate action strings for facility-scoped permission checks. The use ofget_object_or_404ensures proper 404 handling for invalidexternal_idvalues.care/emr/reports/context_builder/data_points/patient.py (1)
20-42: Field definitions look good.The field mappings are well-structured. The
agefield correctly usesget_age()which returns a human-readable string (e.g., "45 years"), andaccountsproperly links toPatientAccountContextBuilderfor nested context resolution.care/emr/reports/report_types.py (2)
1-9: Imports are correctly organized.The new imports for
Account,Patient,AccountReportAuthorizer, andPatientReportAuthorizerare appropriately added to support the new report type registrations.
18-32: New report type registrations look correct.Both
patient_summaryandaccount_reportregistrations follow the established pattern, pairing the correct models with their respective authorizers. The descriptions are clear and consistent with the naming convention.care/emr/reports/context_builder/data_points/charge_items.py (3)
13-27: Display mappings are comprehensive.The status and resource display mappings cover the expected values with sensible fallback logic for unmapped values. This approach ensures graceful degradation when new statuses or resources are introduced.
40-92: Field definitions are well-structured.The field mappings handle edge cases appropriately (null-checking
ci.statusandci.service_resourcebefore operations). The monetary component fields correctly delegate to their respective context builders.
94-100: Context retrieval is correctly scoped.
ChargeItemContextBuilder.get_context()filters bypatient, whileAccountChargeItemContextBuildercorrectly overrides to filter byaccount. This follows the same pattern established ininvoice.py.care/emr/reports/context_builder/data_points/monetary_component.py (3)
6-12: Display mapping is clear and complete.The
MONETARY_COMPONENT_TYPE_DISPLAYmapping covers the standard monetary component types with readable labels.
15-42: Field definitions look reasonable, with a minor note on robustness.The
codefield mapping assumesmc.get("code")returns a dict with adisplaykey. This should generally hold true given the JSONField definition, but if malformed data ever sneaks in, the inner.get("display", "")call would raise anAttributeError. Just something to keep in mind if data integrity becomes a concern down the road.Consider adding defensive handling:
mapping=lambda mc: (mc.get("code") or {}).get("display", ""),
44-50: Context methods are correctly implemented.
MonetaryComponentContextBuilderreturnstotal_price_componentswhile the subclass correctly overrides to returnunit_price_components. This pattern allows reuse of field definitions while varying the data source.care/emr/reports/context_builder/data_points/account.py (4)
1-18: LGTM!Imports are well-organized and follow PEP 8 conventions.
20-34: LGTM!Display mappings are well-defined and follow naming conventions.
61-119: Field definitions are well-structured.The remaining field definitions follow a consistent pattern with appropriate display names, preview values, and descriptions. The fields using
target_contextproperly reference related context builders.
128-137: LGTM!AccountContextBuilder is properly configured as a standalone context with appropriate metadata and registry registration.
care/emr/reports/context_builder/data_points/invoice.py (4)
1-10: LGTM!Imports are properly organized and follow conventions.
12-18: LGTM!Status display mapping is well-defined and follows conventions.
44-67: LGTM!Field definitions are well-structured with appropriate descriptions and preview values. The
get_contextmethod properly filters invoices by patient.
70-72: LGTM!AccountInvoiceContextBuilder appropriately extends InvoiceContextBuilder to filter by account. The implementation is clean and follows the established pattern.
| filterset_class = InvoiceReportFilter | ||
| __filterset_backends__ = [filters.DjangoFilterBackend] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Annotate mutable class attribute with ClassVar.
The static analysis tool correctly identified that __filterset_backends__ is a mutable class attribute (list) that should be annotated with typing.ClassVar to indicate it's a class variable rather than an instance variable.
🔎 Proposed fix
Add the import at the top of the file:
+from typing import ClassVar
+
from django_filters import rest_framework as filtersThen annotate the class attribute:
class InvoiceContextBuilder(QuerysetContextBuilder):
filterset_class = InvoiceReportFilter
- __filterset_backends__ = [filters.DjangoFilterBackend]
+ __filterset_backends__: ClassVar = [filters.DjangoFilterBackend]📝 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.
| filterset_class = InvoiceReportFilter | |
| __filterset_backends__ = [filters.DjangoFilterBackend] | |
| filterset_class = InvoiceReportFilter | |
| __filterset_backends__: ClassVar = [filters.DjangoFilterBackend] |
🧰 Tools
🪛 Ruff (0.14.8)
29-29: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In care/emr/reports/context_builder/data_points/invoice.py around lines 28-29,
the mutable class attribute __filterset_backends__ is currently a plain list and
should be annotated as a class variable; add "from typing import ClassVar, List"
to imports and change the attribute annotation to use ClassVar (e.g.,
__filterset_backends__: ClassVar[List] = [filters.DjangoFilterBackend]) so
static analysis knows it’s a class-level mutable constant.
Proposed Changes
Architecture 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
✏️ Tip: You can customize this high-level summary in your review settings.