feat: add language auto-tagging #32907

rpenido · 2023-08-04T00:52:52Z

Description

This PR adds an auto-tagging feature (language) for Courses and XBlocks.

Every time a Course/XBlock is updated, if there is no Language ObjectTag associated with this Course/XBlock, a new one is created using the Course Language as a reference.

Supporting information:

Closes [Tagging] Automatically add system tags for Language (to courses and course content) modular-learning#79

More Info

Depends on:

Testing instructions

Check the unit tests in test_taks.py.
Run the unit tests

Manual check can be done in the CMS App:

Start lms and studio using make dev.up.lms+studio+frontend-app-learning
Enabled the CONTENT_TAGGING_AUTO CourseWaffleFlag
Open studio in http://localhost:18010/ and login with [email protected] / edx
Create a course
Create a course
Open cms python shell (./in cms python manage.py cms shell) and and check the created ObjectTags:

from openedx_tagging.core.tagging.models import ObjectTag
object_tags = ObjectTag.objects.all()
object_tags

Private-ref: FAL-3460

openedx-webhooks · 2023-08-04T00:52:56Z

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.

openedx/features/content_tagging/tasks.py

openedx/features/content_tagging/fixtures/system_defined.yaml

…ystem-taxonomies

rpenido · 2023-08-24T13:08:42Z

Hi @ChrisChV! Could you give me a light here?

After the auto-tagging, this test is failing.

FAILED xmodule/modulestore/tests/test_mixed_modulestore.py::TestMixedModuleStore::test_delete_draft_vertical_1___mongo___0__3__1_ - AssertionError: 1 != 0 : 1 queries executed, 0 expected
Captured queries were:
1. DELETE FROM "oel_tagging_objecttag" WHERE "oel_tagging_objecttag"."object_id" = 'i4x://MITx/999/html/bug_leaf'

When we delete an XBlock, we also delete the ObjectTag associated with it. The problem here is that this test counts the queries against the DB, and we have one more here because of the delete.

edx-platform/xmodule/modulestore/tests/test_mixed_modulestore.py

Lines 1195 to 1196 in 63a0808

    
           with check_mongo_calls(max_find, max_send), self.assertNumQueries(num_mysql): 
        
               self.store.delete_item(private_leaf.location, self.user_id)

Whats is the best approach to make this maintainable in the long term? Just update the query count? Mock the handler?

bradenmacdonald · 2023-08-24T16:36:30Z

@rpenido You can just update the query count. That's what these tests are for - making you aware that the new functionality is having a performance impact on the deletion of XBlocks in general. However, a single additional query is totally fine, especially if it's a very simple and fast query like deleting one row by ID.

…ystem-taxonomies

This reverts commit 3021492.

rpenido · 2023-08-30T19:43:22Z

Feature flag done: 5200966

bradenmacdonald · 2023-08-30T20:18:56Z

openedx/features/content_tagging/toggles.py

+# .. toggle_use_cases: open_edx
+# .. toggle_creation_date: 2023-08-30
+# .. toggle_tickets: https://github.com/openedx/modular-learning/issues/79
+CONTENT_TAGGING_AUTO = WaffleSwitch('content_tagging.auto', __name__)


Maybe we should use CourseWaffleFlag so that we can turn this on on a per-course basis? What do you think?

Will this feature be enabled by default sometime in the future? Or do you see it always as an opt-in?
Wouldn't it be a hassle to make the user enable it every time he creates a course?

I don't know the product well enough to find the right balance between features and usability here. I'll happily follow your call!

Yes, in the future we'll enable it by default and remove this setting. Making it a course waffle flag doesn't mean it's only enabled on a course-by-course basis; you can still turn it on for the whole instance, on our sandbox for example. It just means that during the testing phase you may choose to only turn it on for certain courses.

Great! I will look at the documentation tomorrow and change it to a CourseWaffleFlag if I don't see any problem.

Thank you for your input!

Done 154a4ae!

…ystem-taxonomies

rpenido · 2023-08-31T14:46:50Z

Hi @bradenmacdonald! I think this is ready for another review round!

ChrisChV

@rpenido Looks good to me 👍 Only one comment about the tests. Also, for the record, you need to update the testing instructions with "enable the waffle flag" to check the functionality.

I tested this by:
- Checking and running the tests
- Run and revert migrations
- Following the testing instructions to check auto tagging functionality
I read through the code.
Includes documentation -- docstrings

ChrisChV · 2023-08-31T15:53:25Z

openedx/features/content_tagging/tests/test_tasks.py

+LANGUAGE_TAXONOMY_ID = -1
+
+
+@skip_unless_cms  # Auto-tagging is only available in the CMS


@rpenido This tests are skipped on the CI action, maybe you need to delete this line?

And I think is happend the same with test_views. (CI action)

Hm..
Do you know a way to include these tests in the CMS test group? The view is not enabled in LMS, so it will fail there.

Do you know a way to include these tests in the CMS test group? The view is not enabled in LMS, so it will fail there.

Oh I understand, I haven't found an example of it, I'm not sure if it's a good practice. @bradenmacdonald could you help us with this?

@ChrisChV good catch - thanks!

Unfortunately it seems like the whole openedx/features/ directory is only tested as LMS on CI. What you'll need to do is after this PR merges, open a second PR to move this whole content_tagging app from openedx/features/ to openedx/core/djangoapps/content_tagging. Then, you can edit unit-test-shards.json and put it into shard openedx-3 or openedx-4 (whichever is currently running faster on the CI).

rpenido · 2023-08-31T17:23:50Z

Also, for the record, you need to update the testing instructions with "enable the waffle flag" to check the functionality.

Well noted @ChrisChV ! Thank you!

bradenmacdonald

Looks good, thanks! I can merge tomorrow.

…ystem-taxonomies

bradenmacdonald · 2023-09-01T17:49:37Z

I see you just updated this with master. I'll hit merge once the build is green. Thanks.

openedx-webhooks · 2023-09-01T18:09:50Z

@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 · 2023-09-01T19:09:58Z

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

edx-pipeline-bot · 2023-09-01T22:18:00Z

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

commit taken from openedx#32907

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 4, 2023

ChrisChV reviewed Aug 4, 2023

View reviewed changes

openedx/features/content_tagging/tasks.py Show resolved Hide resolved

rpenido commented Aug 4, 2023

View reviewed changes

openedx/features/content_tagging/tasks.py Outdated Show resolved Hide resolved

rpenido force-pushed the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch from 7665cf2 to d964ba3 Compare August 10, 2023 21:04

pomegranited reviewed Aug 17, 2023

View reviewed changes

openedx/features/content_tagging/fixtures/system_defined.yaml Outdated Show resolved Hide resolved

pomegranited mentioned this pull request Aug 17, 2023

[Tagging] Remove orphaned course tags openedx/modular-learning#95

Open

rpenido force-pushed the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch 3 times, most recently from f6693fb to fe32017 Compare August 23, 2023 19:04

rpenido and others added 2 commits August 23, 2023 16:20

chore: update requirements

47af3bf

feat: Add retrieve object_tags REST API (#577)

4ed15f5

rpenido force-pushed the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch from fe32017 to a18182b Compare August 23, 2023 19:25

feat: add content auto-tagging (language)

2bc5070

rpenido force-pushed the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch from a18182b to 2bc5070 Compare August 23, 2023 19:36

rpenido added 5 commits August 23, 2023 16:40

chore: add __init__.py

0f62355

Merge branch 'master' into rpenido/fal-3460-content-auto-tagging-by-s…

04b6282

…ystem-taxonomies

style: fix pep8

b06e2bb

style: fix pylint

d6bc300

style: fix pep8

9017818

test: fix query count and comments

ee179b1

rpenido force-pushed the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch from a6e08f2 to ee179b1 Compare August 25, 2023 00:27

rpenido added 5 commits August 25, 2023 09:30

test: fix query count

858f263

fix: add try..except

ea2e149

fix: fix exit condition to avoid error

fe05c57

test: fix query count

5bddde6

Merge branch 'master' into rpenido/fal-3460-content-auto-tagging-by-s…

e0d816a

…ystem-taxonomies

rpenido marked this pull request as ready for review August 25, 2023 20:32

rpenido added 4 commits August 30, 2023 14:44

Revert "test: add replicaset config for lms"

2ccdeb5

This reverts commit 3021492.

feat: add CONTENT_TAGGING_AUTO WaffleSwitch

1c27f65

revert: revert changes in test_mixed_modulestore

5200966

style: remove unused import

2612fe9

bradenmacdonald reviewed Aug 30, 2023

View reviewed changes

rpenido added 4 commits August 31, 2023 08:51

test: add invalid tags tests

5fc811d

refactor: change WaffleSwitch to CourseWaffleFlag

154a4ae

Merge branch 'master' into rpenido/fal-3460-content-auto-tagging-by-s…

6efc2c2

…ystem-taxonomies

test: fix query count

1a1b634

ChrisChV approved these changes Aug 31, 2023

View reviewed changes

bradenmacdonald approved these changes Aug 31, 2023

View reviewed changes

Merge branch 'master' into rpenido/fal-3460-content-auto-tagging-by-s…

68c911a

…ystem-taxonomies

rpenido changed the title ~~feat: add auto-tagging~~ feat: add language auto-tagging Sep 1, 2023

bradenmacdonald merged commit 6e28ba3 into openedx:master Sep 1, 2023
42 checks passed

bradenmacdonald deleted the rpenido/fal-3460-content-auto-tagging-by-system-taxonomies branch September 1, 2023 18:09

rpenido mentioned this pull request Sep 14, 2023

Move content_tagging django app so that CMS-only tests run #33259

Merged

pomegranited mentioned this pull request Jan 30, 2024

[Tagging] Download Course Tag Spreadsheet openedx/modular-learning#172

Closed

andrey-canon added a commit to nelc/edx-platform that referenced this pull request May 8, 2024

fix: update modulestore tests

61c6103

commit taken from openedx#32907

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add language auto-tagging #32907

feat: add language auto-tagging #32907

rpenido commented Aug 4, 2023 •

edited

Loading

openedx-webhooks commented Aug 4, 2023 •

edited

Loading

rpenido commented Aug 24, 2023

bradenmacdonald commented Aug 24, 2023

rpenido commented Aug 30, 2023

bradenmacdonald Aug 30, 2023

rpenido Aug 30, 2023

bradenmacdonald Aug 30, 2023

rpenido Aug 30, 2023

rpenido Aug 31, 2023

rpenido commented Aug 31, 2023

ChrisChV left a comment •

edited

Loading

ChrisChV Aug 31, 2023

ChrisChV Aug 31, 2023

rpenido Aug 31, 2023

ChrisChV Aug 31, 2023

bradenmacdonald Aug 31, 2023

rpenido commented Aug 31, 2023

bradenmacdonald left a comment

bradenmacdonald commented Sep 1, 2023

openedx-webhooks commented Sep 1, 2023

edx-pipeline-bot commented Sep 1, 2023

edx-pipeline-bot commented Sep 1, 2023

		LANGUAGE_TAXONOMY_ID = -1


		@skip_unless_cms # Auto-tagging is only available in the CMS

feat: add language auto-tagging #32907

feat: add language auto-tagging #32907

Conversation

rpenido commented Aug 4, 2023 • edited Loading

Description

Supporting information:

More Info

Testing instructions

openedx-webhooks commented Aug 4, 2023 • edited Loading

rpenido commented Aug 24, 2023

bradenmacdonald commented Aug 24, 2023

rpenido commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Aug 31, 2023

ChrisChV left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Aug 31, 2023

bradenmacdonald left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Sep 1, 2023

openedx-webhooks commented Sep 1, 2023

edx-pipeline-bot commented Sep 1, 2023

edx-pipeline-bot commented Sep 1, 2023

rpenido commented Aug 4, 2023 •

edited

Loading

openedx-webhooks commented Aug 4, 2023 •

edited

Loading

ChrisChV left a comment •

edited

Loading