Skip to content

P
problem-builder
Commit 409214e4 by Braden MacDonald
Options

Fix bugs in the upgrade script and change it to work w/ mentoring v1 and v2 installed in parallel

parent 1eba407c
Showing with 92 additions and 60 deletions
problem_builder/questionnaire.py
...@@ -21,6 +21,7 @@ ...@@ -21,6 +21,7 @@
# Imports ########################################################### # Imports ###########################################################
from django.utils.safestring import mark_safe from django.utils.safestring import mark_safe
import logging
from lxml import etree from lxml import etree
from xblock.core import XBlock from xblock.core import XBlock
from xblock.fields import Scope, String, Float, List, UNIQUE_ID from xblock.fields import Scope, String, Float, List, UNIQUE_ID
...@@ -99,6 +100,9 @@ class QuestionnaireAbstractBlock(StudioEditableXBlockMixin, StudioContainerXBloc ...@@ -99,6 +100,9 @@ class QuestionnaireAbstractBlock(StudioEditableXBlockMixin, StudioContainerXBloc
# Load XBlock properties from the XML attributes: # Load XBlock properties from the XML attributes:
for name, value in node.items(): for name, value in node.items():
if name not in block.fields:
logging.warn("XBlock %s does not contain field %s", type(block), name)
continue
field = block.fields[name] field = block.fields[name]
if isinstance(field, List) and not value.startswith('['): if isinstance(field, List) and not value.startswith('['):
# This list attribute is just a string of comma separated strings: # This list attribute is just a string of comma separated strings:
......
problem_builder/v1/tests/xml/v1_upgrade_b_new.xml
...@@ -8,7 +8,7 @@ ...@@ -8,7 +8,7 @@
<pb-choice value="yes">Yes</pb-choice> <pb-choice value="yes">Yes</pb-choice>
<pb-choice value="maybenot">Maybe not</pb-choice> <pb-choice value="maybenot">Maybe not</pb-choice>
<pb-choice value="understand">I don't understand</pb-choice> <pb-choice value="understand">I don't understand</pb-choice>
<pb-tip values="yes">Great. Your frog should be happy for you.</pb-tip> <pb-tip width="350" height="100" values="yes">Great. Your frog should be happy for you.</pb-tip>
<pb-tip values="maybenot">In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you.</pb-tip> <pb-tip values="maybenot">In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you.</pb-tip>
<pb-tip values="understand"> <pb-tip values="understand">
<p>If a frog is <span class="italic">happy for you</span>, that means it is a frog that you genuinely feel in your own heart to be something that you want to improve. What is in your heart?</p> <p>If a frog is <span class="italic">happy for you</span>, that means it is a frog that you genuinely feel in your own heart to be something that you want to improve. What is in your heart?</p>
......
problem_builder/v1/tests/xml/v1_upgrade_b_old.xml
...@@ -16,7 +16,7 @@ This contains a typical problem taken from a live course (content changed) ...@@ -16,7 +16,7 @@ This contains a typical problem taken from a live course (content changed)
<choice value="yes">Yes</choice> <choice value="yes">Yes</choice>
<choice value="maybenot">Maybe not</choice> <choice value="maybenot">Maybe not</choice>
<choice value="understand">I don't understand</choice> <choice value="understand">I don't understand</choice>
<tip display="yes">Great. Your frog should be happy for you.</tip> <tip width="350" height="100" display="yes">Great. Your frog should be happy for you.</tip>
<tip display="maybenot">In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you.</tip> <tip display="maybenot">In the end, all the feedback you have gotten from others should not lead you to choose a frog that does not also feel happy and important to you.</tip>
<tip display="understand"> <tip display="understand">
<html> <html>
......
problem_builder/v1/upgrade.py
...@@ -24,15 +24,16 @@ This file contains a script to help migrate mentoring blocks to the new format w ...@@ -24,15 +24,16 @@ This file contains a script to help migrate mentoring blocks to the new format w
optimized for editing in Studio. optimized for editing in Studio.
To run the script on devstack: To run the script on devstack:
SERVICE_VARIANT=cms DJANGO_SETTINGS_MODULE="cms.envs.devstack" python -m mentoring.v1.upgrade SERVICE_VARIANT=cms DJANGO_SETTINGS_MODULE="cms.envs.devstack" python -m problem_builder.v1.upgrade [course id here]
""" """
import logging import logging
from lxml import etree from lxml import etree
from mentoring import MentoringBlock from mentoring import MentoringBlock
from problem_builder import MentoringBlock as NewMentoringBlock
from StringIO import StringIO from StringIO import StringIO
import sys import sys
import warnings
from courseware.models import StudentModule from courseware.models import StudentModule
from xmodule.modulestore.exceptions import DuplicateItemError
from .studio_xml_utils import studio_update_from_node from .studio_xml_utils import studio_update_from_node
from .xml_changes import convert_xml_v1_to_v2 from .xml_changes import convert_xml_v1_to_v2
...@@ -42,14 +43,18 @@ def upgrade_block(block): ...@@ -42,14 +43,18 @@ def upgrade_block(block):
Given a MentoringBlock "block" with old-style (v1) data in its "xml_content" field, parse Given a MentoringBlock "block" with old-style (v1) data in its "xml_content" field, parse
the XML and re-create the block with new-style (v2) children and settings. the XML and re-create the block with new-style (v2) children and settings.
""" """
assert isinstance(block, MentoringBlock) assert isinstance(block, (MentoringBlock, NewMentoringBlock))
assert bool(block.xml_content) # If it's a v1 block it will have xml_content assert bool(block.xml_content) # If it's a v1 block it will have xml_content
store = block.runtime.modulestore store = block.runtime.modulestore
xml_content_str = block.xml_content xml_content_str = block.xml_content
parser = etree.XMLParser(remove_blank_text=True) parser = etree.XMLParser(remove_blank_text=True)
root = etree.parse(StringIO(xml_content_str), parser=parser).getroot() root = etree.parse(StringIO(xml_content_str), parser=parser).getroot()
assert root.tag == "mentoring" assert root.tag == "mentoring"
convert_xml_v1_to_v2(root) with warnings.catch_warnings(record=True) as warnings_caught:
warnings.simplefilter("always")
convert_xml_v1_to_v2(root)
for warning in warnings_caught:
print(" ➔ {}".format(str(warning.message)))
# We need some special-case handling to deal with HTML being an XModule and not a pure XBlock: # We need some special-case handling to deal with HTML being an XModule and not a pure XBlock:
try: try:
...@@ -74,50 +79,41 @@ def upgrade_block(block): ...@@ -74,50 +79,41 @@ def upgrade_block(block):
parent = block.get_parent() parent = block.get_parent()
parent_was_published = not store.has_changes(parent) parent_was_published = not store.has_changes(parent)
# If the block has a url_name attribute that doesn't match Studio's url_name, fix that: old_usage_id = block.location
delete_on_success = None if old_usage_id.block_type != "problem-builder":
if "url_name" in root.attrib: # We need to change the block type from "mentoring" to "problem-builder", which requires
url_name = root.attrib.pop("url_name") # deleting then re-creating the block:
if block.url_name != url_name: store.delete_item(old_usage_id, user_id=None)
print(" ➔ This block has two conflicting url_name values set. Attempting to fix...") parent_children = parent.children
# Fix the url_name by replacing the block with a blank block with the correct url_name index = parent_children.index(old_usage_id)
parent_children = parent.children
old_usage_id = block.location if "url_name" in root.attrib:
index = parent_children.index(old_usage_id) url_name = root.attrib.pop("url_name")
try: if unicode(old_usage_id.block_id) != url_name:
new_block = store.create_item( print(" ➔ This block has two conflicting url_name values!! Using the XML value : {}".format(url_name))
user_id=None, block = store.create_item(
course_key=block.location.course_key, user_id=None,
block_type="mentoring", course_key=old_usage_id.course_key,
block_id=url_name, block_type="problem-builder",
fields={"xml_content": xml_content_str}, block_id=url_name,
) fields={"xml_content": xml_content_str},
delete_on_success = block )
parent_children[index] = new_block.location parent_children[index] = block.location
parent.save() parent.save()
store.update_item(parent, user_id=None) store.update_item(parent, user_id=None)
block = new_block print(" ➔ problem-builder created: {}".format(url_name))
print(" ➔ url_name changed to {}".format(url_name))
# Now we've fixed the block's url_name but in doing so we've disrupted the student data. # Now we've changed the block's block_type but in doing so we've disrupted the student data.
# Migrate it now: # Migrate it now:
student_data = StudentModule.objects.filter(module_state_key=old_usage_id) student_data = StudentModule.objects.filter(module_state_key=old_usage_id)
num_entries = student_data.count() num_entries = student_data.count()
if num_entries > 0: if num_entries > 0:
print(" ➔ Migrating {} student records to new url_name".format(num_entries)) print(" ➔ Migrating {} student records to new block".format(num_entries))
student_data.update(module_state_key=new_block.location) student_data.update(module_state_key=block.location)
except DuplicateItemError:
print(
"\n WARNING: The block with url_name '{}' doesn't match "
"the real url_name '{}' and auto repair failed.\n".format(
url_name, block.url_name
)
)
# Replace block with the new version and the new children: # Replace block with the new version and the new children:
studio_update_from_node(block, root) studio_update_from_node(block, root)
if delete_on_success:
store.delete_item(delete_on_success.location, user_id=None)
if parent_was_published: if parent_was_published:
store.publish(parent.location, user_id=None) store.publish(parent.location, user_id=None)
...@@ -148,15 +144,46 @@ if __name__ == '__main__': ...@@ -148,15 +144,46 @@ if __name__ == '__main__':
blocks_found = [] blocks_found = []
def find_mentoring_blocks(block): def find_mentoring_blocks(block):
if isinstance(block, MentoringBlock) and block.xml_content: # If it's a v1 block it will have xml_content """
Find mentoring and recently-upgraded blocks. We check the recently upgraded ones
in case an error happened and we're re-running the upgrade.
"""
# If it's a v1 block or a recently upgraded block it will have xml_content:
if isinstance(block, (MentoringBlock, NewMentoringBlock)) and block.xml_content:
blocks_found.append(block.scope_ids.usage_id) blocks_found.append(block.scope_ids.usage_id)
elif block.has_children: elif block.has_children:
for child_id in block.children: for child_id in block.children:
find_mentoring_blocks(block.runtime.get_block(child_id)) find_mentoring_blocks(block.runtime.get_block(child_id))
find_mentoring_blocks(course) find_mentoring_blocks(course)
total = len(blocks_found) total = len(blocks_found)
print(" ➔ Found {} mentoring blocks".format(total)) print(" ➔ Found {} mentoring blocks".format(total))
print(" ➔ Doing a quick sanity check of the url_names")
url_names = set()
stop = False
for block_id in blocks_found:
block = course.runtime.get_block(block_id)
if block.url_name in url_names:
print(u" ➔ Mentoring block {} conflicts with another block using the same url_name".format(block.url_name))
print(u' (display_name: "{}", parent {}: "{}")'.format(
block.display_name, block.parent, block.get_parent().display_name
))
stop = True
continue
if block.url_name != unicode(block_id.block_id):
print(u" ➔ Mentoring block {} has two different url_names assigned. Real url_name is {}".format(
unicode(block_id.block_id)
))
print(u' (display_name: "{}", parent: "{}")'.format(block.display_name, block.get_parent().display_name))
print(u" (to fix, edit the XML and set the url_name to the real value given above).")
stop = True
url_names.add(block.url_name)
if stop:
print(" ➔ Exiting due to errors preventing the upgrade.")
sys.exit(1)
with store.bulk_operations(course.location.course_key): with store.bulk_operations(course.location.course_key):
count = 1 count = 1
for block_id in blocks_found: for block_id in blocks_found:
......
problem_builder/v1/xml_changes.py
...@@ -258,28 +258,29 @@ class TipChanges(Change): ...@@ -258,28 +258,29 @@ class TipChanges(Change):
else: else:
p.attrib[list_name] = value p.attrib[list_name] = value
if len(self.node.attrib) > 1:
warnings.warn("Invalid <tip> element found.")
return
mode = self.node.attrib.keys()[0]
value = self.node.attrib[mode]
if p.tag == "pb-mrq": if p.tag == "pb-mrq":
if mode == "display": if self.node.attrib.get("display"):
value = self.node.attrib.pop("display")
add_to_list("ignored_choices", value) add_to_list("ignored_choices", value)
elif mode == "require": elif self.node.attrib.get("require"):
value = self.node.attrib.pop("require")
add_to_list("required_choices", value) add_to_list("required_choices", value)
elif mode != "reject": elif self.node.attrib.get("reject"):
warnings.warn("Invalid <tip> element: has {}={}".format(mode, value)) value = self.node.attrib.pop("reject")
else:
warnings.warn("Invalid <tip> element found.")
return return
else: else:
# This is an MCQ or Rating question: # This is an MCQ or Rating question:
if mode == "display": if self.node.attrib.get("display"):
value = self.node.attrib.pop("display")
add_to_list("correct_choices", value) add_to_list("correct_choices", value)
elif mode != "reject": elif self.node.attrib.get("reject"):
warnings.warn("Invalid <tip> element: has {}={}".format(mode, value)) value = self.node.attrib.pop("reject")
else:
warnings.warn("Invalid <tip> element found.")
return return
self.node.attrib["values"] = value self.node.attrib["values"] = value
self.node.attrib.pop(mode)
class SharedHeaderToHTML(Change): class SharedHeaderToHTML(Change):
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment