Filter by name and status in valueset #2867
Conversation
📝 WalkthroughWalkthroughThis pull request enhances the filtering capabilities of the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant V as ValueSetViewSet
participant D as DjangoFilterBackend
participant F as ValueSetFilter
C->>V: Request with filter parameters
V->>D: Pass request for filtering
D->>F: Apply filters (name, status)
F-->>D: Return filtered query set
D-->>V: Provide filtered data
V-->>C: Return response
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)
✨ Finishing Touches
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: 0
🧹 Nitpick comments (1)
care/emr/api/viewsets/valueset.py (1)
20-22:❓ Verification inconclusive
I couldn't help but notice the inconsistent naming.
The class is named
ValuesetFilterwhile the model isValueSet. I'm sure it was just an oversight, but consistency in naming would be lovely.-class ValuesetFilter(filters.FilterSet): +class ValueSetFilter(filters.FilterSet):Also, let's verify that the status filter values are properly documented and validated.
🏁 Script executed:
#!/bin/bash # Search for status field definition in the model and any related documentation rg -l 'status.*=.*' care/emr/models/valueset.pyLength of output: 77
Fix naming inconsistency and verify status field validation
- The filter class is named
ValuesetFilter, but the model is defined asValueSet. For consistency, please rename the filter class toValueSetFilter:-class ValuesetFilter(filters.FilterSet): +class ValueSetFilter(filters.FilterSet):- Although the search confirms that the model file (
care/emr/models/valueset.py) contains a definition for thestatusfield, please double-check that the allowed values for status are well documented and properly validated (e.g., using achoicesattribute or explicit validators).Overall, the changes look good aside from these two points—just a small refinement to keep things consistent and ensure data validation remains robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/valueset.py(2 hunks)
🔇 Additional comments (2)
care/emr/api/viewsets/valueset.py (2)
5-6: LGTM! Nice and tidy imports.The new imports are properly organized with other framework imports. I suppose we can all appreciate when someone takes the time to keep things orderly.
27-28:❓ Verification inconclusive
The filtering setup looks good, but...
The implementation is correct, though I'm sure we'd all feel more confident with some test coverage for these new filters. You know, just to make absolutely certain everything works as intended.
Let's check if there are any test files that could use some filter-related test cases:
🏁 Script executed:
#!/bin/bash # Search for existing test files related to ValueSet fd -g "*test*valueset*.py" -g "*test*value_set*.py"Length of output: 160
Filtering setup is solid, but test coverage appears to be missing
The filtering implementation in
care/emr/api/viewsets/valueset.py(lines 27-28) correctly utilizesValuesetFilterandDjangoFilterBackend, aligning with the PR’s objectives. However, our initial attempt to locate test files related to these filters didn’t produce any results—likely due to the search command’s syntax issues.Could you please verify whether there are existing tests for these changes or consider adding some to ensure the filters behave as expected? For a more reliable file search, you might try running these corrected commands:
#!/bin/bash # Search for test files related to ValueSet using separate glob patterns fd --glob '*test*valueset*.py' fd --glob '*test*value_set*.py'Thanks for addressing this—ensuring proper test coverage will strengthen our confidence in the new functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/api/viewsets/valueset.py (1)
20-22: Filter implementation looks correct, but could use some documentation.The
ValueSetFilterclass is implemented correctly with appropriate filter fields and lookup expressions. However, it wouldn't hurt to add a docstring explaining the purpose of these filters, since not everyone has the luxury of understanding your code at first glance.class ValueSetFilter(filters.FilterSet): + """ + Filter for ValueSet objects. + - name: Filters by case-insensitive partial name match + - status: Filters by exact status match + """ name = filters.CharFilter(field_name="name", lookup_expr="icontains") status = filters.CharFilter(field_name="status", lookup_expr="iexact")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/valueset.py(2 hunks)
🔇 Additional comments (3)
care/emr/api/viewsets/valueset.py (3)
5-6: Good job adding the necessary imports.The new imports for Django filters are correctly added and properly organized with the rest of the imports. It's nice to see someone following import conventions... for once.
27-28: ViewSet configuration looks good.The filter configuration is properly set up in the ViewSet. I suppose we can all agree this is the minimal amount of code required to make it work.
20-28:❓ Verification inconclusive
Consider adding tests for the filtering functionality.
While the implementation looks correct, it would be nice if there were tests to verify that the filters actually work as expected. The PR checklist does mention adding tests, so I assume you're planning to add them... eventually.
🏁 Script executed:
#!/bin/bash # Check if tests exist for ValueSetViewSet filtering echo "Searching for test files related to ValueSet..." fd -t f "test.*valueset.*" --exec grep -l "filter" {} \; echo "Checking if PR includes test file changes..." git diff --name-only origin/main..HEAD | grep -E "test.*\.py$"Length of output: 526
Tests Needed for ValueSet Filtering Functionality
It appears that no tests were found verifying the filtering behavior in
ValueSetViewSet(lines 20–28 incare/emr/api/viewsets/valueset.py). Please add tests to confirm that thename(using"icontains") andstatus(using"iexact") filters work as expected. Given the ambiguous git diff output, kindly double-check your test suite to ensure nothing was overlooked.
|
lint checks failing @AdityaJ2305 make sure you've installed pre-commit btw; |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2867 +/- ##
===========================================
+ Coverage 56.28% 56.31% +0.02%
===========================================
Files 226 226
Lines 10939 10946 +7
Branches 1112 1112
===========================================
+ Hits 6157 6164 +7
Misses 4765 4765
Partials 17 17 ☔ View full report in Codecov by Sentry. |
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