From 2f70278fe28dbdaf34750e836d8a810e5eb65bf0 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 20 Jun 2023 04:51:07 +0930 Subject: [PATCH 1/9] feat: adds content tagging app, models, admin, and API --- cms/envs/common.py | 4 + lms/envs/common.py | 4 + openedx/features/content_tagging/__init__.py | 0 openedx/features/content_tagging/admin.py | 19 ++ openedx/features/content_tagging/api.py | 106 ++++++++++ openedx/features/content_tagging/apps.py | 12 ++ .../migrations/0001_initial.py | 72 +++++++ .../content_tagging/migrations/__init__.py | 0 openedx/features/content_tagging/models.py | 106 ++++++++++ .../content_tagging/tests/__init__.py | 0 .../content_tagging/tests/test_api.py | 198 ++++++++++++++++++ requirements/edx/base.txt | 5 + requirements/edx/development.txt | 5 + requirements/edx/github.in | 2 + requirements/edx/testing.txt | 5 + 15 files changed, 538 insertions(+) create mode 100644 openedx/features/content_tagging/__init__.py create mode 100644 openedx/features/content_tagging/admin.py create mode 100644 openedx/features/content_tagging/api.py create mode 100644 openedx/features/content_tagging/apps.py create mode 100644 openedx/features/content_tagging/migrations/0001_initial.py create mode 100644 openedx/features/content_tagging/migrations/__init__.py create mode 100644 openedx/features/content_tagging/models.py create mode 100644 openedx/features/content_tagging/tests/__init__.py create mode 100644 openedx/features/content_tagging/tests/test_api.py diff --git a/cms/envs/common.py b/cms/envs/common.py index 41ef3f60c2ef..d00f9473bdfa 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1756,10 +1756,14 @@ # API Documentation 'drf_yasg', + # Tagging + 'openedx_tagging.core.tagging.apps.TaggingConfig', + 'openedx.features.course_duration_limits', 'openedx.features.content_type_gating', 'openedx.features.discounts', 'openedx.features.effort_estimation', + 'openedx.features.content_tagging', 'lms.djangoapps.experiments', 'openedx.core.djangoapps.external_user_ids', diff --git a/lms/envs/common.py b/lms/envs/common.py index 16382b6175a8..94361d695317 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3216,6 +3216,9 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # Course Goals 'lms.djangoapps.course_goals.apps.CourseGoalsConfig', + # Tagging + 'openedx_tagging.core.tagging.apps.TaggingConfig', + # Features 'openedx.features.calendar_sync', 'openedx.features.course_bookmarks', @@ -3227,6 +3230,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx.features.discounts', 'openedx.features.effort_estimation', 'openedx.features.name_affirmation_api.apps.NameAffirmationApiConfig', + 'openedx.features.content_tagging', 'lms.djangoapps.experiments', diff --git a/openedx/features/content_tagging/__init__.py b/openedx/features/content_tagging/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/features/content_tagging/admin.py b/openedx/features/content_tagging/admin.py new file mode 100644 index 000000000000..f146a43187d4 --- /dev/null +++ b/openedx/features/content_tagging/admin.py @@ -0,0 +1,19 @@ +""" Tagging app admin """ +from django.contrib import admin + +from .models import ContentTaxonomy + + +class ContentTaxonomyOrgAdmin(admin.TabularInline): + model = ContentTaxonomy.org_owners.through + + +class ContentTaxonomyAdmin(admin.ModelAdmin): + """ + Admin form for the content taxonomy table. + """ + + inlines = (ContentTaxonomyOrgAdmin,) + + +admin.site.register(ContentTaxonomy, ContentTaxonomyAdmin) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py new file mode 100644 index 000000000000..63a9e19b1093 --- /dev/null +++ b/openedx/features/content_tagging/api.py @@ -0,0 +1,106 @@ +""" +Content Tagging APIs +""" +from typing import List + +import openedx_tagging.core.tagging.api as oel_tagging +from django.db.models import QuerySet +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy +from organizations.models import Organization + +from .models import ContentTaxonomy + + +def create_taxonomy( + name, + org_owners: List[Organization] = None, + description=None, + enabled=True, + required=False, + allow_multiple=False, + allow_free_text=False, +) -> ContentTaxonomy: + """ + Creates, saves, and returns a new ContentTaxonomy with the given attributes. + + If `org_owners` is empty/None, then the returned taxonomy is enabled for all organizations. + """ + taxonomy = ContentTaxonomy.objects.create( + name=name, + description=description, + enabled=enabled, + required=required, + allow_multiple=allow_multiple, + allow_free_text=allow_free_text, + ) + if org_owners: + set_taxonomy_org_owners(taxonomy, org_owners) + return taxonomy + + +def set_taxonomy_org_owners( + taxonomy: ContentTaxonomy, + org_owners: List[Organization] = None, +) -> ContentTaxonomy: + """ + Updates the list of org_owners on the given taxonomy. + + If `org_owners` is empty/None, then the returned taxonomy is enabled for all organizations. + """ + if org_owners: + taxonomy.org_owners.set(org_owners) + else: + taxonomy.org_owners.clear() + return taxonomy + + +def get_taxonomies(org_owner: Organization = None, enabled=True) -> QuerySet: + """ + Returns a queryset containing the enabled taxonomies owned by the given org, sorted by name. + + If you want the disabled taxonomies, pass enabled=False. + If you want all taxonomies (both enabled and disabled), pass enabled=None. + """ + return ( + ContentTaxonomy.objects.filter_enabled( + org_owner, + enabled, + ) + .order_by("name", "id") + .select_subclasses() + ) + + +# Expose the oel_tagging APIs + + +def get_tags(taxonomy: Taxonomy) -> List[Tag]: + """ + Exposes the oel_tagging.get_tags API method. + """ + return oel_tagging.get_tags(taxonomy) + + +def resync_object_tags(object_tags: QuerySet = None) -> int: + """ + Exposes the oel_tagging.resync_object_tags API method. + """ + return oel_tagging.resync_object_tags(object_tags) + + +def get_object_tags( + taxonomy: Taxonomy, object_id: str, object_type: str, valid_only=True +) -> List[ObjectTag]: + """ + Exposes the oel_tagging.get_object_tags API method. + """ + return oel_tagging.get_object_tags(taxonomy, object_id, object_type, valid_only) + + +def tag_object( + taxonomy: Taxonomy, tags: List, object_id: str, object_type: str +) -> List[ObjectTag]: + """ + Exposes the oel_tagging.tag_object API method. + """ + return oel_tagging.tag_object(taxonomy, tags, object_id, object_type) diff --git a/openedx/features/content_tagging/apps.py b/openedx/features/content_tagging/apps.py new file mode 100644 index 000000000000..29f9c5005f43 --- /dev/null +++ b/openedx/features/content_tagging/apps.py @@ -0,0 +1,12 @@ +""" +Define the content tagging Django App. +""" + +from django.apps import AppConfig + + +class ContentTaggingConfig(AppConfig): + """App config for the content tagging feature""" + + default_auto_field = "django.db.models.BigAutoField" + name = "openedx.features.content_tagging" diff --git a/openedx/features/content_tagging/migrations/0001_initial.py b/openedx/features/content_tagging/migrations/0001_initial.py new file mode 100644 index 000000000000..1c6cc251c9e9 --- /dev/null +++ b/openedx/features/content_tagging/migrations/0001_initial.py @@ -0,0 +1,72 @@ +# Generated by Django 3.2.19 on 2023-06-19 22:44 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + initial = True + + dependencies = [ + ("organizations", "0003_historicalorganizationcourse"), + ("oel_tagging", "0001_initial"), + ] + + operations = [ + migrations.CreateModel( + name="ContentTaxonomy", + fields=[ + ( + "taxonomy_ptr", + models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + serialize=False, + to="oel_tagging.taxonomy", + ), + ), + ], + options={ + "verbose_name_plural": "ContentTaxonomies", + }, + bases=("oel_tagging.taxonomy",), + ), + migrations.CreateModel( + name="ContentTaxonomyOrg", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "org", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="organizations.organization", + ), + ), + ( + "taxonomy", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="content_tagging.contenttaxonomy", + ), + ), + ], + ), + migrations.AddField( + model_name="contenttaxonomy", + name="org_owners", + field=models.ManyToManyField( + through="content_tagging.ContentTaxonomyOrg", + to="organizations.Organization", + ), + ), + ] diff --git a/openedx/features/content_tagging/migrations/__init__.py b/openedx/features/content_tagging/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/features/content_tagging/models.py b/openedx/features/content_tagging/models.py new file mode 100644 index 000000000000..95c12626a710 --- /dev/null +++ b/openedx/features/content_tagging/models.py @@ -0,0 +1,106 @@ +""" +Content Tagging models +""" +from django.db import models +from model_utils.managers import InheritanceManager +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_tagging.core.tagging.models import Taxonomy +from organizations.models import Organization + + +class ContentTaxonomyManager(InheritanceManager): + """ + Manages ContentTaxonomy objects, providing custom utility methods. + + Inherits from InheritanceManager so that subclasses of ContentTaxonomy can be selected in a queryset. + """ + + def filter_enabled(self, org: Organization = None, enabled=True) -> models.QuerySet: + """ + Returns a query set filtered to return the ContentTaxonomy objects enabled for the given org. + """ + queryset = self + if enabled is not None: + queryset = queryset.filter(enabled=enabled) + org_filter = models.Q(org_owners=None) + if org: + org_filter |= models.Q(org_owners=org) + queryset = queryset.filter(org_filter) + return queryset + + +class ContentTaxonomy(Taxonomy): + """ + Taxonomy used for tagging content-related objects. + + ContentTaxonomies can be owned by one or more organizations, which allows content authors from that organization to + create taxonomies, edit the taxonomy fields, and add/edit/remove Tags linked to the taxonomy. + + .. no_pii: + """ + + objects = ContentTaxonomyManager() + + org_owners = models.ManyToManyField(Organization, through="ContentTaxonomyOrg") + + class Meta: + verbose_name_plural = "ContentTaxonomies" + + def validate_object_tag( + self, + object_tag: "ObjectTag", + check_taxonomy=True, + check_tag=True, + check_object=True, + ) -> bool: + """ + Returns True if the given object tag is valid for this ContentTaxonomy. + + Extends the superclass method by adding its own object checks to ensure: + + * object_tag.object_id is a valid UsageKey or CourseKey, and + * object_tag.object_id's "org" is enabled for this taxonomy. + """ + valid = super().validate_object_tag( + object_tag, + check_taxonomy=check_taxonomy, + check_tag=check_tag, + check_object=check_object, + ) + + if valid and check_object: + # ContentTaxonomies require object_id to be a valid CourseKey or UsageKey + try: + object_key = UsageKey.from_string(object_tag.object_id) + except InvalidKeyError: + try: + object_key = CourseKey.from_string(object_tag.object_id) + except InvalidKeyError: + return False + + # ...and object to be in an org that is enabled for this taxonomy. + if not self.enabled_for_org(object_key.org): + return False + + return valid + + def enabled_for_org(self, org_short_name: str) -> bool: + """ + Returns True if this taxonomy is enabled for the given organization. + """ + enabled = self.enabled + if self.org_owners.count(): + enabled &= self.org_owners.filter( + contenttaxonomyorg__org__short_name=org_short_name, + ).exists() + return enabled + + +class ContentTaxonomyOrg(models.Model): + """ + Represents the many-to-many relationship between ContentTaxonomies and Organizations. + """ + + taxonomy = models.ForeignKey(ContentTaxonomy, on_delete=models.CASCADE) + org = models.ForeignKey(Organization, on_delete=models.CASCADE) diff --git a/openedx/features/content_tagging/tests/__init__.py b/openedx/features/content_tagging/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py new file mode 100644 index 000000000000..433b6f1fc203 --- /dev/null +++ b/openedx/features/content_tagging/tests/test_api.py @@ -0,0 +1,198 @@ +"""Tests for the Tagging models""" +import ddt +from django.test.testcases import TestCase +from openedx_tagging.core.tagging.models import ObjectTag, Tag +from organizations.models import Organization + +from .. import api + + +class TestContentTaxonomyMixin: + """ + Sets up data for testing ContentTaxonomies. + """ + + def setUp(self): + super().setUp() + self.org1 = Organization.objects.create(name="OpenedX", short_name="OeX") + self.org2 = Organization.objects.create(name="Axim", short_name="Ax") + self.taxonomy_disabled = api.create_taxonomy( + name="Learning Objectives", + enabled=False, + ) + self.taxonomy_all_orgs = api.create_taxonomy( + name="Content Types", + enabled=True, + ) + self.taxonomy_both_orgs = api.create_taxonomy( + name="OpenedX/Axim Content Types", + enabled=True, + org_owners=[self.org1, self.org2], + ) + self.taxonomy_one_org = api.create_taxonomy( + name="OpenedX Content Types", + enabled=True, + org_owners=[self.org1], + ) + self.tag_disabled = Tag.objects.create( + taxonomy=self.taxonomy_disabled, + value="learning", + ) + self.tag_all_orgs = Tag.objects.create( + taxonomy=self.taxonomy_all_orgs, + value="learning", + ) + self.tag_both_orgs = Tag.objects.create( + taxonomy=self.taxonomy_both_orgs, + value="learning", + ) + self.tag_one_org = Tag.objects.create( + taxonomy=self.taxonomy_one_org, + value="learning", + ) + self.all_orgs_course_tag = api.tag_object( + taxonomy=self.taxonomy_all_orgs, + tags=[self.tag_all_orgs.id], + object_id="course-v1:OeX+DemoX+Demo_Course", + object_type="course", + )[0] + self.all_orgs_block_tag = api.tag_object( + taxonomy=self.taxonomy_all_orgs, + tags=[self.tag_all_orgs.id], + object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", + object_type="section", + )[0] + self.both_orgs_course_tag = api.tag_object( + taxonomy=self.taxonomy_both_orgs, + tags=[self.tag_both_orgs.id], + object_id="course-v1:Ax+DemoX+Demo_Course", + object_type="course", + )[0] + self.both_orgs_block_tag = api.tag_object( + taxonomy=self.taxonomy_both_orgs, + tags=[self.tag_both_orgs.id], + object_id="block-v1:OeX+DemoX+Demo_Course+type@vertical+block@abcde", + object_type="section", + )[0] + self.one_org_block_tag = api.tag_object( + taxonomy=self.taxonomy_one_org, + tags=[self.tag_one_org.id], + object_id="block-v1:OeX+DemoX+Demo_Course+type@vertical+block@abcde", + object_type="section", + )[0] + + # Invalid object tags must be manually created + self.disabled_course_tag = ObjectTag.objects.create( + taxonomy=self.taxonomy_disabled, + tag=self.tag_disabled, + object_id="course-v1:Ax+DemoX+Demo_Course", + object_type="course", + ) + self.all_orgs_invalid_tag = ObjectTag.objects.create( + taxonomy=self.taxonomy_all_orgs, + tag=self.tag_all_orgs, + object_id="course-v1_OpenedX_DemoX_Demo_Course", + object_type="course", + ) + self.one_org_invalid_org_tag = ObjectTag.objects.create( + taxonomy=self.taxonomy_one_org, + tag=self.tag_one_org, + object_id="course-v1:Ax+DemoX+Demo_Course", + object_type="course", + ) + + +@ddt.ddt +class TestAPIContentTaxonomy(TestContentTaxonomyMixin, TestCase): + """ + Tests the ContentTaxonomy APIs. + """ + + def test_content_taxonomy_enabled_for_all_orgs(self): + with self.assertNumQueries(1): + taxonomies = list(api.get_taxonomies()) + assert taxonomies == [ + self.taxonomy_all_orgs, + ] + + def test_content_taxonomy_enabled_for_org1(self): + with self.assertNumQueries(1): + taxonomies = list(api.get_taxonomies(self.org1)) + assert taxonomies == [ + self.taxonomy_all_orgs, + self.taxonomy_one_org, + self.taxonomy_both_orgs, + ] + + def test_content_taxonomy_enabled_for_org2(self): + with self.assertNumQueries(1): + taxonomies = list(api.get_taxonomies(self.org2)) + assert taxonomies == [ + self.taxonomy_all_orgs, + self.taxonomy_both_orgs, + ] + + @ddt.data( + ("taxonomy_all_orgs", "all_orgs_course_tag", 2), + ("taxonomy_all_orgs", "all_orgs_block_tag", 2), + ("taxonomy_both_orgs", "both_orgs_course_tag", 3), + ("taxonomy_both_orgs", "both_orgs_block_tag", 3), + ("taxonomy_one_org", "one_org_block_tag", 3), + ) + @ddt.unpack + def test_get_object_tags_valid_for_org( + self, taxonomy_attr, object_tag_attr, num_queries + ): + object_tag = getattr(self, object_tag_attr) + with self.assertNumQueries(num_queries): + valid_tags = api.get_object_tags( + getattr(self, taxonomy_attr), + object_tag.object_id, + object_tag.object_type, + valid_only=True, + ) + assert valid_tags == [object_tag] + + @ddt.data( + ("taxonomy_disabled", "disabled_course_tag", 1), + ("taxonomy_all_orgs", "all_orgs_course_tag", 1), + ("taxonomy_all_orgs", "all_orgs_block_tag", 1), + ("taxonomy_all_orgs", "all_orgs_invalid_tag", 1), + ("taxonomy_both_orgs", "both_orgs_course_tag", 1), + ("taxonomy_both_orgs", "both_orgs_block_tag", 1), + ("taxonomy_one_org", "one_org_block_tag", 1), + ("taxonomy_one_org", "one_org_invalid_org_tag", 1), + ) + @ddt.unpack + def test_get_object_tags_include_invalid( + self, taxonomy_attr, object_tag_attr, num_queries + ): + object_tag = getattr(self, object_tag_attr) + with self.assertNumQueries(num_queries): + valid_tags = api.get_object_tags( + getattr(self, taxonomy_attr), + object_tag.object_id, + object_tag.object_type, + valid_only=False, + ) + assert valid_tags == [object_tag] + + def test_get_tags(self): + assert api.get_tags(self.taxonomy_all_orgs) == [self.tag_all_orgs] + + def test_resync_object_tags(self): + # Only all_orgs_invalid_tag needs re-syncing, because it wasn't created with api.tag_object + assert ( + api.resync_object_tags( + ObjectTag.objects.filter(taxonomy=self.taxonomy_all_orgs) + ) + == 1 + ) + self.taxonomy_all_orgs.delete() + for object_tag in [ + self.all_orgs_course_tag, + self.all_orgs_block_tag, + self.all_orgs_invalid_tag, + ]: + object_tag.refresh_from_db() + assert object_tag.name == "Content Types" diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index d288b5d209f4..f1aead715ad6 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -241,6 +241,7 @@ django==3.2.19 # openedx-django-wiki # openedx-events # openedx-filters + # openedx-learning # ora2 # outcome-surveys # skill-tagging @@ -402,6 +403,7 @@ djangorestframework==3.13.1 # edx-submissions # learner-pathway-progress # openedx-blockstore + # openedx-learning # ora2 # super-csv djangorestframework-xml==2.0.0 @@ -792,6 +794,8 @@ openedx-filters==1.3.0 # -r requirements/edx/base.in # lti-consumer-xblock # skill-tagging +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api + # via -r requirements/edx/github.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.in optimizely-sdk==4.1.1 @@ -1029,6 +1033,7 @@ rules==3.3 # -r requirements/edx/base.in # edx-enterprise # edx-proctoring + # openedx-learning s3transfer==0.1.13 # via boto3 sailthru-client==2.2.3 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index e3a66360710d..9f33fd033dec 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -342,6 +342,7 @@ django==3.2.19 # openedx-django-wiki # openedx-events # openedx-filters + # openedx-learning # ora2 # outcome-surveys # skill-tagging @@ -517,6 +518,7 @@ djangorestframework==3.13.1 # edx-submissions # learner-pathway-progress # openedx-blockstore + # openedx-learning # ora2 # super-csv djangorestframework-xml==2.0.0 @@ -1052,6 +1054,8 @@ openedx-filters==1.3.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # skill-tagging +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api + # via -r requirements/edx/testing.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/testing.txt optimizely-sdk==4.1.1 @@ -1436,6 +1440,7 @@ rules==3.3 # -r requirements/edx/testing.txt # edx-enterprise # edx-proctoring + # openedx-learning s3transfer==0.1.13 # via # -r requirements/edx/testing.txt diff --git a/requirements/edx/github.in b/requirements/edx/github.in index 2b4e75f5222c..9fc7795987ff 100644 --- a/requirements/edx/github.in +++ b/requirements/edx/github.in @@ -79,6 +79,8 @@ ############################################################################## # ... add dependencies here +# FIXME JV - move to base.in once pypi package created +git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api#egg=openedx-learning==0.1 ############################################################################## diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index a6107778bcbb..1cf248952c39 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -320,6 +320,7 @@ django==3.2.19 # openedx-django-wiki # openedx-events # openedx-filters + # openedx-learning # ora2 # outcome-surveys # skill-tagging @@ -493,6 +494,7 @@ djangorestframework==3.13.1 # edx-submissions # learner-pathway-progress # openedx-blockstore + # openedx-learning # ora2 # super-csv djangorestframework-xml==2.0.0 @@ -997,6 +999,8 @@ openedx-filters==1.3.0 # -r requirements/edx/base.txt # lti-consumer-xblock # skill-tagging +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api + # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 @@ -1348,6 +1352,7 @@ rules==3.3 # -r requirements/edx/base.txt # edx-enterprise # edx-proctoring + # openedx-learning s3transfer==0.1.13 # via # -r requirements/edx/base.txt From d27c18c7f2a2cb7f7316ada6d4bc097e22763a18 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 26 Jun 2023 20:38:38 +0930 Subject: [PATCH 2/9] refactor: moves is_content_creator from cms.djangoapps.contentstore.helpers to common.djangoapps.student.auth --- cms/djangoapps/contentstore/helpers.py | 14 -------------- cms/djangoapps/contentstore/views/course.py | 4 ++-- cms/djangoapps/contentstore/views/library.py | 4 ++-- common/djangoapps/student/auth.py | 12 ++++++++++++ 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/cms/djangoapps/contentstore/helpers.py b/cms/djangoapps/contentstore/helpers.py index cbaf4b8f80cf..c08f23dd78ae 100644 --- a/cms/djangoapps/contentstore/helpers.py +++ b/cms/djangoapps/contentstore/helpers.py @@ -15,8 +15,6 @@ # from cms.djangoapps.contentstore.views.preview import _load_preview_block from cms.djangoapps.models.settings.course_grading import CourseGradingModel -from common.djangoapps.student import auth -from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole try: # Technically this is a django app plugin, so we should not error if it's not installed: @@ -272,15 +270,3 @@ def is_item_in_course_tree(item): ancestor = ancestor.get_parent() return ancestor is not None - - -def is_content_creator(user, org): - """ - Check if the user has the role to create content. - - This function checks if the User has role to create content - or if the org is supplied, it checks for Org level course content - creator. - """ - return (auth.user_has_role(user, CourseCreatorRole()) or - auth.user_has_role(user, OrgContentCreatorRole(org=org))) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 99669e5a7276..fe70ba32e08d 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -45,7 +45,8 @@ has_course_author_access, has_studio_read_access, has_studio_write_access, - has_studio_advanced_settings_access + has_studio_advanced_settings_access, + is_content_creator, ) from common.djangoapps.student.roles import ( CourseInstructorRole, @@ -102,7 +103,6 @@ update_course_details, ) from .component import ADVANCED_COMPONENT_TYPES -from ..helpers import is_content_creator from cms.djangoapps.contentstore.xblock_services.xblock_service import ( create_xblock_info, ) diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index efcad6e6035d..8b38fbb0763f 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -30,7 +30,8 @@ STUDIO_VIEW_USERS, get_user_permissions, has_studio_read_access, - has_studio_write_access + has_studio_write_access, + is_content_creator, ) from common.djangoapps.student.roles import ( CourseInstructorRole, @@ -42,7 +43,6 @@ from ..config.waffle import REDIRECT_TO_LIBRARY_AUTHORING_MICROFRONTEND from ..utils import add_instructor, reverse_library_url from .component import CONTAINER_TEMPLATES, get_component_templates -from ..helpers import is_content_creator from cms.djangoapps.contentstore.xblock_services.xblock_service import create_xblock_info from .user import user_with_role diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index f8e45ce6ba6c..47aef5cf6385 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -150,6 +150,18 @@ def has_studio_read_access(user, course_key): return bool(STUDIO_VIEW_CONTENT & get_user_permissions(user, course_key)) +def is_content_creator(user, org): + """ + Check if the user has the role to create content. + + This function checks if the User has role to create content + or if the org is supplied, it checks for Org level course content + creator. + """ + return (user_has_role(user, CourseCreatorRole()) or + user_has_role(user, OrgContentCreatorRole(org=org))) + + def add_users(caller, role, *users): """ The caller requests adding the given users to the role. Checks that the caller From 456979ace7db3a740b0f988686b699e36cf53632 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 26 Jun 2023 20:39:51 +0930 Subject: [PATCH 3/9] feat: adds permissions/rules for content_tagging --- openedx/features/content_tagging/rules.py | 101 ++++++ .../content_tagging/tests/test_rules.py | 319 ++++++++++++++++++ 2 files changed, 420 insertions(+) create mode 100644 openedx/features/content_tagging/rules.py create mode 100644 openedx/features/content_tagging/tests/test_rules.py diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py new file mode 100644 index 000000000000..fd54f36303af --- /dev/null +++ b/openedx/features/content_tagging/rules.py @@ -0,0 +1,101 @@ +"""Django rules-based permissions for tagging""" + +import openedx_tagging.core.tagging.rules as oel_tagging +import rules +from organizations.models import Organization + +from common.djangoapps.student.auth import is_content_creator + + +def is_taxonomy_admin(user, taxonomy=None): + """ + Returns True if the given user is a Taxonomy Admin for the given taxonomy. + + Global Taxonomy Admins include global staff and superusers, plus course creators who can create courses for any org. + + Otherwise, a taxonomy must be provided to determine if the user is a org-level course creator for one of the + taxonomy's org_owners. + """ + if oel_tagging.is_taxonomy_admin(user): + return True + + if not taxonomy: + return is_content_creator(user, None) + + # If the user is a content creator for any of this taxonomy's org_owners, they are also an admin for this taxonomy. + taxonomy_orgs = taxonomy.org_owners.values_list( + "contenttaxonomyorg__org__short_name", flat=True + ) + + # Fetch whole orgs list if the taxonomy doesn't specify org_owners + if not taxonomy_orgs: + taxonomy_orgs = Organization.objects.all().values_list("short_name", flat=True) + + if taxonomy_orgs: + for org in taxonomy_orgs: + if is_content_creator(user, org): + return True + return False + + +@rules.predicate +def can_view_taxonomy(user, taxonomy=None): + """ + Anyone can view an enabled taxonomy, + but only taxonomy admins can view a disabled taxonomy. + """ + return (taxonomy and taxonomy.enabled) or is_taxonomy_admin(user, taxonomy) + + +@rules.predicate +def can_change_taxonomy(user, taxonomy=None): + """ + Even taxonomy admins cannot change system taxonomies. + """ + return is_taxonomy_admin(user, taxonomy) and ( + not taxonomy or (taxonomy and not taxonomy.system_defined) + ) + + +@rules.predicate +def can_change_taxonomy_tag(user, tag=None): + """ + Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies + (these don't have predefined tags). + """ + taxonomy = tag.taxonomy if tag else None + return is_taxonomy_admin(user, taxonomy) and ( + not tag + or not taxonomy + or (taxonomy and not taxonomy.allow_free_text and not taxonomy.system_defined) + ) + + +@rules.predicate +def can_change_object_tag(user, object_tag=None): + """ + Taxonomy admins can create or modify object tags on enabled taxonomies. + """ + taxonomy = object_tag.taxonomy if object_tag else None + return is_taxonomy_admin(user, taxonomy) and ( + not object_tag or not taxonomy or (taxonomy and taxonomy.enabled) + ) + + +# Taxonomy +rules.set_perm("oel_tagging.add_taxonomy", can_change_taxonomy) +rules.set_perm("oel_tagging.change_taxonomy", can_change_taxonomy) +rules.set_perm("oel_tagging.delete_taxonomy", can_change_taxonomy) +rules.set_perm("oel_tagging.view_taxonomy", can_view_taxonomy) + +# Tag +rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) +rules.set_perm("oel_tagging.change_tag", can_change_taxonomy_tag) +rules.set_perm("oel_tagging.delete_tag", can_change_taxonomy_tag) +rules.set_perm("oel_tagging.view_tag", rules.always_allow) + +# ObjectTag +rules.set_perm("oel_tagging.add_object_tag", can_change_object_tag) +rules.set_perm("oel_tagging.change_object_tag", can_change_object_tag) +rules.set_perm("oel_tagging.delete_object_tag", can_change_object_tag) +rules.set_perm("oel_tagging.view_object_tag", rules.always_allow) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py new file mode 100644 index 000000000000..e8460307a0ff --- /dev/null +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -0,0 +1,319 @@ +"""Tests content_tagging rules-based permissions""" + +import ddt +from django.contrib.auth import get_user_model +from django.test.testcases import TestCase, override_settings +from mock import Mock +from openedx_tagging.core.tagging.models import ObjectTag, Tag + +from common.djangoapps.student.auth import add_users, update_org_role +from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole + +from ..models import ContentTaxonomy +from .test_api import TestContentTaxonomyMixin + +User = get_user_model() + + +@ddt.ddt +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestRulesContentTaxonomy(TestContentTaxonomyMixin, TestCase): + """ + Tests that the expected rules have been applied to the ContentTaxonomy models. + + We set ENABLE_CREATOR_GROUP for these tests, otherwise all users have course creator access for all orgs. + """ + + def setUp(self): + super().setUp() + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) + # Normal user: grant course creator role (for all orgs) + self.user_all_orgs = User.objects.create( + username="user_all_orgs", + email="staff+all@example.com", + ) + add_users(self.staff, CourseCreatorRole(), self.user_all_orgs) + + # Normal user: grant course creator access to both org1 and org2 + self.user_both_orgs = User.objects.create( + username="user_both_orgs", + email="staff+both@example.com", + ) + update_org_role( + self.staff, + OrgContentCreatorRole, + self.user_both_orgs, + [self.org1.short_name, self.org2.short_name], + ) + + # Normal user: grant course creator access to org2 + self.user_org2 = User.objects.create( + username="user_org2", + email="staff+org2@example.com", + ) + update_org_role( + self.staff, OrgContentCreatorRole, self.user_org2, [self.org2.short_name] + ) + + # Normal user: no course creator access + self.learner = User.objects.create( + username="learner", + email="learner@example.com", + ) + + def _expected_users_have_perm( + self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True + ): + """ + Checks that all users have the given permission on the given object. + + If learners_too, then the learner user should have it too. + """ + # Global Taxonomy Admins can do pretty much anything + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, obj) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, obj) + assert self.user_all_orgs.has_perm(perm) + assert self.user_all_orgs.has_perm(perm, obj) + + # Org content creators are bound by a taxonomy's org restrictions + assert self.user_both_orgs.has_perm(perm) == learner_perm + assert self.user_both_orgs.has_perm(perm, obj) + assert self.user_org2.has_perm(perm) == learner_perm + # user_org2 does not have course creator access for org 1 + assert self.user_org2.has_perm(perm, obj) == user_org2 + + # Learners can't do much but view + assert self.learner.has_perm(perm) == learner_perm + assert self.learner.has_perm(perm, obj) == learner_obj + + # Taxonomy + + @ddt.data( + ("oel_tagging.add_taxonomy", "taxonomy_all_orgs"), + ("oel_tagging.add_taxonomy", "taxonomy_both_orgs"), + ("oel_tagging.add_taxonomy", "taxonomy_disabled"), + ("oel_tagging.change_taxonomy", "taxonomy_all_orgs"), + ("oel_tagging.change_taxonomy", "taxonomy_both_orgs"), + ("oel_tagging.change_taxonomy", "taxonomy_disabled"), + ("oel_tagging.delete_taxonomy", "taxonomy_all_orgs"), + ("oel_tagging.delete_taxonomy", "taxonomy_both_orgs"), + ("oel_tagging.delete_taxonomy", "taxonomy_disabled"), + ) + @ddt.unpack + def test_change_taxonomy_all_orgs(self, perm, taxonomy_attr): + """Taxonomy administrators with course creator access for the taxonomy org""" + taxonomy = getattr(self, taxonomy_attr) + self._expected_users_have_perm(perm, taxonomy) + + @ddt.data( + ("oel_tagging.add_taxonomy", "taxonomy_one_org"), + ("oel_tagging.change_taxonomy", "taxonomy_one_org"), + ("oel_tagging.delete_taxonomy", "taxonomy_one_org"), + ) + @ddt.unpack + def test_change_taxonomy_org1(self, perm, taxonomy_attr): + taxonomy = getattr(self, taxonomy_attr) + self._expected_users_have_perm(perm, taxonomy, user_org2=False) + + @ddt.data( + "oel_tagging.add_taxonomy", + "oel_tagging.change_taxonomy", + "oel_tagging.delete_taxonomy", + ) + def test_system_taxonomy(self, perm): + """Taxonomy administrators cannot edit system taxonomies""" + # TODO: use SystemTaxonomy when available + system_taxonomy = Mock(spec=ContentTaxonomy) + system_taxonomy.system_defined.return_value = True + system_taxonomy.org_owners.return_value = Mock() + system_taxonomy.org_owners.values_list.return_value = [] + assert self.superuser.has_perm(perm, system_taxonomy) + assert not self.staff.has_perm(perm, system_taxonomy) + assert not self.user_all_orgs.has_perm(perm, system_taxonomy) + assert not self.user_both_orgs.has_perm(perm, system_taxonomy) + assert not self.user_org2.has_perm(perm, system_taxonomy) + assert not self.learner.has_perm(perm, system_taxonomy) + + @ddt.data( + (True, "taxonomy_all_orgs"), + (False, "taxonomy_all_orgs"), + (True, "taxonomy_both_orgs"), + (False, "taxonomy_both_orgs"), + ) + @ddt.unpack + def test_view_taxonomy_enabled(self, enabled, taxonomy_attr): + """Anyone can see enabled taxonomies, but learners cannot see disabled taxonomies""" + taxonomy = getattr(self, taxonomy_attr) + taxonomy.enabled = enabled + perm = "oel_tagging.view_taxonomy" + self._expected_users_have_perm(perm, taxonomy, learner_obj=enabled) + + # Tag + + @ddt.data( + ("oel_tagging.add_tag", "tag_all_orgs"), + ("oel_tagging.add_tag", "tag_both_orgs"), + ("oel_tagging.add_tag", "tag_disabled"), + ("oel_tagging.change_tag", "tag_all_orgs"), + ("oel_tagging.change_tag", "tag_both_orgs"), + ("oel_tagging.change_tag", "tag_disabled"), + ("oel_tagging.delete_tag", "tag_all_orgs"), + ("oel_tagging.delete_tag", "tag_both_orgs"), + ("oel_tagging.delete_tag", "tag_disabled"), + ) + @ddt.unpack + def test_change_tag_all_orgs(self, perm, tag_attr): + """Taxonomy administrators can modify tags on non-free-text taxonomies""" + tag = getattr(self, tag_attr) + self._expected_users_have_perm(perm, tag) + + @ddt.data( + ("oel_tagging.add_tag", "tag_one_org"), + ("oel_tagging.change_tag", "tag_one_org"), + ("oel_tagging.delete_tag", "tag_one_org"), + ) + @ddt.unpack + def test_change_tag_org1(self, perm, tag_attr): + """Taxonomy administrators can modify tags on non-free-text taxonomies""" + tag = getattr(self, tag_attr) + self._expected_users_have_perm(perm, tag, user_org2=False) + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + "oel_tagging.delete_tag", + ) + def test_tag_no_taxonomy(self, perm): + """Taxonomy administrators can modify any Tag, even those with no Taxonnmy.""" + tag = Tag() + + # Global Taxonomy Admins can do pretty much anything + assert self.superuser.has_perm(perm, tag) + assert self.staff.has_perm(perm, tag) + assert self.user_all_orgs.has_perm(perm, tag) + + # Org content creators are bound by a taxonomy's org restrictions, + # so if there's no taxonomy, they can't do anything to it. + assert not self.user_both_orgs.has_perm(perm, tag) + assert not self.user_org2.has_perm(perm, tag) + assert not self.learner.has_perm(perm, tag) + + @ddt.data( + "tag_all_orgs", + "tag_both_orgs", + "tag_one_org", + "tag_disabled", + ) + def test_view_tag(self, tag_attr): + """Anyone can view any Tag""" + tag = getattr(self, tag_attr) + self._expected_users_have_perm( + "oel_tagging.view_tag", tag, learner_perm=True, learner_obj=True + ) + + # ObjectTag + + @ddt.data( + ("oel_tagging.add_object_tag", "disabled_course_tag"), + ("oel_tagging.change_object_tag", "disabled_course_tag"), + ("oel_tagging.delete_object_tag", "disabled_course_tag"), + ) + @ddt.unpack + def test_object_tag_disabled_taxonomy(self, perm, tag_attr): + """Taxonomy administrators cannot create/edit an ObjectTag with a disabled Taxonomy""" + object_tag = getattr(self, tag_attr) + assert not object_tag.taxonomy.enabled + assert self.superuser.has_perm(perm, object_tag) + assert not self.staff.has_perm(perm, object_tag) + assert not self.user_all_orgs.has_perm(perm, object_tag) + assert not self.user_both_orgs.has_perm(perm, object_tag) + assert not self.user_org2.has_perm(perm, object_tag) + assert not self.learner.has_perm(perm, object_tag) + + @ddt.data( + ("oel_tagging.add_object_tag", "all_orgs_course_tag"), + ("oel_tagging.add_object_tag", "all_orgs_block_tag"), + ("oel_tagging.add_object_tag", "both_orgs_course_tag"), + ("oel_tagging.add_object_tag", "both_orgs_block_tag"), + ("oel_tagging.add_object_tag", "all_orgs_invalid_tag"), + ("oel_tagging.change_object_tag", "all_orgs_course_tag"), + ("oel_tagging.change_object_tag", "all_orgs_block_tag"), + ("oel_tagging.change_object_tag", "both_orgs_course_tag"), + ("oel_tagging.change_object_tag", "both_orgs_block_tag"), + ("oel_tagging.change_object_tag", "all_orgs_invalid_tag"), + ("oel_tagging.delete_object_tag", "all_orgs_course_tag"), + ("oel_tagging.delete_object_tag", "all_orgs_block_tag"), + ("oel_tagging.delete_object_tag", "both_orgs_course_tag"), + ("oel_tagging.delete_object_tag", "both_orgs_block_tag"), + ("oel_tagging.delete_object_tag", "all_orgs_invalid_tag"), + ) + @ddt.unpack + def test_change_object_tag_all_orgs(self, perm, tag_attr): + """Taxonomy administrators can create/edit an ObjectTag on taxonomies in their org.""" + object_tag = getattr(self, tag_attr) + self._expected_users_have_perm(perm, object_tag) + + @ddt.data( + ("oel_tagging.add_object_tag", "one_org_block_tag"), + ("oel_tagging.add_object_tag", "one_org_invalid_org_tag"), + ("oel_tagging.change_object_tag", "one_org_block_tag"), + ("oel_tagging.change_object_tag", "one_org_invalid_org_tag"), + ("oel_tagging.delete_object_tag", "one_org_block_tag"), + ("oel_tagging.delete_object_tag", "one_org_invalid_org_tag"), + ) + @ddt.unpack + def test_change_object_tag_org1(self, perm, tag_attr): + """Taxonomy administrators can create/edit an ObjectTag on taxonomies in their org.""" + object_tag = getattr(self, tag_attr) + self._expected_users_have_perm(perm, object_tag, user_org2=False) + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + "oel_tagging.delete_object_tag", + ) + def test_object_tag_no_taxonomy(self, perm): + """Taxonomy administrators can modify an ObjectTag with no Taxonomy""" + object_tag = ObjectTag() + + # Global Taxonomy Admins can do pretty much anything + assert self.superuser.has_perm(perm, object_tag) + assert self.staff.has_perm(perm, object_tag) + assert self.user_all_orgs.has_perm(perm, object_tag) + + # Org content creators are bound by a taxonomy's org restrictions, + # so if there's no taxonomy, they can't do anything to it. + assert not self.user_both_orgs.has_perm(perm, object_tag) + assert not self.user_org2.has_perm(perm, object_tag) + assert not self.learner.has_perm(perm, object_tag) + + @ddt.data( + "all_orgs_course_tag", + "all_orgs_block_tag", + "both_orgs_course_tag", + "both_orgs_block_tag", + "one_org_block_tag", + "all_orgs_invalid_tag", + "one_org_invalid_org_tag", + "disabled_course_tag", + ) + def test_view_object_tag(self, tag_attr): + """Anyone can view any ObjectTag""" + object_tag = getattr(self, tag_attr) + self._expected_users_have_perm( + "oel_tagging.view_object_tag", + object_tag, + learner_perm=True, + learner_obj=True, + ) From 3ade910d576b8b5b664396f1a85948c284332d15 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 21:53:03 +0930 Subject: [PATCH 4/9] style: group tagging INSTALLED_APPS together --- cms/envs/common.py | 3 ++- lms/envs/common.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index d00f9473bdfa..dc8ae0a16053 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1758,12 +1758,13 @@ # Tagging 'openedx_tagging.core.tagging.apps.TaggingConfig', + 'openedx.features.content_tagging', + # Features 'openedx.features.course_duration_limits', 'openedx.features.content_type_gating', 'openedx.features.discounts', 'openedx.features.effort_estimation', - 'openedx.features.content_tagging', 'lms.djangoapps.experiments', 'openedx.core.djangoapps.external_user_ids', diff --git a/lms/envs/common.py b/lms/envs/common.py index 94361d695317..755a31479ade 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3218,6 +3218,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # Tagging 'openedx_tagging.core.tagging.apps.TaggingConfig', + 'openedx.features.content_tagging', # Features 'openedx.features.calendar_sync', @@ -3230,7 +3231,6 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring 'openedx.features.discounts', 'openedx.features.effort_estimation', 'openedx.features.name_affirmation_api.apps.NameAffirmationApiConfig', - 'openedx.features.content_tagging', 'lms.djangoapps.experiments', From d51e148c28eeaa2cd2dc0ea6e756d89bc7688d09 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 21:59:18 +0930 Subject: [PATCH 5/9] perf: rearrange ContentTaxonomy.validate_object_tag to do the low-effort checks first. --- openedx/features/content_tagging/api.py | 1 - openedx/features/content_tagging/models.py | 16 +++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index 63a9e19b1093..fcf1a1800150 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -5,7 +5,6 @@ import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import QuerySet -from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from organizations.models import Organization from .models import ContentTaxonomy diff --git a/openedx/features/content_tagging/models.py b/openedx/features/content_tagging/models.py index 95c12626a710..a4d11665a0bc 100644 --- a/openedx/features/content_tagging/models.py +++ b/openedx/features/content_tagging/models.py @@ -62,14 +62,7 @@ def validate_object_tag( * object_tag.object_id is a valid UsageKey or CourseKey, and * object_tag.object_id's "org" is enabled for this taxonomy. """ - valid = super().validate_object_tag( - object_tag, - check_taxonomy=check_taxonomy, - check_tag=check_tag, - check_object=check_object, - ) - - if valid and check_object: + if check_object: # ContentTaxonomies require object_id to be a valid CourseKey or UsageKey try: object_key = UsageKey.from_string(object_tag.object_id) @@ -83,7 +76,12 @@ def validate_object_tag( if not self.enabled_for_org(object_key.org): return False - return valid + return super().validate_object_tag( + object_tag, + check_taxonomy=check_taxonomy, + check_tag=check_tag, + check_object=check_object, + ) def enabled_for_org(self, org_short_name: str) -> bool: """ From d0c446b269a4b9a747e6613a39c34bf5aaa1cab9 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 22:02:10 +0930 Subject: [PATCH 6/9] style: import oel_tagging API methods more concisely --- openedx/features/content_tagging/api.py | 37 ++++--------------------- 1 file changed, 5 insertions(+), 32 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index fcf1a1800150..b7686c9cee63 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -70,36 +70,9 @@ def get_taxonomies(org_owner: Organization = None, enabled=True) -> QuerySet: ) -# Expose the oel_tagging APIs +# Expose the oel_tagging APIs that we haven't overridden here: - -def get_tags(taxonomy: Taxonomy) -> List[Tag]: - """ - Exposes the oel_tagging.get_tags API method. - """ - return oel_tagging.get_tags(taxonomy) - - -def resync_object_tags(object_tags: QuerySet = None) -> int: - """ - Exposes the oel_tagging.resync_object_tags API method. - """ - return oel_tagging.resync_object_tags(object_tags) - - -def get_object_tags( - taxonomy: Taxonomy, object_id: str, object_type: str, valid_only=True -) -> List[ObjectTag]: - """ - Exposes the oel_tagging.get_object_tags API method. - """ - return oel_tagging.get_object_tags(taxonomy, object_id, object_type, valid_only) - - -def tag_object( - taxonomy: Taxonomy, tags: List, object_id: str, object_type: str -) -> List[ObjectTag]: - """ - Exposes the oel_tagging.tag_object API method. - """ - return oel_tagging.tag_object(taxonomy, tags, object_id, object_type) +get_tags = oel_tagging.get_tags +resync_object_tags = oel_tagging.resync_object_tags +get_object_tags = oel_tagging.get_object_tags +tag_object = oel_tagging.tag_object From a0e33e1537875dc45145f09ae2b80ab836ff17bd Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 22:12:31 +0930 Subject: [PATCH 7/9] style: adds type annotations to rules --- openedx/features/content_tagging/rules.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index fd54f36303af..46e65aa65703 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -2,14 +2,19 @@ import openedx_tagging.core.tagging.rules as oel_tagging import rules +from django.contrib.auth import get_user_model from organizations.models import Organization from common.djangoapps.student.auth import is_content_creator +from .models import ContentTaxonomy -def is_taxonomy_admin(user, taxonomy=None): +User = get_user_model() + + +def is_taxonomy_admin(user: User, taxonomy: ContentTaxonomy = None) -> bool: """ - Returns True if the given user is a Taxonomy Admin for the given taxonomy. + Returns True if the given user is a Taxonomy Admin for the given content taxonomy. Global Taxonomy Admins include global staff and superusers, plus course creators who can create courses for any org. @@ -39,7 +44,7 @@ def is_taxonomy_admin(user, taxonomy=None): @rules.predicate -def can_view_taxonomy(user, taxonomy=None): +def can_view_taxonomy(user: User, taxonomy: ContentTaxonomy = None) -> bool: """ Anyone can view an enabled taxonomy, but only taxonomy admins can view a disabled taxonomy. @@ -48,7 +53,7 @@ def can_view_taxonomy(user, taxonomy=None): @rules.predicate -def can_change_taxonomy(user, taxonomy=None): +def can_change_taxonomy(user: User, taxonomy: ContentTaxonomy = None) -> bool: """ Even taxonomy admins cannot change system taxonomies. """ @@ -58,7 +63,7 @@ def can_change_taxonomy(user, taxonomy=None): @rules.predicate -def can_change_taxonomy_tag(user, tag=None): +def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: """ Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies (these don't have predefined tags). @@ -72,7 +77,7 @@ def can_change_taxonomy_tag(user, tag=None): @rules.predicate -def can_change_object_tag(user, object_tag=None): +def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) -> bool: """ Taxonomy admins can create or modify object tags on enabled taxonomies. """ From 7278da0ab7cc8a3ff660a98d65ba4754cfd59645 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 22:20:39 +0930 Subject: [PATCH 8/9] fix: filter ContentTaxonomies by org, independent of the enabled flag --- openedx/features/content_tagging/models.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openedx/features/content_tagging/models.py b/openedx/features/content_tagging/models.py index a4d11665a0bc..19c635916432 100644 --- a/openedx/features/content_tagging/models.py +++ b/openedx/features/content_tagging/models.py @@ -18,15 +18,15 @@ class ContentTaxonomyManager(InheritanceManager): def filter_enabled(self, org: Organization = None, enabled=True) -> models.QuerySet: """ - Returns a query set filtered to return the ContentTaxonomy objects enabled for the given org. + Returns a query set filtered to return the enabled ContentTaxonomy objects owned by the given org. """ queryset = self if enabled is not None: queryset = queryset.filter(enabled=enabled) - org_filter = models.Q(org_owners=None) - if org: - org_filter |= models.Q(org_owners=org) - queryset = queryset.filter(org_filter) + org_filter = models.Q(org_owners=None) + if org: + org_filter |= models.Q(org_owners=org) + queryset = queryset.filter(org_filter) return queryset From 4f297f67ba2908874c7101be9f3fd577dc5ea2b9 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2023 11:08:46 +0930 Subject: [PATCH 9/9] fix: use InheritanceManager in oel_tagging * updates requirement to use WIP branch for openedx-learning * import get_taxonomy and get_taxonomies from the oel_tagging API * rename content_tagging.api.get_taxonomies to get_taxonomies_for_org * adds tests to verify that API returns expected Taxonomy subclasses * adds tests to verify object tag validation uses the correct Taxonomy subclass --- openedx/features/content_tagging/api.py | 13 ++- openedx/features/content_tagging/models.py | 5 +- .../content_tagging/tests/test_api.py | 81 +++++++++++++++---- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/github.in | 2 +- requirements/edx/testing.txt | 2 +- 7 files changed, 82 insertions(+), 25 deletions(-) diff --git a/openedx/features/content_tagging/api.py b/openedx/features/content_tagging/api.py index b7686c9cee63..a80607dba9dd 100644 --- a/openedx/features/content_tagging/api.py +++ b/openedx/features/content_tagging/api.py @@ -53,12 +53,15 @@ def set_taxonomy_org_owners( return taxonomy -def get_taxonomies(org_owner: Organization = None, enabled=True) -> QuerySet: +def get_taxonomies_for_org(org_owner: Organization = None, enabled=True) -> QuerySet: """ - Returns a queryset containing the enabled taxonomies owned by the given org, sorted by name. + Returns a queryset containing the enabled ContentTaxonomies owned by the given org, sorted by name. - If you want the disabled taxonomies, pass enabled=False. - If you want all taxonomies (both enabled and disabled), pass enabled=None. + If you want all Taxonomies, not just ContentTaxonomies, use the get_taxonomies API method, and use `enabled_for_org` + on any returned ContentTaxonomies to filter out by org ownership. + + If you want the disabled ContentTaxonomies, pass enabled=False. + If you want all ContentTaxonomies (both enabled and disabled), pass enabled=None. """ return ( ContentTaxonomy.objects.filter_enabled( @@ -72,6 +75,8 @@ def get_taxonomies(org_owner: Organization = None, enabled=True) -> QuerySet: # Expose the oel_tagging APIs that we haven't overridden here: +get_taxonomies = oel_tagging.get_taxonomies +get_taxonomy = oel_tagging.get_taxonomy get_tags = oel_tagging.get_tags resync_object_tags = oel_tagging.resync_object_tags get_object_tags = oel_tagging.get_object_tags diff --git a/openedx/features/content_tagging/models.py b/openedx/features/content_tagging/models.py index 19c635916432..e5caacc94fc7 100644 --- a/openedx/features/content_tagging/models.py +++ b/openedx/features/content_tagging/models.py @@ -2,14 +2,13 @@ Content Tagging models """ from django.db import models -from model_utils.managers import InheritanceManager from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_tagging.core.tagging.models import Taxonomy +from openedx_tagging.core.tagging.models import Taxonomy, TaxonomyManager from organizations.models import Organization -class ContentTaxonomyManager(InheritanceManager): +class ContentTaxonomyManager(TaxonomyManager): """ Manages ContentTaxonomy objects, providing custom utility methods. diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py index 433b6f1fc203..3b819c9b5bfa 100644 --- a/openedx/features/content_tagging/tests/test_api.py +++ b/openedx/features/content_tagging/tests/test_api.py @@ -16,6 +16,11 @@ def setUp(self): super().setUp() self.org1 = Organization.objects.create(name="OpenedX", short_name="OeX") self.org2 = Organization.objects.create(name="Axim", short_name="Ax") + # Taxonomy + self.taxonomy_base = api.oel_tagging.create_taxonomy( + name="Base Taxonomy", + ) + # ContentTaxonomies self.taxonomy_disabled = api.create_taxonomy( name="Learning Objectives", enabled=False, @@ -34,6 +39,7 @@ def setUp(self): enabled=True, org_owners=[self.org1], ) + # Tags self.tag_disabled = Tag.objects.create( taxonomy=self.taxonomy_disabled, value="learning", @@ -50,6 +56,7 @@ def setUp(self): taxonomy=self.taxonomy_one_org, value="learning", ) + # ObjectTags self.all_orgs_course_tag = api.tag_object( taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.id], @@ -108,28 +115,52 @@ class TestAPIContentTaxonomy(TestContentTaxonomyMixin, TestCase): Tests the ContentTaxonomy APIs. """ - def test_content_taxonomy_enabled_for_all_orgs(self): + def test_get_taxonomies_enabled_subclasses(self): with self.assertNumQueries(1): taxonomies = list(api.get_taxonomies()) assert taxonomies == [ - self.taxonomy_all_orgs, - ] - - def test_content_taxonomy_enabled_for_org1(self): - with self.assertNumQueries(1): - taxonomies = list(api.get_taxonomies(self.org1)) - assert taxonomies == [ + self.taxonomy_base, self.taxonomy_all_orgs, self.taxonomy_one_org, self.taxonomy_both_orgs, ] - def test_content_taxonomy_enabled_for_org2(self): + @ddt.data( + # All orgs + (None, True, ["taxonomy_all_orgs"]), + (None, False, ["taxonomy_disabled"]), + (None, None, ["taxonomy_all_orgs", "taxonomy_disabled"]), + # Org 1 + ("org1", True, ["taxonomy_all_orgs", "taxonomy_one_org", "taxonomy_both_orgs"]), + ("org1", False, ["taxonomy_disabled"]), + ( + "org1", + None, + [ + "taxonomy_all_orgs", + "taxonomy_disabled", + "taxonomy_one_org", + "taxonomy_both_orgs", + ], + ), + # Org 2 + ("org2", True, ["taxonomy_all_orgs", "taxonomy_both_orgs"]), + ("org2", False, ["taxonomy_disabled"]), + ( + "org2", + None, + ["taxonomy_all_orgs", "taxonomy_disabled", "taxonomy_both_orgs"], + ), + ) + @ddt.unpack + def test_get_taxonomies_for_org(self, org_attr, enabled, expected): + org_owner = getattr(self, org_attr) if org_attr else None with self.assertNumQueries(1): - taxonomies = list(api.get_taxonomies(self.org2)) + taxonomies = list( + api.get_taxonomies_for_org(org_owner=org_owner, enabled=enabled) + ) assert taxonomies == [ - self.taxonomy_all_orgs, - self.taxonomy_both_orgs, + getattr(self, taxonomy_attr) for taxonomy_attr in expected ] @ddt.data( @@ -143,10 +174,12 @@ def test_content_taxonomy_enabled_for_org2(self): def test_get_object_tags_valid_for_org( self, taxonomy_attr, object_tag_attr, num_queries ): + taxonomy_id = getattr(self, taxonomy_attr).id + taxonomy = api.get_taxonomy(taxonomy_id) object_tag = getattr(self, object_tag_attr) with self.assertNumQueries(num_queries): valid_tags = api.get_object_tags( - getattr(self, taxonomy_attr), + taxonomy, object_tag.object_id, object_tag.object_type, valid_only=True, @@ -167,16 +200,36 @@ def test_get_object_tags_valid_for_org( def test_get_object_tags_include_invalid( self, taxonomy_attr, object_tag_attr, num_queries ): + taxonomy_id = getattr(self, taxonomy_attr).id + taxonomy = api.get_taxonomy(taxonomy_id) object_tag = getattr(self, object_tag_attr) with self.assertNumQueries(num_queries): valid_tags = api.get_object_tags( - getattr(self, taxonomy_attr), + taxonomy, object_tag.object_id, object_tag.object_type, valid_only=False, ) assert valid_tags == [object_tag] + @ddt.data( + "all_orgs_invalid_tag", + "one_org_invalid_org_tag", + ) + def test_validate_object_tag_check_object(self, tag_attr): + """ + Check that the subclass is correctly checking the object part of the ObjectTag. + """ + object_tag = getattr(self, tag_attr) + assert not object_tag.is_valid + taxonomy = api.get_taxonomy(object_tag.taxonomy_id) + assert not taxonomy.validate_object_tag( + object_tag, check_object=True, check_taxonomy=False, check_tag=False + ) + assert taxonomy.validate_object_tag( + object_tag, check_object=False, check_taxonomy=True, check_tag=True + ) + def test_get_tags(self): assert api.get_tags(self.taxonomy_all_orgs) == [self.tag_all_orgs] diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f1aead715ad6..356e6c19606e 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -794,7 +794,7 @@ openedx-filters==1.3.0 # -r requirements/edx/base.in # lti-consumer-xblock # skill-tagging -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-inheritance-fix # via -r requirements/edx/github.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 9f33fd033dec..3dc9089b0f4d 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1054,7 +1054,7 @@ openedx-filters==1.3.0 # -r requirements/edx/testing.txt # lti-consumer-xblock # skill-tagging -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-inheritance-fix # via -r requirements/edx/testing.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/testing.txt diff --git a/requirements/edx/github.in b/requirements/edx/github.in index 9fc7795987ff..a35a9b39e857 100644 --- a/requirements/edx/github.in +++ b/requirements/edx/github.in @@ -80,7 +80,7 @@ # ... add dependencies here # FIXME JV - move to base.in once pypi package created -git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api#egg=openedx-learning==0.1 +git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-inheritance-fix#egg=openedx-learning==0.1 ############################################################################## diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 1cf248952c39..8d30bac131cd 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -999,7 +999,7 @@ openedx-filters==1.3.0 # -r requirements/edx/base.txt # lti-consumer-xblock # skill-tagging -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-api +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/taxonomy-inheritance-fix # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt