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 73.19% 73.48% +0.28%
===========================================
Files 435 435
Lines 19729 19725 -4
Branches 2132 2131 -1
===========================================
+ Hits 14441 14495 +54
+ Misses 4849 4790 -59
- Partials 439 440 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAuthorization error messages in the delivery order viewset were updated to reference "delivery orders" instead of "supply requests." The authorize_retrieve logic was revised to require read access to either origin or destination facilities. A comprehensive test suite was added covering creation, update, retrieval, and list operations with various permission scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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.
| def test_list_delivery_orders_as_superuser(self): | ||
| """Test listing delivery orders as superuser""" | ||
| delivery_order1 = self.create_delivery_order( | ||
| origin=self.origin, | ||
| destination=self.destination, | ||
| ) | ||
| delivery_order2 = self.create_delivery_order( | ||
| supplier=self.supplier, | ||
| destination=self.destination, | ||
| ) | ||
| self.client.force_authenticate(user=self.superuser) | ||
| url = self.generate_base_url(self.facility.external_id) | ||
| response = self.client.get(url, format="json") | ||
| 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): | ||
| """Test listing delivery orders as a user with permission""" | ||
| delivery_order1 = self.create_delivery_order( | ||
| origin=self.origin, | ||
| destination=self.destination, | ||
| ) | ||
| delivery_order2 = self.create_delivery_order( | ||
| supplier=self.supplier, | ||
| destination=self.destination, | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| self.attach_role_facility_organization_user( | ||
| role=self.role, | ||
| facility_organization=self.facility_organization, | ||
| user=self.user, | ||
| ) | ||
| url = self.generate_base_url(self.facility.external_id) | ||
| response = self.client.get(url, format="json") | ||
| 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): | ||
| """Test listing delivery orders as a user without permission without filters""" | ||
| delivery_order1 = self.create_delivery_order( | ||
| origin=self.origin, | ||
| destination=self.destination, | ||
| ) | ||
| delivery_order2 = self.create_delivery_order( | ||
| supplier=self.supplier, | ||
| destination=self.destination, | ||
| ) | ||
| self.client.force_authenticate(user=self.user) | ||
| url = self.generate_base_url(self.facility.external_id) | ||
| response = self.client.get(url, format="json") | ||
| 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"]], | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
List tests mix response shape and permission expectations; tighten assertions
Two small things here:
-
In the “list as superuser / user with permission / user without permission” tests you assert
len(response.data) == 2and then indexresponse.data["results"]. That length check is really asserting the number of top‑level keys, not the number of orders. If the intention is to assert the number of returned orders, it would be more robust to check either:len(response.data["results"]), or- a dedicated
countfield, if present.
-
test_list_delivery_orders_as_user_without_permissionasserts a 200 with both orders visible to a user with no delivery‑related permissions. That matches the current viewset (no permission check when no origin/destination filter is applied), but it’s a fairly permissive behavior for inventory data. Just double‑check that this is explicitly desired and not an accidental gap in authorization policy.
Neither of these blocks the PR, but they’re worth a quick confirm so future readers don’t have to reverse‑engineer the intent.
🏁 Script executed:
#!/bin/bash
# Optional: search for other list tests in this repo to see how list responses are usually asserted,
# and whether open list access without explicit permissions is a common pattern.
rg -n "test_list_.*delivery" -S
rg -n "results\]\"" -S care/emr/testsLength of output: 1787
Let me gather more information to verify the concerns.
Now let me inspect the actual test file and viewset to understand the response structure and permissions logic.
🏁 Script executed:
#!/bin/bash
# Examine the test file and view its actual response handling
cd /tmp && \
cat care/emr/tests/test_delivery_order_api.py | sed -n '481,600p'Length of output: 73
🏁 Script executed:
#!/bin/bash
# Find the viewset for delivery orders
fd -e py -type f | xargs grep -l "DeliveryOrderViewSet" | head -5Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for the response structure assertion more carefully
rg -n "response\.data\[" care/emr/tests/test_delivery_order_api.py -A2 -B2Length of output: 10463
🏁 Script executed:
#!/bin/bash
# Look for pagination configuration in settings or viewset
rg -n "pagination\|PAGINATION" care/emr --type=py | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for permission checks in delivery order related code
rg -n "permission\|has_perm" care/emr --type=py -lLength of output: 0
Fix assertion pattern for order counts and clarify permission behavior
Both concerns are verified:
-
Lines 493, 494, 495: The three "list as..." tests incorrectly assert
len(response.data) == 2, checking top-level dict keys instead of the order count. All other list tests in the file uselen(response.data["results"])instead. Change to:self.assertEqual(len(response.data["results"]), 2)
-
Lines 534–557 (
test_list_delivery_orders_as_user_without_permission): This test grants 200 + full visibility to an unpermuted user on unfiltered requests, while other tests show that filtered requests from unpermitted users get 403 + "Cannot list delivery orders" (e.g., line 596). This inconsistency should be intentional—confirm the viewset logic allows unfiltered list access without permissions, or tighten it to require explicit permission regardless of filters.
🤖 Prompt for AI Agents
In care/emr/tests/test_delivery_order_api.py around lines 481-557, update the
three "list" tests so they assert the number of returned orders correctly by
using self.assertEqual(len(response.data["results"]), 2) instead of
self.assertEqual(len(response.data), 2); additionally, make permission behavior
consistent by changing test_list_delivery_orders_as_user_without_permission to
expect the same denied response as other filtered-unpermitted cases (assert
response.status_code == 403 and assert the error message contains "Cannot list
delivery orders") so the test matches the viewset's requirement that listing
requires explicit permission.
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