-
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
feat: paste tags when pasting xblocks with tag data [FC-0049] #34270
Conversation
Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
c8bb343
to
880d959
Compare
880d959
to
8b999e1
Compare
0633668
to
b01ddf4
Compare
b01ddf4
to
77f95cb
Compare
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.
@rpenido Great work on this! So far its looking good. However I noticed when I copy a unit that contains an openassessment (open response) block and try to paste it, it fails with errors. (even when there are no tags applied.) Here's what I see in the logs:
When pasting without tags applied:
Traceback (most recent call last):
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py", line 535, in _create_block
created_xblock, notices = import_staged_content_from_user_clipboard(
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 251, in import_staged_content_from_user_clipboard
new_xblock = _import_xml_node_to_parent(
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 347, in _import_xml_node_to_parent
_import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id)
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 354, in _import_xml_node_to_parent
new_xblock.add_tags_from_xml()
File "/edx/app/edxapp/edx-platform/cms/lib/xblock/tagging/tagged_block_mixin.py", line 67, in add_tags_from_xml
tag_data = self.xml_attributes.get('tags-v1', None) if self.xml_attributes else None
AttributeError: 'OpenAssessmentBlockWithMixins' object has no attribute 'xml_attributes'
When pasting with tags:
Traceback (most recent call last):
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py", line 535, in _create_block
created_xblock, notices = import_staged_content_from_user_clipboard(
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 251, in import_staged_content_from_user_clipboard
new_xblock = _import_xml_node_to_parent(
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 347, in _import_xml_node_to_parent
_import_xml_node_to_parent(child_node, new_xblock, store, user_id=user_id)
File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/helpers.py", line 344, in _import_xml_node_to_parent
new_xblock.sync_from_library(upgrade_to_latest=False)
File "/edx/app/edxapp/edx-platform/xmodule/library_content_block.py", line 557, in sync_from_library
self.get_tools(to_read_library_content=True).trigger_library_sync(
File "/edx/app/edxapp/edx-platform/xmodule/library_tools.py", line 127, in trigger_library_sync
if not library_api.get_v1_or_v2_library(library_key, version=library_version):
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content_libraries/api.py", line 1010, in get_v1_or_v2_library
library = get_library(library_key)
File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content_libraries/api.py", line 331, in get_library
num_blocks = publishing_api.get_all_drafts(learning_package.id).count()
AttributeError: 'NoneType' object has no attribute 'id'
edit: One thing I forgot to mention, when the unit is pasted, the tags number+iconbutton for all the units in the subsection disappear, it's might a separate issue not related to this PR, but thought I'd mention it in case we need to create a separate ticket for it.
cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Outdated
Show resolved
Hide resolved
Tag.objects.create(taxonomy=taxonomy_all_org, value="tag_1") | ||
Tag.objects.create(taxonomy=taxonomy_all_org, value="tag_2") | ||
# Removing a tag is causing tag_object to fail | ||
# tag_removed = Tag.objects.create(taxonomy=taxonomy, value="tag_removed") |
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.
Some commented out code
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.
Removed: 6f76a82
cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py
Outdated
Show resolved
Hide resolved
Fixed that here: openedx/edx-ora2#2181
I'm not able to reproduce this. Could you confirm that it is still throwing? |
Sure thing, I left a comment regarding the tests, hopefully it helps!
So I tested with new openassessment blocks, both with and without tags, and can confirm they are working correctly. It seems like I have some openassessment blocks that are in a weird state, even on master, I am not able to view the unit that contains them, so I do not think that it's related to this PR. |
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.
@rpenido Looks good to me! Thanks for addressing the issues. 👍
- I tested this: I confirmed pasting units/blocks with tags the tags are carried over
- I read through the code
-
I checked for accessibility issues -
Includes documentation -
I made sure any change in configuration variables is reflected in the corresponding client'sconfiguration-secure
repository.
|
||
# ToDo: Check the better place to call it | ||
# I tried to call it in the xml_block.py in the parse_xml() function, | ||
# but the usage_key is not persited yet there | ||
from cms.lib.xblock.tagging.tagged_block_mixin import TaggedBlockMixin | ||
if isinstance(new_xblock, TaggedBlockMixin): | ||
new_xblock.add_tags_from_xml() | ||
|
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.
@rpenido I tested copy/pasting a tagged blocks of various types, but the pasted blocks didn't get tagged, even though I was pasting them into the same course. CC @yusuf-musleh
These are the types I've tried:
- ORA2 works with your change from feat: deserialize tag info from xml [FC-0049] edx-ora2#2181 ✅
- Text/Html ❌
- Zooming Image Tool (listed under Text) ❌
- Video ❌
- Checkbox and Numerical problems (so probably all CAPA problems are affected) ❌
- Drag and Drop v2 ❌
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.
Hi @pomegranited! I tested these block types with mixed results. Maybe for some cases, it's missing a refresh after paste? If that's not the case, could you help me identify the issue? Do you have any errors in the shell?
@yusuf-musleh If you have some time, could you also test to see if I have something different in my stack?
* Text/Html ❌
tag_text.webm
* Zooming Image Tool (listed under Text) ❌
tag_zooming.webm
* Video ❌
tag_video.webm
* Checkbox and Numerical problems (so probably all CAPA problems are affected) ❌
The checkbox doesn't show the tags icon in the outline. The video was cut, but the copy/paste seems to work.
tag_checkbox.webm
* Drag and Drop v2 ❌
I'm unsure if it is the v2, but the Drag and Drop I tested didn't copy the tags around. I'm looking into it and will edit here if I find something.
edit: it seems to be the same case of OpenAssessments. I will implement the copy/paste the same way we did.
Is there some other XBlock that we should look for?
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.
DragAndDrop fixed here: openedx/xblock-drag-and-drop-v2#385
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.
@rpenido I don't know what was going on with my devstack yesterday, but I'm testing your branch again, and the issues I had yesterday aren't happening now. My apologies!
Is there some other XBlock that we should look for?
There's a lot of XBlocks that are variously maintained by Open edX or community members.. We can't possibly modify them all, and shouldn't have to either.. so I don't think we're headed down the correct path with openedx/xblock-drag-and-drop-v2#385 and openedx/edx-ora2#2181.
I've raised open-craft#640 to handle this issue better than we've been doing it. The issue is that TaggedBlockMixin is relying on the xml_attributes
field from XmlMixin in order to do its thing, but not all XBlocks will be XmlMixins (since XmlMixin
is not in XBLOCK_MIXINS), and so we can't rely on this field being present.
Instead, I've added a tags_v1
field to TaggedBlockMixin, and use that to store the serialized tag data. What do you think?
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.
Oh I see what's going on. So on master
we have a method in TaggedBlockMixin
that would handle storing the tags in the xml for all the "normal" blocks, since they all call the add_xml_to_node
method, it overrides it and extends it. The HTML serialization was separate so we called the add_tags_to_node
in there.
Seems like in this PR, we removed the add_xml_to_node
method override from TaggedBlockMixin
, hence we started seeing some inconsistent behavior, including blocks that do not have that method, because they would inherit it from TaggedBlockMixin
.
But I like @pomegranited's idea of having a new field that is separate from all that, and is more explicit on what it is and does, so it doesn't get accidentally removed cause some side effects.
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.
I was at first thinking of using an "on paste" event handler to just copy the tags over during paste. But I realize that doesn't support the "copy, delete, paste" workflow. So I think yeah, we'd add a new field to StagedContent
. It could be a tags
field but it's probably better to be a extra_data
JSON field and then allow the content_tagging
app to use that field to store/retrieve tags during copy/paste, so that the content_staging app doesn't need to be aware of tagging. Another option is to create a StagedContentTags
model inside the content_staging
app and use an "on copy" event handler to create the StagedContentTags
model, and an "on paste" event handler to set the tags on paste.
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.
Actually, yet another idea requires no new models or DB fields at all. During a copy event, ObjectTag
instances could be created that point to the StagedContent
instance, rather than pointing to a UsageKey
. I like this approach because the tags don't have to be serialized (they are represented in the DB the same way as other "normal" tags on content). The only downside is if the StagedContent
contains child blocks, some arbitrary new ID format is required to indicate that tags apply to child blocks inside the StagedContent.
Edit: but if we moved StagedContent
to use Component
s from LearningCore 😂, then we wouldn't have that problem, because each staged component would have its own DB model.
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.
@bradenmacdonald are you ok with the approach implemented here (for now)?
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.
Nudge @bradenmacdonald :)
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.
If this is basically done, I'm OK with merging it (because nobody is using this in prod yet), but I expect that next sprint we'll be following up with a PR to change the implementation so it doesn't modify the OLX (but keeping the tests).
* feat: duplicate tags when duplicating a tagged XBlock. * test: fixes flaky test
…blocks-with-tag-data
…blocks-with-tag-data
9561fef
to
24c4729
Compare
This reverts commit dacba7d.
Previously, we relied on the presence of the xml_attributes field, which is added by the XMLMixin. However, XMLMixin is not part of the default XBLOCK_MIXINS, so custom XBlocks weren't able to copy/paste tags.
…blocks-with-tag-data
* refactor: TaggedBlockMixin overrides add_xml_to_node so we can use inheritance to include tags in exported blocks * refactor: store tags_v1 in an internal field instead of an XBlock field, because we don't want this information persisted to the modulestore. * refactor: adds studio_post_paste handler for use after copy/pasting content from the clipboard. * fix: make serialize_tag_data a classmethod so we can serialize tags for blocks that aren't officially TaggedXBlocks
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.
I've left a question and a nit about a comment, but this is working beautifully @rpenido !
👍
- I tested this on my devstack in Studio:
- Created a course and a unit
- Added blocks to the unit of type: html, video, CAPA, drag and drop, survey/poll, ORA
- Added tags to all of the above blocks
- Copy/pasted these tagged blocks into a new unit for the same course -- tags were copied too.
- Tested copy/pasting untagged blocks -- still works as expected
- Tested copy/pasting tagged units into a course with a different org -- disallowed tags were omitted on paste, and allowed tags were copied as expected.
- I read through the code
-
I checked for accessibility issuesN/A - Includes documentation
-
User-facing strings are extracted for translationN/A
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
The ObjectTag.objects.filter above guarantees that tag
is not null.. why do we need to check that again here (and assert below)?
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.
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 comment
The 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 comment
The 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.
Accessing x.tag.taxonomy_id
will always throw a type error if the tag is nullable and we didn't check it before calling it.
3cc533c
to
b4f195a
Compare
Co-authored-by: Jillian <[email protected]>
Updated ora2 package: b4f195a CC @pomegranited |
# Refresh. | ||
source_chapter = self.store.get_item(source_chapter.location) | ||
expected_chapter_tags = 'A:one,two;B:four,three' | ||
assert source_chapter.serialize_tag_data(source_chapter.location) == expected_chapter_tags |
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 using TaggedBlockMixin
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
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 comment
The 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.
90a2992
to
672f76c
Compare
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
This PR changes the paste XBlock feature to apply tags to the destination block.
More information
Depends on:
Testing instructions
Private ref: FAL-3621