-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: Enable taxonomy/tagging feature in MFE by default #989
feat: Enable taxonomy/tagging feature in MFE by default #989
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #989 +/- ##
=======================================
Coverage 92.21% 92.21%
=======================================
Files 703 703
Lines 12361 12363 +2
Branches 2682 2680 -2
=======================================
+ Hits 11399 11401 +2
Misses 926 926
Partials 36 36 ☔ View full report in Codecov by Sentry. |
a445620
to
42a7e7d
Compare
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.
@bradenmacdonald Looks good 👍
- I tested this: I testest the default value, and I testes disabling the value.
- I read through the code and considered the security, stability and performance implications of the changes.
- Includes tests for bugfixes and/or features added.
- Includes documentation
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.
Looks good. Always makes me very happy when something is well-tested like here.
Thanks @jesperhodge! Would you mind merging this once you're confirmed it's overridden to |
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.
Looks great! Thanks!
@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. |
Description
This PR sets the tagging feature as enabled by default. The
ENABLE_TAGGING_TAXONOMY_PAGES
environment variable (along with thecontent_tagging.disabled
waffle flag) can still be used to turn it off when necessary.Supporting information
Relates to openedx/edx-platform#34633
Replaces #971 as I'm worried about the performance implications of that one; this one is a dead simple env var, rather than loading waffle flag data from the LMS.
Testing instructions
Override the
ENABLE_TAGGING_TAXONOMY_PAGES
variable to false, and make sure no tagging functionality appears in the MFE.Other information
Before merging: make sure 2U sets the env var to false on edx.org because they aren't ready to launch this feature yet.
But otherwise, we want it on by default for developers and all users of the Redwood release.