Skip to content

Use icontains as fallback for AD search#3676

Open
gigincg wants to merge 1 commit into
developfrom
improve_ad_search
Open

Use icontains as fallback for AD search#3676
gigincg wants to merge 1 commit into
developfrom
improve_ad_search

Conversation

@gigincg

@gigincg gigincg commented Jun 4, 2026

Copy link
Copy Markdown
Member

Proposed Changes

  • Brief of changes made.

Merge Checklist

  • Tests added/fixed
  • Linting Complete

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

Release Notes

  • Bug Fixes
    • Enhanced activity definition search filtering to support both similarity-based matching and case-insensitive substring searches, allowing users to find definitions more easily using acronyms or partial text matches.

@gigincg gigincg requested a review from a team as a code owner June 4, 2026 10:59
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR expands activity definition filtering to support acronym matching. The TrigramFilter.filter method now uses an OR condition to return results when either trigram similarity exceeds the threshold or the field contains the input substring (case-insensitive), gracefully complemented by a test validating the acronym presence detection.

Changes

Acronym Filtering Enhancement

Layer / File(s) Summary
Filter implementation with Q import
care/emr/api/viewsets/activity_definition.py
Adds Q import and updates TrigramFilter.filter to combine trigram similarity (similarity__gt=0.1) with case-insensitive substring matching (<field_name>__icontains) using OR logic.
Test coverage for acronym filtering
care/emr/tests/test_activity_definition_api.py
New test creates two activity definitions, filters by title=CRP query parameter, and verifies only the record containing the acronym in its title is returned.

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is largely incomplete, missing critical details about what changed and why, despite including test and lint checklist items. Replace placeholder text with actual change details, add associated issue link, and explain how the icontains fallback solves the search problem.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Use icontains as fallback for AD search' directly aligns with the main change: expanding search filtering to include case-insensitive substring matches as a fallback option.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch improve_ad_search

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

@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR improves the TrigramFilter used for activity definition title search by adding an icontains OR condition alongside the existing trigram similarity threshold, ensuring that short acronym-style queries (like "CRP") that score below the 0.1 similarity threshold are still returned as results.

  • activity_definition.py: The .filter(similarity__gt=0.1) is replaced with .filter(Q(similarity__gt=0.1) | Q(**{f"{self.field_name}__icontains": value})), so records matching via case-insensitive substring search are included even when trigram similarity is too low.
  • test_activity_definition_api.py: A new test test_list_activity_definition_with_title_acronym_filter verifies that searching for "CRP" correctly returns an activity definition whose title contains that acronym.

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted OR-condition addition with a direct test covering the new path.

The filter change is two lines, does not touch auth or write paths, and is covered by a new test that specifically exercises the acronym case that motivated the fix. The annotation+order_by(-similarity) contract is preserved for both match types.

No files require special attention.

Important Files Changed

Filename Overview
care/emr/api/viewsets/activity_definition.py Adds an icontains OR branch to TrigramFilter so acronym searches below the 0.1 similarity threshold are still matched; ordering by -similarity remains correct for both match types.
care/emr/tests/test_activity_definition_api.py Adds a focused test for acronym-based title filtering ("CRP"), confirming the icontains fallback returns the expected single result.

Reviews (1): Last reviewed commit: "Use icontains as fallback for AD search" | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
care/emr/tests/test_activity_definition_api.py (1)

1152-1158: ⚡ Quick win

Add a lowercase-query assertion to actually prove case-insensitive fallback.

This test passes, but it currently validates only exact-case "CRP". Please add one more request with "crp" and assert the same result, so __icontains behavior is explicitly locked in rather than assumed.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@care/emr/tests/test_activity_definition_api.py` around lines 1152 - 1158, Add
a second GET in the test to assert case-insensitive matching by querying
self.base_url with {"title": "crp"} and verifying the response status_code is
200, results length is 1, and that response.data["results"][0]["slug"] equals
str(activity_definition.slug); update the test alongside the existing {"title":
"CRP"} call (the relevant symbols are self.client.get, self.base_url, and
activity_definition.slug) so the __icontains fallback is explicitly validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@care/emr/tests/test_activity_definition_api.py`:
- Around line 1152-1158: Add a second GET in the test to assert case-insensitive
matching by querying self.base_url with {"title": "crp"} and verifying the
response status_code is 200, results length is 1, and that
response.data["results"][0]["slug"] equals str(activity_definition.slug); update
the test alongside the existing {"title": "CRP"} call (the relevant symbols are
self.client.get, self.base_url, and activity_definition.slug) so the __icontains
fallback is explicitly validated.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 98585603-2de5-4788-a57f-9df2d49ede3d

📥 Commits

Reviewing files that changed from the base of the PR and between dd4f997 and 1a35a82.

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

@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.67%. Comparing base (dd4f997) to head (1a35a82).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3676   +/-   ##
========================================
  Coverage    76.66%   76.67%           
========================================
  Files          479      479           
  Lines        23055    23056    +1     
  Branches      2385     2385           
========================================
+ Hits         17676    17679    +3     
+ Misses        4824     4823    -1     
+ Partials       555      554    -1     

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant