added a separately_billable to charge item def#3476
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3476 +/- ##
===========================================
+ Coverage 74.48% 74.59% +0.10%
===========================================
Files 471 476 +5
Lines 21667 21796 +129
Branches 2257 2280 +23
===========================================
+ Hits 16139 16259 +120
- Misses 5027 5031 +4
- Partials 501 506 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 4
🤖 Fix all issues with AI agents
In `@care/emr/signals/charge_item_def/activity_definition.py`:
- Around line 29-49: The soft-delete branch in
update_separately_billable_activity currently returns early and skips
recalculating removed IDs; compute removed_ids = old_ids - new_ids (using
_OLD_CHARGE_ITEM_DEFS_IDS and instance.charge_item_definitions) before the
is_soft_deleted check and, when is_soft_deleted is true, call
recalculate_separately_billable for both each id in new_ids and each id in
removed_ids so removed charge_item_definitions are handled even on soft-delete;
keep existing calls to set_separately_billable_false for added_ids when not
deleted and retain original logic otherwise.
In `@care/emr/signals/charge_item_def/product.py`:
- Around line 27-43: The soft-delete branch in
update_separately_billable_product currently returns after only recalculating
the new_charge_item_def_id, skipping any recalculation for an
old_charge_item_def_id changed in the same save; update the handler so that when
is_soft_deleted is True and new_charge_item_def_id exists you also recalculates
the old_charge_item_def_id (if old_charge_item_def_id and old_charge_item_def_id
!= new_charge_item_def_id) before returning. Locate the receiver function
update_separately_billable_product and adjust the soft-delete branch to call
recalculate_separately_billable(old_charge_item_def_id) as appropriate (using
the existing _OLD_CHARGE_ITEM_DEF_ID, new_charge_item_def_id and old_deleted
checks) so both old and new IDs are handled.
In `@care/emr/signals/charge_item_def/schedule.py`:
- Around line 34-61: The soft-delete branch in
update_separately_billable_schedule currently only triggers recalculation for
new_charge_item_def_id/new_revisit_id and thus misses recalculating old IDs when
IDs changed during deletion; modify that branch so that before returning it also
calls recalculate_separately_billable for old_charge_item_def_id and
old_revisit_id when they exist and differ from the new IDs (i.e. mirror the
non-deleted change logic: if old_charge_item_def_id and old_charge_item_def_id
!= instance.charge_item_definition_id then
recalculate_separately_billable(old_charge_item_def_id), and similarly for
old_revisit_id), then keep the existing calls for new IDs and return.
In `@care/emr/utils/charge_item_definition.py`:
- Around line 7-21: The linkage checks in is_linked_to_any_resource currently
count soft-deleted rows; update each queryset (Product.objects.filter(...),
Schedule.objects.filter(...), the revisit_charge_item_definition_id query, and
ActivityDefinition.objects.filter(...charge_item_definitions__contains=...)) to
only consider non-deleted records by adding the appropriate soft-delete filter
for our models (e.g. add is_deleted=False or deleted_at__isnull=True depending
on our soft-delete field) so soft-deleted Product, Schedule, and
ActivityDefinition rows are excluded from the existence checks.
🧹 Nitpick comments (3)
care/emr/migrations/0058_sync_separately_billable_values.py (2)
10-20: Consider performance optimization for large datasets.The current implementation has an N+1 query pattern – for each
ChargeItemDefinition, four separate database queries are executed to check links. This could be... let's say, suboptimal on larger datasets.Consider using prefetch/annotation or collecting linked IDs upfront:
♻️ Proposed optimization
def sync_separately_billable(apps, schema_editor): ChargeItemDefinition = apps.get_model("emr", "ChargeItemDefinition") Product = apps.get_model("emr", "Product") Schedule = apps.get_model("emr", "Schedule") ActivityDefinition = apps.get_model("emr", "ActivityDefinition") - for charge_item_def in ChargeItemDefinition.objects.all(): - is_linked = ( - Product.objects.filter(charge_item_definition_id=charge_item_def.id).exists() - or Schedule.objects.filter(charge_item_definition_id=charge_item_def.id).exists() - or Schedule.objects.filter(revisit_charge_item_definition_id=charge_item_def.id).exists() - or ActivityDefinition.objects.filter(charge_item_definitions__contains=[charge_item_def.id]).exists() - ) - - if is_linked and charge_item_def.separately_billable: - charge_item_def.separately_billable = False - charge_item_def.save(update_fields=["separately_billable"]) + # Collect all linked ChargeItemDefinition IDs upfront + linked_ids = set() + linked_ids.update( + Product.objects.exclude(charge_item_definition__isnull=True) + .values_list("charge_item_definition_id", flat=True) + ) + linked_ids.update( + Schedule.objects.exclude(charge_item_definition__isnull=True) + .values_list("charge_item_definition_id", flat=True) + ) + linked_ids.update( + Schedule.objects.exclude(revisit_charge_item_definition__isnull=True) + .values_list("revisit_charge_item_definition_id", flat=True) + ) + for activity_def in ActivityDefinition.objects.exclude(charge_item_definitions=[]): + linked_ids.update(activity_def.charge_item_definitions or []) + + # Bulk update + ChargeItemDefinition.objects.filter( + id__in=linked_ids, separately_billable=True + ).update(separately_billable=False)
29-31: Missing reverse migration function.The
RunPythonoperation lacks a reverse function, which meansmigrate --fakeor rollbacks won't be fully reversible. It would be quite nice to have amigrations.RunPython.noopat minimum, or a proper reverse function if the original state needs restoration.♻️ Proposed fix
operations = [ - migrations.RunPython(sync_separately_billable), + migrations.RunPython(sync_separately_billable, migrations.RunPython.noop), ]care/emr/tests/test_charge_item_def_signals.py (1)
129-170: Consider adding test coverage forrevisit_charge_item_definitionunlink.The Schedule tests cover the
revisit_charge_item_definitionlink scenario but, interestingly, don't test unlinking or changing therevisit_charge_item_definitionfield. For consistency with thecharge_item_definitiontests, you might want to add those cases.💡 Suggested additional tests
def test_schedule_revisit_unlink_recalculates_separately_billable(self): charge_item_def = self.create_charge_item_definition() schedule = self.create_schedule(revisit_charge_item_definition=charge_item_def) charge_item_def.refresh_from_db() self.assertFalse(charge_item_def.separately_billable) schedule.revisit_charge_item_definition = None schedule.save() charge_item_def.refresh_from_db() self.assertTrue(charge_item_def.separately_billable)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@care/emr/migrations/0062_sync_seprately_billable_values.py`:
- Line 1: The migration filename has a typo ("seprately"); rename the file to
0062_sync_separately_billable_values.py and update any references to that
migration if present (e.g., explicit imports or dependency entries in other
migration files) so Django continues to detect the migration; verify git history
move/rename is a proper rename (git mv) to preserve blame and that the migration
module still contains the original Migration class and operations.
🧹 Nitpick comments (2)
care/emr/migrations/0062_sync_seprately_billable_values.py (2)
10-20: Consider optimizing for larger datasets.The current approach executes up to 4 queries per
ChargeItemDefinitionrecord (N+1 pattern). For a one-time migration on a small dataset, this is probably fine. For larger datasets, you might want to consider a more efficient approach—or not, if you enjoy watching progress bars.♻️ Suggested optimization using bulk update
def sync_separately_billable(apps, schema_editor): ChargeItemDefinition = apps.get_model("emr", "ChargeItemDefinition") Product = apps.get_model("emr", "Product") Schedule = apps.get_model("emr", "Schedule") ActivityDefinition = apps.get_model("emr", "ActivityDefinition") - for charge_item_def in ChargeItemDefinition.objects.all(): - is_linked = ( - Product.objects.filter(charge_item_definition_id=charge_item_def.id).exists() - or Schedule.objects.filter(charge_item_definition_id=charge_item_def.id).exists() - or Schedule.objects.filter(revisit_charge_item_definition_id=charge_item_def.id).exists() - or ActivityDefinition.objects.filter(charge_item_definitions__contains=[charge_item_def.id]).exists() - ) - - if is_linked and charge_item_def.separately_billable: - charge_item_def.separately_billable = False - charge_item_def.save(update_fields=["separately_billable"]) + # Collect all linked ChargeItemDefinition IDs + linked_ids = set() + + linked_ids.update( + Product.objects.exclude(charge_item_definition_id__isnull=True) + .values_list("charge_item_definition_id", flat=True) + ) + linked_ids.update( + Schedule.objects.exclude(charge_item_definition_id__isnull=True) + .values_list("charge_item_definition_id", flat=True) + ) + linked_ids.update( + Schedule.objects.exclude(revisit_charge_item_definition_id__isnull=True) + .values_list("revisit_charge_item_definition_id", flat=True) + ) + + # For ActivityDefinition with ArrayField, iterate but only once + for activity_def in ActivityDefinition.objects.exclude(charge_item_definitions=[]): + linked_ids.update(activity_def.charge_item_definitions) + + # Bulk update all linked items + ChargeItemDefinition.objects.filter( + id__in=linked_ids, separately_billable=True + ).update(separately_billable=False)
29-31: Consider adding a reverse migration for rollback support.The
RunPythonoperation lacks areverse_codefunction. If you ever need to roll back this migration, Django will refuse unless you provide one (or usemigrations.RunPython.noop). I'm sure that won't be a problem though.♻️ Add noop reverse if rollback isn't meaningful
operations = [ - migrations.RunPython(sync_separately_billable), + migrations.RunPython(sync_separately_billable, migrations.RunPython.noop), ]
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.
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.