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.
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
Changes from 6 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
There are no files selected for viewing
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:
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?
tag_text.webm
tag_zooming.webm
tag_video.webm
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
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!
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 (sinceXmlMixin
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
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.
@pomegranited @rpenido
Oh I see what's going on. So on
master
we have a method inTaggedBlockMixin
that would handle storing the tags in the xml for all the "normal" blocks, since they all call theadd_xml_to_node
method, it overrides it and extends it. The HTML serialization was separate so we called theadd_tags_to_node
in there.Seems like in this PR, we removed the
add_xml_to_node
method override fromTaggedBlockMixin
, hence we started seeing some inconsistent behavior, including blocks that do not have that method, because they would inherit it fromTaggedBlockMixin
.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 atags
field but it's probably better to be aextra_data
JSON field and then allow thecontent_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 aStagedContentTags
model inside thecontent_staging
app and use an "on copy" event handler to create theStagedContentTags
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 theStagedContent
instance, rather than pointing to aUsageKey
. 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 theStagedContent
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 useComponent
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).
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
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.