fix: do not remove comments from XML during course import

[Agrendalath]

This is a follow-up to edx#1087, which reverted this change.

According to the PR comments, parsing strings with XML comments inside them was causing errors. This does not seem to be the case anymore - these strings are just hidden when the block is rendered, but they are not breaking XBlocks.

Jira

OSPR-5996
BB-3827

Sandbox

https://pr28557.sandbox.opencraft.hosting/ (provisioning)

Testing instructions

1. Create a new course with the following Blank Advanced Problem:

    <problem display_name="Checkboxes" markdown="null">
      <choiceresponse>
        <!-- This information is essential for the course authoring team. -->
        <p>
          Select a correct answer.
          <!-- Comment. -->
        </p>
        <checkboxgroup>
          <choice correct="true">a correct answer</choice>
          <choice correct="false">an incorrect answer</choice>
          <choice correct="false">an incorrect answer</choice>
        </checkboxgroup>
      </choiceresponse>
    </problem>

2. Export the course.

3. Import the exported course.

4. Check that the comments were retained in the XBlock.

Deadline

None.

Reviewers

• @xitij2000 / @kaizoku
• edX reviewer[s] TBD

Settings

[openedx-webhooks]

Thanks for the pull request, @Agrendalath! I've created OSPR-5996 to keep track of it in JIRA.

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

[xitij2000]

👍

• I tested this: I tried exporting and importing a course without this PR and the XML comments were lost in the process. I tried again with this PR checked out and the comments are retained now.
• I read through the code
• [na] I checked for accessibility issues
• [na] Includes documentation
• [na] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

[Agrendalath]

@sarina, should we treat this as a bug fix, technical enhancement, or an end-user facing change? This is going to retain comments after importing the course, so technically it is going to face staff users, but not learners. Who could acknowledge/review this additionally?

[sarina]

@Agrendalath I think I would treat as probably a technical enhancement. Since this is capa-related, the owning team is @edx/teaching-and-learning

Would someone on T&L ack this change, as per https://openedx.atlassian.net/wiki/spaces/COMM/pages/1529675973/Open+edX+Core+Committers#Reviews-and-Ownership which says that ack from owning team is sufficient for a PR with CC review. I tagged you because this changes the xml parser behavior.

[sarina]

or possibly @nedbat who reviewed the original revert PR?

[Agrendalath]

@sarina, won't we need another CC to review this change, as @xitij2000 is not the CC of edx-platform at the moment? Or does it still qualify as the non-author CC review?

[sarina]

Ah, great point. Yeah it's be good if Ned or someone from T&L could review in that case.

[nedbat]

The code I mentioned in my comment on the old PR is still in the repo (process_xml(etree.tostring). It could be that XML comments in some particular places will cause problems, but the comments you used in the example here do not.

It is true that if you try to parse "<!-- comment -->" as XML, it will raise XMLSyntaxError. There are nine occurrences of process_xml(etree.tostring. Some catch exceptions and move on to the next child, which is fine. Some do not. LibraryContentBlock seems vulnerable. Can you try adding an XML comment in a LibraryContentBlock?

[Agrendalath]

Thank you for your guidance here, @nedbat. I've scheduled some time for the next week to verify this case and add a test for it.

[Agrendalath]

@nedbat, you were right - when a comment was manually added to the LibraryContentBlock directly in the XML export, then this block was being ignored on import. I've changed this behavior (to ignoring XML comments) and added a test case for this.

I've also tried manually adding comments to verticals in the XML export and there were some exceptions raised by modulestore/xml.py, but they are already handled in seq_module and vertical_block.

I see that this code from the TranslateCustomTagBlock might not handle this, but when I'm trying to add a book, image, or slides XBlock, I'm getting the following exception in the Studio. I've checked #26973, but I haven't found any additional steps required to set this up.

Traceback (most recent call last):             
   File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/preview.py", line 327, in get_preview_fragment                                
     fragment = module.render(preview_view, context)
   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/xblock/core.py", line 198, in render
     return self.runtime.render(self, view, context)
   File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1969, in render
     return self.__getattr__('render')(block, view_name, context)
   File "/edx/app/edxapp/edx-platform/common/lib/xmodule/xmodule/x_module.py", line 1418, in render
     return super().render(block, view_name, context=context)
   File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/xblock/runtime.py", line 823, in render                                              
     raise NoSuchViewError(block, view_name)
 xblock.exceptions.NoSuchViewError

[nedbat]

This isn't an error, it would be better to do what XBlock does: explicitly check for a comment node and skip it. But this will rarely happen, so let's move ahead.

[nedbat]

You don't need to define setUp if it only calls super().setUp(). The parent's setUp will be inherited.

[Agrendalath]

@nedbat, it looks a bit confusing in the diff, but both super().setUp() and self.lc_block.refresh_children() have the same indentation level.

[nedbat]

Sorry for the delay. This is good to merge.

[nedbat]

Let me know if you want me to merge, or if you want to make any more adjustments.

[Agrendalath]

@nedbat, I've squashed the commits. Please merge this when the CI passes. Thank you!

[Agrendalath]

@nedbat, it looks a bit confusing in the diff, but both super().setUp() and self.lc_block.refresh_children() have the same indentation level.

[Agrendalath]

jenkins run all

[Agrendalath]

@nedbat, just a friendly reminder about this PR.

[Agrendalath]

@nedbat, checking again, as it would be great to include this in Maple.

[openedx-webhooks]

@jmbowman: thought you might like to know that Agrendalath merged this pull request.

[openedx-webhooks]

@Agrendalath 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[nedbat]

I've cherry-picked this onto Maple.

[Agrendalath]

Thank you, @nedbat!

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.