feat: Enable taxonomy/tagging feature in MFE by default #34633

rpenido · 2024-04-25T23:00:14Z

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.

Testing Instructions

Delete the following Waffle flags using the Django admin:

new_studio_mfe.use_tagging_taxonomy_list_page
content_tagging.auto

Check if the tagging feature is enabled in the course authoring MFE:

In the course authoring home page, you should see the Taxonomy tab (http://apps.local.edly.io:2001/course-authoring/home)
In the Course Outline, you should see a "Manage Tags" link in the Course Header and the Section/Subsection menus.
In the Unit Outline, you should see the Tags card in the sidebar

Create a Waffle flag with name content_tagging.disabled and set it to True to everyone
Check if the taxonomy feature is disabled in the places above

Private ref: FAL-3708

openedx-webhooks · 2024-04-25T23:00:18Z

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:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

yusuf-musleh

👍 @rpenido Great work, looks good to me!

I tested this: I followed the instructions in the PR
I read through the code
~~I checked for accessibility issues~~
Includes documentation

yusuf-musleh · 2024-04-26T10:34:38Z

openedx/core/djangoapps/content_tagging/migrations/0009_enable_tagging_as_default.py

+
+def create_flags(apps, schema_editor):


nit: add more more blank line/space between the imports and function def. Not sure why the linter in the CI didn't catch it.

Thank you for noticing this! Fixed here: 720ce0b30fadc2202a43030ee3be279e0d090204

bradenmacdonald · 2024-04-26T18:56:44Z

@rpenido Do we really need a migration for this? Can't we just change the default in the code?

rpenido · 2024-04-26T19:00:01Z

@rpenido Do we really need a migration for this? Can't we just change the default in the code?

I will check/test it, but it makes sense to work only to changing the default.

rpenido · 2024-04-29T11:54:26Z

Hi @bradenmacdonald ! From my research, it is not possible to do it easily without a migration. We have the WAFFLE_FLAG_DEFAULT, but I didn't find any way to do it in a per-flag way.

Also, we use migrations to set defaults in our codebase:

edx-platform/lms/djangoapps/grades/migrations/0018_add_waffle_flag_defaults.py

Line 14 in 45cd459

name=REJECTED_EXAM_OVERRIDES_GRADE.name, defaults={'everyone': True}
edx-platform/cms/djangoapps/contentstore/migrations/0005_add_enable_checklists_quality_waffle_flag.py

Line 9 in 45cd459

Flag.objects.get_or_create(name=ENABLE_CHECKLISTS_QUALITY.name, defaults={'everyone': True})

Let me know what you think!

bradenmacdonald · 2024-04-29T17:28:00Z

@rpenido OK, I guess a migration is fine. BTW This might be a good time to rename the flag to enable_tagging_feature which is much more clear than enable_tagging_taxonomies_list_page. We want it to control both the "Taxonomies" page in Studio and the "Manage Tags" option on the outline/unit pages.

Also, I'm not sure if the "Automatic tagging" has been tested well enough to turn it on by default yet - let me ask about that.

bradenmacdonald · 2024-04-29T17:34:30Z

@rpenido Yeah, I just checked with @jmakowski1123 and since the "automatic tagging" hasn't been tested enough (and probably won't be), we want to leave it off by default.

So, if you want, one option is to replace enable_tagging_taxonomies_list_page with a new flag called disable_tagging_feature that defaults to False - then no migration is required. I think that would be my preference. But you can also keep the current flag and make it enabled with a migration.

rpenido · 2024-04-29T18:37:34Z

So, if you want, one option is to replace enable_tagging_taxonomies_list_page with a new flag called disable_tagging_feature that defaults to False - then no migration is required. I think that would be my preference.

Sure @bradenmacdonald! I will make this change and let you know when it's ready to review again!

But in that case, I think we would still want a migration to clean the old flag from database as it will not be used anymore and may cause confusion. Or you think this is not necessary?

bradenmacdonald · 2024-04-30T19:38:26Z

@rpenido That's not necessary - the waffle flag has never been included in a release version, so it's likely only a few of us developers that have it.

rpenido · 2024-05-01T10:27:36Z

cms/djangoapps/contentstore/rest_api/v1/views/tests/test_course_index.py

@@ -150,6 +150,6 @@ def test_number_of_calls_to_db(self):
        """
        Test to check number of queries made to mysql and mongo
        """
-        with self.assertNumQueries(29, table_ignorelist=WAFFLE_TABLES):
+        with self.assertNumQueries(31, table_ignorelist=WAFFLE_TABLES):


These are from the tag count queries

Now +3: (1 for the course as a whole, 1 for all the blocks in the course, and 1 for getting the org-specific value of COURSE_AUTHORING_MICROFRONTEND_URL (within get_course_authoring_url within get_taxonomy_tags_widget_url within _get_course_index_context).

rpenido · 2024-05-01T10:29:26Z

cms/djangoapps/contentstore/views/tests/test_course_index.py

@@ -717,7 +717,7 @@ def test_number_of_calls_to_db(self):
        """
        Test to check number of queries made to mysql and mongo
        """
-        with self.assertNumQueries(26, table_ignorelist=WAFFLE_TABLES):
+        with self.assertNumQueries(28, table_ignorelist=WAFFLE_TABLES):


These are from the tag count queries

cms/static/js/views/xblock_outline.js

yusuf-musleh

@rpenido Thanks for your work on this! It's almost there, I've gone through the testing steps for this and noticed a few issues:

After deleting the tags mentioned in the testing instructions, by default the manage tags and taxonomies tabs/buttons appear as expected. However, when I try the new content_tagging.disabled flag, it only hides the Taxonomies tab in the course authoring mfe, however the "manage tags" buttons still appear. I left a comment in the mfe PR.

Also in legacy studio UI, the manage tags no longer appears regardless of the flag being set or not, that is because there is some missing code needed below.

cms/static/js/views/xblock_outline.js

cms/templates/studio_xblock_wrapper.html

bradenmacdonald · 2024-05-06T18:11:09Z

Great review @yusuf-musleh - thanks :)

bradenmacdonald · 2024-05-08T02:04:42Z

@yusuf-musleh Can you please take another look at this? It should be ready to go now.

yusuf-musleh

👍 @bradenmacdonald Looks good to me, its working well now!

I tested this: I followed the testing instructions in the PR
I read through the code
~~I checked for accessibility issues~~
~~includes documentation~~

bradenmacdonald · 2024-05-08T17:00:28Z

I tested this and it should be safe to merge before the associated course-authoring PR. Tagging features will still be disabled on edx.org. I already asked them to proactively set the content_tagging.disabled flag on their instance, which they did.

ormsbee

Minor nit in that I think it's more clear to always have the function be is_xxxxxx_enabled() and then have it do the right thing when switching waffle flags. But I certainly won't hold this up over that.

…ure-in-mfe-by-default' into taxonomy-sandbox-20240508

…/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default

openedx-webhooks · 2024-05-09T13:27:08Z

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2024-05-09T14:49:55Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-05-09T15:15:09Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-05-09T15:28:13Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2024-05-09T15:48:20Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-05-09T15:48:20Z

2U Release Notice: This PR has been deployed to the edX production environment.

* feat: make tagging feature enabled by default * fix: use the correct flag for tagging enabled * fix: make compatible with other changes from master * fix: more compatibility fixes * fix: show tag counts at all levels of the outline, not just units * chore: typo * test: fix counts in test suite now that tagging is on by default --------- Co-authored-by: Braden MacDonald <[email protected]> Co-authored-by: Yusuf Musleh <[email protected]>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 25, 2024

rpenido requested a review from yusuf-musleh April 25, 2024 23:07

yusuf-musleh approved these changes Apr 26, 2024

View reviewed changes

rpenido marked this pull request as ready for review April 29, 2024 11:57

rpenido force-pushed the rpenido/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default branch 4 times, most recently from f581786 to 0d5fe4f Compare April 30, 2024 19:22

rpenido force-pushed the rpenido/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default branch 2 times, most recently from 3be828e to 014295c Compare April 30, 2024 20:31

rpenido marked this pull request as draft April 30, 2024 20:48

rpenido commented May 1, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default branch 3 times, most recently from 8fe6de5 to e5640c1 Compare May 1, 2024 10:46

rpenido mentioned this pull request May 1, 2024

feat: Enable taxonomy/tagging feature in MFE by default openedx/frontend-app-authoring#971

Closed

rpenido requested a review from yusuf-musleh May 1, 2024 10:51

rpenido marked this pull request as ready for review May 1, 2024 10:51

rpenido commented May 1, 2024

View reviewed changes

cms/static/js/views/xblock_outline.js Show resolved Hide resolved

feat: make tagging feature enabled by default

b272763

rpenido force-pushed the rpenido/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default branch from e5640c1 to b272763 Compare May 1, 2024 10:53

yusuf-musleh suggested changes May 2, 2024

View reviewed changes

cms/static/js/views/xblock_outline.js Show resolved Hide resolved

cms/templates/studio_xblock_wrapper.html Outdated Show resolved Hide resolved

fix: use the correct flag for tagging enabled

0f57287

bradenmacdonald added 6 commits May 7, 2024 13:34

chore: update with latest master

8c2ef4e

fix: make compatible with other changes from master

0ab435a

fix: more compatibility fixes

a585591

fix: show tag counts at all levels of the outline, not just units

cc9cec5

chore: typo

97d31a9

test: fix counts in test suite now that tagging is on by default

b131866

yusuf-musleh approved these changes May 8, 2024

View reviewed changes

bradenmacdonald approved these changes May 8, 2024

View reviewed changes

bradenmacdonald mentioned this pull request May 8, 2024

feat: Enable taxonomy/tagging feature in MFE by default openedx/frontend-app-authoring#989

Merged

ormsbee approved these changes May 8, 2024

View reviewed changes

bradenmacdonald added a commit to open-craft/edx-platform that referenced this pull request May 8, 2024

Merge PR openedx#34633 'rpenido/fal-3708-enable-taxonomy-tagging-feat…

55369e7

…ure-in-mfe-by-default' into taxonomy-sandbox-20240508

Merge branch 'master' of github.com:openedx/edx-platform into rpenido…

737e292

…/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default

farhaanbukhsh merged commit b42da74 into openedx:master May 9, 2024
80 checks passed

farhaanbukhsh deleted the rpenido/fal-3708-enable-taxonomy-tagging-feature-in-mfe-by-default branch May 9, 2024 13:27

bradenmacdonald mentioned this pull request May 9, 2024

[Tagging] Enable taxonomy/tagging feature in MFE by default openedx/modular-learning#207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable taxonomy/tagging feature in MFE by default #34633

feat: Enable taxonomy/tagging feature in MFE by default #34633

rpenido commented Apr 25, 2024 •

edited by bradenmacdonald

Loading

openedx-webhooks commented Apr 25, 2024 •

edited

Loading

yusuf-musleh left a comment

yusuf-musleh Apr 26, 2024

rpenido Apr 29, 2024

bradenmacdonald commented Apr 26, 2024

rpenido commented Apr 26, 2024

rpenido commented Apr 29, 2024

bradenmacdonald commented Apr 29, 2024

bradenmacdonald commented Apr 29, 2024

rpenido commented Apr 29, 2024 •

edited

Loading

bradenmacdonald commented Apr 30, 2024

rpenido May 1, 2024

bradenmacdonald May 8, 2024

rpenido May 1, 2024

yusuf-musleh left a comment

bradenmacdonald commented May 6, 2024

bradenmacdonald commented May 8, 2024

yusuf-musleh left a comment

bradenmacdonald commented May 8, 2024

ormsbee left a comment

openedx-webhooks commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

feat: Enable taxonomy/tagging feature in MFE by default #34633

feat: Enable taxonomy/tagging feature in MFE by default #34633

Conversation

rpenido commented Apr 25, 2024 • edited by bradenmacdonald Loading

Description

Testing Instructions

openedx-webhooks commented Apr 25, 2024 • edited Loading

yusuf-musleh left a comment

Choose a reason for hiding this comment

yusuf-musleh Apr 26, 2024

Choose a reason for hiding this comment

rpenido Apr 29, 2024

Choose a reason for hiding this comment

bradenmacdonald commented Apr 26, 2024

rpenido commented Apr 26, 2024

rpenido commented Apr 29, 2024

bradenmacdonald commented Apr 29, 2024

bradenmacdonald commented Apr 29, 2024

rpenido commented Apr 29, 2024 • edited Loading

bradenmacdonald commented Apr 30, 2024

rpenido May 1, 2024

Choose a reason for hiding this comment

bradenmacdonald May 8, 2024

Choose a reason for hiding this comment

rpenido May 1, 2024

Choose a reason for hiding this comment

yusuf-musleh left a comment

Choose a reason for hiding this comment

bradenmacdonald commented May 6, 2024

bradenmacdonald commented May 8, 2024

yusuf-musleh left a comment

Choose a reason for hiding this comment

bradenmacdonald commented May 8, 2024

ormsbee left a comment

Choose a reason for hiding this comment

openedx-webhooks commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

edx-pipeline-bot commented May 9, 2024

rpenido commented Apr 25, 2024 •

edited by bradenmacdonald

Loading

openedx-webhooks commented Apr 25, 2024 •

edited

Loading

rpenido commented Apr 29, 2024 •

edited

Loading