add a default XML serialization if the 'data' field is empty or None. So... #413
Conversation
|
@peter-fogg @cpennington can you review @cpennington this is what we'd need for a hotfix to prod |
| # certain XModules do not have a payload in 'data' any longer, e.g. the Video XModule | ||
| # so in that case, we serialize it out simply as an empty XML element named after | ||
| # the category | ||
| return etree.fromstring('<{0} />'.format(self.location.category)) |
There was a problem hiding this comment.
I think this edge case handling makes sense, just to avoid the RawDescriptor blowing up in other cases where data is empty (although it means that definition_to_xml and definition_from_xml aren't inverses, since parsing this xml will end up with data not being None).
On second thought... I think maybe I'd prefer to have the video module just define definition_to_xml that returns an empty xml node, since it's the code that expects to have an empty data attribute, and leave the RawDescriptor expecting to have non-empty data.
|
@cpennington @peter-fogg addressed comments |
|
👍 Looks good. |
|
👍, but a) please squash the commits, and b) please make a new pull request based on the latest release, as per the hotfix process |
|
Hotfix PR onto 'release' at: https://github.com/edx/edx-platform/pull/420 |
| Override the base implementation. We don't actually have anything in the 'data' field | ||
| (it's an empty string), so we just return a simple XML element | ||
| """ | ||
| return etree.fromstring('<video />') |
There was a problem hiding this comment.
I think that ideally this would actually just use etree.Element('video'), rather than parsing a string to generate an element.
There was a problem hiding this comment.
sure thing. Can I do this post merge?
There was a problem hiding this comment.
Let's do it in this PR, but the HF will go through as is.
-Cale
On Wed, Jul 17, 2013 at 1:06 PM, chrisndodge notifications@github.comwrote:
In common/lib/xmodule/xmodule/video_module.py:
@@ -128,6 +128,13 @@ def from_xml(cls, xml_data, system, org=None, course=None):
_parse_video_xml(video, xml_data)
return video
- def definition_to_xml(self, resource_fs):
"""Override the base implementation. We don't actually have anything in the 'data' field(it's an empty string), so we just return a simple XML element"""return etree.fromstring('<video />')sure thing. Can I do this post merge?
—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/413/files#r5247592
.
There was a problem hiding this comment.
In fact, lets hold off on merging this PR until we've got the HF through
and merged into master (since that'll shrink the changes in this PR
significantly).
-Cale
On Wed, Jul 17, 2013 at 1:16 PM, Calen Pennington cale@edx.org wrote:
Let's do it in this PR, but the HF will go through as is.
-Cale
On Wed, Jul 17, 2013 at 1:06 PM, chrisndodge notifications@github.comwrote:
In common/lib/xmodule/xmodule/video_module.py:
@@ -128,6 +128,13 @@ def from_xml(cls, xml_data, system, org=None, course=None):
_parse_video_xml(video, xml_data)
return video
- def definition_to_xml(self, resource_fs):
"""Override the base implementation. We don't actually have anything in the 'data' field(it's an empty string), so we just return a simple XML element"""return etree.fromstring('<video />')sure thing. Can I do this post merge?
—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/413/files#r5247592
.
- don't call has_access directly from template, pass a staff_access variable instead
…timeouts Temporarily add analytics 5s timeout
add program tencent video id api
ju/ednx/FFI-8: Adding CircleCI 2 and making tests PASS
...me XModules now - like video XModule - do not have XML set in the 'data' field