-
Notifications
You must be signed in to change notification settings - Fork 2
Display matched search term on results page #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@rhodges @pollardld again, review isn't blocking but feedback is always appreciated! ⭐ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the search functionality to display which specific field matched the search query. The main change involves renaming getResults to get_results to follow Python naming conventions, and implementing headline generation that shows users exactly where their search term was found in the results.
- Renamed
getResultstoget_resultsfor consistent snake_case naming - Added headline generation to show matched fields with highlighted search terms
- Implemented helper functions to determine which attribute has the greatest similarity match
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| TEKDB/explore/views.py | Renamed function to snake_case, added helper functions for match attribute detection, field name resolution, and headline generation |
| TEKDB/explore/templates/results.html | Added new "Searched Result" column to display headlines, updated data bindings with CSS classes |
| TEKDB/explore/static/explore/css/results.css | Added styling for headline paragraph and highlight class |
| TEKDB/TEKDB/tests/test_models.py | Added comprehensive tests for new helper functions, updated function references, added test for resource search |
| TEKDB/TEKDB/models.py | Modified search to generate match and headline annotations for each field, restructured annotation building |
Comments suppressed due to low confidence (1)
TEKDB/TEKDB/tests/test_models.py:300
- assertTrue(a == b) cannot provide an informative message. Using assertEqual(a, b) instead will give more informative messages.
self.assertTrue(len(unique_attributes) == pks[pk])
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current_model = field.related_model | ||
|
|
||
| return field.verbose_name.title() if field else field_path.replace("_", " ").title() | ||
|
|
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line before function definition. PEP 8 requires two blank lines before top-level function definitions. Add an extra blank line before this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passes ruff check and format so 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot is a nit picker! I take issue with the phrase 'PEP 8 requires'. S'cool to follow PEP 8, but Python doesn't bust your butt about this stuff.
| .distinct() | ||
| .order_by("pk", "-rank", "-similarity", sort_field) | ||
| # adding pk to distinct to prevent duplicate ids | ||
| .distinct("pk") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added pk as an arg for distinct to prevent multiple of the same entry returning in the search results. The addition of **annotations in model.objects.annotate resulted multiple of the same entry returning in the search results because the annotations would have some differences. Without this change the PlacesTest.test_search_foreign_key_field failed. @rhodges @pollardld Any objections to this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm not concerned about the change in ordering here because in get_results we return
sorted(
resultlist, key=lambda res: (res["rank"], res["similarity"]), reverse=True
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any reason to object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason "-rank" isn't the first item in order_by? Do I misremember sort ordering as from R -> L?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No objections to .distinct("pk") though, that makes total sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bummer, because as I read it, given the following search results (made up) given the search term 'cheese':
{
name: 'cojack cheese',
rank: 2,
similarity: 0.7,
pk: 1,
},
{
name: 'cheese',
rank: 1,
similarity: 1,
pk: 2,
},
{
name: 'colby cheese',
rank: 3,
similarity: 0.6,
pk: 3,
},
This should return:
- cheese
- cojack cheese
- colby cheese
But since "pk" is the first order by, the result order will be:
- cojack cheese
- cheese
- colby cheese
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have testing in place to ensure that the results come back in the correct order? I might be totally missing something (like by 'distinct' on "pk" removes it from consideration in ordering somehow). It's also possible these results get re-sorted again later in the code making this order_by moot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was kinda treating this order_by as not a big deal because we have this line that sorts by rank and similarity, which is ultimately what the user sees. Does that suffice to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
That means either:
- Cool! Leave it as is. I'm happy with it.
- You may wish to remove the other arguments (just
order_by("pk")) since they'll be sorted later: why spend the compute power to sort them twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave it as is for now 👍🏻 thanks for the feedback!
| self.assertEqual(tribe_fk_search.count(), 13) | ||
| self.assertTrue(25 in [x.pk for x in tribe_fk_search]) | ||
|
|
||
| def test_search_model_set_reference(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
broke out assertions into their own tests
| % query_value | ||
| ) | ||
|
|
||
| resultlist = getResults(query_string, categories) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was bugging me that this was camel case not snake case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more you know... https://peps.python.org/pep-0008/#function-and-variable-names
I am a 'case criminal'. I need to work on this.
rhodges
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good stuff! Just fix the ordering (or help me understand why the 'pk' is okay in that tuple and specifically in that position) and it'll be there.
| .distinct() | ||
| .order_by("pk", "-rank", "-similarity", sort_field) | ||
| # adding pk to distinct to prevent duplicate ids | ||
| .distinct("pk") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading this, "pk" definitely either doesn't belong in the sort or needs to be at the end, otherwise your results will always be in order of 'oldest-first' (by record type)
| % query_value | ||
| ) | ||
|
|
||
| resultlist = getResults(query_string, categories) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more you know... https://peps.python.org/pep-0008/#function-and-variable-names
I am a 'case criminal'. I need to work on this.
| current_model = field.related_model | ||
|
|
||
| return field.verbose_name.title() if field else field_path.replace("_", " ").title() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot is a nit picker! I take issue with the phrase 'PEP 8 requires'. S'cool to follow PEP 8, but Python doesn't bust your butt about this stuff.
rhodges
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paigewilliams explained the reason my concerns were unfounded. I'm sold on this now.
asana task
Description
Adds a column to the search page for the team that had the highest similarity ranking. Also includes the field verbose name in the column values, such as "Alt Name: Filbert".
I used the SearchHeadline class from the Django full text search library. Suuuper helpful.
I thought about adding the column headers back, but I thought that having the verbose name in the column value helped give enough context into what the cell value meant. Open to feedback on this!
Tested downloading still works. ✅
Screenshots