From 959472835b598c0482cd28492e8498f514b8126d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 12 Oct 2023 18:15:15 -0300 Subject: [PATCH 01/30] fix: update taxonomies permission rules --- .../core/djangoapps/content_tagging/api.py | 7 +- .../content_tagging/rest_api/v1/filters.py | 76 +- .../rest_api/v1/tests/test_views.py | 1165 +++++++++++------ .../content_tagging/rest_api/v1/views.py | 18 +- .../core/djangoapps/content_tagging/rules.py | 241 +++- .../content_tagging/tests/test_api.py | 6 +- .../content_tagging/tests/test_rules.py | 194 +-- requirements/constraints.txt | 2 +- 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 +- 13 files changed, 1227 insertions(+), 493 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 4867160c1b83..9fddc109d7ce 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -20,11 +20,12 @@ def create_taxonomy( enabled=True, allow_multiple=False, allow_free_text=False, + orgs: list[Organization] = [], ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. """ - return oel_tagging.create_taxonomy( + taxonomy = oel_tagging.create_taxonomy( name=name, description=description, enabled=enabled, @@ -32,6 +33,10 @@ def create_taxonomy( allow_free_text=allow_free_text, ) + set_taxonomy_orgs(taxonomy=taxonomy, all_orgs=False, orgs=orgs) + + return taxonomy + def set_taxonomy_orgs( taxonomy: Taxonomy, diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py index 9ad192f545de..723a90d8774a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py @@ -2,20 +2,92 @@ API Filters for content tagging org """ +from django.db.models import Exists, OuterRef, Q from rest_framework.filters import BaseFilterBackend import openedx_tagging.core.tagging.rules as oel_tagging +from organizations.models import Organization + +from ...rules import get_admin_orgs, get_user_orgs +from ...models import TaxonomyOrg class UserOrgFilterBackend(BaseFilterBackend): """ + Filter taxonomies based on user's orgs roles + Taxonomy admin can see all taxonomies - Everyone else can see only enabled taxonomies + Org staff can see all taxonomies from their orgs + Content creators and instructors can see enabled taxonomies avaliable to their orgs + """ + + def filter_queryset(self, request, queryset, _): + if oel_tagging.is_taxonomy_admin(request.user): + return queryset + orgs = list(Organization.objects.all()) + user_admin_orgs = get_admin_orgs(request.user, orgs) + user_orgs = get_user_orgs(request.user, orgs) # Orgs that the user is a content creator or instructor + + if len(user_orgs) == 0 and len(user_admin_orgs) == 0: + return queryset.none() + + return queryset.filter( + # Get enabled taxonomies available to all orgs, or from orgs that the user is + # a content creator or instructor + Q( + Exists( + TaxonomyOrg.objects + .filter( + taxonomy=OuterRef("pk"), + rel_type=TaxonomyOrg.RelType.OWNER, + ) + .filter( + Q(org=None) | + Q(org__in=user_orgs) + ) + ), + enabled=True, + ) | + # Get all taxonomies from orgs that the user is OrgStaff + Q( + Exists( + TaxonomyOrg.objects + .filter(taxonomy=OuterRef("pk"), rel_type=TaxonomyOrg.RelType.OWNER) + .filter(org__in=user_admin_orgs) + ) + ) + ) + + +class ObjectTagTaxonomyOrgFilterBackend(BaseFilterBackend): + """ + Filter for ObjectTagViewSet to only show taxonomies that the user can view. """ def filter_queryset(self, request, queryset, _): if oel_tagging.is_taxonomy_admin(request.user): return queryset - return queryset.filter(enabled=True) + orgs = list(Organization.objects.all()) + user_admin_orgs = get_admin_orgs(request.user, orgs) + user_orgs = get_user_orgs(request.user, orgs) + user_or_admin_orgs = list(set(user_orgs) | set(user_admin_orgs)) + + return queryset.filter(taxonomy__enabled=True).filter( + # Get ObjectTags from taxonomies available to all orgs, or from orgs that the user is + # a OrgStaff, content creator or instructor + Q( + Exists( + TaxonomyOrg.objects + .filter( + taxonomy=OuterRef("taxonomy_id"), + rel_type=TaxonomyOrg.RelType.OWNER, + ) + .filter( + Q(org=None) | + Q(org__in=user_or_admin_orgs) + ) + ) + ) + ) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 8cb4b94fd227..2b4ff2a6b331 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -2,9 +2,12 @@ Tests tagging rest api views """ +from __future__ import annotations + from urllib.parse import parse_qs, urlparse import ddt +import uuid from django.contrib.auth import get_user_model from django.test.testcases import override_settings from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator @@ -16,7 +19,16 @@ from rest_framework.test import APITestCase from common.djangoapps.student.auth import add_users, update_org_role -from common.djangoapps.student.roles import CourseStaffRole, OrgContentCreatorRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseStaffRole, + OrgContentCreatorRole, + OrgInstructorRole, + OrgLibraryUserRole, + OrgStaffRole, +) +from openedx.core.djangoapps.content_libraries.api import set_library_user_permissions, AccessLevel +from openedx.core.djangoapps.content_libraries.models import ContentLibrary from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -64,7 +76,7 @@ def setUp(self): username="user", email="user@example.com", ) - self.userS = User.objects.create( + self.staff = User.objects.create( username="staff", email="staff@example.com", is_staff=True, @@ -74,11 +86,61 @@ def setUp(self): self.orgB = Organization.objects.create(name="Organization B", short_name="orgB") self.orgX = Organization.objects.create(name="Organization X", short_name="orgX") - self.userA = User.objects.create( - username="userA", + self.courseA = CourseLocator("orgA", "101", "test") + self.courseB = CourseLocator("orgB", "101", "test") + + self.staffA = User.objects.create( + username="staffA", email="userA@example.com", ) - update_org_role(self.userS, OrgContentCreatorRole, self.userA, [self.orgA.short_name]) + update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + + self.content_creatorA = User.objects.create( + username="content_creatorA", + email="content_creatorA@example.com", + ) + update_org_role(self.staff, OrgContentCreatorRole, self.content_creatorA, [self.orgA.short_name]) + + self.instructorA = User.objects.create( + username="instructorA", + email="instructorA@example.com", + ) + update_org_role(self.staff, OrgInstructorRole, self.instructorA, [self.orgA.short_name]) + + self.library_staffA = User.objects.create( + username="library_staffA", + email="library_staffA@example.com", + ) + update_org_role(self.staff, OrgLibraryUserRole, self.library_staffA, [self.orgA.short_name]) + + self.course_instructorA = User.objects.create( + username="course_instructorA", + email="course_instructorA@example.com", + ) + add_users(self.staff, CourseInstructorRole(self.courseA), self.course_instructorA) + + self.course_staffA = User.objects.create( + username="course_staffA", + email="course_staffA@example.com", + ) + add_users(self.staff, CourseStaffRole(self.courseA), self.course_staffA) + + self.library_userA = User.objects.create( + username="library_userA", + email="library_userA@example.com", + ) + self.content_libraryA = ContentLibrary.objects.create( + org=self.orgA, + slug='foobar', + bundle_uuid=uuid.uuid4(), + allow_public_learning=False, + allow_public_read=False, + ) + set_library_user_permissions( + self.content_libraryA.library_key, + self.library_userA, + AccessLevel.READ_LEVEL + ) # Orphaned taxonomy self.ot1 = Taxonomy.objects.create(name="ot1", enabled=True) @@ -142,70 +204,64 @@ def setUp(self): ) # OrgA and OrgB taxonomy - self.tC1 = Taxonomy.objects.create(name="tC1", enabled=True) + self.tBA1 = Taxonomy.objects.create(name="tBA1", enabled=True) TaxonomyOrg.objects.create( - taxonomy=self.tC1, + taxonomy=self.tBA1, org=self.orgA, rel_type=TaxonomyOrg.RelType.OWNER, ) TaxonomyOrg.objects.create( - taxonomy=self.tC1, + taxonomy=self.tBA1, org=self.orgB, rel_type=TaxonomyOrg.RelType.OWNER, ) - self.tC2 = Taxonomy.objects.create(name="tC2", enabled=False) + self.tBA2 = Taxonomy.objects.create(name="tBA2", enabled=False) TaxonomyOrg.objects.create( - taxonomy=self.tC2, + taxonomy=self.tBA2, org=self.orgA, rel_type=TaxonomyOrg.RelType.OWNER, ) TaxonomyOrg.objects.create( - taxonomy=self.tC2, + taxonomy=self.tBA2, org=self.orgB, rel_type=TaxonomyOrg.RelType.OWNER, ) + # # ToDo: OrgX content library test + # # This will show tX1 to every studio user + # self.content_libraryX = ContentLibrary.objects.create( + # org=self.orgX, + # slug='foobar', + # bundle_uuid=uuid.uuid4(), + # allow_public_learning=True, + # allow_public_read=True, + # ) + # self.tX1 = Taxonomy.objects.create(name="tX1", enabled=True) + # TaxonomyOrg.objects.create( + # taxonomy=self.tX1, + # org=self.orgX, + # rel_type=TaxonomyOrg.RelType.OWNER, + # ) + @skip_unless_cms @ddt.ddt @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) -class TestTaxonomyViewSet(TestTaxonomyObjectsMixin, APITestCase): +class TestTaxonomyReadCreateViewSet(TestTaxonomyObjectsMixin, APITestCase): """ - Test cases for TaxonomyViewSet when ENABLE_CREATOR_GROUP is True + Test cases for TestTaxonomyReadViewSet when ENABLE_CREATOR_GROUP is True """ - @ddt.data( - ("user", None, None, ("ot1", "st1", "t1", "tA1", "tB1", "tC1")), - ("userA", None, None, ("ot1", "st1", "t1", "tA1", "tB1", "tC1")), - ("userS", None, None, ("ot1", "ot2", "st1", "st2", "t1", "t2", "tA1", "tA2", "tB1", "tB2")), - # Default page_size=10, and so "tC1" and "tC2" appear on the second page - ("user", True, None, ("ot1", "st1", "t1", "tA1", "tB1", "tC1")), - ("userA", True, None, ("ot1", "st1", "t1", "tA1", "tB1", "tC1")), - ("userS", True, None, ("ot1", "st1", "t1", "tA1", "tB1", "tC1")), - ("user", False, None, ()), - ("userA", False, None, ()), - ("userS", False, None, ("ot2", "st2", "t2", "tA2", "tB2", "tC2")), - ("user", None, "orgA", ("st1", "t1", "tA1", "tC1")), - ("userA", None, "orgA", ("st1", "t1", "tA1", "tC1")), - ("userS", None, "orgA", ("st1", "st2", "t1", "t2", "tA1", "tA2", "tC1", "tC2")), - ("user", True, "orgA", ("st1", "t1", "tA1", "tC1")), - ("userA", True, "orgA", ("st1", "t1", "tA1", "tC1")), - ("userS", True, "orgA", ("st1", "t1", "tA1", "tC1")), - ("user", False, "orgA", ()), - ("userA", False, "orgA", ()), - ("userS", False, "orgA", ("st2", "t2", "tA2", "tC2")), - ("user", None, "orgX", ("st1", "t1")), - ("userA", None, "orgX", ("st1", "t1")), - ("userS", None, "orgX", ("st1", "st2", "t1", "t2")), - ("user", True, "orgX", ("st1", "t1")), - ("userA", True, "orgX", ("st1", "t1")), - ("userS", True, "orgX", ("st1", "t1")), - ("user", False, "orgX", ()), - ("userA", False, "orgX", ()), - ("userS", False, "orgX", ("st2", "t2")), - ) - @ddt.unpack - def test_list_taxonomy(self, user_attr, enabled_parameter, org_name, expected_taxonomies): + def _test_list_taxonomy( + self, + user_attr: str, + expected_taxonomies: list[str], + enabled_parameter: bool | None = None, + org_parameter: str | None = None + ) -> None: + """ + Helper function to call the list endpoint and check the response + """ url = TAXONOMY_ORG_LIST_URL if user_attr: @@ -213,21 +269,82 @@ def test_list_taxonomy(self, user_attr, enabled_parameter, org_name, expected_ta self.client.force_authenticate(user=user) # Set parameters cleaning empty values - query_params = {k: v for k, v in {"enabled": enabled_parameter, "org": org_name}.items() if v is not None} + query_params = {k: v for k, v in {"enabled": enabled_parameter, "org": org_parameter}.items() if v is not None} response = self.client.get(url, query_params, format="json") assert response.status_code == status.HTTP_200_OK self.assertEqual(set(t["name"] for t in response.data["results"]), set(expected_taxonomies)) - def test_list_taxonomy_invalid_org( - self, - ): + def test_list_taxonomy_staff(self) -> None: + """ + Tests that staff users see all taxonomies + """ + # Default page_size=10, and so "tBA1" and "tBA2" appear on the second page + expected_taxonomies = ["ot1", "ot2", "st1", "st2", "t1", "t2", "tA1", "tA2", "tB1", "tB2"] + self._test_list_taxonomy( + user_attr="staff", + expected_taxonomies=expected_taxonomies, + ) + + @ddt.data( + "content_creatorA", + "instructorA", + "library_staffA", + "course_instructorA", + "course_staffA", + "library_userA", + ) + def test_list_taxonomy_orgA(self, user_attr: str) -> None: + """ + Tests that non staff users from orgA can see only enabled taxonomies from orgA and global taxonomies + """ + expected_taxonomies = ["st1", "t1", "tA1", "tBA1"] + self._test_list_taxonomy( + user_attr=user_attr, + enabled_parameter=True, + expected_taxonomies=expected_taxonomies, + ) + + @ddt.data( + (True, ["ot1", "st1", "t1", "tA1", "tB1", "tBA1"]), + (False, ["ot2", "st2", "t2", "tA2", "tB2", "tBA2"]), + ) + @ddt.unpack + def test_list_taxonomy_enabled_filter(self, enabled_parameter: bool, expected_taxonomies: list[str]) -> None: + """ + Tests that the enabled filter works as expected + """ + self._test_list_taxonomy( + user_attr="staff", + enabled_parameter=enabled_parameter, + expected_taxonomies=expected_taxonomies + ) + + @ddt.data( + ("orgA", ["st1", "st2", "t1", "t2", "tA1", "tA2", "tBA1", "tBA2"]), + ("orgB", ["st1", "st2", "t1", "t2", "tB1", "tB2", "tBA1", "tBA2"]), + ("orgX", ["st1", "st2", "t1", "t2"]), + ) + @ddt.unpack + def test_list_taxonomy_org_filter(self, org_parameter: str, expected_taxonomies: list[str]) -> None: + """ + Tests that the org filter works as expected + """ + self._test_list_taxonomy( + user_attr="staff", + org_parameter=org_parameter, + expected_taxonomies=expected_taxonomies, + ) + + def test_list_taxonomy_invalid_org(self) -> None: + """ + Tests that using an invalid org in the filter will raise BAD_REQUEST + """ url = TAXONOMY_ORG_LIST_URL - self.client.force_authenticate(user=self.userS) + self.client.force_authenticate(user=self.staff) - # Set parameters cleaning empty values query_params = {"org": "invalidOrg"} response = self.client.get(url, query_params, format="json") @@ -235,12 +352,17 @@ def test_list_taxonomy_invalid_org( assert response.status_code == status.HTTP_400_BAD_REQUEST @ddt.data( - ("user", ("tA1", "tB1", "tC1"), None), - ("userA", ("tA1", "tB1", "tC1"), None), - ("userS", ("st2", "t1", "t2"), "3"), + ("user", (), None), + ("staffA", ["tA2", "tBA1", "tBA2"], None), + ("staff", ["st2", "t1", "t2"], "3"), ) @ddt.unpack - def test_list_taxonomy_pagination(self, user_attr, expected_taxonomies, expected_next_page): + def test_list_taxonomy_pagination( + self, user_attr: str, expected_taxonomies: list[str], expected_next_page: str | None + ) -> None: + """ + Tests that the pagination works as expected + """ url = TAXONOMY_ORG_LIST_URL if user_attr: @@ -251,14 +373,18 @@ def test_list_taxonomy_pagination(self, user_attr, expected_taxonomies, expected response = self.client.get(url, query_params, format="json") - assert response.status_code == status.HTTP_200_OK - self.assertEqual(set(t["name"] for t in response.data["results"]), set(expected_taxonomies)) - parsed_url = urlparse(response.data["next"]) + assert response.status_code == status.HTTP_200_OK if len(expected_taxonomies) > 0 else status.HTTP_404_NOT_FOUND + if status.is_success(response.status_code): + self.assertEqual(set(t["name"] for t in response.data["results"]), set(expected_taxonomies)) + parsed_url = urlparse(response.data["next"]) - next_page = parse_qs(parsed_url.query).get("page", [None])[0] - assert next_page == expected_next_page + next_page = parse_qs(parsed_url.query).get("page", [None])[0] + assert next_page == expected_next_page - def test_list_invalid_page(self): + def test_list_invalid_page(self) -> None: + """ + Tests that using an invalid page will raise NOT_FOUND + """ url = TAXONOMY_ORG_LIST_URL self.client.force_authenticate(user=self.user) @@ -269,58 +395,12 @@ def test_list_invalid_page(self): assert response.status_code == status.HTTP_404_NOT_FOUND - @ddt.data( - (None, "ot1", status.HTTP_403_FORBIDDEN), - (None, "ot2", status.HTTP_403_FORBIDDEN), - (None, "st1", status.HTTP_403_FORBIDDEN), - (None, "st2", status.HTTP_403_FORBIDDEN), - (None, "t1", status.HTTP_403_FORBIDDEN), - (None, "t2", status.HTTP_403_FORBIDDEN), - (None, "tA1", status.HTTP_403_FORBIDDEN), - (None, "tA2", status.HTTP_403_FORBIDDEN), - (None, "tB1", status.HTTP_403_FORBIDDEN), - (None, "tB2", status.HTTP_403_FORBIDDEN), - (None, "tC1", status.HTTP_403_FORBIDDEN), - (None, "tC2", status.HTTP_403_FORBIDDEN), - ("user", "ot1", status.HTTP_200_OK), - ("user", "ot2", status.HTTP_404_NOT_FOUND), - ("user", "st1", status.HTTP_200_OK), - ("user", "st2", status.HTTP_404_NOT_FOUND), - ("user", "t1", status.HTTP_200_OK), - ("user", "t2", status.HTTP_404_NOT_FOUND), - ("user", "tA1", status.HTTP_200_OK), - ("user", "tA2", status.HTTP_404_NOT_FOUND), - ("user", "tB1", status.HTTP_200_OK), - ("user", "tB2", status.HTTP_404_NOT_FOUND), - ("user", "tC1", status.HTTP_200_OK), - ("user", "tC2", status.HTTP_404_NOT_FOUND), - ("userA", "ot1", status.HTTP_200_OK), - ("userA", "ot2", status.HTTP_404_NOT_FOUND), - ("userA", "st1", status.HTTP_200_OK), - ("userA", "st2", status.HTTP_404_NOT_FOUND), - ("userA", "t1", status.HTTP_200_OK), - ("userA", "t2", status.HTTP_404_NOT_FOUND), - ("userA", "tA1", status.HTTP_200_OK), - ("userA", "tA2", status.HTTP_404_NOT_FOUND), - ("userA", "tB1", status.HTTP_200_OK), - ("userA", "tB2", status.HTTP_404_NOT_FOUND), - ("userA", "tC1", status.HTTP_200_OK), - ("userA", "tC2", status.HTTP_404_NOT_FOUND), - ("userS", "ot1", status.HTTP_200_OK), - ("userS", "ot2", status.HTTP_200_OK), - ("userS", "st1", status.HTTP_200_OK), - ("userS", "st2", status.HTTP_200_OK), - ("userS", "t1", status.HTTP_200_OK), - ("userS", "t2", status.HTTP_200_OK), - ("userS", "tA1", status.HTTP_200_OK), - ("userS", "tA2", status.HTTP_200_OK), - ("userS", "tB1", status.HTTP_200_OK), - ("userS", "tB2", status.HTTP_200_OK), - ("userS", "tC1", status.HTTP_200_OK), - ("userS", "tC2", status.HTTP_200_OK), - ) - @ddt.unpack - def test_detail_taxonomy(self, user_attr, taxonomy_attr, expected_status): + def _test_detail_taxonomy( + self, user_attr: str, taxonomy_attr: str, expected_status: int, reason: str = "Unexpected response status" + ) -> None: + """ + Helper function to call the retrieve endpoint and check the response + """ taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -330,19 +410,296 @@ def test_detail_taxonomy(self, user_attr, taxonomy_attr, expected_status): self.client.force_authenticate(user=user) response = self.client.get(url) - assert response.status_code == expected_status + assert response.status_code == expected_status, reason if status.is_success(expected_status): check_taxonomy(response.data, taxonomy.pk, **(TaxonomySerializer(taxonomy.cast()).data)) + pass + + @ddt.data( + "user", + "content_creatorA", + "instructorA", + "library_staffA", + "course_instructorA", + "course_staffA", + "library_userA", + ) + def test_detail_taxonomy_all_org_enabled(self, user_attr: str) -> None: + """ + Tests that everyone can see enabled global taxonomies + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr="t1", + expected_status=status.HTTP_200_OK, + reason="Everyone should see enabled global taxonomies", + ) + + @ddt.data( + ("content_creatorA", "tA1", "User with OrgContentCreatorRole(orgA) should see an enabled taxonomy from orgA"), + ("content_creatorA", "tBA1", "User with OrgContentCreatorRole(orgA) should see an enabled taxonomy from orgA"), + ("content_creatorA", "t1", "User with OrgContentCreatorRole(orgA) should see an enabled global taxonomy"), + ("instructorA", "tA1", "User with OrgInstructorRole(orgA) should see an enabled taxonomy from orgA"), + ("instructorA", "tBA1", "User with OrgInstructorRole(orgA) should see an enabled taxonomy from orgA"), + ("instructorA", "t1", "User with OrgInstructorRole(orgA) should see an enabled global taxonomy"), + ("library_staffA", "tA1", "User with OrgLibraryUserRole(orgA) should see an enabled taxonomy from orgA"), + ("library_staffA", "tBA1", "User with OrgLibraryUserRole(orgA) should see an enabled taxonomy from orgA"), + ("library_staffA", "t1", "User with OrgInstructorRole(orgA) should see an enabled global taxonomy"), + ( + "course_instructorA", + "tA1", + "User with CourseInstructorRole in a course from orgA should see an enabled taxonomy from orgA" + ), + ( + "course_instructorA", + "tBA1", + "User with CourseInstructorRole in a course from orgA should see an enabled taxonomy from orgA" + ), + ( + "course_instructorA", + "t1", + "User with CourseInstructorRole in a course from orgA should see an enabled global taxonomy" + ), + ( + "course_staffA", + "tA1", + "User with CourseStaffRole in a course from orgA should see an enabled taxonomy from orgA" + ), + ( + "course_staffA", + "tBA1", + "User with CourseStaffRole in a course from orgA should see an enabled taxonomy from orgA" + ), + ( + "course_staffA", + "t1", + "User with CourseStaffRole in a course from orgA should see an enabled global taxonomy" + ), + ( + "library_userA", + "tA1", + "User with permission on a library from orgA should see an enabled taxonomy from orgA" + ), + ( + "library_userA", + "tBA1", + "User with permission on a library from orgA should see an enabled taxonomy from orgA" + ), + ( + "library_userA", + "t1", + "User with permission on a library from orgA should see an enabled global taxonomy" + ), + ) + @ddt.unpack + def test_detail_taxonomy_org_user_see_enabled(self, user_attr: str, taxonomy_attr: str, reason: str) -> None: + """ + Tests that org users (content creators and instructors) can see enabled global taxonomies and taxonomies + from their orgs + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_200_OK, + reason=reason, + ) + + @ddt.data( + "tA2", + "tBA2", + ) + def test_detail_taxonomy_org_admin_see_disabled(self, taxonomy_attr: str) -> None: + """ + Tests that org admins can see disabled taxonomies from their orgs + """ + self._test_detail_taxonomy( + user_attr="staffA", + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_200_OK, + reason="User with OrgContentCreatorRole(orgA) should see a disabled taxonomy from orgA", + ) + + @ddt.data( + "st2", + "t2", + ) + def test_detail_taxonomy_org_admin_dont_see_disabled_global(self, taxonomy_attr: str) -> None: + """ + Tests that org admins can't see disabled global taxonomies + """ + self._test_detail_taxonomy( + user_attr="staffA", + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_404_NOT_FOUND, + reason="User with OrgContentCreatorRole(orgA) shouldn't see a disabled global taxonomy", + ) + + @ddt.data( + ("content_creatorA", "t2", "User with OrgContentCreatorRole(orgA) shouldn't see a disabled global taxonomy"), + ("instructorA", "tA2", "User with OrgInstructorRole(orgA) shouldn't see a disabled taxonomy from orgA"), + ("instructorA", "tBA2", "User with OrgInstructorRole(orgA) shouldn't see a disabled taxonomy from orgA"), + ("instructorA", "t2", "User with OrgInstructorRole(orgA) shouldn't see a disabled global taxonomy"), + ("library_staffA", "tA2", "User with OrgLibraryUserRole(orgA) shouldn't see a disabled taxonomy from orgA"), + ("library_staffA", "tBA2", "User with OrgLibraryUserRole(orgA) shouldn't see a disabled taxonomy from orgA"), + ("library_staffA", "t2", "User with OrgInstructorRole(orgA) shouldn't see a disabled global taxonomy"), + ( + "course_instructorA", + "tA2", + "User with CourseInstructorRole in a course from orgA shouldn't see a disabled taxonomy from orgA" + ), + ( + "course_instructorA", + "tBA2", + "User with CourseInstructorRole in a course from orgA shouldn't see a disabled taxonomy from orgA" + ), + ( + "course_instructorA", + "t2", + "User with CourseInstructorRole in a course from orgA shouldn't see a disabled global taxonomy" + ), + ( + "course_staffA", + "tA2", + "User with CourseStaffRole in a course from orgA shouldn't see a disabled taxonomy from orgA" + ), + ( + "course_staffA", + "tBA2", + "User with CourseStaffRole in a course from orgA shouldn't see a disabled taxonomy from orgA" + ), + ( + "course_staffA", + "t2", + "User with CourseStaffRole in a course from orgA should't see a disabled global taxonomy" + ), + ( + "library_userA", + "tA2", + "User with permission on a library from orgA shouldn't see an disabled taxonomy from orgA" + ), + ( + "library_userA", + "tBA2", + "User with permission on a library from orgA shouldn't see an disabled taxonomy from orgA" + ), + ( + "library_userA", + "t2", + "User with permission on a library from orgA shouldn't see an disabled global taxonomy" + ), + ) + @ddt.unpack + def test_detail_taxonomy_org_user_dont_see_disabled(self, user_attr: str, taxonomy_attr: str, reason: str) -> None: + """ + Tests that org users (content creators and instructors) can't see disabled global taxonomies and taxonomies + from their orgs + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_404_NOT_FOUND, + reason=reason, + ) + + @ddt.data( + ("staff", "ot1", "Staff should see an enabled no org taxonomy"), + ("staff", "ot2", "Staff should see a disabled no org taxonomy"), + ) + @ddt.unpack + def test_detail_taxonomy_staff_see_no_org(self, user_attr: str, taxonomy_attr: str, reason: str) -> None: + """ + Tests that staff can see taxonomies with no org + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_200_OK, + reason=reason, + ) + + @ddt.data( + "staffA", + "content_creatorA", + "instructorA", + "library_staffA", + "course_instructorA", + "course_staffA", + "library_userA" + ) + def test_detail_taxonomy_other_dont_see_no_org(self, user_attr: str) -> None: + """ + Tests that org users can't see taxonomies with no org + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr="ot1", + expected_status=status.HTTP_404_NOT_FOUND, + reason="Only staff should see taxonomies with no org", + ) + + @ddt.data( + "staffA", + "content_creatorA", + "instructorA", + "library_staffA", + "course_instructorA", + "course_staffA", + "library_userA" + ) + def test_detail_taxonomy_dont_see_other_org(self, user_attr: str) -> None: + """ + Tests that org users can't see taxonomies from other orgs + """ + self._test_detail_taxonomy( + user_attr=user_attr, + taxonomy_attr="tB1", + expected_status=status.HTTP_404_NOT_FOUND, + reason="Users shouldn't see taxonomies from other orgs", + ) + + @ddt.data( + "ot1", + "ot2", + "st1", + "st2", + "t1", + "t2", + "tA1", + "tA2", + "tB1", + "tB2", + "tBA1", + "tBA2", + ) + def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: + """ + Tests that org users can't see taxonomies from other orgs + """ + self._test_detail_taxonomy( + user_attr="staff", + taxonomy_attr=taxonomy_attr, + expected_status=status.HTTP_200_OK, + reason="Staff should see all taxonomies", + ) @ddt.data( (None, status.HTTP_403_FORBIDDEN), ("user", status.HTTP_403_FORBIDDEN), - ("userA", status.HTTP_403_FORBIDDEN), - ("userS", status.HTTP_201_CREATED), + ("content_creatorA", status.HTTP_403_FORBIDDEN), + ("instructorA", status.HTTP_403_FORBIDDEN), + ("library_staffA", status.HTTP_403_FORBIDDEN), + ("course_instructorA", status.HTTP_403_FORBIDDEN), + ("course_staffA", status.HTTP_403_FORBIDDEN), + ("library_userA", status.HTTP_403_FORBIDDEN), + ("staffA", status.HTTP_201_CREATED), + ("staff", status.HTTP_201_CREATED), ) @ddt.unpack - def test_create_taxonomy(self, user_attr, expected_status): + def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: + """ + Tests that only Taxonomy admins and org level admins can create taxonomies + """ url = TAXONOMY_ORG_LIST_URL create_data = { @@ -367,60 +724,159 @@ def test_create_taxonomy(self, user_attr, expected_status): response = self.client.get(url) check_taxonomy(response.data, response.data["id"], **create_data) + # Also checks if the taxonomy was associated with the org + if user_attr == "staffA": + assert TaxonomyOrg.objects.filter(taxonomy=response.data["id"], org=self.orgA).exists() + + +@ddt.ddt +class TestTaxonomyChangeMixin(TestTaxonomyObjectsMixin): + """ + Test cases for TestTaxonomyChangeViewSet when ENABLE_CREATOR_GROUP is True + """ + + def _test_api_call( + self, + **_kwargs, + ) -> None: + """ + Helper function to call the update endpoint and check the response + """ + pass + @ddt.data( - (None, "ot1", status.HTTP_403_FORBIDDEN), - (None, "ot2", status.HTTP_403_FORBIDDEN), - (None, "st1", status.HTTP_403_FORBIDDEN), - (None, "st2", status.HTTP_403_FORBIDDEN), - (None, "t1", status.HTTP_403_FORBIDDEN), - (None, "t2", status.HTTP_403_FORBIDDEN), - (None, "tA1", status.HTTP_403_FORBIDDEN), - (None, "tA2", status.HTTP_403_FORBIDDEN), - (None, "tB1", status.HTTP_403_FORBIDDEN), - (None, "tB2", status.HTTP_403_FORBIDDEN), - (None, "tC1", status.HTTP_403_FORBIDDEN), - (None, "tC2", status.HTTP_403_FORBIDDEN), - ("user", "ot1", status.HTTP_403_FORBIDDEN), - ("user", "ot2", status.HTTP_403_FORBIDDEN), - ("user", "st1", status.HTTP_403_FORBIDDEN), - ("user", "st2", status.HTTP_403_FORBIDDEN), - ("user", "t1", status.HTTP_403_FORBIDDEN), - ("user", "t2", status.HTTP_403_FORBIDDEN), - ("user", "tA1", status.HTTP_403_FORBIDDEN), - ("user", "tA2", status.HTTP_403_FORBIDDEN), - ("user", "tB1", status.HTTP_403_FORBIDDEN), - ("user", "tB2", status.HTTP_403_FORBIDDEN), - ("user", "tC1", status.HTTP_403_FORBIDDEN), - ("user", "tC2", status.HTTP_403_FORBIDDEN), - ("userA", "ot1", status.HTTP_403_FORBIDDEN), - ("userA", "ot2", status.HTTP_403_FORBIDDEN), - ("userA", "st1", status.HTTP_403_FORBIDDEN), - ("userA", "st2", status.HTTP_403_FORBIDDEN), - ("userA", "t1", status.HTTP_403_FORBIDDEN), - ("userA", "t2", status.HTTP_403_FORBIDDEN), - ("userA", "tA1", status.HTTP_403_FORBIDDEN), - ("userA", "tA2", status.HTTP_403_FORBIDDEN), - ("userA", "tB1", status.HTTP_403_FORBIDDEN), - ("userA", "tB2", status.HTTP_403_FORBIDDEN), - ("userA", "tC1", status.HTTP_403_FORBIDDEN), - ("userA", "tC2", status.HTTP_403_FORBIDDEN), - ("userS", "ot1", status.HTTP_200_OK), - ("userS", "ot2", status.HTTP_200_OK), - ("userS", "st1", status.HTTP_403_FORBIDDEN), - ("userS", "st2", status.HTTP_403_FORBIDDEN), - ("userS", "t1", status.HTTP_200_OK), - ("userS", "t2", status.HTTP_200_OK), - ("userS", "t1", status.HTTP_200_OK), - ("userS", "t2", status.HTTP_200_OK), - ("userS", "tA1", status.HTTP_200_OK), - ("userS", "tA2", status.HTTP_200_OK), - ("userS", "tB1", status.HTTP_200_OK), - ("userS", "tB2", status.HTTP_200_OK), - ("userS", "tC1", status.HTTP_200_OK), - ("userS", "tC2", status.HTTP_200_OK), + "ot1", + "ot2", + "st1", + "st2", + "t1", + "t2", + "tA1", + "tA2", + "tB1", + "tB2", + "tBA1", + "tBA2", ) - @ddt.unpack - def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status): + def test_regular_user_cant_edit_taxonomies(self, taxonomy_attr: str) -> None: + """ + Tests that regular users can't edit taxonomies + """ + self._test_api_call( + user_attr="user", + taxonomy_attr=taxonomy_attr, + expected_status=[status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND], + reason="Regular users shouldn't be able to edit taxonomies", + ) + + @ddt.data( + "content_creatorA", + "instructorA", + "library_staffA", + "course_instructorA", + "course_staffA", + "library_userA", + ) + def test_org_user_cant_edit_org_taxonomies(self, user_attr: str) -> None: + """ + Tests that content creators and instructors from orgA can't edit taxonomies from orgA + """ + self._test_api_call( + user_attr=user_attr, + taxonomy_attr="tA1", + expected_status=[status.HTTP_403_FORBIDDEN], + reason="Content creators and instructors shouldn't be able to edit taxonomies", + ) + + @ddt.data( + "tA1", + "tA2", + "tBA1", + "tBA2", + ) + def test_org_staff_can_edit_org_taxonomies(self, taxonomy_attr: str) -> None: + """ + Tests that org staff can edit taxonomies from their orgs + """ + self._test_api_call( + user_attr="staffA", + taxonomy_attr=taxonomy_attr, + # Check both status: 200 for update and 204 for delete + expected_status=[status.HTTP_200_OK, status.HTTP_204_NO_CONTENT], + reason="Org staff should be able to edit taxonomies from their orgs", + ) + + @ddt.data( + "tB1", + "tB2", + ) + def test_org_staff_cant_edit_other_org_taxonomies(self, taxonomy_attr: str) -> None: + """ + Tests that org staff can't edit taxonomies from other orgs + """ + self._test_api_call( + user_attr="staffA", + taxonomy_attr=taxonomy_attr, + expected_status=[status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND], + reason="Org staff shouldn't be able to edit taxonomies from other orgs", + ) + + @ddt.data( + "ot1", + "ot2", + "t1", + "t2", + "tA1", + "tA2", + "tB1", + "tB2", + "tBA1", + "tBA2", + + ) + def test_staff_can_edit_almost_all_taxonomies(self, taxonomy_attr: str) -> None: + """ + Tests that staff can edit all but system defined taxonomies + """ + self._test_api_call( + user_attr="staff", + taxonomy_attr=taxonomy_attr, + # Check both status: 200 for update and 204 for delete + expected_status=[status.HTTP_200_OK, status.HTTP_204_NO_CONTENT], + reason="Staff should be able to edit all but system defined taxonomies", + ) + + @ddt.data( + "st1", + "st2", + ) + def test_staff_cant_edit_system_defined_taxonomies(self, taxonomy_attr: str) -> None: + """ + Tests that staff can't edit system defined taxonomies + """ + self._test_api_call( + user_attr="staff", + taxonomy_attr=taxonomy_attr, + # Check both status: 200 for update and 204 for delete + expected_status=[status.HTTP_403_FORBIDDEN], + reason="Staff shouldn't be able to edit system defined ", + ) + + +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestTaxonomyUpdateViewSet(TestTaxonomyChangeMixin, APITestCase): + """ + Test cases for TaxonomyChangeViewSet with PUT method + """ + + def _test_api_call( + self, + user_attr: str, + taxonomy_attr: str, + expected_status: list[int], + reason: str = "Unexpected response status" + ) -> None: taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -430,10 +886,10 @@ def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status): self.client.force_authenticate(user=user) response = self.client.put(url, {"name": "new name"}, format="json") - assert response.status_code == expected_status + assert response.status_code in expected_status, reason # If we were able to update the taxonomy, check if the name changed - if status.is_success(expected_status): + if status.is_success(response.status_code): response = self.client.get(url) check_taxonomy( response.data, @@ -445,77 +901,21 @@ def test_update_taxonomy(self, user_attr, taxonomy_attr, expected_status): }, ) - @ddt.data( - (False, status.HTTP_403_FORBIDDEN), - (True, status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_update_taxonomy_system_defined(self, update_value, expected_status): - """ - Test that we can't update system_defined field - """ - url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) - - self.client.force_authenticate(user=self.userS) - response = self.client.put(url, {"name": "new name", "system_defined": update_value}, format="json") - assert response.status_code == expected_status - # Verify that system_defined has not changed - response = self.client.get(url) - assert response.data["system_defined"] is True +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestTaxonomyPatchViewSet(TestTaxonomyChangeMixin, APITestCase): + """ + Test cases for TaxonomyChangeViewSet with PATCH method + """ - @ddt.data( - (None, "ot1", status.HTTP_403_FORBIDDEN), - (None, "ot2", status.HTTP_403_FORBIDDEN), - (None, "st1", status.HTTP_403_FORBIDDEN), - (None, "st2", status.HTTP_403_FORBIDDEN), - (None, "t1", status.HTTP_403_FORBIDDEN), - (None, "t2", status.HTTP_403_FORBIDDEN), - (None, "tA1", status.HTTP_403_FORBIDDEN), - (None, "tA2", status.HTTP_403_FORBIDDEN), - (None, "tB1", status.HTTP_403_FORBIDDEN), - (None, "tB2", status.HTTP_403_FORBIDDEN), - (None, "tC1", status.HTTP_403_FORBIDDEN), - (None, "tC2", status.HTTP_403_FORBIDDEN), - ("user", "ot1", status.HTTP_403_FORBIDDEN), - ("user", "ot2", status.HTTP_403_FORBIDDEN), - ("user", "st1", status.HTTP_403_FORBIDDEN), - ("user", "st2", status.HTTP_403_FORBIDDEN), - ("user", "t1", status.HTTP_403_FORBIDDEN), - ("user", "t2", status.HTTP_403_FORBIDDEN), - ("user", "tA1", status.HTTP_403_FORBIDDEN), - ("user", "tA2", status.HTTP_403_FORBIDDEN), - ("user", "tB1", status.HTTP_403_FORBIDDEN), - ("user", "tB2", status.HTTP_403_FORBIDDEN), - ("user", "tC1", status.HTTP_403_FORBIDDEN), - ("user", "tC2", status.HTTP_403_FORBIDDEN), - ("userA", "ot1", status.HTTP_403_FORBIDDEN), - ("userA", "ot2", status.HTTP_403_FORBIDDEN), - ("userA", "st1", status.HTTP_403_FORBIDDEN), - ("userA", "st2", status.HTTP_403_FORBIDDEN), - ("userA", "t1", status.HTTP_403_FORBIDDEN), - ("userA", "t2", status.HTTP_403_FORBIDDEN), - ("userA", "tA1", status.HTTP_403_FORBIDDEN), - ("userA", "tA2", status.HTTP_403_FORBIDDEN), - ("userA", "tB1", status.HTTP_403_FORBIDDEN), - ("userA", "tB2", status.HTTP_403_FORBIDDEN), - ("userA", "tC1", status.HTTP_403_FORBIDDEN), - ("userA", "tC2", status.HTTP_403_FORBIDDEN), - ("userS", "ot1", status.HTTP_200_OK), - ("userS", "ot2", status.HTTP_200_OK), - ("userS", "st1", status.HTTP_403_FORBIDDEN), - ("userS", "st2", status.HTTP_403_FORBIDDEN), - ("userS", "t1", status.HTTP_200_OK), - ("userS", "t2", status.HTTP_200_OK), - ("userS", "tA1", status.HTTP_200_OK), - ("userS", "tA2", status.HTTP_200_OK), - ("userS", "tB1", status.HTTP_200_OK), - ("userS", "tB2", status.HTTP_200_OK), - ("userS", "tC1", status.HTTP_200_OK), - ("userS", "tC2", status.HTTP_200_OK), - ) - @ddt.unpack - def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status): + def _test_api_call( + self, + user_attr: str, + taxonomy_attr: str, + expected_status: list[int], + reason: str = "Unexpected response status" + ) -> None: taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -525,10 +925,10 @@ def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status): self.client.force_authenticate(user=user) response = self.client.patch(url, {"name": "new name"}, format="json") - assert response.status_code == expected_status + assert response.status_code in expected_status, reason # If we were able to patch the taxonomy, check if the name changed - if status.is_success(expected_status): + if status.is_success(response.status_code): response = self.client.get(url) check_taxonomy( response.data, @@ -540,77 +940,21 @@ def test_patch_taxonomy(self, user_attr, taxonomy_attr, expected_status): }, ) - @ddt.data( - (False, status.HTTP_403_FORBIDDEN), - (True, status.HTTP_403_FORBIDDEN), - ) - @ddt.unpack - def test_patch_taxonomy_system_defined(self, update_value, expected_status): - """ - Test that we can't patch system_defined field - """ - url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.st1.pk) - - self.client.force_authenticate(user=self.userS) - response = self.client.patch(url, {"name": "new name", "system_defined": update_value}, format="json") - assert response.status_code == expected_status - # Verify that system_defined has not changed - response = self.client.get(url) - assert response.data["system_defined"] is True +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestTaxonomyDeleteViewSet(TestTaxonomyChangeMixin, APITestCase): + """ + Test cases for TaxonomyChangeViewSet with DELETE method + """ - @ddt.data( - (None, "ot1", status.HTTP_403_FORBIDDEN), - (None, "ot2", status.HTTP_403_FORBIDDEN), - (None, "st1", status.HTTP_403_FORBIDDEN), - (None, "st2", status.HTTP_403_FORBIDDEN), - (None, "t1", status.HTTP_403_FORBIDDEN), - (None, "t2", status.HTTP_403_FORBIDDEN), - (None, "tA1", status.HTTP_403_FORBIDDEN), - (None, "tA2", status.HTTP_403_FORBIDDEN), - (None, "tB1", status.HTTP_403_FORBIDDEN), - (None, "tB2", status.HTTP_403_FORBIDDEN), - (None, "tC1", status.HTTP_403_FORBIDDEN), - (None, "tC2", status.HTTP_403_FORBIDDEN), - ("user", "ot1", status.HTTP_403_FORBIDDEN), - ("user", "ot2", status.HTTP_403_FORBIDDEN), - ("user", "st1", status.HTTP_403_FORBIDDEN), - ("user", "st2", status.HTTP_403_FORBIDDEN), - ("user", "t1", status.HTTP_403_FORBIDDEN), - ("user", "t2", status.HTTP_403_FORBIDDEN), - ("user", "tA1", status.HTTP_403_FORBIDDEN), - ("user", "tA2", status.HTTP_403_FORBIDDEN), - ("user", "tB1", status.HTTP_403_FORBIDDEN), - ("user", "tB2", status.HTTP_403_FORBIDDEN), - ("user", "tC1", status.HTTP_403_FORBIDDEN), - ("user", "tC2", status.HTTP_403_FORBIDDEN), - ("userA", "ot1", status.HTTP_403_FORBIDDEN), - ("userA", "ot2", status.HTTP_403_FORBIDDEN), - ("userA", "st1", status.HTTP_403_FORBIDDEN), - ("userA", "st2", status.HTTP_403_FORBIDDEN), - ("userA", "t1", status.HTTP_403_FORBIDDEN), - ("userA", "t2", status.HTTP_403_FORBIDDEN), - ("userA", "tA1", status.HTTP_403_FORBIDDEN), - ("userA", "tA2", status.HTTP_403_FORBIDDEN), - ("userA", "tB1", status.HTTP_403_FORBIDDEN), - ("userA", "tB2", status.HTTP_403_FORBIDDEN), - ("userA", "tC1", status.HTTP_403_FORBIDDEN), - ("userA", "tC2", status.HTTP_403_FORBIDDEN), - ("userS", "ot1", status.HTTP_204_NO_CONTENT), - ("userS", "ot2", status.HTTP_204_NO_CONTENT), - ("userS", "st1", status.HTTP_403_FORBIDDEN), - ("userS", "st2", status.HTTP_403_FORBIDDEN), - ("userS", "t1", status.HTTP_204_NO_CONTENT), - ("userS", "t2", status.HTTP_204_NO_CONTENT), - ("userS", "tA1", status.HTTP_204_NO_CONTENT), - ("userS", "tA2", status.HTTP_204_NO_CONTENT), - ("userS", "tB1", status.HTTP_204_NO_CONTENT), - ("userS", "tB2", status.HTTP_204_NO_CONTENT), - ("userS", "tC1", status.HTTP_204_NO_CONTENT), - ("userS", "tC2", status.HTTP_204_NO_CONTENT), - ) - @ddt.unpack - def test_delete_taxonomy(self, user_attr, taxonomy_attr, expected_status): + def _test_api_call( + self, + user_attr: str, + taxonomy_attr: str, + expected_status: list[int], + reason: str = "Unexpected response status" + ) -> None: taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -620,43 +964,68 @@ def test_delete_taxonomy(self, user_attr, taxonomy_attr, expected_status): self.client.force_authenticate(user=user) response = self.client.delete(url) - assert response.status_code == expected_status + assert response.status_code in expected_status, reason # If we were able to delete the taxonomy, check that it's really gone - if status.is_success(expected_status): + if status.is_success(response.status_code): response = self.client.get(url) assert response.status_code == status.HTTP_404_NOT_FOUND @skip_unless_cms -@ddt.ddt @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyViewSetNoCreatorGroup(TestTaxonomyViewSet): # pylint: disable=test-inherits-tests +class TestTaxonomyReadViewSetNoCreatorGroup(TestTaxonomyReadCreateViewSet): # pylint: disable=test-inherits-tests """ - Test cases for TaxonomyViewSet when ENABLE_CREATOR_GROUP is False + Test cases for TaxonomyReadViewSet when ENABLE_CREATOR_GROUP is False The permissions are the same for when ENABLED_CREATOR_GRUP is True """ @skip_unless_cms -@ddt.ddt -class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) +class TestTaxonomyUpdateViewSetNoCreatorGroup(TestTaxonomyUpdateViewSet): # pylint: disable=test-inherits-tests """ - Testing various cases for the ObjectTagView. + Test cases for TaxonomyUpdateViewSet when ENABLE_CREATOR_GROUP is False + + The permissions are the same for when ENABLED_CREATOR_GRUP is True + """ + + +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) +class TestTaxonomyPatchViewSetNoCreatorGroup(TestTaxonomyUpdateViewSet): # pylint: disable=test-inherits-tests + """ + Test cases for TaxonomyPatchViewSet when ENABLE_CREATOR_GROUP is False + + The permissions are the same for when ENABLED_CREATOR_GRUP is True + """ + + +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) +class TestTaxonomyDeleteViewSetNoCreatorGroup(TestTaxonomyPatchViewSet): # pylint: disable=test-inherits-tests + """ + Test cases for TaxonomyDeleteViewSet when ENABLE_CREATOR_GROUP is False + + The permissions are the same for when ENABLED_CREATOR_GRUP is True + """ + + +class TestObjectTagMixin(TestTaxonomyObjectsMixin): + """ + Sets up data for testing ObjectTags. """ def setUp(self): """ Setup the test cases """ super().setUp() - self.courseA = CourseLocator("orgA", "101", "test") self.xblockA = BlockUsageLocator( course_key=self.courseA, block_type='problem', block_id='block_id' ) - self.courseB = CourseLocator("orgB", "101", "test") self.xblockB = BlockUsageLocator( course_key=self.courseB, block_type='problem', @@ -691,31 +1060,34 @@ def setUp(self): rel_type=TaxonomyOrg.RelType.OWNER, ) - add_users(self.userS, CourseStaffRole(self.courseA), self.userA) + add_users(self.staff, CourseStaffRole(self.courseA), self.staffA) + + +@skip_unless_cms +@ddt.ddt +class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): + """ + Testing various cases for the ObjectTagView. + """ @ddt.data( # 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), + ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staff", "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), + ("staffA", "tA1", [], status.HTTP_200_OK), + ("staff", "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), + ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staff", "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), + ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), ) @ddt.unpack def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -735,20 +1107,36 @@ def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status) assert set(t["value"] for t in response.data) == 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), + "staffA", + "staff", + ) + def test_tag_course_disabled_taxonomy(self, user_attr): + """ + Nobody can use disable taxonomies to tag objects + """ + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + disabled_taxonomy = self.tA2 + assert disabled_taxonomy.enabled is False + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseA, taxonomy_id=disabled_taxonomy.pk) + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + @ddt.data( + ("staffA", "tA1"), + ("staff", "tA1"), + ("staffA", "multiple_taxonomy"), + ("staff", "multiple_taxonomy"), ) @ddt.unpack - def test_tag_course_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + def test_tag_course_invalid(self, user_attr, taxonomy_attr): + """ + Tests that nobody can add invalid tags to a course using a closed taxonomy + """ if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) @@ -757,33 +1145,19 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr, tag_values, expected 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 + response = self.client.put(url, {"tags": ["invalid"]}, format="json") + assert response.status_code == status.HTTP_400_BAD_REQUEST @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), + # userA and userS are staff in courseA (owner of xblockA) and can tag using any taxonomies + ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staffA", "tA1", [], status.HTTP_200_OK), + ("staff", "tA1", [], status.HTTP_200_OK), + ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), ) @ddt.unpack def test_tag_xblock(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -803,20 +1177,36 @@ def test_tag_xblock(self, user_attr, taxonomy_attr, tag_values, expected_status) assert set(t["value"] for t in response.data) == 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), + "staffA", + "staff", + ) + def test_tag_xblock_disabled_taxonomy(self, user_attr): + """ + Tests that nobody can use disabled taxonomies to tag xblocks + """ + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + disabled_taxonomy = self.tA2 + assert disabled_taxonomy.enabled is False + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.xblockA, taxonomy_id=disabled_taxonomy.pk) + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + @ddt.data( + ("staffA", "tA1"), + ("staff", "tA1"), + ("staffA", "multiple_taxonomy"), + ("staff", "multiple_taxonomy"), ) @ddt.unpack - def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): + """ + Tests that staff can't add invalid tags to a xblock using a closed taxonomy + """ if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) @@ -825,9 +1215,8 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected 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 + response = self.client.put(url, {"tags": ["invalid"]}, format="json") + assert response.status_code == status.HTTP_400_BAD_REQUEST @ddt.data( "courseB", @@ -837,7 +1226,7 @@ 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) + self.client.force_authenticate(user=self.staffA) object_id = getattr(self, objectid_attr) url = OBJECT_TAG_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.tA1.pk) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 7bdfe7cd3961..6f1f7dd73a9c 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -2,7 +2,7 @@ Tagging Org API Views """ -from openedx_tagging.core.tagging.rest_api.v1.views import TaxonomyView +from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from ...api import ( @@ -10,8 +10,9 @@ get_taxonomies, get_taxonomies_for_org, ) +from ...rules import get_admin_orgs from .serializers import TaxonomyOrgListQueryParamsSerializer -from .filters import UserOrgFilterBackend +from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend class TaxonomyOrgView(TaxonomyView): @@ -57,4 +58,15 @@ def perform_create(self, serializer): """ Create a new taxonomy. """ - serializer.instance = create_taxonomy(**serializer.validated_data) + user_admin_orgs = get_admin_orgs(self.request.user) + serializer.instance = create_taxonomy(**serializer.validated_data, orgs=user_admin_orgs) + + +class ObjectTagOrgView(ObjectTagView): + """ + View to create and retrieve ObjectTags for a provided Object ID (object_id). + This view extends the ObjectTagView to add Organization filters for the results. + + Refer to ObjectTagView docstring for usage details. + """ + filter_backends = [ObjectTagTaxonomyOrgFilterBackend] diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index bad38019ce51..8c5c4be1ec86 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -6,35 +6,223 @@ import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging +from organizations.models import Organization 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 +from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access +from common.djangoapps.student.models import CourseAccessRole +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseStaffRole, + OrgContentCreatorRole, + OrgInstructorRole, + OrgLibraryUserRole, + OrgStaffRole +) +from openedx.core.djangoapps.content_libraries.api import get_libraries_for_user from .models import TaxonomyOrg UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] -def is_taxonomy_user(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: +def is_org_admin(user: UserType, orgs: list[Organization] | None = None) -> bool: """ - Returns True if the given user is a Taxonomy User for the given content taxonomy. + Return True if the given user is an admin for any of the given orgs. + """ + + return len(get_admin_orgs(user, orgs)) > 0 + + +def is_org_content_creator(user: UserType, orgs: list[Organization]) -> bool: + """ + Return True if the given user is a content creator for any of the given orgs. + """ + return len(get_content_creator_orgs(user, orgs)) > 0 + + +def is_org_user(user: UserType, orgs: list[Organization]) -> bool: + """ + Return True if the given user is a member of any of the given orgs. + """ + return len(get_user_orgs(user, orgs)) > 0 + + +def is_org_instructor(user: UserType, orgs: list[Organization]) -> bool: + """ + Return True if the given user is an instructor for any of the given orgs. + """ + return len(get_instructor_orgs(user, orgs)) > 0 + + +def get_admin_orgs(user: UserType, orgs: list[Organization] | None = None) -> list[Organization]: + """ + Returns a list of orgs that the given user is an admin, from the given list of orgs. + + If no orgs are provided, check all orgs + """ + org_list = Organization.objects.all() if orgs is None else orgs + return [ + org for org in org_list if OrgStaffRole(org=org.short_name).has_user(user) + ] + + +def get_content_creator_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: + """ + Returns a list of orgs that the given user is a content creator, the given list of orgs. + """ + return [ + org for org in orgs if ( + OrgLibraryUserRole(org=org.short_name).has_user(user) or + OrgInstructorRole(org=org.short_name).has_user(user) or + OrgContentCreatorRole(org=org.short_name).has_user(user) + ) + ] + + +def get_instructor_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: + """ + Returns a list of orgs that the given user is an instructor, from the given list of orgs. + """ + instructor_roles = CourseAccessRole.objects.filter( + org__in=(org.short_name for org in orgs), + user=user, + role__in=(CourseStaffRole.ROLE, CourseInstructorRole.ROLE), + ) + instructor_orgs = [role.org for role in instructor_roles] + return [org for org in orgs if org.short_name in instructor_orgs] + + +def get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: + """ + Returns a list of orgs that the given user has explicity permission, from the given list of orgs. + """ + return [ + org for org in orgs if ( + len(get_libraries_for_user(user, org=org.short_name)) > 0 + ) + ] + + +def get_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: + """ + Return a list of orgs that the given user is a member of (instructor or content creator), from the given list of orgs. + """ + content_creator_orgs = get_content_creator_orgs(user, orgs) + instructor_orgs = get_instructor_orgs(user, orgs) + library_user_orgs = get_library_user_orgs(user, orgs) + user_orgs = list(set(content_creator_orgs) | set(instructor_orgs) | set(library_user_orgs)) + + return user_orgs + + +def can_create_taxonomy(user: UserType) -> bool: + """ + Returns True if the given user can create a taxonomy. - Taxonomy users include global staff and superusers, plus course creators who can create courses for any org. - 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. + Taxonomy admins and org-level staff can create taxonomies. """ + # Taxonomy admins can view any taxonomy if oel_tagging.is_taxonomy_admin(user): return True + # Org-level staff can create taxonomies associated with one of their orgs. + if is_org_admin(user): + return True + + return False + + +@rules.predicate +def can_view_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: + """ + Returns True if the given user can view the given taxonomy. + + Taxonomy admins can view any taxonomy. + Org-level staff can view any taxonomy that is associated with one of their orgs. + Org-level course creators and instructors can view any enabled taxonomy that is owned by one of their orgs. + """ + # The following code allows METHOD permission (GET) in the viewset for everyone + if not taxonomy: + return True + + taxonomy = taxonomy.cast() + + # Taxonomy admins can view any taxonomy + if oel_tagging.is_taxonomy_admin(user): + return True + + is_all_org = TaxonomyOrg.objects.filter( + taxonomy=taxonomy, + org=None, + rel_type=TaxonomyOrg.RelType.OWNER, + ).exists() + + # Enabled all-org taxonomies can be viewed by any registred user + if is_all_org: + return taxonomy.enabled + taxonomy_orgs = TaxonomyOrg.get_organizations( taxonomy=taxonomy, rel_type=TaxonomyOrg.RelType.OWNER, ) - for org in taxonomy_orgs: - if is_content_creator(user, org.short_name): - return True + + # Org-level staff can view any taxonomy that is associated with one of their orgs. + if is_org_admin(user, taxonomy_orgs): + return True + + # Org-level course creators and instructors can view any enabled taxonomy that is owned by one of their orgs. + if is_org_user(user, taxonomy_orgs): + return taxonomy.enabled + + return False + + +@rules.predicate +def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: + """ + Returns True if the given user can edit the given taxonomy. + + System definied taxonomies cannot be edited + Taxonomy admins can edit any non system defined taxonomies + Only taxonomy admins can edit all org taxonomies + Org-level staff can edit any taxonomy that is associated with one of their orgs. + """ + # The following code allows METHOD permission (PUT, PATCH) in the viewset for everyone + if not taxonomy: + return True + + taxonomy = taxonomy.cast() + + # System definied taxonomies cannot be edited + if taxonomy.system_defined: + return False + + # Taxonomy admins can edit any non system defined taxonomies + if oel_tagging.is_taxonomy_admin(user): + return True + + is_all_org = TaxonomyOrg.objects.filter( + taxonomy=taxonomy, + org=None, + rel_type=TaxonomyOrg.RelType.OWNER, + ).exists() + + # Only taxonomy admins can edit all org taxonomies + if is_all_org: + return False + + taxonomy_orgs = TaxonomyOrg.get_organizations( + taxonomy=taxonomy, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + + # Org-level staff can edit any taxonomy that is associated with one of their orgs. + if is_org_admin(user, taxonomy_orgs): + return True + return False @@ -56,13 +244,31 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) +@rules.predicate +def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: + """ + Everyone that has permission to view the object should be able to tag it. + """ + 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_read_access(user, course_key) + + @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)) + return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) @rules.predicate @@ -82,10 +288,10 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) # 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) +rules.set_perm("oel_tagging.add_taxonomy", can_create_taxonomy) +rules.set_perm("oel_tagging.change_taxonomy", can_change_taxonomy) +rules.set_perm("oel_tagging.delete_taxonomy", can_change_taxonomy) +rules.set_perm("oel_tagging.view_taxonomy", can_view_taxonomy) # Tag rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) @@ -95,11 +301,12 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) # ObjectTag 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) +rules.set_perm("oel_tagging.change_objecttag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.delete_objecttag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.view_objecttag", rules.always_allow) # 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.view_objecttag_objectid", can_view_object_tag_objectid) 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) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index c0595dffb679..985199322deb 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -22,7 +22,7 @@ def setUp(self): name="Learning Objectives", enabled=False, ) - api.set_taxonomy_orgs(self.taxonomy_disabled, all_orgs=True) + api.set_taxonomy_orgs(self.taxonomy_disabled, orgs=[self.org1, self.org2]) self.taxonomy_all_orgs = api.create_taxonomy( name="Content Types", enabled=True, @@ -121,8 +121,8 @@ def test_get_taxonomies_enabled_subclasses(self): @ddt.data( # All orgs (None, True, ["taxonomy_all_orgs"]), - (None, False, ["taxonomy_disabled"]), - (None, None, ["taxonomy_all_orgs", "taxonomy_disabled"]), + (None, False, []), + (None, None, ["taxonomy_all_orgs"]), # Org 1 ("org1", True, ["taxonomy_all_orgs", "taxonomy_one_org", "taxonomy_both_orgs"]), ("org1", False, ["taxonomy_disabled"]), diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 013b82255840..8870ede83269 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -12,7 +12,7 @@ from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role -from common.djangoapps.student.roles import CourseCreatorRole, CourseStaffRole, OrgContentCreatorRole +from common.djangoapps.student.roles import CourseStaffRole, OrgStaffRole from .. import api from .test_api import TestTaxonomyMixin @@ -41,12 +41,6 @@ def setUp(self): email="staff@example.com", is_staff=True, ) - # Normal user: grant course creator role (for all orgs) - self.user_all_orgs = User.objects.create( - username="user_all_orgs", - email="staff+all@example.com", - ) - add_users(self.staff, CourseCreatorRole(), self.user_all_orgs) # Normal user: grant course creator access to both org1 and org2 self.user_both_orgs = User.objects.create( @@ -55,7 +49,7 @@ def setUp(self): ) update_org_role( self.staff, - OrgContentCreatorRole, + OrgStaffRole, self.user_both_orgs, [self.org1.short_name, self.org2.short_name], ) @@ -66,7 +60,7 @@ def setUp(self): email="staff+org2@example.com", ) update_org_role( - self.staff, OrgContentCreatorRole, self.user_org2, [self.org2.short_name] + self.staff, OrgStaffRole, self.user_org2, [self.org2.short_name] ) # Normal user: no course creator access @@ -95,9 +89,7 @@ def setUp(self): 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) @@ -155,7 +147,7 @@ def setUp(self): object_id=str(self.xblock1), ) - self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( + self.disabled_course2_tag_perm = ChangeObjectTagPermissionItem( taxonomy=self.taxonomy_disabled, object_id=str(self.course2), ) @@ -184,8 +176,6 @@ def _expected_users_have_perm( assert self.superuser.has_perm(perm, obj) assert self.staff.has_perm(perm) assert self.staff.has_perm(perm, obj) - assert self.user_all_orgs.has_perm(perm) - assert self.user_all_orgs.has_perm(perm, obj) # Org content creators are bound by a taxonomy's org restrictions assert self.user_both_orgs.has_perm(perm) == learner_perm @@ -199,58 +189,78 @@ def _expected_users_have_perm( assert self.learner.has_perm(perm, obj) == learner_obj # Taxonomy + def test_taxonomy_base_add_permissions(self): + """ + Test that staff, superuser and org admins can call POST on taxonomies. + """ + perm = "oel_tagging.add_taxonomy" + assert self.superuser.has_perm(perm) + assert self.staff.has_perm(perm) + assert self.user_both_orgs.has_perm(perm) + assert self.user_org2.has_perm(perm) + assert not self.learner.has_perm(perm) @ddt.data( - "oel_tagging.add_taxonomy", "oel_tagging.change_taxonomy", "oel_tagging.delete_taxonomy", ) def test_taxonomy_base_edit_permissions(self, perm): """ - Test that only Staff & Superuser can call add/edit/delete taxonomies. + Test that everyone can call PUT, PATCH and DELETE on taxonomies. """ assert self.superuser.has_perm(perm) assert self.staff.has_perm(perm) - assert not self.user_all_orgs.has_perm(perm) - assert not self.user_both_orgs.has_perm(perm) - assert not self.user_org2.has_perm(perm) - assert not self.learner.has_perm(perm) + assert self.user_both_orgs.has_perm(perm) + assert self.user_org2.has_perm(perm) + assert self.learner.has_perm(perm) @ddt.data( "oel_tagging.view_taxonomy", ) def test_taxonomy_base_view_permissions(self, perm): """ - Test that everyone can call view taxonomy. + Test that everyone can call GET on taxonomies. """ assert self.superuser.has_perm(perm) assert self.staff.has_perm(perm) - assert self.user_all_orgs.has_perm(perm) assert self.user_both_orgs.has_perm(perm) assert self.user_org2.has_perm(perm) assert self.learner.has_perm(perm) @ddt.data( - ("oel_tagging.change_taxonomy", "taxonomy_all_orgs"), ("oel_tagging.change_taxonomy", "taxonomy_disabled"), ("oel_tagging.change_taxonomy", "taxonomy_both_orgs"), ("oel_tagging.change_taxonomy", "taxonomy_one_org"), - ("oel_tagging.change_taxonomy", "taxonomy_no_orgs"), - ("oel_tagging.delete_taxonomy", "taxonomy_all_orgs"), ("oel_tagging.delete_taxonomy", "taxonomy_disabled"), ("oel_tagging.delete_taxonomy", "taxonomy_both_orgs"), ("oel_tagging.delete_taxonomy", "taxonomy_one_org"), - ("oel_tagging.delete_taxonomy", "taxonomy_no_orgs"), ) @ddt.unpack def test_change_taxonomy(self, perm, taxonomy_attr): """ - Test that only Staff & Superuser can edit/delete taxonomies. + Test that only instance level and org level admins can edit/delete taxonomies from their orgs. + """ + taxonomy = getattr(self, taxonomy_attr) + assert self.superuser.has_perm(perm, taxonomy) + assert self.staff.has_perm(perm, taxonomy) + assert self.user_both_orgs.has_perm(perm, taxonomy) + assert self.user_org2.has_perm(perm, taxonomy) == (taxonomy_attr != "taxonomy_one_org") + assert not self.learner.has_perm(perm, taxonomy) + + @ddt.data( + ("oel_tagging.change_taxonomy", "taxonomy_all_orgs"), + ("oel_tagging.change_taxonomy", "taxonomy_no_orgs"), + ("oel_tagging.delete_taxonomy", "taxonomy_all_orgs"), + ("oel_tagging.delete_taxonomy", "taxonomy_no_orgs"), + ) + @ddt.unpack + def test_change_taxonomy_all_no_org(self, perm, taxonomy_attr): + """ + Test that only Staff & Superuser can edit/delete taxonomies from all or no org. """ taxonomy = getattr(self, taxonomy_attr) assert self.superuser.has_perm(perm, taxonomy) assert self.staff.has_perm(perm, taxonomy) - assert not self.user_all_orgs.has_perm(perm, taxonomy) assert not self.user_both_orgs.has_perm(perm, taxonomy) assert not self.user_org2.has_perm(perm, taxonomy) assert not self.learner.has_perm(perm, taxonomy) @@ -270,20 +280,31 @@ def test_system_taxonomy(self, perm): system_taxonomy = system_taxonomy.cast() assert self.superuser.has_perm(perm, system_taxonomy) assert not self.staff.has_perm(perm, system_taxonomy) - assert not self.user_all_orgs.has_perm(perm, system_taxonomy) assert not self.user_both_orgs.has_perm(perm, system_taxonomy) assert not self.user_org2.has_perm(perm, system_taxonomy) assert not self.learner.has_perm(perm, system_taxonomy) + def test_view_taxonomy_no_orgs(self): + """ + Test that only Staff & Superuser can view taxonomies with no orgs. + """ + taxonomy = self.taxonomy_no_orgs + taxonomy.enabled = True + perm = "oel_tagging.view_taxonomy" + + assert self.superuser.has_perm(perm, taxonomy) + assert self.staff.has_perm(perm, taxonomy) + assert not self.user_both_orgs.has_perm(perm, taxonomy) + assert not self.user_org2.has_perm(perm, taxonomy) + assert not self.learner.has_perm(perm, taxonomy) + @ddt.data( - "taxonomy_all_orgs", "taxonomy_both_orgs", "taxonomy_one_org", - "taxonomy_no_orgs", ) def test_view_taxonomy_enabled(self, taxonomy_attr): """ - Test that anyone can view enabled taxonomies. + Test that anyone can view enabled taxonomies from their org. """ taxonomy = getattr(self, taxonomy_attr) taxonomy.enabled = True @@ -291,20 +312,31 @@ def test_view_taxonomy_enabled(self, taxonomy_attr): assert self.superuser.has_perm(perm, taxonomy) assert self.staff.has_perm(perm, taxonomy) - assert self.user_all_orgs.has_perm(perm, taxonomy) + assert self.user_both_orgs.has_perm(perm, taxonomy) + assert self.user_org2.has_perm(perm, taxonomy) == (taxonomy_attr != "taxonomy_one_org") + assert not self.learner.has_perm(perm, taxonomy) + + def test_view_taxonomy_enabled_all_orgs(self): + """ + Test that anyone can view enabled global taxonomies. + """ + taxonomy = self.taxonomy_all_orgs + taxonomy.enabled = True + perm = "oel_tagging.view_taxonomy" + + assert self.superuser.has_perm(perm, taxonomy) + assert self.staff.has_perm(perm, taxonomy) assert self.user_both_orgs.has_perm(perm, taxonomy) assert self.user_org2.has_perm(perm, taxonomy) assert self.learner.has_perm(perm, taxonomy) @ddt.data( - "taxonomy_all_orgs", "taxonomy_both_orgs", "taxonomy_one_org", - "taxonomy_no_orgs", ) def test_view_taxonomy_disabled(self, taxonomy_attr): """ - Test that only Staff & Superuser can view disabled taxonomies. + Test that only instance level and org level admins can view disabled taxonomies. """ taxonomy = getattr(self, taxonomy_attr) taxonomy.enabled = False @@ -312,7 +344,34 @@ def test_view_taxonomy_disabled(self, taxonomy_attr): assert self.superuser.has_perm(perm, taxonomy) assert self.staff.has_perm(perm, taxonomy) - assert not self.user_all_orgs.has_perm(perm, taxonomy) + assert self.user_both_orgs.has_perm(perm, taxonomy) + assert self.user_org2.has_perm(perm, taxonomy) == (taxonomy_attr != "taxonomy_one_org") + assert not self.learner.has_perm(perm, taxonomy) + + def test_view_taxonomy_all_orgs_disabled(self): + """ + Test that only instance level admins can view disabled all org taxonomies. + """ + taxonomy = self.taxonomy_all_orgs + taxonomy.enabled = False + perm = "oel_tagging.view_taxonomy" + + assert self.superuser.has_perm(perm, taxonomy) + assert self.staff.has_perm(perm, taxonomy) + assert not self.user_both_orgs.has_perm(perm, taxonomy) + assert not self.user_org2.has_perm(perm, taxonomy) + assert not self.learner.has_perm(perm, taxonomy) + + def test_view_taxonomy_disabled_no_org(self): + """ + Test that only Staff & Superuser can view disabled taxonomies with no orgs. + """ + taxonomy = self.taxonomy_no_orgs + taxonomy.enabled = False + perm = "oel_tagging.view_taxonomy" + + assert self.superuser.has_perm(perm, taxonomy) + assert self.staff.has_perm(perm, taxonomy) assert not self.user_both_orgs.has_perm(perm, taxonomy) assert not self.user_org2.has_perm(perm, taxonomy) assert not self.learner.has_perm(perm, taxonomy) @@ -330,21 +389,17 @@ def test_tag_base_edit_permissions(self, perm): """ assert self.superuser.has_perm(perm) assert self.staff.has_perm(perm) - assert not self.user_all_orgs.has_perm(perm) assert not self.user_both_orgs.has_perm(perm) assert not self.user_org2.has_perm(perm) assert not self.learner.has_perm(perm) - @ddt.data( - "oel_tagging.view_tag", - ) - def test_tag_base_view_permissions(self, perm): + def test_tag_base_view_permissions(self): """ Test that everyone can call view tag. """ + perm = "oel_tagging.view_tag" assert self.superuser.has_perm(perm) assert self.staff.has_perm(perm) - assert self.user_all_orgs.has_perm(perm) assert self.user_both_orgs.has_perm(perm) assert self.user_org2.has_perm(perm) assert self.learner.has_perm(perm) @@ -369,7 +424,6 @@ def test_change_tag(self, perm, tag_attr): tag = getattr(self, tag_attr) assert self.superuser.has_perm(perm, tag) assert self.staff.has_perm(perm, tag) - assert not self.user_all_orgs.has_perm(perm, tag) assert not self.user_both_orgs.has_perm(perm, tag) assert not self.user_org2.has_perm(perm, tag) assert not self.learner.has_perm(perm, tag) @@ -394,7 +448,6 @@ def test_system_taxonomy_tag(self, perm): assert self.superuser.has_perm(perm, tag_system_taxonomy) assert not self.staff.has_perm(perm, tag_system_taxonomy) - assert not self.user_all_orgs.has_perm(perm, tag_system_taxonomy) assert not self.user_both_orgs.has_perm(perm, tag_system_taxonomy) assert not self.user_org2.has_perm(perm, tag_system_taxonomy) assert not self.learner.has_perm(perm, tag_system_taxonomy) @@ -419,7 +472,6 @@ def test_free_text_taxonomy_tag(self, perm): assert self.superuser.has_perm(perm, tag_free_text_taxonomy) assert not self.staff.has_perm(perm, tag_free_text_taxonomy) - assert not self.user_all_orgs.has_perm(perm, tag_free_text_taxonomy) assert not self.user_both_orgs.has_perm(perm, tag_free_text_taxonomy) assert not self.user_org2.has_perm(perm, tag_free_text_taxonomy) assert not self.learner.has_perm(perm, tag_free_text_taxonomy) @@ -437,7 +489,6 @@ def test_tag_no_taxonomy(self, perm): assert self.staff.has_perm(perm, tag) # Everyone else can't do anything - assert not self.user_all_orgs.has_perm(perm, tag) assert not self.user_both_orgs.has_perm(perm, tag) assert not self.user_org2.has_perm(perm, tag) assert not self.learner.has_perm(perm, tag) @@ -459,28 +510,29 @@ def test_view_tag(self, tag_attr): # ObjectTag @ddt.data( - ("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"), + ("oel_tagging.add_objecttag", "disabled_course2_tag_perm"), + ("oel_tagging.change_objecttag", "disabled_course2_tag_perm"), + ("oel_tagging.delete_objecttag", "disabled_course2_tag_perm"), ) @ddt.unpack def test_object_tag_disabled_taxonomy(self, perm, tag_attr): - """Only taxonomy administrators can create/edit an ObjectTag using a disabled Taxonomy""" + """ + Only superuser 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.staff.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", "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"), + ("oel_tagging.add_objecttag", "tax_no_org_course1"), + ("oel_tagging.add_objecttag", "tax_no_org_xblock1"), + ("oel_tagging.change_objecttag", "tax_no_org_course1"), + ("oel_tagging.change_objecttag", "tax_no_org_xblock1"), + ("oel_tagging.delete_objecttag", "tax_no_org_xblock1"), + ("oel_tagging.delete_objecttag", "tax_no_org_course1"), ) @ddt.unpack def test_object_tag_no_orgs(self, perm, tag_attr): @@ -488,15 +540,14 @@ def test_object_tag_no_orgs(self, perm, tag_attr): object_tag = getattr(self, tag_attr) assert self.superuser.has_perm(perm, object_tag) assert self.staff.has_perm(perm, object_tag) - assert not self.user_all_orgs.has_perm(perm, object_tag) assert not self.user_both_orgs.has_perm(perm, object_tag) assert not self.user_org2.has_perm(perm, object_tag) assert not self.learner.has_perm(perm, object_tag) @ddt.data( - "oel_tagging.add_object_tag", - "oel_tagging.change_object_tag", - "oel_tagging.delete_object_tag", + "oel_tagging.add_objecttag", + "oel_tagging.change_objecttag", + "oel_tagging.delete_objecttag", ) def test_change_object_tag_all_orgs(self, perm): """ @@ -506,18 +557,17 @@ def test_change_object_tag_all_orgs(self, perm): 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"), - ("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"), + ("oel_tagging.add_objecttag", "tax1_course1"), + ("oel_tagging.add_objecttag", "tax1_xblock1"), + ("oel_tagging.change_objecttag", "tax1_course1"), + ("oel_tagging.change_objecttag", "tax1_xblock1"), + ("oel_tagging.delete_objecttag", "tax1_course1"), + ("oel_tagging.delete_objecttag", "tax1_xblock1"), ) @ddt.unpack def test_change_object_tag_org1(self, perm, tag_attr): @@ -525,7 +575,6 @@ def test_change_object_tag_org1(self, perm, tag_attr): 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) @@ -542,7 +591,7 @@ def test_view_object_tag(self, tag_attr): """Anyone can view any ObjectTag""" object_tag = getattr(self, tag_attr) self._expected_users_have_perm( - "oel_tagging.view_object_tag", + "oel_tagging.view_objecttag", object_tag, learner_perm=True, learner_obj=True, @@ -601,7 +650,6 @@ def test_no_orgs_no_perms(self, perm, taxonomy_attr): assert self.staff.has_perm(perm, taxonomy) # But everyone else's object-level access is removed - assert not self.user_all_orgs.has_perm(perm, taxonomy) assert not self.user_both_orgs.has_perm(perm, taxonomy) assert not self.user_org2.has_perm(perm, taxonomy) assert not self.learner.has_perm(perm, taxonomy) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index f39da29f11c5..6b68db0e11bf 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -121,7 +121,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.2.3 +openedx-learning==0.2.4 # lti-consumer-xblock 9.6.2 contains a breaking change that makes # existing custom parameter configurations unusable. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index c5ed717a7720..036a3401911d 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -785,7 +785,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.2.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 86cb1ccb51d4..81220b1b0756 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1318,7 +1318,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.2.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 4f6abf333fda..570ab8ff981e 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -925,7 +925,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.2.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 1c727d2f9c2c..327d50b5495d 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -117,7 +117,8 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require openedx-events # 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/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies openedx-mongodbproxy openedx-django-wiki openedx-blockstore diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 2aabad42efe6..1818d5b4c012 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -992,7 +992,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.2.3 +openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From fd29a6d5c59ddfaec951ecd92e8132881022bf91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 12 Oct 2023 18:26:33 -0300 Subject: [PATCH 02/30] fix: rename ChangeObjectTagPermissionItem -> ObjectTagPermissionItem --- .../content_tagging/tests/test_rules.py | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 8870ede83269..9e1c60dfffd7 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -8,7 +8,7 @@ Tag, UserSystemDefinedTaxonomy, ) -from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem +from openedx_tagging.core.tagging.rules import ObjectTagPermissionItem from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role @@ -94,60 +94,60 @@ 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( + self.tax_all_course1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_all_orgs, object_id=str(self.course1), ) - self.tax_all_course2 = ChangeObjectTagPermissionItem( + self.tax_all_course2 = ObjectTagPermissionItem( taxonomy=self.taxonomy_all_orgs, object_id=str(self.course2), ) - self.tax_all_xblock1 = ChangeObjectTagPermissionItem( + self.tax_all_xblock1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_all_orgs, object_id=str(self.xblock1), ) - self.tax_all_xblock2 = ChangeObjectTagPermissionItem( + self.tax_all_xblock2 = ObjectTagPermissionItem( taxonomy=self.taxonomy_all_orgs, object_id=str(self.xblock2), ) - self.tax_both_course1 = ChangeObjectTagPermissionItem( + self.tax_both_course1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_both_orgs, object_id=str(self.course1), ) - self.tax_both_course2 = ChangeObjectTagPermissionItem( + self.tax_both_course2 = ObjectTagPermissionItem( taxonomy=self.taxonomy_both_orgs, object_id=str(self.course2), ) - self.tax_both_xblock1 = ChangeObjectTagPermissionItem( + self.tax_both_xblock1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_both_orgs, object_id=str(self.xblock1), ) - self.tax_both_xblock2 = ChangeObjectTagPermissionItem( + self.tax_both_xblock2 = ObjectTagPermissionItem( taxonomy=self.taxonomy_both_orgs, object_id=str(self.xblock2), ) - self.tax1_course1 = ChangeObjectTagPermissionItem( + self.tax1_course1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_one_org, object_id=str(self.course1), ) - self.tax1_xblock1 = ChangeObjectTagPermissionItem( + self.tax1_xblock1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_one_org, object_id=str(self.xblock1), ) - self.tax_no_org_course1 = ChangeObjectTagPermissionItem( + self.tax_no_org_course1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_no_orgs, object_id=str(self.course1), ) - self.tax_no_org_xblock1 = ChangeObjectTagPermissionItem( + self.tax_no_org_xblock1 = ObjectTagPermissionItem( taxonomy=self.taxonomy_no_orgs, object_id=str(self.xblock1), ) - self.disabled_course2_tag_perm = ChangeObjectTagPermissionItem( + self.disabled_course2_tag_perm = ObjectTagPermissionItem( taxonomy=self.taxonomy_disabled, object_id=str(self.course2), ) From 09b62ffc004b90041c1f5e7b21eb637c2f68a6f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 14:50:37 -0300 Subject: [PATCH 03/30] refactor: use content library api --- .../rest_api/v1/tests/test_views.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 2b4ff2a6b331..912458e6a3a7 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -9,7 +9,7 @@ import ddt import uuid from django.contrib.auth import get_user_model -from django.test.testcases import override_settings +from django.test import override_settings 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 @@ -27,8 +27,8 @@ OrgLibraryUserRole, OrgStaffRole, ) -from openedx.core.djangoapps.content_libraries.api import set_library_user_permissions, AccessLevel -from openedx.core.djangoapps.content_libraries.models import ContentLibrary +from openedx.core.djangoapps.content_libraries.api import AccessLevel, create_library, set_library_user_permissions +from openedx.core.djangoapps.content_libraries.constants import COMPLEX, ALL_RIGHTS_RESERVED from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -129,12 +129,16 @@ def setUp(self): username="library_userA", email="library_userA@example.com", ) - self.content_libraryA = ContentLibrary.objects.create( + self.content_libraryA = create_library( + collection_uuid=uuid.uuid4(), org=self.orgA, - slug='foobar', - bundle_uuid=uuid.uuid4(), + slug="lib_a", + library_type=COMPLEX, + title="Library Org A", + description="This is a library from Org A", allow_public_learning=False, allow_public_read=False, + library_license=ALL_RIGHTS_RESERVED, ) set_library_user_permissions( self.content_libraryA.library_key, From a1cc0fa809d67d37f6c2477309ddece62d0367d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 15:31:06 -0300 Subject: [PATCH 04/30] test: fix tests --- .../rest_api/v1/tests/test_views.py | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 912458e6a3a7..8e4d7403afab 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -31,6 +31,7 @@ from openedx.core.djangoapps.content_libraries.constants import COMPLEX, ALL_RIGHTS_RESERVED from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.lib import blockstore_api User = get_user_model() @@ -129,8 +130,9 @@ def setUp(self): username="library_userA", email="library_userA@example.com", ) + self.collection = blockstore_api.create_collection("Test library collection") self.content_libraryA = create_library( - collection_uuid=uuid.uuid4(), + collection_uuid=self.collection.uuid, org=self.orgA, slug="lib_a", library_type=COMPLEX, @@ -141,7 +143,7 @@ def setUp(self): library_license=ALL_RIGHTS_RESERVED, ) set_library_user_permissions( - self.content_libraryA.library_key, + self.content_libraryA.key, self.library_userA, AccessLevel.READ_LEVEL ) @@ -688,7 +690,7 @@ def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: ) @ddt.data( - (None, status.HTTP_403_FORBIDDEN), + (None, status.HTTP_401_UNAUTHORIZED), ("user", status.HTTP_403_FORBIDDEN), ("content_creatorA", status.HTTP_403_FORBIDDEN), ("instructorA", status.HTTP_403_FORBIDDEN), @@ -1076,19 +1078,15 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): @ddt.data( # 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), ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), - (None, "tA1", [], status.HTTP_403_FORBIDDEN), ("user", "tA1", [], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", [], status.HTTP_200_OK), ("staff", "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), ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), ("staff", "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), ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), @@ -1226,7 +1224,7 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): "courseB", "xblockB", ) - def test_tag_unauthorized(self, objectid_attr): + def test_tag_no_permission(self, objectid_attr): """ Test that a user without access to courseB can't apply tags to it """ @@ -1239,6 +1237,22 @@ def test_tag_unauthorized(self, objectid_attr): assert response.status_code == status.HTTP_403_FORBIDDEN + @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 + """ + 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_401_UNAUTHORIZED + @skip_unless_cms @ddt.ddt From 999fe764a249e717de4618ac5c2c5e76896dbf51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 15:48:21 -0300 Subject: [PATCH 05/30] fix: lint-import --- .../content_tagging/rest_api/v1/tests/test_views.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 8e4d7403afab..e298a07602fe 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -27,8 +27,13 @@ OrgLibraryUserRole, OrgStaffRole, ) -from openedx.core.djangoapps.content_libraries.api import AccessLevel, create_library, set_library_user_permissions -from openedx.core.djangoapps.content_libraries.constants import COMPLEX, ALL_RIGHTS_RESERVED +from openedx.core.djangoapps.content_libraries.api import ( + AccessLevel, + ALL_RIGHTS_RESERVED, + create_library, + COMPLEX, + set_library_user_permissions, +) from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api From 2d2555f282232216366a9cc0342bbf68f9926060 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 16:00:23 -0300 Subject: [PATCH 06/30] test: fix constant --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index e298a07602fe..e6d188bf7054 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -29,7 +29,6 @@ ) from openedx.core.djangoapps.content_libraries.api import ( AccessLevel, - ALL_RIGHTS_RESERVED, create_library, COMPLEX, set_library_user_permissions, @@ -145,7 +144,7 @@ def setUp(self): description="This is a library from Org A", allow_public_learning=False, allow_public_read=False, - library_license=ALL_RIGHTS_RESERVED, + library_license="", ) set_library_user_permissions( self.content_libraryA.key, From 5223bcc081735041bfe81f1d77225f3ba57af42d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 16:20:30 -0300 Subject: [PATCH 07/30] test: add export tests --- .../rest_api/v1/tests/test_views.py | 169 +++++++++++------- .../core/djangoapps/content_tagging/rules.py | 1 + 2 files changed, 110 insertions(+), 60 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index e6d188bf7054..b6cfe663923b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -257,7 +257,7 @@ def setUp(self): @skip_unless_cms @ddt.ddt @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) -class TestTaxonomyReadCreateViewSet(TestTaxonomyObjectsMixin, APITestCase): +class TestTaxonomyListCreateViewSet(TestTaxonomyObjectsMixin, APITestCase): """ Test cases for TestTaxonomyReadViewSet when ENABLE_CREATOR_GROUP is True """ @@ -405,26 +405,63 @@ def test_list_invalid_page(self) -> None: assert response.status_code == status.HTTP_404_NOT_FOUND - def _test_detail_taxonomy( - self, user_attr: str, taxonomy_attr: str, expected_status: int, reason: str = "Unexpected response status" - ) -> None: + + @ddt.data( + (None, status.HTTP_401_UNAUTHORIZED), + ("user", status.HTTP_403_FORBIDDEN), + ("content_creatorA", status.HTTP_403_FORBIDDEN), + ("instructorA", status.HTTP_403_FORBIDDEN), + ("library_staffA", status.HTTP_403_FORBIDDEN), + ("course_instructorA", status.HTTP_403_FORBIDDEN), + ("course_staffA", status.HTTP_403_FORBIDDEN), + ("library_userA", status.HTTP_403_FORBIDDEN), + ("staffA", status.HTTP_201_CREATED), + ("staff", status.HTTP_201_CREATED), + ) + @ddt.unpack + def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: """ - Helper function to call the retrieve endpoint and check the response + Tests that only Taxonomy admins and org level admins can create taxonomies """ - taxonomy = getattr(self, taxonomy_attr) + url = TAXONOMY_ORG_LIST_URL - url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) + create_data = { + "name": "taxonomy_data", + "description": "This is a description", + "enabled": True, + "allow_multiple": True, + } if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.get(url) - assert response.status_code == expected_status, reason + response = self.client.post(url, create_data, format="json") + assert response.status_code == expected_status + # If we were able to create the taxonomy, check if it was created if status.is_success(expected_status): - check_taxonomy(response.data, taxonomy.pk, **(TaxonomySerializer(taxonomy.cast()).data)) - pass + check_taxonomy(response.data, response.data["id"], **create_data) + url = TAXONOMY_ORG_DETAIL_URL.format(pk=response.data["id"]) + + response = self.client.get(url) + check_taxonomy(response.data, response.data["id"], **create_data) + + # Also checks if the taxonomy was associated with the org + if user_attr == "staffA": + assert TaxonomyOrg.objects.filter(taxonomy=response.data["id"], org=self.orgA).exists() + +@ddt.ddt +class TestTaxonomyDetailExportMixin(TestTaxonomyObjectsMixin): + """ + Test cases to be used with detail and export actions + """ + + @abstractmethod + def _test_api_call(self, **_kwargs) -> None: + """ + Helper function to call the detail/export endpoint and check the response + """ @ddt.data( "user", @@ -439,7 +476,7 @@ def test_detail_taxonomy_all_org_enabled(self, user_attr: str) -> None: """ Tests that everyone can see enabled global taxonomies """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr="t1", expected_status=status.HTTP_200_OK, @@ -508,7 +545,7 @@ def test_detail_taxonomy_org_user_see_enabled(self, user_attr: str, taxonomy_att Tests that org users (content creators and instructors) can see enabled global taxonomies and taxonomies from their orgs """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_200_OK, @@ -523,7 +560,7 @@ def test_detail_taxonomy_org_admin_see_disabled(self, taxonomy_attr: str) -> Non """ Tests that org admins can see disabled taxonomies from their orgs """ - self._test_detail_taxonomy( + self._test_api_call( user_attr="staffA", taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_200_OK, @@ -538,7 +575,7 @@ def test_detail_taxonomy_org_admin_dont_see_disabled_global(self, taxonomy_attr: """ Tests that org admins can't see disabled global taxonomies """ - self._test_detail_taxonomy( + self._test_api_call( user_attr="staffA", taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_404_NOT_FOUND, @@ -605,7 +642,7 @@ def test_detail_taxonomy_org_user_dont_see_disabled(self, user_attr: str, taxono Tests that org users (content creators and instructors) can't see disabled global taxonomies and taxonomies from their orgs """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_404_NOT_FOUND, @@ -621,7 +658,7 @@ def test_detail_taxonomy_staff_see_no_org(self, user_attr: str, taxonomy_attr: s """ Tests that staff can see taxonomies with no org """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_200_OK, @@ -641,7 +678,7 @@ def test_detail_taxonomy_other_dont_see_no_org(self, user_attr: str) -> None: """ Tests that org users can't see taxonomies with no org """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr="ot1", expected_status=status.HTTP_404_NOT_FOUND, @@ -661,7 +698,7 @@ def test_detail_taxonomy_dont_see_other_org(self, user_attr: str) -> None: """ Tests that org users can't see taxonomies from other orgs """ - self._test_detail_taxonomy( + self._test_api_call( user_attr=user_attr, taxonomy_attr="tB1", expected_status=status.HTTP_404_NOT_FOUND, @@ -686,73 +723,85 @@ def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: """ Tests that org users can't see taxonomies from other orgs """ - self._test_detail_taxonomy( + self._test_api_call( user_attr="staff", taxonomy_attr=taxonomy_attr, expected_status=status.HTTP_200_OK, reason="Staff should see all taxonomies", ) - @ddt.data( - (None, status.HTTP_401_UNAUTHORIZED), - ("user", status.HTTP_403_FORBIDDEN), - ("content_creatorA", status.HTTP_403_FORBIDDEN), - ("instructorA", status.HTTP_403_FORBIDDEN), - ("library_staffA", status.HTTP_403_FORBIDDEN), - ("course_instructorA", status.HTTP_403_FORBIDDEN), - ("course_staffA", status.HTTP_403_FORBIDDEN), - ("library_userA", status.HTTP_403_FORBIDDEN), - ("staffA", status.HTTP_201_CREATED), - ("staff", status.HTTP_201_CREATED), - ) - @ddt.unpack - def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestTaxonomyDetailViewSet(TestTaxonomyDetailExportMixin, APITestCase): + """ + Test cases for TaxonomyViewSet with detail action + """ + + def _test_api_call( + self, + user_attr: str, + taxonomy_attr: str, + expected_status: int, + reason: str = "Unexpected response status" + ) -> None: """ - Tests that only Taxonomy admins and org level admins can create taxonomies + Helper function to call the retrieve endpoint and check the response """ - url = TAXONOMY_ORG_LIST_URL + taxonomy = getattr(self, taxonomy_attr) - create_data = { - "name": "taxonomy_data", - "description": "This is a description", - "enabled": True, - "allow_multiple": True, - } + url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) if user_attr: user = getattr(self, user_attr) self.client.force_authenticate(user=user) - response = self.client.post(url, create_data, format="json") - assert response.status_code == expected_status + response = self.client.get(url) + assert response.status_code == expected_status, reason - # If we were able to create the taxonomy, check if it was created if status.is_success(expected_status): - check_taxonomy(response.data, response.data["id"], **create_data) - url = TAXONOMY_ORG_DETAIL_URL.format(pk=response.data["id"]) + check_taxonomy(response.data, taxonomy.pk, **(TaxonomySerializer(taxonomy.cast()).data)) - response = self.client.get(url) - check_taxonomy(response.data, response.data["id"], **create_data) +@skip_unless_cms +@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) +class TestTaxonomyExportViewSet(TestTaxonomyDetailExportMixin, APITestCase): + """ + Test cases for TaxonomyViewSet with export action + """ - # Also checks if the taxonomy was associated with the org - if user_attr == "staffA": - assert TaxonomyOrg.objects.filter(taxonomy=response.data["id"], org=self.orgA).exists() + def _test_api_call( + self, + user_attr: str, + taxonomy_attr: str, + expected_status: int, + reason: str = "Unexpected response status" + ) -> None: + """ + Helper function to call the export endpoint and check the response + """ + taxonomy = getattr(self, taxonomy_attr) + + url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) + + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + response = self.client.get(url) + assert response.status_code == expected_status, reason + assert len(response.data) > 0 @ddt.ddt class TestTaxonomyChangeMixin(TestTaxonomyObjectsMixin): """ - Test cases for TestTaxonomyChangeViewSet when ENABLE_CREATOR_GROUP is True + Test cases to be used with update, patch and delete actions """ - def _test_api_call( - self, - **_kwargs, - ) -> None: + @abstractmethod + def _test_api_call(self, **_kwargs) -> None: """ - Helper function to call the update endpoint and check the response + Helper function to call the update/patch/delete endpoint and check the response """ - pass @ddt.data( "ot1", @@ -984,7 +1033,7 @@ def _test_api_call( @skip_unless_cms @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyReadViewSetNoCreatorGroup(TestTaxonomyReadCreateViewSet): # pylint: disable=test-inherits-tests +class TestTaxonomyReadViewSetNoCreatorGroup(TestTaxonomyListCreateViewSet): # pylint: disable=test-inherits-tests """ Test cases for TaxonomyReadViewSet when ENABLE_CREATOR_GROUP is False diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 8c5c4be1ec86..22bfc431516a 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -292,6 +292,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) 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.export_taxonomy", oel_tagging.can_view_taxonomy) # Tag rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) From b420d59764e02b946fec3d05c737180b9fd94faf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 16:25:34 -0300 Subject: [PATCH 08/30] fix: pylint and import --- .../content_tagging/rest_api/v1/tests/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index b6cfe663923b..7ad7d3034f4f 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -6,8 +6,8 @@ from urllib.parse import parse_qs, urlparse +import abc import ddt -import uuid from django.contrib.auth import get_user_model from django.test import override_settings from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator @@ -457,7 +457,7 @@ class TestTaxonomyDetailExportMixin(TestTaxonomyObjectsMixin): Test cases to be used with detail and export actions """ - @abstractmethod + @abc.abstractmethod def _test_api_call(self, **_kwargs) -> None: """ Helper function to call the detail/export endpoint and check the response @@ -797,7 +797,7 @@ class TestTaxonomyChangeMixin(TestTaxonomyObjectsMixin): Test cases to be used with update, patch and delete actions """ - @abstractmethod + @abc.abstractmethod def _test_api_call(self, **_kwargs) -> None: """ Helper function to call the update/patch/delete endpoint and check the response From 32d20b2af85af0e7eafc033abc2161e4b28c0c22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 13 Oct 2023 16:39:29 -0300 Subject: [PATCH 09/30] style: fix pylint --- .../content_tagging/rest_api/v1/tests/test_views.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 7ad7d3034f4f..c3610081fd47 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -405,7 +405,6 @@ def test_list_invalid_page(self) -> None: assert response.status_code == status.HTTP_404_NOT_FOUND - @ddt.data( (None, status.HTTP_401_UNAUTHORIZED), ("user", status.HTTP_403_FORBIDDEN), @@ -451,6 +450,7 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: if user_attr == "staffA": assert TaxonomyOrg.objects.filter(taxonomy=response.data["id"], org=self.orgA).exists() + @ddt.ddt class TestTaxonomyDetailExportMixin(TestTaxonomyObjectsMixin): """ @@ -730,6 +730,7 @@ def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: reason="Staff should see all taxonomies", ) + @skip_unless_cms @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyDetailViewSet(TestTaxonomyDetailExportMixin, APITestCase): @@ -761,6 +762,7 @@ def _test_api_call( if status.is_success(expected_status): check_taxonomy(response.data, taxonomy.pk, **(TaxonomySerializer(taxonomy.cast()).data)) + @skip_unless_cms @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyExportViewSet(TestTaxonomyDetailExportMixin, APITestCase): From 5d85063642bbc94d9a6279befd795ca17c086609 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:20:51 -0300 Subject: [PATCH 10/30] fix: use correct ObjectTagOrgViewSet --- openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py index d6fc047e5288..38bb0a9ac17a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/urls.py @@ -7,7 +7,6 @@ from django.urls.conf import path, include from openedx_tagging.core.tagging.rest_api.v1 import ( - views as oel_tagging_views, views_import as oel_tagging_views_import, ) @@ -15,7 +14,7 @@ router = DefaultRouter() router.register("taxonomies", views.TaxonomyOrgView, basename="taxonomy") -router.register("object_tags", oel_tagging_views.ObjectTagView, basename="object_tag") +router.register("object_tags", views.ObjectTagOrgView, basename="object_tag") urlpatterns = [ path( From 02deb593d6d0880ba9008af85af824e4a34748fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:29:30 -0300 Subject: [PATCH 11/30] refactor: cleaning unused methods Co-authored-by: Jillian --- openedx/core/djangoapps/content_tagging/rules.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 22bfc431516a..6512962ce27b 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -36,13 +36,6 @@ def is_org_admin(user: UserType, orgs: list[Organization] | None = None) -> bool return len(get_admin_orgs(user, orgs)) > 0 -def is_org_content_creator(user: UserType, orgs: list[Organization]) -> bool: - """ - Return True if the given user is a content creator for any of the given orgs. - """ - return len(get_content_creator_orgs(user, orgs)) > 0 - - def is_org_user(user: UserType, orgs: list[Organization]) -> bool: """ Return True if the given user is a member of any of the given orgs. @@ -50,13 +43,6 @@ def is_org_user(user: UserType, orgs: list[Organization]) -> bool: return len(get_user_orgs(user, orgs)) > 0 -def is_org_instructor(user: UserType, orgs: list[Organization]) -> bool: - """ - Return True if the given user is an instructor for any of the given orgs. - """ - return len(get_instructor_orgs(user, orgs)) > 0 - - def get_admin_orgs(user: UserType, orgs: list[Organization] | None = None) -> list[Organization]: """ Returns a list of orgs that the given user is an admin, from the given list of orgs. From 43ebe3694c457eee70f3a49ba92015a8ce2d521e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:51:19 -0300 Subject: [PATCH 12/30] chore: update requirements constraints --- requirements/constraints.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 6b68db0e11bf..ef3b5d6c0dba 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -121,7 +121,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.2.4 +openedx-learning==0.2.5 # lti-consumer-xblock 9.6.2 contains a breaking change that makes # existing custom parameter configurations unusable. From 86ee94812ae6d610e305ea3a37aece2f888ff724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:51:33 -0300 Subject: [PATCH 13/30] test: fix test and remove unecessary checks --- .../rest_api/v1/tests/test_views.py | 74 +++++++++---------- 1 file changed, 35 insertions(+), 39 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index c3610081fd47..df9f087fa61a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -274,9 +274,8 @@ def _test_list_taxonomy( """ url = TAXONOMY_ORG_LIST_URL - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) # Set parameters cleaning empty values query_params = {k: v for k, v in {"enabled": enabled_parameter, "org": org_parameter}.items() if v is not None} @@ -375,9 +374,8 @@ def test_list_taxonomy_pagination( """ url = TAXONOMY_ORG_LIST_URL - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) query_params = {"page_size": 3, "page": 2} @@ -752,9 +750,8 @@ def _test_api_call( url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.get(url) assert response.status_code == expected_status, reason @@ -784,9 +781,8 @@ def _test_api_call( url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.get(url) assert response.status_code == expected_status, reason @@ -942,9 +938,8 @@ def _test_api_call( url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.put(url, {"name": "new name"}, format="json") assert response.status_code in expected_status, reason @@ -981,9 +976,8 @@ def _test_api_call( url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.patch(url, {"name": "new name"}, format="json") assert response.status_code in expected_status, reason @@ -1020,9 +1014,8 @@ def _test_api_call( url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.delete(url) assert response.status_code in expected_status, reason @@ -1148,9 +1141,11 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): ) @ddt.unpack 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) + """ + Tests that only staff and org level users can tag courses + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) @@ -1171,9 +1166,8 @@ def test_tag_course_disabled_taxonomy(self, user_attr): """ Nobody can use disable taxonomies to tag objects """ - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) disabled_taxonomy = self.tA2 assert disabled_taxonomy.enabled is False @@ -1194,9 +1188,8 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr): """ Tests that nobody can add invalid tags to a course using a closed taxonomy """ - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) @@ -1207,20 +1200,25 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr): @ddt.data( # userA and userS are staff in courseA (owner of xblockA) and can tag using any taxonomies + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", [], status.HTTP_200_OK), ("staff", "tA1", [], status.HTTP_200_OK), ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), ("staff", "open_taxonomy", ["tag1"], 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) + """ + Tests that only staff and org level users can tag xblocks + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) @@ -1241,9 +1239,8 @@ def test_tag_xblock_disabled_taxonomy(self, user_attr): """ Tests that nobody can use disabled taxonomies to tag xblocks """ - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) disabled_taxonomy = self.tA2 assert disabled_taxonomy.enabled is False @@ -1264,9 +1261,8 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): """ Tests that staff can't add invalid tags to a xblock using a closed taxonomy """ - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) taxonomy = getattr(self, taxonomy_attr) From bfbc1d96d79b8ce74778292ca5825b226e750612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:52:33 -0300 Subject: [PATCH 14/30] style: remove comments --- .../rest_api/v1/tests/test_views.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index df9f087fa61a..f134bf92ef25 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -237,22 +237,6 @@ def setUp(self): rel_type=TaxonomyOrg.RelType.OWNER, ) - # # ToDo: OrgX content library test - # # This will show tX1 to every studio user - # self.content_libraryX = ContentLibrary.objects.create( - # org=self.orgX, - # slug='foobar', - # bundle_uuid=uuid.uuid4(), - # allow_public_learning=True, - # allow_public_read=True, - # ) - # self.tX1 = Taxonomy.objects.create(name="tX1", enabled=True) - # TaxonomyOrg.objects.create( - # taxonomy=self.tX1, - # org=self.orgX, - # rel_type=TaxonomyOrg.RelType.OWNER, - # ) - @skip_unless_cms @ddt.ddt From 6803f50d21006fe74d24230e53715de47a2bc24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 12:07:19 -0300 Subject: [PATCH 15/30] refactor: fix pylint --- .../rest_api/v1/tests/test_views.py | 383 ++++++++++-------- 1 file changed, 203 insertions(+), 180 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index f134bf92ef25..9e65b48faf8a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -76,166 +76,194 @@ class TestTaxonomyObjectsMixin: """ def setUp(self): - super().setUp() - self.user = User.objects.create( - username="user", - email="user@example.com", - ) - self.staff = User.objects.create( - username="staff", - email="staff@example.com", - is_staff=True, - ) + def _setUp_orgs(): + """ + Create orgs for testing + """ + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.orgB = Organization.objects.create(name="Organization B", short_name="orgB") + self.orgX = Organization.objects.create(name="Organization X", short_name="orgX") + + def _setUp_courses(): + """ + Create courses for testing + """ + self.courseA = CourseLocator("orgA", "101", "test") + self.courseB = CourseLocator("orgB", "101", "test") + + def _setUp_library(): + """ + Create library for testing + """ + self.collection = blockstore_api.create_collection("Test library collection") + self.content_libraryA = create_library( + collection_uuid=self.collection.uuid, + org=self.orgA, + slug="lib_a", + library_type=COMPLEX, + title="Library Org A", + description="This is a library from Org A", + allow_public_learning=False, + allow_public_read=False, + library_license="", + ) - self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") - self.orgB = Organization.objects.create(name="Organization B", short_name="orgB") - self.orgX = Organization.objects.create(name="Organization X", short_name="orgX") + def _setUp_users(): + """ + Create users for testing + """ + self.user = User.objects.create( + username="user", + email="user@example.com", + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) - self.courseA = CourseLocator("orgA", "101", "test") - self.courseB = CourseLocator("orgB", "101", "test") + self.staffA = User.objects.create( + username="staffA", + email="userA@example.com", + ) + update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) - self.staffA = User.objects.create( - username="staffA", - email="userA@example.com", - ) - update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + self.content_creatorA = User.objects.create( + username="content_creatorA", + email="content_creatorA@example.com", + ) + update_org_role(self.staff, OrgContentCreatorRole, self.content_creatorA, [self.orgA.short_name]) - self.content_creatorA = User.objects.create( - username="content_creatorA", - email="content_creatorA@example.com", - ) - update_org_role(self.staff, OrgContentCreatorRole, self.content_creatorA, [self.orgA.short_name]) + self.instructorA = User.objects.create( + username="instructorA", + email="instructorA@example.com", + ) + update_org_role(self.staff, OrgInstructorRole, self.instructorA, [self.orgA.short_name]) - self.instructorA = User.objects.create( - username="instructorA", - email="instructorA@example.com", - ) - update_org_role(self.staff, OrgInstructorRole, self.instructorA, [self.orgA.short_name]) + self.library_staffA = User.objects.create( + username="library_staffA", + email="library_staffA@example.com", + ) + update_org_role(self.staff, OrgLibraryUserRole, self.library_staffA, [self.orgA.short_name]) - self.library_staffA = User.objects.create( - username="library_staffA", - email="library_staffA@example.com", - ) - update_org_role(self.staff, OrgLibraryUserRole, self.library_staffA, [self.orgA.short_name]) + self.course_instructorA = User.objects.create( + username="course_instructorA", + email="course_instructorA@example.com", + ) + add_users(self.staff, CourseInstructorRole(self.courseA), self.course_instructorA) - self.course_instructorA = User.objects.create( - username="course_instructorA", - email="course_instructorA@example.com", - ) - add_users(self.staff, CourseInstructorRole(self.courseA), self.course_instructorA) + self.course_staffA = User.objects.create( + username="course_staffA", + email="course_staffA@example.com", + ) + add_users(self.staff, CourseStaffRole(self.courseA), self.course_staffA) - self.course_staffA = User.objects.create( - username="course_staffA", - email="course_staffA@example.com", - ) - add_users(self.staff, CourseStaffRole(self.courseA), self.course_staffA) + self.library_userA = User.objects.create( + username="library_userA", + email="library_userA@example.com", + ) + set_library_user_permissions( + self.content_libraryA.key, + self.library_userA, + AccessLevel.READ_LEVEL + ) - self.library_userA = User.objects.create( - username="library_userA", - email="library_userA@example.com", - ) - self.collection = blockstore_api.create_collection("Test library collection") - self.content_libraryA = create_library( - collection_uuid=self.collection.uuid, - org=self.orgA, - slug="lib_a", - library_type=COMPLEX, - title="Library Org A", - description="This is a library from Org A", - allow_public_learning=False, - allow_public_read=False, - library_license="", - ) - set_library_user_permissions( - self.content_libraryA.key, - self.library_userA, - AccessLevel.READ_LEVEL - ) + def _setUp_taxonomies(): + """ + Create taxonomies for testing + """ + # Orphaned taxonomy + self.ot1 = Taxonomy.objects.create(name="ot1", enabled=True) + self.ot2 = Taxonomy.objects.create(name="ot2", enabled=False) + + # System defined taxonomy + self.st1 = Taxonomy.objects.create(name="st1", enabled=True) + self.st1.taxonomy_class = SystemDefinedTaxonomy + self.st1.save() + TaxonomyOrg.objects.create( + taxonomy=self.st1, + rel_type=TaxonomyOrg.RelType.OWNER, + org=None, + ) + self.st2 = Taxonomy.objects.create(name="st2", enabled=False) + self.st2.taxonomy_class = SystemDefinedTaxonomy + self.st2.save() + TaxonomyOrg.objects.create( + taxonomy=self.st2, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # Orphaned taxonomy - self.ot1 = Taxonomy.objects.create(name="ot1", enabled=True) - self.ot2 = Taxonomy.objects.create(name="ot2", enabled=False) + # Global taxonomy + self.t1 = Taxonomy.objects.create(name="t1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.t1, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.t2 = Taxonomy.objects.create(name="t2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.t2, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # System defined taxonomy - self.st1 = Taxonomy.objects.create(name="st1", enabled=True) - self.st1.taxonomy_class = SystemDefinedTaxonomy - self.st1.save() - TaxonomyOrg.objects.create( - taxonomy=self.st1, - rel_type=TaxonomyOrg.RelType.OWNER, - org=None, - ) - self.st2 = Taxonomy.objects.create(name="st2", enabled=False) - self.st2.taxonomy_class = SystemDefinedTaxonomy - self.st2.save() - TaxonomyOrg.objects.create( - taxonomy=self.st2, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # OrgA taxonomy + 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) + TaxonomyOrg.objects.create( + taxonomy=self.tA2, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # Global taxonomy - self.t1 = Taxonomy.objects.create(name="t1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.t1, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.t2 = Taxonomy.objects.create(name="t2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.t2, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # OrgB taxonomy + self.tB1 = Taxonomy.objects.create(name="tB1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.tB1, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.tB2 = Taxonomy.objects.create(name="tB2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.tB2, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # OrgA taxonomy - 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) - TaxonomyOrg.objects.create( - taxonomy=self.tA2, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # OrgA and OrgB taxonomy + self.tBA1 = Taxonomy.objects.create(name="tBA1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.tBA1, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.tBA1, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.tBA2 = Taxonomy.objects.create(name="tBA2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.tBA2, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.tBA2, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # OrgB taxonomy - self.tB1 = Taxonomy.objects.create(name="tB1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.tB1, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.tB2 = Taxonomy.objects.create(name="tB2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.tB2, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + super().setUp() - # OrgA and OrgB taxonomy - self.tBA1 = Taxonomy.objects.create(name="tBA1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.tBA1, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - TaxonomyOrg.objects.create( - taxonomy=self.tBA1, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.tBA2 = Taxonomy.objects.create(name="tBA2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.tBA2, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - TaxonomyOrg.objects.create( - taxonomy=self.tBA2, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + _setUp_orgs() + _setUp_courses() + _setUp_library() + _setUp_users() + _setUp_taxonomies() @skip_unless_cms @@ -720,16 +748,15 @@ class TestTaxonomyDetailViewSet(TestTaxonomyDetailExportMixin, APITestCase): Test cases for TaxonomyViewSet with detail action """ - def _test_api_call( - self, - user_attr: str, - taxonomy_attr: str, - expected_status: int, - reason: str = "Unexpected response status" - ) -> None: + def _test_api_call(self, **kwargs) -> None: """ Helper function to call the retrieve endpoint and check the response """ + user_attr = kwargs.get("user_attr") + taxonomy_attr = kwargs.get("taxonomy_attr") + expected_status = kwargs.get("expected_status") + reason = kwargs.get("reason", "Unexpected response status") + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -751,16 +778,15 @@ class TestTaxonomyExportViewSet(TestTaxonomyDetailExportMixin, APITestCase): Test cases for TaxonomyViewSet with export action """ - def _test_api_call( - self, - user_attr: str, - taxonomy_attr: str, - expected_status: int, - reason: str = "Unexpected response status" - ) -> None: + def _test_api_call(self, **kwargs) -> None: """ Helper function to call the export endpoint and check the response """ + user_attr = kwargs.get("user_attr") + taxonomy_attr = kwargs.get("taxonomy_attr") + expected_status = kwargs.get("expected_status") + reason = kwargs.get("reason", "Unexpected response status") + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -911,13 +937,12 @@ class TestTaxonomyUpdateViewSet(TestTaxonomyChangeMixin, APITestCase): Test cases for TaxonomyChangeViewSet with PUT method """ - def _test_api_call( - self, - user_attr: str, - taxonomy_attr: str, - expected_status: list[int], - reason: str = "Unexpected response status" - ) -> None: + def _test_api_call(self, **kwargs) -> None: + user_attr = kwargs.get("user_attr") + taxonomy_attr = kwargs.get("taxonomy_attr") + expected_status = kwargs.get("expected_status") + reason = kwargs.get("reason", "Unexpected response status") + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -949,13 +974,12 @@ class TestTaxonomyPatchViewSet(TestTaxonomyChangeMixin, APITestCase): Test cases for TaxonomyChangeViewSet with PATCH method """ - def _test_api_call( - self, - user_attr: str, - taxonomy_attr: str, - expected_status: list[int], - reason: str = "Unexpected response status" - ) -> None: + def _test_api_call(self, **kwargs) -> None: + user_attr = kwargs.get("user_attr") + taxonomy_attr = kwargs.get("taxonomy_attr") + expected_status = kwargs.get("expected_status") + reason = kwargs.get("reason", "Unexpected response status") + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -987,13 +1011,12 @@ class TestTaxonomyDeleteViewSet(TestTaxonomyChangeMixin, APITestCase): Test cases for TaxonomyChangeViewSet with DELETE method """ - def _test_api_call( - self, - user_attr: str, - taxonomy_attr: str, - expected_status: list[int], - reason: str = "Unexpected response status" - ) -> None: + def _test_api_call(self, **kwargs) -> None: + user_attr = kwargs.get("user_attr") + taxonomy_attr = kwargs.get("taxonomy_attr") + expected_status = kwargs.get("expected_status") + reason = kwargs.get("reason", "Unexpected response status") + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) From a6b4b4631a14b82ca045b2f233ad874cf880edf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 22:22:39 -0300 Subject: [PATCH 16/30] fix: pylint --- .../rest_api/v1/tests/test_views.py | 344 +++++++++--------- .../core/djangoapps/content_tagging/rules.py | 3 +- 2 files changed, 173 insertions(+), 174 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 9e65b48faf8a..56236fbbabb6 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -74,196 +74,194 @@ class TestTaxonomyObjectsMixin: """ Sets up data for testing Content Taxonomies. """ + def _setUp_orgs(self): + """ + Create orgs for testing + """ + self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") + self.orgB = Organization.objects.create(name="Organization B", short_name="orgB") + self.orgX = Organization.objects.create(name="Organization X", short_name="orgX") - def setUp(self): - def _setUp_orgs(): - """ - Create orgs for testing - """ - self.orgA = Organization.objects.create(name="Organization A", short_name="orgA") - self.orgB = Organization.objects.create(name="Organization B", short_name="orgB") - self.orgX = Organization.objects.create(name="Organization X", short_name="orgX") - - def _setUp_courses(): - """ - Create courses for testing - """ - self.courseA = CourseLocator("orgA", "101", "test") - self.courseB = CourseLocator("orgB", "101", "test") - - def _setUp_library(): - """ - Create library for testing - """ - self.collection = blockstore_api.create_collection("Test library collection") - self.content_libraryA = create_library( - collection_uuid=self.collection.uuid, - org=self.orgA, - slug="lib_a", - library_type=COMPLEX, - title="Library Org A", - description="This is a library from Org A", - allow_public_learning=False, - allow_public_read=False, - library_license="", - ) + def _setUp_courses(self): + """ + Create courses for testing + """ + self.courseA = CourseLocator("orgA", "101", "test") + self.courseB = CourseLocator("orgB", "101", "test") - def _setUp_users(): - """ - Create users for testing - """ - self.user = User.objects.create( - username="user", - email="user@example.com", - ) - self.staff = User.objects.create( - username="staff", - email="staff@example.com", - is_staff=True, - ) + def _setUp_library(self): + """ + Create library for testing + """ + self.collection = blockstore_api.create_collection("Test library collection") + self.content_libraryA = create_library( + collection_uuid=self.collection.uuid, + org=self.orgA, + slug="lib_a", + library_type=COMPLEX, + title="Library Org A", + description="This is a library from Org A", + allow_public_learning=False, + allow_public_read=False, + library_license="", + ) - self.staffA = User.objects.create( - username="staffA", - email="userA@example.com", - ) - update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + def _setUp_users(self): + """ + Create users for testing + """ + self.user = User.objects.create( + username="user", + email="user@example.com", + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) - self.content_creatorA = User.objects.create( - username="content_creatorA", - email="content_creatorA@example.com", - ) - update_org_role(self.staff, OrgContentCreatorRole, self.content_creatorA, [self.orgA.short_name]) + self.staffA = User.objects.create( + username="staffA", + email="userA@example.com", + ) + update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) - self.instructorA = User.objects.create( - username="instructorA", - email="instructorA@example.com", - ) - update_org_role(self.staff, OrgInstructorRole, self.instructorA, [self.orgA.short_name]) + self.content_creatorA = User.objects.create( + username="content_creatorA", + email="content_creatorA@example.com", + ) + update_org_role(self.staff, OrgContentCreatorRole, self.content_creatorA, [self.orgA.short_name]) - self.library_staffA = User.objects.create( - username="library_staffA", - email="library_staffA@example.com", - ) - update_org_role(self.staff, OrgLibraryUserRole, self.library_staffA, [self.orgA.short_name]) + self.instructorA = User.objects.create( + username="instructorA", + email="instructorA@example.com", + ) + update_org_role(self.staff, OrgInstructorRole, self.instructorA, [self.orgA.short_name]) - self.course_instructorA = User.objects.create( - username="course_instructorA", - email="course_instructorA@example.com", - ) - add_users(self.staff, CourseInstructorRole(self.courseA), self.course_instructorA) + self.library_staffA = User.objects.create( + username="library_staffA", + email="library_staffA@example.com", + ) + update_org_role(self.staff, OrgLibraryUserRole, self.library_staffA, [self.orgA.short_name]) - self.course_staffA = User.objects.create( - username="course_staffA", - email="course_staffA@example.com", - ) - add_users(self.staff, CourseStaffRole(self.courseA), self.course_staffA) + self.course_instructorA = User.objects.create( + username="course_instructorA", + email="course_instructorA@example.com", + ) + add_users(self.staff, CourseInstructorRole(self.courseA), self.course_instructorA) - self.library_userA = User.objects.create( - username="library_userA", - email="library_userA@example.com", - ) - set_library_user_permissions( - self.content_libraryA.key, - self.library_userA, - AccessLevel.READ_LEVEL - ) + self.course_staffA = User.objects.create( + username="course_staffA", + email="course_staffA@example.com", + ) + add_users(self.staff, CourseStaffRole(self.courseA), self.course_staffA) - def _setUp_taxonomies(): - """ - Create taxonomies for testing - """ - # Orphaned taxonomy - self.ot1 = Taxonomy.objects.create(name="ot1", enabled=True) - self.ot2 = Taxonomy.objects.create(name="ot2", enabled=False) - - # System defined taxonomy - self.st1 = Taxonomy.objects.create(name="st1", enabled=True) - self.st1.taxonomy_class = SystemDefinedTaxonomy - self.st1.save() - TaxonomyOrg.objects.create( - taxonomy=self.st1, - rel_type=TaxonomyOrg.RelType.OWNER, - org=None, - ) - self.st2 = Taxonomy.objects.create(name="st2", enabled=False) - self.st2.taxonomy_class = SystemDefinedTaxonomy - self.st2.save() - TaxonomyOrg.objects.create( - taxonomy=self.st2, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + self.library_userA = User.objects.create( + username="library_userA", + email="library_userA@example.com", + ) + set_library_user_permissions( + self.content_libraryA.key, + self.library_userA, + AccessLevel.READ_LEVEL + ) - # Global taxonomy - self.t1 = Taxonomy.objects.create(name="t1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.t1, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.t2 = Taxonomy.objects.create(name="t2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.t2, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + def _setUp_taxonomies(self): + """ + Create taxonomies for testing + """ + # Orphaned taxonomy + self.ot1 = Taxonomy.objects.create(name="ot1", enabled=True) + self.ot2 = Taxonomy.objects.create(name="ot2", enabled=False) - # OrgA taxonomy - 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) - TaxonomyOrg.objects.create( - taxonomy=self.tA2, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # System defined taxonomy + self.st1 = Taxonomy.objects.create(name="st1", enabled=True) + self.st1.taxonomy_class = SystemDefinedTaxonomy + self.st1.save() + TaxonomyOrg.objects.create( + taxonomy=self.st1, + rel_type=TaxonomyOrg.RelType.OWNER, + org=None, + ) + self.st2 = Taxonomy.objects.create(name="st2", enabled=False) + self.st2.taxonomy_class = SystemDefinedTaxonomy + self.st2.save() + TaxonomyOrg.objects.create( + taxonomy=self.st2, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # OrgB taxonomy - self.tB1 = Taxonomy.objects.create(name="tB1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.tB1, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.tB2 = Taxonomy.objects.create(name="tB2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.tB2, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # Global taxonomy + self.t1 = Taxonomy.objects.create(name="t1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.t1, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.t2 = Taxonomy.objects.create(name="t2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.t2, + rel_type=TaxonomyOrg.RelType.OWNER, + ) - # OrgA and OrgB taxonomy - self.tBA1 = Taxonomy.objects.create(name="tBA1", enabled=True) - TaxonomyOrg.objects.create( - taxonomy=self.tBA1, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - TaxonomyOrg.objects.create( - taxonomy=self.tBA1, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - self.tBA2 = Taxonomy.objects.create(name="tBA2", enabled=False) - TaxonomyOrg.objects.create( - taxonomy=self.tBA2, - org=self.orgA, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - TaxonomyOrg.objects.create( - taxonomy=self.tBA2, - org=self.orgB, - rel_type=TaxonomyOrg.RelType.OWNER, - ) + # OrgA taxonomy + 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) + TaxonomyOrg.objects.create( + taxonomy=self.tA2, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + + # OrgB taxonomy + self.tB1 = Taxonomy.objects.create(name="tB1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.tB1, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.tB2 = Taxonomy.objects.create(name="tB2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.tB2, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + + # OrgA and OrgB taxonomy + self.tBA1 = Taxonomy.objects.create(name="tBA1", enabled=True) + TaxonomyOrg.objects.create( + taxonomy=self.tBA1, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.tBA1, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + self.tBA2 = Taxonomy.objects.create(name="tBA2", enabled=False) + TaxonomyOrg.objects.create( + taxonomy=self.tBA2, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.tBA2, + org=self.orgB, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + + def setUp(self): super().setUp() - _setUp_orgs() - _setUp_courses() - _setUp_library() - _setUp_users() - _setUp_taxonomies() + self._setUp_orgs() + self._setUp_courses() + self._setUp_library() + self._setUp_users() + self._setUp_taxonomies() @skip_unless_cms diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 6512962ce27b..97a68467b2e0 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -94,7 +94,8 @@ def get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Orga def get_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: """ - Return a list of orgs that the given user is a member of (instructor or content creator), from the given list of orgs. + Return a list of orgs that the given user is a member of (instructor or content creator), + from the given list of orgs. """ content_creator_orgs = get_content_creator_orgs(user, orgs) instructor_orgs = get_instructor_orgs(user, orgs) From 334822db2d81ee2f3763d47282749a86b869856a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sun, 15 Oct 2023 18:40:33 -0300 Subject: [PATCH 17/30] style: fix pylint --- openedx/core/djangoapps/content_tagging/api.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 9fddc109d7ce..8e1019208771 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -20,7 +20,7 @@ def create_taxonomy( enabled=True, allow_multiple=False, allow_free_text=False, - orgs: list[Organization] = [], + orgs: list[Organization] | None = None, ) -> Taxonomy: """ Creates, saves, and returns a new Taxonomy with the given attributes. @@ -33,7 +33,8 @@ def create_taxonomy( allow_free_text=allow_free_text, ) - set_taxonomy_orgs(taxonomy=taxonomy, all_orgs=False, orgs=orgs) + if orgs is not None: + set_taxonomy_orgs(taxonomy=taxonomy, all_orgs=False, orgs=orgs) return taxonomy From 990039ca695517eefab84f23c918a796c9a5e73a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sun, 15 Oct 2023 18:42:44 -0300 Subject: [PATCH 18/30] fix: override export_taxonomy rule --- openedx/core/djangoapps/content_tagging/rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 97a68467b2e0..82f06681c267 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -279,7 +279,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) 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.export_taxonomy", oel_tagging.can_view_taxonomy) +rules.set_perm("oel_tagging.export_taxonomy", can_view_taxonomy) # Tag rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) From 7c9a90794d0bd947342f3c03269e2480fab70141 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sun, 15 Oct 2023 18:53:22 -0300 Subject: [PATCH 19/30] fix: update rule use --- openedx/core/djangoapps/content_tagging/rules.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 82f06681c267..eb47f27124cc 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -230,6 +230,9 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) +@rules.predicate +def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: + return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) @rules.predicate def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: @@ -291,10 +294,11 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) rules.set_perm("oel_tagging.add_object_tag", oel_tagging.can_change_object_tag) rules.set_perm("oel_tagging.change_objecttag", oel_tagging.can_change_object_tag) rules.set_perm("oel_tagging.delete_objecttag", oel_tagging.can_change_object_tag) -rules.set_perm("oel_tagging.view_objecttag", rules.always_allow) +rules.set_perm("oel_tagging.view_objecttag", oel_tagging.can_view_object_tag) # 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.view_objecttag_taxonomy", can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) 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 6a04d1b6e803c03e94617e8e0c17d043e1ab97c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sun, 15 Oct 2023 19:12:33 -0300 Subject: [PATCH 20/30] style: fix pylint --- openedx/core/djangoapps/content_tagging/rules.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index eb47f27124cc..2ce098a5b914 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -230,10 +230,12 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) + @rules.predicate def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) + @rules.predicate def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: """ From 8c329be83f478ce0ceccd824138e95f8d1736c5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sun, 15 Oct 2023 20:25:03 -0300 Subject: [PATCH 21/30] test: fix test_rules --- .../content_tagging/tests/test_rules.py | 39 ++++++++++++------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 9e1c60dfffd7..6c904d3f4623 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -580,22 +580,35 @@ def test_change_object_tag_org1(self, perm, tag_attr): assert not self.learner.has_perm(perm, perm_item) @ddt.data( - "all_orgs_course_tag", - "all_orgs_block_tag", - "both_orgs_course_tag", - "both_orgs_block_tag", - "one_org_block_tag", - "disabled_course_tag", + "tax_all_course1", + "tax_all_course2", + "tax_all_xblock1", + "tax_all_xblock2", + "tax_both_course1", + "tax_both_course2", + "tax_both_xblock1", + "tax_both_xblock2", ) def test_view_object_tag(self, tag_attr): """Anyone can view any ObjectTag""" - object_tag = getattr(self, tag_attr) - self._expected_users_have_perm( - "oel_tagging.view_objecttag", - object_tag, - learner_perm=True, - learner_obj=True, - ) + perm = "oel_tagging.view_objecttag" + 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_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) + + def test_view_object_tag_diabled(self): + """ + Noboty can view a ObjectTag from a disable taxonomy + """ + perm = "oel_tagging.view_objecttag" + assert self.superuser.has_perm(perm, self.disabled_course_tag) + assert not self.staff.has_perm(perm, self.disabled_course_tag) + assert not self.user_both_orgs.has_perm(perm, self.disabled_course_tag) + assert not self.user_org2.has_perm(perm, self.disabled_course_tag) + assert not self.learner.has_perm(perm, self.disabled_course_tag) @ddt.ddt From 9695d67a30b0222a430226b4c8dab2c4a135df4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 14:19:40 -0300 Subject: [PATCH 22/30] test: add get objet tags test --- .../content_tagging/rest_api/v1/tests/test_views.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 56236fbbabb6..1f060b2241c9 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -1129,6 +1129,9 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): Testing various cases for the ObjectTagView. """ + def test_get_tags(self): + pass + @ddt.data( # userA and userS are staff in courseA and can tag using enabled taxonomies ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), @@ -1163,6 +1166,11 @@ def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status) assert len(response.data) == len(tag_values) assert set(t["value"] for t in response.data) == set(tag_values) + # Check that re-fetching the tags returns what we set + response = self.client.get(url, format="json") + assert status.is_success(response.status_code) + assert set(t["value"] for t in response.data) == set(tag_values) + @ddt.data( "staffA", "staff", @@ -1236,6 +1244,11 @@ def test_tag_xblock(self, user_attr, taxonomy_attr, tag_values, expected_status) assert len(response.data) == len(tag_values) assert set(t["value"] for t in response.data) == set(tag_values) + # Check that re-fetching the tags returns what we set + response = self.client.get(url, format="json") + assert status.is_success(response.status_code) + assert set(t["value"] for t in response.data) == set(tag_values) + @ddt.data( "staffA", "staff", From 381f4dfc987cdadd36a04012267f5c60684189a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 15:46:00 -0300 Subject: [PATCH 23/30] refactor: remove can_change_object_tag_taxonomy --- .../core/djangoapps/content_tagging/rules.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 2ce098a5b914..71ab272eb0ed 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -231,11 +231,6 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) -@rules.predicate -def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: - return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) - - @rules.predicate def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: """ @@ -254,15 +249,6 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_read_access(user, course_key) -@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 taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) - - @rules.predicate def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) -> bool: """ @@ -300,7 +286,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) # 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.view_objecttag_taxonomy", can_view_object_tag_taxonomy) +rules.set_perm("oel_tagging.view_objecttag_taxonomy", oel_tagging.can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) -rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_change_object_tag_taxonomy) +rules.set_perm("oel_tagging.change_objecttag_taxonomy", oel_tagging.can_change_object_tag_taxonomy) rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From 575db344d3fa12a8e1dd2387fb6f51157448e285 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 15:57:31 -0300 Subject: [PATCH 24/30] fix: wrong permission name --- openedx/core/djangoapps/content_tagging/rules.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 71ab272eb0ed..1782eea3cbbc 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -288,5 +288,5 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) # to include Organization and objects permissions. rules.set_perm("oel_tagging.view_objecttag_taxonomy", oel_tagging.can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) -rules.set_perm("oel_tagging.change_objecttag_taxonomy", oel_tagging.can_change_object_tag_taxonomy) +rules.set_perm("oel_tagging.change_objecttag_taxonomy", oel_tagging.can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From 992a28c68297020a707358d1cf6c833ddedb2d05 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:25:44 -0300 Subject: [PATCH 25/30] fix: override can_view_object_tag_taxonomy rule --- openedx/core/djangoapps/content_tagging/rules.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 1782eea3cbbc..5206f6204534 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -231,6 +231,17 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_write_access(user, course_key) +@rules.predicate +def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: + """ + Only enabled taxonomy and users with permission to view this taxonomy can view object tags + from that taxonomy. + + This rule is different from can_view_taxonomy because it checks if the taxonomy is enabled. + """ + return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) + + @rules.predicate def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: """ @@ -286,7 +297,7 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) # 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.view_objecttag_taxonomy", oel_tagging.can_view_object_tag_taxonomy) +rules.set_perm("oel_tagging.view_objecttag_taxonomy", can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) -rules.set_perm("oel_tagging.change_objecttag_taxonomy", oel_tagging.can_view_object_tag_taxonomy) +rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_view_object_tag_taxonomy) rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) From 10c34b474f453c2814ecd0d61d5d569ef690e434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:49:21 -0300 Subject: [PATCH 26/30] chore: update 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, 5 insertions(+), 6 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index d13f7ddc0f48..e9e6f1a3df49 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -785,7 +785,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies +openedx-learning==0.2.5 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 1a9018694265..9a04788b5647 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1318,7 +1318,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/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies +openedx-learning==0.2.5 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 995b56a58a17..ea7cb693c2ed 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -925,7 +925,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies +openedx-learning==0.2.5 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 327d50b5495d..1c727d2f9c2c 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -117,8 +117,7 @@ openedx-calc # Library supporting mathematical calculatio openedx-django-require openedx-events # 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/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies +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 5e71eb8bcd85..4c142c7bc54e 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -992,7 +992,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning@rpenido/fal-3518-permissions-for-taxonomies +openedx-learning==0.2.5 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 9f24e32295f58f17ed35d703202bc9f7f0e9912d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:59:20 -0300 Subject: [PATCH 27/30] refactor: add type guards --- .../rest_api/v1/tests/test_views.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 1f060b2241c9..1956b13a7484 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -755,6 +755,10 @@ def _test_api_call(self, **kwargs) -> None: expected_status = kwargs.get("expected_status") reason = kwargs.get("reason", "Unexpected response status") + assert taxonomy_attr is not None, "taxonomy_attr is required" + assert user_attr is not None, "user_attr is required" + assert expected_status is not None, "expected_status is required" + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -941,6 +945,10 @@ def _test_api_call(self, **kwargs) -> None: expected_status = kwargs.get("expected_status") reason = kwargs.get("reason", "Unexpected response status") + assert taxonomy_attr is not None, "taxonomy_attr is required" + assert user_attr is not None, "user_attr is required" + assert expected_status is not None, "expected_status is required" + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -978,6 +986,10 @@ def _test_api_call(self, **kwargs) -> None: expected_status = kwargs.get("expected_status") reason = kwargs.get("reason", "Unexpected response status") + assert taxonomy_attr is not None, "taxonomy_attr is required" + assert user_attr is not None, "user_attr is required" + assert expected_status is not None, "expected_status is required" + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) @@ -1015,6 +1027,10 @@ def _test_api_call(self, **kwargs) -> None: expected_status = kwargs.get("expected_status") reason = kwargs.get("reason", "Unexpected response status") + assert taxonomy_attr is not None, "taxonomy_attr is required" + assert user_attr is not None, "user_attr is required" + assert expected_status is not None, "expected_status is required" + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) From 5398ee76c396781e8212c717949255e8de211b01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 17:06:28 -0300 Subject: [PATCH 28/30] fix: add type guards --- .../content_tagging/rest_api/v1/tests/test_views.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 1956b13a7484..908540ac59f1 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -789,6 +789,10 @@ def _test_api_call(self, **kwargs) -> None: expected_status = kwargs.get("expected_status") reason = kwargs.get("reason", "Unexpected response status") + assert taxonomy_attr is not None, "taxonomy_attr is required" + assert user_attr is not None, "user_attr is required" + assert expected_status is not None, "expected_status is required" + taxonomy = getattr(self, taxonomy_attr) url = TAXONOMY_ORG_DETAIL_URL.format(pk=taxonomy.pk) From 66158a75acd923f72d740cedcbd30a9ad150b127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 17 Oct 2023 14:58:14 -0300 Subject: [PATCH 29/30] docs: fix docstring --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 908540ac59f1..f988b5542cd4 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -729,7 +729,7 @@ def test_detail_taxonomy_dont_see_other_org(self, user_attr: str) -> None: ) def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: """ - Tests that org users can't see taxonomies from other orgs + Tests that staff can see all taxonomies """ self._test_api_call( user_attr="staff", From 217cedb79f6fb30416c446328dd5528ab51cb4a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 18 Oct 2023 11:26:06 -0300 Subject: [PATCH 30/30] test: remove ENABLE_CREATOR_GROUP tests and fix some docstrings --- .../rest_api/v1/tests/test_views.py | 55 ++--------------- .../content_tagging/tests/test_rules.py | 61 +------------------ 2 files changed, 5 insertions(+), 111 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index f988b5542cd4..d37b5df26c2a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -9,7 +9,6 @@ import abc import ddt from django.contrib.auth import get_user_model -from django.test import override_settings 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 @@ -266,10 +265,9 @@ def setUp(self): @skip_unless_cms @ddt.ddt -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyListCreateViewSet(TestTaxonomyObjectsMixin, APITestCase): """ - Test cases for TestTaxonomyReadViewSet when ENABLE_CREATOR_GROUP is True + Test cases for TaxonomyViewSet for list and create actions """ def _test_list_taxonomy( @@ -740,7 +738,6 @@ def test_detail_taxonomy_staff_see_all(self, taxonomy_attr: str) -> None: @skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyDetailViewSet(TestTaxonomyDetailExportMixin, APITestCase): """ Test cases for TaxonomyViewSet with detail action @@ -774,7 +771,6 @@ def _test_api_call(self, **kwargs) -> None: @skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyExportViewSet(TestTaxonomyDetailExportMixin, APITestCase): """ Test cases for TaxonomyViewSet with export action @@ -937,10 +933,9 @@ def test_staff_cant_edit_system_defined_taxonomies(self, taxonomy_attr: str) -> @skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyUpdateViewSet(TestTaxonomyChangeMixin, APITestCase): """ - Test cases for TaxonomyChangeViewSet with PUT method + Test cases for TaxonomyViewSet with PUT method """ def _test_api_call(self, **kwargs) -> None: @@ -978,10 +973,9 @@ def _test_api_call(self, **kwargs) -> None: @skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyPatchViewSet(TestTaxonomyChangeMixin, APITestCase): """ - Test cases for TaxonomyChangeViewSet with PATCH method + Test cases for TaxonomyViewSet with PATCH method """ def _test_api_call(self, **kwargs) -> None: @@ -1019,10 +1013,9 @@ def _test_api_call(self, **kwargs) -> None: @skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestTaxonomyDeleteViewSet(TestTaxonomyChangeMixin, APITestCase): """ - Test cases for TaxonomyChangeViewSet with DELETE method + Test cases for TaxonomyViewSet with DELETE method """ def _test_api_call(self, **kwargs) -> None: @@ -1051,46 +1044,6 @@ def _test_api_call(self, **kwargs) -> None: assert response.status_code == status.HTTP_404_NOT_FOUND -@skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyReadViewSetNoCreatorGroup(TestTaxonomyListCreateViewSet): # pylint: disable=test-inherits-tests - """ - Test cases for TaxonomyReadViewSet when ENABLE_CREATOR_GROUP is False - - The permissions are the same for when ENABLED_CREATOR_GRUP is True - """ - - -@skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyUpdateViewSetNoCreatorGroup(TestTaxonomyUpdateViewSet): # pylint: disable=test-inherits-tests - """ - Test cases for TaxonomyUpdateViewSet when ENABLE_CREATOR_GROUP is False - - The permissions are the same for when ENABLED_CREATOR_GRUP is True - """ - - -@skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyPatchViewSetNoCreatorGroup(TestTaxonomyUpdateViewSet): # pylint: disable=test-inherits-tests - """ - Test cases for TaxonomyPatchViewSet when ENABLE_CREATOR_GROUP is False - - The permissions are the same for when ENABLED_CREATOR_GRUP is True - """ - - -@skip_unless_cms -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestTaxonomyDeleteViewSetNoCreatorGroup(TestTaxonomyPatchViewSet): # pylint: disable=test-inherits-tests - """ - Test cases for TaxonomyDeleteViewSet when ENABLE_CREATOR_GROUP is False - - The permissions are the same for when ENABLED_CREATOR_GRUP is True - """ - - class TestObjectTagMixin(TestTaxonomyObjectsMixin): """ Sets up data for testing ObjectTags. diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 09e3db7f0f9b..9c1187ab9137 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -2,14 +2,13 @@ import ddt from django.contrib.auth import get_user_model -from django.test import TestCase, override_settings +from django.test import TestCase from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx_tagging.core.tagging.models import ( Tag, UserSystemDefinedTaxonomy, ) from openedx_tagging.core.tagging.rules import ObjectTagPermissionItem -from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role from common.djangoapps.student.roles import CourseStaffRole, OrgStaffRole @@ -21,7 +20,6 @@ @ddt.ddt -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): """ Tests that the expected rules have been applied to the Taxonomy models. @@ -609,60 +607,3 @@ def test_view_object_tag_diabled(self): assert not self.user_both_orgs.has_perm(perm, self.disabled_course_tag) assert not self.user_org2.has_perm(perm, self.disabled_course_tag) assert not self.learner.has_perm(perm, self.disabled_course_tag) - - -@ddt.ddt -@override_settings(FEATURES={"ENABLE_CREATOR_GROUP": False}) -class TestRulesTaxonomyNoCreatorGroup( - TestRulesTaxonomy -): # pylint: disable=test-inherits-tests - """ - Run the above tests with ENABLE_CREATOR_GROUP unset, to demonstrate that all users have course creator access for - all orgs, and therefore everyone is a Taxonomy Administrator. - - However, if there are no Organizations in the database, then nobody has access to the Tagging models. - """ - - def _expected_users_have_perm( - self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True - ): - """ - When ENABLE_CREATOR_GROUP is disabled, all users have all permissions. - """ - super()._expected_users_have_perm( - perm=perm, - obj=obj, - learner_perm=learner_perm, - learner_obj=learner_obj, - user_org2=user_org2, - ) - - # Taxonomy - - @ddt.data( - ("oel_tagging.change_taxonomy", "taxonomy_all_orgs"), - ("oel_tagging.change_taxonomy", "taxonomy_both_orgs"), - ("oel_tagging.change_taxonomy", "taxonomy_disabled"), - ("oel_tagging.change_taxonomy", "taxonomy_one_org"), - ("oel_tagging.change_taxonomy", "taxonomy_no_orgs"), - ("oel_tagging.delete_taxonomy", "taxonomy_all_orgs"), - ("oel_tagging.delete_taxonomy", "taxonomy_both_orgs"), - ("oel_tagging.delete_taxonomy", "taxonomy_disabled"), - ("oel_tagging.delete_taxonomy", "taxonomy_one_org"), - ("oel_tagging.delete_taxonomy", "taxonomy_no_orgs"), - ) - @ddt.unpack - def test_no_orgs_no_perms(self, perm, taxonomy_attr): - """ - Org-level permissions are revoked when there are no orgs. - """ - Organization.objects.all().delete() - taxonomy = getattr(self, taxonomy_attr) - # Superusers & Staff always have access - assert self.superuser.has_perm(perm, taxonomy) - assert self.staff.has_perm(perm, taxonomy) - - # But everyone else's object-level access is removed - assert not self.user_both_orgs.has_perm(perm, taxonomy) - assert not self.user_org2.has_perm(perm, taxonomy) - assert not self.learner.has_perm(perm, taxonomy)