Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughA new test file is added for the Observation API endpoints, introducing comprehensive test coverage for list, retrieve, create, update, delete, and analyse actions. Tests verify authentication, permissions, filtering by encounter and codes, ordering, and response structure. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/emr/tests/test_observation_api.py (1)
173-183: Ordering assertion is a bit brittle under timestamp tiesThis test assumes
obs2.modified_date > obs1.modified_date. If both rows get identicalmodified_date, DB tie-order can be nondeterministic and the test may flake.Suggested hardening
def test_list_ordered_by_modified_date_descending(self): self._grant_patient_read_permission() - obs1 = self._create_observation() - obs2 = self._create_observation() + obs1 = self._create_observation(modified_date="2026-01-01T00:00:00Z") + obs2 = self._create_observation(modified_date="2026-01-01T00:00:01Z")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/tests/test_observation_api.py` around lines 173 - 183, The test test_list_ordered_by_modified_date_descending is brittle because it assumes obs2.modified_date > obs1.modified_date; make the ordering deterministic by explicitly controlling timestamps or asserting with a deterministic secondary key: either set distinct modified_date values when creating observations via _create_observation (e.g., update obs2.modified_date to be later and save before the GET) or change the assertion to compare results sorted by (modified_date, id) so the expected order is deterministic using obs1.external_id and obs2.external_id; update the test body around obs1/obs2 and the GET/assertion accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@care/emr/tests/test_observation_api.py`:
- Around line 173-183: The test test_list_ordered_by_modified_date_descending is
brittle because it assumes obs2.modified_date > obs1.modified_date; make the
ordering deterministic by explicitly controlling timestamps or asserting with a
deterministic secondary key: either set distinct modified_date values when
creating observations via _create_observation (e.g., update obs2.modified_date
to be later and save before the GET) or change the assertion to compare results
sorted by (modified_date, id) so the expected order is deterministic using
obs1.external_id and obs2.external_id; update the test body around obs1/obs2 and
the GET/assertion accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c98374ea-c7f9-49f2-b2db-428c82d1398b
📒 Files selected for processing (1)
care/emr/tests/test_observation_api.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3623 +/- ##
===========================================
+ Coverage 77.20% 77.31% +0.11%
===========================================
Files 474 474
Lines 22421 22421
Branches 2348 2348
===========================================
+ Hits 17310 17335 +25
+ Misses 4531 4504 -27
- Partials 580 582 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed Changes
Associated Issue
Architecture changes
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