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.

Sign up for GitHub

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

fix: Update ObjectTagView and ObjectTagCountsView to accept complex objectIds #246

Merged
merged 4 commits into from
Nov 21, 2024
Merged

fix: Update ObjectTagView and ObjectTagCountsView to accept complex objectIds #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2d7b0ff
fix: accept complex objectId e.g; dot(.) in objectId
marslanabdulrauf Oct 21, 2024
27ea5f9
test: ObjectTagView and ObjectTagCountView tests updated with complex…
marslanabdulrauf Nov 15, 2024
59b992e
test: Fix quality checks
marslanabdulrauf Nov 18, 2024
9de4145
docs: Bump version to 0.18.2
marslanabdulrauf Nov 21, 2024
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
2 changes: 1 addition & 1 deletion openedx_learning/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Open edX Learning ("Learning Core").
"""

__version__ = "0.18.0"
__version__ = "0.18.1"
2 changes: 2 additions & 0 deletions openedx_tagging/core/tagging/rest_api/v1/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ class ObjectTagView(
minimal_serializer_class = ObjectTagMinimalSerializer
permission_classes = [ObjectTagObjectPermissions]
lookup_field = "object_id"
lookup_value_regex = r'[\w\.\+\-@:]+'

def get_queryset(self) -> models.QuerySet:
"""
Expand Down Expand Up @@ -619,6 +620,7 @@ class ObjectTagCountsView(

serializer_class = ObjectTagSerializer
lookup_field = "object_id_pattern"
lookup_value_regex = r'[\w\.\+\-@:*,]+'

def retrieve(self, request, *args, **kwargs) -> Response:
"""
Expand Down
142 changes: 76 additions & 66 deletions tests/openedx_tagging/core/tagging/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,9 @@ def _change_object_permission(user, object_id: str) -> bool:
"""
For testing, let everyone have edit object permission on object_id "abc" and "limit_tag_count"
"""
if object_id in ("abc", "limit_tag_count", "problem7", "problem15", "html7"):
basic_object_ids = ("abc", "limit_tag_count", "problem7", "problem15", "html7")
complex_object_ids = ("abc.xyz", "limit_tag_count1.1", "problem7.2", "problem1.5", "html7.3")
if object_id in basic_object_ids or object_id in complex_object_ids:
return True

return can_change_object_tag_objectid(user, object_id)
Expand All @@ -726,16 +728,15 @@ def _view_object_permission(user, object_id: str) -> bool:
self.taxonomy.save()

@ddt.data(
(None, status.HTTP_401_UNAUTHORIZED),
("user_1", status.HTTP_200_OK),
("staff", status.HTTP_200_OK),
(None, status.HTTP_401_UNAUTHORIZED, 'problem15'),
("user_1", status.HTTP_200_OK, 'problem1.5'),
("staff", status.HTTP_200_OK, 'problem1.5'),
)
@ddt.unpack
def test_retrieve_object_tags(self, user_attr, expected_status):
def test_retrieve_object_tags(self, user_attr, expected_status, object_id):
"""
Test retrieving object tags
"""
object_id = "problem15"

# Apply the object tags that we're about to retrieve:
api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"])
Expand All @@ -755,7 +756,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status):
assert response.data == {
# In the future, this API may allow retrieving tags for multiple objects at once, so it's grouped by
# object ID.
"problem15": {
object_id: {
"taxonomies": [
{
"name": "Life on Earth",
Expand Down Expand Up @@ -798,7 +799,7 @@ def prepare_for_sort_test(self, expected_perm=False) -> tuple[str, list[dict]]:
"""
Tag an object with tags from the "sort test" taxonomy
"""
object_id = "problem7"
object_id = "problem7.2"
# Some selected tags to use, from the taxonomy create by self.create_sort_test_taxonomy()
sort_test_tags = [
"ANVIL",
Expand Down Expand Up @@ -897,18 +898,17 @@ def test_retrieve_object_tags_unauthorized(self):
assert response.status_code == status.HTTP_403_FORBIDDEN

@ddt.data(
(None, status.HTTP_401_UNAUTHORIZED),
("user_1", status.HTTP_200_OK),
("staff", status.HTTP_200_OK),
(None, status.HTTP_401_UNAUTHORIZED, 'html7.3'),
("user_1", status.HTTP_200_OK, 'html7'),
("staff", status.HTTP_200_OK, 'html7.3'),
)
@ddt.unpack
def test_retrieve_object_tags_taxonomy_queryparam(
self, user_attr, expected_status,
self, user_attr, expected_status, object_id
):
"""
Test retrieving object tags for specific taxonomies provided
"""
object_id = "html7"

# Apply the object tags that we're about to retrieve:
api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"])
Expand Down Expand Up @@ -947,9 +947,9 @@ def test_retrieve_object_tags_taxonomy_queryparam(
}

@ddt.data(
(None, "abc", status.HTTP_401_UNAUTHORIZED),
(None, "abc.xyz", status.HTTP_401_UNAUTHORIZED),
("user_1", "abc", status.HTTP_400_BAD_REQUEST),
("staff", "abc", status.HTTP_400_BAD_REQUEST),
("staff", "abc.xyz", status.HTTP_400_BAD_REQUEST),
)
@ddt.unpack
def test_retrieve_object_tags_invalid_taxonomy_queryparam(self, user_attr, object_id, expected_status):
Expand All @@ -967,30 +967,31 @@ def test_retrieve_object_tags_invalid_taxonomy_queryparam(self, user_attr, objec
assert response.status_code == expected_status

@ddt.data(
(None, "POST", status.HTTP_401_UNAUTHORIZED),
(None, "PATCH", status.HTTP_401_UNAUTHORIZED),
(None, "DELETE", status.HTTP_401_UNAUTHORIZED),
("user_1", "POST", status.HTTP_405_METHOD_NOT_ALLOWED),
("user_1", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED),
("user_1", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED),
("staff", "POST", status.HTTP_405_METHOD_NOT_ALLOWED),
("staff", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED),
("staff", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED),
(None, "POST", status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
(None, "PATCH", status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
(None, "DELETE", status.HTTP_401_UNAUTHORIZED, "abc"),
("user_1", "POST", status.HTTP_405_METHOD_NOT_ALLOWED, "abc"),
("user_1", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED, "abc"),
("user_1", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED, "abc.xyz"),
("staff", "POST", status.HTTP_405_METHOD_NOT_ALLOWED, "abc.xyz"),
("staff", "PATCH", status.HTTP_405_METHOD_NOT_ALLOWED, "abc.xyz"),
("staff", "DELETE", status.HTTP_405_METHOD_NOT_ALLOWED, "abc"),
)
@ddt.unpack
def test_object_tags_remaining_http_methods(
self,
user_attr,
http_method,
expected_status,
object_id
):
"""
Test POST/PATCH/DELETE method for ObjectTagView

Only staff users should have permissions to perform the actions,
however the methods are currently not allowed.
"""
url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="abc")
url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id)

if user_attr:
user = getattr(self, user_attr)
Expand All @@ -1008,36 +1009,39 @@ def test_object_tags_remaining_http_methods(

@ddt.data(
# Users and staff can add tags
(None, "language_taxonomy", {}, ["Portuguese"], status.HTTP_401_UNAUTHORIZED),
("user_1", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK),
("staff", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK),
(None, "language_taxonomy", {}, ["Portuguese"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
("user_1", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK, "abc"),
("staff", "language_taxonomy", {}, ["Portuguese"], status.HTTP_200_OK, "abc.xyz"),
# user_1s and staff can clear add tags
(None, "taxonomy", {}, ["Fungi"], status.HTTP_401_UNAUTHORIZED),
("user_1", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK),
("staff", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK),
(None, "taxonomy", {}, ["Fungi"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
("user_1", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK, "abc.xyz"),
("staff", "taxonomy", {}, ["Fungi"], status.HTTP_200_OK, "abc.xyz"),
# Nobody can add tag using a disabled taxonomy
(None, "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_401_UNAUTHORIZED),
("user_1", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN),
("staff", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN),
(None, "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
("user_1", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN, "abc"),
("staff", "taxonomy", {"enabled": False}, ["Fungi"], status.HTTP_403_FORBIDDEN, "abc.xyz"),
# If allow_multiple=True, multiple tags can be added, but not if it's false:
("user_1", "taxonomy", {"allow_multiple": True}, ["Mammalia", "Fungi"], status.HTTP_200_OK),
("user_1", "taxonomy", {"allow_multiple": False}, ["Mammalia", "Fungi"], status.HTTP_400_BAD_REQUEST),
("user_1", "taxonomy", {"allow_multiple": True}, ["Mammalia", "Fungi"], status.HTTP_200_OK, "abc.xyz"),
(
"user_1", "taxonomy", {"allow_multiple": False},
["Mammalia", "Fungi"], status.HTTP_400_BAD_REQUEST, "abc.xyz"
),
# user_1s and staff can add tags using an open taxonomy
(None, "free_text_taxonomy", {}, ["tag1"], status.HTTP_401_UNAUTHORIZED),
("user_1", "free_text_taxonomy", {}, ["tag1", "tag2"], status.HTTP_200_OK),
("staff", "free_text_taxonomy", {}, ["tag1", "tag4"], status.HTTP_200_OK),
(None, "free_text_taxonomy", {}, ["tag1"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
("user_1", "free_text_taxonomy", {}, ["tag1", "tag2"], status.HTTP_200_OK, "abc"),
("staff", "free_text_taxonomy", {}, ["tag1", "tag4"], status.HTTP_200_OK, "abc.xyz"),
# Nobody can add tags using a disabled open taxonomy
(None, "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_401_UNAUTHORIZED),
("user_1", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN),
("staff", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN),
(None, "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_401_UNAUTHORIZED, "abc.xyz"),
("user_1", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN, "abc.xyz"),
("staff", "free_text_taxonomy", {"enabled": False}, ["tag1"], status.HTTP_403_FORBIDDEN, "abc"),
# Can't add invalid/nonexistent tags using a closed taxonomy
(None, "language_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED),
("user_1", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST),
("staff", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST),
("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST),
(None, "language_taxonomy", {}, ["Invalid"], status.HTTP_401_UNAUTHORIZED, "abc"),
("user_1", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"),
("staff", "language_taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc"),
("staff", "taxonomy", {}, ["Invalid"], status.HTTP_400_BAD_REQUEST, "abc.xyz"),
)
@ddt.unpack
def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status):
def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values, expected_status, object_id):
if user_attr:
user = getattr(self, user_attr)
self.client.force_authenticate(user=user)
Expand All @@ -1048,8 +1052,6 @@ def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values,
setattr(taxonomy, k, v)
taxonomy.save()

object_id = "abc"

url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id)

data = [{
Expand All @@ -1064,10 +1066,14 @@ def test_tag_object(self, user_attr, taxonomy_attr, taxonomy_flags, tag_values,
# And retrieving the object tags again should return an identical response:
assert response.data == self.client.get(OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id)).data

def test_tag_object_multiple_taxonomy(self):
@ddt.data(
("abc",),
("abc.xyz",),
)
@ddt.unpack
def test_tag_object_multiple_taxonomy(self, object_id):
self.client.force_authenticate(user=self.staff)

object_id = "abc"
url = OBJECT_TAGS_UPDATE_URL.format(object_id=object_id)
tag_value_1 = ["Tag 4"]
tag_value_2 = ["Mammalia", "Fungi"]
Expand Down Expand Up @@ -1251,12 +1257,13 @@ def test_get_counts(self):
# Course 2
api.tag_object(object_id="course02-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["other"])
# Course 7 Unit 1
api.tag_object(object_id="course07-unit01-problem01", taxonomy=self.free_text_taxonomy, tags=["a", "b", "c"])
api.tag_object(object_id="course07-unit01-problem02", taxonomy=self.free_text_taxonomy, tags=["a", "b"])
api.tag_object(object_id="course07-unit01-problem01.1", taxonomy=self.free_text_taxonomy, tags=["a", "b", "c"])
api.tag_object(object_id="course07-unit01-problem02.2", taxonomy=self.free_text_taxonomy, tags=["a", "b"])
# Course 7 Unit 2
api.tag_object(object_id="course07-unit02-problem01", taxonomy=self.free_text_taxonomy, tags=["b"])
api.tag_object(object_id="course07-unit02-problem02", taxonomy=self.free_text_taxonomy, tags=["c", "d"])
api.tag_object(object_id="course07-unit02-problem02.2", taxonomy=self.free_text_taxonomy, tags=["c", "d"])
api.tag_object(object_id="course07-unit02-problem03", taxonomy=self.free_text_taxonomy, tags=["N", "M"])
api.tag_object(object_id="course07-unit02-problem03.3", taxonomy=self.free_text_taxonomy, tags=["N", "M"])
api.tag_object(object_id="course07-unit02-problem03", taxonomy=self.taxonomy, tags=["Mammalia"])

def check(object_id_pattern: str, count_implicit=False):
Expand All @@ -1273,40 +1280,43 @@ def check(object_id_pattern: str, count_implicit=False):
}
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit01-*") == {
"course07-unit01-problem01": 3,
"course07-unit01-problem02": 2,
"course07-unit01-problem01.1": 3,
"course07-unit01-problem02.2": 2,
}
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit02-*") == {
"course07-unit02-problem01": 1,
"course07-unit02-problem02": 2,
"course07-unit02-problem02.2": 2,
"course07-unit02-problem03": 3,
"course07-unit02-problem03.3": 2,
}
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit02-*", count_implicit=True) == {
"course07-unit02-problem01": 1,
"course07-unit02-problem02": 2,
"course07-unit02-problem02.2": 2,
"course07-unit02-problem03.3": 2,
# "Mammalia" includes 1 explicit + 3 implicit tags: "Eukaryota > Animalia > Chordata > Mammalia"
# so problem03 has 2 free text tags and "4" life on earth tags:
"course07-unit02-problem03": 6,
}
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit*") == {
"course07-unit01-problem01": 3,
"course07-unit01-problem02": 2,
"course07-unit01-problem01.1": 3,
"course07-unit01-problem02.2": 2,
"course07-unit02-problem01": 1,
"course07-unit02-problem02": 2,
"course07-unit02-problem02.2": 2,
"course07-unit02-problem03": 3,
"course07-unit02-problem03.3": 2,
}
# Can also use a comma to separate explicit object IDs:
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit01-problem01") == {
"course07-unit01-problem01": 3,
assert check(object_id_pattern="course07-unit01-problem01.1") == {
"course07-unit01-problem01.1": 3,
}
with self.assertNumQueries(1):
assert check(object_id_pattern="course07-unit01-problem01,course07-unit02-problem02") == {
"course07-unit01-problem01": 3,
"course07-unit02-problem02": 2,
assert check(object_id_pattern="course07-unit01-problem01.1,course07-unit02-problem02.2") == {
"course07-unit01-problem01.1": 3,
"course07-unit02-problem02.2": 2,
}

def test_get_counts_invalid_spec(self):
Expand Down