From 49cc8360e24367d21b2c9035f551cbc6ee437224 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 5 Sep 2023 09:21:50 -0300 Subject: [PATCH 01/25] refactor: simplify CMS tagging rules --- openedx/features/content_tagging/rules.py | 48 ++++------------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index df256b9331dd..cc602b579d6f 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -1,5 +1,7 @@ """Django rules-based permissions for tagging""" +from __future__ import annotations + import openedx_tagging.core.tagging.rules as oel_tagging import rules from django.contrib.auth import get_user_model @@ -35,48 +37,12 @@ def is_taxonomy_user(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: return False -def is_taxonomy_admin(user: User) -> bool: - """ - Returns True if the given user is a Taxonomy Admin. - - Taxonomy Admins include global staff and superusers. - """ - return oel_tagging.is_taxonomy_admin(user) - - -@rules.predicate -def can_view_taxonomy(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: - """ - Everyone can potentially view a taxonomy (taxonomy=None). The object permission must be checked - to determine if the user can view a specific taxonomy. - Only taxonomy admins can view a disabled taxonomy. - """ - if not taxonomy: - return True - - taxonomy = taxonomy.cast() - - return taxonomy.enabled or is_taxonomy_admin(user) - - @rules.predicate def can_add_taxonomy(user: User) -> bool: """ Only taxonomy admins can add taxonomies. """ - return is_taxonomy_admin(user) - - -@rules.predicate -def can_change_taxonomy(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: - """ - Only taxonomy admins can change a taxonomies. - Even taxonomy admins cannot change system taxonomies. - """ - if taxonomy: - taxonomy = taxonomy.cast() - - return (not taxonomy or (not taxonomy.system_defined)) and is_taxonomy_admin(user) + return oel_tagging.is_taxonomy_admin(user) @rules.predicate @@ -88,7 +54,7 @@ def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: taxonomy = tag.taxonomy if tag else None if taxonomy: taxonomy = taxonomy.cast() - return is_taxonomy_admin(user) and ( + 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) @@ -110,9 +76,9 @@ def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) # Taxonomy rules.set_perm("oel_tagging.add_taxonomy", can_add_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) +rules.set_perm("oel_tagging.change_taxonomy", oel_tagging.can_change_taxonomy) +rules.set_perm("oel_tagging.delete_taxonomy", oel_tagging.can_change_taxonomy) +rules.set_perm("oel_tagging.view_taxonomy", oel_tagging.can_view_taxonomy) # Tag rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) From 74c72f257e4d21b491b1b093559a521c0c3960de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 5 Sep 2023 19:02:51 -0300 Subject: [PATCH 02/25] fix: permissions and tests --- .../content_tagging/rest_api/v1/filters.py | 4 +- .../rest_api/v1/tests/test_views.py | 31 ++++++++++++- openedx/features/content_tagging/rules.py | 44 ++++++++----------- 3 files changed, 50 insertions(+), 29 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/filters.py b/openedx/features/content_tagging/rest_api/v1/filters.py index ee8771d17ee8..9ad192f545de 100644 --- a/openedx/features/content_tagging/rest_api/v1/filters.py +++ b/openedx/features/content_tagging/rest_api/v1/filters.py @@ -4,7 +4,7 @@ from rest_framework.filters import BaseFilterBackend -from ...rules import is_taxonomy_admin +import openedx_tagging.core.tagging.rules as oel_tagging class UserOrgFilterBackend(BaseFilterBackend): @@ -15,7 +15,7 @@ class UserOrgFilterBackend(BaseFilterBackend): """ def filter_queryset(self, request, queryset, _): - if is_taxonomy_admin(request.user): + if oel_tagging.is_taxonomy_admin(request.user): return queryset return queryset.filter(enabled=True) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index a8177426eb4a..1f8c077e584e 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -23,6 +23,7 @@ TAXONOMY_ORG_LIST_URL = "/api/content_tagging/v1/taxonomies/" TAXONOMY_ORG_DETAIL_URL = "/api/content_tagging/v1/taxonomies/{pk}/" +OBJECT_TAG_LIST_URL = "/api/content_tagging/v1/object_tags/{object_id}/" def check_taxonomy( @@ -52,7 +53,7 @@ def check_taxonomy( assert data["visible_to_authors"] == visible_to_authors -class TestTaxonomyViewSetMixin: +class TestTaxonomyObjectsMixin: """ Sets up data for testing Content Taxonomies. """ @@ -168,7 +169,7 @@ def setUp(self): @skip_unless_cms @ddt.ddt @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) -class TestTaxonomyViewSet(TestTaxonomyViewSetMixin, APITestCase): +class TestTaxonomyViewSet(TestTaxonomyObjectsMixin, APITestCase): """ Test cases for TaxonomyViewSet when ENABLE_CREATOR_GROUP is True """ @@ -639,3 +640,29 @@ class TestTaxonomyViewSetNoCreatorGroup(TestTaxonomyViewSet): # pylint: disable The permissions are the same for when ENABLED_CREATOR_GRUP is True """ + +@skip_unless_cms +@ddt.ddt +class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): + """ + Testing various cases for the ObjectTagView. + """ + def setUp(self): + super().setUp() + + + @ddt.data( + (None, status.HTTP_403_FORBIDDEN), + ) + @ddt.unpack + def test_tag_object(self, user_attr, expected_status): + url = OBJECT_TAG_LIST_URL.format(object_id="abc") + + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + response = self.client.put(url, format="json") + assert response.status_code == expected_status + + diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index cc602b579d6f..8130ad11173a 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -2,18 +2,20 @@ from __future__ import annotations +from typing import Union + +import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging import rules -from django.contrib.auth import get_user_model -from common.djangoapps.student.auth import is_content_creator +from common.djangoapps.student.auth import is_content_creator, has_studio_write_access from .models import TaxonomyOrg -User = get_user_model() +UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] -def is_taxonomy_user(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: +def is_taxonomy_user(user: UserType, taxonomy: oel_tagging.Taxonomy | None = None) -> bool: """ Returns True if the given user is a Taxonomy User for the given content taxonomy. @@ -38,15 +40,16 @@ def is_taxonomy_user(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: @rules.predicate -def can_add_taxonomy(user: User) -> bool: +def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: """ - Only taxonomy admins can add taxonomies. + Everyone that has permission to edit the object should be able to tag it. """ - return oel_tagging.is_taxonomy_admin(user) + + return has_studio_write_access(user, object_id) @rules.predicate -def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: +def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = 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). @@ -61,21 +64,8 @@ def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: ) -@rules.predicate -def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) -> bool: - """ - Taxonomy users can create or modify object tags on enabled taxonomies. - """ - taxonomy = object_tag.taxonomy if object_tag else None - if taxonomy: - taxonomy = taxonomy.cast() - return is_taxonomy_user(user, taxonomy) and ( - not object_tag or not taxonomy or (taxonomy and taxonomy.cast().enabled) - ) - - # Taxonomy -rules.set_perm("oel_tagging.add_taxonomy", can_add_taxonomy) +rules.set_perm("oel_tagging.add_taxonomy", oel_tagging.is_taxonomy_admin) rules.set_perm("oel_tagging.change_taxonomy", oel_tagging.can_change_taxonomy) rules.set_perm("oel_tagging.delete_taxonomy", oel_tagging.can_change_taxonomy) rules.set_perm("oel_tagging.view_taxonomy", oel_tagging.can_view_taxonomy) @@ -87,7 +77,11 @@ def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) 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.add_object_tag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.change_object_tag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.delete_object_tag", oel_tagging.can_change_object_tag) rules.set_perm("oel_tagging.view_object_tag", rules.always_allow) + +# Users can tag objects using tags from any taxonomy that they have permission to view +rules.set_perm("oel_tagging.change_objecttag_taxonomy", is_taxonomy_user) +rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From b9a8cd7ca5f8e479636036fdbd4fed50fb87978a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 6 Sep 2023 10:20:15 -0300 Subject: [PATCH 03/25] style: fix pep8 --- .../features/content_tagging/rest_api/v1/tests/test_views.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 1f8c077e584e..f69074196ac6 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -641,6 +641,7 @@ class TestTaxonomyViewSetNoCreatorGroup(TestTaxonomyViewSet): # pylint: disable The permissions are the same for when ENABLED_CREATOR_GRUP is True """ + @skip_unless_cms @ddt.ddt class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): @@ -650,7 +651,6 @@ class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): def setUp(self): super().setUp() - @ddt.data( (None, status.HTTP_403_FORBIDDEN), ) @@ -664,5 +664,3 @@ def test_tag_object(self, user_attr, expected_status): response = self.client.put(url, format="json") assert response.status_code == expected_status - - From 3593a7ab289d9af7bc97bf70837e1faf482af62a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 6 Sep 2023 13:07:32 -0300 Subject: [PATCH 04/25] chore: update deps --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 0c1ba85d50a1..20e7342b1ba8 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -777,7 +777,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.1.5 +openedx-learning==0.1.6 # via -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index e9a1794f3ed3..e99d82c5f7dc 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.1.5 +openedx-learning==0.1.6 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 7d6d0d5f4766..c1d6ab79bf95 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -918,7 +918,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.5 +openedx-learning==0.1.6 # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index dbf5092a9dbe..7a4e390ec7a3 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -988,7 +988,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.5 +openedx-learning==0.1.6 # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt From bf0fcf3750a79564687f6edc1f362a868b6f2ef8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 6 Sep 2023 13:07:39 -0300 Subject: [PATCH 05/25] fix: permissions and add tests --- .../rest_api/v1/tests/test_views.py | 44 ++++++++++++++----- openedx/features/content_tagging/rules.py | 22 ++++++---- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index f69074196ac6..7730bbb3e770 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -7,6 +7,7 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import override_settings +from opaque_keys.edx.locator import CourseLocator from openedx_tagging.core.tagging.models import Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer @@ -14,8 +15,9 @@ from rest_framework import status from rest_framework.test import APITestCase -from common.djangoapps.student.auth import update_org_role -from common.djangoapps.student.roles import OrgContentCreatorRole +from common.djangoapps.student.auth import add_users, update_org_role +from common.djangoapps.student.roles import CourseStaffRole, OrgContentCreatorRole +from common.djangoapps.student.tests.factories import AdminFactory, UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.features.content_tagging.models import TaxonomyOrg @@ -23,7 +25,7 @@ TAXONOMY_ORG_LIST_URL = "/api/content_tagging/v1/taxonomies/" TAXONOMY_ORG_DETAIL_URL = "/api/content_tagging/v1/taxonomies/{pk}/" -OBJECT_TAG_LIST_URL = "/api/content_tagging/v1/object_tags/{object_id}/" +OBJECT_TAG_UPDATE_URL = "/api/content_tagging/v1/object_tags/{object_id}/?taxonomy={taxonomy_id}" def check_taxonomy( @@ -114,13 +116,13 @@ def setUp(self): ) # OrgA taxonomy - self.tA1 = Taxonomy.objects.create(name="tA1", enabled=True) + self.tA1 = Taxonomy.objects.create(name="tA1", enabled=True, allow_free_text=True) TaxonomyOrg.objects.create( taxonomy=self.tA1, org=self.orgA, rel_type=TaxonomyOrg.RelType.OWNER, ) - self.tA2 = Taxonomy.objects.create(name="tA2", enabled=False) + self.tA2 = Taxonomy.objects.create(name="tA2", enabled=False, allow_free_text=True) TaxonomyOrg.objects.create( taxonomy=self.tA2, org=self.orgA, @@ -454,7 +456,7 @@ def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status): @ddt.unpack def test_update_taxonomy_system_defined(self, update_value, expected_status): """ - Test that we can't update system_defined field + Test that we can"t update system_defined field """ url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) @@ -550,7 +552,7 @@ def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status): @ddt.unpack def test_patch_taxonomy_system_defined(self, update_value, expected_status): """ - Test that we can't patch system_defined field + Test that we can"t patch system_defined field """ url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) @@ -625,7 +627,7 @@ def test_delete_taxonomy(self, user_attr, taxonomy_attr, expected_status): response = self.client.delete(url) assert response.status_code == expected_status - # If we were able to delete the taxonomy, check that it's really gone + # If we were able to delete the taxonomy, check that it"s really gone if status.is_success(expected_status): response = self.client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND @@ -650,17 +652,35 @@ class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): """ def setUp(self): super().setUp() + self.courseA = CourseLocator("orgA", "101", "test") + add_users(self.userS, CourseStaffRole(self.courseA), self.userA) @ddt.data( - (None, status.HTTP_403_FORBIDDEN), + # userA and userS are staff in courseA + (None, "courseA", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "courseA", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "courseA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("userS", "courseA", "tA1", ["Tag 1"], status.HTTP_200_OK), + # Only userS is Tagging Admin and can tag using disable taxonomies + (None, "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userS", "courseA", "tA2", ["Tag 1"], status.HTTP_200_OK), ) @ddt.unpack - def test_tag_object(self, user_attr, expected_status): - url = OBJECT_TAG_LIST_URL.format(object_id="abc") + def test_tag_coure(self, user_attr, course_attr, taxonomy_attr, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.put(url, format="json") + course = getattr(self, course_attr) + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=course, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + + if response.status_code != expected_status: + breakpoint() assert response.status_code == expected_status diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index 8130ad11173a..d073d3f9b184 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -7,6 +7,7 @@ import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging import rules +from opaque_keys.edx.keys import CourseKey, UsageKey from common.djangoapps.student.auth import is_content_creator, has_studio_write_access @@ -15,20 +16,17 @@ UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] -def is_taxonomy_user(user: UserType, taxonomy: oel_tagging.Taxonomy | None = None) -> bool: +def is_taxonomy_user(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: """ Returns True if the given user is a Taxonomy User for the given content taxonomy. Taxonomy users include global staff and superusers, plus course creators who can create courses for any org. - Otherwise, we need a taxonomy provided to determine if the user is an org-level course creator for one of the - orgs allowed to use this taxonomy. + Otherwise, we need to check taxonomy provided to determine if the user is an org-level course creator for one of + the orgs allowed to use this taxonomy. Only global staff and superusers can use disabled system taxonomies. """ if oel_tagging.is_taxonomy_admin(user): return True - if not taxonomy: - return is_content_creator(user, None) - taxonomy_orgs = TaxonomyOrg.get_organizations( taxonomy=taxonomy, rel_type=TaxonomyOrg.RelType.OWNER, @@ -44,8 +42,16 @@ 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. """ + course_key = CourseKey.from_string(object_id) + return has_studio_write_access(user, course_key) - return has_studio_write_access(user, object_id) +@rules.predicate +def can_change_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: + """ + Taxonomy users can tag objects using tags from any taxonomy that they have permission to view. Only taxonomy admins + can tag objects using tags from disabled taxonomies. + """ + return oel_tagging.is_taxonomy_admin(user) or (taxonomy.cast().enabled and is_taxonomy_user(user, taxonomy)) @rules.predicate @@ -83,5 +89,5 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) rules.set_perm("oel_tagging.view_object_tag", rules.always_allow) # Users can tag objects using tags from any taxonomy that they have permission to view -rules.set_perm("oel_tagging.change_objecttag_taxonomy", is_taxonomy_user) +rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_change_object_tag_taxonomy) rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From 0fe4886f5125f2e7776d42b4b895be5e1cd9e914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 11 Sep 2023 19:48:46 -0300 Subject: [PATCH 06/25] test: add more tests --- .../rest_api/v1/tests/test_views.py | 109 +++++++++++++++--- 1 file changed, 90 insertions(+), 19 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 7730bbb3e770..339f99b91183 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -8,7 +8,7 @@ from django.contrib.auth import get_user_model from django.test.testcases import override_settings from opaque_keys.edx.locator import CourseLocator -from openedx_tagging.core.tagging.models import Taxonomy +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer from organizations.models import Organization @@ -116,13 +116,13 @@ def setUp(self): ) # OrgA taxonomy - self.tA1 = Taxonomy.objects.create(name="tA1", enabled=True, allow_free_text=True) + self.tA1 = Taxonomy.objects.create(name="tA1", enabled=True) TaxonomyOrg.objects.create( taxonomy=self.tA1, org=self.orgA, rel_type=TaxonomyOrg.RelType.OWNER, ) - self.tA2 = Taxonomy.objects.create(name="tA2", enabled=False, allow_free_text=True) + self.tA2 = Taxonomy.objects.create(name="tA2", enabled=False) TaxonomyOrg.objects.create( taxonomy=self.tA2, org=self.orgA, @@ -651,36 +651,107 @@ class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): Testing various cases for the ObjectTagView. """ def setUp(self): + """ + Setup the test cases + """ super().setUp() self.courseA = CourseLocator("orgA", "101", "test") + + self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) + self.required_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", required=True) + for i in range(20): + # Valid ObjectTags + Tag.objects.create(taxonomy=self.tA1, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.tA2, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.multiple_taxonomy, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.required_taxonomy, value=f"Tag {i}") + + self.open_taxonomy = Taxonomy.objects.create(name="Enabled Free-Text Taxonomy", allow_free_text=True) + + # Add org permissions to taxonomy + TaxonomyOrg.objects.create( + taxonomy=self.multiple_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.required_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.open_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + add_users(self.userS, CourseStaffRole(self.courseA), self.userA) @ddt.data( - # userA and userS are staff in courseA - (None, "courseA", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("user", "courseA", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("userA", "courseA", "tA1", ["Tag 1"], status.HTTP_200_OK), - ("userS", "courseA", "tA1", ["Tag 1"], status.HTTP_200_OK), - # Only userS is Tagging Admin and can tag using disable taxonomies - (None, "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("user", "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("userA", "courseA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("userS", "courseA", "tA2", ["Tag 1"], status.HTTP_200_OK), + # userA and userS are staff in courseA and can tag using enabled taxonomies + (None, "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("userS", "tA1", ["Tag 1"], status.HTTP_200_OK), + (None, "tA1", [], status.HTTP_403_FORBIDDEN), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", [], status.HTTP_200_OK), + ("userS", "tA1", [], status.HTTP_200_OK), + (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("userS", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + (None, "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("userA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("userS", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + # Only userS is Tagging Admin and can tag objects using disabled taxonomies + (None, "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userS", "tA2", ["Tag 1"], status.HTTP_200_OK), ) @ddt.unpack - def test_tag_coure(self, user_attr, course_attr, taxonomy_attr, tag_values, expected_status): - + def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status): if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - course = getattr(self, course_attr) taxonomy = getattr(self, taxonomy_attr) - url = OBJECT_TAG_UPDATE_URL.format(object_id=course, taxonomy_id=taxonomy.pk) + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseA, taxonomy_id=taxonomy.pk) response = self.client.put(url, {"tags": tag_values}, format="json") - if response.status_code != expected_status: - breakpoint() assert response.status_code == expected_status + if status.is_success(expected_status): + assert len(response.data.get("results")) == len(tag_values) + assert set(t["value"] for t in response.data["results"]) == set(tag_values) + + @ddt.data( + # Can't add invalid tags to a object using a closed taxonomy + (None, "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + (None, "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + # Staff can't add invalid tags to a object using a closed taxonomy + ("userS", "tA2", ["invalid"], status.HTTP_400_BAD_REQUEST), + ) + @ddt.unpack + def test_tag_course_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + assert response.status_code == expected_status + assert not status.is_success(expected_status) # No success cases here + From 1a6c014797ecf39383e889d58642e7dcc8605d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 12 Sep 2023 14:24:46 -0300 Subject: [PATCH 07/25] feat: add xblock tagging support --- .../rest_api/v1/tests/test_views.py | 78 ++++++++++++++++++- openedx/features/content_tagging/rules.py | 12 ++- 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 339f99b91183..a7bd663121f6 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -7,8 +7,8 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import override_settings -from opaque_keys.edx.locator import CourseLocator -from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from openedx_tagging.core.tagging.models import Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer from organizations.models import Organization @@ -17,7 +17,6 @@ from common.djangoapps.student.auth import add_users, update_org_role from common.djangoapps.student.roles import CourseStaffRole, OrgContentCreatorRole -from common.djangoapps.student.tests.factories import AdminFactory, UserFactory from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.features.content_tagging.models import TaxonomyOrg @@ -656,6 +655,11 @@ def setUp(self): """ super().setUp() self.courseA = CourseLocator("orgA", "101", "test") + self.xblockA = BlockUsageLocator( + course_key=self.courseA, + block_type='problem', + block_id='block_id' + ) self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) self.required_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", required=True) @@ -755,3 +759,71 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr, tag_values, expected assert response.status_code == expected_status assert not status.is_success(expected_status) # No success cases here + @ddt.data( + # userA and userS are staff in courseA (owner of xblockA) and can tag using enabled taxonomies + (None, "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("userS", "tA1", ["Tag 1"], status.HTTP_200_OK), + (None, "tA1", [], status.HTTP_403_FORBIDDEN), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", [], status.HTTP_200_OK), + ("userS", "tA1", [], status.HTTP_200_OK), + (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("userS", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + (None, "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("userA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("userS", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + # Only userS is Tagging Admin and can tag objects using disabled taxonomies + (None, "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userS", "tA2", ["Tag 1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_xblock(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.xblockA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + + assert response.status_code == expected_status + if status.is_success(expected_status): + assert len(response.data.get("results")) == len(tag_values) + assert set(t["value"] for t in response.data["results"]) == set(tag_values) + + @ddt.data( + # Can't add invalid tags to a object using a closed taxonomy + (None, "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + (None, "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + # Staff can't add invalid tags to a object using a closed taxonomy + ("userS", "tA2", ["invalid"], status.HTTP_400_BAD_REQUEST), + ) + @ddt.unpack + def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.xblockA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + assert response.status_code == expected_status + assert not status.is_success(expected_status) # No success cases here + diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index d073d3f9b184..2cd7ba53a564 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -7,6 +7,7 @@ import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging import rules +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from common.djangoapps.student.auth import is_content_creator, has_studio_write_access @@ -42,7 +43,16 @@ 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. """ - course_key = CourseKey.from_string(object_id) + if not object_id: + raise ValueError("object_id must be provided") + try: + usage_key = UsageKey.from_string(object_id) + if not usage_key.course_key.is_course: + raise ValueError("object_id must be from a block or a course") + course_key = usage_key.course_key + except InvalidKeyError: + course_key = CourseKey.from_string(object_id) + return has_studio_write_access(user, course_key) @rules.predicate From 03878b790c9f79e90a118a1198fbfe4dc60ec9bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 12 Sep 2023 14:33:07 -0300 Subject: [PATCH 08/25] chore: temp update requirements --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 20e7342b1ba8..179f262a24c6 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -777,7 +777,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.1.6 +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged # via -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index e99d82c5f7dc..7f2d71086235 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.1.6 +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index c1d6ab79bf95..e6889bf86eee 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -918,7 +918,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.6 +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 7a4e390ec7a3..a9c43ea0ede8 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -988,7 +988,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.6 +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt From 1a5974129d957ef52ad8434ead8c2518c2476907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 12 Sep 2023 14:49:16 -0300 Subject: [PATCH 09/25] fix: requirements --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 3 ++- requirements/edx/testing.txt | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 179f262a24c6..9512efb65494 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -777,7 +777,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # via -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 7f2d71086235..3bb9e1606e07 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 @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index e6889bf86eee..8a704a9bd49b 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -918,7 +918,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 8c0258f55f94..a9d3dc3be177 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -115,7 +115,8 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require openedx-events>=8.3.0 # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) -openedx-learning # Open edX Learning core (experimental) +# openedx-learning # Open edX Learning core (experimental) +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged openedx-mongodbproxy openedx-django-wiki openedx-blockstore diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index a9c43ea0ede8..237530f031b0 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -988,7 +988,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged +openedx-learning @ git+https://github.com/openedx/openedx-learning@main # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt From 42168020fdbd781e9f200e1820b41e0724258322 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 12 Sep 2023 17:16:49 -0300 Subject: [PATCH 10/25] style: fix pep8 --- openedx/features/content_tagging/rest_api/v1/tests/test_views.py | 1 - openedx/features/content_tagging/rules.py | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index a7bd663121f6..b3ba305011f0 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -826,4 +826,3 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected response = self.client.put(url, {"tags": tag_values}, format="json") assert response.status_code == expected_status assert not status.is_success(expected_status) # No success cases here - diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index 2cd7ba53a564..562f77253dcc 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -55,6 +55,7 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) + @rules.predicate def can_change_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: """ From d39f623132e6507ff7c98f6a5cc54fdffdb0ddcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 12 Sep 2023 19:31:30 -0300 Subject: [PATCH 11/25] test: fixing test_rules WIP --- .../content_tagging/tests/test_api.py | 45 +++++++ .../content_tagging/tests/test_rules.py | 112 ++++++++++-------- 2 files changed, 105 insertions(+), 52 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py index 263ae761ef57..3563c8243b41 100644 --- a/openedx/features/content_tagging/tests/test_api.py +++ b/openedx/features/content_tagging/tests/test_api.py @@ -3,6 +3,7 @@ from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import ObjectTag, Tag +from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem from organizations.models import Organization from .. import api @@ -69,6 +70,12 @@ def setUp(self): tags=[self.tag_all_orgs.id], object_id=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), )[0] + self.all_orgs_course_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id="course-v1:OeX+DemoX+Demo_Course", + ) + + self.all_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.id], @@ -76,11 +83,21 @@ def setUp(self): "block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde" ), )[0] + self.all_orgs_block_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", + ) + self.both_orgs_course_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.id], object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), )[0] + self.both_orgs_course_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id="course-v1:Ax+DemoX+Demo_Course", + ) + self.both_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.id], @@ -88,6 +105,11 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde" ), )[0] + self.both_orgs_block_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id="block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde", + ) + self.one_org_block_tag = api.tag_content_object( taxonomy=self.taxonomy_one_org, tags=[self.tag_one_org.id], @@ -95,11 +117,20 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde" ), )[0] + self.one_org_block_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id="block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde", + ) + self.disabled_course_tag = api.tag_content_object( taxonomy=self.taxonomy_disabled, tags=[self.tag_disabled.id], object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), )[0] + self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_disabled, + object_id="course-v1:Ax+DemoX+Demo_Course", + ) # Invalid object tags must be manually created self.all_orgs_invalid_tag = ObjectTag.objects.create( @@ -107,16 +138,30 @@ def setUp(self): tag=self.tag_all_orgs, object_id="course-v1_OpenedX_DemoX_Demo_Course", ) + self.all_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id="course-v1_OpenedX_DemoX_Demo_Course", + ) + self.one_org_invalid_org_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_one_org, tag=self.tag_one_org, object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", ) + self.one_org_invalid_org_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", + ) + self.no_orgs_invalid_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_no_orgs, tag=self.tag_no_orgs, object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), ) + self.no_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id="course-v1:Ax+DemoX+Demo_Course", + ) @ddt.ddt diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 0bb0e538156c..c991b32d213a 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -3,6 +3,8 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import TestCase, override_settings +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx_tagging.core.tagging.models import ( ObjectTag, Tag, @@ -11,7 +13,7 @@ from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role -from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole +from common.djangoapps.student.roles import CourseCreatorRole, CourseStaffRole, OrgContentCreatorRole from .. import api from .test_api import TestTaxonomyMixin @@ -30,6 +32,7 @@ class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): def setUp(self): super().setUp() + self.superuser = User.objects.create( username="superuser", email="superuser@example.com", @@ -47,6 +50,7 @@ def setUp(self): ) 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", @@ -74,6 +78,20 @@ def setUp(self): email="learner@example.com", ) + self.courseA = CourseLocator(self.org1.short_name, "DemoX", "Demo_Course") + self.courseB = CourseLocator(self.org2.short_name, "DemoX", "Demo_Course") + + self.xblockA = BlockUsageLocator( + course_key=self.courseA, + block_type='problem', + block_id='block_id' + ) + + add_users(self.staff, CourseStaffRole(self.courseA), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.courseA), self.user_both_orgs) + add_users(self.staff, CourseStaffRole(self.courseB), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.courseB), self.user_both_orgs) + def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True ): @@ -362,20 +380,20 @@ def test_view_tag(self, tag_attr): # 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"), + ("oel_tagging.add_object_tag", "disabled_course_tag_perm"), + ("oel_tagging.change_object_tag", "disabled_course_tag_perm"), + ("oel_tagging.delete_object_tag", "disabled_course_tag_perm"), ) @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 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) + """Only taxonomy administrators can create/edit an ObjectTag using a disabled Taxonomy""" + object_tag_perm = getattr(self, tag_attr) + assert self.superuser.has_perm(perm, object_tag_perm) + assert self.staff.has_perm(perm, object_tag_perm) + assert not self.user_all_orgs.has_perm(perm, object_tag_perm) + assert not self.user_both_orgs.has_perm(perm, object_tag_perm) + assert not self.user_org2.has_perm(perm, object_tag_perm) + assert not self.learner.has_perm(perm, object_tag_perm) @ddt.data( ("oel_tagging.add_object_tag", "no_orgs_invalid_tag"), @@ -394,21 +412,18 @@ def test_object_tag_no_orgs(self, perm, tag_attr): 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"), + ("oel_tagging.add_object_tag", "all_orgs_course_tag_perm"), + ("oel_tagging.add_object_tag", "all_orgs_block_tag_perm"), + ("oel_tagging.add_object_tag", "both_orgs_course_tag_perm"), + ("oel_tagging.add_object_tag", "both_orgs_block_tag_perm"), + ("oel_tagging.change_object_tag", "all_orgs_course_tag_perm"), + ("oel_tagging.change_object_tag", "all_orgs_block_tag_perm"), + ("oel_tagging.change_object_tag", "both_orgs_course_tag_perm"), + ("oel_tagging.change_object_tag", "both_orgs_block_tag_perm"), + ("oel_tagging.delete_object_tag", "all_orgs_course_tag_perm"), + ("oel_tagging.delete_object_tag", "all_orgs_block_tag_perm"), + ("oel_tagging.delete_object_tag", "both_orgs_course_tag_perm"), + ("oel_tagging.delete_object_tag", "both_orgs_block_tag_perm"), ) @ddt.unpack def test_change_object_tag_all_orgs(self, perm, tag_attr): @@ -417,12 +432,9 @@ def test_change_object_tag_all_orgs(self, perm, 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"), + ("oel_tagging.add_object_tag", "one_org_block_tag_perm"), + ("oel_tagging.change_object_tag", "one_org_block_tag_perm"), + ("oel_tagging.delete_object_tag", "one_org_block_tag_perm"), ) @ddt.unpack def test_change_object_tag_org1(self, perm, tag_attr): @@ -431,24 +443,20 @@ def test_change_object_tag_org1(self, perm, 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) + ("oel_tagging.add_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.add_object_tag", "all_orgs_invalid_tag_perm"), + ("oel_tagging.change_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.change_object_tag", "all_orgs_invalid_tag_perm"), + ("oel_tagging.delete_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.delete_object_tag", "all_orgs_invalid_tag_perm"), + ) + @ddt.unpack + def test_change_object_tag_invalid_key(self, perm, tag_attr): + object_tag = getattr(self, tag_attr) + with self.assertRaises(InvalidKeyError): + self._expected_users_have_perm(perm, object_tag, user_org2=False) - # 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", @@ -493,9 +501,9 @@ def _expected_users_have_perm( super()._expected_users_have_perm( perm=perm, obj=obj, - learner_perm=True, - learner_obj=True, - user_org2=True, + learner_perm=learner_perm, + learner_obj=learner_obj, + user_org2=user_org2, ) @ddt.data( From 06dceb9b3a128113be3c17c76e0bbbcf4e659f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 10:20:07 -0300 Subject: [PATCH 12/25] test: fix test_rules --- .../content_tagging/tests/test_api.py | 38 ---- .../content_tagging/tests/test_rules.py | 214 +++++++++++++----- 2 files changed, 161 insertions(+), 91 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py index 3563c8243b41..a18c95d37711 100644 --- a/openedx/features/content_tagging/tests/test_api.py +++ b/openedx/features/content_tagging/tests/test_api.py @@ -3,7 +3,6 @@ from django.test.testcases import TestCase from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import ObjectTag, Tag -from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem from organizations.models import Organization from .. import api @@ -70,11 +69,6 @@ def setUp(self): tags=[self.tag_all_orgs.id], object_id=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), )[0] - self.all_orgs_course_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_all_orgs, - object_id="course-v1:OeX+DemoX+Demo_Course", - ) - self.all_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_all_orgs, @@ -83,20 +77,12 @@ def setUp(self): "block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde" ), )[0] - self.all_orgs_block_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_all_orgs, - object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", - ) self.both_orgs_course_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.id], object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), )[0] - self.both_orgs_course_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_both_orgs, - object_id="course-v1:Ax+DemoX+Demo_Course", - ) self.both_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, @@ -105,10 +91,6 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde" ), )[0] - self.both_orgs_block_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_both_orgs, - object_id="block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde", - ) self.one_org_block_tag = api.tag_content_object( taxonomy=self.taxonomy_one_org, @@ -117,20 +99,12 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde" ), )[0] - self.one_org_block_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_one_org, - object_id="block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde", - ) self.disabled_course_tag = api.tag_content_object( taxonomy=self.taxonomy_disabled, tags=[self.tag_disabled.id], object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), )[0] - self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_disabled, - object_id="course-v1:Ax+DemoX+Demo_Course", - ) # Invalid object tags must be manually created self.all_orgs_invalid_tag = ObjectTag.objects.create( @@ -138,30 +112,18 @@ def setUp(self): tag=self.tag_all_orgs, object_id="course-v1_OpenedX_DemoX_Demo_Course", ) - self.all_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_all_orgs, - object_id="course-v1_OpenedX_DemoX_Demo_Course", - ) self.one_org_invalid_org_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_one_org, tag=self.tag_one_org, object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", ) - self.one_org_invalid_org_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_one_org, - object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", - ) self.no_orgs_invalid_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_no_orgs, tag=self.tag_no_orgs, object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), ) - self.no_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( - taxonomy=self.taxonomy_no_orgs, - object_id="course-v1:Ax+DemoX+Demo_Course", - ) @ddt.ddt diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index c991b32d213a..232f5de12c53 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -10,6 +10,7 @@ Tag, UserSystemDefinedTaxonomy, ) +from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role @@ -78,19 +79,115 @@ def setUp(self): email="learner@example.com", ) - self.courseA = CourseLocator(self.org1.short_name, "DemoX", "Demo_Course") - self.courseB = CourseLocator(self.org2.short_name, "DemoX", "Demo_Course") + self.course1 = CourseLocator(self.org1.short_name, "DemoX", "Demo_Course") + self.course2 = CourseLocator(self.org2.short_name, "DemoX", "Demo_Course") + self.courseC = CourseLocator("orgC", "DemoX", "Demo_Course") - self.xblockA = BlockUsageLocator( - course_key=self.courseA, + self.xblock1 = BlockUsageLocator( + course_key=self.course1, block_type='problem', block_id='block_id' ) + self.xblock2 = BlockUsageLocator( + course_key=self.course2, + block_type='problem', + block_id='block_id' + ) + self.xblockC = BlockUsageLocator( + course_key=self.courseC, + block_type='problem', + block_id='block_id' + ) + + add_users(self.staff, CourseStaffRole(self.course1), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.course1), self.user_both_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_both_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) + add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) + + + self.tax_all_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.course1), + ) + self.tax_all_course2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.course2), + ) + self.tax_all_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.xblock1), + ) + self.tax_all_xblock2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.xblock2), + ) + + # self.both_orgs_course_tag_perm = ChangeObjectTagPermissionItem( + # taxonomy=self.taxonomy_both_orgs, + # object_id=str(self.course2), + # ) + # self.both_orgs_block_tag_perm = ChangeObjectTagPermissionItem( + # taxonomy=self.taxonomy_both_orgs, + # object_id=str(self.xblock1), + # ) + self.tax_both_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.course1), + ) + self.tax_both_course2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.course2), + ) + self.tax_both_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.xblock1), + ) + self.tax_both_xblock2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.xblock2), + ) + + self.tax1_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id=str(self.course1), + ) + self.tax1_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id=str(self.xblock1), + ) + + self.tax_no_org_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.course1), + ) + + self.tax_no_org_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.xblock1), + ) + + + + self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_disabled, + object_id=str(self.course2), + ) + + self.all_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id="course-v1_OpenedX_DemoX_Demo_Course", + ) + self.one_org_invalid_org_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", + ) + self.no_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.course1), + ) - add_users(self.staff, CourseStaffRole(self.courseA), self.user_all_orgs) - add_users(self.staff, CourseStaffRole(self.courseA), self.user_both_orgs) - add_users(self.staff, CourseStaffRole(self.courseB), self.user_all_orgs) - add_users(self.staff, CourseStaffRole(self.courseB), self.user_both_orgs) def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True @@ -396,9 +493,12 @@ def test_object_tag_disabled_taxonomy(self, perm, tag_attr): assert not self.learner.has_perm(perm, object_tag_perm) @ddt.data( - ("oel_tagging.add_object_tag", "no_orgs_invalid_tag"), - ("oel_tagging.change_object_tag", "no_orgs_invalid_tag"), - ("oel_tagging.delete_object_tag", "no_orgs_invalid_tag"), + ("oel_tagging.add_object_tag", "tax_no_org_course1"), + ("oel_tagging.add_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.change_object_tag", "tax_no_org_course1"), + ("oel_tagging.change_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.delete_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.delete_object_tag", "tax_no_org_course1"), ) @ddt.unpack def test_object_tag_no_orgs(self, perm, tag_attr): @@ -412,35 +512,63 @@ def test_object_tag_no_orgs(self, perm, tag_attr): assert not self.learner.has_perm(perm, object_tag) @ddt.data( - ("oel_tagging.add_object_tag", "all_orgs_course_tag_perm"), - ("oel_tagging.add_object_tag", "all_orgs_block_tag_perm"), - ("oel_tagging.add_object_tag", "both_orgs_course_tag_perm"), - ("oel_tagging.add_object_tag", "both_orgs_block_tag_perm"), - ("oel_tagging.change_object_tag", "all_orgs_course_tag_perm"), - ("oel_tagging.change_object_tag", "all_orgs_block_tag_perm"), - ("oel_tagging.change_object_tag", "both_orgs_course_tag_perm"), - ("oel_tagging.change_object_tag", "both_orgs_block_tag_perm"), - ("oel_tagging.delete_object_tag", "all_orgs_course_tag_perm"), - ("oel_tagging.delete_object_tag", "all_orgs_block_tag_perm"), - ("oel_tagging.delete_object_tag", "both_orgs_course_tag_perm"), - ("oel_tagging.delete_object_tag", "both_orgs_block_tag_perm"), + ("oel_tagging.add_object_tag", "tax_all_course1"), + ("oel_tagging.add_object_tag", "tax_all_course2"), + ("oel_tagging.add_object_tag", "tax_all_xblock1"), + ("oel_tagging.add_object_tag", "tax_all_xblock2"), + ("oel_tagging.add_object_tag", "tax_both_course1"), + ("oel_tagging.add_object_tag", "tax_both_course2"), + ("oel_tagging.add_object_tag", "tax_both_xblock1"), + ("oel_tagging.add_object_tag", "tax_both_xblock2"), + ("oel_tagging.change_object_tag", "tax_all_course1"), + ("oel_tagging.change_object_tag", "tax_all_course2"), + ("oel_tagging.change_object_tag", "tax_all_xblock1"), + ("oel_tagging.change_object_tag", "tax_all_xblock2"), + ("oel_tagging.change_object_tag", "tax_both_course1"), + ("oel_tagging.change_object_tag", "tax_both_course2"), + ("oel_tagging.change_object_tag", "tax_both_xblock1"), + ("oel_tagging.change_object_tag", "tax_both_xblock2"), + ("oel_tagging.delete_object_tag", "tax_all_course1"), + ("oel_tagging.delete_object_tag", "tax_all_course2"), + ("oel_tagging.delete_object_tag", "tax_all_xblock1"), + ("oel_tagging.delete_object_tag", "tax_all_xblock2"), + ("oel_tagging.delete_object_tag", "tax_both_course1"), + ("oel_tagging.delete_object_tag", "tax_both_course2"), + ("oel_tagging.delete_object_tag", "tax_both_xblock1"), + ("oel_tagging.delete_object_tag", "tax_both_xblock2"), ) @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) + """ + Taxonomy administrators can create/edit an ObjectTag using taxonomies in their org, + but only on objects they have write access to. + """ + perm_item = getattr(self, tag_attr) + assert self.superuser.has_perm(perm, perm_item) + assert self.staff.has_perm(perm, perm_item) + assert self.user_all_orgs.has_perm(perm, perm_item) + assert self.user_both_orgs.has_perm(perm, perm_item) + assert self.user_org2.has_perm(perm, perm_item) == (tag_attr.endswith("2")) + assert not self.learner.has_perm(perm, perm_item) @ddt.data( - ("oel_tagging.add_object_tag", "one_org_block_tag_perm"), - ("oel_tagging.change_object_tag", "one_org_block_tag_perm"), - ("oel_tagging.delete_object_tag", "one_org_block_tag_perm"), + ("oel_tagging.add_object_tag", "tax1_course1"), + ("oel_tagging.add_object_tag", "tax1_xblock1"), + ("oel_tagging.change_object_tag", "tax1_course1"), + ("oel_tagging.change_object_tag", "tax1_xblock1"), + ("oel_tagging.delete_object_tag", "tax1_course1"), + ("oel_tagging.delete_object_tag", "tax1_xblock1"), ) @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) + perm_item = getattr(self, tag_attr) + assert self.superuser.has_perm(perm, perm_item) + assert self.staff.has_perm(perm, perm_item) + assert self.user_all_orgs.has_perm(perm, perm_item) + assert self.user_both_orgs.has_perm(perm, perm_item) + assert not self.user_org2.has_perm(perm, perm_item) + assert not self.learner.has_perm(perm, perm_item) @ddt.data( @@ -453,9 +581,9 @@ def test_change_object_tag_org1(self, perm, tag_attr): ) @ddt.unpack def test_change_object_tag_invalid_key(self, perm, tag_attr): - object_tag = getattr(self, tag_attr) + perm_item = getattr(self, tag_attr) with self.assertRaises(InvalidKeyError): - self._expected_users_have_perm(perm, object_tag, user_org2=False) + assert self.staff.has_perm(perm, perm_item) @ddt.data( @@ -506,26 +634,6 @@ def _expected_users_have_perm( user_org2=user_org2, ) - @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, - # but since there's no org restrictions enabled, anyone has these permissions. - assert self.user_both_orgs.has_perm(perm, object_tag) - assert self.user_org2.has_perm(perm, object_tag) - assert self.learner.has_perm(perm, object_tag) - # Taxonomy @ddt.data( From d7dcca63f33b59b42687151857876bda1a3bc323 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 10:34:42 -0300 Subject: [PATCH 13/25] style: fix pep8 --- openedx/features/content_tagging/tests/test_rules.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 232f5de12c53..615868bc4846 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -51,7 +51,6 @@ def setUp(self): ) 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", @@ -106,7 +105,6 @@ def setUp(self): add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) - self.tax_all_course1 = ChangeObjectTagPermissionItem( taxonomy=self.taxonomy_all_orgs, object_id=str(self.course1), @@ -168,8 +166,6 @@ def setUp(self): object_id=str(self.xblock1), ) - - self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( taxonomy=self.taxonomy_disabled, object_id=str(self.course2), @@ -188,7 +184,6 @@ def setUp(self): object_id=str(self.course1), ) - def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True ): @@ -585,7 +580,6 @@ def test_change_object_tag_invalid_key(self, perm, tag_attr): with self.assertRaises(InvalidKeyError): assert self.staff.has_perm(perm, perm_item) - @ddt.data( "all_orgs_course_tag", "all_orgs_block_tag", From 43f50a789f0ad882c6203e0b83e73b2b6a44cc70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 10:58:57 -0300 Subject: [PATCH 14/25] style: cleaning code --- openedx/features/content_tagging/tests/test_rules.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 615868bc4846..7fe924b86c8d 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -6,7 +6,6 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx_tagging.core.tagging.models import ( - ObjectTag, Tag, UserSystemDefinedTaxonomy, ) @@ -122,14 +121,6 @@ def setUp(self): object_id=str(self.xblock2), ) - # self.both_orgs_course_tag_perm = ChangeObjectTagPermissionItem( - # taxonomy=self.taxonomy_both_orgs, - # object_id=str(self.course2), - # ) - # self.both_orgs_block_tag_perm = ChangeObjectTagPermissionItem( - # taxonomy=self.taxonomy_both_orgs, - # object_id=str(self.xblock1), - # ) self.tax_both_course1 = ChangeObjectTagPermissionItem( taxonomy=self.taxonomy_both_orgs, object_id=str(self.course1), From f045f7637a7c94cea5f8e5aa9fe0b11b6bc932f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 12:50:00 -0300 Subject: [PATCH 15/25] style: revert minor style changes --- .../content_tagging/rest_api/v1/tests/test_views.py | 4 ++-- openedx/features/content_tagging/tests/test_api.py | 7 ------- openedx/features/content_tagging/tests/test_rules.py | 1 - 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index b3ba305011f0..0dddff177777 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -455,7 +455,7 @@ def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status): @ddt.unpack def test_update_taxonomy_system_defined(self, update_value, expected_status): """ - Test that we can"t update system_defined field + Test that we can't update system_defined field """ url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) @@ -551,7 +551,7 @@ def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status): @ddt.unpack def test_patch_taxonomy_system_defined(self, update_value, expected_status): """ - Test that we can"t patch system_defined field + Test that we can't patch system_defined field """ url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) diff --git a/openedx/features/content_tagging/tests/test_api.py b/openedx/features/content_tagging/tests/test_api.py index a18c95d37711..263ae761ef57 100644 --- a/openedx/features/content_tagging/tests/test_api.py +++ b/openedx/features/content_tagging/tests/test_api.py @@ -69,7 +69,6 @@ def setUp(self): tags=[self.tag_all_orgs.id], object_id=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), )[0] - self.all_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.id], @@ -77,13 +76,11 @@ def setUp(self): "block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde" ), )[0] - self.both_orgs_course_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.id], object_id=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), )[0] - self.both_orgs_block_tag = api.tag_content_object( taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.id], @@ -91,7 +88,6 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde" ), )[0] - self.one_org_block_tag = api.tag_content_object( taxonomy=self.taxonomy_one_org, tags=[self.tag_one_org.id], @@ -99,7 +95,6 @@ def setUp(self): "block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde" ), )[0] - self.disabled_course_tag = api.tag_content_object( taxonomy=self.taxonomy_disabled, tags=[self.tag_disabled.id], @@ -112,13 +107,11 @@ def setUp(self): tag=self.tag_all_orgs, object_id="course-v1_OpenedX_DemoX_Demo_Course", ) - self.one_org_invalid_org_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_one_org, tag=self.tag_one_org, object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", ) - self.no_orgs_invalid_tag = ObjectTag.objects.create( taxonomy=self.taxonomy_no_orgs, tag=self.tag_no_orgs, diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 7fe924b86c8d..38dfaca38dfa 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -32,7 +32,6 @@ class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): def setUp(self): super().setUp() - self.superuser = User.objects.create( username="superuser", email="superuser@example.com", From 33dafac1a8bb520054743d353e83775bde8f562b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 19:10:09 -0300 Subject: [PATCH 16/25] docs: add better description on permission override --- openedx/features/content_tagging/rules.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index 562f77253dcc..bad38019ce51 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -99,6 +99,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) rules.set_perm("oel_tagging.delete_object_tag", oel_tagging.can_change_object_tag) rules.set_perm("oel_tagging.view_object_tag", rules.always_allow) -# Users can tag objects using tags from any taxonomy that they have permission to view +# This perms are used in the tagging rest api from openedx_tagging that is exposed in the CMS. They are overridden here +# to include Organization and objects permissions. rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_change_object_tag_taxonomy) rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From d0216ee513398899c24c17e26f68d5be76bdb6ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 19:10:51 -0300 Subject: [PATCH 17/25] docs: fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Chris Chávez --- .../features/content_tagging/rest_api/v1/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 0dddff177777..4ee17c6ce67a 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -626,7 +626,7 @@ def test_delete_taxonomy(self, user_attr, taxonomy_attr, expected_status): response = self.client.delete(url) assert response.status_code == expected_status - # If we were able to delete the taxonomy, check that it"s really gone + # If we were able to delete the taxonomy, check that it's really gone if status.is_success(expected_status): response = self.client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND From 7eb4139dfdabd01a7fdd6ef4523f51356dabc18f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 19:57:13 -0300 Subject: [PATCH 18/25] refactor: simply test --- .../content_tagging/tests/test_rules.py | 56 ++++++++----------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 38dfaca38dfa..6b0aa7a224a9 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -174,6 +174,18 @@ def setUp(self): object_id=str(self.course1), ) + self.all_org_perms = ( + self.tax_all_course1, + self.tax_all_course2, + self.tax_all_xblock1, + self.tax_all_xblock2, + self.tax_both_course1, + self.tax_both_course2, + self.tax_both_xblock1, + self.tax_both_xblock2, + ) + + def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True ): @@ -497,44 +509,22 @@ def test_object_tag_no_orgs(self, perm, tag_attr): assert not self.learner.has_perm(perm, object_tag) @ddt.data( - ("oel_tagging.add_object_tag", "tax_all_course1"), - ("oel_tagging.add_object_tag", "tax_all_course2"), - ("oel_tagging.add_object_tag", "tax_all_xblock1"), - ("oel_tagging.add_object_tag", "tax_all_xblock2"), - ("oel_tagging.add_object_tag", "tax_both_course1"), - ("oel_tagging.add_object_tag", "tax_both_course2"), - ("oel_tagging.add_object_tag", "tax_both_xblock1"), - ("oel_tagging.add_object_tag", "tax_both_xblock2"), - ("oel_tagging.change_object_tag", "tax_all_course1"), - ("oel_tagging.change_object_tag", "tax_all_course2"), - ("oel_tagging.change_object_tag", "tax_all_xblock1"), - ("oel_tagging.change_object_tag", "tax_all_xblock2"), - ("oel_tagging.change_object_tag", "tax_both_course1"), - ("oel_tagging.change_object_tag", "tax_both_course2"), - ("oel_tagging.change_object_tag", "tax_both_xblock1"), - ("oel_tagging.change_object_tag", "tax_both_xblock2"), - ("oel_tagging.delete_object_tag", "tax_all_course1"), - ("oel_tagging.delete_object_tag", "tax_all_course2"), - ("oel_tagging.delete_object_tag", "tax_all_xblock1"), - ("oel_tagging.delete_object_tag", "tax_all_xblock2"), - ("oel_tagging.delete_object_tag", "tax_both_course1"), - ("oel_tagging.delete_object_tag", "tax_both_course2"), - ("oel_tagging.delete_object_tag", "tax_both_xblock1"), - ("oel_tagging.delete_object_tag", "tax_both_xblock2"), + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + "oel_tagging.delete_object_tag", ) - @ddt.unpack - def test_change_object_tag_all_orgs(self, perm, tag_attr): + def test_change_object_tag_all_orgs(self, perm): """ Taxonomy administrators can create/edit an ObjectTag using taxonomies in their org, but only on objects they have write access to. """ - perm_item = getattr(self, tag_attr) - assert self.superuser.has_perm(perm, perm_item) - assert self.staff.has_perm(perm, perm_item) - assert self.user_all_orgs.has_perm(perm, perm_item) - assert self.user_both_orgs.has_perm(perm, perm_item) - assert self.user_org2.has_perm(perm, perm_item) == (tag_attr.endswith("2")) - assert not self.learner.has_perm(perm, perm_item) + for perm_item in self.all_org_perms: + assert self.superuser.has_perm(perm, perm_item) + assert self.staff.has_perm(perm, perm_item) + assert self.user_all_orgs.has_perm(perm, perm_item) + assert self.user_both_orgs.has_perm(perm, perm_item) + assert self.user_org2.has_perm(perm, perm_item) == (self.org2.short_name in perm_item.object_id) + assert not self.learner.has_perm(perm, perm_item) @ddt.data( ("oel_tagging.add_object_tag", "tax1_course1"), From fe16c835979fdbb42517c3fe7aad38a3f1f8710b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 13 Sep 2023 20:08:43 -0300 Subject: [PATCH 19/25] style: fix pep8 --- openedx/features/content_tagging/tests/test_rules.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 6b0aa7a224a9..442fda5a71bd 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -185,7 +185,6 @@ def setUp(self): self.tax_both_xblock2, ) - def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True ): From 52c529babe60370470aa57dff8704697d6e22b92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 08:47:03 -0300 Subject: [PATCH 20/25] chore: update openedx-learning version --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/kernel.in | 3 +-- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 6 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 3ca41113b9ad..8de6d0b48c8c 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -778,7 +778,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main +openedx-learning==0.1.6 # via -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 5f87480e6711..b6cf37d111a5 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1314,7 +1314,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main +openedx-learning==0.1.6 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 56dec3cdd3c5..d37ad3cf79b9 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -919,7 +919,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main +openedx-learning==0.1.6 # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 12a205994bda..fbebdc26c71e 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -116,8 +116,7 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require openedx-events>=8.3.0 # Open edX Events from Hooks Extension Framework (OEP-50) openedx-filters # Open edX Filters from Hooks Extension Framework (OEP-50) -# openedx-learning # Open edX Learning core (experimental) -openedx-learning @ git+https://github.com/openedx/openedx-learning@main # Remove this once openedx-learning is tagged +openedx-learning # Open edX Learning core (experimental) openedx-mongodbproxy openedx-django-wiki openedx-blockstore diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 9e97b5a2bb5f..748e6e1521b6 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -989,7 +989,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/openedx/openedx-learning@main +openedx-learning==0.1.6 # via -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt From 64107d1bf93f727b8b26fa68cf2923b715094d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 09:00:13 -0300 Subject: [PATCH 21/25] test: test tagging without object permissions --- .../rest_api/v1/tests/test_views.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 4ee17c6ce67a..7d5a37c27a0c 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -660,6 +660,12 @@ def setUp(self): block_type='problem', block_id='block_id' ) + self.courseB = CourseLocator("orgB", "101", "test") + self.xblockB = BlockUsageLocator( + course_key=self.courseB, + block_type='problem', + block_id='block_id' + ) self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) self.required_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", required=True) @@ -826,3 +832,21 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected response = self.client.put(url, {"tags": tag_values}, format="json") assert response.status_code == expected_status assert not status.is_success(expected_status) # No success cases here + + + @ddt.data( + "courseB", + "xblockB", + ) + def test_tag_unauthorized(self, objectid_attr): + """ + Test that a user without access to courseB can't apply tags to it + """ + self.client.force_authenticate(user=self.userA) + object_id = getattr(self, objectid_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.tA1.pk) + + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN From bb1809500e34338a5e88f46cdce5225e6695b49c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 09:06:02 -0300 Subject: [PATCH 22/25] chore: pin openedx-learning version --- requirements/constraints.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index b3a09eb01d74..56b2e3d20083 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -123,3 +123,6 @@ libsass==0.10.0 # greater version breaking upgrade builds click==8.1.6 + +# pinning this version to avoid updates while the library is being developed +openedx-learning==0.1.6 From 84e1af015e9dc8d66fb8ebb8e04fbd76f502e45b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 09:21:29 -0300 Subject: [PATCH 23/25] chore: fix requirements --- requirements/edx/base.txt | 4 +++- requirements/edx/development.txt | 1 + requirements/edx/doc.txt | 4 +++- requirements/edx/testing.txt | 4 +++- 4 files changed, 10 insertions(+), 3 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 67de3c15796e..c906a7c1d1f6 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -777,7 +777,9 @@ openedx-filters==1.6.0 # -r requirements/edx/kernel.in # lti-consumer-xblock openedx-learning==0.1.6 - # via -r requirements/edx/kernel.in + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in optimizely-sdk==4.1.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3a066aaba5c7..65255f51de8c 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1311,6 +1311,7 @@ openedx-filters==1.6.0 # lti-consumer-xblock openedx-learning==0.1.6 # via + # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt openedx-mongodbproxy==0.2.0 diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index b2b568af1e21..8dfe3ff76003 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -918,7 +918,9 @@ openedx-filters==1.6.0 # -r requirements/edx/base.txt # lti-consumer-xblock openedx-learning==0.1.6 - # via -r requirements/edx/base.txt + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 4e470f79014c..b249ab560753 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -988,7 +988,9 @@ openedx-filters==1.6.0 # -r requirements/edx/base.txt # lti-consumer-xblock openedx-learning==0.1.6 - # via -r requirements/edx/base.txt + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 From 8eea75de38412fa4c58e334835a169c75a651175 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 09:22:23 -0300 Subject: [PATCH 24/25] style: fix pep8 --- openedx/features/content_tagging/rest_api/v1/tests/test_views.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index 7d5a37c27a0c..f2ce3c406469 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -833,7 +833,6 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected assert response.status_code == expected_status assert not status.is_success(expected_status) # No success cases here - @ddt.data( "courseB", "xblockB", From eab33aadfcc855e662438679f0ee70984c426bcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 15 Sep 2023 09:41:35 -0300 Subject: [PATCH 25/25] chore: trigger CD/CI