-
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
feat: paste tags when pasting xblocks with tag data [FC-0049] #34270
Changes from 26 commits
8b999e1
efbcccf
4b895de
905f711
77f95cb
9ac16fa
f9c82a8
6f76a82
f1c806f
74d907a
2eb0230
dc2b973
59e37db
d44efc0
15288dd
dacba7d
24c4729
c1fed5e
2ec5afa
b98e2f8
a3b181e
e0fb8e9
46556f9
3166daf
b4f195a
0f2ac20
672f76c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,15 @@ | |
import ddt | ||
from opaque_keys.edx.keys import UsageKey | ||
from rest_framework.test import APIClient | ||
from openedx_tagging.core.tagging.models import Tag | ||
from organizations.models import Organization | ||
from xmodule.modulestore.django import contentstore, modulestore | ||
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, upload_file_to_course | ||
from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory, ToyCourseFactory | ||
|
||
from cms.djangoapps.contentstore.utils import reverse_usage_url | ||
from openedx.core.djangoapps.content_libraries import api as library_api | ||
from openedx.core.djangoapps.content_tagging import api as tagging_api | ||
|
||
CLIPBOARD_ENDPOINT = "/api/content-staging/v1/clipboard/" | ||
XBLOCK_ENDPOINT = "/xblock/" | ||
|
@@ -141,6 +143,69 @@ def test_copy_and_paste_component(self, block_args): | |
# The new block should store a reference to where it was copied from | ||
assert dest_block.copied_from_block == str(source_block.location) | ||
|
||
def test_copy_and_paste_unit_with_tags(self): | ||
""" | ||
Test copying a unit (vertical) with tags from one course into another | ||
""" | ||
course_key, client = self._setup_course() | ||
dest_course = CourseFactory.create(display_name='Destination Course') | ||
with self.store.bulk_operations(dest_course.id): | ||
dest_chapter = BlockFactory.create(parent=dest_course, category='chapter', display_name='Section') | ||
dest_sequential = BlockFactory.create(parent=dest_chapter, category='sequential', display_name='Subsection') | ||
|
||
unit_key = course_key.make_usage_key("vertical", "vertical_test") | ||
# Add tags to the unit | ||
taxonomy_all_org = tagging_api.create_taxonomy("test_taxonomy", "Test Taxonomy") | ||
|
||
tagging_api.set_taxonomy_orgs(taxonomy_all_org, all_orgs=True) | ||
Tag.objects.create(taxonomy=taxonomy_all_org, value="tag_1") | ||
Tag.objects.create(taxonomy=taxonomy_all_org, value="tag_2") | ||
tagging_api.tag_object( | ||
object_id=str(unit_key), | ||
taxonomy=taxonomy_all_org, | ||
tags=["tag_1", "tag_2"], | ||
) | ||
|
||
taxonomy_all_org_removed = tagging_api.create_taxonomy("test_taxonomy_removed", "Test Taxonomy Removed") | ||
tagging_api.set_taxonomy_orgs(taxonomy_all_org_removed, all_orgs=True) | ||
Tag.objects.create(taxonomy=taxonomy_all_org_removed, value="tag_1") | ||
Tag.objects.create(taxonomy=taxonomy_all_org_removed, value="tag_2") | ||
tagging_api.tag_object( | ||
object_id=str(unit_key), | ||
taxonomy=taxonomy_all_org_removed, | ||
tags=["tag_1", "tag_2"], | ||
) | ||
tagging_api.get_object_tags(str(unit_key)) | ||
|
||
taxonomy_no_org = tagging_api.create_taxonomy("test_taxonomy_no_org", "Test Taxonomy No Org") | ||
Tag.objects.create(taxonomy=taxonomy_no_org, value="tag_1") | ||
Tag.objects.create(taxonomy=taxonomy_no_org, value="tag_2") | ||
tagging_api.tag_object( | ||
object_id=str(unit_key), | ||
taxonomy=taxonomy_no_org, | ||
tags=["tag_1", "tag_2"], | ||
) | ||
|
||
# Copy the unit | ||
copy_response = client.post(CLIPBOARD_ENDPOINT, {"usage_key": str(unit_key)}, format="json") | ||
assert copy_response.status_code == 200 | ||
|
||
taxonomy_all_org_removed.delete() | ||
|
||
# Paste the unit | ||
paste_response = client.post(XBLOCK_ENDPOINT, { | ||
"parent_locator": str(dest_sequential.location), | ||
"staged_content": "clipboard", | ||
}, format="json") | ||
assert paste_response.status_code == 200 | ||
dest_unit_key = UsageKey.from_string(paste_response.json()["locator"]) | ||
|
||
# Only tags from the taxonomy that is associated with the dest org should be copied | ||
tags = list(tagging_api.get_object_tags(str(dest_unit_key))) | ||
assert len(tags) == 2 | ||
assert str(tags[0]) == f'<ObjectTag> {dest_unit_key}: test_taxonomy=tag_1' | ||
assert str(tags[1]) == f'<ObjectTag> {dest_unit_key}: test_taxonomy=tag_2' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ^ This test is great, and even if we end up changing how we handle the tags when copying and pasting, this test shouldn't need to be modified. |
||
|
||
def test_paste_with_assets(self): | ||
""" | ||
When pasting into a different course, any required static assets should | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,8 @@ | |
from organizations.models import Organization | ||
|
||
from .models import TaxonomyOrg | ||
from .types import ObjectTagByObjectIdDict, TaxonomyDict | ||
from .types import ContentKey, ObjectTagByObjectIdDict, TagValuesByTaxonomyExportIdDict, TaxonomyDict | ||
from .utils import check_taxonomy_context_key_org, get_context_key_from_key | ||
|
||
|
||
def create_taxonomy( | ||
|
@@ -161,16 +162,42 @@ def get_all_object_tags( | |
|
||
for object_id, block_tags in groupby(all_object_tags, lambda x: x.object_id): | ||
grouped_object_tags[object_id] = {} | ||
for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id): | ||
for taxonomy_id, taxonomy_tags in groupby(block_tags, lambda x: x.tag.taxonomy_id if x.tag else 0): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ObjectTag.objects.filter above guarantees that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a type guard to make our typer checker happy. I usually use assert for type guards (without error-handling routines), as this is not expected to be thrown at all. Is there a better way to handle this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm mainly puzzled as to why we had to add these checks now, but they weren't needed before. But it's not a big deal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, we "opt-in" to type checking when we add a return type to the function. |
||
object_tags_list = list(taxonomy_tags) | ||
grouped_object_tags[object_id][taxonomy_id] = object_tags_list | ||
|
||
if taxonomy_id not in taxonomies: | ||
assert object_tags_list[0].tag | ||
assert object_tags_list[0].tag.taxonomy | ||
taxonomies[taxonomy_id] = object_tags_list[0].tag.taxonomy | ||
|
||
return grouped_object_tags, taxonomies | ||
|
||
|
||
def set_object_tags( | ||
content_key: ContentKey, | ||
object_tags: TagValuesByTaxonomyExportIdDict, | ||
) -> None: | ||
""" | ||
Sets the tags for the given content object. | ||
""" | ||
context_key = get_context_key_from_key(content_key) | ||
|
||
for taxonomy_export_id, tags_values in object_tags.items(): | ||
taxonomy = oel_tagging.get_taxonomy_by_export_id(taxonomy_export_id) | ||
if not taxonomy: | ||
continue | ||
|
||
if not check_taxonomy_context_key_org(taxonomy, context_key): | ||
continue | ||
|
||
oel_tagging.tag_object( | ||
object_id=str(content_key), | ||
taxonomy=taxonomy, | ||
tags=tags_values, | ||
) | ||
|
||
|
||
# Expose the oel_tagging APIs | ||
|
||
get_taxonomy = oel_tagging.get_taxonomy | ||
|
@@ -181,3 +208,4 @@ def get_all_object_tags( | |
resync_object_tags = oel_tagging.resync_object_tags | ||
get_object_tags = oel_tagging.get_object_tags | ||
tag_object = oel_tagging.tag_object | ||
add_tag_to_taxonomy = oel_tagging.add_tag_to_taxonomy |
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.
Can you just change this test so that it uses
tagging_api.get_object_tags
to get the tags, rather than. serialize_tag_data()
?Because
serialize_tag_data
assumes that we're usingTaggedBlockMixin
but that's an implementation detail that we may change in the near future, based on that discussion.What I'd like to see is that even if we change to not include tags in the OLX, these tests will still be working and not need any changes.
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.
Done: 672f76c