feat: tagging: serialize object permissions to REST API (#34004)

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).

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"]

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):

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)
     )
 
 

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

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

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

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

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