[ENG-830] feat:support for diagnsotic reports and medication prescriptions at Patient OTP level - #3720
Open
nandkishorr wants to merge 12 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesOTP EMR endpoints
OTP booking response
Estimated code review effort: 3 (Moderate) | ~15 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds patient-OTP-authenticated read access to existing EMR resources (Diagnostic Reports and Medication Prescriptions), enabling patients logged in via OTP to list/retrieve these records scoped by their phone number.
Changes:
- Registered two new OTP API routes for diagnostic reports and medication prescriptions.
- Added OTP-authenticated ViewSets for
DiagnosticReportandMedicationRequestPrescriptionwith phone-number-scoped querysets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| config/api_router.py | Registers OTP routes for diagnostic reports and medication prescriptions. |
| care/emr/api/otp_viewsets/medication_request_prescription.py | Adds OTP-authenticated list/retrieve access to medication prescriptions scoped by patient phone number. |
| care/emr/api/otp_viewsets/diagnostic_report.py | Adds OTP-authenticated list/retrieve access to diagnostic reports scoped by patient phone number. |
Comment on lines
+138
to
+142
| router.register( | ||
| "otp/diagnostic_report", | ||
| OTPDiagnosticReportViewSet, | ||
| basename="otp-diagnostic-report", | ||
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/api/otp_viewsets/diagnostic_report.py (1)
14-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare mutable DRF class configuration with
ClassVar.
care/emr/api/otp_viewsets/diagnostic_report.py#L14-L15: annotateauthentication_classesandpermission_classes.care/emr/api/otp_viewsets/medication_request_prescription.py#L16-L17: annotate the corresponding attributes.As per coding guidelines, run
ruff check --fix .andruff format .after the change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/otp_viewsets/diagnostic_report.py` around lines 14 - 15, Annotate the authentication_classes and permission_classes attributes with ClassVar in care/emr/api/otp_viewsets/diagnostic_report.py:14-15 and care/emr/api/otp_viewsets/medication_request_prescription.py:16-17, preserving their existing values. Run ruff check --fix . and ruff format . afterward.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@care/emr/api/otp_viewsets/diagnostic_report.py`:
- Around line 20-23: Update get_queryset in
care/emr/api/otp_viewsets/diagnostic_report.py at lines 20-23 to filter from
super().get_queryset() while preserving the patient phone-number condition; make
the same change in care/emr/api/otp_viewsets/medication_request_prescription.py
at lines 22-25. This retains EMRBaseViewSet’s -id ordering in both OTP
querysets.
---
Nitpick comments:
In `@care/emr/api/otp_viewsets/diagnostic_report.py`:
- Around line 14-15: Annotate the authentication_classes and permission_classes
attributes with ClassVar in care/emr/api/otp_viewsets/diagnostic_report.py:14-15
and care/emr/api/otp_viewsets/medication_request_prescription.py:16-17,
preserving their existing values. Run ruff check --fix . and ruff format .
afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 697881fd-9c01-4de8-b340-4f4c8a953dd3
📒 Files selected for processing (3)
care/emr/api/otp_viewsets/diagnostic_report.pycare/emr/api/otp_viewsets/medication_request_prescription.pyconfig/api_router.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3720 +/- ##
===========================================
+ Coverage 79.45% 79.58% +0.12%
===========================================
Files 480 483 +3
Lines 23214 23324 +110
Branches 2420 2429 +9
===========================================
+ Hits 18445 18562 +117
+ Misses 4165 4156 -9
- Partials 604 606 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/emr/api/otp_viewsets/diagnostic_report.py (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace mutable viewset configuration lists with immutable tuples.
Ruff’s RUF012 warning applies to both viewsets.
care/emr/api/otp_viewsets/diagnostic_report.py#L23-L25: convertordering_fieldsandfilter_backendsto tuples.care/emr/api/otp_viewsets/medication_request_prescription.py#L27-L29: make the same conversion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/otp_viewsets/diagnostic_report.py` around lines 23 - 25, Replace the mutable viewset configuration lists with tuples in care/emr/api/otp_viewsets/diagnostic_report.py lines 23-25 and care/emr/api/otp_viewsets/medication_request_prescription.py lines 27-29: convert ordering_fields and filter_backends in both viewsets to immutable tuples, preserving their existing values and order.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@care/emr/api/otp_viewsets/diagnostic_report.py`:
- Around line 23-25: Replace the mutable viewset configuration lists with tuples
in care/emr/api/otp_viewsets/diagnostic_report.py lines 23-25 and
care/emr/api/otp_viewsets/medication_request_prescription.py lines 27-29:
convert ordering_fields and filter_backends in both viewsets to immutable
tuples, preserving their existing values and order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d411d499-0578-494a-b536-fbe93608fa9e
📒 Files selected for processing (2)
care/emr/api/otp_viewsets/diagnostic_report.pycare/emr/api/otp_viewsets/medication_request_prescription.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
config/api_router.py:142
- These new OTP endpoints add patient-facing access to clinical data, but there are no corresponding API tests in the suite to verify (1) list/retrieve works with patient OTP auth and (2) data is restricted to the token phone number (i.e., other patients’ records are not returned). There are already OTP API tests for
otp/slots(seecare/emr/tests/test_booking_api.py), so similar coverage should be added forotp/diagnostic_reportandotp/medication_prescription.
router.register(
"otp/diagnostic_report",
OTPDiagnosticReportViewSet,
basename="otp-diagnostic-report",
)
care/emr/api/otp_viewsets/medication_request_prescription.py:34
get_queryset()bypassesEMRBaseViewSet.get_queryset(), so you lose the defaultorder_by("-id"). Since this viewset supportslistwith pagination, the underlying queryset should have a deterministic ordering to avoid unstable pagination (duplicates/missing items between pages). Prefer filteringsuper().get_queryset()instead of starting fromModel.objectsdirectly.
def get_queryset(self):
return MedicationRequestPrescription.objects.filter(
patient__phone_number=self.request.user.phone_number
)
care/emr/api/otp_viewsets/diagnostic_report.py:30
get_queryset()bypassesEMRBaseViewSet.get_queryset(), so you lose the defaultorder_by("-id"). Since this viewset supportslistwith pagination, the underlying queryset should have a deterministic ordering to avoid unstable pagination (duplicates/missing items between pages). Prefer filteringsuper().get_queryset()instead of starting fromModel.objectsdirectly.
def get_queryset(self):
return DiagnosticReport.objects.filter(
patient__phone_number=self.request.user.phone_number
)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/emr/api/otp_viewsets/diagnostic_report.py (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAddress the duplicated Ruff RUF012 findings consistently.
Both viewsets define
filter_backendsas a mutable class attribute. Annotate each asClassVaror use an immutable tuple.
care/emr/api/otp_viewsets/diagnostic_report.py#L23-L23: updatefilter_backends.care/emr/api/otp_viewsets/medication_request_prescription.py#L27-L27: apply the same fix.As per coding guidelines, Python changes must be checked with Ruff.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/otp_viewsets/diagnostic_report.py` at line 23, Address the duplicated Ruff RUF012 findings by updating the filter_backends class attributes in care/emr/api/otp_viewsets/diagnostic_report.py lines 23-23 and care/emr/api/otp_viewsets/medication_request_prescription.py lines 27-27 to use ClassVar annotations or immutable tuples. Apply the same consistent fix in both viewsets and verify the Python changes with Ruff.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@care/emr/api/otp_viewsets/diagnostic_report.py`:
- Line 23: Address the duplicated Ruff RUF012 findings by updating the
filter_backends class attributes in
care/emr/api/otp_viewsets/diagnostic_report.py lines 23-23 and
care/emr/api/otp_viewsets/medication_request_prescription.py lines 27-27 to use
ClassVar annotations or immutable tuples. Apply the same consistent fix in both
viewsets and verify the Python changes with Ruff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d55b40b-31d3-48c4-a71a-97cfba20d88f
📒 Files selected for processing (3)
care/emr/api/otp_viewsets/diagnostic_report.pycare/emr/api/otp_viewsets/medication_request_prescription.pyconfig/api_router.py
🚧 Files skipped from review as they are similar to previous changes (1)
- config/api_router.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
care/emr/api/otp_viewsets/medication_request_prescription.py:24
- This PR adds new OTP-authenticated endpoints, but there are no corresponding API tests to verify phone-number scoping for list/retrieve (and that filtering/ordering behaves as expected). There are existing OTP endpoint tests (e.g.,
care/emr/tests/test_booking_api.py’sTestOtpSlotViewSet) that can be used as a template for OTP authentication setup.
class OTPMedicationRequestPrescriptionViewSet(
EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin
):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = MedicationRequestPrescription
pydantic_read_model = MedicationRequestPrescriptionReadSpec
care/emr/api/otp_viewsets/diagnostic_report.py:24
- The PR description mentions request-side ordering, but this OTP viewset only enables DjangoFilterBackend, so
?ordering=requests won’t work (unlike the mainDiagnosticReportViewSet, which includesOrderingFilterandordering_fields). Add DRF’sOrderingFilterand expose the same ordering fields here.
database_model = DiagnosticReport
pydantic_read_model = DiagnosticReportListSpec
pydantic_retrieve_model = DiagnosticReportRetrieveSpec
filterset_class = DiagnosticReportFilters
filter_backends = [filters.DjangoFilterBackend]
care/emr/api/otp_viewsets/medication_request_prescription.py:2
- To support request-side ordering (
?ordering=) as described in the PR, this viewset needs DRF’sOrderingFilteravailable (currently onlydjango_filtersis imported asfilters).
This issue also appears on line 18 of the same file.
from django_filters import rest_framework as filters
care/emr/api/otp_viewsets/medication_request_prescription.py:28
- The PR description mentions request-side ordering, but this OTP viewset only enables
DjangoFilterBackend, so?ordering=won’t work. Add DRF’sOrderingFilterand exposecreated_date/modified_dateas ordering fields for parity withMedicationRequestPrescriptionViewSet.
filterset_class = MedicationRequestPrescriptionFilter
filter_backends = [filters.DjangoFilterBackend]
care/emr/api/otp_viewsets/diagnostic_report.py:20
- This PR adds new OTP-authenticated endpoints, but there are no corresponding API tests to ensure (1) only reports for the authenticated phone number are returned and (2) retrieve of another patient’s report is blocked. There are existing OTP endpoint tests (e.g.,
care/emr/tests/test_booking_api.py’sTestOtpSlotViewSet) that can be followed forforce_authenticatewithPatientOtpObject.
class OTPDiagnosticReportViewSet(EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = DiagnosticReport
pydantic_read_model = DiagnosticReportListSpec
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
care/emr/api/otp_viewsets/medication_request_prescription.py:39
- The OTP list/retrieve endpoints will likely trigger extra DB queries during serialization (e.g., prescribed_by / encounter / audit users). The non-OTP MedicationRequestPrescriptionViewSet uses select_related() to mitigate this; doing the same here will reduce N+1 query risk for OTP clients.
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
care/emr/api/otp_viewsets/medication_request_prescription.py:27
- New OTP endpoint behavior (auth via patient JWT, filtering by phone_number, and filter parameters like status/facility/patient/created_date) is introduced here but there are no corresponding tests covering list and retrieve access control. Please add API tests to verify that (1) only records belonging to the authenticated phone_number are returned and (2) filtering works as expected.
class OTPMedicationRequestPrescriptionViewSet(
EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin
):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
care/emr/api/otp_viewsets/diagnostic_report.py:36
- This viewset serializes related objects (service_request, encounter, audit users) but the queryset does not eager-load them, which can cause avoidable extra queries on list/retrieve. Adding select_related() here keeps OTP endpoints more efficient under pagination.
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
care/emr/api/otp_viewsets/diagnostic_report.py:26
- New OTP endpoint behavior (auth via patient JWT, filtering by phone_number, and filter parameters like status/facility/patient/created_date) is introduced here but there are no tests exercising the list/retrieve endpoints. Please add API tests to ensure a patient OTP token cannot access another patient’s diagnostic reports and that the query filters behave correctly.
class OTPDiagnosticReportViewSet(EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = DiagnosticReport
pydantic_read_model = DiagnosticReportListSpec
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/emr/api/otp_viewsets/diagnostic_report.py (1)
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the mutable class attributes for Ruff.
authentication_classesandpermission_classestrigger Ruff RUF012 as mutable class attributes. AddClassVarannotations so the required lint check passes; Ruff is being unusually helpful here.As per coding guidelines, Ruff is the required Python lint and formatting tool.
Suggested fix
+from typing import ClassVar + class OTPDiagnosticReportViewSet(EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin): - authentication_classes = [JWTTokenPatientAuthentication] - permission_classes = [OTPAuthenticatedPermission] + authentication_classes: ClassVar[list] = [JWTTokenPatientAuthentication] + permission_classes: ClassVar[list] = [OTPAuthenticatedPermission]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@care/emr/api/otp_viewsets/diagnostic_report.py` around lines 24 - 25, Annotate the authentication_classes and permission_classes class attributes in the diagnostic report viewset with typing.ClassVar, preserving their existing list values, and add the required import so Ruff RUF012 passes.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@care/emr/api/otp_viewsets/diagnostic_report.py`:
- Around line 24-25: Annotate the authentication_classes and permission_classes
class attributes in the diagnostic report viewset with typing.ClassVar,
preserving their existing list values, and add the required import so Ruff
RUF012 passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4aefa3f8-0056-49b0-a6fc-212afcc6e0cb
📒 Files selected for processing (2)
care/emr/api/otp_viewsets/diagnostic_report.pycare/emr/api/otp_viewsets/medication_request_prescription.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
config/api_router.py:142
- New OTP endpoints for diagnostic reports and medication prescriptions are introduced here, but this PR doesn't add API tests covering OTP auth, scoping to the authenticated phone number, and filter behavior (status/facility/encounter/date range). Given the repo has extensive API tests, adding coverage would help prevent regressions and unintended data exposure.
router.register(
"otp/diagnostic_report",
OTPDiagnosticReportViewSet,
basename="otp-diagnostic-report",
)
care/emr/api/otp_viewsets/medication_request_prescription.py:26
- The mixin inheritance order is inconsistent with other EMR/OTP viewsets (typically mixins first, EMRBaseViewSet last). Keeping a consistent MRO makes it easier to reason about which mixin implements list/retrieve and avoids surprises if EMRBaseViewSet later adds methods that overlap mixins.
class OTPMedicationRequestPrescriptionViewSet(
EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin
):
care/emr/api/otp_viewsets/diagnostic_report.py:37
- This queryset scopes records only by patient phone number. Because Patient uses soft-delete (BaseModel.deleted) and default managers are not applied across FK joins, this can still return diagnostic reports belonging to soft-deleted patients with the same phone number. Consider explicitly excluding deleted patients and eager-loading related objects to avoid N+1 queries during list/retrieve serialization.
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
care/emr/api/otp_viewsets/medication_request_prescription.py:40
- This queryset scopes records only by patient phone number. Because Patient uses soft-delete (BaseModel.deleted) and default managers are not applied across FK joins, this can still return prescriptions belonging to soft-deleted patients with the same phone number. Consider explicitly excluding deleted patients and eager-loading related objects to avoid per-row queries during serialization.
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
care/emr/api/otp_viewsets/diagnostic_report.py:23
- The mixin inheritance order is inconsistent with other EMR/OTP viewsets (typically mixins first, EMRBaseViewSet last). Aligning the order keeps MRO predictable for list/retrieve behavior.
class OTPDiagnosticReportViewSet(EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin):
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@care/emr/resources/scheduling/slot/spec.py`:
- Line 147: Update the Pydantic model fields named associated_encounter in both
the shown model and TokenBookingRetrieveSpec to use Field(default_factory=dict)
instead of a mutable dictionary literal, and import Field from Pydantic.
Preserve the existing dictionary type and default behavior while ensuring each
model instance receives its own dictionary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d9391fe3-2689-45ea-88ba-cf87b84db36a
📒 Files selected for processing (1)
care/emr/resources/scheduling/slot/spec.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
care/emr/api/otp_viewsets/diagnostic_report.py:37
DiagnosticReportListSpecserialization readsobj.service_requestto build the response, but this queryset does notselect_related("service_request")(or other FKs likeencounter/facility). On list endpoints this can cause N+1 queries during serialization.
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
)
care/emr/api/otp_viewsets/diagnostic_report.py:31
- There are no automated tests covering these new OTP-authenticated list/retrieve diagnostic report endpoints (especially verifying patient scoping via OTP token and filter behavior). The repo already has API tests for the non-OTP diagnostic report endpoints (e.g.
care/emr/tests/test_diagnostic_report_api.py), so adding OTP-specific tests would help prevent access-control regressions.
class OTPDiagnosticReportViewSet(EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = DiagnosticReport
pydantic_read_model = DiagnosticReportListSpec
pydantic_retrieve_model = DiagnosticReportRetrieveSpec
filterset_class = OTPDiagnosticReportFilters
filter_backends = [filters.DjangoFilterBackend]
care/emr/api/otp_viewsets/medication_request_prescription.py:40
get_queryset()doesn’tselect_related()any of the foreign keys that the list/retrieve specs access (notablyprescribed_by), so listing many prescriptions can trigger N+1 queries when serializingMedicationRequestPrescriptionReadSpec. Align this with the non-OTPMedicationRequestPrescriptionViewSet, whichselect_related(...)for list performance.
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
)
care/emr/api/otp_viewsets/medication_request_prescription.py:33
- There are no automated tests covering these new OTP-authenticated list/retrieve endpoints (including scoping to
request.user.phone_numberand filter behavior). The repo already has API test coverage for the non-OTP prescription endpoints (e.g.care/emr/tests/test_medication_request_prescription_api.py), so adding OTP-specific tests would help prevent regressions in access control.
class OTPMedicationRequestPrescriptionViewSet(
EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin
):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = MedicationRequestPrescription
pydantic_read_model = MedicationRequestPrescriptionReadSpec
pydantic_retrieve_model = MedicationRequestPrescriptionRetrieveMedicationsSpec
filterset_class = OTPMedicationPrescriptionFilter
filter_backends = [filters.DjangoFilterBackend]
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
config/settings/base.py:693
- The new OTP diagnostic/medication endpoints are effectively disabled by default because
QuerysetEnablerMixin.get_queryset()returns.none()unlessOTP_QUERYSET_ENABLEDis true. If the intent is for the feature to be available immediately, consider enabling by default or documenting this required flag as part of the change.
OTP_QUERYSET_ENABLED = env.bool("OTP_QUERYSET_ENABLED", default=False)
care/emr/api/otp_viewsets/diagnostic_report.py:42
- New OTP-authenticated diagnostic report list/retrieve behavior is introduced here, but there are no corresponding API tests covering OTP auth, patient phone scoping, and filtering (contrast with existing OTP slot tests in
care/emr/tests/test_booking_api.py).
class OTPDiagnosticReportViewSet(
QuerysetEnablerMixin, EMRRetrieveMixin, EMRBaseViewSet, EMRListMixin
):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = DiagnosticReport
pydantic_read_model = DiagnosticReportListSpec
pydantic_retrieve_model = DiagnosticReportRetrieveSpec
filterset_class = OTPDiagnosticReportFilters
filter_backends = [filters.DjangoFilterBackend]
resource_type = OTPResourceType.diagnostic_report
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
)
care/emr/api/otp_viewsets/medication_request_prescription.py:45
- New OTP-authenticated medication prescription list/retrieve behavior is introduced here, but there are no corresponding API tests covering OTP auth, patient phone scoping, and filtering (contrast with existing OTP slot tests in
care/emr/tests/test_booking_api.py).
class OTPMedicationRequestPrescriptionViewSet(
QuerysetEnablerMixin,
EMRRetrieveMixin,
EMRBaseViewSet,
EMRListMixin,
):
authentication_classes = [JWTTokenPatientAuthentication]
permission_classes = [OTPAuthenticatedPermission]
database_model = MedicationRequestPrescription
pydantic_read_model = MedicationRequestPrescriptionReadSpec
pydantic_retrieve_model = MedicationRequestPrescriptionRetrieveMedicationsSpec
filterset_class = OTPMedicationRequestPrescriptionFilters
filter_backends = [filters.DjangoFilterBackend]
resource_type = OTPResourceType.medication_request_prescription
def get_queryset(self):
return (
super()
.get_queryset()
.filter(patient__phone_number=self.request.user.phone_number)
)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
care/emr/api/otp_viewsets/base.py:30
apply_default_filters()assumesget_env_value()returns a list of dicts, butjson.loads()can also yield a dict / scalar (or a list containing non-objects). In those cases the loop will raiseAttributeErrorand return a 500 instead of a controlled 400 validation error. Validate that the env value is a JSON array of objects (and return an empty list when unset) so misconfiguration is handled safely.
def get_env_value(self, key):
if not getenv(key):
return {}
try:
return json.loads(getenv(key))
praffq
requested changes
Aug 4, 2026
praffq
approved these changes
Aug 5, 2026
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
care/emr/api/otp_viewsets/base.py:34
get_env_valuecan return a non-list JSON value (e.g.{}), in which caseapply_default_filterswill iterate the dict keys and fail atfilter_config.get(...). Returning{}when unset also makes the return type inconsistent (dict vs list). Consider always returning a list and validating the decoded type, raising a 400 if it's not a list of filter configs.
def get_env_value(self, key):
if not getenv(key):
return {}
try:
return json.loads(getenv(key))
except json.JSONDecodeError as e:
raise ValidationError(
{key: "Invalid JSON in default filter configuration."}
) from e
care/emr/tests/test_otp_diagnostic_report_api.py:196
- Test name looks inverted:
test_retrieve_diagnostic_report_without_queryset_disableduses@override_settings(OTP_QUERYSET_ENABLED=False)and asserts a 404. Renaming to reflect that the queryset is disabled will make intent clearer and avoid confusion when scanning failures.
@override_settings(OTP_QUERYSET_ENABLED=False)
def test_retrieve_diagnostic_report_without_queryset_disabled(self):
Comment on lines
+40
to
+45
| def get_queryset(self): | ||
| return ( | ||
| super() | ||
| .get_queryset() | ||
| .filter(patient__phone_number=self.request.user.phone_number) | ||
| ) |
Comment on lines
+37
to
+42
| def get_queryset(self): | ||
| return ( | ||
| super() | ||
| .get_queryset() | ||
| .filter(patient__phone_number=self.request.user.phone_number) | ||
| ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (4)
care/emr/api/otp_viewsets/base.py:25
resource_typeis set to theOTPResourceTypeenum class by default, butconfig_key()assumes it is an enum value (self.resource_type.value). If a viewset forgets to overrideresource_type, this will raise an AttributeError at runtime. Consider making the default explicit (None) and failing fast with a clearer error.
resource_type = OTPResourceType
def config_key(self):
return f"OTP_{self.resource_type.value.upper()}_FILTERS"
care/emr/api/otp_viewsets/base.py:35
get_env_value()returns{}when the env var is unset and does not validate the decoded JSON shape. If the env var is accidentally set to a JSON object (dict) instead of a list,apply_default_filters()will iterate string keys and crash with an AttributeError atfilter_config.get(...)(500). Returning a list consistently and validating the type prevents this misconfiguration from taking down the endpoint.
def get_env_value(self, key):
if not getenv(key):
return {}
try:
return json.loads(getenv(key))
except json.JSONDecodeError as e:
raise ValidationError(
{key: "Invalid JSON in default filter configuration."}
) from e
care/emr/tests/test_otp_diagnostic_report_api.py:196
- Test name reads as a double-negative:
without_queryset_disabledis confusing given the test asserts behavior whenOTP_QUERYSET_ENABLED=False. Renaming will make intent clearer when the test fails.
def test_retrieve_diagnostic_report_without_queryset_disabled(self):
care/emr/resources/scheduling/slot/spec.py:158
- Serializing
obj.associated_encounterwill trigger an additional DB query for each booking unless the queryset isselect_related('associated_encounter'). This can easily become an N+1 query problem for OTP appointment listings (OTPSlotViewSet.get_appointments).
if obj.associated_encounter_id:
mapping["associated_encounter"] = EncounterSpecBase.serialize(
obj.associated_encounter
).to_json()
Contributor
|
Tests failing |
Contributor
Author
I reran it and it's fine now. |
8 tasks
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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