diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 2a64f77b74f9..70aa7e2150da 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -3,8 +3,6 @@ """ from __future__ import annotations -from typing import Iterator - import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Q, QuerySet, Exists, OuterRef from openedx_tagging.core.tagging.models import Taxonomy @@ -101,7 +99,7 @@ def get_taxonomies_for_org( return oel_tagging.get_taxonomies(enabled=enabled).filter( Exists( TaxonomyOrg.get_relationships( - taxonomy=OuterRef("pk"), + taxonomy=OuterRef("pk"), # type: ignore rel_type=TaxonomyOrg.RelType.OWNER, org_short_name=org_short_name, ) @@ -130,7 +128,7 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: def get_content_tags( object_key: ContentKey, taxonomy_id: int | None = None, -) -> Iterator[ContentObjectTag]: +) -> QuerySet: """ Generates a list of content tags for a given object. @@ -147,7 +145,7 @@ def tag_content_object( object_key: ContentKey, taxonomy: Taxonomy, tags: list, -) -> Iterator[ContentObjectTag]: +) -> QuerySet: """ 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/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 3ff0623eb851..20b1deb661b7 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -6,6 +6,7 @@ from urllib.parse import parse_qs, urlparse import json +from unittest.mock import MagicMock import abc import ddt @@ -33,6 +34,7 @@ create_library, set_library_user_permissions, ) +from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api @@ -192,7 +194,7 @@ def _setUp_taxonomies(self): rel_type=TaxonomyOrg.RelType.OWNER, ) - # Global taxonomy + # Global taxonomy, which contains tags self.t1 = Taxonomy.objects.create(name="t1", enabled=True) TaxonomyOrg.objects.create( taxonomy=self.t1, @@ -203,6 +205,12 @@ def _setUp_taxonomies(self): taxonomy=self.t2, rel_type=TaxonomyOrg.RelType.OWNER, ) + root1 = Tag.objects.create(taxonomy=self.t1, value="ALPHABET") + Tag.objects.create(taxonomy=self.t1, value="android", parent=root1) + Tag.objects.create(taxonomy=self.t1, value="abacus", parent=root1) + Tag.objects.create(taxonomy=self.t1, value="azure", parent=root1) + Tag.objects.create(taxonomy=self.t1, value="aardvark", parent=root1) + Tag.objects.create(taxonomy=self.t1, value="anvil", parent=root1) # OrgA taxonomy self.tA1 = Taxonomy.objects.create(name="tA1", enabled=True) @@ -278,7 +286,8 @@ def _test_list_taxonomy( expected_taxonomies: list[str], enabled_parameter: bool | None = None, org_parameter: str | None = None, - unassigned_parameter: bool | None = None + unassigned_parameter: bool | None = None, + page_size: int | None = None, ) -> None: """ Helper function to call the list endpoint and check the response @@ -293,6 +302,7 @@ def _test_list_taxonomy( "enabled": enabled_parameter, "org": org_parameter, "unassigned": unassigned_parameter, + "page_size": page_size, }.items() if v is not None} response = self.client.get(url, query_params, format="json") @@ -304,11 +314,12 @@ def test_list_taxonomy_staff(self) -> None: """ Tests that staff users see all taxonomies """ - # Default page_size=10, and so "tBA1" and "tBA2" appear on the second page + # page_size=10, and so "tBA1" and "tBA2" appear on the second page expected_taxonomies = ["ot1", "ot2", "st1", "st2", "t1", "t2", "tA1", "tA2", "tB1", "tB2"] self._test_list_taxonomy( user_attr="staff", expected_taxonomies=expected_taxonomies, + page_size=10, ) @ddt.data( @@ -476,6 +487,29 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: if user_attr == "staffA": assert response.data["orgs"] == [self.orgA.short_name] + def test_list_taxonomy_query_count(self): + """ + Test how many queries are used when retrieving taxonomies and permissions + """ + url = TAXONOMY_ORG_LIST_URL + f'?org=${self.orgA.short_name}&enabled=true' + + self.client.force_authenticate(user=self.staff) + with self.assertNumQueries(16): # TODO Why so many queries? + response = self.client.get(url) + + assert response.status_code == 200 + assert response.data["can_add_taxonomy"] + assert len(response.data["results"]) == 2 + for taxonomy in response.data["results"]: + if taxonomy["system_defined"]: + assert not taxonomy["can_change_taxonomy"] + assert not taxonomy["can_delete_taxonomy"] + assert taxonomy["can_tag_object"] + else: + assert taxonomy["can_change_taxonomy"] + assert taxonomy["can_delete_taxonomy"] + assert taxonomy["can_tag_object"] + @ddt.ddt class TestTaxonomyDetailExportMixin(TestTaxonomyObjectsMixin): @@ -787,7 +821,14 @@ def _test_api_call(self, **kwargs) -> None: assert response.status_code == expected_status, reason if status.is_success(expected_status): - check_taxonomy(response.data, taxonomy.pk, **(TaxonomySerializer(taxonomy.cast()).data)) + request = MagicMock() + request.user = user + context = {"request": request} + check_taxonomy( + response.data, + taxonomy.pk, + **(TaxonomySerializer(taxonomy.cast(), context=context)).data, + ) @skip_unless_cms @@ -1538,12 +1579,12 @@ def test_get_tags(self): # Fetch this object's tags for a single taxonomy expected_tags = [{ - 'editable': True, 'name': 'Multiple Taxonomy', 'taxonomy_id': taxonomy.pk, + 'can_tag_object': True, 'tags': [ - {'value': 'Tag 1', 'lineage': ['Tag 1']}, - {'value': 'Tag 2', 'lineage': ['Tag 2']}, + {'value': 'Tag 1', 'lineage': ['Tag 1'], 'can_delete_objecttag': True}, + {'value': 'Tag 2', 'lineage': ['Tag 2'], 'can_delete_objecttag': True}, ], }] @@ -1560,6 +1601,28 @@ def test_get_tags(self): assert status.is_success(response3.status_code) assert response3.data[str(self.courseA)]["taxonomies"] == expected_tags + def test_object_tags_query_count(self): + """ + Test how many queries are used when retrieving object tags and permissions + """ + object_key = self.courseA + object_id = str(object_key) + tagging_api.tag_content_object(object_key=object_key, taxonomy=self.t1, tags=["anvil", "android"]) + expected_tags = [ + {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": True}, + {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": True}, + ] + + url = OBJECT_TAGS_URL.format(object_id=object_id) + self.client.force_authenticate(user=self.staff) + with self.assertNumQueries(7): # TODO Why so many queries? + response = self.client.get(url) + + assert response.status_code == 200 + assert len(response.data[object_id]["taxonomies"]) == 1 + assert response.data[object_id]["taxonomies"][0]["can_tag_object"] + assert response.data[object_id]["taxonomies"][0]["tags"] == expected_tags + @skip_unless_cms @ddt.ddt @@ -2029,3 +2092,27 @@ def test_import_no_perm(self) -> None: assert len(tags) == len(self.old_tags) for i, tag in enumerate(tags): assert tag["value"] == self.old_tags[i].value + + +@skip_unless_cms +@ddt.ddt +class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase): + """ + Test cases for TaxonomyTagsViewSet retrive action. + """ + def test_taxonomy_tags_query_count(self): + """ + Test how many queries are used when retrieving small taxonomies+tags and permissions + """ + url = f"{TAXONOMY_TAGS_URL}?search_term=an&parent_tag=ALPHABET".format(pk=self.t1.id) + + self.client.force_authenticate(user=self.staff) + with self.assertNumQueries(13): # TODO Why so many queries? + response = self.client.get(url) + + assert response.status_code == status.HTTP_200_OK + assert response.data["can_add_tag"] + assert len(response.data["results"]) == 2 + for taxonomy in response.data["results"]: + assert taxonomy["can_change_tag"] + assert taxonomy["can_delete_tag"] diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index f04256f4a1a2..151bc09f5d76 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -81,11 +81,11 @@ def perform_create(self, serializer): serializer.instance = create_taxonomy(**serializer.validated_data, orgs=user_admin_orgs) @action(detail=False, url_path="import", methods=["post"]) - def create_import(self, request: Request, **kwargs) -> Response: + def create_import(self, request: Request, **kwargs) -> Response: # type: ignore """ Creates a new taxonomy with the given orgs and imports the tags from the uploaded file. """ - response = super().create_import(request, **kwargs) + response = super().create_import(request=request, **kwargs) # type: ignore # If creation was successful, set the orgs for the new taxonomy if status.is_success(response.status_code): diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index f3d0b70c3093..af6bdbeb9435 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -219,7 +219,7 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: Everyone that has permission to edit the object should be able to tag it. """ if not object_id: - raise ValueError("object_id must be provided") + return True try: usage_key = UsageKey.from_string(object_id) if not usage_key.course_key.is_course: @@ -274,7 +274,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) return oel_tagging.is_taxonomy_admin(user) and ( not tag or not taxonomy - or (taxonomy and not taxonomy.allow_free_text and not taxonomy.system_defined) + or (bool(taxonomy) and not taxonomy.allow_free_text and not taxonomy.system_defined) ) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 453c040c2071..f0b2dd767fa3 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -103,7 +103,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.4.2 +openedx-learning==0.4.4 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 20a692289525..4731fcfed665 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -779,7 +779,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.4.2 +openedx-learning==0.4.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 65927c3a8e28..50b40cf84993 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1311,7 +1311,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.4.2 +openedx-learning==0.4.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 154c2d677555..677a299aee52 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -921,7 +921,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.4.2 +openedx-learning==0.4.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 21cf769c8300..4358a89d7d9b 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -981,7 +981,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.4.2 +openedx-learning==0.4.4 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt