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

feat: paste tags when pasting xblocks with tag data [FC-0049] #34270

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented Feb 21, 2024

Description

This PR changes the paste XBlock feature to apply tags to the destination block.

More information

Depends on:

Testing instructions

  • Make sure the tests cover the various cases.
  • Start you localdev stack on this branch
  • Make sure you have Taxonomy/Tags data setup locally: https://github.com/open-craft/taxonomy-sample-data/
  • Once you have a course with a bunch of units and blocks with tags applied, try copying units and various types of blocks
  • Paste the blocks to another place and check if the tags are applied correctly. Note that if you paste a block from a course from one org to another, only the tags that are associated with the destination org will be applied.

Private ref: FAL-3621

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 21, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Feb 21, 2024

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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

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.

@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from c8bb343 to 880d959 Compare February 21, 2024 21:26
@rpenido rpenido changed the title feat: paste tags when pasting xblocks with tag data feat: paste tags when pasting xblocks with tag data [FC-0049] Feb 22, 2024
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 880d959 to 8b999e1 Compare February 22, 2024 21:27
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 0633668 to b01ddf4 Compare February 24, 2024 00:45
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from b01ddf4 to 77f95cb Compare February 24, 2024 01:21
Copy link
Member

@yusuf-musleh yusuf-musleh left a 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.

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")
Copy link
Member

Choose a reason for hiding this comment

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

Some commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed: 6f76a82

@rpenido
Copy link
Contributor Author

rpenido commented Feb 27, 2024

When pasting without tags applied:

Fixed that here: openedx/edx-ora2#2181
Could you take a look? I'm having a problem with the tests and would appreciate an opinion!

When pasting with tags:

I'm not able to reproduce this. Could you confirm that it is still throwing?

@yusuf-musleh
Copy link
Member

@rpenido

Could you take a look? I'm having a problem with the tests and would appreciate an opinion!

Sure thing, I left a comment regarding the tests, hopefully it helps!

I'm not able to reproduce this. Could you confirm that it is still throwing?

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.

Copy link
Member

@yusuf-musleh yusuf-musleh left a 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's configuration-secure repository.

Comment on lines 348 to 355

# 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()

Copy link
Contributor

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:

Copy link
Contributor Author

@rpenido rpenido Feb 29, 2024

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?

Copy link
Contributor Author

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

Copy link
Contributor

@pomegranited pomegranited Mar 1, 2024

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?

CC @yusuf-musleh @bradenmacdonald

Copy link
Member

Choose a reason for hiding this comment

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

@pomegranited @rpenido

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.

Copy link
Contributor

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.

Copy link
Contributor

@bradenmacdonald bradenmacdonald Mar 6, 2024

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 Components from LearningCore 😂, then we wouldn't have that problem, because each staged component would have its own DB model.

Copy link
Contributor

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nudge @bradenmacdonald :)

Copy link
Contributor

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).

cms/djangoapps/contentstore/helpers.py Outdated Show resolved Hide resolved
@rpenido rpenido requested a review from pomegranited February 29, 2024 20:23
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 9561fef to 24c4729 Compare February 29, 2024 20:55
rpenido and others added 5 commits February 29, 2024 18:07
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.
@rpenido rpenido marked this pull request as ready for review March 1, 2024 18:52
rpenido and others added 2 commits March 4, 2024 16:38
* 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
Copy link
Contributor

@pomegranited pomegranited left a 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 issues N/A
  • Includes documentation
  • User-facing strings are extracted for translation N/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):
Copy link
Contributor

@pomegranited pomegranited Mar 6, 2024

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)?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

openedx/core/djangoapps/content_tagging/utils.py Outdated Show resolved Hide resolved
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 3cc533c to b4f195a Compare March 7, 2024 13:41
@rpenido
Copy link
Contributor Author

rpenido commented Mar 7, 2024

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
Copy link
Contributor

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.

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: 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'
Copy link
Contributor

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.

@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 90a2992 to 672f76c Compare March 8, 2024 14:37
@bradenmacdonald bradenmacdonald merged commit cb6801d into openedx:master Mar 8, 2024
47 checks passed
@bradenmacdonald bradenmacdonald deleted the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch March 8, 2024 20:03
@openedx-webhooks
Copy link

@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.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Tagging] Pasting XBlocks containing Tag data in clipboard
7 participants