feat:Added proper validations and testcases to diagnostic report api#3566
Open
nandkishorr wants to merge 11 commits intodevelopfrom
Open
feat:Added proper validations and testcases to diagnostic report api#3566nandkishorr wants to merge 11 commits intodevelopfrom
nandkishorr wants to merge 11 commits intodevelopfrom
Conversation
…dkishorr/fix/diagnostic_report
Contributor
📝 WalkthroughWalkthroughThe pull request refactors authorization checks in the diagnostic report viewset to use explicit PermissionDenied exceptions instead of conditional filtering, and introduces a comprehensive test suite covering CRUD operations, filtering, and observation upserts across multiple permission scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
care/emr/api/viewsets/diagnostic_report.py (1)
148-158: Redundant patient filter on line 158.The queryset is already filtered by
patient=self.get_patient_obj()on line 117. The additional.filter(patient=patient)on line 158 is technically harmless but unnecessary.🔧 Suggested simplification
# Authorize with Patient patient = self.get_patient_obj() if not AuthorizationController.call( "can_view_clinical_data", self.request.user, patient, ): raise PermissionDenied( "You do not have permission to view clinical data for this patient" ) - return queryset.filter(patient=patient) + return queryset🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/api/viewsets/diagnostic_report.py` around lines 148 - 158, The extra queryset.filter(patient=patient) after the authorization check is redundant because get_patient_obj() was already used to filter the queryset earlier; remove the final .filter(patient=patient) and simply return the existing queryset after the AuthorizationController.call check (locate this in the DiagnosticReportViewSet or the view method using get_patient_obj, AuthorizationController.call and PermissionDenied).care/emr/tests/test_diagnostic_report_api.py (3)
576-594: Samesetdefaultpattern as noted earlier.The
create_observationhelper also usessetdefaulton kwargs, which has the same mutation side effect mentioned before.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_diagnostic_report_api.py` around lines 576 - 594, The helper create_observation mutates the incoming kwargs by repeatedly calling kwargs.setdefault (on keys like "diagnostic_report", "encounter", "patient", "created_by"), which can cause surprising side effects for callers; replace the setdefault usage by constructing local defaults and merging them with a new dict (e.g., build a local defaults = {...} using self.diagnostic_report, self.encounter, self.patient, self.superuser and then do data.update(defaults) followed by data.update(kwargs)) so create_observation no longer mutates the caller's kwargs.
1-1: Consider usingrandom.choiceinstead ofsecrets.choice.The
secretsmodule is designed for cryptographic purposes like generating tokens and passwords. For test fixtures where security isn't a concern,random.choiceis the conventional and more readable option.🔧 Suggested change
-from secrets import choice +from random import choice🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_diagnostic_report_api.py` at line 1, Replace the cryptographic import and usage of secrets.choice with the standard library random.choice in the test file: change the import from "from secrets import choice" to use "import random" (or "from random import choice") and update any calls that reference choice to use random.choice (or leave as choice if using from random import choice); target the test module where the import occurs and the functions/tests that rely on choice so tests use random.choice for non-crypto randomness.
80-96:setdefaultmutates the incomingkwargsdictionary.Using
setdefaultonkwargsmodifies the caller's dictionary in place, which could cause surprising behavior if the same dict were ever reused. This is a minor concern since each test typically passes fresh kwargs, but it's worth noting for maintainability.🔧 Safer approach using `get` with defaults
def create_diagnostic_report(self, **kwargs): data = { "status": DiagnosticReportStatusChoices.final.value, "category": { "display": "Laboratory", "code": "LAB", "system": "http://terminology.hl7.org/CodeSystem/v2-0074", }, - "service_request": kwargs.setdefault( - "service_request", self.service_request - ), - "patient": kwargs.setdefault("patient", self.patient), - "encounter": kwargs.setdefault("encounter", self.encounter), - "facility": kwargs.setdefault("facility", self.facility), + "service_request": kwargs.get("service_request", self.service_request), + "patient": kwargs.get("patient", self.patient), + "encounter": kwargs.get("encounter", self.encounter), + "facility": kwargs.get("facility", self.facility), } data.update(**kwargs) return baker.make(DiagnosticReport, **data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_diagnostic_report_api.py` around lines 80 - 96, The helper create_diagnostic_report mutates the incoming kwargs via kwargs.setdefault; change it to avoid in-place mutation by reading defaults with kwargs.get (e.g., service_request = kwargs.get("service_request", self.service_request), patient = kwargs.get("patient", self.patient), encounter = kwargs.get("encounter", self.encounter), facility = kwargs.get("facility", self.facility)), use those local variables when building data, then call data.update(kwargs) and return baker.make(DiagnosticReport, **data) so the original kwargs is not modified; update the setdefault usages in create_diagnostic_report accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@care/emr/api/viewsets/diagnostic_report.py`:
- Around line 148-158: The extra queryset.filter(patient=patient) after the
authorization check is redundant because get_patient_obj() was already used to
filter the queryset earlier; remove the final .filter(patient=patient) and
simply return the existing queryset after the AuthorizationController.call check
(locate this in the DiagnosticReportViewSet or the view method using
get_patient_obj, AuthorizationController.call and PermissionDenied).
In `@care/emr/tests/test_diagnostic_report_api.py`:
- Around line 576-594: The helper create_observation mutates the incoming kwargs
by repeatedly calling kwargs.setdefault (on keys like "diagnostic_report",
"encounter", "patient", "created_by"), which can cause surprising side effects
for callers; replace the setdefault usage by constructing local defaults and
merging them with a new dict (e.g., build a local defaults = {...} using
self.diagnostic_report, self.encounter, self.patient, self.superuser and then do
data.update(defaults) followed by data.update(kwargs)) so create_observation no
longer mutates the caller's kwargs.
- Line 1: Replace the cryptographic import and usage of secrets.choice with the
standard library random.choice in the test file: change the import from "from
secrets import choice" to use "import random" (or "from random import choice")
and update any calls that reference choice to use random.choice (or leave as
choice if using from random import choice); target the test module where the
import occurs and the functions/tests that rely on choice so tests use
random.choice for non-crypto randomness.
- Around line 80-96: The helper create_diagnostic_report mutates the incoming
kwargs via kwargs.setdefault; change it to avoid in-place mutation by reading
defaults with kwargs.get (e.g., service_request = kwargs.get("service_request",
self.service_request), patient = kwargs.get("patient", self.patient), encounter
= kwargs.get("encounter", self.encounter), facility = kwargs.get("facility",
self.facility)), use those local variables when building data, then call
data.update(kwargs) and return baker.make(DiagnosticReport, **data) so the
original kwargs is not modified; update the setdefault usages in
create_diagnostic_report accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87feb246-9c46-4116-bc38-6d72acd5bec9
📒 Files selected for processing (2)
care/emr/api/viewsets/diagnostic_report.pycare/emr/tests/test_diagnostic_report_api.py
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
1 participant
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
Added detailed permission validation exceptions
Added testcases for the diagnostic report api
ohcnetwork/roadmap#246
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
Release Notes
Bug Fixes
Tests