Facility Users filter out deleted users #2638
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a filter condition to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
care/facility/tests/test_facilityuser_api.py (2)
70-70: That comment seems a bit... implementation-specificThe comment
Testing FE's delete functionalitycouples the test to frontend implementation details. Perhaps we could make it more generic?- # Testing FE's delete functionality (soft delete/is_active is set to false when user is deleted) + # Verify that soft-deleted users are excluded from facility user list
69-90: The test coverage could be a tiny bit more comprehensiveWhile the test verifies the
is_activeflag behavior, it doesn't cover thedeletedflag or edge cases like:
- User with
is_active=Truebutdeleted=True- User with
is_active=Falsebutdeleted=FalseConsider adding these test cases to ensure robust behavior:
def test_user_is_not_listed_if_marked_deleted(self): self.user.deleted = True self.user.save() response = self.client.get( f"/api/v1/facility/{self.facility.external_id}/get_users/" ) self.assertEqual(len(response.json()["results"]), 1) self.assertNotIn( self.user.username, [user["username"] for user in response.json()["results"]] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/viewsets/facility_users.py(1 hunks)care/facility/tests/test_facilityuser_api.py(1 hunks)
🔇 Additional comments (1)
care/facility/api/viewsets/facility_users.py (1)
42-43: I couldn't help but notice we're using two deletion flags...
The code uses both is_active=True and deleted=False filters. While this works, it seems a bit... redundant? Django's is_active flag is typically sufficient for soft deletion.
Let's check if we're consistently using both flags across the codebase:
Consider standardizing on one deletion mechanism across the codebase. Django's built-in is_active flag might be more idiomatic.
✅ Verification successful
Actually, both flags serve distinct purposes in this codebase...
The deleted and is_active flags are used for different scenarios:
deletedis part of a custom soft-deletion mechanism used consistently across models (seecare/utils/models/base.py)is_activeis specifically used for patient state management:- Set to
Falseduring discharge (seepatient_consultation.py) - Set to
Truewhen patient is active in the facility - Used in patient-related queries and summaries
- Set to
So while it might seem redundant at first glance, they're not quite the same thing. The current implementation in facility_users.py is actually correct, if not entirely... elegant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns of using is_active and deleted flags together
rg -A 2 "is_active.*deleted|deleted.*is_active" --type py
# Search for deleted flag usage
rg "deleted\s*=\s*(True|False)" --type py
# Search for is_active flag usage
rg "is_active\s*=\s*(True|False)" --type py
Length of output: 9597
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2638 +/- ##
========================================
Coverage 69.63% 69.63%
========================================
Files 211 211
Lines 11879 11879
Branches 1202 1202
========================================
Hits 8272 8272
Misses 3240 3240
Partials 367 367 ☔ View full report in Codecov by Sentry. |
Proposed Changes
is_activeflag to be false, which we aren't filtering out in FacilityUsers, resulting in deleted users being displayed in Facility Users page, Doctors Connect and LinkedFacilityUsers (used in Shift update).Associated Issue
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