[ENG-580] feat: allow clearing facility org/org#3690
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTagConfigUpdateSpec Deserialization Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds
Confidence Score: 3/5Not safe to merge: the update handler will silently clear existing organization associations whenever a client omits those fields from the request body. The care/emr/resources/tag/config_spec.py — specifically Important Files Changed
Reviews (4): Last reviewed commit: "chore: allow none default" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/tag/config_spec.py`:
- Around line 71-72: The truthy checks on the organization and
facility_organization fields prevent explicit None values from clearing the
database fields. Replace the truthy checks (such as `if organization:` and `if
facility_organization:`) with explicit `is not None` checks throughout the
update logic (lines 75-84) so that passing `organization=None` or
`facility_organization=None` will properly clear the existing DB values instead
of leaving them unchanged.
🪄 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
Run ID: 6b57f861-64da-4dfb-bda4-b5492166a35e
📒 Files selected for processing (1)
care/emr/resources/tag/config_spec.py
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/emr/resources/tag/config_spec.py (1)
75-88: 💤 Low valueTruthy checks work here, but explicit
is not Nonewould be clearerThe else branches successfully address the clearing functionality—which is great, that's the whole point of ENG-580. However, the truthy checks (
if self.organization:) differ from the past review's suggestion to use explicitis not Nonechecks.For
UUID4 | None, this is functionally equivalent since Pydantic validates the type. But usingis not Nonewould make the intent more obvious to future readers who might wonder if there's some edge case being handled.Optional: Use explicit None checks for clarity
def perform_extra_deserialization(self, is_update, obj): - if self.organization: + if self.organization is not None: obj.organization = get_object_or_404( Organization.objects.only("id"), external_id=self.organization ) else: obj.organization = None - if self.facility_organization: + if self.facility_organization is not None: obj.facility_organization = get_object_or_404( FacilityOrganization.objects.only("id"), external_id=self.facility_organization, facility=obj.facility, ) else: obj.facility_organization = None🤖 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/resources/tag/config_spec.py` around lines 75 - 88, Replace the truthy checks for self.organization and self.facility_organization with explicit is not None checks to make the intent clearer. Change the if conditions from checking truthiness (if self.organization:) to explicitly checking against None (if self.organization is not None:) in both the organization assignment block and the facility_organization assignment block to improve code clarity for future readers.
🤖 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/resources/tag/config_spec.py`:
- Around line 75-88: Replace the truthy checks for self.organization and
self.facility_organization with explicit is not None checks to make the intent
clearer. Change the if conditions from checking truthiness (if
self.organization:) to explicitly checking against None (if self.organization is
not None:) in both the organization assignment block and the
facility_organization assignment block to improve code clarity for future
readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c70e6b6-2b47-474f-a893-2f486a2145aa
📒 Files selected for processing (1)
care/emr/resources/tag/config_spec.py
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
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/tag/config_spec.py`:
- Around line 71-72: The facility_organization and organization fields both
default to None, making omitted fields indistinguishable from explicitly nulled
fields. This causes the else branches (lines 79 and 87) to unconditionally clear
relations even when those fields were not provided in the PATCH request. To fix
this, use Pydantic's model_fields_set attribute to check whether each field was
actually included in the request before executing the else branch logic that
clears the relation. Only clear the relation if the field was explicitly present
in the request data, not when it was omitted.
🪄 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
Run ID: 3967a86e-543a-4a8b-9393-8a89cff29f7f
📒 Files selected for processing (1)
care/emr/resources/tag/config_spec.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/tag/config_spec.py`:
- Around line 71-72: The facility_organization and organization fields both
default to None, making omitted fields indistinguishable from explicitly nulled
fields. This causes the else branches (lines 79 and 87) to unconditionally clear
relations even when those fields were not provided in the PATCH request. To fix
this, use Pydantic's model_fields_set attribute to check whether each field was
actually included in the request before executing the else branch logic that
clears the relation. Only clear the relation if the field was explicitly present
in the request data, not when it was omitted.
🪄 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
Run ID: 3967a86e-543a-4a8b-9393-8a89cff29f7f
📒 Files selected for processing (1)
care/emr/resources/tag/config_spec.py
🛑 Comments failed to post (1)
care/emr/resources/tag/config_spec.py (1)
71-72: 🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Verify update semantics (PATCH/partial vs full update) ==" rg -n --type=py -C3 'class\s+EMRUpdateMixin|def\s+update\(|def\s+partial_update\(' care/emr echo echo "== Confirm TagConfig update model wiring ==" rg -n --type=py -C2 'pydantic_update_model\s*=\s*TagConfigUpdateSpec' care/emr echo echo "== Check whether tests cover omitted-vs-null behavior for tag config updates ==" rg -n --type=py -C2 'TagConfigUpdateSpec|facility_organization|organization' care/emr -g '*test*' echo echo "== Detect Pydantic major version from manifests/locks ==" fd -HI 'pyproject.toml' . -x rg -n 'pydantic|requires-python' {} fd -HI 'requirements*.txt' . -x rg -n 'pydantic' {} fd -HI 'poetry.lock|uv.lock|Pipfile.lock' . -x rg -n 'pydantic' {}Repository: ohcnetwork/care
Length of output: 50372
🏁 Script executed:
# Read the actual TagConfigUpdateSpec implementation ast-grep outline care/emr/resources/tag/config_spec.pyRepository: ohcnetwork/care
Length of output: 1566
🏁 Script executed:
# Extract Pydantic version more efficiently grep -i "pydantic" pyproject.toml 2>/dev/null || echo "No pyproject.toml" grep -i "pydantic" requirements*.txt 2>/dev/null | head -5 || echo "No requirements files" find . -name "poetry.lock" -o -name "uv.lock" -o -name "Pipfile.lock" | head -1 | xargs grep -A5 "name = \"pydantic\"" 2>/dev/null | head -10Repository: ohcnetwork/care
Length of output: 171
🏁 Script executed:
# Check if TagConfigUpdateSpec update endpoint uses PATCH or PUT semantics rg -n "class.*TagConfigViewSet|def (update|partial_update)" care/emr -A 5 --type=pyRepository: ohcnetwork/care
Length of output: 7672
🏁 Script executed:
# Read the actual implementation of TagConfigUpdateSpec including perform_extra_deserialization sed -n '70,90p' care/emr/resources/tag/config_spec.pyRepository: ohcnetwork/care
Length of output: 894
🏁 Script executed:
# Find Pydantic version in requirements files find . -name "*.txt" -o -name "*.lock" | xargs grep -l "pydantic" 2>/dev/null | head -3Repository: ohcnetwork/care
Length of output: 168
🏁 Script executed:
# Extract Pydantic version from Pipfile.lock rg -A 10 '"pydantic"' Pipfile.lock | head -20Repository: ohcnetwork/care
Length of output: 570
🏁 Script executed:
# Verify whether update endpoints support partial_update (PATCH semantics) rg -B 5 -A 15 'class TagConfigViewSet' care/emr/api/viewsets/tag_config.pyRepository: ohcnetwork/care
Length of output: 1019
🏁 Script executed:
# Check if there are tests for tag config updates with omitted vs null fields rg -n 'TagConfig.*update|facility_organization.*update|organization.*update' care/emr/tests -g '*tag*' -B 2 -A 5Repository: ohcnetwork/care
Length of output: 153
Update omission and explicit null are indistinguishable, risking silent data loss.
With both fields defaulting to
Noneon lines 71–72, a PATCH request that omitsorganizationandfacility_organizationis treated identically to one that explicitly nulls them. Theelsebranches on lines 79 and 87 then unconditionally clear those relations, wiping out existing data in unrelated updates.Pydantic 2.12.5 has
model_fields_setavailable—use it to gate assignment by actual field presence:Suggested fix
class TagConfigUpdateSpec(TagConfigBaseSpec): facility_organization: UUID4 | None = None organization: UUID4 | None = None def perform_extra_deserialization(self, is_update, obj): - if self.organization: - obj.organization = get_object_or_404( - Organization.objects.only("id"), external_id=self.organization - ) - else: - obj.organization = None - if self.facility_organization: - obj.facility_organization = get_object_or_404( - FacilityOrganization.objects.only("id"), - external_id=self.facility_organization, - facility=obj.facility, - ) - else: - obj.facility_organization = None + if "organization" in self.model_fields_set: + if self.organization is None: + obj.organization = None + else: + obj.organization = get_object_or_404( + Organization.objects.only("id"), external_id=self.organization + ) + + if "facility_organization" in self.model_fields_set: + if self.facility_organization is None: + obj.facility_organization = None + else: + obj.facility_organization = get_object_or_404( + FacilityOrganization.objects.only("id"), + external_id=self.facility_organization, + facility=obj.facility, + )🤖 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/resources/tag/config_spec.py` around lines 71 - 72, The facility_organization and organization fields both default to None, making omitted fields indistinguishable from explicitly nulled fields. This causes the else branches (lines 79 and 87) to unconditionally clear relations even when those fields were not provided in the PATCH request. To fix this, use Pydantic's model_fields_set attribute to check whether each field was actually included in the request before executing the else branch logic that clears the relation. Only clear the relation if the field was explicitly present in the request data, not when it was omitted.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3690 +/- ##
========================================
Coverage 79.55% 79.56%
========================================
Files 479 479
Lines 22996 22998 +2
Branches 2378 2378
========================================
+ Hits 18295 18298 +3
+ Misses 4096 4095 -1
Partials 605 605 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
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
Release Notes
null), instead of leaving previously stored values in place.