Skip to content

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

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

Merged
pomegranited merged 4 commits into from
Feb 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e9f359
feat: deserialize tags from xml
rpenido Feb 26, 2024
7725d93
test: add mocks to fix test
rpenido Feb 28, 2024
e8b7395
chore: bump version
rpenido Feb 28, 2024
48f7a8f
test: improve coverage
rpenido Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openassessment/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
Initialization Information for Open Assessment Module
"""

__version__ = '6.1.0'
__version__ = '6.2.0'
11 changes: 10 additions & 1 deletion openassessment/xblock/openassessmentblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from webob import Response
from xblock.core import XBlock
from xblock.exceptions import NoSuchServiceError
from xblock.fields import Boolean, Integer, List, Scope, String
from xblock.fields import Boolean, Dict, Integer, List, Scope, String

from openassessment.staffgrader.staff_grader_mixin import StaffGraderMixin
from openassessment.workflow.errors import AssessmentWorkflowError
Expand Down Expand Up @@ -283,6 +283,9 @@ class OpenAssessmentBlock(
help="A title to display to a student (plain text)."
)

xml_attributes = Dict(help="Map of unhandled xml attributes, used only for storage between import and export",
default={}, scope=Scope.settings)

white_listed_file_types = List(
default=[],
scope=Scope.content,
Expand Down Expand Up @@ -934,6 +937,12 @@ def parse_xml(cls, node, runtime, keys, id_generator):
block.text_response_editor = config['text_response_editor']
block.title = config['title']
block.white_listed_file_types_string = config['white_listed_file_types']

# 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
Comment on lines +941 to +944

@rpenido rpenido Feb 27, 2024

Copy link
Copy Markdown
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.

@yusuf-musleh yusuf-musleh Feb 28, 2024

Copy link
Copy Markdown
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:

edx-ora2/openassessment/xblock/test/test_xml.py

Lines 219 to 251 in c76a89c

@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

@rpenido rpenido Feb 28, 2024

Copy link
Copy Markdown
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?


return block

@property
Expand Down
93 changes: 93 additions & 0 deletions openassessment/xblock/test/data/content_tags.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<openassessment text_response="required" file_upload_response="" group_access="{&quot;381451918&quot;: [1179773159]}" prompts_type="text" tags-v1="test content tags">
<title>Open Assessment Test</title>
<prompts>
<prompt>
<description>Given the state of the world today, what do you think should be done to combat poverty? Please answer in a short essay of 200-300 words.</description>
</prompt>
<prompt>
<description>Given the state of the world today, what do you think should be done to combat pollution?</description>
</prompt>
</prompts>
<rubric>
<criterion>
<name>Concise</name>
<prompt>How concise is it?</prompt>
<option points="0">
<name>Neal Stephenson (late)</name>
<explanation>Neal Stephenson explanation</explanation>
</option>
<option points="1">
<name>HP Lovecraft</name>
<explanation>HP Lovecraft explanation</explanation>
</option>
<option points="3">
<name>Robert Heinlein</name>
<explanation>Robert Heinlein explanation</explanation>
</option>
<option points="4">
<name>Neal Stephenson (early)</name>
<explanation>Neal Stephenson (early) explanation</explanation>
</option>
<option points="5">
<name>Earnest Hemingway</name>
<explanation>Earnest Hemingway</explanation>
</option>
</criterion>
<criterion>
<name>Clear-headed</name>
<prompt>How clear is the thinking?</prompt>
<option points="0">
<name>Yogi Berra</name>
<explanation>Yogi Berra explanation</explanation>
</option>
<option points="1">
<name>Hunter S. Thompson</name>
<explanation>Hunter S. Thompson explanation</explanation>
</option>
<option points="2">
<name>Robert Heinlein</name>
<explanation>Robert Heinlein explanation</explanation>
</option>
<option points="3">
<name>Isaac Asimov</name>
<explanation>Isaac Asimov explanation</explanation>
</option>
<option points="10">
<name>Spock</name>
<explanation>Spock explanation</explanation>
</option>
</criterion>
<criterion>
<name>Form</name>
<prompt>Lastly, how is its form? Punctuation, grammar, and spelling all count.</prompt>
<option points="0">
<name>lolcats</name>
<explanation>lolcats explanation</explanation>
</option>
<option points="1">
<name>Facebook</name>
<explanation>Facebook explanation</explanation>
</option>
<option points="2">
<name>Reddit</name>
<explanation>Reddit explanation</explanation>
</option>
<option points="3">
<name>metafilter</name>
<explanation>metafilter explanation</explanation>
</option>
<option points="4">
<name>Usenet, 1996</name>
<explanation>Usenet, 1996 explanation</explanation>
</option>
<option points="5">
<name>The Elements of Style</name>
<explanation>The Elements of Style explanation</explanation>
</option>
</criterion>
</rubric>
<assessments>
<assessment name="peer-assessment" must_grade="5" must_be_graded_by="3" />
<assessment name="self-assessment" />
</assessments>
</openassessment>
33 changes: 32 additions & 1 deletion openassessment/xblock/test/test_openassessment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from unittest import mock
from unittest.mock import MagicMock, Mock, PropertyMock, patch
from django.test.utils import override_settings
from workbench.runtime import WorkbenchRuntime

import ddt
import pytz
Expand All @@ -24,6 +25,29 @@
from .base import XBlockHandlerTestCase, scenario


original_construct_xblock_from_class = WorkbenchRuntime.construct_xblock_from_class


def _read_tags_from_node(self, node):
"""
This method is originally defined in the XmlMixin in edx-platform.
"""
assert 'tags-v1' in node.attrib
self.xml_attributes['tags-v1'] = str(node.attrib['tags-v1'])


def _construct_xblock_from_class(*args, **kwargs):
"""
Mock the original construct_xblock_from_class method to add the read_tags_from_node method and xml_attributes
property to the xblock.

In edx-platform, these members are part of the XmlMixin.
"""
xblock = original_construct_xblock_from_class(*args, **kwargs)
xblock.read_tags_from_node = lambda node: _read_tags_from_node(xblock, node)
return xblock


def assert_is_closed(
xblock,
now,
Expand Down Expand Up @@ -733,6 +757,12 @@ def test_mfe_views_supported__rearranged_steps(self, xblock):
# Given this ORA has rearranged our assessment steps
self.assertTrue(xblock.mfe_views_supported)

@patch.object(WorkbenchRuntime, 'construct_xblock_from_class', new=_construct_xblock_from_class)
@scenario('data/content_tags.xml')
def test_content_tags(self, xblock):
# Check if content tags are set properly
self.assertEqual(xblock.xml_attributes["tags-v1"], "test content tags")


class TestDates(XBlockHandlerTestCase):
""" Test Assessment Dates. """
Expand Down Expand Up @@ -1160,7 +1190,8 @@ def defined_manual_dates(self, xblock, step):
dt.datetime.fromisoformat(assessment.get('start')),
dt.datetime.fromisoformat(assessment.get('due'))
)
return None

assert False, f"Assessment {step} not found" # pragma: no cover

def setup_dates(self, xblock, course_dates=None, subsection_dates=None):
"""
Expand Down