Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tag in Collections [FC-0062] #35383

Merged
merged 14 commits into from
Sep 13, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions openedx/core/djangoapps/content_tagging/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import openedx_tagging.core.tagging.api as oel_tagging
from django.db.models import Exists, OuterRef, Q, QuerySet
from django.utils.timezone import now
from opaque_keys.edx.keys import CourseKey
from opaque_keys.edx.keys import CourseKey, LibraryCollectionKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy
from openedx_tagging.core.tagging.models.utils import TAGS_CSV_SEPARATOR
Expand Down Expand Up @@ -227,7 +227,7 @@ def generate_csv_rows(object_id, buffer) -> Iterator[str]:
"""
content_key = get_content_key_from_string(object_id)

if isinstance(content_key, UsageKey):
if isinstance(content_key, (UsageKey, LibraryCollectionKey)):
raise ValueError("The object_id must be a CourseKey or a LibraryLocatorV2.")

all_object_tags, taxonomies = get_all_object_tags(content_key)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import ddt
from django.contrib.auth import get_user_model
from django.core.files.uploadedfile import SimpleUploadedFile
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryCollectionLocator
from openedx_tagging.core.tagging.models import Tag, Taxonomy
from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy
from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer
Expand Down Expand Up @@ -111,6 +111,9 @@ def _setUp_library(self):
)
self.libraryA = str(self.content_libraryA.key)

def _setUp_collection(self):
self.collection_key = str(LibraryCollectionLocator(self.content_libraryA.key, 'test-collection'))

def _setUp_users(self):
"""
Create users for testing
Expand Down Expand Up @@ -284,6 +287,7 @@ def setUp(self):
self._setUp_library()
self._setUp_users()
self._setUp_taxonomies()
self._setUp_collection()

# Clear the rules cache in between test runs to keep query counts consistent.
rules_cache.clear()
Expand Down Expand Up @@ -1653,6 +1657,87 @@ def test_tag_library_invalid(self, user_attr, taxonomy_attr):
response = self._call_put_request(self.libraryA, taxonomy.pk, ["invalid"])
assert response.status_code == status.HTTP_400_BAD_REQUEST

@ddt.data(
# staffA and staff are staff in collection and can tag using enabled 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", "tA1", [], status.HTTP_403_FORBIDDEN),
("staffA", "tA1", [], status.HTTP_200_OK),
("staff", "tA1", [], status.HTTP_200_OK),
("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),
("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_collection(self, user_attr, taxonomy_attr, tag_values, expected_status):
"""
Tests that only staff and org level users can tag collections
"""
user = getattr(self, user_attr)
self.client.force_authenticate(user=user)

taxonomy = getattr(self, taxonomy_attr)

response = self._call_put_request(self.collection_key, taxonomy.pk, tag_values)

assert response.status_code == expected_status
if status.is_success(expected_status):
tags_by_taxonomy = response.data[str(self.collection_key)]["taxonomies"]
if tag_values:
response_taxonomy = tags_by_taxonomy[0]
assert response_taxonomy["name"] == taxonomy.name
response_tags = response_taxonomy["tags"]
assert [t["value"] for t in response_tags] == tag_values
else:
assert tags_by_taxonomy == [] # No tags are set from any taxonomy

# Check that re-fetching the tags returns what we set
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.collection_key)
new_response = self.client.get(url, format="json")
assert status.is_success(new_response.status_code)
assert new_response.data == response.data

@ddt.data(
"staffA",
"staff",
)
def test_tag_collection_disabled_taxonomy(self, user_attr):
"""
Nobody can use disabled taxonomies to tag objects
"""
user = getattr(self, user_attr)
self.client.force_authenticate(user=user)

disabled_taxonomy = self.tA2
assert disabled_taxonomy.enabled is False

response = self._call_put_request(self.collection_key, disabled_taxonomy.pk, ["Tag 1"])

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_collection_invalid(self, user_attr, taxonomy_attr):
"""
Tests that nobody can add invalid tags to a collection using a closed taxonomy
"""
user = getattr(self, user_attr)
self.client.force_authenticate(user=user)

taxonomy = getattr(self, taxonomy_attr)

response = self._call_put_request(self.collection_key, taxonomy.pk, ["invalid"])
assert response.status_code == status.HTTP_400_BAD_REQUEST

@ddt.data(
("superuser", status.HTTP_200_OK),
("staff", status.HTTP_403_FORBIDDEN),
Expand Down Expand Up @@ -1768,10 +1853,14 @@ def test_get_tags(self):
@ddt.data(
('staff', 'courseA', 8),
('staff', 'libraryA', 8),
('staff', 'collection_key', 8),
("content_creatorA", 'courseA', 11, False),
("content_creatorA", 'libraryA', 11, False),
("content_creatorA", 'collection_key', 11, False),
("library_staffA", 'libraryA', 11, False), # Library users can only view objecttags, not change them?
("library_staffA", 'collection_key', 11, False),
("library_userA", 'libraryA', 11, False),
("library_userA", 'collection_key', 11, False),
("instructorA", 'courseA', 11),
("course_instructorA", 'courseA', 11),
("course_staffA", 'courseA', 11),
Expand Down
19 changes: 18 additions & 1 deletion openedx/core/djangoapps/content_tagging/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import ddt
from django.test.testcases import TestCase
from fs.osfs import OSFS
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_tagging.core.tagging.models import ObjectTag
from organizations.models import Organization
Expand Down Expand Up @@ -380,6 +380,23 @@ def test_copy_cross_org_tags(self):
with self.assertNumQueries(31): # TODO why so high?
self._test_copy_object_tags(src_key, dst_key, expected_tags)

def test_tag_collection(self):
collection_key = LibraryCollectionKey.from_string("lib-collection:orgA:libX:1")

api.tag_object(
object_id=str(collection_key),
taxonomy=self.taxonomy_3,
tags=["Tag 3.1"],
)

with self.assertNumQueries(1):
object_tags, taxonomies = api.get_all_object_tags(collection_key)

assert object_tags == {'lib-collection:orgA:libX:1': {3: ['Tag 3.1']}}
assert taxonomies == {
self.taxonomy_3.id: self.taxonomy_3,
}


class TestExportImportTags(TaggedCourseMixin):
"""
Expand Down
4 changes: 2 additions & 2 deletions openedx/core/djangoapps/content_tagging/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@

from typing import Dict, List, Union

from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_tagging.core.tagging.models import Taxonomy

ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey]
ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey, LibraryCollectionKey]
ContextKey = Union[LibraryLocatorV2, CourseKey]

TagValuesByTaxonomyIdDict = Dict[int, List[str]]
Expand Down
16 changes: 13 additions & 3 deletions openedx/core/djangoapps/content_tagging/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from edx_django_utils.cache import RequestCache
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey, UsageKey
from opaque_keys.edx.keys import CourseKey, UsageKey, LibraryCollectionKey
from opaque_keys.edx.locator import LibraryLocatorV2
from openedx_tagging.core.tagging.models import Taxonomy
from organizations.models import Organization
Expand All @@ -26,8 +26,14 @@ def get_content_key_from_string(key_str: str) -> ContentKey:
except InvalidKeyError:
try:
return UsageKey.from_string(key_str)
except InvalidKeyError as usage_key_error:
raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error
except InvalidKeyError:
try:
return LibraryCollectionKey.from_string(key_str)
except InvalidKeyError as usage_key_error:
raise ValueError(
"object_id must be one of the following "
"keys: CourseKey, LibraryLocatorV2, UsageKey or LibCollectionKey"
) from usage_key_error


def get_context_key_from_key(content_key: ContentKey) -> ContextKey:
Expand All @@ -38,6 +44,10 @@ def get_context_key_from_key(content_key: ContentKey) -> ContextKey:
if isinstance(content_key, (CourseKey, LibraryLocatorV2)):
return content_key

# If the content key is a LibraryCollectionKey, return the LibraryLocatorV2
if isinstance(content_key, LibraryCollectionKey):
return content_key.library_key

# If the content key is a UsageKey, return the context key
context_key = content_key.context_key

Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ edx-milestones==0.6.0
# via -r requirements/edx/kernel.in
edx-name-affirmation==2.3.7
# via -r requirements/edx/kernel.in
edx-opaque-keys[django]==2.10.0
edx-opaque-keys[django] @ git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key
# via
# -r requirements/edx/kernel.in
# -r requirements/edx/paver.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/development.txt
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ edx-name-affirmation==2.3.7
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
edx-opaque-keys[django]==2.10.0
edx-opaque-keys[django] @ git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key
# via
# -r requirements/edx/doc.txt
# -r requirements/edx/testing.txt
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ edx-milestones==0.6.0
# via -r requirements/edx/base.txt
edx-name-affirmation==2.3.7
# via -r requirements/edx/base.txt
edx-opaque-keys[django]==2.10.0
edx-opaque-keys[django] @ git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key
# via
# -r requirements/edx/base.txt
# edx-bulk-grades
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/kernel.in
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ edx-event-bus-kafka>=5.6.0 # Kafka implementation of event bus
edx-event-bus-redis
edx-milestones
edx-name-affirmation
edx-opaque-keys
git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key#egg=edx-opaque-keys
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Bump opaque-keys version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

edx-organizations
edx-proctoring>=2.0.1
# using hash to support django42
Expand Down
2 changes: 1 addition & 1 deletion requirements/edx/testing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ edx-milestones==0.6.0
# via -r requirements/edx/base.txt
edx-name-affirmation==2.3.7
# via -r requirements/edx/base.txt
edx-opaque-keys[django]==2.10.0
edx-opaque-keys[django] @ git+https://github.com/open-craft/opaque-keys.git@chris/FAL-3785-collections-key
# via
# -r requirements/edx/base.txt
# edx-bulk-grades
Expand Down
Loading