-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
refactor: remove ContentObjectTag model and related functions #34146
Merged
bradenmacdonald
merged 13 commits into
openedx:master
from
open-craft:rpenido/fal-3610-content-tag-cleanup
Feb 8, 2024
+318
−175
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
d72ae83
refactor: remove ContentObjectTag model and related functions
rpenido 739fdb5
refactor: remove tag_object function and add cross-org check to rules
rpenido 82e9923
Merge branch 'master' into rpenido/fal-3610-content-tag-cleanup
rpenido 1a4c858
fix: allow taxonomy admins use no-org taxonomies
rpenido 51be568
fix: skip check if taxonomy not provided
rpenido dfb908e
refactor: add utils module to handle opaque keys
rpenido 0278ad8
test: add tests for tagging LibraryV2
rpenido 38e03ce
fix: types
rpenido 9975a78
docs: add comments
rpenido cc6e2fd
fix: pylint
rpenido 2791963
fix: add test to invalid object id tagging
rpenido 73970f3
docs: fix typo
rpenido 78b3d53
docs: fix test comments
rpenido File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 16 additions & 0 deletions
16
openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
# Generated by Django 3.2.23 on 2024-01-30 21:15 | ||
|
||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('content_tagging', '0007_system_defined_org_2'), | ||
] | ||
|
||
operations = [ | ||
migrations.DeleteModel( | ||
name='ContentObjectTag', | ||
), | ||
] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,5 +3,4 @@ | |
""" | ||
from .base import ( | ||
TaxonomyOrg, | ||
ContentObjectTag, | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,7 @@ def _setUp_library(self): | |
title="Library Org A", | ||
description="This is a library from Org A", | ||
) | ||
self.libraryA = str(self.content_libraryA.key) | ||
|
||
def _setUp_users(self): | ||
""" | ||
|
@@ -125,10 +126,16 @@ def _setUp_users(self): | |
|
||
self.staffA = User.objects.create( | ||
username="staffA", | ||
email="userA@example.com", | ||
email="staffA@example.com", | ||
) | ||
update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) | ||
|
||
self.staffB = User.objects.create( | ||
username="staffB", | ||
email="[email protected]", | ||
) | ||
update_org_role(self.staff, OrgStaffRole, self.staffB, [self.orgB.short_name]) | ||
|
||
self.content_creatorA = User.objects.create( | ||
username="content_creatorA", | ||
email="[email protected]", | ||
|
@@ -1366,7 +1373,7 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): | |
""" | ||
|
||
@ddt.data( | ||
# userA and userS are staff in courseA and can tag using enabled taxonomies | ||
# staffA and staff are staff in courseA 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), | ||
|
@@ -1451,7 +1458,7 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr): | |
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 any taxonomies | ||
# staffA and staff 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), | ||
|
@@ -1534,6 +1541,129 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): | |
response = self.client.put(url, {"tags": ["invalid"]}, format="json") | ||
assert response.status_code == status.HTTP_400_BAD_REQUEST | ||
|
||
@ddt.data( | ||
# staffA and staff are staff in libraryA 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_library(self, user_attr, taxonomy_attr, tag_values, expected_status): | ||
""" | ||
Tests that only staff and org level users can tag libraries | ||
""" | ||
user = getattr(self, user_attr) | ||
self.client.force_authenticate(user=user) | ||
|
||
taxonomy = getattr(self, taxonomy_attr) | ||
|
||
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, taxonomy_id=taxonomy.pk) | ||
|
||
response = self.client.put(url, {"tags": tag_values}, format="json") | ||
|
||
assert response.status_code == expected_status | ||
if status.is_success(expected_status): | ||
tags_by_taxonomy = response.data[str(self.libraryA)]["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 | ||
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_library_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 | ||
|
||
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, 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_library_invalid(self, user_attr, taxonomy_attr): | ||
""" | ||
Tests that nobody can add invalid tags to a library using a closed taxonomy | ||
""" | ||
user = getattr(self, user_attr) | ||
self.client.force_authenticate(user=user) | ||
|
||
taxonomy = getattr(self, taxonomy_attr) | ||
|
||
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, taxonomy_id=taxonomy.pk) | ||
|
||
response = self.client.put(url, {"tags": ["invalid"]}, format="json") | ||
assert response.status_code == status.HTTP_400_BAD_REQUEST | ||
|
||
@ddt.data( | ||
("staff", status.HTTP_200_OK), | ||
("staffA", status.HTTP_403_FORBIDDEN), | ||
("staffB", status.HTTP_403_FORBIDDEN), | ||
) | ||
@ddt.unpack | ||
def test_tag_cross_org(self, user_attr, expected_status): | ||
""" | ||
Tests that only global admins can add a taxonomy from orgA to an object from orgB | ||
""" | ||
user = getattr(self, user_attr) | ||
self.client.force_authenticate(user=user) | ||
|
||
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseB, taxonomy_id=self.tA1.pk) | ||
|
||
response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") | ||
|
||
assert response.status_code == expected_status | ||
|
||
@ddt.data( | ||
("staff", status.HTTP_200_OK), | ||
("staffA", status.HTTP_403_FORBIDDEN), | ||
("staffB", status.HTTP_403_FORBIDDEN), | ||
) | ||
@ddt.unpack | ||
def test_tag_no_org(self, user_attr, expected_status): | ||
""" | ||
Tests that only global admins can add a no-org taxonomy to an object | ||
""" | ||
user = getattr(self, user_attr) | ||
self.client.force_authenticate(user=user) | ||
|
||
url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseA, taxonomy_id=self.ot1.pk) | ||
|
||
response = self.client.put(url, {"tags": []}, format="json") | ||
|
||
assert response.status_code == expected_status | ||
|
||
@ddt.data( | ||
"courseB", | ||
"xblockB", | ||
|
@@ -1567,7 +1697,21 @@ def test_tag_unauthorized(self, objectid_attr): | |
|
||
assert response.status_code == status.HTTP_401_UNAUTHORIZED | ||
|
||
def test_tag_invalid_object(self): | ||
""" | ||
Test that we cannot tag an object that is not a CouseKey, LibraryLocatorV2 or UsageKey | ||
""" | ||
url = OBJECT_TAG_UPDATE_URL.format(object_id='invalid_key', taxonomy_id=self.tA1.pk) | ||
self.client.force_authenticate(user=self.staff) | ||
|
||
response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") | ||
|
||
assert response.status_code == status.HTTP_403_FORBIDDEN | ||
|
||
def test_get_tags(self): | ||
""" | ||
Test that we can get tags for an object | ||
""" | ||
self.client.force_authenticate(user=self.staffA) | ||
taxonomy = self.multiple_taxonomy | ||
tag_values = ["Tag 1", "Tag 2"] | ||
|
@@ -1607,7 +1751,7 @@ def test_object_tags_query_count(self): | |
""" | ||
object_key = self.courseA | ||
object_id = str(object_key) | ||
tagging_api.tag_content_object(object_key=object_key, taxonomy=self.t1, tags=["anvil", "android"]) | ||
tagging_api.tag_object(object_id=object_id, taxonomy=self.t1, tags=["anvil", "android"]) | ||
expected_tags = [ | ||
{"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": True}, | ||
{"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": True}, | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no data stored with this model, right? This won't delete any actual database rows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Also, we weren't using it outside auto-tagging: our rest API uses the ObjectTag model (this is the main issue that motivated this PR)