Facility Users Username Filter #2719
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new filtering mechanism for facility users in the EMR (Electronic Medical Record) system. A custom filter class Changes
Poem
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 (
|
![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 (1)
care/emr/api/viewsets/facility.py (1)
110-111: Would it kill you to add a docstring? 😊The
FacilityUserFilterimplementation looks good, but it could use some documentation to explain its purpose and usage.Consider adding a docstring:
class FacilityUserFilter(FilterSet): + """Filter set for facility users with case-insensitive username filtering.""" username = CharFilter(field_name="username", lookup_expr="icontains")🧰 Tools
🪛 GitHub Actions: Lint Code Base
[warning] 107-111: Code formatting issues detected. File was automatically reformatted by ruff-format to fix spacing/formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/facility.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint Code Base
care/emr/api/viewsets/facility.py
[warning] 107-111: Code formatting issues detected. File was automatically reformatted by ruff-format to fix spacing/formatting.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (1)
care/emr/api/viewsets/facility.py (1)
110-117: Verify consistent filtering implementation across related viewsetsLet's ensure this filtering pattern is consistently implemented across related user viewsets.
✅ Verification successful
Your filtering implementation is fine, though you could make it even better
The username filtering implementation in
FacilityUsersViewSetis consistent with Django best practices and similar viewsets in the codebase. While we're at it, you might want to consider adding the same filtering capability toUserViewSet– you know, for consistency's sake. But that's entirely up to you, of course.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other viewsets that might benefit from similar username filtering ast-grep --pattern 'class $_(ViewSet): $$$ database_model = User $$$'Length of output: 77
Script:
#!/bin/bash # Search for viewsets with User model and filtering rg -l "class.*ViewSet.*:|database_model = User" | xargs rg -l "filter" - # Detailed look at the implementations rg -B2 -A5 "class.*ViewSet.*:|database_model = User" --context-separator="---"Length of output: 27629
| filterset_class = FacilityUserFilter | ||
| filter_backends = [DjangoFilterBackend] |
There was a problem hiding this comment.
💡 Codebase verification
Oh, would you look at that... no test coverage for filters.
While the implementation is technically correct, it seems we're living dangerously without any tests for the FacilityUserFilter functionality. It would be absolutely wonderful if we could add some test cases to ensure this keeps working as intended.
Suggested test cases should cover:
- Basic filter functionality
- Edge cases for each filterable field
- Filter combination scenarios
🔗 Analysis chain
The implementation looks perfect... almost.
The filter configuration is correctly implemented, but I couldn't help but notice there's no test coverage for the new filtering functionality.
Let's check if there are any test files for this viewset:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to facility viewsets
fd -g "*test*facility*.py" -g "*test*user*.py"
Length of output: 150
Script:
#!/bin/bash
# Search for test files with correct fd syntax
fd -e py -g "*test*" | grep -iE 'facility|user'
# Search for test cases related to FacilityUserFilter
rg -l "class.*Test.*Facility.*User|FacilityUserFilter" -g "*.py"
# Search for actual filter test implementations
rg "def test.*facility.*user.*filter" -g "*.py" -i
# Find the filter class definition to understand what's being tested
ast-grep --pattern 'class FacilityUserFilter'
Length of output: 819
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2719 +/- ##
===========================================
+ Coverage 45.99% 46.00% +0.01%
===========================================
Files 313 313
Lines 17471 17475 +4
Branches 1679 1679
===========================================
+ Hits 8036 8040 +4
Misses 9375 9375
Partials 60 60 ☔ View full report in Codecov by Sentry. |
Proposed 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