Skip to content

bug fixes and testcases for medication dispense#3589

Open
nandkishorr wants to merge 12 commits intodevelopfrom
nandkishorr/test/medication_dispense
Open

bug fixes and testcases for medication dispense#3589
nandkishorr wants to merge 12 commits intodevelopfrom
nandkishorr/test/medication_dispense

Conversation

@nandkishorr
Copy link
Copy Markdown
Contributor

@nandkishorr nandkishorr commented Mar 25, 2026

Proposed Changes

  • Added testcases for medication dispense
  • Updated the location filter to dummyfilter
  • Added order_by filter for summary api queryset

Associated Issue

Merge Checklist

  • Tests added/fixed
  • Update docs in /docs
  • Linting Complete
  • Any other necessary step

Only 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

  • Tests

    • Added comprehensive test suite for medication dispense API covering creation, updates, cancellation, filtering, and authorization scenarios.
  • Bug Fixes

    • Updated medication dispense API filtering and result ordering for improved accuracy and consistency.

@nandkishorr nandkishorr self-assigned this Mar 25, 2026
@nandkishorr nandkishorr requested a review from a team as a code owner March 25, 2026 13:08
@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Mar 25, 2026

Greptile Summary

This PR delivers three targeted changes to the medication dispense feature: a bug fix converting the location filter to a DummyUUIDFilter so it no longer conflicts with the manual location-filtering logic already implemented in get_queryset, an order_by("encounter_id") added to the summary action's queryset to ensure correct GROUP BY behaviour when paginating, and a new comprehensive test suite (test_medication_dispense_api.py) covering create, update, list, retrieve, and summary endpoints.

  • The DummyUUIDFilter change is correct: the location field in MedicationDispenseFilters was previously applying an ORM filter via location__external_id, which would run in addition to (and could conflict with) the more complex manual filtering (including parent_cache overlap for child locations) already done in get_queryset.
  • The order_by("encounter_id") addition is a valid fix to prevent non-deterministic GROUP BY results caused by a default model ordering interfering with values().annotate().
  • The test suite is thorough, but test_list_medication_dispense_with_location_include_children_filter (line 746) contains a bug: the second medication dispense is created without location=child_location, so both records end up at the parent location. The test passes for the wrong reason and does not actually verify that child-location records are included when include_children=True.

Confidence Score: 3/5

  • Safe to merge after fixing the include-children test; the production code changes are correct.
  • The two production-code fixes are straightforward and correct. The test file is large and mostly sound, but the include_children=True test silently passes without exercising the actual child-location filtering path, which means a regression in that logic could go undetected.
  • care/emr/tests/test_medication_dispense_api.py — line 746 needs location=child_location to make the include-children test meaningful.

Important Files Changed

Filename Overview
care/emr/api/viewsets/medication_dispense.py Two targeted bug fixes: location filter changed to DummyUUIDFilter (preventing a double-filter conflict with manual location logic in get_queryset), and .order_by("encounter_id") added before .annotate() in the summary action to ensure correct GROUP BY behaviour.
care/emr/tests/test_medication_dispense_api.py Comprehensive new test suite covering create, update, list, retrieve, and summary flows. One test (test_list_medication_dispense_with_location_include_children_filter) silently passes without actually testing the include-children path because the child-location dispense is missing location=child_location.

Reviews (1): Last reviewed commit: "bug:update the location filter and summa..." | Re-trigger Greptile

Comment thread care/emr/tests/test_medication_dispense_api.py Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 25, 2026

Warning

Rate limit exceeded

@nandkishorr has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 8 minutes and 47 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 47 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: b1cbce2c-e195-4081-9474-39d38dde6905

📥 Commits

Reviewing files that changed from the base of the PR and between 2496272 and 3173144.

📒 Files selected for processing (1)
  • care/emr/tests/test_medication_dispense_api.py
📝 Walkthrough

Walkthrough

The pull request adds a comprehensive test suite for medication dispense API endpoints and updates the medication dispense viewset to use a custom UUID filter for location validation and explicit encounter_id ordering in the summary action results.

Changes

Cohort / File(s) Summary
Viewset Filter & Ordering Updates
care/emr/api/viewsets/medication_dispense.py
Replaced filters.UUIDFilter with DummyUUIDFilter for location filter handling and added explicit .order_by("encounter_id") to the summary action's queryset prior to pagination.
Medication Dispense API Test Suite
care/emr/tests/test_medication_dispense_api.py
New comprehensive test module establishing full EMR scenario with users, facilities, inventory, and medications. Covers creation, updates, cancellation, listing, retrieval, and summary aggregation across multiple endpoints with assertions for auth/authorization, domain constraints, side effects, and filter requirements.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is partially related to the changeset, mentioning test cases and medication dispense but lacks specificity about the actual bug fixes and filter changes. Consider being more specific about the main changes, such as 'Add medication dispense test cases and fix location filter ordering' for better clarity.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description covers the main changes and includes the required template sections, though it could provide more detail about what the bug fixes specifically address.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nandkishorr/test/medication_dispense

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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_medication_dispense_api.py`:
- Around line 833-853: The test test_summary_medication_dispense_as_superuser
currently only verifies aggregation because both dispenses share the same
encounter; modify it to create a second MedicationDispense tied to a different
encounter (use create_dispense_order to create a second encounter-order pair,
then create_medication_dispense for that order), call generate_summary_url the
same way, then assert that response.data["results"] contains two entries in the
expected encounter order and that their counts correspond to each encounter
(this will detect removal of the order_by("encounter_id") sorting).
- Around line 727-753: The test
test_list_medication_dispense_with_location_include_children_filter creates a
child_location and a dispense order for it but then calls
create_medication_dispense(order=another_dispense_order) without passing
location, so both dispenses end up attached to self.location; update the second
create_medication_dispense call to include location=child_location (i.e. call
create_medication_dispense(order=another_dispense_order,
location=child_location)) so the include_children=True path is actually
exercised.
🪄 Autofix (Beta)

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

Run ID: 65eb251d-9f49-4098-b652-a72f6618cb6a

📥 Commits

Reviewing files that changed from the base of the PR and between 0a72994 and 2496272.

📒 Files selected for processing (2)
  • care/emr/api/viewsets/medication_dispense.py
  • care/emr/tests/test_medication_dispense_api.py

Comment thread care/emr/tests/test_medication_dispense_api.py
Comment thread care/emr/tests/test_medication_dispense_api.py
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.71%. Comparing base (2645cb0) to head (13fedd4).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3589      +/-   ##
===========================================
+ Coverage    77.20%   77.71%   +0.51%     
===========================================
  Files          474      474              
  Lines        22421    22421              
  Branches      2348     2348              
===========================================
+ Hits         17310    17425     +115     
+ Misses        4531     4414     -117     
- Partials       580      582       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

queryset = (
self.filter_queryset(self.get_queryset())
.values("encounter_id")
.order_by("encounter_id")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?

Copy link
Copy Markdown
Contributor Author

@nandkishorr nandkishorr Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vigneshhari ,instead of showing the n count of medication dispense related to an encounter in the response ,the api response returns n times encounter details with each count =1

@nandkishorr nandkishorr requested a review from vigneshhari March 27, 2026 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants