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
Add resource search for autocomplete #37224
Conversation
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.
nice progress, Bethany!
@@ -0,0 +1,12 @@ | |||
class ResourcesAutocomplete < AutocompleteHelper |
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.
nice find on AutocompleteHelper!
# We rely on fulltext indices to be used in this test. | ||
# In order to get this test to work, we need to have the resources created | ||
# as fixtures. These are defined in test/fixtures/resource.yml |
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.
thanks for documenting. this strategy seems fine for now, though I'm a bit worried it will no longer work once we are only searching within a specific course version.
|
||
rows = Resource.limit(limit) | ||
query = format_query(query) | ||
return [] if query.length < MIN_WORD_LENGTH + 2 |
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.
Why + 2?
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 query adds a symbol on each side of the word. I'll add 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.
Actually, I'm going to move it up a line and not bother with the +2 thing
# In order to get this test to work, we need to have the resources created | ||
# as fixtures. These are defined in test/fixtures/resource.yml | ||
|
||
test "finds resource with matching name" do |
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.
Maybe add a test for when there is less than 5 characters in the search?
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.
So cool to see this come together. I left a question and an idea for tests but I will leave it up to you if you think the test is helpful
Follow up to #37118, which added the front end search calls. This actually implements the autocomplete search part, on name and URL.
I had a fair number of issues with the test for this. Adding a fixture that is seeded into the test database solved those. I think the issue was that this feature relies on an index and the tests just weren't creating those fast enough, but that's just a theory.
Links
Testing story
unit test in addition to manual testing
Reviewer Checklist: