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

Jump to bottom

[Tagging] Don't put tags in OLX when duplicating/copy-pasting #203

Closed
bradenmacdonald opened this issue Mar 11, 2024 · 11 comments · Fixed by openedx/edx-platform#34386
Closed

[Tagging] Don't put tags in OLX when duplicating/copy-pasting #203

bradenmacdonald opened this issue Mar 11, 2024 · 11 comments · Fixed by openedx/edx-platform#34386
Milestone
Tagging MVP Step ...

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Mar 11, 2024 •
edited
Loading

"As platform developers, we don't want to change the OLX format for Redwood in a way that will be difficult to maintain when Learning Core is used more in the future".

Context: see this discussion.

Acceptance criteria:

  1. When an XBlock is duplicated in Studio, don't use the OLX to determine its new tags. Just use an event handler and the tagging API to duplicate the tags.
  2. When an XBlock is copied in Studio, copy its tags without including them in the OLX. (e.g. duplicate the ObjectTag entries and make them point to the StagedContent instances, or add a new tags field to the StagedContent model. Note it should be able to handle units and sections that are copied, with children that have tags.
  3. When an XBlock is pasted in Studio, paste its tags without reading them from the OLX.
  4. Remove TaggedBlockMixin from the platform and remove the related functionality from ORA2.
@bradenmacdonald bradenmacdonald mentioned this issue Mar 11, 2024
Closed
@pomegranited pomegranited mentioned this issue Mar 19, 2024
Merged
@pomegranited
Copy link

pomegranited commented Mar 21, 2024 •
edited
Loading

@yusuf-musleh CC @bradenmacdonald This part is giving me trouble:

Note it should be able to handle units and sections that are copied, with children that have tags.

The StagedContent OLX doesn't store usage keys for the child blocks, and I can't always compute them from the source_usage_key. So when the child blocks get pasted in, I don't have a way to map between the source child block keys and their new destination block IDs. And thus have trouble figuring out which tags to copy over to the new blocks.

E.g. suppose I copy a Unit to the clipboard that contains a bunch of blocks of various types:

Source UsageKey: block-v1:OpenCraft+OC+4+type@vertical+block@b08978228f39473499abe2880398c573
Source OLX:

<vertical display_name="Source" url_name="b08978228f39473499abe2880398c573">
<html url_name="4291c60f58624da0934e1f075b3084e3" display_name="Text">{snip}</html>
<openassessment submission_start="2001-01-01T00:00" submission_due="2029-01-01T00:00" text_response="required" text_response_editor="text" allow_multiple_files="True" allow_latex="False" prompts_type="text" teams_enabled="False" selected_teamset_id="" show_rubric_during_response="False">{snip}</openassessment>
<problem attempts_before_showanswer_button="0" display_name="Monty Python" showanswer="finished" submission_wait_seconds="0" weight="1.0" url_name="af7ad9b193de491d8acab060c6a72d41">{snip}</problem>
<video youtube="1.00:9aKxRzqeVlw" url_name="82e68620ef30401eb1f58973d5c23a3a" display_name="Video" edx_video_id="" end_time="00:00:00" html5_sources="[]" start_time="00:00:00" track="" youtube_id_1_0="9aKxRzqeVlw"/>
<drag-and-drop-v2 xblock-family="xblock.v1"/>
<discussion xblock-family="xblock.v1"/>
<library_content url_name="e085e2ba6c4a475e8bf93fdbcf061c43"/>
</vertical>

I can work out the source child usage keys for the blocks that have a url_name attribute, e.g. block-v1:OpenCraft+OC+4+type@html+block@4291c60f58624da0934e1f075b3084e3. But the ORA, DnDv2, and Discussion blocks don't provide this field, so I can't do it for them.

Alternatively, I could accumulate the tags for each block into a list that matches the block creation order, and just pop them off as new blocks are created.. but that doesn't seem very reliable.

How should I approach this?

@yusuf-musleh
Copy link
Member

yusuf-musleh commented Mar 21, 2024 •
edited
Loading

@pomegranited One approach I can think of was building on top of your suggestion

I could accumulate the tags for each block into a list that matches the block creation order, and just pop them off as new blocks are created

It would be to create a hash for each block and store that in a dictionary (rather than a list), mapping the hash -> tags that block holds. So then when it's time to paste, and you're extracting the serialized tags from the staged content blocks, you can recompute the hash and get the tags you should apply to it from the dict, that way its more reliable.

I was looking through the codebase, there are a few examples of hashing content. For example (though for a different purpose), here we hash the content of the static assets we are copying.

What do you think of this approach?

edit: I realized I'm making the assumption that each block would have something different/unique about it, is that the case? From the example above, if we look at <discussion xblock-family="xblock.v1"/> is that the full olx for it or are parts of it omitted? if we add another discussion block would it also be <discussion xblock-family="xblock.v1"/> ? If that is the case, I'm afraid my suggestion doens't really solve the problem.

@bradenmacdonald
Copy link
Contributor Author

@pomegranited It's too bad the url_name is inconsistent. One option would be to modify XBlockSerializer so that (a subclass of) it will always add the url_name attribute in case it's missing.

Alternatively, I could accumulate the tags for each block into a list that matches the block creation order, and just pop them off as new blocks are created.. but that doesn't seem very reliable.

I think an approach along those lines would be fine. But I wouldn't rely on the creation order or the order they're re-created. I agree it's not very reliable. But what should be reliable is the order of the children relative to the root block.

So you can store the tags like:

tags = {
    "0": {root block tags},
    "0.0": {tags for the first child of the root block},
    "0.1": {tags for the second child of the root block},
    "0.1.0": {tags for the first child of the second child of the root block},
}

etc.

When you get the BLOCK_COPIED signal, you can recurse the children, capture the tags, and create this structure. And when you get the BLOCK_PASTED, you wait until the whole tree of blocks is pasted, then apply the tags by walking the tree structure.

You can store this tag data in StagedContent.tags as a JSONField, or you could even create ObjectTag entries where each object_id is something like staged-content:123.0.0.1. But with the latter approach, I'm not sure how we'd make sure they got cleaned up, so I think the new tags field that you're going with makes more sense.

@pomegranited
Copy link

Thank you for your suggestions @yusuf-musleh and @bradenmacdonald !

One option would be to modify XBlockSerializer so that (a subclass of) it will always add the url_name attribute in case it's missing.

This one seems the simplest, so I'll try that first.

When you get the BLOCK_COPIED signal, you can recurse the children, capture the tags, and create this structure. And when you get the BLOCK_PASTED, you wait until the whole tree of blocks is pasted, then apply the tags by walking the tree structure.

We don't currently have BLOCK_COPIED or BLOCK_PASTED signals, but I can add them if that's the preferred way for content_tagging to hook into content_staging.

@bradenmacdonald
Copy link
Contributor Author

We don't currently have BLOCK_COPIED or BLOCK_PASTED signals, but I can add them if that's the preferred way for content_tagging to hook into content_staging.

Oh, it doesn't have to be a signal/event. You can just call the function directly. My bad.

@yusuf-musleh
Copy link
Member

One option would be to modify XBlockSerializer so that (a subclass of) it will always add the url_name attribute in case it's missing.

@bradenmacdonald @pomegranited Wouldn't we hit the same issue we faced before when we wanted to add tags to the OLX, i.e. where we'd need to make changes in other special block repos (like ora2) to introduce changes to the data being serialized?

@pomegranited
Copy link

@yusuf-musleh XBlockSerializer is different -- before, we were relying on properties and methods in the XBLOCK_MIXIN classes, which could be overridden by custom XBlocks like ora2. But XBlockSerializer is just a tool used to serialize any XBlock, so I can modify it to inject a url_name if the XBlock doesn't provide one.

@yusuf-musleh
Copy link
Member

@pomegranited Maybe I'm missing something, but XBlockSerializer uses the add_xml_to_node method internally to serialize blocks, which is defined in XMLMixin and inherited by xblocks (directly or though some mixin) or defined inside xblocks themselves (like the ora2 case). This approach seems very similar to the one we had for add tags to the xml right? Or does it work in this case because we would modify the _serialize_normal_block and _serialize_html_block directly rather than creating a new XBLOCK_MIXIN? In theory would adding tags to the olx technically have worked if we also followed this approach?

@pomegranited
Copy link

@yusuf-musleh

Here's what I ended up doing: openedx/edx-platform@7702971

This approach seems very similar to the one we had for add tags to the xml right?

Yes, there's a lot of similarities to this approach. The main difference is that instead of storing the serialized tags in the OLX (using TaggedBlockMixin), I'm storing them in a new XBlockSerializer.tags property. These serialized tags are then saved to the new StagedContent.tags field.

Or does it work in this case because we would modify the _serialize_normal_block and _serialize_html_block directly rather than creating a new XBLOCK_MIXIN?

I ended up modifying _serialize_block, but same effect.

In theory would adding tags to the olx technically have worked if we also followed this approach?

Yes -- we could have added tags to the OLX using this approach, and avoided TaggedBlockMixin and the issues with the non-XML block types.

But Axim didn't want us putting tags in the OLX, even temporarily, for a bunch of reasons.

@bradenmacdonald
Copy link
Contributor Author

@yusuf-musleh What you're missing is that XBlockSerializer is only used for copy-paste. So when we were planning to include tags in the OLX, modifying XBlockSerializer to include them would have made copy-paste work, but would not have worked for course export/import, library export/import, or studio's "duplicate" functionality.

And yeah, as @pomegranited has pointed out, we've decided not to include tags into OLX at any point due to concerns about forwards/backwards compatibility with the way that Learning Core will be using OLX for all XBlock storage in the future.

@yusuf-musleh
Copy link
Member

@pomegranited @bradenmacdonald I see, makes sense, thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Modular Learning - Tagging and Libraries
Status: Done
Milestone
Tagging MVP Step 5 - Tag Import/Expor...
Development

Successfully merging a pull request may close this issue.

Don't use OLX when copying/duplicating blocks [FC-0049] open-craft/edx-platform
3 participants
@bradenmacdonald @yusuf-musleh @pomegranited