Skip to content

Comments

feat: deserialize tag info from xml [FC-0049]#2181

Merged
pomegranited merged 4 commits intoopenedx:masterfrom
open-craft:rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data
Feb 29, 2024
Merged

feat: deserialize tag info from xml [FC-0049]#2181
pomegranited merged 4 commits intoopenedx:masterfrom
open-craft:rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data

Conversation

@rpenido
Copy link
Contributor

@rpenido rpenido commented Feb 27, 2024

Description

This PR adds support to deserialize tag info from the block XML, adding it to the new xml_attributes property. This is used to allow pasting OpenAsssessments components with their applied tags.

More Information

Testing instructions

@rpenido rpenido requested a review from a team February 27, 2024 12:46
@openedx-webhooks
Copy link

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.

Comment on lines +941 to +944
# Deserialize and add tag data info to block if any
if hasattr(block, 'read_tags_from_node') and callable(block.read_tags_from_node): # pylint: disable=no-member
# This comes from TaggedBlockMixin
block.read_tags_from_node(node) # pylint: disable=no-member
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 working as expected manual testing in the edx-platform, but in the tests here, it fails. I think it is because this block doesn't have the XmlMixin. As it is a new dependency, I'm unsure if I should add it here or find another way to test it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I faced similar issues when implementing the copy functionality. Since we don't have access to the XBLOCK_MIXINS in edx-platform in this repo (because edx-ora2 is imported into edx-platform), you need to manually add the methods/fields for the tests to work.

Here is an example of how I did it in my tests:

@ddt.file_data('data/serialize.json')
def test_serialize_with_tags(self, data):
self._configure_xblock(data)
# Create a mocked serialize tag data method that returns no data
def add_tags_to_node_no_tags(node): # pylint: disable=unused-argument
return
# Manually add the mocked method to the OpenAssessment block instance
# This method will be added in the edx-platform repo through applying XBLOCK_MIXINS
self.oa_block.add_tags_to_node = add_tags_to_node_no_tags
xml = serialize_content(self.oa_block)
# Confirm that no tags appear in the xml
self.assertNotIn("tags-v1", xml)
# Create a mocked serialize tag data method that returns data
def add_tags_to_node_with_tags(node):
# return "lightcast-skills:Typing,Microsoft Office"
node.set('tags-v1', 'lightcast-skills:Typing,Microsoft Office')
# Manually add the mocked method to the OpenAssessment block instance
# This method will be added in the edx-platform repo through applying XBLOCK_MIXINS
self.oa_block.add_tags_to_node = add_tags_to_node_with_tags
xml = serialize_content(self.oa_block)
# Confirm that tags appear in the xml
self.assertIn("tags-v1=\"lightcast-skills:Typing,Microsoft Office\"", xml)
# Clear the mocked serialize tag data method
del self.oa_block.add_tags_to_node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @yusuf-musleh! It took me some time but I figure it out with your help!

Could you take another look at it please?

@rpenido rpenido changed the title feat: deserialize tag info from xml feat: deserialize tag info from xml [FC-0049] Feb 27, 2024
@codecov
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (c76a89c) to head (48f7a8f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2181      +/-   ##
==========================================
+ Coverage   94.99%   95.01%   +0.01%     
==========================================
  Files         191      191              
  Lines       20999    21014      +15     
  Branches     1898     1899       +1     
==========================================
+ Hits        19949    19966      +17     
+ Misses        786      785       -1     
+ Partials      264      263       -1     
Flag Coverage Δ
unittests 95.01% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from 17f12dd to e8b7395 Compare February 28, 2024 16:32
@rpenido rpenido force-pushed the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch from e17bca6 to 48f7a8f Compare February 28, 2024 20:00
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 tested this by copy/pasting tagged ORA2 blocks using this code and openedx/openedx-platform#34270 , and it works 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

@pomegranited pomegranited merged commit 5329fea into openedx:master Feb 29, 2024
@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.

@pomegranited pomegranited deleted the rpenido/fal-3621-paste-tags-when-pasting-xblocks-with-tag-data branch March 1, 2024 04:50
@pomegranited
Copy link
Contributor

FYI @rpenido We can revert this change if open-craft/openedx-platform#640 tests out ok.

rpenido added a commit to open-craft/edx-ora2 that referenced this pull request Mar 1, 2024
pomegranited pushed a commit that referenced this pull request Mar 7, 2024
* Revert "feat: deserialize tag info from xml [FC-0049] (#2181)"

This reverts commit 5329fea.

* Revert "feat: Serialize tag data in OpenAssessmentBlocks (#2171)"

* test: improve coverage

* chore: bump version to 6.4.0
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.

4 participants