From 47af3bfdbee075c59071f65dbc750b0a4c717980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 10:05:31 -0300 Subject: [PATCH 01/49] chore: update requirements --- requirements/constraints.txt | 5 ----- requirements/edx/base.txt | 6 ++---- requirements/edx/development.txt | 1 - requirements/edx/doc.txt | 6 ++---- requirements/edx/testing.txt | 6 ++---- 5 files changed, 6 insertions(+), 18 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index a94ea0e02de4..683a8a2799dd 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -131,8 +131,3 @@ click==8.1.6 # plz upgrade this in separate ticket redis==4.6.0 - -# openedx-learning new version has some changes which are breaking quality tests -# See https://github.com/openedx/openedx-learning/pull/68 for the changes. -# It needs to be updated in a separate issue. -openedx-learning==0.1.2 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6cd543e793a2..f0679246cc60 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -770,10 +770,8 @@ openedx-filters==1.5.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.1.2 - # via - # -c requirements/edx/../constraints.txt - # -r requirements/edx/kernel.in +openedx-learning==0.1.5 + # via -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in optimizely-sdk==4.1.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 08eafdea9536..46595f4186c7 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1301,7 +1301,6 @@ openedx-filters==1.5.0 # lti-consumer-xblock openedx-learning==0.1.2 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt openedx-mongodbproxy==0.2.0 diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 7a4f2f81a2dd..3f68d7befa65 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -910,10 +910,8 @@ openedx-filters==1.5.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.2 - # via - # -c requirements/edx/../constraints.txt - # -r requirements/edx/base.txt +openedx-learning==0.1.5 + # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 6446f7dd7991..edb0cb93bab3 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -979,10 +979,8 @@ openedx-filters==1.5.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.2 - # via - # -c requirements/edx/../constraints.txt - # -r requirements/edx/base.txt +openedx-learning==0.1.5 + # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 From 4ed15f536a5c73efdd00c724ba5f163aed65b2ca Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 23 Aug 2023 00:21:21 +0100 Subject: [PATCH 02/49] feat: Add retrieve object_tags REST API (#577) --- openedx/features/content_tagging/api.py | 8 ++------ .../content_tagging/rest_api/v1/urls.py | 19 +++++++++++++++++++ .../content_tagging/tests/test_api.py | 12 ++++-------- requirements/edx/base.txt | 2 ++ requirements/edx/development.txt | 4 +++- requirements/edx/doc.txt | 2 ++ requirements/edx/testing.txt | 2 ++ 7 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 openedx/features/content_tagging/rest_api/v1/urls.py diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index f1bb43f66ce9..b2cb4653b2f5 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -101,20 +101,16 @@ def get_taxonomies_for_org( def get_content_tags( - object_id: str, taxonomy: Taxonomy = None, valid_only=True + object_id: str, taxonomy_id: str = None ) -> Iterator[ContentObjectTag]: """ Generates a list of content tags for a given object. Pass taxonomy to limit the returned object_tags to a specific taxonomy. - - Pass valid_only=False when displaying tags to content authors, so they can see invalid tags too. - Invalid tags will (probably) be hidden from learners. """ for object_tag in oel_tagging.get_object_tags( object_id=object_id, - taxonomy=taxonomy, - valid_only=valid_only, + taxonomy_id=taxonomy_id, ): yield ContentObjectTag.cast(object_tag) diff --git a/openedx/features/content_tagging/rest_api/v1/urls.py b/openedx/features/content_tagging/rest_api/v1/urls.py new file mode 100644 index 000000000000..5c0bceb38ee4 --- /dev/null +++ b/openedx/features/content_tagging/rest_api/v1/urls.py @@ -0,0 +1,19 @@ +""" +Taxonomies API v1 URLs. +""" + +from rest_framework.routers import DefaultRouter + +from django.urls.conf import path, include + +from openedx_tagging.core.tagging.rest_api.v1 import views as oel_tagging_views + +from . import views + +router = DefaultRouter() +router.register("taxonomies", views.TaxonomyOrgView, basename="taxonomy") +router.register("object_tags", oel_tagging_views.ObjectTagView, basename="object_tag") + +urlpatterns = [ + path('', include(router.urls)) +] diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py index f97958f9f60c..263ae761ef57 100644 --- a/openedx/features/content_tagging/tests/test_api.py +++ b/openedx/features/content_tagging/tests/test_api.py @@ -189,14 +189,12 @@ def test_get_content_tags_valid_for_org( object_tag_attr, ): taxonomy_id = getattr(self, taxonomy_attr).id - taxonomy = api.get_taxonomy(taxonomy_id) object_tag = getattr(self, object_tag_attr) - with self.assertNumQueries(1): + with self.assertNumQueries(2): valid_tags = list( api.get_content_tags( - taxonomy=taxonomy, + taxonomy_id=taxonomy_id, object_id=object_tag.object_id, - valid_only=True, ) ) assert len(valid_tags) == 1 @@ -219,14 +217,12 @@ def test_get_content_tags_include_invalid( object_tag_attr, ): taxonomy_id = getattr(self, taxonomy_attr).id - taxonomy = api.get_taxonomy(taxonomy_id) object_tag = getattr(self, object_tag_attr) - with self.assertNumQueries(1): + with self.assertNumQueries(2): valid_tags = list( api.get_content_tags( - taxonomy=taxonomy, + taxonomy_id=taxonomy_id, object_id=object_tag.object_id, - valid_only=False, ) ) assert len(valid_tags) == 1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f0679246cc60..bb7ba0268d59 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -43,6 +43,7 @@ attrs==23.1.0 # lti-consumer-xblock # openedx-blockstore # openedx-events + # openedx-learning # referencing babel==2.11.0 # via @@ -98,6 +99,7 @@ celery==5.3.1 # edx-celeryutils # edx-enterprise # event-tracking + # openedx-learning certifi==2023.7.22 # via # -r requirements/edx/paver.txt diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 46595f4186c7..bc4f6e2a9a46 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -92,6 +92,7 @@ attrs==23.1.0 # lti-consumer-xblock # openedx-blockstore # openedx-events + # openedx-learning # referencing babel==2.11.0 # via @@ -175,6 +176,7 @@ celery==5.3.1 # edx-celeryutils # edx-enterprise # event-tracking + # openedx-learning certifi==2023.7.22 # via # -r requirements/edx/doc.txt @@ -1299,7 +1301,7 @@ openedx-filters==1.5.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.1.2 +openedx-learning==0.1.5 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 3f68d7befa65..052b61028b40 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -60,6 +60,7 @@ attrs==23.1.0 # lti-consumer-xblock # openedx-blockstore # openedx-events + # openedx-learning # referencing babel==2.11.0 # via @@ -125,6 +126,7 @@ celery==5.3.1 # edx-celeryutils # edx-enterprise # event-tracking + # openedx-learning certifi==2023.7.22 # via # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index edb0cb93bab3..043164cafc3b 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -66,6 +66,7 @@ attrs==23.1.0 # lti-consumer-xblock # openedx-blockstore # openedx-events + # openedx-learning # referencing babel==2.11.0 # via @@ -131,6 +132,7 @@ celery==5.3.1 # edx-celeryutils # edx-enterprise # event-tracking + # openedx-learning certifi==2023.7.22 # via # -r requirements/edx/base.txt From 2bc5070a5d9b20499deb40f909377fc671bda49d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 16:36:22 -0300 Subject: [PATCH 03/49] feat: add content auto-tagging (language) --- openedx/features/content_tagging/api.py | 2 +- openedx/features/content_tagging/apps.py | 4 + openedx/features/content_tagging/handlers.py | 66 +++++++++ .../migrations/0004_system_defined_org.py | 29 ++++ openedx/features/content_tagging/tasks.py | 129 ++++++++++++++++++ .../content_tagging/tests/test_tasks.py | 111 +++++++++++++++ 6 files changed, 340 insertions(+), 1 deletion(-) create mode 100644 openedx/features/content_tagging/handlers.py create mode 100644 openedx/features/content_tagging/migrations/0004_system_defined_org.py create mode 100644 openedx/features/content_tagging/tasks.py create mode 100644 openedx/features/content_tagging/tests/test_tasks.py diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index b2cb4653b2f5..715f69cc8baf 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -146,8 +146,8 @@ def tag_content_object( # Expose the oel_tagging APIs - get_taxonomy = oel_tagging.get_taxonomy get_taxonomies = oel_tagging.get_taxonomies get_tags = oel_tagging.get_tags +delete_object_tags = oel_tagging.delete_object_tags resync_object_tags = oel_tagging.resync_object_tags diff --git a/openedx/features/content_tagging/apps.py b/openedx/features/content_tagging/apps.py index 29f9c5005f43..29952b7bc33d 100644 --- a/openedx/features/content_tagging/apps.py +++ b/openedx/features/content_tagging/apps.py @@ -10,3 +10,7 @@ class ContentTaggingConfig(AppConfig): default_auto_field = "django.db.models.BigAutoField" name = "openedx.features.content_tagging" + + def ready(self): + # Connect signal handlers + from . import handlers # pylint: disable=unused-import diff --git a/openedx/features/content_tagging/handlers.py b/openedx/features/content_tagging/handlers.py new file mode 100644 index 000000000000..bf0048ca569b --- /dev/null +++ b/openedx/features/content_tagging/handlers.py @@ -0,0 +1,66 @@ +""" +Automatic tagging of content +""" + +import logging + +from django.dispatch import receiver +from openedx_events.content_authoring.data import CourseData, XBlockData +from openedx_events.content_authoring.signals import COURSE_CREATED, XBLOCK_CREATED, XBLOCK_DELETED, XBLOCK_UPDATED + +from .tasks import delete_course_tags +from .tasks import ( + delete_xblock_tags, + update_course_tags, + update_xblock_tags +) + +log = logging.getLogger(__name__) + + +@receiver(COURSE_CREATED) +def auto_tag_course(**kwargs): + """ + Automatically tag course based on their metadata + """ + course_data = kwargs.get("course", None) + if not course_data or not isinstance(course_data, CourseData): + log.error("Received null or incorrect data for event") + return + + update_course_tags.delay(str(course_data.course_key)) + + +@receiver(XBLOCK_CREATED) +@receiver(XBLOCK_UPDATED) +def auto_tag_xblock(**kwargs): + """ + Automatically tag XBlock based on their metadata + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): + log.error("Received null or incorrect data for event") + return + + if xblock_info.block_type == "course": + # Course update is handled by XBlock of course type + update_course_tags.delay(str(xblock_info.usage_key.course_key)) + + update_xblock_tags.delay(str(xblock_info.usage_key)) + + +@receiver(XBLOCK_DELETED) +def delete_tag_xblock(**kwargs): + """ + Automatically delete XBlock auto tags. + """ + xblock_info = kwargs.get("xblock_info", None) + if not xblock_info or not isinstance(xblock_info, XBlockData): + log.error("Received null or incorrect data for event") + return + + if xblock_info.block_type == "course": + # Course deletion is handled by XBlock of course type + delete_course_tags.delay(str(xblock_info.usage_key.course_key)) + + delete_xblock_tags.delay(str(xblock_info.usage_key)) diff --git a/openedx/features/content_tagging/migrations/0004_system_defined_org.py b/openedx/features/content_tagging/migrations/0004_system_defined_org.py new file mode 100644 index 000000000000..4a7586719f6e --- /dev/null +++ b/openedx/features/content_tagging/migrations/0004_system_defined_org.py @@ -0,0 +1,29 @@ +from django.db import migrations + + +def load_system_defined_org_taxonomies(apps, _schema_editor): + """ + Associates the system defined taxonomy Language (id=-1) to all orgs + """ + TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg") + + TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None) + + +def revert_system_defined_org_taxonomies(apps, _schema_editor): + """ + Deletes association of system defined taxonomy Language (id=-1) to all orgs + """ + TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg") + + TaxonomyOrg.objects.get(id=-1).delete() + + +class Migration(migrations.Migration): + dependencies = [ + ("content_tagging", "0003_system_defined_fixture"), + ] + + operations = [ + migrations.RunPython(load_system_defined_org_taxonomies, revert_system_defined_org_taxonomies), + ] diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py new file mode 100644 index 000000000000..5fc67e8e19aa --- /dev/null +++ b/openedx/features/content_tagging/tasks.py @@ -0,0 +1,129 @@ +""" +Defines asynchronous celery task for auto-tagging content +""" + +import logging + +from celery import shared_task +from celery_utils.logged_task import LoggedTask +from django.contrib.auth import get_user_model +from edx_django_utils.monitoring import set_code_owner_attribute +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_tagging.core.tagging.models import Taxonomy + +from xmodule.modulestore.django import modulestore + +from . import api + +LANGUAGE_TAXONOMY_ID = -1 + +log = logging.getLogger(__name__) +User = get_user_model() + + +def _has_taxonomy(taxonomy: Taxonomy, content_object) -> bool: + """ + Return True if this Taxonomy have some Tag set in the content_object + """ + _exausted = object() + + content_tags = api.get_content_tags(object_id=content_object, taxonomy_id=taxonomy.id) + return next(content_tags, _exausted) is not _exausted + + +def _update_tags(content_object, lang) -> None: + lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) + + if lang and not _has_taxonomy(lang_taxonomy, content_object): + tags = api.get_tags(lang_taxonomy) + lang_tag = next(tag for tag in tags if tag.external_id == lang) + api.tag_content_object(lang_taxonomy, [lang_tag.id], content_object) + + +def _delete_tags(content_object) -> None: + api.delete_object_tags(content_object) + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def update_course_tags(course_key_str: str) -> bool: + """ + Updates the tags for a Course. + + Params: + course_key_str (str): identifier of the Course + """ + try: + course_key = CourseKey.from_string(course_key_str) + + log.info("Updating tags for Course with id: %s", course_key) + + course = modulestore().get_course(course_key) + lang = course.language + + _update_tags(course_key, lang) + + return True + except Exception as e: + log.error("Error updating tags for Course with id: %s. %s", course_key, e) + return False + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def delete_course_tags(course_key_str: str): + """ + Delete the tags for a Course. + + Params: + course_key_str (str): identifier of the Course + """ + course_key = CourseKey.from_string(course_key_str) + + log.info("Deleting tags for Course with id: %s", course_key) + + _delete_tags(course_key) + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def update_xblock_tags(usage_key_str: str): + """ + Updates the tags for a XBlock. + + Params: + usage_key_str (str): identifier of the XBlock + """ + try: + usage_key = UsageKey.from_string(usage_key_str) + + log.info("Updating tags for XBlock with id: %s", usage_key) + + if usage_key.course_key.is_course: + course = modulestore().get_course(usage_key.course_key) + lang = course.language + else: + return False + + _update_tags(usage_key, lang) + + return True + except Exception as e: + log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) + return False, e + + +@shared_task(base=LoggedTask) +@set_code_owner_attribute +def delete_xblock_tags(usage_key_str: str): + """ + Delete the tags for a XBlock. + + Params: + usage_key_str (str): identifier of the XBlock + """ + usage_key = UsageKey.from_string(usage_key_str) + + log.info("Deleting tags for XBlock with id: %s", usage_key) + + _delete_tags(usage_key) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py new file mode 100644 index 000000000000..eedccb425448 --- /dev/null +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -0,0 +1,111 @@ +""" +Test for auto-tagging content +""" +from unittest.mock import patch + +import ddt +from django.conf import settings +from django.core.management import call_command +from django.test.utils import override_settings +from openedx_tagging.core.tagging.models import ObjectTag +from organizations.models import Organization + +from openedx.core.lib.tests import attr +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.test_mixed_modulestore import CommonMixedModuleStoreSetup + +from ..tasks import delete_xblock_tags, update_course_tags, update_xblock_tags + +if not settings.configured: + settings.configure() + +LANGUAGE_TAXONOMY_ID = -1 + + +@ddt.ddt +@attr("mongo") +@override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", +) +class TestCourseAutoTagging(CommonMixedModuleStoreSetup): + """ + Test if the handlers are callend and if they call the right tasks + """ + + def _check_tag(self, object_id, taxonomy, value): + object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy=taxonomy).first() + assert object_tag, "Tag not found" + assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" + return True + + @classmethod + def setUpClass(cls): + # Run fixtures to create the system defined tags + super().setUpClass() + call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml") + call_command("loaddata", "--app=content_tagging", "system_defined.yaml") + + def setUp(self): + super().setUp() + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + + @ddt.data( + ModuleStoreEnum.Type.mongo, + ModuleStoreEnum.Type.split, + ) + @patch("openedx.features.content_tagging.tasks.modulestore") + def test_create_course_with_xblock(self, default_ms, mock_modulestore): + with patch.object(update_course_tags, "delay") as mock_update_course_tags: + self.initdb(default_ms) + mock_modulestore.return_value = self.store + # initdb will create a Course and trigger mock_update_course_tags, so we need to reset it + mock_update_course_tags.reset_mock() + + # Create course + course = self.store.create_course( + self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + ) + course_key_str = str(course.id) + # Check if task was called + mock_update_course_tags.assert_called_with(course_key_str) + + # Make the actual call synchronously + assert update_course_tags(course_key_str) == True + + # Check if the tags are created in the Course + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + + with patch.object(update_xblock_tags, "delay") as mock_update_xblock_tags: + # Create XBlock + sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + + # publish sequential changes + self.store.publish(sequential.location, self.user_id) + + usage_key_str = str(vertical.location) + # Check if task was called + mock_update_xblock_tags.assert_any_call(usage_key_str) + + # Make the actual call synchronously + assert update_xblock_tags(usage_key_str) == True + + # Check if the tags are created in the XBlock + assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, "Portuguese") + + # Update course language + with patch.object(update_course_tags, "delay") as mock_update_course_tags: + course.language = "en" + self.store.update_item(course, self.user_id) + # Check if task was called + mock_update_course_tags.assert_called_with(course_key_str) + + # Make the actual call synchronously + assert update_course_tags(course_key_str) == True + + self.store.publish(sequential.location, self.user_id) + with patch.object(delete_xblock_tags, "delay") as mock_delete_xblock_tags: + self.store.delete_item(vertical.location, self.user_id) + mock_delete_xblock_tags.assert_called_with(usage_key_str) From 0f62355b70d9c3b1a80259a6aedc16b52e867d7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 16:40:06 -0300 Subject: [PATCH 04/49] chore: add __init__.py --- openedx/features/content_tagging/rest_api/v1/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 openedx/features/content_tagging/rest_api/v1/__init__.py diff --git a/openedx/features/content_tagging/rest_api/v1/__init__.py b/openedx/features/content_tagging/rest_api/v1/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 From b06e2bb9af9e712689b8735f0bfbd10676be7b44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 17:45:24 -0300 Subject: [PATCH 05/49] style: fix pep8 --- openedx/features/content_tagging/tests/test_tasks.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index eedccb425448..b4bcdbc326df 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -72,7 +72,7 @@ def test_create_course_with_xblock(self, default_ms, mock_modulestore): mock_update_course_tags.assert_called_with(course_key_str) # Make the actual call synchronously - assert update_course_tags(course_key_str) == True + assert update_course_tags(course_key_str) # Check if the tags are created in the Course assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") @@ -90,7 +90,7 @@ def test_create_course_with_xblock(self, default_ms, mock_modulestore): mock_update_xblock_tags.assert_any_call(usage_key_str) # Make the actual call synchronously - assert update_xblock_tags(usage_key_str) == True + assert update_xblock_tags(usage_key_str) # Check if the tags are created in the XBlock assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, "Portuguese") @@ -103,7 +103,7 @@ def test_create_course_with_xblock(self, default_ms, mock_modulestore): mock_update_course_tags.assert_called_with(course_key_str) # Make the actual call synchronously - assert update_course_tags(course_key_str) == True + assert update_course_tags(course_key_str) self.store.publish(sequential.location, self.user_id) with patch.object(delete_xblock_tags, "delay") as mock_delete_xblock_tags: From d6bc3002cdcba194956d5a65b4e02e3fb08b26e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 18:07:39 -0300 Subject: [PATCH 06/49] style: fix pylint --- openedx/features/content_tagging/tasks.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 5fc67e8e19aa..413ec90dc7fc 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -32,6 +32,11 @@ def _has_taxonomy(taxonomy: Taxonomy, content_object) -> bool: def _update_tags(content_object, lang) -> None: + """ + Update the tags for a content_object. + + If the content_object already have a tag for the language taxonomy, it will be skipped. + """ lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) if lang and not _has_taxonomy(lang_taxonomy, content_object): @@ -64,7 +69,7 @@ def update_course_tags(course_key_str: str) -> bool: _update_tags(course_key, lang) return True - except Exception as e: + except Exception as e: # pylint: disable=broad-except log.error("Error updating tags for Course with id: %s. %s", course_key, e) return False @@ -108,7 +113,7 @@ def update_xblock_tags(usage_key_str: str): _update_tags(usage_key, lang) return True - except Exception as e: + except Exception as e: # pylint: disable=broad-except log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) return False, e From 9017818a9a26a07287505b2ea859b54791e6c3a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 23 Aug 2023 18:47:44 -0300 Subject: [PATCH 07/49] style: fix pep8 --- openedx/features/content_tagging/tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 413ec90dc7fc..696d4767e0c5 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -69,7 +69,7 @@ def update_course_tags(course_key_str: str) -> bool: _update_tags(course_key, lang) return True - except Exception as e: # pylint: disable=broad-except + except Exception as e: # pylint: disable=broad-except log.error("Error updating tags for Course with id: %s. %s", course_key, e) return False @@ -113,7 +113,7 @@ def update_xblock_tags(usage_key_str: str): _update_tags(usage_key, lang) return True - except Exception as e: # pylint: disable=broad-except + except Exception as e: # pylint: disable=broad-except log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) return False, e From ee179b141de17acd53db962dd593b9a1211f2978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 24 Aug 2023 21:27:34 -0300 Subject: [PATCH 08/49] test: fix query count and comments --- .../tests/test_mixed_modulestore.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 7e35217a1064..361551a06909 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -541,7 +541,8 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, + # select (by course_id, from XBLOCK_UPDATED handler in Content Tagging) # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) @@ -1069,15 +1070,17 @@ def test_has_changes_missing_child(self, default_ms, default_branch): assert self.store.has_changes(parent) # Draft + # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # Find: find parents (definition.children query), get parent, get course (fill in run?), # find parents of the parent (course), get inheritance items, # get item (to delete subtree), get inheritance again. # Sends: delete item, update parent # Split - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1099,14 +1102,16 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): self.store.get_item(self.writable_chapter_location, revision=ModuleStoreEnum.RevisionOption.published_only) # Draft: + # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: find parent (definition.children), count versions of item, get parent, count grandparents, # inheritance items, draft item, draft child, inheritance # sends: delete draft vertical and update parent # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: draft and published structures, definition (unnecessary) # sends: update published (why?), draft, and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 8, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 2), (ModuleStoreEnum.Type.split, 5, 3, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1154,13 +1159,15 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send assert vert_loc not in course.children # Draft: + # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: structure (cached) # send: update structure and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 1), (ModuleStoreEnum.Type.split, 5, 1, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ From 858f2638b0a427e6bcc269310b5e06ba4e78322e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 09:30:54 -0300 Subject: [PATCH 09/49] test: fix query count --- xmodule/modulestore/tests/test_mixed_modulestore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 361551a06909..c459d4c5cf2b 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -545,7 +545,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # select (by course_id, from XBLOCK_UPDATED handler in Content Tagging) # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ From ea2e149e41ed0e431142c83226751aac2281ec97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 09:36:21 -0300 Subject: [PATCH 10/49] fix: add try..except --- openedx/features/content_tagging/tasks.py | 32 ++++++++++++++++------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 696d4767e0c5..390c46b30035 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -76,23 +76,29 @@ def update_course_tags(course_key_str: str) -> bool: @shared_task(base=LoggedTask) @set_code_owner_attribute -def delete_course_tags(course_key_str: str): +def delete_course_tags(course_key_str: str) -> bool: """ Delete the tags for a Course. Params: course_key_str (str): identifier of the Course """ - course_key = CourseKey.from_string(course_key_str) + try: + course_key = CourseKey.from_string(course_key_str) - log.info("Deleting tags for Course with id: %s", course_key) + log.info("Deleting tags for Course with id: %s", course_key) - _delete_tags(course_key) + _delete_tags(course_key) + + return True + except Exception as e: # pylint: disable=broad-except + log.error("Error deleting tags for Course with id: %s. %s", course_key, e) + return False @shared_task(base=LoggedTask) @set_code_owner_attribute -def update_xblock_tags(usage_key_str: str): +def update_xblock_tags(usage_key_str: str) -> bool: """ Updates the tags for a XBlock. @@ -115,20 +121,26 @@ def update_xblock_tags(usage_key_str: str): return True except Exception as e: # pylint: disable=broad-except log.error("Error updating tags for XBlock with id: %s. %s", usage_key, e) - return False, e + return False @shared_task(base=LoggedTask) @set_code_owner_attribute -def delete_xblock_tags(usage_key_str: str): +def delete_xblock_tags(usage_key_str: str) -> bool: """ Delete the tags for a XBlock. Params: usage_key_str (str): identifier of the XBlock """ - usage_key = UsageKey.from_string(usage_key_str) + try: + usage_key = UsageKey.from_string(usage_key_str) + + log.info("Deleting tags for XBlock with id: %s", usage_key) - log.info("Deleting tags for XBlock with id: %s", usage_key) + _delete_tags(usage_key) - _delete_tags(usage_key) + return True + except Exception as e: # pylint: disable=broad-except + log.error("Error deleting tags for XBlock with id: %s. %s", usage_key, e) + return False From fe05c57b071728985132b2679bcec97429b7214e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 15:11:38 -0300 Subject: [PATCH 11/49] fix: fix exit condition to avoid error --- openedx/features/content_tagging/tasks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 390c46b30035..6dfe842578af 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -112,9 +112,11 @@ def update_xblock_tags(usage_key_str: str) -> bool: if usage_key.course_key.is_course: course = modulestore().get_course(usage_key.course_key) + if course is None: + return True lang = course.language else: - return False + return True _update_tags(usage_key, lang) From 5bddde6a382d83ca324923b17d1397b85c6e3716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 16:11:31 -0300 Subject: [PATCH 12/49] test: fix query count --- xmodule/modulestore/tests/test_mixed_modulestore.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index c459d4c5cf2b..2c7a130051c8 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -541,11 +541,10 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, - # select (by course_id, from XBLOCK_UPDATED handler in Content Tagging) - # find: definitions (calculator field), structures + # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record + # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ From 56300b7dbc593566ae4dbcc10d8058572c360f54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 18:13:20 -0300 Subject: [PATCH 13/49] refactor: change fixture to migration --- .../fixtures/system_defined.yaml | 22 ---------- .../migrations/0003_system_defined_fixture.py | 43 +++++++++++++++---- 2 files changed, 35 insertions(+), 30 deletions(-) delete mode 100644 openedx/features/content_tagging/fixtures/system_defined.yaml diff --git a/openedx/features/content_tagging/fixtures/system_defined.yaml b/openedx/features/content_tagging/fixtures/system_defined.yaml deleted file mode 100644 index 07445346272f..000000000000 --- a/openedx/features/content_tagging/fixtures/system_defined.yaml +++ /dev/null @@ -1,22 +0,0 @@ -- model: oel_tagging.taxonomy - pk: -2 - fields: - name: Organizations - description: Allows tags for any organization ID created on the instance. - enabled: true - required: true - allow_multiple: false - allow_free_text: false - visible_to_authors: false - _taxonomy_class: openedx.features.content_tagging.models.ContentAuthorTaxonomy -- model: oel_tagging.taxonomy - pk: -3 - fields: - name: Content Authors - description: Allows tags for any user ID created on the instance. - enabled: true - required: true - allow_multiple: false - allow_free_text: false - visible_to_authors: false - _taxonomy_class: openedx.features.content_tagging.models.ContentOrganizationTaxonomy diff --git a/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py b/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py index c155b341518c..747a5b89f8d9 100644 --- a/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py +++ b/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py @@ -1,37 +1,64 @@ # Generated by Django 3.2.20 on 2023-07-11 22:57 from django.db import migrations -from django.core.management import call_command -from openedx.features.content_tagging.models import ContentLanguageTaxonomy +from openedx.features.content_tagging.models import ( + ContentAuthorTaxonomy, + ContentLanguageTaxonomy, + ContentOrganizationTaxonomy, +) def load_system_defined_taxonomies(apps, schema_editor): """ Creates system defined taxonomies - """ + """ # Create system defined taxonomy instances - call_command('loaddata', '--app=content_tagging', 'system_defined.yaml') + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + author_taxonomy = Taxonomy( + pk=-2, + name="Content Authors", + description="Allows tags for any user ID created on the instance.", + enabled=True, + required=True, + allow_multiple=False, + allow_free_text=False, + visible_to_authors=False, + ) + author_taxonomy.taxonomy_class = ContentAuthorTaxonomy + author_taxonomy.save() + + org_taxonomy = Taxonomy( + pk=-3, + name="Organizations", + description="Allows tags for any organization ID created on the instance.", + enabled=True, + required=True, + allow_multiple=False, + allow_free_text=False, + visible_to_authors=False, + ) + org_taxonomy.taxonomy_class = ContentOrganizationTaxonomy + org_taxonomy.save() # Adding taxonomy class to the language taxonomy - Taxonomy = apps.get_model('oel_tagging', 'Taxonomy') language_taxonomy = Taxonomy.objects.get(id=-1) language_taxonomy.taxonomy_class = ContentLanguageTaxonomy + language_taxonomy.save() def revert_system_defined_taxonomies(apps, schema_editor): """ Deletes all system defined taxonomies """ - Taxonomy = apps.get_model('oel_tagging', 'Taxonomy') + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") Taxonomy.objects.get(id=-2).delete() Taxonomy.objects.get(id=-3).delete() class Migration(migrations.Migration): - dependencies = [ - ('content_tagging', '0002_system_defined_taxonomies'), + ("content_tagging", "0002_system_defined_taxonomies"), ] operations = [ From 901f4d3018ed4a91bef03cfd8b8010504d2d8603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 25 Aug 2023 18:50:08 -0300 Subject: [PATCH 14/49] test: fix setup --- openedx/features/content_tagging/tests/test_tasks.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index b4bcdbc326df..2f204af0c717 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -7,13 +7,14 @@ from django.conf import settings from django.core.management import call_command from django.test.utils import override_settings -from openedx_tagging.core.tagging.models import ObjectTag +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization from openedx.core.lib.tests import attr from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.test_mixed_modulestore import CommonMixedModuleStoreSetup +from ..models import ContentLanguageTaxonomy, TaxonomyOrg from ..tasks import delete_xblock_tags, update_course_tags, update_xblock_tags if not settings.configured: @@ -45,7 +46,12 @@ def setUpClass(cls): # Run fixtures to create the system defined tags super().setUpClass() call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml") - call_command("loaddata", "--app=content_tagging", "system_defined.yaml") + + # Configure language taxonomy + language_taxonomy = Taxonomy.objects.get(id=-1) + language_taxonomy.taxonomy_class = ContentLanguageTaxonomy + language_taxonomy.save() + TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None) def setUp(self): super().setUp() From 09dea58e83fb53604586bd49869ca4a1430874cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 26 Aug 2023 10:23:30 -0300 Subject: [PATCH 15/49] chore: trigger CD/CI From 5e7b097f859913eae2d2998666d6f5add501373e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 26 Aug 2023 11:10:10 -0300 Subject: [PATCH 16/49] test: trying to fix race condition --- .../tests/test_mixed_modulestore.py | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 2c7a130051c8..288fb7c5c011 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -30,6 +30,7 @@ # before importing the module # TODO remove this import and the configuration -- xmodule should not depend on django! from django.conf import settings +from django.test.utils import override_settings from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator # pylint: disable=unused-import from pytz import UTC @@ -541,11 +542,16 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record + # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, XBLOCK_UPDATED handler call # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack + @override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", + ) def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ Update should succeed for r/w dbs @@ -1081,6 +1087,11 @@ def test_has_changes_missing_child(self, default_ms, default_branch): # Sends: updated draft and published structures and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) @ddt.unpack + @override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", + ) def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ Delete should reject on r/o db and work on r/w one @@ -1112,6 +1123,11 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): # sends: update published (why?), draft, and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 2), (ModuleStoreEnum.Type.split, 5, 3, 3)) @ddt.unpack + @override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", + ) def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ Because old mongo treated verticals as the first layer which could be draft, it has some interesting @@ -1168,6 +1184,11 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send # send: update structure and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 1), (ModuleStoreEnum.Type.split, 5, 1, 2)) @ddt.unpack + @override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", + ) def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ Test deleting a draft vertical which has a published version. @@ -2046,6 +2067,11 @@ def _get_split_modulestore(self): # Split: active_versions (mysql), structure (mongo) @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack + @override_settings( + CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, + CELERY_ALWAYS_EAGER=True, + BROKER_BACKEND="memory", + ) def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ Test finding orphans. From a08f08a75fabc1759ac6e654e68b1c3e98ad9e9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 26 Aug 2023 14:49:44 -0300 Subject: [PATCH 17/49] test: fix query count --- xmodule/modulestore/tests/test_mixed_modulestore.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 288fb7c5c011..7d394404636f 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -545,7 +545,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, XBLOCK_UPDATED handler call # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 3, 2)) @ddt.unpack @override_settings( CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, @@ -2065,7 +2065,7 @@ def _get_split_modulestore(self): # Draft: get all items which can be or should have parents # Split: active_versions (mysql), structure (mongo) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 1, 0)) @ddt.unpack @override_settings( CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, From 08cd8b596f9fb6a5f4ee99dbd43b4c8af3e48bfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 26 Aug 2023 14:55:34 -0300 Subject: [PATCH 18/49] style: fixed pylint --- xmodule/modulestore/tests/test_mixed_modulestore.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 7d394404636f..31e1a8b93439 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -542,7 +542,8 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, XBLOCK_UPDATED handler call + # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, + # XBLOCK_UPDATED handler call # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 3, 2)) From 778552a221218ac64979e5fabbaddf4d731ee627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 17:14:49 -0300 Subject: [PATCH 19/49] docs: fix docstring from review Co-authored-by: Braden MacDonald --- openedx/features/content_tagging/tasks.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 6dfe842578af..976222d29f07 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -53,7 +53,8 @@ def _delete_tags(content_object) -> None: @set_code_owner_attribute def update_course_tags(course_key_str: str) -> bool: """ - Updates the tags for a Course. + Updates the automatically-managed tags for a course + (whenever a course is created or updated) Params: course_key_str (str): identifier of the Course @@ -78,7 +79,7 @@ def update_course_tags(course_key_str: str) -> bool: @set_code_owner_attribute def delete_course_tags(course_key_str: str) -> bool: """ - Delete the tags for a Course. + Delete the tags for a Course (when the course itself has been deleted). Params: course_key_str (str): identifier of the Course @@ -100,7 +101,8 @@ def delete_course_tags(course_key_str: str) -> bool: @set_code_owner_attribute def update_xblock_tags(usage_key_str: str) -> bool: """ - Updates the tags for a XBlock. + Updates the automatically-managed tags for a XBlock + (whenever an XBlock is created/updated). Params: usage_key_str (str): identifier of the XBlock @@ -130,7 +132,7 @@ def update_xblock_tags(usage_key_str: str) -> bool: @set_code_owner_attribute def delete_xblock_tags(usage_key_str: str) -> bool: """ - Delete the tags for a XBlock. + Delete the tags for a XBlock (when the XBlock itself is deleted). Params: usage_key_str (str): identifier of the XBlock From 975edfd808920bb9054f43cacaf0ae127bced8a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 17:25:13 -0300 Subject: [PATCH 20/49] test: remove override_settings --- .../tests/test_mixed_modulestore.py | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 31e1a8b93439..7fe4a4ed91b7 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -30,7 +30,6 @@ # before importing the module # TODO remove this import and the configuration -- xmodule should not depend on django! from django.conf import settings -from django.test.utils import override_settings from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocator # pylint: disable=unused-import from pytz import UTC @@ -548,11 +547,6 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 3, 2)) @ddt.unpack - @override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", - ) def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ Update should succeed for r/w dbs @@ -1088,11 +1082,6 @@ def test_has_changes_missing_child(self, default_ms, default_branch): # Sends: updated draft and published structures and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) @ddt.unpack - @override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", - ) def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ Delete should reject on r/o db and work on r/w one @@ -1124,11 +1113,6 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): # sends: update published (why?), draft, and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 2), (ModuleStoreEnum.Type.split, 5, 3, 3)) @ddt.unpack - @override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", - ) def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ Because old mongo treated verticals as the first layer which could be draft, it has some interesting @@ -1185,11 +1169,6 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send # send: update structure and active_versions @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 1), (ModuleStoreEnum.Type.split, 5, 1, 2)) @ddt.unpack - @override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", - ) def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ Test deleting a draft vertical which has a published version. @@ -2068,11 +2047,6 @@ def _get_split_modulestore(self): # Split: active_versions (mysql), structure (mongo) @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 1, 0)) @ddt.unpack - @override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", - ) def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ Test finding orphans. From 70650e9165226e862320f417bc6773ccffb083e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 18:36:04 -0300 Subject: [PATCH 21/49] refactor: add typings --- openedx/features/content_tagging/api.py | 9 ++++----- openedx/features/content_tagging/models/base.py | 2 +- openedx/features/content_tagging/tasks.py | 16 ++++++++-------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index 715f69cc8baf..4a5adbd8fe58 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -5,8 +5,7 @@ import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import QuerySet -from opaque_keys.edx.keys import LearningContextKey -from opaque_keys.edx.locator import BlockUsageLocator +from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization @@ -80,7 +79,7 @@ def set_taxonomy_orgs( def get_taxonomies_for_org( enabled=True, - org_owner: Organization = None, + org_owner: Organization | None = None, ) -> QuerySet: """ Generates a list of the enabled Taxonomies available for the given org, sorted by name. @@ -101,7 +100,7 @@ def get_taxonomies_for_org( def get_content_tags( - object_id: str, taxonomy_id: str = None + object_id: str, taxonomy_id: str | None = None ) -> Iterator[ContentObjectTag]: """ Generates a list of content tags for a given object. @@ -118,7 +117,7 @@ def get_content_tags( def tag_content_object( taxonomy: Taxonomy, tags: List, - object_id: Union[BlockUsageLocator, LearningContextKey], + object_id: CourseKey | UsageKey, ) -> List[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or diff --git a/openedx/features/content_tagging/models/base.py b/openedx/features/content_tagging/models/base.py index 16df0d3752e0..7907cd206110 100644 --- a/openedx/features/content_tagging/models/base.py +++ b/openedx/features/content_tagging/models/base.py @@ -115,7 +115,7 @@ class ContentTaxonomyMixin: def taxonomies_for_org( cls, queryset: QuerySet, - org: Organization = None, + org: Organization | None = None, ) -> QuerySet: """ Filters the given QuerySet to those ContentTaxonomies which are available for the given organization. diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 976222d29f07..b9fcc8214044 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -21,17 +21,17 @@ User = get_user_model() -def _has_taxonomy(taxonomy: Taxonomy, content_object) -> bool: +def _has_taxonomy(taxonomy: Taxonomy, content_object: CourseKey | UsageKey) -> bool: """ Return True if this Taxonomy have some Tag set in the content_object """ _exausted = object() - content_tags = api.get_content_tags(object_id=content_object, taxonomy_id=taxonomy.id) + content_tags = api.get_content_tags(object_id=str(content_object), taxonomy_id=taxonomy.id) return next(content_tags, _exausted) is not _exausted -def _update_tags(content_object, lang) -> None: +def _update_tags(content_object: CourseKey | UsageKey, lang) -> None: """ Update the tags for a content_object. @@ -45,8 +45,8 @@ def _update_tags(content_object, lang) -> None: api.tag_content_object(lang_taxonomy, [lang_tag.id], content_object) -def _delete_tags(content_object) -> None: - api.delete_object_tags(content_object) +def _delete_tags(content_object: CourseKey | UsageKey) -> None: + api.delete_object_tags(str(content_object)) @shared_task(base=LoggedTask) @@ -65,9 +65,9 @@ def update_course_tags(course_key_str: str) -> bool: log.info("Updating tags for Course with id: %s", course_key) course = modulestore().get_course(course_key) - lang = course.language - - _update_tags(course_key, lang) + if (course): + lang = course.language + _update_tags(course_key, lang) return True except Exception as e: # pylint: disable=broad-except From 75f4b704b618cf101bd4ff7e020a406bc01fa16c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 19:45:55 -0300 Subject: [PATCH 22/49] style: add typings --- openedx/features/content_tagging/api.py | 14 ++++++++------ openedx/features/content_tagging/models/base.py | 8 ++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index 4a5adbd8fe58..ded2953ba183 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -1,7 +1,9 @@ """ Content Tagging APIs """ -from typing import Iterator, List, Type, Union +from __future__ import annotations + +from typing import Iterator import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import QuerySet @@ -14,12 +16,12 @@ def create_taxonomy( name: str, - description: str = None, + description: str | None = None, enabled=True, required=False, allow_multiple=False, allow_free_text=False, - taxonomy_class: Type = ContentTaxonomy, + taxonomy_class: type[ContentTaxonomy] | None = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. @@ -40,7 +42,7 @@ def create_taxonomy( def set_taxonomy_orgs( taxonomy: Taxonomy, all_orgs=False, - orgs: List[Organization] = None, + orgs: list[Organization | None] | None = None, relationship: TaxonomyOrg.RelType = TaxonomyOrg.RelType.OWNER, ): """ @@ -116,9 +118,9 @@ def get_content_tags( def tag_content_object( taxonomy: Taxonomy, - tags: List, + tags: list, object_id: CourseKey | UsageKey, -) -> List[ContentObjectTag]: +) -> list[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or course). diff --git a/openedx/features/content_tagging/models/base.py b/openedx/features/content_tagging/models/base.py index 7907cd206110..38dcfb5b7572 100644 --- a/openedx/features/content_tagging/models/base.py +++ b/openedx/features/content_tagging/models/base.py @@ -1,7 +1,7 @@ """ Content Tagging models """ -from typing import List, Union +from __future__ import annotations from django.db import models from django.db.models import Exists, OuterRef, Q, QuerySet @@ -49,7 +49,7 @@ class Meta: @classmethod def get_relationships( - cls, taxonomy: Taxonomy, rel_type: RelType, org_short_name: Union[str, None] = None + cls, taxonomy: Taxonomy, rel_type: RelType, org_short_name: str | None = None ) -> QuerySet: """ Returns the relationships of the given rel_type and taxonomy where: @@ -68,7 +68,7 @@ def get_relationships( @classmethod def get_organizations( cls, taxonomy: Taxonomy, rel_type: RelType - ) -> List[Organization]: + ) -> list[Organization]: """ Returns the list of Organizations which have the given relationship to the taxonomy. """ @@ -91,7 +91,7 @@ class Meta: proxy = True @property - def object_key(self) -> Union[BlockUsageLocator, LearningContextKey]: + def object_key(self) -> BlockUsageLocator | LearningContextKey: """ Returns the object ID parsed as a UsageKey or LearningContextKey. Raises InvalidKeyError object_id cannot be parse into one of those key types. From e6dbd5286b5149f2eb2feae27a0249593007f787 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 19:46:15 -0300 Subject: [PATCH 23/49] fix: import annotations --- openedx/features/content_tagging/tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index b9fcc8214044..079d2f2a54b0 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -1,6 +1,7 @@ """ Defines asynchronous celery task for auto-tagging content """ +from __future__ import annotations import logging From 7097dd4100c54b3888c3da4719b882a1323d9b05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 28 Aug 2023 20:00:07 -0300 Subject: [PATCH 24/49] test: refactor tests --- .../content_tagging/tests/test_tasks.py | 119 +++++++++--------- 1 file changed, 61 insertions(+), 58 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 2f204af0c717..3db02d7e88e2 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -1,6 +1,8 @@ """ Test for auto-tagging content """ +from __future__ import annotations + from unittest.mock import patch import ddt @@ -14,8 +16,8 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.test_mixed_modulestore import CommonMixedModuleStoreSetup +from .. import api from ..models import ContentLanguageTaxonomy, TaxonomyOrg -from ..tasks import delete_xblock_tags, update_course_tags, update_xblock_tags if not settings.configured: settings.configure() @@ -23,22 +25,24 @@ LANGUAGE_TAXONOMY_ID = -1 -@ddt.ddt -@attr("mongo") -@override_settings( - CELERY_EAGER_PROPAGATES_EXCEPTIONS=True, - CELERY_ALWAYS_EAGER=True, - BROKER_BACKEND="memory", -) class TestCourseAutoTagging(CommonMixedModuleStoreSetup): """ Test if the handlers are callend and if they call the right tasks """ - def _check_tag(self, object_id, taxonomy, value): - object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy=taxonomy).first() - assert object_tag, "Tag not found" - assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" + def _check_tag(self, object_id: str, taxonomy_id: int, value: str | None): + """ + Check if the ObjectTag exists for the given object_id and taxonomy_id + + If value is None, check if the ObjectTag does not exists + """ + object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=taxonomy_id).first() + if value is None: + assert not object_tag, f"Expected no tag for taxonomy_id={taxonomy_id}, but one found with value={value}" + else: + assert object_tag, f"Tag for taxonomy_id={taxonomy_id} with value={value} with expected, but none found" + assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" + return True @classmethod @@ -56,62 +60,61 @@ def setUpClass(cls): def setUp(self): super().setUp() self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.initdb(ModuleStoreEnum.Type.split) + self.patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + + def tearDown(self): + self.patcher.stop() + super().tearDown() - @ddt.data( - ModuleStoreEnum.Type.mongo, - ModuleStoreEnum.Type.split, - ) - @patch("openedx.features.content_tagging.tasks.modulestore") - def test_create_course_with_xblock(self, default_ms, mock_modulestore): - with patch.object(update_course_tags, "delay") as mock_update_course_tags: - self.initdb(default_ms) - mock_modulestore.return_value = self.store - # initdb will create a Course and trigger mock_update_course_tags, so we need to reset it - mock_update_course_tags.reset_mock() - - # Create course - course = self.store.create_course( - self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} - ) - course_key_str = str(course.id) - # Check if task was called - mock_update_course_tags.assert_called_with(course_key_str) - - # Make the actual call synchronously - assert update_course_tags(course_key_str) + def test_create_course(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + ) # Check if the tags are created in the Course assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") - with patch.object(update_xblock_tags, "delay") as mock_update_xblock_tags: - # Create XBlock - sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") - vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + def test_update_course(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + ) + + # Simulates user manually changing a tag + lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) + api.tag_content_object(lang_taxonomy, ["Spanish"], course.id) + + # Update course language + course.language = "en" + self.store.update_item(course, self.user_id) - # publish sequential changes - self.store.publish(sequential.location, self.user_id) + # Does not automatically update the tag + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Spanish") - usage_key_str = str(vertical.location) - # Check if task was called - mock_update_xblock_tags.assert_any_call(usage_key_str) + def test_create_delete_xblock(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + ) - # Make the actual call synchronously - assert update_xblock_tags(usage_key_str) + # Create XBlocks + sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + + # Publish sequential changes + self.store.publish(sequential.location, self.user_id) + + usage_key_str = str(vertical.location) # Check if the tags are created in the XBlock assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, "Portuguese") - # Update course language - with patch.object(update_course_tags, "delay") as mock_update_course_tags: - course.language = "en" - self.store.update_item(course, self.user_id) - # Check if task was called - mock_update_course_tags.assert_called_with(course_key_str) - - # Make the actual call synchronously - assert update_course_tags(course_key_str) + # Delete the XBlock + self.store.delete_item(vertical.location, self.user_id) - self.store.publish(sequential.location, self.user_id) - with patch.object(delete_xblock_tags, "delay") as mock_delete_xblock_tags: - self.store.delete_item(vertical.location, self.user_id) - mock_delete_xblock_tags.assert_called_with(usage_key_str) + # Check if the tags are deleted + assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) From 2a6bcaf1f2887ac33f08736c24f42c4305dec2a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 11:10:44 -0300 Subject: [PATCH 25/49] refactor: fix tests and cleaning code --- openedx/features/content_tagging/tasks.py | 8 +-- .../content_tagging/tests/test_tasks.py | 55 ++++++++++++------- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 079d2f2a54b0..5c7cf93243fe 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -8,7 +8,6 @@ from celery import shared_task from celery_utils.logged_task import LoggedTask from django.contrib.auth import get_user_model -from edx_django_utils.monitoring import set_code_owner_attribute from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Taxonomy @@ -51,7 +50,6 @@ def _delete_tags(content_object: CourseKey | UsageKey) -> None: @shared_task(base=LoggedTask) -@set_code_owner_attribute def update_course_tags(course_key_str: str) -> bool: """ Updates the automatically-managed tags for a course @@ -61,12 +59,13 @@ def update_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: + logging.error('teste') course_key = CourseKey.from_string(course_key_str) log.info("Updating tags for Course with id: %s", course_key) course = modulestore().get_course(course_key) - if (course): + if course: lang = course.language _update_tags(course_key, lang) @@ -77,7 +76,6 @@ def update_course_tags(course_key_str: str) -> bool: @shared_task(base=LoggedTask) -@set_code_owner_attribute def delete_course_tags(course_key_str: str) -> bool: """ Delete the tags for a Course (when the course itself has been deleted). @@ -99,7 +97,6 @@ def delete_course_tags(course_key_str: str) -> bool: @shared_task(base=LoggedTask) -@set_code_owner_attribute def update_xblock_tags(usage_key_str: str) -> bool: """ Updates the automatically-managed tags for a XBlock @@ -130,7 +127,6 @@ def update_xblock_tags(usage_key_str: str) -> bool: @shared_task(base=LoggedTask) -@set_code_owner_attribute def delete_xblock_tags(usage_key_str: str) -> bool: """ Delete the tags for a XBlock (when the XBlock itself is deleted). diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 3db02d7e88e2..0ec261978dbc 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -3,33 +3,30 @@ """ from __future__ import annotations +import logging from unittest.mock import patch -import ddt -from django.conf import settings from django.core.management import call_command -from django.test.utils import override_settings from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization -from openedx.core.lib.tests import attr +from common.djangoapps.student.tests.factories import UserFactory from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.tests.test_mixed_modulestore import CommonMixedModuleStoreSetup +from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_MODULESTORE, ModuleStoreTestCase from .. import api from ..models import ContentLanguageTaxonomy, TaxonomyOrg -if not settings.configured: - settings.configure() - LANGUAGE_TAXONOMY_ID = -1 -class TestCourseAutoTagging(CommonMixedModuleStoreSetup): +class TestAutoTagging(ModuleStoreTestCase): """ - Test if the handlers are callend and if they call the right tasks + Test if the Course and XBlock tags are automatically created """ + MODULESTORE = TEST_DATA_MIXED_MODULESTORE + def _check_tag(self, object_id: str, taxonomy_id: int, value: str | None): """ Check if the ObjectTag exists for the given object_id and taxonomy_id @@ -48,32 +45,40 @@ def _check_tag(self, object_id: str, taxonomy_id: int, value: str | None): @classmethod def setUpClass(cls): # Run fixtures to create the system defined tags - super().setUpClass() call_command("loaddata", "--app=oel_tagging", "language_taxonomy.yaml") # Configure language taxonomy language_taxonomy = Taxonomy.objects.get(id=-1) language_taxonomy.taxonomy_class = ContentLanguageTaxonomy language_taxonomy.save() - TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None) + + # Enable Language taxonomy for all orgs + TaxonomyOrg.objects.create(id=-1, taxonomy=language_taxonomy, org=None) + + super().setUpClass() def setUp(self): super().setUp() + # Create user + self.user = UserFactory.create() + self.user_id = self.user.id + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") - self.initdb(ModuleStoreEnum.Type.split) self.patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) self.addCleanup(self.patcher.stop) self.patcher.start() - def tearDown(self): - self.patcher.stop() - super().tearDown() - def test_create_course(self): # Create course + logging.warning("Creating course") course = self.store.create_course( - self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "pt"}, ) + logging.warning("Creating course: done") # Check if the tags are created in the Course assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") @@ -81,7 +86,11 @@ def test_create_course(self): def test_update_course(self): # Create course course = self.store.create_course( - self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "pt"}, ) # Simulates user manually changing a tag @@ -98,7 +107,11 @@ def test_update_course(self): def test_create_delete_xblock(self): # Create course course = self.store.create_course( - self.orgA.short_name, "test_course", "test_run", self.user_id, fields={"language": "pt"} + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "pt"}, ) # Create XBlocks @@ -106,7 +119,7 @@ def test_create_delete_xblock(self): vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") # Publish sequential changes - self.store.publish(sequential.location, self.user_id) + # self.store.publish(sequential.location, self.user_id) usage_key_str = str(vertical.location) From fabb4aa208f9efff26aa275f49b24fa6cf65ae74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 12:12:57 -0300 Subject: [PATCH 26/49] revert: revert changes in api --- openedx/features/content_tagging/api.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index ded2953ba183..715f69cc8baf 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -1,13 +1,12 @@ """ Content Tagging APIs """ -from __future__ import annotations - -from typing import Iterator +from typing import Iterator, List, Type, Union import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import QuerySet -from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.keys import LearningContextKey +from opaque_keys.edx.locator import BlockUsageLocator from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization @@ -16,12 +15,12 @@ def create_taxonomy( name: str, - description: str | None = None, + description: str = None, enabled=True, required=False, allow_multiple=False, allow_free_text=False, - taxonomy_class: type[ContentTaxonomy] | None = None, + taxonomy_class: Type = ContentTaxonomy, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. @@ -42,7 +41,7 @@ def create_taxonomy( def set_taxonomy_orgs( taxonomy: Taxonomy, all_orgs=False, - orgs: list[Organization | None] | None = None, + orgs: List[Organization] = None, relationship: TaxonomyOrg.RelType = TaxonomyOrg.RelType.OWNER, ): """ @@ -81,7 +80,7 @@ def set_taxonomy_orgs( def get_taxonomies_for_org( enabled=True, - org_owner: Organization | None = None, + org_owner: Organization = None, ) -> QuerySet: """ Generates a list of the enabled Taxonomies available for the given org, sorted by name. @@ -102,7 +101,7 @@ def get_taxonomies_for_org( def get_content_tags( - object_id: str, taxonomy_id: str | None = None + object_id: str, taxonomy_id: str = None ) -> Iterator[ContentObjectTag]: """ Generates a list of content tags for a given object. @@ -118,9 +117,9 @@ def get_content_tags( def tag_content_object( taxonomy: Taxonomy, - tags: list, - object_id: CourseKey | UsageKey, -) -> list[ContentObjectTag]: + tags: List, + object_id: Union[BlockUsageLocator, LearningContextKey], +) -> List[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or course). From 7adcd8b10d9ec7c38ff4c15922ac142a307aec27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 12:15:16 -0300 Subject: [PATCH 27/49] refactor: cleaning code --- openedx/features/content_tagging/tests/test_tasks.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 0ec261978dbc..577d95df794d 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -118,9 +118,6 @@ def test_create_delete_xblock(self): sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") - # Publish sequential changes - # self.store.publish(sequential.location, self.user_id) - usage_key_str = str(vertical.location) # Check if the tags are created in the XBlock From 9f5f538566ec6f03b4f8a7ceb0f838bec2e90f90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 14:03:08 -0300 Subject: [PATCH 28/49] fix: pylint --- openedx/features/content_tagging/tests/test_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 577d95df794d..d6dac63878cf 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -11,7 +11,6 @@ from organizations.models import Organization from common.djangoapps.student.tests.factories import UserFactory -from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_MODULESTORE, ModuleStoreTestCase from .. import api From c9e77e3bd0a1d3ed8d781296effae249ba068159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 14:22:55 -0300 Subject: [PATCH 29/49] fix: patch tasks.modulestore and change replicaSet config --- xmodule/modulestore/tests/test_mixed_modulestore.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 7fe4a4ed91b7..360a3d1f5fea 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -100,6 +100,7 @@ class CommonMixedModuleStoreSetup(CourseComparisonTest, OpenEdxEventsTestMixin): 'db': DB, 'collection': COLLECTION, 'asset_collection': ASSET_COLLECTION, + 'replicaSet': None, } OPTIONS = { 'stores': [ @@ -269,8 +270,13 @@ def _initialize_mixed(self, mappings=None, contentstore=None): mappings=mappings, **self.options ) + self.addCleanup(self.store.close_all_connections) + self.patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(self.patcher.stop) + self.patcher.start() + def initdb(self, default): """ Initialize the database and create one test course in it From 721377849a77ce8afb07c671fb13ec6ecd0ffdd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 14:53:03 -0300 Subject: [PATCH 30/49] fix: pylint --- xmodule/modulestore/tests/test_mixed_modulestore.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 360a3d1f5fea..c96ddf59f595 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -273,9 +273,9 @@ def _initialize_mixed(self, mappings=None, contentstore=None): self.addCleanup(self.store.close_all_connections) - self.patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) - self.addCleanup(self.patcher.stop) - self.patcher.start() + patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) + self.addCleanup(patcher.stop) + patcher.start() def initdb(self, default): """ From b66a51baccc9514a80e30a47142922b8ce103449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 15:15:20 -0300 Subject: [PATCH 31/49] test: change scope --- openedx/features/content_tagging/tests/test_tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index d6dac63878cf..fa38c16fd20f 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -19,6 +19,7 @@ LANGUAGE_TAXONOMY_ID = -1 +@skip_unless_cms # Auto-tagging is only available in the CMS class TestAutoTagging(ModuleStoreTestCase): """ Test if the Course and XBlock tags are automatically created From 28c6b88f30caed153de0e875367a2dc8220816ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 15:23:40 -0300 Subject: [PATCH 32/49] fix: import --- openedx/features/content_tagging/tests/test_tasks.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index fa38c16fd20f..a0fa0a4c868f 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -11,6 +11,7 @@ from organizations.models import Organization from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_cms from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_MODULESTORE, ModuleStoreTestCase from .. import api From 302149233b7349441926fbbc8c796fad4409ce46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 19:16:18 -0300 Subject: [PATCH 33/49] test: add replicaset config for lms --- lms/envs/test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lms/envs/test.py b/lms/envs/test.py index 284ebc915d47..6dd7ecc27d80 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -167,6 +167,7 @@ 'port': MONGO_PORT_NUM, 'db': f'test_xmodule_{THIS_UUID}', 'collection': 'test_modulestore', + 'replicaSet': None, }, ) From 14db0ef99cd32265e62039d2bf8d5a390c7af8c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 19:59:53 -0300 Subject: [PATCH 34/49] refactor: cleaning code --- openedx/features/content_tagging/tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 5c7cf93243fe..3ecfe02d5c39 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -59,7 +59,6 @@ def update_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - logging.error('teste') course_key = CourseKey.from_string(course_key_str) log.info("Updating tags for Course with id: %s", course_key) From 5f452b40778e1111e0fb83d81beb1f9ec6987c3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 20:00:06 -0300 Subject: [PATCH 35/49] test: fix query count --- xmodule/modulestore/tests/test_mixed_modulestore.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index c96ddf59f595..5d4072151ddc 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -551,7 +551,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # XBLOCK_UPDATED handler call # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 4, 3, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 5), (ModuleStoreEnum.Type.split, 6, 3, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1086,7 +1086,7 @@ def test_has_changes_missing_child(self, default_ms, default_branch): # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 5, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1216,7 +1216,7 @@ def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): # executed twice, possibly unnecessarily) # find: 2 reads of structure, definition (s/b lazy; so, unnecessary), # plus 1 wildcard find in draft mongo which has none - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 2, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) @ddt.unpack def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -1256,7 +1256,7 @@ def test_create_child_detached_tabs(self, default_ms): # draft is 2: find out which ms owns course, get item # split: active_versions (mysql), structure, definition (to load course wiki string) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) @ddt.unpack def test_get_course(self, default_ms, num_mysql, max_find, max_send): """ From 5421f87c1d7c5c7883b57585e128774e2f2a1ced Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 29 Aug 2023 20:39:54 -0300 Subject: [PATCH 36/49] test: fix query count --- xmodule/modulestore/tests/test_mixed_modulestore.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 5d4072151ddc..d1b9f679e326 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -551,7 +551,7 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # XBLOCK_UPDATED handler call # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 5), (ModuleStoreEnum.Type.split, 6, 3, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 5), (ModuleStoreEnum.Type.split, 6, 4, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ From f711c61f410005ba98d4828b8740bce0e4745a9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 09:57:21 -0300 Subject: [PATCH 37/49] fix: remove Org Taxonomy in migration --- .../migrations/0004_system_defined_org.py | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/openedx/features/content_tagging/migrations/0004_system_defined_org.py b/openedx/features/content_tagging/migrations/0004_system_defined_org.py index 4a7586719f6e..ad4d25886843 100644 --- a/openedx/features/content_tagging/migrations/0004_system_defined_org.py +++ b/openedx/features/content_tagging/migrations/0004_system_defined_org.py @@ -1,23 +1,45 @@ from django.db import migrations +from openedx.features.content_tagging.models import ( + ContentOrganizationTaxonomy, +) def load_system_defined_org_taxonomies(apps, _schema_editor): """ - Associates the system defined taxonomy Language (id=-1) to all orgs + Associates the system defined taxonomy Language (id=-1) to all orgs and + removes the ContentOrganizationTaxonomy (id=-3) from the database """ TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg") - TaxonomyOrg.objects.create(id=-1, taxonomy_id=-1, org=None) + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + Taxonomy.objects.get(id=-3).delete() + + + def revert_system_defined_org_taxonomies(apps, _schema_editor): """ - Deletes association of system defined taxonomy Language (id=-1) to all orgs + Deletes association of system defined taxonomy Language (id=-1) to all orgs and + creates the ContentOrganizationTaxonomy (id=-3) in the database """ TaxonomyOrg = apps.get_model("content_tagging", "TaxonomyOrg") - TaxonomyOrg.objects.get(id=-1).delete() + Taxonomy = apps.get_model("oel_tagging", "Taxonomy") + org_taxonomy = Taxonomy( + pk=-3, + name="Organizations", + description="Allows tags for any organization ID created on the instance.", + enabled=True, + required=True, + allow_multiple=False, + allow_free_text=False, + visible_to_authors=False, + ) + org_taxonomy.taxonomy_class = ContentOrganizationTaxonomy + org_taxonomy.save() + class Migration(migrations.Migration): dependencies = [ From 6df73038c0e6c3895a411bbf2c7b870ba844c0aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 10:01:40 -0300 Subject: [PATCH 38/49] style: add empty space --- openedx/features/content_tagging/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index 715f69cc8baf..0dd46fba4738 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -146,6 +146,7 @@ def tag_content_object( # Expose the oel_tagging APIs + get_taxonomy = oel_tagging.get_taxonomy get_taxonomies = oel_tagging.get_taxonomies get_tags = oel_tagging.get_tags From ce23eb19aeb442e8878e070484eb0a82a887f31d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 10:04:32 -0300 Subject: [PATCH 39/49] refactor: rename _update_tags to _set_initial_language_tag --- openedx/features/content_tagging/tasks.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index 3ecfe02d5c39..d00c1c986945 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -31,11 +31,9 @@ def _has_taxonomy(taxonomy: Taxonomy, content_object: CourseKey | UsageKey) -> b return next(content_tags, _exausted) is not _exausted -def _update_tags(content_object: CourseKey | UsageKey, lang) -> None: +def _set_initial_language_tag(content_object: CourseKey | UsageKey, lang) -> None: """ - Update the tags for a content_object. - - If the content_object already have a tag for the language taxonomy, it will be skipped. + Create a tag for the language taxonomy in the content_object if it doesn't exist. """ lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) @@ -66,7 +64,7 @@ def update_course_tags(course_key_str: str) -> bool: course = modulestore().get_course(course_key) if course: lang = course.language - _update_tags(course_key, lang) + _set_initial_language_tag(course_key, lang) return True except Exception as e: # pylint: disable=broad-except @@ -117,7 +115,7 @@ def update_xblock_tags(usage_key_str: str) -> bool: else: return True - _update_tags(usage_key, lang) + _set_initial_language_tag(usage_key, lang) return True except Exception as e: # pylint: disable=broad-except From eb9f2c09c66f378722a54e001fd4031bf4652e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 10:27:01 -0300 Subject: [PATCH 40/49] refactor: fix types --- openedx/features/content_tagging/api.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index 0dd46fba4738..bc420b908489 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -1,12 +1,13 @@ """ Content Tagging APIs """ -from typing import Iterator, List, Type, Union +from __future__ import annotations + +from typing import Iterator, List, Type import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import QuerySet -from opaque_keys.edx.keys import LearningContextKey -from opaque_keys.edx.locator import BlockUsageLocator +from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization @@ -117,9 +118,9 @@ def get_content_tags( def tag_content_object( taxonomy: Taxonomy, - tags: List, - object_id: Union[BlockUsageLocator, LearningContextKey], -) -> List[ContentObjectTag]: + tags: list, + object_id: CourseKey | UsageKey, +) -> list[ContentObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or course). From a99b725eeec1286c9f58e587679ae40d47a818e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 12:19:48 -0300 Subject: [PATCH 41/49] fix: remove ContentOrganizationTaxonomy --- .../migrations/0003_system_defined_fixture.py | 8 ++-- .../migrations/0004_system_defined_org.py | 4 +- .../migrations/0005_auto_20230830_1517.py | 19 ++++++++ .../content_tagging/models/__init__.py | 1 - .../content_tagging/models/system_defined.py | 48 ------------------- .../content_tagging/tests/test_models.py | 4 -- 6 files changed, 23 insertions(+), 61 deletions(-) create mode 100644 openedx/features/content_tagging/migrations/0005_auto_20230830_1517.py diff --git a/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py b/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py index 747a5b89f8d9..7846b907c4e6 100644 --- a/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py +++ b/openedx/features/content_tagging/migrations/0003_system_defined_fixture.py @@ -1,11 +1,6 @@ # Generated by Django 3.2.20 on 2023-07-11 22:57 from django.db import migrations -from openedx.features.content_tagging.models import ( - ContentAuthorTaxonomy, - ContentLanguageTaxonomy, - ContentOrganizationTaxonomy, -) def load_system_defined_taxonomies(apps, schema_editor): @@ -25,6 +20,7 @@ def load_system_defined_taxonomies(apps, schema_editor): allow_free_text=False, visible_to_authors=False, ) + ContentAuthorTaxonomy = apps.get_model("content_tagging", "ContentAuthorTaxonomy") author_taxonomy.taxonomy_class = ContentAuthorTaxonomy author_taxonomy.save() @@ -38,11 +34,13 @@ def load_system_defined_taxonomies(apps, schema_editor): allow_free_text=False, visible_to_authors=False, ) + ContentOrganizationTaxonomy = apps.get_model("content_tagging", "ContentOrganizationTaxonomy") org_taxonomy.taxonomy_class = ContentOrganizationTaxonomy org_taxonomy.save() # Adding taxonomy class to the language taxonomy language_taxonomy = Taxonomy.objects.get(id=-1) + ContentLanguageTaxonomy = apps.get_model("content_tagging", "ContentLanguageTaxonomy") language_taxonomy.taxonomy_class = ContentLanguageTaxonomy language_taxonomy.save() diff --git a/openedx/features/content_tagging/migrations/0004_system_defined_org.py b/openedx/features/content_tagging/migrations/0004_system_defined_org.py index ad4d25886843..852d67ae4ab6 100644 --- a/openedx/features/content_tagging/migrations/0004_system_defined_org.py +++ b/openedx/features/content_tagging/migrations/0004_system_defined_org.py @@ -1,7 +1,4 @@ from django.db import migrations -from openedx.features.content_tagging.models import ( - ContentOrganizationTaxonomy, -) def load_system_defined_org_taxonomies(apps, _schema_editor): @@ -37,6 +34,7 @@ def revert_system_defined_org_taxonomies(apps, _schema_editor): allow_free_text=False, visible_to_authors=False, ) + ContentOrganizationTaxonomy = apps.get_model("content_tagging", "ContentOrganizationTaxonomy") org_taxonomy.taxonomy_class = ContentOrganizationTaxonomy org_taxonomy.save() diff --git a/openedx/features/content_tagging/migrations/0005_auto_20230830_1517.py b/openedx/features/content_tagging/migrations/0005_auto_20230830_1517.py new file mode 100644 index 000000000000..6252594e8bff --- /dev/null +++ b/openedx/features/content_tagging/migrations/0005_auto_20230830_1517.py @@ -0,0 +1,19 @@ +# Generated by Django 3.2.20 on 2023-08-30 15:17 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_tagging', '0004_system_defined_org'), + ] + + operations = [ + migrations.DeleteModel( + name='ContentOrganizationTaxonomy', + ), + migrations.DeleteModel( + name='OrganizationModelObjectTag', + ), + ] diff --git a/openedx/features/content_tagging/models/__init__.py b/openedx/features/content_tagging/models/__init__.py index dc748c355b2d..4606c5b85386 100644 --- a/openedx/features/content_tagging/models/__init__.py +++ b/openedx/features/content_tagging/models/__init__.py @@ -9,5 +9,4 @@ from .system_defined import ( ContentLanguageTaxonomy, ContentAuthorTaxonomy, - ContentOrganizationTaxonomy, ) diff --git a/openedx/features/content_tagging/models/system_defined.py b/openedx/features/content_tagging/models/system_defined.py index 642e1c08b03d..9948625455e7 100644 --- a/openedx/features/content_tagging/models/system_defined.py +++ b/openedx/features/content_tagging/models/system_defined.py @@ -1,62 +1,14 @@ """ System defined models """ -from typing import Type - from openedx_tagging.core.tagging.models import ( - ModelSystemDefinedTaxonomy, - ModelObjectTag, UserSystemDefinedTaxonomy, LanguageTaxonomy, ) -from organizations.models import Organization from .base import ContentTaxonomyMixin -class OrganizationModelObjectTag(ModelObjectTag): - """ - ObjectTags for the OrganizationSystemDefinedTaxonomy. - """ - - class Meta: - proxy = True - - @property - def tag_class_model(self) -> Type: - """ - Associate the organization model - """ - return Organization - - @property - def tag_class_value(self) -> str: - """ - Returns the organization name to use it on Tag.value when creating Tags for this taxonomy. - """ - return "name" - - -class ContentOrganizationTaxonomy(ContentTaxonomyMixin, ModelSystemDefinedTaxonomy): - """ - Organization system-defined taxonomy that accepts ContentTags - - Side note: The organization of an object is already encoded in its usage ID, - but a Taxonomy with Organization as Tags is being used so that the objects can be - indexed and can be filtered in the same tagging system, without any special casing. - """ - - class Meta: - proxy = True - - @property - def object_tag_class(self) -> Type: - """ - Returns OrganizationModelObjectTag as ObjectTag subclass associated with this taxonomy. - """ - return OrganizationModelObjectTag - - class ContentLanguageTaxonomy(ContentTaxonomyMixin, LanguageTaxonomy): """ Language system-defined taxonomy that accepts ContentTags diff --git a/openedx/features/content_tagging/tests/test_models.py b/openedx/features/content_tagging/tests/test_models.py index a0b358ea3131..81c5da86413c 100644 --- a/openedx/features/content_tagging/tests/test_models.py +++ b/openedx/features/content_tagging/tests/test_models.py @@ -12,7 +12,6 @@ from ..models import ( ContentLanguageTaxonomy, ContentAuthorTaxonomy, - ContentOrganizationTaxonomy, ) @@ -29,9 +28,6 @@ class TestSystemDefinedModels(TestCase): (ContentAuthorTaxonomy, "taxonomy"), # Invalid object key (ContentAuthorTaxonomy, "tag"), # Invalid external_id, User don't exits (ContentAuthorTaxonomy, "object"), # Invalid object key - (ContentOrganizationTaxonomy, "taxonomy"), # Invalid object key - (ContentOrganizationTaxonomy, "tag"), # Invalid external_id, Organization don't exits - (ContentOrganizationTaxonomy, "object"), # Invalid object key ) @ddt.unpack def test_validations( From f174ecefae486ee2fc746426d8b1ef36c7357031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 12:50:53 -0300 Subject: [PATCH 42/49] feat: use default language if course.language not found --- openedx/features/content_tagging/tasks.py | 29 +++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/openedx/features/content_tagging/tasks.py b/openedx/features/content_tagging/tasks.py index d00c1c986945..a328634f0735 100644 --- a/openedx/features/content_tagging/tasks.py +++ b/openedx/features/content_tagging/tasks.py @@ -7,6 +7,7 @@ from celery import shared_task from celery_utils.logged_task import LoggedTask +from django.conf import settings from django.contrib.auth import get_user_model from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Taxonomy @@ -31,15 +32,39 @@ def _has_taxonomy(taxonomy: Taxonomy, content_object: CourseKey | UsageKey) -> b return next(content_tags, _exausted) is not _exausted -def _set_initial_language_tag(content_object: CourseKey | UsageKey, lang) -> None: +def _set_initial_language_tag(content_object: CourseKey | UsageKey, lang: str) -> None: """ Create a tag for the language taxonomy in the content_object if it doesn't exist. + + If the language is not configured in the plataform or the language tag doesn't exist, + use the default language of the platform. """ lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) if lang and not _has_taxonomy(lang_taxonomy, content_object): tags = api.get_tags(lang_taxonomy) - lang_tag = next(tag for tag in tags if tag.external_id == lang) + is_language_configured = any(lang_code == lang for lang_code, _ in settings.LANGUAGES) is not None + if not is_language_configured: + logging.warning( + "Language not configured in the plataform: %s. Using default language: %s", + lang, + settings.LANGUAGE_CODE, + ) + lang = settings.LANGUAGE_CODE + + lang_tag = next((tag for tag in tags if tag.external_id == lang), None) + if lang_tag is None: + if not is_language_configured: + logging.error( + "Language tag not found for default language: %s. Skipping", lang + ) + return + + logging.warning( + "Language tag not found for language: %s. Using default language: %s", lang, settings.LANGUAGE_CODE + ) + lang_tag = next(tag for tag in tags if tag.external_id == settings.LANGUAGE_CODE) + api.tag_content_object(lang_taxonomy, [lang_tag.id], content_object) From 2ccdeb54f8facbb96da5905ae92e7dfaee4e0038 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 14:44:40 -0300 Subject: [PATCH 43/49] Revert "test: add replicaset config for lms" This reverts commit 302149233b7349441926fbbc8c796fad4409ce46. --- lms/envs/test.py | 1 - 1 file changed, 1 deletion(-) diff --git a/lms/envs/test.py b/lms/envs/test.py index 6dd7ecc27d80..284ebc915d47 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -167,7 +167,6 @@ 'port': MONGO_PORT_NUM, 'db': f'test_xmodule_{THIS_UUID}', 'collection': 'test_modulestore', - 'replicaSet': None, }, ) From 1c27f65dbd969e76b61e2e661442cd81900d1535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 15:52:59 -0300 Subject: [PATCH 44/49] feat: add CONTENT_TAGGING_AUTO WaffleSwitch --- openedx/features/content_tagging/handlers.py | 10 ++++++++++ .../content_tagging/tests/test_tasks.py | 5 +++-- openedx/features/content_tagging/toggles.py | 17 +++++++++++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 openedx/features/content_tagging/toggles.py diff --git a/openedx/features/content_tagging/handlers.py b/openedx/features/content_tagging/handlers.py index bf0048ca569b..608a50aea72e 100644 --- a/openedx/features/content_tagging/handlers.py +++ b/openedx/features/content_tagging/handlers.py @@ -14,6 +14,7 @@ update_course_tags, update_xblock_tags ) +from .toggles import CONTENT_TAGGING_AUTO log = logging.getLogger(__name__) @@ -23,6 +24,9 @@ def auto_tag_course(**kwargs): """ Automatically tag course based on their metadata """ + if not CONTENT_TAGGING_AUTO.is_enabled(): + return + course_data = kwargs.get("course", None) if not course_data or not isinstance(course_data, CourseData): log.error("Received null or incorrect data for event") @@ -37,6 +41,9 @@ def auto_tag_xblock(**kwargs): """ Automatically tag XBlock based on their metadata """ + if not CONTENT_TAGGING_AUTO.is_enabled(): + return + xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") @@ -54,6 +61,9 @@ def delete_tag_xblock(**kwargs): """ Automatically delete XBlock auto tags. """ + if not CONTENT_TAGGING_AUTO.is_enabled(): + return + xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index a0fa0a4c868f..2da98357a225 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -7,6 +7,7 @@ from unittest.mock import patch from django.core.management import call_command +from edx_toggles.toggles.testutils import override_waffle_switch from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization @@ -16,11 +17,13 @@ from .. import api from ..models import ContentLanguageTaxonomy, TaxonomyOrg +from ..toggles import CONTENT_TAGGING_AUTO LANGUAGE_TAXONOMY_ID = -1 @skip_unless_cms # Auto-tagging is only available in the CMS +@override_waffle_switch(CONTENT_TAGGING_AUTO, active=True) class TestAutoTagging(ModuleStoreTestCase): """ Test if the Course and XBlock tags are automatically created @@ -71,7 +74,6 @@ def setUp(self): def test_create_course(self): # Create course - logging.warning("Creating course") course = self.store.create_course( self.orgA.short_name, "test_course", @@ -79,7 +81,6 @@ def test_create_course(self): self.user_id, fields={"language": "pt"}, ) - logging.warning("Creating course: done") # Check if the tags are created in the Course assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") diff --git a/openedx/features/content_tagging/toggles.py b/openedx/features/content_tagging/toggles.py new file mode 100644 index 000000000000..458f94a930da --- /dev/null +++ b/openedx/features/content_tagging/toggles.py @@ -0,0 +1,17 @@ + +""" +Toggles for content tagging +""" + +from edx_toggles.toggles import WaffleSwitch + +# .. toggle_name: content_tagging.auto +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Setting this enables automatic tagging of content +# .. toggle_type: feature_flag +# .. toggle_category: admin +# .. 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__) From 52009661b07ef3479a8d34572ff1e356c4993e56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 15:54:57 -0300 Subject: [PATCH 45/49] revert: revert changes in test_mixed_modulestore --- .../tests/test_mixed_modulestore.py | 37 ++++++------------- 1 file changed, 12 insertions(+), 25 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index d1b9f679e326..7e35217a1064 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -100,7 +100,6 @@ class CommonMixedModuleStoreSetup(CourseComparisonTest, OpenEdxEventsTestMixin): 'db': DB, 'collection': COLLECTION, 'asset_collection': ASSET_COLLECTION, - 'replicaSet': None, } OPTIONS = { 'stores': [ @@ -270,13 +269,8 @@ def _initialize_mixed(self, mappings=None, contentstore=None): mappings=mappings, **self.options ) - self.addCleanup(self.store.close_all_connections) - patcher = patch("openedx.features.content_tagging.tasks.modulestore", return_value=self.store) - self.addCleanup(patcher.stop) - patcher.start() - def initdb(self, default): """ Initialize the database and create one test course in it @@ -547,11 +541,10 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select (by course_id), update, update historical record, - # XBLOCK_UPDATED handler call - # find: definitions (calculator field), structures, XBLOCK_UPDATED handler call + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 5), (ModuleStoreEnum.Type.split, 6, 4, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1076,17 +1069,15 @@ def test_has_changes_missing_child(self, default_ms, default_branch): assert self.store.has_changes(parent) # Draft - # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # Find: find parents (definition.children query), get parent, get course (fill in run?), # find parents of the parent (course), get inheritance items, # get item (to delete subtree), get inheritance again. # Sends: delete item, update parent # Split - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 5, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1108,16 +1099,14 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): self.store.get_item(self.writable_chapter_location, revision=ModuleStoreEnum.RevisionOption.published_only) # Draft: - # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: find parent (definition.children), count versions of item, get parent, count grandparents, # inheritance items, draft item, draft child, inheritance # sends: delete draft vertical and update parent # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: draft and published structures, definition (unnecessary) # sends: update published (why?), draft, and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 2), (ModuleStoreEnum.Type.split, 5, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 8, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1165,15 +1154,13 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send assert vert_loc not in course.children # Draft: - # mysql: delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, - # delete oel_tagging_objecttag from XBLOCK_DELETED handler in Content Tagging + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record # find: structure (cached) # send: update structure and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 1), (ModuleStoreEnum.Type.split, 5, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1216,7 +1203,7 @@ def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): # executed twice, possibly unnecessarily) # find: 2 reads of structure, definition (s/b lazy; so, unnecessary), # plus 1 wildcard find in draft mongo which has none - @ddt.data((ModuleStoreEnum.Type.mongo, 1, 2, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 0), (ModuleStoreEnum.Type.split, 2, 3, 0)) @ddt.unpack def test_get_courses(self, default_ms, num_mysql, max_find, max_send): self.initdb(default_ms) @@ -1256,7 +1243,7 @@ def test_create_child_detached_tabs(self, default_ms): # draft is 2: find out which ms owns course, get item # split: active_versions (mysql), structure, definition (to load course wiki string) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 2, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 0), (ModuleStoreEnum.Type.split, 1, 2, 0)) @ddt.unpack def test_get_course(self, default_ms, num_mysql, max_find, max_send): """ @@ -2051,7 +2038,7 @@ def _get_split_modulestore(self): # Draft: get all items which can be or should have parents # Split: active_versions (mysql), structure (mongo) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 0, 1, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 0, 1, 0), (ModuleStoreEnum.Type.split, 1, 1, 0)) @ddt.unpack def test_get_orphans(self, default_ms, num_mysql, max_find, max_send): """ From 2612fe93f3e8d272252776d34f54d4512e76a4f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 30 Aug 2023 16:13:41 -0300 Subject: [PATCH 46/49] style: remove unused import --- openedx/features/content_tagging/tests/test_tasks.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 2da98357a225..8724f4f8ea2f 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -3,7 +3,6 @@ """ from __future__ import annotations -import logging from unittest.mock import patch from django.core.management import call_command From 5fc811d77b205df79181103c3d9df21b7cb62d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 31 Aug 2023 08:51:51 -0300 Subject: [PATCH 47/49] test: add invalid tags tests --- .../content_tagging/tests/test_tasks.py | 63 ++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 8724f4f8ea2f..7cc9a21e2737 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -6,8 +6,9 @@ from unittest.mock import patch from django.core.management import call_command +from django.test import override_settings from edx_toggles.toggles.testutils import override_waffle_switch -from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from organizations.models import Organization from common.djangoapps.student.tests.factories import UserFactory @@ -84,6 +85,66 @@ def test_create_course(self): # Check if the tags are created in the Course assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + @override_settings(LANGUAGE_CODE='pt') + def test_create_course_invalid_language(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "11"}, + ) + + # Check if the tags are created in the Course is the system default + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + + @override_settings(LANGUAGES=[('pt', 'Portuguese')], LANGUAGE_CODE='pt') + def test_create_course_unsuported_language(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "en"}, + ) + + # Check if the tags are created in the Course is the system default + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + + @override_settings(LANGUAGE_CODE='pt') + def test_create_course_no_tag_language(self): + # Remove English tag + Tag.objects.filter(taxonomy_id=LANGUAGE_TAXONOMY_ID, value="English").delete() + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "en"}, + ) + + # Check if the tags are created in the Course is the system default + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, "Portuguese") + + @override_settings(LANGUAGE_CODE='pt') + def test_create_course_no_tag_default_language(self): + # Remove Portuguese tag + Tag.objects.filter(taxonomy_id=LANGUAGE_TAXONOMY_ID, value="Portuguese").delete() + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "11"}, + ) + + # No tags created + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) + def test_update_course(self): # Create course course = self.store.create_course( From 154a4aec8b6117a8701af74ec216f086f023bdd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 31 Aug 2023 09:53:50 -0300 Subject: [PATCH 48/49] refactor: change WaffleSwitch to CourseWaffleFlag --- openedx/features/content_tagging/handlers.py | 18 +++---- .../content_tagging/tests/test_tasks.py | 54 +++++++++++++++++-- openedx/features/content_tagging/toggles.py | 4 +- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/openedx/features/content_tagging/handlers.py b/openedx/features/content_tagging/handlers.py index 608a50aea72e..211c8f4fbf5c 100644 --- a/openedx/features/content_tagging/handlers.py +++ b/openedx/features/content_tagging/handlers.py @@ -24,14 +24,14 @@ def auto_tag_course(**kwargs): """ Automatically tag course based on their metadata """ - if not CONTENT_TAGGING_AUTO.is_enabled(): - return - course_data = kwargs.get("course", None) if not course_data or not isinstance(course_data, CourseData): log.error("Received null or incorrect data for event") return + if not CONTENT_TAGGING_AUTO.is_enabled(course_data.course_key): + return + update_course_tags.delay(str(course_data.course_key)) @@ -41,14 +41,14 @@ def auto_tag_xblock(**kwargs): """ Automatically tag XBlock based on their metadata """ - if not CONTENT_TAGGING_AUTO.is_enabled(): - return - xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") return + if not CONTENT_TAGGING_AUTO.is_enabled(xblock_info.usage_key.course_key): + return + if xblock_info.block_type == "course": # Course update is handled by XBlock of course type update_course_tags.delay(str(xblock_info.usage_key.course_key)) @@ -61,14 +61,14 @@ def delete_tag_xblock(**kwargs): """ Automatically delete XBlock auto tags. """ - if not CONTENT_TAGGING_AUTO.is_enabled(): - return - xblock_info = kwargs.get("xblock_info", None) if not xblock_info or not isinstance(xblock_info, XBlockData): log.error("Received null or incorrect data for event") return + if not CONTENT_TAGGING_AUTO.is_enabled(xblock_info.usage_key.course_key): + return + if xblock_info.block_type == "course": # Course deletion is handled by XBlock of course type delete_course_tags.delay(str(xblock_info.usage_key.course_key)) diff --git a/openedx/features/content_tagging/tests/test_tasks.py b/openedx/features/content_tagging/tests/test_tasks.py index 7cc9a21e2737..47bf864951f6 100644 --- a/openedx/features/content_tagging/tests/test_tasks.py +++ b/openedx/features/content_tagging/tests/test_tasks.py @@ -7,7 +7,7 @@ from django.core.management import call_command from django.test import override_settings -from edx_toggles.toggles.testutils import override_waffle_switch +from edx_toggles.toggles.testutils import override_waffle_flag from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from organizations.models import Organization @@ -23,7 +23,7 @@ @skip_unless_cms # Auto-tagging is only available in the CMS -@override_waffle_switch(CONTENT_TAGGING_AUTO, active=True) +@override_waffle_flag(CONTENT_TAGGING_AUTO, active=True) class TestAutoTagging(ModuleStoreTestCase): """ Test if the Course and XBlock tags are automatically created @@ -39,7 +39,8 @@ def _check_tag(self, object_id: str, taxonomy_id: int, value: str | None): """ object_tag = ObjectTag.objects.filter(object_id=object_id, taxonomy_id=taxonomy_id).first() if value is None: - assert not object_tag, f"Expected no tag for taxonomy_id={taxonomy_id}, but one found with value={value}" + assert not object_tag, f"Expected no tag for taxonomy_id={taxonomy_id}, " \ + f"but one found with value={object_tag.value}" else: assert object_tag, f"Tag for taxonomy_id={taxonomy_id} with value={value} with expected, but none found" assert object_tag.value == value, f"Tag value mismatch {object_tag.value} != {value}" @@ -190,3 +191,50 @@ def test_create_delete_xblock(self): # Check if the tags are deleted assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) + + @override_waffle_flag(CONTENT_TAGGING_AUTO, active=False) + def test_waffle_disabled_create_update_course(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "pt"}, + ) + + # No tags created + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) + + # Update course language + course.language = "en" + self.store.update_item(course, self.user_id) + + # No tags created + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) + + @override_waffle_flag(CONTENT_TAGGING_AUTO, active=False) + def test_waffle_disabled_create_delete_xblock(self): + # Create course + course = self.store.create_course( + self.orgA.short_name, + "test_course", + "test_run", + self.user_id, + fields={"language": "pt"}, + ) + + # Create XBlocks + sequential = self.store.create_child(self.user_id, course.location, "sequential", "test_sequential") + vertical = self.store.create_child(self.user_id, sequential.location, "vertical", "test_vertical") + + usage_key_str = str(vertical.location) + + # No tags created + assert self._check_tag(course.id, LANGUAGE_TAXONOMY_ID, None) + + # Delete the XBlock + self.store.delete_item(vertical.location, self.user_id) + + # Still no tags + assert self._check_tag(usage_key_str, LANGUAGE_TAXONOMY_ID, None) diff --git a/openedx/features/content_tagging/toggles.py b/openedx/features/content_tagging/toggles.py index 458f94a930da..30a21cf77e51 100644 --- a/openedx/features/content_tagging/toggles.py +++ b/openedx/features/content_tagging/toggles.py @@ -3,7 +3,7 @@ Toggles for content tagging """ -from edx_toggles.toggles import WaffleSwitch +from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag # .. toggle_name: content_tagging.auto # .. toggle_implementation: WaffleSwitch @@ -14,4 +14,4 @@ # .. 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__) +CONTENT_TAGGING_AUTO = CourseWaffleFlag('content_tagging.auto', __name__) From 1a1b63417ca95a1e158539037e6fb52de2f0ce90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 31 Aug 2023 11:34:48 -0300 Subject: [PATCH 49/49] test: fix query count --- .../tests/test_mixed_modulestore.py | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 7e35217a1064..a7b31d2b29ae 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -538,13 +538,15 @@ def test_get_items_include_orphans(self, default_ms, expected_items_in_tree, orp assert len(items_in_tree) == expected_items_in_tree # draft: + # mysql: check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: get draft, get ancestors up to course (2-6), compute inheritance # sends: update problem and then each ancestor up to course (edit info) # split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: definitions (calculator field), structures # sends: 2 sends to update index & structure (note, it would also be definition if a content field changed) - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 5), (ModuleStoreEnum.Type.split, 3, 2, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 5), (ModuleStoreEnum.Type.split, 4, 2, 2)) @ddt.unpack def test_update_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1069,15 +1071,17 @@ def test_has_changes_missing_child(self, default_ms, default_branch): assert self.store.has_changes(parent) # Draft + # mysql: check CONTENT_TAGGING_AUTO CourseWaffleFlag # Find: find parents (definition.children query), get parent, get course (fill in run?), # find parents of the parent (course), get inheritance items, # get item (to delete subtree), get inheritance again. # Sends: delete item, update parent # Split - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # check CONTENT_TAGGING_AUTO CourseWaffleFlag # Find: active_versions, 2 structures (published & draft), definition (unnecessary) # Sends: updated draft and published structures and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 6, 2), (ModuleStoreEnum.Type.split, 4, 2, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 6, 2), (ModuleStoreEnum.Type.split, 5, 2, 3)) @ddt.unpack def test_delete_item(self, default_ms, num_mysql, max_find, max_send): """ @@ -1099,14 +1103,16 @@ def test_delete_item(self, default_ms, num_mysql, max_find, max_send): self.store.get_item(self.writable_chapter_location, revision=ModuleStoreEnum.RevisionOption.published_only) # Draft: + # mysql: check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: find parent (definition.children), count versions of item, get parent, count grandparents, # inheritance items, draft item, draft child, inheritance # sends: delete draft vertical and update parent # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: draft and published structures, definition (unnecessary) # sends: update published (why?), draft, and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 8, 2), (ModuleStoreEnum.Type.split, 4, 3, 3)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 8, 2), (ModuleStoreEnum.Type.split, 5, 3, 3)) @ddt.unpack def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send): """ @@ -1154,13 +1160,15 @@ def test_delete_private_vertical(self, default_ms, num_mysql, max_find, max_send assert vert_loc not in course.children # Draft: + # mysql: check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: find parent (definition.children) 2x, find draft item, get inheritance items # send: one delete query for specific item # Split: - # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record + # mysql: SplitModulestoreCourseIndex - select 2x (by course_id, by objectid), update, update historical record, + # check CONTENT_TAGGING_AUTO CourseWaffleFlag # find: structure (cached) # send: update structure and active_versions - @ddt.data((ModuleStoreEnum.Type.mongo, 0, 3, 1), (ModuleStoreEnum.Type.split, 4, 1, 2)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 3, 1), (ModuleStoreEnum.Type.split, 5, 1, 2)) @ddt.unpack def test_delete_draft_vertical(self, default_ms, num_mysql, max_find, max_send): """