[ENG-88] Test cases for delivery order#3318
Conversation
…er/authz fix:validation update
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3318 +/- ##
===========================================
+ Coverage 77.53% 78.33% +0.80%
===========================================
Files 479 479
Lines 22998 22994 -4
Branches 2379 2378 -1
===========================================
+ Hits 17831 18013 +182
+ Misses 4613 4412 -201
- Partials 554 569 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated delivery-order authorization to require read access to at least one of origin or destination and corrected authorization error messages to reference "delivery orders"; added an extensive delivery order API test suite; renamed a supply delivery test base class, added a subclass, and included a 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 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 |
![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
🧹 Nitpick comments (3)
care/emr/api/viewsets/inventory/delivery_order.py (1)
122-137: authorize_retrieve logic matches intent; consider minor readability tweakThe new logic (require read access to origin or destination, using
raise_error=Falseand a singlePermissionDenied("Cannot read delivery orders")) matches the docstring and keeps failure messaging consistent. If you ever touch this again, you might consider naming the booleans (origin_allowed,destination_allowed) to avoid the nested parentheses, but as is it’s still quite readable.care/emr/tests/test_delivery_order_api.py (2)
33-40: FacilityLocationOrganization helper may bind locations to the wrong facility_organization
create_facility_locationalways usesself.facility_organization, even when creating locations for another facility (e.g., in the mismatched origin/destination test). That can produceFacilityLocationOrganizationrows whereorganization.facilitydoesn’t matchlocation.facility, which is confusing at best.It’s a small test‑only thing, but it would be cleaner to either:
- accept a
facility_organizationargument, or- create a matching facility_organization for the passed
facilityinside this helper.
379-477: Retrieve tests validate basic permission boundaries; partial-access cases could be added laterYou’re covering internal vs external orders and the with/without permission cases, including the “Cannot read delivery orders” message. That matches the new
authorize_retrievebehavior.If you ever need to exercise the “origin OR destination” semantics more explicitly, additional tests where a user has access to only one side (origin‑only or destination‑only) would make that logic harder to accidentally break, but it’s not strictly required to merge this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
care/emr/api/viewsets/inventory/delivery_order.py(3 hunks)care/emr/tests/test_delivery_order_api.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/inventory/delivery_order.pycare/emr/tests/test_delivery_order_api.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_delivery_order_api.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: rithviknishad
Repo: ohcnetwork/care PR: 3176
File: care/emr/api/viewsets/inventory/product_knowledge.py:33-40
Timestamp: 2025-07-30T12:06:18.804Z
Learning: In EMR viewsets using EMRUpsertMixin, the upsert method properly handles authorization by delegating to handle_update() and handle_create(), which in turn call authorize_update() and authorize_create() respectively. No separate authorize_upsert() method is needed.
🧬 Code graph analysis (2)
care/emr/api/viewsets/inventory/delivery_order.py (3)
care/emr/api/viewsets/inventory/request_order.py (1)
authorize_location_read(71-78)care/emr/api/viewsets/inventory/supply_delivery.py (1)
authorize_location_read(140-147)care/emr/api/viewsets/inventory/dispense_order.py (1)
authorize_location_read(54-57)
care/emr/tests/test_delivery_order_api.py (4)
care/emr/models/location.py (2)
FacilityLocation(12-127)FacilityLocationOrganization(130-143)care/emr/resources/inventory/supply_delivery/delivery_order.py (1)
SupplyDeliveryOrderStatusOptions(18-24)care/security/permissions/supply_delivery.py (1)
SupplyDeliveryPermissions(16-37)care/utils/tests/base.py (4)
CareAPITestBase(22-128)create_facility_organization(46-49)create_role_with_permissions(58-71)attach_role_facility_organization_user(112-115)
🪛 Ruff (0.14.4)
care/emr/api/viewsets/inventory/delivery_order.py
72-72: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (4)
care/emr/api/viewsets/inventory/delivery_order.py (1)
67-83: Updated error messages look correct and consistentThe switched messages to "Cannot list/write delivery orders" match the domain language and the expectations in the new tests. The authorization wiring still mirrors the other inventory viewsets, so behavior is preserved apart from the wording change.
care/emr/tests/test_delivery_order_api.py (3)
71-200: Create‑flow tests are thorough and aligned with the viewset behaviorThe create tests cover internal vs external orders, superuser vs permissioned vs unpermissioned users, and the cross‑facility origin/destination validation. Assertions on the exact error messages for forbidden writes and mismatched facilities line up with the viewset changes and should catch regressions nicely.
203-376: Update‑flow tests give good coverage for internal/external + permissionsThe update tests mirror the create cases (internal/external, superuser, permissioned, unpermissioned) and validate both success paths and the “Cannot write delivery orders” failure. This lines up with
authorize_update’s origin/destination ownership rules, so you’ve got solid guard rails here.
558-888: Filter and hierarchy tests give strong coverage of list behaviorThe origin/destination filters,
include_children,origin_isnull, and supplier/status combinations are all exercised with both positive and negative cases. That aligns nicely withDeliveryOrderFiltersandfilter_location_queryset, and should keep any future refactors of the filtering logic honest.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
care/emr/tests/test_delivery_order_api.py (1)
45-52:create_facility_locationis duplicated fromTestSupplyDeliveryViewSetBase.This exact helper (make a
FacilityLocation+FacilityLocationOrganization) already exists inTestSupplyDeliveryViewSetBase. Consider pulling it up intoCareAPITestBaseso both test classes can share it. Or not — I'm sure copy-paste is a perfectly valid design pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_delivery_order_api.py` around lines 45 - 52, Duplicate helper create_facility_location exists in TestSupplyDeliveryViewSetBase and this test file; move the shared logic into CareAPITestBase (or a common test base) as a single method definition that creates FacilityLocation and FacilityLocationOrganization, then update TestSupplyDeliveryViewSetBase and this test to call CareAPITestBase.create_facility_location (or remove the duplicate here and inherit it). Ensure the method signature and baker.make calls (creating FacilityLocation and FacilityLocationOrganization with facility and facility_organization) remain the same so tests using create_facility_location keep their behavior.care/emr/tests/test_supply_delivery.py (1)
211-214: RedundantsetUpoverride.This
setUponly callssuper().setUp()and adds nothing else. You could omit it entirely — Python will call the parent'ssetUpautomatically. But hey, I'm sure there was a very important reason to keep it.♻️ Suggested simplification
class TestSupplyDeliveryViewSet(TestSupplyDeliveryViewSetBase): - def setUp(self): - super().setUp() - - def test_create_supply_delivery_internally_as_superuser(self): + def test_create_supply_delivery_internally_as_superuser(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_supply_delivery.py` around lines 211 - 214, The TestSupplyDeliveryViewSet class contains a redundant setUp method that only calls super().setUp(); remove the setUp override from class TestSupplyDeliveryViewSet so the parent TestSupplyDeliveryViewSetBase.setUp will be used automatically, keeping behavior unchanged and eliminating dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/tests/test_delivery_order_api.py`:
- Around line 956-968: The triple-quoted string inside the setUp method is
placed after super().setUp() so it is treated as a dead string expression rather
than documentation; fix by moving that multi-line string immediately after the
def setUp(self): line (so it becomes the method docstring) or replace the
triple-quoted block with equivalent # comments, and ensure the call to
super().setUp() remains after the docstring; refer to the setUp method in the
TestSupplyDeliveryViewSetBase-derived test class to locate and update the block.
---
Duplicate comments:
In `@care/emr/tests/test_delivery_order_api.py`:
- Around line 545-621: The tests test_list_delivery_orders_as_superuser,
test_list_delivery_orders_as_user_with_permission, and
test_list_delivery_orders_as_user_without_permission currently use
len(response.data) which counts top-level dict keys; change those assertions to
check the number of returned orders instead (e.g.,
assertEqual(len(response.data["results"]), 2) or
assertEqual(response.data.get("count"), 2)) and update any similar len(...)
checks to reference "results" or "count"; also flag the inconsistent
authorization behavior (200 vs 403) for the unpermitted user so the reviewer can
confirm intended behavior.
---
Nitpick comments:
In `@care/emr/tests/test_delivery_order_api.py`:
- Around line 45-52: Duplicate helper create_facility_location exists in
TestSupplyDeliveryViewSetBase and this test file; move the shared logic into
CareAPITestBase (or a common test base) as a single method definition that
creates FacilityLocation and FacilityLocationOrganization, then update
TestSupplyDeliveryViewSetBase and this test to call
CareAPITestBase.create_facility_location (or remove the duplicate here and
inherit it). Ensure the method signature and baker.make calls (creating
FacilityLocation and FacilityLocationOrganization with facility and
facility_organization) remain the same so tests using create_facility_location
keep their behavior.
In `@care/emr/tests/test_supply_delivery.py`:
- Around line 211-214: The TestSupplyDeliveryViewSet class contains a redundant
setUp method that only calls super().setUp(); remove the setUp override from
class TestSupplyDeliveryViewSet so the parent
TestSupplyDeliveryViewSetBase.setUp will be used automatically, keeping behavior
unchanged and eliminating dead code.
Re-enable the superclass setup method in the test case.
|
Tests failing |
Greptile SummaryThis PR adds a comprehensive test suite for the delivery order API and makes two targeted fixes to
Confidence Score: 4/5Safe to merge once the three list-test len(response.data) assertions are corrected; the viewset changes themselves are correct. The viewset fixes are clean and the medication-return tests correctly use entered_in_error. However, three unfiltered list tests assert len(response.data) == 2 rather than len(response.data["results"]) == 2; these will produce AssertionError: 4 != 2 under DRF pagination, meaning those test cases will never pass as written. These were flagged in the previous review round but remain unaddressed. care/emr/tests/test_delivery_order_api.py — the three unfiltered list tests at lines 632, 661, and 685 Important Files Changed
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/tes..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(len(response.data), 2) | ||
| self.assertIn( | ||
| str(delivery_order1.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
| self.assertIn( | ||
| str(delivery_order2.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
|
|
||
| def test_list_delivery_orders_as_user_with_permission(self): |
There was a problem hiding this comment.
len(response.data) counts dict keys, not result items, so these assertions always produce the wrong number
When DRF pagination is active, response.data is an OrderedDict with keys count, next, previous, results. len(response.data) returns 4 (the number of dict keys), never 2. The same test file already uses len(response.data["results"]) consistently in every other list test (e.g., lines 692, 789, 876), so these three assertions are out of step with the rest of the suite and will fail whenever pagination is applied.
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_as_user_with_permission(self): | |
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data["results"]), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_as_user_with_permission(self): |
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(len(response.data), 2) | ||
| self.assertIn( | ||
| str(delivery_order1.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
| self.assertIn( | ||
| str(delivery_order2.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
|
|
||
| def test_list_delivery_orders_as_user_without_permission(self): |
There was a problem hiding this comment.
len(response.data) issue in the user-with-permission list test. Should reference response.data["results"] for consistency with the rest of the file.
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_as_user_without_permission(self): | |
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data["results"]), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_as_user_without_permission(self): |
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(len(response.data), 2) | ||
| self.assertIn( | ||
| str(delivery_order1.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
| self.assertIn( | ||
| str(delivery_order2.external_id), | ||
| [order["id"] for order in response.data["results"]], | ||
| ) | ||
|
|
||
| def test_list_delivery_orders_with_origin_filter(self): |
There was a problem hiding this comment.
len(response.data) issue in the user-without-permission list test.
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_with_origin_filter(self): | |
| self.assertEqual(response.status_code, 200) | |
| self.assertEqual(len(response.data["results"]), 2) | |
| self.assertIn( | |
| str(delivery_order1.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| self.assertIn( | |
| str(delivery_order2.external_id), | |
| [order["id"] for order in response.data["results"]], | |
| ) | |
| def test_list_delivery_orders_with_origin_filter(self): |
| class TestSupplyDeliveryViewSet(TestSupplyDeliveryViewSetBase): | ||
| def setUp(self): | ||
| super().setUp() | ||
|
|
||
| def test_create_supply_delivery_internally_as_superuser(self): |
There was a problem hiding this comment.
setUp override in TestSupplyDeliveryViewSet is a no-op — it just calls super().setUp(), which Python would invoke automatically via inheritance. The override can be removed.
| class TestSupplyDeliveryViewSet(TestSupplyDeliveryViewSetBase): | |
| def setUp(self): | |
| super().setUp() | |
| def test_create_supply_delivery_internally_as_superuser(self): | |
| class TestSupplyDeliveryViewSet(TestSupplyDeliveryViewSetBase): | |
| def test_create_supply_delivery_internally_as_superuser(self): |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
vigneshhari
changed the title
4 participants
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
Bug Fixes
Tests