-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix: searching tag refinement was sometimes not working #34933
fix: searching tag refinement was sometimes not working #34933
Conversation
Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
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.
LGTM @bradenmacdonald! 👍
- I tested this following the testing instructions
- I read through the code and considered the security, stability and performance implications of the changes.
-
I tested that the UI can be used with a keyboard only (tab order, keyboard controls). - Includes tests for bugfixes and/or features added.
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.
👍 This fixes the issue described perfectly, and I've raised another potential issue, see inline comment.
- I tested this on my tutor dev stack using the PR instructions
- I read through the code.
-
I checked for accessibility issuesN/A - Includes code docs to explain the change.
-
User-facing strings are extracted for translationN/A
Fields.tags + "." + Fields.tags_level0, | ||
Fields.tags + "." + Fields.tags_level1, | ||
Fields.tags + "." + Fields.tags_level2, | ||
Fields.tags + "." + Fields.tags_level3, |
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.
Hmm.. I thought we only supported a 3-level tag hierarchy, but this PR uses 4 levels, and I can see 4 levels in ESDC..
Do we need to update openedx-learning too?
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.
Yeah, I've been a bit inconsistent about that. We only officially support 3 levels, though at the core the code will support a higher number of levels - it's just that some search and listing features only work on three (or in this case 4) levels. It would be a good change in the future to drop this fourth level from the search engine and to prevent the import or creation of taxonomies with 4 levels (or support 4 levels). For now I don't think it's a bug we need to fix though.
@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
Fixes openedx/modular-learning#224
It turns out that if no documents on the instance contain any "level 3" tags, then the search which tries to filter the tags using a keyword search (and explicitly includes the
tags.level3
field) would fail. The fix is simple, to always explicitly addtags.levelX
as "searchable fields", rather than assuming they'll be implicitly searchable because the parenttags
field is included.Supporting information
Testing instructions
To reproduce the bug, before checking out this branch, go to this line:
edx-platform/openedx/core/djangoapps/content/search/documents.py
Line 205 in 7f7cade
and change
4
to3
. This simulates "no documents have level 3 tags". Then runpython manage.py cms reindex_studio --experimental
.Then open the search UI in course authoring and enter a keyword into the tags filter. It should have no effect.
Then checkout this branch, run the reindex command again, and see that the fix worked.
Deadline
We want to get this backported to Redwood ASAP.
Private Ref: MNG-4283