diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 0df9e470035a..d59dcfafc173 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -84,7 +84,7 @@ def set_taxonomy_orgs( def get_taxonomies_for_org( enabled=True, - org_owner: Organization | None = None, + org_short_name: str | None = None, ) -> QuerySet: """ Generates a list of the enabled Taxonomies available for the given org, sorted by name. @@ -97,7 +97,6 @@ def get_taxonomies_for_org( If you want the disabled Taxonomies, pass enabled=False. If you want all Taxonomies (both enabled and disabled), pass enabled=None. """ - org_short_name = org_owner.short_name if org_owner else None return oel_tagging.get_taxonomies(enabled=enabled).filter( Exists( TaxonomyOrg.get_relationships( diff --git a/openedx/core/djangoapps/content_tagging/models/base.py b/openedx/core/djangoapps/content_tagging/models/base.py index 77061e46118a..2880fb848874 100644 --- a/openedx/core/djangoapps/content_tagging/models/base.py +++ b/openedx/core/djangoapps/content_tagging/models/base.py @@ -67,19 +67,24 @@ def get_relationships( @classmethod def get_organizations( - cls, taxonomy: Taxonomy, rel_type: RelType - ) -> list[Organization]: + cls, taxonomy: Taxonomy, rel_type=RelType.OWNER, + ) -> tuple[bool, list[Organization]]: """ - Returns the list of Organizations which have the given relationship to the taxonomy. + Returns a tuple containing: + * bool: flag indicating whether "all organizations" have the given relationship to the taxonomy + * orgs: list of Organizations which have the given relationship to the taxonomy """ - rels = cls.objects.filter( - taxonomy=taxonomy, - rel_type=rel_type, - ) - # A relationship with org=None means all Organizations - if rels.filter(org=None).exists(): - return list(Organization.objects.all()) - return [rel.org for rel in rels] + is_all_org = False + orgs = [] + # Iterate over the taxonomyorgs instead of filtering to take advantage of prefetched data. + for taxonomy_org in taxonomy.taxonomyorg_set.all(): + if taxonomy_org.rel_type == rel_type: + if taxonomy_org.org is None: + is_all_org = True + else: + orgs.append(taxonomy_org.org) + + return (is_all_org, orgs) class ContentObjectTag(ObjectTag): diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 12433f8a381b..3e4d1a047a10 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -4,7 +4,6 @@ from __future__ import annotations -from django.core.exceptions import ObjectDoesNotExist from rest_framework import serializers, fields from openedx_tagging.core.tagging.rest_api.v1.serializers import ( @@ -14,26 +13,7 @@ from organizations.models import Organization - -class OptionalSlugRelatedField(serializers.SlugRelatedField): - """ - Modifies the DRF serializer SlugRelatedField. - - Non-existent slug values are represented internally as an empty queryset, instead of throwing a validation error. - """ - - def to_internal_value(self, data): - """ - Returns the object related to the given slug value, or an empty queryset if not found. - """ - - queryset = self.get_queryset() - try: - return queryset.get(**{self.slug_field: data}) - except ObjectDoesNotExist: - return queryset.none() - except (TypeError, ValueError): - self.fail('invalid') +from ...models import TaxonomyOrg class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): @@ -41,13 +21,22 @@ class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): Serializer for the query params for the GET view """ - org: fields.Field = OptionalSlugRelatedField( - slug_field="short_name", - queryset=Organization.objects.all(), + org: fields.Field = serializers.CharField( required=False, ) unassigned: fields.Field = serializers.BooleanField(required=False) + def validate(self, attrs: dict) -> dict: + """ + Validate the serializer data + """ + if "org" in attrs and "unassigned" in attrs: + raise serializers.ValidationError( + "'org' and 'unassigned' params cannot be both defined" + ) + + return attrs + class TaxonomyUpdateOrgBodySerializer(serializers.Serializer): """ @@ -86,14 +75,21 @@ class TaxonomyOrgSerializer(TaxonomySerializer): def get_orgs(self, obj) -> list[str]: """ Return the list of orgs for the taxonomy. - """ - return [taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all() if taxonomy_org.org] + """ + return [ + taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all() + if taxonomy_org.org and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER + ] def get_all_orgs(self, obj) -> bool: """ Return True if the taxonomy is associated with all orgs. """ - return obj.taxonomyorg_set.filter(org__isnull=True).exists() + is_all_orgs = False + for taxonomy_org in obj.taxonomyorg_set.all(): + if taxonomy_org.org_id is None and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER: + return True + return False class Meta: model = TaxonomySerializer.Meta.model 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 f65be2cd8c16..983a24edf83c 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 @@ -494,15 +494,15 @@ 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' + 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? + with self.assertNumQueries(11): response = self.client.get(url) assert response.status_code == 200 assert response.data["can_add_taxonomy"] - assert len(response.data["results"]) == 2 + assert len(response.data["results"]) == 4 for taxonomy in response.data["results"]: if taxonomy["system_defined"]: assert not taxonomy["can_change_taxonomy"] 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 151bc09f5d76..a1a1a318014d 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -5,7 +5,7 @@ from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status from rest_framework.decorators import action -from rest_framework.exceptions import PermissionDenied, ValidationError +from rest_framework.exceptions import PermissionDenied from rest_framework.request import Request from rest_framework.response import Response @@ -56,13 +56,8 @@ def get_queryset(self): query_params = TaxonomyOrgListQueryParamsSerializer(data=self.request.query_params.dict()) query_params.is_valid(raise_exception=True) enabled = query_params.validated_data.get("enabled", None) - unassigned = query_params.validated_data.get("unassigned", None) org = query_params.validated_data.get("org", None) - # Raise an error if both "org" and "unassigned" query params were provided - if "org" in query_params.validated_data and "unassigned" in query_params.validated_data: - raise ValidationError("'org' and 'unassigned' params cannot be both defined") - # If org filtering was requested, then use it, even if the org is invalid/None if "org" in query_params.validated_data: queryset = get_taxonomies_for_org(enabled, org) @@ -71,7 +66,8 @@ def get_queryset(self): else: queryset = get_taxonomies(enabled) - return queryset.prefetch_related("taxonomyorg_set") + # Prefetch tag_set so we can serialize the tag counts + return queryset.prefetch_related("taxonomyorg_set__org", "tag_set") def perform_create(self, serializer): """ diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index af6bdbeb9435..0fd958d1873d 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -141,21 +141,12 @@ def can_view_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if oel_tagging.is_taxonomy_admin(user): return True - is_all_org = TaxonomyOrg.objects.filter( - taxonomy=taxonomy, - org=None, - rel_type=TaxonomyOrg.RelType.OWNER, - ).exists() + is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(taxonomy) # Enabled all-org taxonomies can be viewed by any registred user if is_all_org: return taxonomy.enabled - taxonomy_orgs = TaxonomyOrg.get_organizations( - taxonomy=taxonomy, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - # Org-level staff can view any taxonomy that is associated with one of their orgs. if is_org_admin(user, taxonomy_orgs): return True @@ -191,21 +182,12 @@ def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if oel_tagging.is_taxonomy_admin(user): return True - is_all_org = TaxonomyOrg.objects.filter( - taxonomy=taxonomy, - org=None, - rel_type=TaxonomyOrg.RelType.OWNER, - ).exists() + is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(taxonomy) # Only taxonomy admins can edit all org taxonomies if is_all_org: return False - taxonomy_orgs = TaxonomyOrg.get_organizations( - taxonomy=taxonomy, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - # Org-level staff can edit any taxonomy that is associated with one of their orgs. if is_org_admin(user, taxonomy_orgs): return True diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 9a297be968b1..837e17e7c19c 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -145,11 +145,11 @@ def test_get_taxonomies_enabled_subclasses(self): ) @ddt.unpack def test_get_taxonomies_for_org(self, org_attr, enabled, expected): - org_owner = getattr(self, org_attr) if org_attr else None + org_owner = getattr(self, org_attr).short_name if org_attr else None taxonomies = list( taxonomy.cast() for taxonomy in api.get_taxonomies_for_org( - org_owner=org_owner, enabled=enabled + org_short_name=org_owner, enabled=enabled ) ) assert taxonomies == [ diff --git a/requirements/constraints.txt b/requirements/constraints.txt index b24c8bc528f6..3f25fb3f21c2 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -108,7 +108,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # 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 2ebcab2954d8..a353c27b6ecc 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -782,7 +782,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index eb0837ed46bb..5544b80e95de 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1313,7 +1313,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index c7112a7770bf..ea78484056cc 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -924,7 +924,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 53e6c2746b01..6984c33cc079 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -982,7 +982,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt