[Eng-834]:testcases for invoice - #3726
Open
nandkishorr wants to merge 9 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughChangesInvoice API coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds/expands Django REST Framework API test coverage for the EMR invoice endpoints (create/update/list/retrieve/cancel/lock + permission and validation scenarios) to support ENG-834.
Changes:
- Refactors invoice API tests into a shared helper-based test class and adds broad endpoint coverage.
- Adds tests for invoice numbering defaults, create-lock failure handling, and multiple status-transition validations.
- Adds tests for payment-reconciliation-related invoice filtering and locked-invoice permission behavior.
Suppressed comments (5)
care/emr/tests/test_invoice_api.py:553
- This
PaymentReconciliation.objects.create(...)omits required non-null fields (reconciliation_type,kind,issuer_type,outcome,method) and uses an invalidstatusvalue ("completed"). This will fail when saving the model.
PaymentReconciliation.objects.create(
facility=self.facility,
account=self.account,
status="completed",
amount=invoice.total_gross,
tendered_amount=invoice.total_gross,
returned_amount=Decimal("0.00"),
target_invoice=invoice,
)
care/emr/tests/test_invoice_api.py:651
- This
PaymentReconciliation.objects.create(...)omits required non-null fields (reconciliation_type,kind,issuer_type,outcome,method) and uses an invalidstatusvalue ("completed"). This will fail when saving the model.
PaymentReconciliation.objects.create(
facility=self.facility,
account=self.account,
status="completed",
amount=invoice.total_gross,
tendered_amount=invoice.total_gross,
returned_amount=Decimal("0.00"),
target_invoice=invoice,
)
care/emr/tests/test_invoice_api.py:678
- This
PaymentReconciliation.objects.create(...)omits required non-null fields (reconciliation_type,kind,issuer_type,outcome,method) and uses an invalidstatusvalue ("completed"). This will fail when saving the model.
PaymentReconciliation.objects.create(
facility=self.facility,
account=self.account,
status="completed",
amount=invoice.total_gross,
tendered_amount=invoice.total_gross,
returned_amount=Decimal("0.00"),
target_invoice=invoice,
)
care/emr/tests/test_invoice_api.py:719
- Typo in test name:
retrive→retrieve(helps readability and consistency when searching for tests).
def test_retrive_locked_invoice_with_user_without_permission(self):
care/emr/tests/test_invoice_api.py:738
- Typo in test name:
retrive→retrieve(helps readability and consistency when searching for tests).
def test_retrive_locked_invoice_with_user_with_permission(self):
|
|
||
|
|
||
| class TestAttachAccountToInvoice(CareAPITestBase): | ||
| class InvoiceAPITestBase(CareAPITestBase): |
Comment on lines
34
to
+37
| def setUp(self): | ||
| super().setUp() | ||
| NameIdentifierConfig.CACHED_CONFIG = {} | ||
| PhoneNumberIdentifierConfig.CACHED_CONFIG = {} | ||
| FacilityPatientNameIdentifierConfig.CACHED_CONFIG = {} |
Comment on lines
+517
to
+525
| PaymentReconciliation.objects.create( | ||
| facility=self.facility, | ||
| account=self.account, | ||
| patient=self.patient, | ||
| status=InvoiceStatusOptions.issued.value, | ||
| status="completed", | ||
| amount=invoice.total_gross, | ||
| tendered_amount=invoice.total_gross, | ||
| returned_amount=Decimal("0.00"), | ||
| target_invoice=invoice, | ||
| ) |
| response_data = response.data | ||
| self.assertEqual(response_data["id"], str(invoice.external_id)) | ||
|
|
||
| def test_retrive_locked_invoice_with_superuser(self): |
![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: 7
🧹 Nitpick comments (6)
care/emr/tests/test_invoice_api.py (6)
270-281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a local
rolevariable instead of reassigningself.role.This test overwrites the
self.rolecreated insetUp. Other tests in this file (for example lines 1156 and 1219) use a localrolefor the same purpose. Both styles work becausesetUpruns per test, but the mix makes the fixture contract harder to follow. Pick the local variable form throughout.♻️ Proposed change
- self.client.force_authenticate(user=self.user) permissions = [ InvoicePermissions.can_read_invoice.name, ] - self.role = self.create_role_with_permissions(permissions) - self.attach_role_facility_organization_user( - self.organization, self.user, self.role - ) + role = self.create_role_with_permissions(permissions) + self.attach_role_facility_organization_user(self.organization, self.user, role) self.client.force_authenticate(user=self.user)🤖 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/tests/test_invoice_api.py` around lines 270 - 281, Update test_update_invoice_with_user_without_write_permission to store the result of create_role_with_permissions in a local role variable instead of overwriting self.role, and pass that local role to attach_role_facility_organization_user.
912-912: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the section comment.
The comment reads "# def testcases for attach and detach charge items to invoice". The
defis left over. The endpoints and helpers use "remove", not "detach", so align the wording.♻️ Proposed fix
- # def testcases for attach and detach charge items to invoice + # testcases for attach and remove charge items to invoice🤖 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/tests/test_invoice_api.py` at line 912, Update the section comment above the invoice charge-item tests to remove the stray “def” wording and replace “detach” with “remove,” matching the endpoint and helper terminology.
1118-1128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
lock_historyas well.The
lockaction appends an entry withuser,timestamp, andactiontoinvoice.lock_history. No test checks it. That is an audit trail on a financial record, so a silent regression there would be unfortunate. Refresh the invoice and assert one entry withaction == "lock".💚 Proposed addition
self.assertEqual(response.status_code, 200) response_data = response.data self.assertTrue(response_data["locked"]) + invoice.refresh_from_db() + self.assertEqual(len(invoice.lock_history), 1) + self.assertEqual(invoice.lock_history[0]["action"], "lock")🤖 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/tests/test_invoice_api.py` around lines 1118 - 1128, Extend test_lock_invoice_with_superuser to refresh the invoice after the lock request, then assert invoice.lock_history contains exactly one entry whose action is "lock". Also validate the recorded user and timestamp fields are present, preserving the existing response assertions.
89-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deterministic invoice numbers.
random.randint(1000, 9999)can produce the same number twice inside one test. IfInvoice.numbercarries a unique constraint per facility, that creates a rare flaky failure. A counter orself.fake.uniqueremoves the randomness. Not urgent, just one of those things that fails at 2 AM.🤖 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/tests/test_invoice_api.py` around lines 89 - 104, Update the create_invoice helper to generate deterministic, unique invoice numbers within a test, replacing random.randint with an existing counter or self.fake.unique mechanism while preserving the default INV- prefix and allowing an explicitly supplied number via kwargs.
77-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
generate_invoice_data.
kwargs.get("status", ...)andkwargs.get("charge_items", ...)are redundant. Line 86 already appliesdata.update(**kwargs), which overrides both keys. Keep the defaults only.♻️ Proposed simplification
def generate_invoice_data(self, **kwargs): data = { "account": self.account.external_id, - "status": kwargs.get("status", InvoiceStatusOptions.draft.value), - "charge_items": kwargs.get("charge_items", [self.charge_item.external_id]), + "status": InvoiceStatusOptions.draft.value, + "charge_items": [self.charge_item.external_id], "title": "Test Invoice", "number": f"INV-{random.randint(1000, 9999)}", # noqa: S311 "issue_date": datetime.now(UTC).isoformat(), } data.update(**kwargs) return data🤖 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/tests/test_invoice_api.py` around lines 77 - 87, Update generate_invoice_data so the initial data dictionary assigns the default InvoiceStatusOptions.draft value and default charge-item list directly, relying on the existing data.update(**kwargs) call to override them when provided.
932-948: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a cross-account attach test.
The suite covers permissions and non-draft status for attach, but no test attaches a charge item that belongs to a different account or facility. That is a tenant-isolation boundary, and it is the kind of thing that only gets noticed after it ships. A single negative test would cover it.
🤖 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/tests/test_invoice_api.py` around lines 932 - 948, Add a negative cross-account or cross-facility test alongside test_attach_charge_items_to_invoice_with_superuser, creating the invoice and charge item under different tenants and posting the charge item to the attach endpoint. Assert the request is rejected and the invoice’s charge items remain unchanged, using the existing test helpers and URL construction.
🤖 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/tests/test_invoice_api.py`:
- Around line 1255-1266: Update test_attach_account_to_invoice_as_superuser and
test_attach_account_to_invoice_as_user_with_permissions to create an additional
billable charge item on the account after create_invoice, ensuring it is not
initially attached to the invoice. Assert that this new item appears in
response.data["charge_items"] after the request, rather than asserting the
pre-attached self.charge_item.
- Around line 563-579: The invoice list tests, including
test_list_invoices_with_account_filter and
test_list_invoices_with_payment_reconciliation_present_filter_and_account, do
not prove account filtering because only self.account has an invoice. Extend
create_invoice to accept an account override, create a second account and
invoice for it in both tests, then assert the requested account’s result
contains only the expected invoice and excludes the second account’s invoice.
- Around line 346-364: Update
test_update_invoice_with_no_charge_items_and_issued_status so the generated
request data explicitly contains an empty charge_items list, while preserving
the issued status. Ensure the PUT payload matches the invoice created with
charge_items=[] and continues asserting the existing 400 response and validation
message.
- Around line 1284-1287: Correct the docstring in
test_attach_account_to_invoice_as_user_without_permissions to describe a user
without write permission, matching the test name and covered scenario.
- Around line 706-757: Rename all three test methods beginning with
test_retrive_locked_invoice—test_retrive_locked_invoice_with_superuser,
test_retrive_locked_invoice_with_user_without_permission, and
test_retrive_locked_invoice_with_user_with_permission—to use
test_retrieve_locked_invoice, preserving their behavior and test coverage.
- Around line 168-182: Strengthen
test_create_invoice_without_number_auto_generates by asserting
response.data["number"] equals the value produced by the configured expression,
including the expected INV- prefix, invoice count, and two-digit current year,
rather than only checking that it is truthy.
- Around line 894-910: Update
test_cancel_invoice_with_user_without_permission_outside_period to apply
`@override_settings`(INVOICE_FREE_CANCEL_PERIOD_MINUTES=5), ensuring the invoice
created 10 minutes earlier remains outside the free-cancel period and the 403
assertion exercises the destroy-permission path.
---
Nitpick comments:
In `@care/emr/tests/test_invoice_api.py`:
- Around line 270-281: Update
test_update_invoice_with_user_without_write_permission to store the result of
create_role_with_permissions in a local role variable instead of overwriting
self.role, and pass that local role to attach_role_facility_organization_user.
- Line 912: Update the section comment above the invoice charge-item tests to
remove the stray “def” wording and replace “detach” with “remove,” matching the
endpoint and helper terminology.
- Around line 1118-1128: Extend test_lock_invoice_with_superuser to refresh the
invoice after the lock request, then assert invoice.lock_history contains
exactly one entry whose action is "lock". Also validate the recorded user and
timestamp fields are present, preserving the existing response assertions.
- Around line 89-104: Update the create_invoice helper to generate
deterministic, unique invoice numbers within a test, replacing random.randint
with an existing counter or self.fake.unique mechanism while preserving the
default INV- prefix and allowing an explicitly supplied number via kwargs.
- Around line 77-87: Update generate_invoice_data so the initial data dictionary
assigns the default InvoiceStatusOptions.draft value and default charge-item
list directly, relying on the existing data.update(**kwargs) call to override
them when provided.
- Around line 932-948: Add a negative cross-account or cross-facility test
alongside test_attach_charge_items_to_invoice_with_superuser, creating the
invoice and charge item under different tenants and posting the charge item to
the attach endpoint. Assert the request is rejected and the invoice’s charge
items remain unchanged, using the existing test helpers and URL construction.
🪄 Autofix
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 Plus
Run ID: cedd7106-ca6a-4b73-abe5-066af166e829
📒 Files selected for processing (1)
care/emr/tests/test_invoice_api.py
Comment on lines
+168
to
+182
| def test_create_invoice_without_number_auto_generates(self): | ||
| """ | ||
| Test that omitting number triggers auto-generation via the configured expression. | ||
| """ | ||
| config = FacilityMonetoryConfig.get_monetory_config(self.facility.id) | ||
| config.invoice_number_expression = ( | ||
| "f'INV-{invoice_count + 1}-{current_year_yy}'" | ||
| ) | ||
| config.save() | ||
| self.client.force_authenticate(user=self.superuser) | ||
| data = self.generate_invoice_data() | ||
| data.pop("number") | ||
| response = self.client.post(self.url, data, format="json") | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertTrue(response.data["number"]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the generated number matches the configured expression.
The docstring promises auto-generation "via the configured expression", but line 182 only checks that number is truthy. Any fallback value passes. Assert the actual shape so the expression is really exercised.
💚 Proposed stronger assertion
response = self.client.post(self.url, data, format="json")
self.assertEqual(response.status_code, 200)
- self.assertTrue(response.data["number"])
+ expected_year = datetime.now(UTC).strftime("%y")
+ self.assertEqual(response.data["number"], f"INV-1-{expected_year}")📝 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.
Suggested change
| def test_create_invoice_without_number_auto_generates(self): | |
| """ | |
| Test that omitting number triggers auto-generation via the configured expression. | |
| """ | |
| config = FacilityMonetoryConfig.get_monetory_config(self.facility.id) | |
| config.invoice_number_expression = ( | |
| "f'INV-{invoice_count + 1}-{current_year_yy}'" | |
| ) | |
| config.save() | |
| self.client.force_authenticate(user=self.superuser) | |
| data = self.generate_invoice_data() | |
| data.pop("number") | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 200) | |
| self.assertTrue(response.data["number"]) | |
| def test_create_invoice_without_number_auto_generates(self): | |
| """ | |
| Test that omitting number triggers auto-generation via the configured expression. | |
| """ | |
| config = FacilityMonetoryConfig.get_monetory_config(self.facility.id) | |
| config.invoice_number_expression = ( | |
| "f'INV-{invoice_count + 1}-{current_year_yy}'" | |
| ) | |
| config.save() | |
| self.client.force_authenticate(user=self.superuser) | |
| data = self.generate_invoice_data() | |
| data.pop("number") | |
| response = self.client.post(self.url, data, format="json") | |
| self.assertEqual(response.status_code, 200) | |
| expected_year = datetime.now(UTC).strftime("%y") | |
| self.assertEqual(response.data["number"], f"INV-1-{expected_year}") |
🤖 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/tests/test_invoice_api.py` around lines 168 - 182, Strengthen
test_create_invoice_without_number_auto_generates by asserting
response.data["number"] equals the value produced by the configured expression,
including the expected INV- prefix, invoice count, and two-digit current year,
rather than only checking that it is truthy.
Comment on lines
+346
to
+364
| def test_update_invoice_with_no_charge_items_and_issued_status(self): | ||
| """ | ||
| Test updating an invoice with no charge items and issued status. | ||
| """ | ||
| self.attach_role_facility_organization_user( | ||
| self.organization, self.user, self.role | ||
| ) | ||
| self.attach_role_facility_organization_user(self.organization, self.user, role) | ||
| self.client.force_authenticate(user=self.user) | ||
| invoice = self.create_invoice(charge_items=[]) | ||
| data = self.generate_invoice_data(status=InvoiceStatusOptions.issued.value) | ||
| response = self.client.put( | ||
| self.get_detail_url(invoice.external_id), data, format="json" | ||
| ) | ||
| self.assertEqual(response.status_code, 400) | ||
| response_data = response.data | ||
| self.assertEqual( | ||
| response_data["errors"][0]["msg"], | ||
| "Invoice must have at least one charge item", | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The payload contradicts the test name.
Line 354 creates the invoice with charge_items=[], but line 355 calls generate_invoice_data(status=...), which still sends the default charge_items=[self.charge_item.external_id]. So the request body does contain a charge item. The test currently passes only because the viewset validates the stored invoice rather than the payload. That coupling is invisible to a reader, and the test would silently change meaning if the viewset ever honors payload charge items. Send an empty list to match the stated intent.
💚 Proposed fix
invoice = self.create_invoice(charge_items=[])
- data = self.generate_invoice_data(status=InvoiceStatusOptions.issued.value)
+ data = self.generate_invoice_data(
+ status=InvoiceStatusOptions.issued.value, charge_items=[]
+ )📝 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.
Suggested change
| def test_update_invoice_with_no_charge_items_and_issued_status(self): | |
| """ | |
| Test updating an invoice with no charge items and issued status. | |
| """ | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.attach_role_facility_organization_user(self.organization, self.user, role) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(charge_items=[]) | |
| data = self.generate_invoice_data(status=InvoiceStatusOptions.issued.value) | |
| response = self.client.put( | |
| self.get_detail_url(invoice.external_id), data, format="json" | |
| ) | |
| self.assertEqual(response.status_code, 400) | |
| response_data = response.data | |
| self.assertEqual( | |
| response_data["errors"][0]["msg"], | |
| "Invoice must have at least one charge item", | |
| ) | |
| def test_update_invoice_with_no_charge_items_and_issued_status(self): | |
| """ | |
| Test updating an invoice with no charge items and issued status. | |
| """ | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(charge_items=[]) | |
| data = self.generate_invoice_data( | |
| status=InvoiceStatusOptions.issued.value, charge_items=[] | |
| ) | |
| response = self.client.put( | |
| self.get_detail_url(invoice.external_id), data, format="json" | |
| ) | |
| self.assertEqual(response.status_code, 400) | |
| response_data = response.data | |
| self.assertEqual( | |
| response_data["errors"][0]["msg"], | |
| "Invoice must have at least one charge item", | |
| ) |
🤖 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/tests/test_invoice_api.py` around lines 346 - 364, Update
test_update_invoice_with_no_charge_items_and_issued_status so the generated
request data explicitly contains an empty charge_items list, while preserving
the issued status. Ensure the PUT payload matches the invoice created with
charge_items=[] and continues asserting the existing 400 response and validation
message.
Comment on lines
+563
to
+579
| def test_list_invoices_with_account_filter(self): | ||
| """ | ||
| Test listing invoices with an account filter. | ||
| """ | ||
| self.attach_role_facility_organization_user( | ||
| self.organization, self.user, self.role | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| invoice = self.create_invoice() | ||
| response = self.client.get( | ||
| f"{self.url}?payment_reconciliation_present=false&account={self.account.external_id}", | ||
| format="json", | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| response_data = response.data["results"] | ||
| self.assertEqual(len(response_data), 1) | ||
| self.assertEqual(response_data[0]["id"], str(invoice.external_id)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The account filter is not actually proven.
Only one invoice exists in the database, so len(response_data) == 1 passes even if the server ignores the account parameter entirely. Add a second account with its own invoice, then assert the filtered result excludes it. The same gap applies to test_list_invoices_with_payment_reconciliation_present_filter_and_account at lines 535-561.
💚 Proposed addition
invoice = self.create_invoice()
+ other_account = Account.objects.create(
+ facility=self.facility,
+ patient=self.patient,
+ name="Other Account",
+ status=AccountStatusOptions.active.value,
+ billing_status=AccountBillingStatusOptions.open.value,
+ )
+ self.create_invoice(account=other_account)
response = self.client.get(
f"{self.url}?payment_reconciliation_present=false&account={self.account.external_id}",
format="json",
)Note that create_invoice currently hardcodes account=self.account; it needs an account override for this to work.
🤖 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/tests/test_invoice_api.py` around lines 563 - 579, The invoice list
tests, including test_list_invoices_with_account_filter and
test_list_invoices_with_payment_reconciliation_present_filter_and_account, do
not prove account filtering because only self.account has an invoice. Extend
create_invoice to accept an account override, create a second account and
invoice for it in both tests, then assert the requested account’s result
contains only the expected invoice and excludes the second account’s invoice.
Comment on lines
+706
to
+757
| def test_retrive_locked_invoice_with_superuser(self): | ||
| """ | ||
| Test retrieving a locked invoice with a superuser. | ||
| """ | ||
| self.client.force_authenticate(user=self.superuser) | ||
| invoice = self.create_invoice(locked=True) | ||
| response = self.client.get( | ||
| self.get_detail_url(invoice.external_id), format="json" | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| response_data = response.data | ||
| self.assertEqual(response_data["id"], str(invoice.external_id)) | ||
|
|
||
| def test_retrive_locked_invoice_with_user_without_permission(self): | ||
| """ | ||
| Test retrieving a locked invoice with a user without read permission. | ||
| """ | ||
| permissions = [ | ||
| InvoicePermissions.can_read_invoice.name, | ||
| ] | ||
| self.role = self.create_role_with_permissions(permissions) | ||
| self.attach_role_facility_organization_user( | ||
| self.organization, self.user, self.role | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| invoice = self.create_invoice(locked=True) | ||
| response = self.client.get( | ||
| self.get_detail_url(invoice.external_id), format="json" | ||
| ) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual(response.data["detail"], "Locked invoice permission denied.") | ||
|
|
||
| def test_retrive_locked_invoice_with_user_with_permission(self): | ||
| """ | ||
| Test retrieving a locked invoice with a user with locked invoice management permission. | ||
| """ | ||
| permissions = [ | ||
| InvoicePermissions.can_read_invoice.name, | ||
| InvoicePermissions.can_manage_locked_invoice.name, | ||
| ] | ||
| self.role = self.create_role_with_permissions(permissions) | ||
| self.attach_role_facility_organization_user( | ||
| self.organization, self.user, self.role | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| invoice = self.create_invoice(locked=True) | ||
| response = self.client.get( | ||
| self.get_detail_url(invoice.external_id), format="json" | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| response_data = response.data | ||
| self.assertEqual(response_data["id"], str(invoice.external_id)) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the "retrive" spelling in three test names.
Lines 706, 719, and 738 all use test_retrive_locked_invoice_.... The word is "retrieve". These names appear in test output, so the typo travels. Rename all three.
♻️ Proposed rename
- def test_retrive_locked_invoice_with_superuser(self):
+ def test_retrieve_locked_invoice_with_superuser(self):- def test_retrive_locked_invoice_with_user_without_permission(self):
+ def test_retrieve_locked_invoice_with_user_without_permission(self):- def test_retrive_locked_invoice_with_user_with_permission(self):
+ def test_retrieve_locked_invoice_with_user_with_permission(self):As per coding guidelines: "Use descriptive variable and function names; adhere to naming conventions".
📝 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.
Suggested change
| def test_retrive_locked_invoice_with_superuser(self): | |
| """ | |
| Test retrieving a locked invoice with a superuser. | |
| """ | |
| self.client.force_authenticate(user=self.superuser) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| response_data = response.data | |
| self.assertEqual(response_data["id"], str(invoice.external_id)) | |
| def test_retrive_locked_invoice_with_user_without_permission(self): | |
| """ | |
| Test retrieving a locked invoice with a user without read permission. | |
| """ | |
| permissions = [ | |
| InvoicePermissions.can_read_invoice.name, | |
| ] | |
| self.role = self.create_role_with_permissions(permissions) | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 403) | |
| self.assertEqual(response.data["detail"], "Locked invoice permission denied.") | |
| def test_retrive_locked_invoice_with_user_with_permission(self): | |
| """ | |
| Test retrieving a locked invoice with a user with locked invoice management permission. | |
| """ | |
| permissions = [ | |
| InvoicePermissions.can_read_invoice.name, | |
| InvoicePermissions.can_manage_locked_invoice.name, | |
| ] | |
| self.role = self.create_role_with_permissions(permissions) | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| response_data = response.data | |
| self.assertEqual(response_data["id"], str(invoice.external_id)) | |
| def test_retrieve_locked_invoice_with_superuser(self): | |
| """ | |
| Test retrieving a locked invoice with a superuser. | |
| """ | |
| self.client.force_authenticate(user=self.superuser) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| response_data = response.data | |
| self.assertEqual(response_data["id"], str(invoice.external_id)) | |
| def test_retrieve_locked_invoice_with_user_without_permission(self): | |
| """ | |
| Test retrieving a locked invoice with a user without read permission. | |
| """ | |
| permissions = [ | |
| InvoicePermissions.can_read_invoice.name, | |
| ] | |
| self.role = self.create_role_with_permissions(permissions) | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 403) | |
| self.assertEqual(response.data["detail"], "Locked invoice permission denied.") | |
| def test_retrieve_locked_invoice_with_user_with_permission(self): | |
| """ | |
| Test retrieving a locked invoice with a user with locked invoice management permission. | |
| """ | |
| permissions = [ | |
| InvoicePermissions.can_read_invoice.name, | |
| InvoicePermissions.can_manage_locked_invoice.name, | |
| ] | |
| self.role = self.create_role_with_permissions(permissions) | |
| self.attach_role_facility_organization_user( | |
| self.organization, self.user, self.role | |
| ) | |
| self.client.force_authenticate(user=self.user) | |
| invoice = self.create_invoice(locked=True) | |
| response = self.client.get( | |
| self.get_detail_url(invoice.external_id), format="json" | |
| ) | |
| self.assertEqual(response.status_code, 200) | |
| response_data = response.data | |
| self.assertEqual(response_data["id"], str(invoice.external_id)) |
🤖 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/tests/test_invoice_api.py` around lines 706 - 757, Rename all three
test methods beginning with
test_retrive_locked_invoice—test_retrive_locked_invoice_with_superuser,
test_retrive_locked_invoice_with_user_without_permission, and
test_retrive_locked_invoice_with_user_with_permission—to use
test_retrieve_locked_invoice, preserving their behavior and test coverage.
Source: Coding guidelines
Comment on lines
+894
to
+910
| def test_cancel_invoice_with_user_without_permission_outside_period(self): | ||
| """ | ||
| Test cancelling an invoice with a user without write permission outside the free cancel period. | ||
| """ | ||
| self.attach_role_facility_organization_user( | ||
| self.organization, self.user, self.role | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| invoice = self.create_invoice() | ||
| # Manually set the issue_date to be outside the free cancel period | ||
| invoice.created_date = datetime.now(UTC) - timedelta(minutes=10) | ||
| invoice.save() | ||
| response = self.cancel_invoice( | ||
| invoice.external_id, InvoiceStatusOptions.cancelled.value | ||
| ) | ||
| self.assertEqual(response.status_code, 403) | ||
| self.assertEqual(response.data["detail"], "Cannot cancel invoice") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the default free-cancel period and the created_date field definition.
# Default cancellation window
rg -n 'INVOICE_FREE_CANCEL_PERIOD_MINUTES' --type=py
# created_date definition on the EMR base model
fd -t f 'base.py' -p 'care/emr/models' --exec rg -n -C3 'created_date'Repository: ohcnetwork/care
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repo files count: '
git ls-files | wc -l
printf '\nCandidate Python files containing invoice cancel/free settings text:\n'
git ls-files '*.py' | xargs grep -liE 'free cancel|FREE_CANCEL|cancel_invoice|created_date' || true
printf '\nLocate target test file:\n'
fd -t f 'test_invoice_api.py' .
printf '\nSearch symbol INVOICE_FREE_CANCEL_PERIOD_MINUTES ignoring case/type filter:\n'
git ls-files | xargs grep -RIn 'INVOICE_FREE_CANCEL_PERIOD_MINUTES' || true
printf '\nSearch invoices model/base files:\n'
git ls-files '*.py' | xargs grep -lE 'class .*Invoice|created_date|auto_now_add' || trueRepository: ohcnetwork/care
Length of output: 14157
🏁 Script executed:
#!/bin/bash
set -u
printf 'Invoiceviewset relevant methods:\n'
sed -n '260,350p' care/emr/api/viewsets/invoice.py
printf '\nBilling role/permission setup in test:\n'
sed -n '1,130p' care/emr/tests/test_invoice_api.py
sed -n '860,915p' care/emr/tests/test_invoice_api.py
printf '\nInvoice model fields:\n'
sed -n '1,180p' care/emr/models/invoice.py
printf '\nDjango import in test file:\n'
sed -n '1,40p' care/emr/tests/test_invoice_api.pyRepository: ohcnetwork/care
Length of output: 15471
🏁 Script executed:
#!/bin/bash
set -u
printf 'Settings definition with surrounding context:\n'
sed -n '240,265p' config/settings/config.py
printf '\nRelevant InvoicePermissions definitions:\n'
rg -n -C4 "can_write_invoice|can_destroy_invoice|InvoicePermissions" care/security care/emr --glob '*.py'
printf '\nFind EMRBaseModel created_date definition:\n'
fd -t f '*.py' . | xargs grep -n "class EMRBaseModel\|created_date" | head -80
printf '\nCheck whether Invoice has issue_date-related base fields in migrations:\n'
sed -n '1,220p' care/emr/migrations/0001_initial.py | grep -n -C3 -i "invoice\|created_date\|updated_date\|issue_date" || trueRepository: ohcnetwork/care
Length of output: 28852
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from datetime import datetime, timedelta, UTC
created = datetime.now(UTC) - timedelta(minutes=10)
now = datetime.now(UTC)
for minutes in [0, 0.5, 1, 5, 10, 60]:
inside_window = created >= now - timedelta(minutes=minutes)
permission_needed = "can_write_invoice_in_facility" if inside_window else "can_destroy_invoice_in_facility"
print(f"{minutes}: inside={inside_window} needs={permission_needed}")
PYRepository: ohcnetwork/care
Length of output: 315
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
from datetime import datetime, timedelta
UTC = datetime.utcnow()
created = UTC - timedelta(minutes=10)
now = UTC
for minutes in [0, 0.5, 1, 5, 10, 60]:
inside_window = created >= now - timedelta(minutes=minutes)
permission_needed = "can_write_invoice_in_facility" if inside_window else "can_destroy_invoice_in_facility"
print(f"{minutes}: inside={inside_window} needs={permission_needed}")
PYRepository: ohcnetwork/care
Length of output: 475
Set the free-cancel period to an outside-period value before asserting the destroy-permission path.
The default INVOICE_FREE_CANCEL_PERIOD_MINUTES=0 makes this test pass, but a later settings bump to 10 minutes or more moves the 10-minute-old invoice into can_write_invoice_in_facility. Add an explicit @override_settings(INVOICE_FREE_CANCEL_PERIOD_MINUTES=5) so the 403 assertion tests the outside-period branch.
🤖 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/tests/test_invoice_api.py` around lines 894 - 910, Update
test_cancel_invoice_with_user_without_permission_outside_period to apply
`@override_settings`(INVOICE_FREE_CANCEL_PERIOD_MINUTES=5), ensuring the invoice
created 10 minutes earlier remains outside the free-cancel period and the 403
assertion exercises the destroy-permission path.
Comment on lines
+1255
to
+1266
| def test_attach_account_to_invoice_as_superuser(self): | ||
| """ | ||
| Test attaching account to an invoice as superuser | ||
| """ | ||
| self.client.force_authenticate(user=self.superuser) | ||
| invoice = self.create_invoice() | ||
| url = self.get_attach_account_url(invoice.external_id) | ||
| response = self.client.post(url, format="json") | ||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual( | ||
| response.data["charge_items"][0]["id"], str(self.charge_item.external_id) | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The assertion does not prove the account attach did anything.
self.charge_item is already attached to the invoice by create_invoice, so response.data["charge_items"][0]["id"] matches before the request is ever sent. Create an extra billable charge item on the account that is not on the invoice, then assert it appears in the response. Otherwise this test passes even if the endpoint is a no-op. The same applies to test_attach_account_to_invoice_as_user_with_permissions at lines 1268-1282.
🤖 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/tests/test_invoice_api.py` around lines 1255 - 1266, Update
test_attach_account_to_invoice_as_superuser and
test_attach_account_to_invoice_as_user_with_permissions to create an additional
billable charge item on the account after create_invoice, ensuring it is not
initially attached to the invoice. Assert that this new item appears in
response.data["charge_items"] after the request, rather than asserting the
pre-attached self.charge_item.
Comment on lines
+1284
to
+1287
| def test_attach_account_to_invoice_as_user_without_permissions(self): | ||
| """ | ||
| Test attaching account to an invoice as user with permissions | ||
| """ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the docstring.
The docstring says "as user with permissions", but this test covers the user without write permission. It was copied from the test above.
♻️ Proposed fix
def test_attach_account_to_invoice_as_user_without_permissions(self):
"""
- Test attaching account to an invoice as user with permissions
+ Test attaching account to an invoice as user without permissions
"""📝 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.
Suggested change
| def test_attach_account_to_invoice_as_user_without_permissions(self): | |
| """ | |
| Test attaching account to an invoice as user with permissions | |
| """ | |
| def test_attach_account_to_invoice_as_user_without_permissions(self): | |
| """ | |
| Test attaching account to an invoice as user without permissions | |
| """ |
🤖 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/tests/test_invoice_api.py` around lines 1284 - 1287, Correct the
docstring in test_attach_account_to_invoice_as_user_without_permissions to
describe a user without write permission, matching the test name and covered
scenario.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3726 +/- ##
===========================================
+ Coverage 79.45% 80.08% +0.62%
===========================================
Files 480 480
Lines 23215 23214 -1
Branches 2420 2420
===========================================
+ Hits 18446 18590 +144
+ Misses 4165 4025 -140
+ Partials 604 599 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Associated Issue
-ENG-834
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