Add chronic condition api #2775
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several modifications across multiple files, focusing on enhancing chronic condition management, updating migration scripts, and refining API viewsets. The changes primarily involve adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant ChronicConditionViewSet
participant AuthorizationController
participant Patient
participant Encounter
User->>ChronicConditionViewSet: Create/Update Chronic Condition
ChronicConditionViewSet->>AuthorizationController: Check Permissions
AuthorizationController-->>ChronicConditionViewSet: Permission Result
ChronicConditionViewSet->>Patient: Validate Patient Access
ChronicConditionViewSet->>Encounter: Check Encounter Status
ChronicConditionViewSet->>ChronicConditionViewSet: Perform Create/Update
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
care/emr/api/viewsets/condition.py (4)
6-15: Cohesive import structure
You’ve consistently imported necessary modules (mixins, models, specs,AuthorizationController). While it’s workable, a small check: be sure that all newly introduced dependencies are indeed used, to prevent build-up of unused imports over time.Also applies to: 19-19, 25-28, 31-31
170-175: authorize_create
Good that you’re checkingcan_write_patient_objfor the identifiedpatient_external_id. The error message, however, references updating an “encounter,” which may confuse the user if the real error pertains to the patient. Consider updating the message for clarity.- raise PermissionDenied("You do not have permission to update encounter") + raise PermissionDenied("You do not have permission to create a condition for this patient")
176-179: authorize_update
Delegating toauthorize_createis a pragmatic approach, though it might be slightly indirect. If the create permission check changes in the future, keep in mind to update this as well.
186-192: perform_update
Verifiescan_update_encounter_objbefore proceeding, which is crucial. The message references encounter updates, which can be relevant, but remember you’re also dealing with overall condition data.care/emr/migrations/0011_alter_condition_encounter.py (1)
10-10: Convert single quotes to double quotes
The static analysis suggests using double quotes for consistency, which might be mandated by project style guidelines. While it's mostly a style concern, it prevents minor friction during lint checks.- ('emr', '0010_condition_abatement'), + ("emr", "0010_condition_abatement"), - model_name='condition', - name='encounter', - field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='emr.encounter'), + model_name="condition", + name="encounter", + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to="emr.encounter"),Also applies to: 15-17
🧰 Tools
🪛 Ruff (0.8.2)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/utils/tests/test_feature_flags.py (1)
3-3: Consider restoring coverage for flag unregistration.
The removal oftest_unregister_flag_not_founddecreases test coverage and could mask future regressions. While dropping tests may appear to simplify maintenance now, it might lead to nasty surprises later if theunregisterlogic breaks.config/settings/test.py (1)
71-75: Ensure the logging level suits your needs.
Adding the"audit_log"logger is great, but setting the level to"ERROR"might hide critical warnings or informational messages. If you truly only need to see critical failures, this works; otherwise, consider a lower threshold.care/emr/resources/condition/spec.py (2)
123-124: Consider a fallback for missing encounters.
The conditional avoids settingmapping["encounter"]ifobj.encounterisNone. This is a safe approach. If there's any scenario where an empty encounter must be surfaced differently, you may want to revisit.
149-154: Add robust validation logic.
ChronicConditionUpdateSpecelegantly extendsConditionUpdateSpec. You might still want additional validation that checks whetherencountercan be safely updated for chronic conditions. This could avoid confusion if an encounter ID is introduced that doesn’t align with the current patient’s record.care/emr/api/viewsets/base.py (1)
148-155: Keep an eye on partial update edge cases.
Introducingkeep_fieldsinclean_update_dataprovides flexibility, but it may be worth verifying that you don’t accidentally pop essential fields during partial updates. A docstring or mini-test verifying expected behavior would help.config/api_router.py (1)
11-15: Clean import groupingConsider grouping imports consistently with other sections. It’s not a big deal, but standardizing import blocks might help keep imports structured.
care/emr/tests/test_chronic_condition_api.py (1)
209-228: Slight duplication in permission checksMultiple tests repeat the same arrangement of permission checks for encounters. This is almost perfect, though you could consider using a helper method to reduce repetition and improve maintainability.
pyproject.toml (1)
3-6: Omitting concurrency flagsCommenting out concurrency-related coverage features might slow down coverage runs for larger codebases. If parallel execution was only temporarily disabled, consider re-enabling it for performance gains.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
care/abdm/migrations/0014_replace_0013.py(0 hunks)care/abdm/migrations_old/0013_abhanumber_patient.py(0 hunks)care/audit_log/middleware.py(1 hunks)care/emr/api/viewsets/allergy_intolerance.py(0 hunks)care/emr/api/viewsets/base.py(1 hunks)care/emr/api/viewsets/condition.py(4 hunks)care/emr/migrations/0011_alter_condition_encounter.py(1 hunks)care/emr/models/condition.py(1 hunks)care/emr/resources/condition/spec.py(4 hunks)care/emr/tests/test_booking_api.py(5 hunks)care/emr/tests/test_chronic_condition_api.py(1 hunks)care/emr/tests/test_schedule_api.py(4 hunks)care/utils/tests/test_feature_flags.py(1 hunks)config/api_router.py(2 hunks)config/settings/test.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (3)
- care/abdm/migrations_old/0013_abhanumber_patient.py
- care/abdm/migrations/0014_replace_0013.py
- care/emr/api/viewsets/allergy_intolerance.py
✅ Files skipped from review due to trivial changes (3)
- care/audit_log/middleware.py
- care/emr/tests/test_schedule_api.py
- care/emr/tests/test_booking_api.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0011_alter_condition_encounter.py
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: 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)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
care/emr/api/viewsets/condition.py (7)
3-4: Seamless permission/error import usage
The addition ofPermissionDeniedandValidationErrorensures better error handling. It's acceptable, though you might consider selectively raising exceptions if you foresee multiple failure scenarios to provide more context for clients.
69-70: SymptomViewSet model references updated
Switching toConditionReadSpecandConditionUpdateSpecaligns well with the renamed classes. This keeps the code cleaner and more consistent with the new spec naming.
109-110: DiagnosisViewSet spec usage
Likewise, usingConditionReadSpecandConditionUpdateSpecensures consistent naming. No concerns here.
142-142: Introduction of ChronicConditionViewSet
This class extends multiple mixins, making it flexible for CRUD operations on conditions designated as chronic. Just keep an eye on future expansions to avoid an overly large class.
165-168: get_patient_obj
Excellent approach to retrieving thePatientinstance. Do confirm that your error handling is user-friendly when the patient is missing (thoughget_object_or_404typically suffices).
183-185: clean_update_data
Preserving theencounterfield is wise to ensure continuity of data. This is concise and consistent with the new approach of possibly having conditions without strict encounter ties.
193-209: get_queryset
Filtering bypatient_external_idandcategory=chronic_conditionhelps isolate relevant conditions. The additional permission check withcan_view_clinical_datais properly placed.care/emr/models/condition.py (1)
14-16: Encounter field changes
Allowingnull=True, blank=Trueis a practical step for chronic conditions that may not always associate with a specific encounter. However, in some edge cases, you might consider verifying how the rest of the code handles aConditionwithout an encounter, ensuring no subtle null pointer issues arise.✅ Verification successful
Encounter field handling is properly implemented
After reviewing the codebase, I can confirm that null encounters are handled safely throughout. The code consistently uses Django's ORM features and proper null-safe filtering methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching references for Condition and verifying no code path incorrectly assumes a non-null encounter. rg -A 5 "Condition"Length of output: 21973
care/emr/resources/condition/spec.py (4)
5-5: Validate custom exception handling.
Replacing manual lookups withget_object_or_404is convenient, though keep in mind it raisesHttp404directly. Confirm this aligns with how you handle errors in other parts of the codebase.
100-100: Renaming for clarity.
RenamingConditionSpecReadtoConditionReadSpecis a neater naming convention. Small improvement that makes the class purpose less ambiguous.
132-132: Renaming for consistency.
SwappingConditionSpecUpdateforConditionUpdateSpecunifies naming. It's a subtle detail, but good for code clarity.
147-148: Empty lines.
The added blank lines appear intentional to separate sections. No immediate concerns here.config/api_router.py (1)
250-252: Neat route additionRegistering the
chronic_conditionroute is consistent with existing patterns. The name is well-chosen and helps keep endpoints scannable.care/emr/tests/test_chronic_condition_api.py (3)
22-49: Robust test setupThe
setUpandtearDownusage and mocking strategy are well-structured and thorough. This approach ensures consistent test data and environment, which is commendably detail-oriented.
280-304: Comprehensive scenario coverageGood job covering both read and write permission checks. It’s refreshing to see such attention to detail, ensuring all angles are tested.
680-766: Encounter reassignment logicThe tests for changing an encounter’s ID on an existing chronic condition are invaluable. However, a quick verification for partial updates (PATCH) might help ensure we don’t inadvertently swap encounters or break partial updates.
pyproject.toml (2)
16-25: Improved coverage exclusionsShifting to
exclude_alsowith more conditions is well-planned, though do ensure that critical lines are not inadvertently excluded. Overly broad rules can hide coverage gaps.
35-36: Python version alignmentUpdating the Ruff target to
py313is great, but please confirm the environment is prepared for Python 3.13. It’d be unfortunate if your build environment is still pinned to 3.12.
sainak
force-pushed
the
sainak/chronic-condition
branch
from
ba0dbbd to
237fa80
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pyproject.toml (1)
16-31: Oh, I see we're being thorough with our exclusions... very thorough.The switch to
exclude_alsois good (it's the newer recommended approach), but we might want to consider:
- Do we really need to duplicate the
omitpatterns from the run section? It feels a bit... excessive.- The exclusion of
if settings.DEBUGmight hide important debug-only code paths.Consider consolidating the omit patterns into a single variable:
[tool] omit_patterns = [ "care/facility/**", "care/users/**", "*/tests/**", "*/migrations/**" ] [tool.coverage.run] omit = "${tool.omit_patterns}" [tool.coverage.report] omit = "${tool.omit_patterns}"care/emr/api/viewsets/condition.py (2)
142-150: Add docstrings to describe the viewset's purpose and permissions.The viewset could use some documentation about its purpose and required permissions. You know, just in case someone else needs to maintain this code someday... 😊
170-174: Consider consolidating duplicate permission checks.The permission check for
can_write_patientis duplicated inauthorize_createandperform_update. Perhaps we could extract this into a shared method?+ def _check_write_permission(self): + if not AuthorizationController.call( + "can_write_patient", self.request.user, self.get_patient_obj() + ): + raise PermissionDenied("You do not have permission to update encounter") + def authorize_create(self, instance): - if not AuthorizationController.call( - "can_write_patient", self.request.user, self.get_patient_obj() - ): - raise PermissionDenied("You do not have permission to update encounter") + self._check_write_permission() def perform_update(self, instance): - if not AuthorizationController.call( - "can_update_encounter_obj", self.request.user, instance.encounter - ): - raise PermissionDenied("You do not have permission to update encounter") + self._check_write_permission() super().perform_update(instance)Also applies to: 186-191
care/emr/tests/test_chronic_condition_api.py (2)
60-78: Consider extracting test data creation into helper methods.The
create_chronic_conditionandgenerate_data_for_chronic_conditionmethods have quite a bit of duplicated code. Maybe we could make them a tiny bit more DRY? 😊+ def _get_default_condition_data(self, **kwargs): + return { + "clinical_status": kwargs.pop( + "clinical_status", choice(list(ClinicalStatusChoices)).value + ), + "verification_status": kwargs.pop( + "verification_status", choice(list(VerificationStatusChoices)).value + ), + "severity": kwargs.pop("severity", choice(list(SeverityChoices)).value), + } + def create_chronic_condition(self, encounter, patient, **kwargs): - clinical_status = kwargs.pop( - "clinical_status", choice(list(ClinicalStatusChoices)).value - ) - verification_status = kwargs.pop( - "verification_status", choice(list(VerificationStatusChoices)).value - ) - severity = kwargs.pop("severity", choice(list(SeverityChoices)).value) + condition_data = self._get_default_condition_data(**kwargs) return baker.make( Condition, encounter=encounter, patient=patient, category=CategoryChoices.chronic_condition.value, - clinical_status=clinical_status, - verification_status=verification_status, - severity=severity, + **condition_data, **kwargs, ) def generate_data_for_chronic_condition(self, encounter, **kwargs): - clinical_status = kwargs.pop( - "clinical_status", choice(list(ClinicalStatusChoices)).value - ) - verification_status = kwargs.pop( - "verification_status", choice(list(VerificationStatusChoices)).value - ) - severity = kwargs.pop("severity", choice(list(SeverityChoices)).value) + condition_data = self._get_default_condition_data(**kwargs) code = self.valid_code return { "encounter": encounter.external_id, "category": CategoryChoices.chronic_condition.value, - "clinical_status": clinical_status, - "verification_status": verification_status, - "severity": severity, + **condition_data, "code": code, **kwargs, }Also applies to: 80-97
527-590: Consider breaking down long test methods.The update test methods are quite lengthy. Perhaps we could split the setup and assertions into smaller, more focused test methods? Just a thought! 🤔
care/emr/migrations/0012_alter_condition_encounter.py (1)
10-10: Use double quotes consistently.The code uses single quotes for strings. While it works, the project's static analysis tools suggest using double quotes for consistency.
- dependencies = [ - ('emr', '0011_medicationrequest_requester'), - ] + dependencies = [ + ("emr", "0011_medicationrequest_requester"), + ] operations = [ migrations.AlterField( - model_name='condition', - name='encounter', - field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to='emr.encounter'), + model_name="condition", + name="encounter", + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to="emr.encounter"), ), ]Also applies to: 15-17
🧰 Tools
🪛 Ruff (0.8.2)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: 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 selected for processing (16)
care/abdm/migrations/0014_replace_0013.py(0 hunks)care/abdm/migrations_old/0013_abhanumber_patient.py(0 hunks)care/audit_log/middleware.py(1 hunks)care/emr/api/viewsets/allergy_intolerance.py(0 hunks)care/emr/api/viewsets/base.py(1 hunks)care/emr/api/viewsets/condition.py(4 hunks)care/emr/migrations/0012_alter_condition_encounter.py(1 hunks)care/emr/models/condition.py(1 hunks)care/emr/resources/condition/spec.py(4 hunks)care/emr/tests/test_booking_api.py(5 hunks)care/emr/tests/test_chronic_condition_api.py(1 hunks)care/emr/tests/test_schedule_api.py(4 hunks)care/utils/tests/test_feature_flags.py(1 hunks)config/api_router.py(2 hunks)config/settings/test.py(1 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (3)
- care/abdm/migrations/0014_replace_0013.py
- care/emr/api/viewsets/allergy_intolerance.py
- care/abdm/migrations_old/0013_abhanumber_patient.py
🚧 Files skipped from review as they are similar to previous changes (9)
- care/audit_log/middleware.py
- config/settings/test.py
- care/emr/models/condition.py
- care/utils/tests/test_feature_flags.py
- config/api_router.py
- care/emr/api/viewsets/base.py
- care/emr/resources/condition/spec.py
- care/emr/tests/test_schedule_api.py
- care/emr/tests/test_booking_api.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0012_alter_condition_encounter.py
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: 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)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/api/viewsets/condition.py (1)
69-70: LGTM: Model name updates are consistent.The updates to use
ConditionReadSpecandConditionUpdateSpecare consistent across both viewsets.Also applies to: 109-110
care/emr/migrations/0012_alter_condition_encounter.py (1)
17-17: Consider data integrity implications of nullable encounter.Making the encounter field nullable could lead to orphaned conditions. Are we sure we want to allow conditions without encounters? 🤔
✅ Verification successful
Nullable encounter field is actually correct per FHIR standards
While it might seem counterintuitive, making the encounter field nullable is the right approach here. In FHIR, conditions can exist independently of encounters (e.g., chronic conditions, pre-existing conditions). However, you might want to:
- Add a comment in the model explaining this FHIR compliance decision
- Consider adding validation logic for scenarios where encounters should be required
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any existing conditions without encounters ast-grep --pattern 'class Condition { $$$ encounter = models.ForeignKey($$$) $$$ }'Length of output: 91
Script:
#!/bin/bash # Find and show the Condition model definition rg "class Condition" -A 10 --type py # Look for any references to Condition and encounter together rg "Condition.*encounter|encounter.*Condition" --type py # Check for any tests or business logic related to conditions fd "test.*\.py" --exec rg "Condition" {} \;Length of output: 7072
🧰 Tools
🪛 Ruff (0.8.2)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
sainak
force-pushed
the
sainak/chronic-condition
branch
from
237fa80 to
843f0fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
care/emr/migrations/0012_alter_condition_encounter.py (1)
9-11: Inconsistent quote style detected.I see we're using single quotes. The static analyzer gently suggests using double quotes for consistency. Not that it matters much, but consistency is nice, isn't it?
- ('emr', '0011_medicationrequest_requester'), + ("emr", "0011_medicationrequest_requester"),🧰 Tools
🪛 Ruff (0.8.2)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/api/viewsets/condition.py (3)
176-177: Questionable authorization implementation.The
authorize_updatemethod just callsauthorize_createwith an empty dict. While it works, it feels a bit... creative? Maybe we should have separate logic for updates?- def authorize_update(self, request_obj, model_instance): - self.authorize_create({}) + def authorize_update(self, request_obj, model_instance): + if not AuthorizationController.call( + "can_write_patient_obj", self.request.user, self.get_patient_obj() + ): + raise PermissionDenied("You do not have permission to update chronic conditions for this patient")
183-184: Potential improvement in update data cleaning.The
keep_fieldsparameter is passed as a set with a single item. If we're only ever keeping 'encounter', maybe we could make this more explicit?- return super().clean_update_data(request_data, keep_fields={"encounter"}) + PRESERVED_FIELDS = {"encounter"} + return super().clean_update_data(request_data, keep_fields=PRESERVED_FIELDS)
193-206: Query optimization opportunity.The queryset method makes an authorization check and then performs a database query. We could potentially optimize this by:
- Caching the patient object
- Using
prefetch_relatedfor reverse relationships if neededdef get_queryset(self): + patient = self.get_patient_obj() # Cache the patient object if not AuthorizationController.call( - "can_view_clinical_data", self.request.user, self.get_patient_obj() + "can_view_clinical_data", self.request.user, patient ): raise PermissionDenied("Permission denied for patient data") return ( super() .get_queryset() .filter( - patient__external_id=self.kwargs["patient_external_id"], + patient=patient, # Use cached patient object category=CategoryChoices.chronic_condition.value, ) .select_related("patient", "encounter", "created_by", "updated_by") + .prefetch_related("condition_related_fields") # Add if needed )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
care/emr/api/viewsets/base.py(1 hunks)care/emr/api/viewsets/condition.py(4 hunks)care/emr/migrations/0012_alter_condition_encounter.py(1 hunks)care/emr/models/condition.py(1 hunks)care/emr/resources/condition/spec.py(4 hunks)care/emr/tests/test_chronic_condition_api.py(1 hunks)config/api_router.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- config/api_router.py
- care/emr/models/condition.py
- care/emr/resources/condition/spec.py
- care/emr/api/viewsets/base.py
- care/emr/tests/test_chronic_condition_api.py
🧰 Additional context used
🪛 Ruff (0.8.2)
care/emr/migrations/0012_alter_condition_encounter.py
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: 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)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
care/emr/migrations/0012_alter_condition_encounter.py (1)
14-18: Consider the implications of nullable encounters.While making the encounter field optional might seem convenient, it could lead to data inconsistencies. Perhaps we should discuss whether all conditions should be tied to an encounter for proper medical record keeping?
Also, the CASCADE deletion might be a bit... aggressive? Consider using SET_NULL instead to preserve condition history even after encounter deletion.
🧰 Tools
🪛 Ruff (0.8.2)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
care/emr/api/viewsets/condition.py (1)
69-70: Model updates look good, but...The updates to
pydantic_read_modelandpydantic_update_modelin both viewsets are fine, though it might have been nice to see some comments explaining why we're switching from the old spec classes. Just saying...Also applies to: 109-110
| def authorize_create(self, instance): | ||
| if not AuthorizationController.call( | ||
| "can_write_patient_obj", self.request.user, self.get_patient_obj() | ||
| ): | ||
| raise PermissionDenied("You do not have permission to update encounter") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate authorization logic detected.
The authorize_create method seems to reimplement what can_write_patient_obj already does. Also, the error message mentions "update encounter" in a create method. A bit confusing, don't you think?
- raise PermissionDenied("You do not have permission to update encounter")
+ raise PermissionDenied("You do not have permission to create chronic conditions for this patient")📝 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.
| def authorize_create(self, instance): | |
| if not AuthorizationController.call( | |
| "can_write_patient_obj", self.request.user, self.get_patient_obj() | |
| ): | |
| raise PermissionDenied("You do not have permission to update encounter") | |
| def authorize_create(self, instance): | |
| if not AuthorizationController.call( | |
| "can_write_patient_obj", self.request.user, self.get_patient_obj() | |
| ): | |
| raise PermissionDenied("You do not have permission to create chronic conditions for this patient") |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2775 +/- ##
===========================================
- Coverage 64.90% 56.66% -8.25%
===========================================
Files 252 208 -44
Lines 12761 9539 -3222
Branches 1121 939 -182
===========================================
- Hits 8283 5405 -2878
+ Misses 4369 4118 -251
+ Partials 109 16 -93 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
care/emr/api/viewsets/condition.py (1)
170-174:⚠️ Potential issueThat error message seems... interesting.
The error message mentions "update encounter" in a create method. We've been through this before, haven't we?
Apply this fix:
- raise PermissionDenied("You do not have permission to update encounter") + raise PermissionDenied("You do not have permission to create chronic conditions for this patient")
🧹 Nitpick comments (2)
care/emr/api/viewsets/condition.py (2)
142-164: Would it kill you to add some docstrings?The new viewset could use some documentation explaining its purpose and functionality. Consider adding class and method docstrings.
189-191: A docstring explaining thekeep_fieldsparameter would be lovely.The
clean_update_datamethod uses akeep_fieldsparameter, but its purpose isn't immediately clear.Add a docstring:
def clean_update_data(self, request_data): + """ + Clean update data while preserving specified fields. + + Args: + request_data: The data to be cleaned + + Returns: + Cleaned data with 'encounter' field preserved + """ return super().clean_update_data(request_data, keep_fields={"encounter"})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
care/emr/api/viewsets/condition.py(4 hunks)care/emr/resources/condition/spec.py(4 hunks)care/emr/tests/test_chronic_condition_api.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- care/emr/resources/condition/spec.py
- care/emr/tests/test_chronic_condition_api.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test / test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
care/emr/api/viewsets/condition.py (3)
3-4: LGTM! The imports look perfectly organized.The new imports are well-structured and all seem to be used appropriately in the code.
Also applies to: 19-19, 25-26, 28-28
69-70: Oh look, matching updates to both viewsets. How... consistent.The pydantic model updates in both viewsets align perfectly with the spec file changes.
Also applies to: 109-110
192-205: The queryset method looks almost perfect.Consider adding an index on
categoryfield if not already present, as it's used in filtering across all viewsets.Let's check if the index exists:
| def authorize_update(self, request_obj, model_instance): | ||
| encounter = get_object_or_404(Encounter, external_id=request_obj.encounter) | ||
| if not AuthorizationController.call( | ||
| "can_update_encounter_obj", | ||
| self.request.user, | ||
| encounter, | ||
| ): | ||
| raise PermissionDenied("You do not have permission to update encounter") | ||
|
|
There was a problem hiding this comment.
The authorization flow could use some love.
The authorize_update method fetches the encounter but doesn't verify if it belongs to the correct patient. This could lead to potential security issues.
Add patient verification:
def authorize_update(self, request_obj, model_instance):
encounter = get_object_or_404(Encounter, external_id=request_obj.encounter)
+ if str(encounter.patient.external_id) != self.kwargs["patient_external_id"]:
+ raise ValidationError("Patient external ID mismatch with encounter's patient")
if not AuthorizationController.call(
"can_update_encounter_obj",
self.request.user,
encounter,
):
raise PermissionDenied("You do not have permission to update encounter")📝 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.
| def authorize_update(self, request_obj, model_instance): | |
| encounter = get_object_or_404(Encounter, external_id=request_obj.encounter) | |
| if not AuthorizationController.call( | |
| "can_update_encounter_obj", | |
| self.request.user, | |
| encounter, | |
| ): | |
| raise PermissionDenied("You do not have permission to update encounter") | |
| def authorize_update(self, request_obj, model_instance): | |
| encounter = get_object_or_404(Encounter, external_id=request_obj.encounter) | |
| if str(encounter.patient.external_id) != self.kwargs["patient_external_id"]: | |
| raise ValidationError("Patient external ID mismatch with encounter's patient") | |
| if not AuthorizationController.call( | |
| "can_update_encounter_obj", | |
| self.request.user, | |
| encounter, | |
| ): | |
| raise PermissionDenied("You do not have permission to update encounter") |
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
New Features
Bug Fixes
Refactor
Tests