-
Notifications
You must be signed in to change notification settings - Fork 89
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 #971
feat: Enable taxonomy/tagging feature in MFE by default #971
Conversation
Thanks for the pull request, @rpenido! 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. |
…ABLE_TAGGING_FEATURE
5a4da02
to
5fbfe66
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #971 +/- ##
=======================================
Coverage 92.21% 92.22%
=======================================
Files 703 703
Lines 12361 12382 +21
Branches 2682 2649 -33
=======================================
+ Hits 11399 11419 +20
- Misses 926 927 +1
Partials 36 36 ☔ View full report in Codecov by Sentry. |
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.
@rpenido Great work on this it's on the right path, just faced a few issues and have some suggestions below.
The "Manage tags" and tag counts are always present, regardless of whether the new waffle flag is set or not, see my comment below. The taxonomies tab however appears/disappears as expected.
src/index.jsx
Outdated
@@ -121,7 +121,7 @@ initialize({ | |||
ENABLE_UNIT_PAGE: process.env.ENABLE_UNIT_PAGE || 'false', | |||
ENABLE_ASSETS_PAGE: process.env.ENABLE_ASSETS_PAGE || 'false', | |||
ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN: process.env.ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN || 'false', | |||
ENABLE_TAGGING_TAXONOMY_PAGES: process.env.ENABLE_TAGGING_TAXONOMY_PAGES || 'false', | |||
DISABLE_TAGGING_FEATURE: process.env.DISABLE_TAGGING_FEATURE || 'false', |
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 always seems to be falling back to false
since the env variable is not being set, regardless of whether the flags is set in the edx-platform or not. So the "Manage Tags" and tags counts always show regardless if the new waffle flag is set or not.
The previous flag has the following env variables set:
Since it currently doens't seem to be reading it from the edx-platform backend, I'm not sure what the general practice is for this, however maybe we could utilize the taxonomiesEnabled
flag that is passed in from the backend:
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.
Hi @yusuf-musleh!
I think getting the data from the backend is the right approach, but we need to fetch a lot of data to use this selector.
I created a new hook (using react-query) that solves that problem. The only issue is that we need this info in the index.jsx, where we don't have the necessary configs just to call a hook.
We may need to use some kind of "lazy load" for the routes. Maybe @bradenmacdonald could help us with some ideas?
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.
hey guys, I don't know if there is a good pattern for this now. I think we should just stick to the simplest option and have an environment variable that disables the MFE features (getConfig().DISABLE_TAGGING_FEATURE
) and a waffle flag that controls the legacy UI (content_tagging.disabled
).
The waffle flag is already set on edx.org, in anticipation of this PR, but we'll also have to get them to set the env var once it's confirmed.
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 like @rpenido's approach and I think it's even better if we can get it to work. Which index.jsx
is the problem? The studio home page or the taxonomies page?
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 root index, where we declare the <App>
.
One "easy" approach to avoid this problem (needing a provider at the App level) is to move the check to the destination pages, showing an error message or redirecting to another place if the flag is disabled. I think this is not the best solution, but it will work.
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 fine. We want most people using this, and having it disabled will be fairly rare. The important thing is that reading this flag doesn't slow down the frontend, so shouldn't introduce an additional blocking API request on every page.
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.
One "easy" approach to avoid this problem (needing a provider at the App level) is to move the check to the destination pages, showing an error message or redirecting to another place if the flag is disabled.
So let's go with that.
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.
OK, I implemented that ^
@yusuf-musleh could you please review/test this PR again?
ed8986d
to
914c240
Compare
914c240
to
c0404ae
Compare
9ddeac2
to
e17324c
Compare
e17324c
to
29daee5
Compare
@yusuf-musleh I have finished this up and it's ready for review again |
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 I tested it out, its working well. There was just the unit side bar tags that was missing the check if tagging was enabled/disabled, but I went ahead
and added it here: 5d37761
- I tested this: Followed the testing instructions in the edx-platform PR
- I read through the code
-
I checked for accessibility issues - Includes documentation
Thinking about this more, I'm worried about the performance implications of loading the studio home data on every page since it's consumed by the updated header. I'm going to suggest we keep it simple and use an env var instead: #989 |
@rpenido Even though your pull request wasn’t 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 and creates a new waffle flag, content_tagging.disabled, to allow the feature to be turned off.
More Information
Testing Instructions
See openedx/edx-platform#34633