feat: add availability status to FacilityLocation #3469
Conversation
📝 WalkthroughWalkthroughIntroduces a new 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/location.py`:
- Around line 360-367: The lock in perform_destroy is acquired after calling
super().perform_destroy, allowing a race where a concurrent create/update can
change location.availability_status and be clobbered; move the
Lock(f"facility_location:{instance.location.id}") context to wrap the deletion
and the subsequent update/reset so that super().perform_destroy(instance),
setting instance.location.availability_status to available,
instance.location.save(update_fields=["availability_status"]), and
self.reset_encounter_location_association(instance.location) all execute while
holding the same lock (referencing perform_destroy, Lock, and
reset_encounter_location_association to locate the code).
In `@care/emr/models/location.py`:
- Around line 12-16: Update the FacilityLocation model to match migration 0057
by adding the missing default for availability_status: modify the
availability_status field on class FacilityLocation (in models.location) to
include default='available' so new instances get the same default as the
migration; ensure any model imports or validations remain unchanged and run
makemigrations/migrate if needed to keep model state in sync with migration
0057.
🧹 Nitpick comments (1)
care/emr/api/viewsets/location.py (1)
337-345: Unify availability mapping so create/update can’t drift.
Right nowcompleted → availableis only applied in update. If someone ever creates a completed encounter, you’ll storecompletedinstead ofavailable. A tiny helper keeps this consistent and future-proof.♻️ Suggested refactor
class FacilityLocationEncounterViewSet(EMRModelViewSet): + def _to_availability_status(self, status): + if status == LocationEncounterAvailabilityStatusChoices.completed.value: + return LocationEncounterAvailabilityStatusChoices.available.value + return status + def perform_create(self, instance): location = self.get_location_obj() with Lock(f"facility_location:{location.id}"): instance.location = location self._validate_data(instance) super().perform_create(instance) - location.availability_status = instance.status + location.availability_status = self._to_availability_status(instance.status) location.save(update_fields=["availability_status"]) self.reset_encounter_location_association(location) def perform_update(self, instance): location = self.get_location_obj() with Lock(f"facility_location:{location.id}"): # Keep in mind that instance here is an ORM instance and not pydantic self._validate_data(instance, self.get_object()) super().perform_update(instance) - status = instance.status - if status == LocationEncounterAvailabilityStatusChoices.completed.value: - status = LocationEncounterAvailabilityStatusChoices.available.value - location.availability_status = status + location.availability_status = self._to_availability_status(instance.status) location.save(update_fields=["availability_status"]) self.reset_encounter_location_association(location)Also applies to: 347-357
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
care/emr/api/viewsets/location.pycare/emr/management/commands/load_fixtures.pycare/emr/migrations/0057_facilitylocation_availability_status.pycare/emr/models/location.pycare/emr/resources/location/spec.pycare/emr/tests/test_location_api.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/api/viewsets/location.pycare/emr/models/location.pycare/emr/tests/test_location_api.pycare/emr/resources/location/spec.pycare/emr/management/commands/load_fixtures.pycare/emr/migrations/0057_facilitylocation_availability_status.py
**/{models,views,management/commands}/*.py
📄 CodeRabbit inference engine (.cursorrules)
Leverage Django’s ORM for database interactions; avoid raw SQL queries unless necessary for performance.
Files:
care/emr/models/location.pycare/emr/management/commands/load_fixtures.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursorrules)
Use Django’s built-in tools for testing (unittest and pytest-django) to ensure code quality and reliability.
Files:
care/emr/tests/test_location_api.py
🧠 Learnings (2)
📚 Learning: 2025-07-08T13:27:05.363Z
Learnt from: NikhilA8606
Repo: ohcnetwork/care PR: 3124
File: care/emr/resources/medication/administration/spec.py:163-164
Timestamp: 2025-07-08T13:27:05.363Z
Learning: In the care codebase, all EMR models inherit from EMRBaseModel which inherits from BaseModel (in care/utils/models/base.py). BaseModel provides common fields including created_date and modified_date with auto_now_add=True and auto_now=True respectively. These fields are available on all EMR models through inheritance.
Applied to files:
care/emr/models/location.py
📚 Learning: 2024-11-28T06:16:31.373Z
Learnt from: DraKen0009
Repo: ohcnetwork/care PR: 2620
File: care/facility/models/facility.py:306-311
Timestamp: 2024-11-28T06:16:31.373Z
Learning: In the 'care' project, moving imports of models like `Asset`, `AssetLocation`, `FacilityDefaultAssetLocation`, and `PatientSample` to the module level in `care/facility/models/facility.py` causes circular imports. Therefore, it's acceptable to keep these imports inside the `delete` method of the `Facility` class.
Applied to files:
care/emr/tests/test_location_api.py
🧬 Code graph analysis (3)
care/emr/api/viewsets/location.py (3)
care/emr/resources/location/spec.py (1)
LocationEncounterAvailabilityStatusChoices(13-18)care/emr/api/viewsets/base.py (1)
perform_destroy(250-252)care/utils/lock.py (1)
Lock(12-30)
care/emr/tests/test_location_api.py (1)
care/emr/models/location.py (1)
FacilityLocation(12-128)
care/emr/migrations/0057_facilitylocation_availability_status.py (2)
care/emr/models/location.py (1)
FacilityLocation(12-128)care/emr/api/viewsets/location.py (1)
filter(40-45)
🪛 Ruff (0.14.11)
care/emr/migrations/0057_facilitylocation_availability_status.py
6-6: Unused function argument: schema_editor
(ARG001)
13-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
17-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
🔇 Additional comments (12)
care/emr/management/commands/load_fixtures.py (1)
623-632: Operational status update looks fine.
Assuming"U"is the intended default operational status, this change is consistent and harmless.care/emr/resources/location/spec.py (2)
13-18: Enum extension is clean and consistent.
Addingavailablefits the lifecycle states and keeps the enum cohesive.
117-122: Typed availability_status is a good tighten-up.
The list spec now reflects the real domain values instead of a plain string.care/emr/migrations/0057_facilitylocation_availability_status.py (1)
6-25: Migration logic is straightforward and sane.
Backfillingactivefor rows withcurrent_encounteris the right move.care/emr/api/viewsets/location.py (1)
480-486: Close flow update is solid.
Settingavailability_statustoavailablealongside clearingcurrent_encounteris exactly what you want here.care/emr/tests/test_location_api.py (7)
773-777: Good, we actually verify the location flips to active now.
This assertion pins the new availability_status behavior after create.
794-799: Nice to see the state asserted before the conflict path.
Covers both current_encounter and availability_status.
829-833: Yep, this makes the overlap test actually prove the location is active.
869-873: Good—precondition is now explicit instead of implied.
1046-1050: Solid post-delete verification for availability_status.
1108-1112: Good coverage of the completed → available transition.
1189-1194: Finally checking both current_encounter and availability_status—nice.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def perform_destroy(self, instance): | ||
| super().perform_destroy(instance) | ||
| self.reset_encounter_location_association(instance.location) | ||
| with Lock(f"facility_location:{instance.location.id}"): | ||
| instance.location.availability_status = ( | ||
| LocationEncounterAvailabilityStatusChoices.available.value | ||
| ) | ||
| instance.location.save(update_fields=["availability_status"]) | ||
| self.reset_encounter_location_association(instance.location) |
There was a problem hiding this comment.
Lock acquisition happens too late in destroy.
Because the lock is taken after deletion, a concurrent create/update can set availability_status and then get overwritten by this destroy path. Wrap the delete + update in the same lock.
🔒 Proposed fix
def perform_destroy(self, instance):
- super().perform_destroy(instance)
- with Lock(f"facility_location:{instance.location.id}"):
+ with Lock(f"facility_location:{instance.location.id}"):
+ super().perform_destroy(instance)
instance.location.availability_status = (
LocationEncounterAvailabilityStatusChoices.available.value
)
instance.location.save(update_fields=["availability_status"])
self.reset_encounter_location_association(instance.location)📝 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 perform_destroy(self, instance): | |
| super().perform_destroy(instance) | |
| self.reset_encounter_location_association(instance.location) | |
| with Lock(f"facility_location:{instance.location.id}"): | |
| instance.location.availability_status = ( | |
| LocationEncounterAvailabilityStatusChoices.available.value | |
| ) | |
| instance.location.save(update_fields=["availability_status"]) | |
| self.reset_encounter_location_association(instance.location) | |
| def perform_destroy(self, instance): | |
| with Lock(f"facility_location:{instance.location.id}"): | |
| super().perform_destroy(instance) | |
| instance.location.availability_status = ( | |
| LocationEncounterAvailabilityStatusChoices.available.value | |
| ) | |
| instance.location.save(update_fields=["availability_status"]) | |
| self.reset_encounter_location_association(instance.location) |
🤖 Prompt for AI Agents
In `@care/emr/api/viewsets/location.py` around lines 360 - 367, The lock in
perform_destroy is acquired after calling super().perform_destroy, allowing a
race where a concurrent create/update can change location.availability_status
and be clobbered; move the Lock(f"facility_location:{instance.location.id}")
context to wrap the deletion and the subsequent update/reset so that
super().perform_destroy(instance), setting instance.location.availability_status
to available, instance.location.save(update_fields=["availability_status"]), and
self.reset_encounter_location_association(instance.location) all execute while
holding the same lock (referencing perform_destroy, Lock, and
reset_encounter_location_association to locate the code).
| class FacilityLocation(EMRBaseModel): | ||
| status = models.CharField(max_length=255) | ||
| operational_status = models.CharField(max_length=255) | ||
| availability_status = models.CharField(max_length=255) | ||
| name = models.CharField(max_length=255) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the location.py file
fd -t f "location.py" --path "*emr/models*"Repository: ohcnetwork/care
Length of output: 292
🏁 Script executed:
# Check the actual model definition
cat -n care/emr/models/location.py | head -30Repository: ohcnetwork/care
Length of output: 1560
🏁 Script executed:
# Find migration files related to this change
fd -t f "*.py" --path "*migrations*" | grep -E "(location|facility)" | head -20Repository: ohcnetwork/care
Length of output: 292
🏁 Script executed:
# Find migration files - corrected syntax
fd -t f "\.py$" care/emr/migrations | head -30Repository: ohcnetwork/care
Length of output: 1825
🏁 Script executed:
# Search for migrations related to FacilityLocation
rg "availability_status" care/emr/migrations/ -A 3 -B 3Repository: ohcnetwork/care
Length of output: 5469
🏁 Script executed:
# Check if there are any recent changes to this file
git log --oneline care/emr/models/location.py | head -10Repository: ohcnetwork/care
Length of output: 117
🏁 Script executed:
# Look at the full model to understand the context better
cat -n care/emr/models/location.py | tail -20Repository: ohcnetwork/care
Length of output: 889
Add default value to match migration 0057.
The availability_status field definition is missing the default='available' that migration 0057 specifies. Without it, object creation will fail unless the field is explicitly set—fun times debugging that later.
🩹 Proposed fix
- availability_status = models.CharField(max_length=255)
+ availability_status = models.CharField(max_length=255, default="available")📝 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.
| class FacilityLocation(EMRBaseModel): | |
| status = models.CharField(max_length=255) | |
| operational_status = models.CharField(max_length=255) | |
| availability_status = models.CharField(max_length=255) | |
| name = models.CharField(max_length=255) | |
| class FacilityLocation(EMRBaseModel): | |
| status = models.CharField(max_length=255) | |
| operational_status = models.CharField(max_length=255) | |
| availability_status = models.CharField(max_length=255, default="available") | |
| name = models.CharField(max_length=255) |
🤖 Prompt for AI Agents
In `@care/emr/models/location.py` around lines 12 - 16, Update the
FacilityLocation model to match migration 0057 by adding the missing default for
availability_status: modify the availability_status field on class
FacilityLocation (in models.location) to include default='available' so new
instances get the same default as the migration; ensure any model imports or
validations remain unchanged and run makemigrations/migrate if needed to keep
model state in sync with migration 0057.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3469 +/- ##
===========================================
+ Coverage 74.82% 74.84% +0.01%
===========================================
Files 465 465
Lines 21115 21127 +12
Branches 2183 2184 +1
===========================================
+ Hits 15800 15812 +12
Misses 4835 4835
Partials 480 480 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| class LocationEncounterAvailabilityStatusChoices(str, Enum): | ||
| available = "available" | ||
| planned = "planned" | ||
| active = "active" | ||
| reserved = "reserved" | ||
| active = "active" | ||
| completed = "completed" |
There was a problem hiding this comment.
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
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.