Skip to content

Enable (and test) xml export of pure XBlocks - #2221

Merged
cpennington merged 4 commits into
openedx:masterfrom
cpennington:xblock-xml-serialization
Feb 7, 2014
Merged

Enable (and test) xml export of pure XBlocks#2221
cpennington merged 4 commits into
openedx:masterfrom
cpennington:xblock-xml-serialization

Conversation

@cpennington

Copy link
Copy Markdown
Contributor

[LMS-179]
[LMS-209]
[LMS-1345]

@cpennington

Copy link
Copy Markdown
Contributor Author

@nedbat @sarina @antoviaque: Review?

@antoviaque

Copy link
Copy Markdown
Contributor

@cpennington Did a few tests, importing/exporting the mentoring XBlocks.

What worked (thanks!):

  • Creating a <mentoring> XBlock in Studio and exporting the course
  • Adding children to the <mentoring> block in the XML export, and importing it back -- the children had been correctly created

What didn't work:

screenshot from 2014-01-18 17 25 35

Let me know if you need additional details/tests.

@cpennington

Copy link
Copy Markdown
Contributor Author

Hrm... Thanks for checking that out. XBlocks have been deprioritized (pushed till next sprint) so that we can finish up some i18n work, but I'll try and get this XML stuff sorted out after that's done.

@antoviaque

Copy link
Copy Markdown
Contributor

@cpennington Okidok - thanks!

@cpennington

Copy link
Copy Markdown
Contributor Author

@nedbat, @sarina: Talked w/ @antoviaque, and we weren't able to reproduce one of his issues, and identified another as a bug in his code, rather than this code. Can you guys review, and we can see about merging this?

@sarina

sarina commented Feb 4, 2014

Copy link
Copy Markdown
Contributor

There's like no coverage of xml_exporter.py.

What's the best way to test this code out?

@cpennington

Copy link
Copy Markdown
Contributor Author

cms/djangoapps/contentstore/tests/test_contentstore.py uses export_to_xml a lot. It just doesn't get reported as part of the coverage for common/lib stuff. @wedaly what would we need to do to fix that? It seems odd not to count that sort of coverage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor point of style: don't need the [], a generator expression would work also.

@cpennington

Copy link
Copy Markdown
Contributor Author

@nedbat: Addressed your concerns. Have anymore?

@nedbat

nedbat commented Feb 5, 2014

Copy link
Copy Markdown
Contributor

I'm all out. 👍 :)

[LMS-179]
[LMS-209]
[LMS-1345]
@sarina

sarina commented Feb 5, 2014

Copy link
Copy Markdown
Contributor

LGTM 👍

@cpennington

Copy link
Copy Markdown
Contributor Author

@sarina, @nedbat can you review the last few commits? They're to add some testing of the api wrapping.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

put a comment here explaining what this is a dictionary of (most values are [{}] which is hard to understand why this is necessary)

@sarina

sarina commented Feb 6, 2014

Copy link
Copy Markdown
Contributor

2 tweaks and 2 questions, then 👍 by me

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we get rid of the __test__ = False above, then we can also get rid of this here, I think.

@nedbat

nedbat commented Feb 7, 2014

Copy link
Copy Markdown
Contributor

Figure out the best way to avoid __test__ if you can, then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants