From 1b66d3cd7413256d90b5c3e48e90e1d35596137e Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sun, 29 Mar 2015 14:16:24 -0700
Subject: [PATCH 1/8] New show_title and custom title options

---
 problem_builder/answer.py                     |   9 +-
 problem_builder/mrq.py                        |   5 +-
 problem_builder/questionnaire.py              |   9 +-
 problem_builder/step.py                       |  33 ++++-
 .../tests/integration/test_titles.py          | 126 ++++++++++++++++++
 5 files changed, 166 insertions(+), 16 deletions(-)
 create mode 100644 problem_builder/tests/integration/test_titles.py

diff --git a/problem_builder/answer.py b/problem_builder/answer.py
index c4584886..f09016d5 100644
--- a/problem_builder/answer.py
+++ b/problem_builder/answer.py
@@ -141,13 +141,7 @@ class AnswerBlock(AnswerMixin, StepMixin, StudioEditableXBlockMixin, XBlock):
         enforce_type=True
     )
 
-    editable_fields = ('question', 'name', 'min_characters', 'weight', 'default_from')
-
-    @property
-    def display_name_with_default(self):
-        if not self.lonely_step:
-            return self._(u"Question {number}").format(number=self.step_number)
-        return self._(u"Question")
+    editable_fields = ('question', 'name', 'min_characters', 'weight', 'default_from', 'display_name', 'show_title')
 
     @lazy
     def student_input(self):
@@ -172,6 +166,7 @@ def mentoring_view(self, context=None):
         """ Render this XBlock within a mentoring block. """
         context = context or {}
         context['self'] = self
+        context['hide_header'] = context.get('hide_header', False) or not self.show_title
         html = loader.render_template('templates/html/answer_editable.html', context)
 
         fragment = Fragment(html)
diff --git a/problem_builder/mrq.py b/problem_builder/mrq.py
index 44ff13c4..0fec59ca 100644
--- a/problem_builder/mrq.py
+++ b/problem_builder/mrq.py
@@ -69,7 +69,10 @@ class MRQBlock(QuestionnaireAbstractBlock):
         default=[],
     )
     hide_results = Boolean(display_name="Hide results", scope=Scope.content, default=False)
-    editable_fields = ('question', 'required_choices', 'ignored_choices', 'message', 'weight', 'hide_results', )
+    editable_fields = (
+        'question', 'required_choices', 'ignored_choices', 'message', 'display_name',
+        'show_title', 'weight', 'hide_results',
+    )
 
     def describe_choice_correctness(self, choice_value):
         if choice_value in self.required_choices:
diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py
index 34feed39..f4f36f84 100644
--- a/problem_builder/questionnaire.py
+++ b/problem_builder/questionnaire.py
@@ -81,7 +81,7 @@ class QuestionnaireAbstractBlock(StudioEditableXBlockMixin, StudioContainerXBloc
         scope=Scope.content,
         enforce_type=True
     )
-    editable_fields = ('question', 'message', 'weight')
+    editable_fields = ('question', 'message', 'weight', 'display_name', 'show_title')
     has_children = True
 
     def _(self, text):
@@ -113,12 +113,6 @@ def parse_xml(cls, node, runtime, keys, id_generator):
 
         return block
 
-    @property
-    def display_name_with_default(self):
-        if not self.lonely_step:
-            return self._(u"Question {number}").format(number=self.step_number)
-        return self._(u"Question")
-
     def student_view(self, context=None):
         name = getattr(self, "unmixed_class", self.__class__).__name__
 
@@ -127,6 +121,7 @@ def student_view(self, context=None):
         context = context or {}
         context['self'] = self
         context['custom_choices'] = self.custom_choices
+        context['hide_header'] = context.get('hide_header', False) or not self.show_title
 
         fragment = Fragment(loader.render_template(template_path, context))
         # If we use local_resource_url(self, ...) the runtime may insert many identical copies
diff --git a/problem_builder/step.py b/problem_builder/step.py
index 49ff2f77..1b386d97 100644
--- a/problem_builder/step.py
+++ b/problem_builder/step.py
@@ -19,9 +19,15 @@
 #
 
 from lazy import lazy
+from xblock.fields import String, Boolean, Scope
 from xblockutils.helpers import child_isinstance
 
 
+# Make '_' a no-op so we can scrape strings
+def _(text):
+    return text
+
+
 def _normalize_id(key):
     """
     Helper method to normalize a key to avoid issues where some keys have version/branch and others don't.
@@ -49,10 +55,26 @@ def steps(self):
 
 class StepMixin(object):
     """
-    An XBlock mixin for a child block that is a "Step"
+    An XBlock mixin for a child block that is a "Step".
+
+    A step is a question that the user can answer (as opposed to a read-only child).
     """
     has_author_view = True
 
+    # Fields:
+    display_name = String(
+        display_name=_("Question title"),
+        help=_('Leave blank to use the default ("Question 1", "Question 2", etc.)'),
+        default="",  # Blank will use 'Question x' - see display_name_with_default
+        scope=Scope.content
+    )
+    show_title = Boolean(
+        display_name=_("Show title"),
+        help=_("Display the title?"),
+        default=True,
+        scope=Scope.content
+    )
+
     @lazy
     def step_number(self):
         return list(self.get_parent().steps).index(_normalize_id(self.scope_ids.usage_id)) + 1
@@ -63,6 +85,15 @@ def lonely_step(self):
             raise ValueError("Step's parent should contain Step", self, self.get_parent().steps)
         return len(self.get_parent().steps) == 1
 
+    @property
+    def display_name_with_default(self):
+        """ Get the title/display_name of this question. """
+        if self.display_name:
+            return self.display_name
+        if not self.lonely_step:
+            return self._(u"Question {number}").format(number=self.step_number)
+        return self._(u"Question")
+
     def author_view(self, context):
         context = context or {}
         context['hide_header'] = True
diff --git a/problem_builder/tests/integration/test_titles.py b/problem_builder/tests/integration/test_titles.py
new file mode 100644
index 00000000..498d6a73
--- /dev/null
+++ b/problem_builder/tests/integration/test_titles.py
@@ -0,0 +1,126 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (c) 2014-2015 Harvard, edX & OpenCraft
+#
+# This software's license gives you freedom; you can copy, convey,
+# propagate, redistribute and/or modify this program under the terms of
+# the GNU Affero General Public License (AGPL) as published by the Free
+# Software Foundation (FSF), either version 3 of the License, or (at your
+# option) any later version of the AGPL published by the FSF.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Affero
+# General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program in a file in the toplevel directory called
+# "AGPLv3".  If not, see <http://www.gnu.org/licenses/>.
+#
+"""
+Test that the various title/display_name options for Answer and MCQ/MRQ/Ratings work.
+"""
+
+# Imports ###########################################################
+from mock import patch
+from xblockutils.base_test import SeleniumXBlockTest
+
+
+# Classes ###########################################################
+
+
+class StepTitlesTest(SeleniumXBlockTest):
+    """
+    Test that the various title/display_name options for Answer and MCQ/MRQ/Ratings work.
+    """
+
+    test_parameters = (
+        # display_name, show_title?, expected_title:  (None means default value)
+        ("Custom Title", None,  "Custom Title",),
+        ("Custom Title", True,  "Custom Title",),
+        ("Custom Title", False, None),
+        ("",             None,  "Question"),
+        ("",             True,  "Question"),
+        ("",             False, None),
+    )
+
+    mcq_template = """
+        <problem-builder mode="{{mode}}">
+            <pb-mcq name="mcq_1_1" question="Who was your favorite character?"
+              correct_choices="gaius,adama,starbuck,roslin,six,lee"
+              {display_name_attr} {show_title_attr}
+            >
+                <pb-choice value="gaius">Gaius Baltar</pb-choice>
+                <pb-choice value="adama">Admiral William Adama</pb-choice>
+                <pb-choice value="starbuck">Starbuck</pb-choice>
+                <pb-choice value="roslin">Laura Roslin</pb-choice>
+                <pb-choice value="six">Number Six</pb-choice>
+                <pb-choice value="lee">Lee Adama</pb-choice>
+            </pb-mcq>
+        </problem-builder>
+    """
+
+    mrq_template = """
+        <problem-builder mode="{{mode}}">
+            <pb-mrq name="mrq_1_1" question="What makes a great MRQ?"
+              ignored_choices="1,2,3"
+              {display_name_attr} {show_title_attr}
+            >
+                <pb-choice value="1">Lots of choices</pb-choice>
+                <pb-choice value="2">Funny choices</pb-choice>
+                <pb-choice value="3">Not sure</pb-choice>
+            </pb-mrq>
+        </problem-builder>
+    """
+
+    rating_template = """
+        <problem-builder mode="{{mode}}">
+            <pb-rating name="rating_1_1" question="How do you rate Battlestar Galactica?"
+              correct_choices="5,6"
+              {display_name_attr} {show_title_attr}
+            >
+                <pb-choice value="6">More than 5 stars</pb-choice>
+            </pb-rating>
+        </problem-builder>
+    """
+
+    long_answer_template = """
+        <problem-builder mode="{{mode}}">
+            <pb-answer name="answer_1_1" question="What did you think of the ending?"
+              {display_name_attr} {show_title_attr} />
+        </problem-builder>
+    """
+
+    def setUp(self):
+        super(StepTitlesTest, self).setUp()
+        # Disable asides for this test since the acid aside seems to cause Database errors
+        # When we test multiple scenarios in one test method.
+        patcher = patch(
+            'workbench.runtime.WorkbenchRuntime.applicable_aside_types',
+            lambda self, block: [], create=True
+        )
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def test_all_the_things(self):
+        """ Test various permutations of our problem-builder components and title options. """
+        # We use a loop within the test rather than DDT, because this is WAY faster
+        # since we can bypass the Selenium set-up and teardown
+        for display_name, show_title, expected_title in self.test_parameters:
+            for mode in ("standard", "assessment"):
+                for qtype in ("mcq", "mrq", "rating", "long_answer"):
+                    template = getattr(self, qtype + "_template")
+                    xml = template.format(
+                        mode=mode,
+                        display_name_attr='display_name="{}"'.format(display_name) if display_name is not None else "",
+                        show_title_attr='show_title="{}"'.format(show_title) if show_title is not None else "",
+                    )
+                    self.set_scenario_xml(xml)
+                    pb_element = self.go_to_view()
+                    if expected_title:
+                        h3 = pb_element.find_element_by_css_selector('h3')
+                        self.assertEqual(h3.text, expected_title)
+                    else:
+                        # No <h3> element should be present:
+                        all_h3s = pb_element.find_elements_by_css_selector('h3')
+                        self.assertEqual(len(all_h3s), 0)

From 18243ce25190caffc86ac936556f06e1d200f7dd Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sat, 28 Mar 2015 15:52:31 -0700
Subject: [PATCH 2/8] Fwd port fix to OC-545 (Issues in scrollbars in Feedback
 pop up for large data)

---
 problem_builder/public/css/questionnaire.css | 4 ++--
 problem_builder/public/js/questionnaire.js   | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/problem_builder/public/css/questionnaire.css b/problem_builder/public/css/questionnaire.css
index c56344d4..fafa07f0 100644
--- a/problem_builder/public/css/questionnaire.css
+++ b/problem_builder/public/css/questionnaire.css
@@ -32,9 +32,8 @@
     background: none repeat scroll 0 0 #66A5B5;
     font-family: arial;
     font-size: 14px;
-    overflow-y: auto;
     opacity: 0.9;
-    padding: 10px;
+    padding: 22px 10px;
     width: 300px;
 }
 
@@ -48,6 +47,7 @@
 .mentoring .questionnaire .feedback .tip-choice-group,
 .mentoring .questionnaire .feedback .message-content {
     position: relative;
+    overflow-y: auto;
 }
 
 .mentoring .questionnaire .choice-tips .close,
diff --git a/problem_builder/public/js/questionnaire.js b/problem_builder/public/js/questionnaire.js
index 033a2b3d..901dda31 100644
--- a/problem_builder/public/js/questionnaire.js
+++ b/problem_builder/public/js/questionnaire.js
@@ -20,15 +20,21 @@ function MessageView(element, mentoring) {
             var data = $(tip).data();
             if (data && data.width) {
                 popupDOM.css('width', data.width);
+                popupDOM.find('.tip-choice-group').css('width', data.width);
             } else {
                 popupDOM.css('width', '');
+                popupDOM.find('.tip-choice-group').css('width', '');
             }
 
             if (data && data.height) {
                 popupDOM.css('height', data.height);
             } else {
                 popupDOM.css('height', '');
+                popupDOM.css('maxHeight', '');
             }
+            // .tip-choice-group should always be the same height as the popup
+            // for scrolling to work properly.
+            popupDOM.find('.tip-choice-group').height(popupDOM.height());
 
             var container = popupDOM.parent('.choice-tips-container');
             if (container.length) {

From 709006f9a286a6f4b7169733c3136ee5e073b5f3 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sun, 29 Mar 2015 00:19:17 -0700
Subject: [PATCH 3/8] Fwd port fixes for popup heights

---
 problem_builder/public/css/questionnaire.css  |  2 ++
 problem_builder/public/js/questionnaire.js    |  1 +
 problem_builder/tests/integration/test_mcq.py | 35 +++++++++++++++++++
 .../xml/mcq_with_fixed_height_tips.xml        | 13 +++++++
 4 files changed, 51 insertions(+)
 create mode 100644 problem_builder/tests/integration/xml/mcq_with_fixed_height_tips.xml

diff --git a/problem_builder/public/css/questionnaire.css b/problem_builder/public/css/questionnaire.css
index fafa07f0..f6f9589f 100644
--- a/problem_builder/public/css/questionnaire.css
+++ b/problem_builder/public/css/questionnaire.css
@@ -35,6 +35,8 @@
     opacity: 0.9;
     padding: 22px 10px;
     width: 300px;
+    min-height: 40px;
+    z-index: 10000;
 }
 
 .mentoring .questionnaire .choice-tips .title {
diff --git a/problem_builder/public/js/questionnaire.js b/problem_builder/public/js/questionnaire.js
index 901dda31..a967c5b6 100644
--- a/problem_builder/public/js/questionnaire.js
+++ b/problem_builder/public/js/questionnaire.js
@@ -28,6 +28,7 @@ function MessageView(element, mentoring) {
 
             if (data && data.height) {
                 popupDOM.css('height', data.height);
+                popupDOM.css('maxHeight', data.height);
             } else {
                 popupDOM.css('height', '');
                 popupDOM.css('maxHeight', '');
diff --git a/problem_builder/tests/integration/test_mcq.py b/problem_builder/tests/integration/test_mcq.py
index 9cfc1680..317da0e7 100644
--- a/problem_builder/tests/integration/test_mcq.py
+++ b/problem_builder/tests/integration/test_mcq.py
@@ -261,6 +261,41 @@ def test_questionnaire_html_choices(self, page):
 
         self.assertIn('Congratulations!', messages.text)
 
+    def _get_inner_height(self, elem):
+        return elem.size['height'] - \
+            int(elem.value_of_css_property("padding-top").replace(u'px', u'')) - \
+            int(elem.value_of_css_property("padding-bottom").replace(u'px', u''))
+
+    @ddt.unpack
+    @ddt.data(
+        ('yes', 40),
+        ('maybenot', 60),
+        ('understand', 600)
+    )
+    def test_tip_height(self, choice_value, expected_height):
+        mentoring = self.go_to_page("Mcq With Fixed Height Tips")
+        choices_list = mentoring.find_element_by_css_selector(".choices-list")
+        submit = mentoring.find_element_by_css_selector('.submit input.input-main')
+
+        choice_input_css_selector = ".choice input[value={}]".format(choice_value)
+        choice_input = choices_list.find_element_by_css_selector(choice_input_css_selector)
+        choice_wrapper = choice_input.find_element_by_xpath("./ancestor::div[@class='choice']")
+
+        choice_input.click()
+        self.wait_until_clickable(submit)
+        submit.click()
+        self.wait_until_disabled(submit)
+
+        item_feedback_popup = choice_wrapper.find_element_by_css_selector(".choice-tips")
+        self.assertTrue(item_feedback_popup.is_displayed())
+        feedback_height = self._get_inner_height(item_feedback_popup)
+        self.assertEqual(feedback_height, expected_height)
+
+        choice_wrapper.find_element_by_css_selector(".choice-result").click()
+        item_feedback_popup = choice_wrapper.find_element_by_css_selector(".choice-tips")
+        item_feedback_height = self._get_inner_height(item_feedback_popup)
+        self.assertEqual(item_feedback_height, expected_height)
+
 
 @patch.object(MentoringBlock, 'get_theme', Mock(return_value={'package': 'problem_builder',
                                                               'locations': ['public/themes/lms.css']}))
diff --git a/problem_builder/tests/integration/xml/mcq_with_fixed_height_tips.xml b/problem_builder/tests/integration/xml/mcq_with_fixed_height_tips.xml
new file mode 100644
index 00000000..4a4dee69
--- /dev/null
+++ b/problem_builder/tests/integration/xml/mcq_with_fixed_height_tips.xml
@@ -0,0 +1,13 @@
+<vertical_demo>
+    <problem-builder url_name="mcq_1" enforce_dependency="false">
+        <pb-mcq name="mcq_1_1" question="Do you like this MCQ?" correct_choices="yes">
+            <pb-choice value="yes">Yes</pb-choice>
+            <pb-choice value="maybenot">Maybe not</pb-choice>
+            <pb-choice value="understand">I don't understand</pb-choice>
+
+            <pb-tip values="yes" height="10">Great!</pb-tip> <!--should be no less than 40 -->
+            <pb-tip values="maybenot" height="60">Ah, damn.</pb-tip>
+            <pb-tip values="understand" height="600">Really?</pb-tip> <!-- should override max-height -->
+        </pb-mcq>
+    </problem-builder>
+</vertical_demo>

From c72721829f427b91c098b103c7c8f82f3e0e661f Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Sat, 28 Mar 2015 22:51:52 -0700
Subject: [PATCH 4/8] Fwd port fix to OC-89: Improve MCQ/MRQ choice HTML

---
 problem_builder/public/css/questionnaire.css  | 31 +++++--------
 problem_builder/questionnaire.py              |  9 ++++
 problem_builder/templates/html/mcqblock.html  | 11 +++--
 problem_builder/templates/html/mrqblock.html  | 13 ++++--
 .../templates/html/ratingblock.html           | 35 ++++++++------
 problem_builder/tests/integration/test_mcq.py | 46 +++++++++----------
 6 files changed, 79 insertions(+), 66 deletions(-)

diff --git a/problem_builder/public/css/questionnaire.css b/problem_builder/public/css/questionnaire.css
index f6f9589f..f7b1af8b 100644
--- a/problem_builder/public/css/questionnaire.css
+++ b/problem_builder/public/css/questionnaire.css
@@ -1,19 +1,23 @@
 .mentoring .questionnaire .choices-list {
+    display: table;
     position: relative;
+    width: 100%;
+    border-spacing: 0 6px;
     padding-top: 10px;
     margin-bottom: 10px;
 }
 
 .mentoring .questionnaire .choice-result {
-    display: inline-block;
+    display: table-cell;
     width: 40px;
-    vertical-align: middle;
+    vertical-align: top;
     cursor: pointer;
     float: none;
 }
 
 .mentoring .questionnaire .choice {
     overflow-y: hidden;
+    display: table-row;
 }
 
 .mentoring .questionnaire .choice-result.checkmark-correct,
@@ -71,26 +75,13 @@
 }
 
 .mentoring .choices-list .choice-selector {
-    margin-right: 5px;
+    display: table-cell;
+    vertical-align: top;
+    width: 28px;
 }
 
 .mentoring .choice-label {
-    display: inline-block;
-    margin-top: 8px;
-    margin-bottom: 5px;
+    display: table-cell;
+    vertical-align: top;
     line-height: 1.3;
 }
-
-.mentoring .choices-list .choice-text > .xblock-light-child * {
-    vertical-align: middle;
-}
-
-.mentoring .choices-list .choice-text > .xblock-light-child,
-.mentoring .choices-list .choice-text > .xblock-light-child > .html_child {
-    /*
-    HTML Light Child content is wrapped in two divs: div.xblock-light-child and just div
-    On the other hand, choice are usually rendered inline.
-    Hence, we render first two divs inline, than all the actual content of HTML is rendered as is
-    */
-    display: inline-block;
-}
diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py
index f4f36f84..bca6c10c 100644
--- a/problem_builder/questionnaire.py
+++ b/problem_builder/questionnaire.py
@@ -113,6 +113,15 @@ def parse_xml(cls, node, runtime, keys, id_generator):
 
         return block
 
+    @property
+    def html_id(self):
+        """
+        A short, simple ID string used to uniquely identify this question.
+
+        This is only used by templates for matching <input> and <label> elements.
+        """
+        return unicode(id(self))  # Unique as long as all choices are loaded at once
+
     def student_view(self, context=None):
         name = getattr(self, "unmixed_class", self.__class__).__name__
 
diff --git a/problem_builder/templates/html/mcqblock.html b/problem_builder/templates/html/mcqblock.html
index c7e953ef..96c59340 100644
--- a/problem_builder/templates/html/mcqblock.html
+++ b/problem_builder/templates/html/mcqblock.html
@@ -7,9 +7,14 @@
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ choice.value }}"{% if self.student_choice == choice.value %} checked{% endif %} />
-          <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-{{ forloop.counter }}" type="radio"
+          name="{{ self.name }}" value="{{ choice.value }}"
+          {% if self.student_choice == choice.value %} checked{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-{{ forloop.counter }}">
+          {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>
diff --git a/problem_builder/templates/html/mrqblock.html b/problem_builder/templates/html/mrqblock.html
index fa9c8a19..31128d26 100644
--- a/problem_builder/templates/html/mrqblock.html
+++ b/problem_builder/templates/html/mrqblock.html
@@ -7,11 +7,14 @@
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="checkbox" name="{{ self.name }}"
-               value="{{ choice.value }}"
-               {% if choice.value in self.student_choices %} checked{% endif %} />
-        <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-{{ forloop.counter }}" type="checkbox"
+         name="{{ self.name }}" value="{{ choice.value }}"
+         {% if choice.value in self.student_choices %} checked{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-{{ forloop.counter }}">
+        {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>
diff --git a/problem_builder/templates/html/ratingblock.html b/problem_builder/templates/html/ratingblock.html
index 70f007ca..f5f599bb 100644
--- a/problem_builder/templates/html/ratingblock.html
+++ b/problem_builder/templates/html/ratingblock.html
@@ -4,32 +4,37 @@
     <p>{{ self.question }}</p>
   </legend>
   <div class="choices-list">
-    {% for value in '12345' %}
+    {% for i in '12345' %}
       <div class="choice">
         <div class="choice-result fa icon-2x"></div>
-        <label class="choice-label">
-          <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ value }}"
-                 {% if self.student_choice == value %}checked{% endif %} />
-          {{ value }}
-          {% if forloop.first %}
-            <span class="low">- {{ self.low }}</span>
-          {% endif %}
-          {% if forloop.last %}
-            <span class="low">- {{ self.high }}</span>
-          {% endif %}
+        <div class="choice-selector">
+          <input id="choice-{{ self.html_id }}-{{i}}" type="radio"
+            name="{{ self.name }}" value="{{i}}"
+            {% if self.student_choice == i %} checked{%else%} data-student-choice='{{self.student_choice}}'{% endif %}
+            />
+        </div>
+        <label class="choice-label" for="choice-{{ self.html_id }}-{{i}}">
+          {{i}}
+          {% if i == '1' %} - {{ self.low }}{% endif %}
+          {% if i == '5' %} - {{ self.high }}{% endif %}
         </label>
         <div class="choice-tips-container">
           <div class="choice-tips"></div>
         </div>
       </div>
     {% endfor %}
+
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ choice.value }}"
-               {% if self.student_choice == '{{ choice.value }}' %} checked{% endif %} />
-          <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-custom{{ forloop.counter }}" type="radio"
+          name="{{ self.name }}" value="{{ choice.value }}"
+          {% if self.student_choice == choice.value %} checked{%else%} data-student-choice='{{self.student_choice}}'{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-custom{{ forloop.counter }}">
+          {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>
diff --git a/problem_builder/tests/integration/test_mcq.py b/problem_builder/tests/integration/test_mcq.py
index 317da0e7..34d7d3d3 100644
--- a/problem_builder/tests/integration/test_mcq.py
+++ b/problem_builder/tests/integration/test_mcq.py
@@ -45,6 +45,9 @@ def _selenium_bug_workaround_scroll_to(self, mcq_legend):
         """
         mcq_legend.click()
 
+    def _get_choice_label_text(self, choice):
+        return choice.find_element_by_css_selector('label').text
+
     def _get_inputs(self, choices):
         return [choice.find_element_by_css_selector('input') for choice in choices]
 
@@ -71,20 +74,17 @@ def test_mcq_choices_rating(self):
         self.assertEqual(mcq1_legend.text, 'Question 1\nDo you like this MCQ?')
         self.assertEqual(mcq2_legend.text, 'Question 2\nHow do you rate this MCQ?')
 
-        mcq1_choices = mcq1.find_elements_by_css_selector('.choices .choice label')
-        mcq2_choices = mcq2.find_elements_by_css_selector('.rating .choice label')
-
-        self.assertEqual(len(mcq1_choices), 3)
-        self.assertEqual(len(mcq2_choices), 6)
-        self.assertEqual(mcq1_choices[0].text, 'Yes')
-        self.assertEqual(mcq1_choices[1].text, 'Maybe not')
-        self.assertEqual(mcq1_choices[2].text, "I don't understand")
-        self.assertEqual(mcq2_choices[0].text, '1 - Not good at all')
-        self.assertEqual(mcq2_choices[1].text, '2')
-        self.assertEqual(mcq2_choices[2].text, '3')
-        self.assertEqual(mcq2_choices[3].text, '4')
-        self.assertEqual(mcq2_choices[4].text, '5 - Extremely good')
-        self.assertEqual(mcq2_choices[5].text, "I don't want to rate it")
+        mcq1_choices = mcq1.find_elements_by_css_selector('.choices .choice')
+        mcq2_choices = mcq2.find_elements_by_css_selector('.rating .choice')
+
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq1_choices],
+            ["Yes", "Maybe not", "I don't understand"]
+        )
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq2_choices],
+            ['1 - Not good at all', '2', '3', '4', '5 - Extremely good', "I don't want to rate it"]
+        )
 
         mcq1_choices_input = self._get_inputs(mcq1_choices)
         mcq2_choices_input = self._get_inputs(mcq2_choices)
@@ -172,13 +172,13 @@ def test_mcq_with_comments(self):
         mcq_legend = mcq.find_element_by_css_selector('legend')
         self.assertEqual(mcq_legend.text, 'Question\nWhat do you like in this MRQ?')
 
-        mcq_choices = mcq.find_elements_by_css_selector('.choices .choice label')
+        mcq_choices = mcq.find_elements_by_css_selector('.choices .choice')
 
         self.assertEqual(len(mcq_choices), 4)
-        self.assertEqual(mcq_choices[0].text, 'Its elegance')
-        self.assertEqual(mcq_choices[1].text, 'Its beauty')
-        self.assertEqual(mcq_choices[2].text, "Its gracefulness")
-        self.assertEqual(mcq_choices[3].text, "Its bugs")
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq_choices],
+            ['Its elegance', 'Its beauty', "Its gracefulness", "Its bugs"]
+        )
 
         mcq_choices_input = self._get_inputs(mcq_choices)
         self.assertEqual(mcq_choices_input[0].get_attribute('value'), 'elegance')
@@ -200,7 +200,7 @@ def test_mcq_feedback_popups(self):
 
         for index, expected_feedback in enumerate(item_feedbacks):
             choice_wrapper = choices_list.find_elements_by_css_selector(".choice")[index]
-            choice_wrapper.find_element_by_css_selector(".choice-selector").click()  # clicking on actual radio button
+            choice_wrapper.find_element_by_css_selector(".choice-selector input").click()  # click actual radio button
             submit.click()
             self.wait_until_disabled(submit)
             item_feedback_icon = choice_wrapper.find_element_by_css_selector(".choice-result")
@@ -220,8 +220,8 @@ def _get_questionnaire_options(self, questionnaire):
         result = []
         # this could be a list comprehension, but a bit complicated one - hence explicit loop
         for choice_wrapper in questionnaire.find_elements_by_css_selector(".choice"):
-            choice_label = choice_wrapper.find_element_by_css_selector("label .choice-text")
-            result.append(choice_label.get_attribute('innerHTML'))
+            choice_label = choice_wrapper.find_element_by_css_selector("label")
+            result.append(choice_label.get_attribute('innerHTML').strip())
 
         return result
 
@@ -249,7 +249,7 @@ def test_questionnaire_html_choices(self, page):
         submit = mentoring.find_element_by_css_selector('.submit input.input-main')
         self.assertFalse(submit.is_enabled())
 
-        inputs = choices_list.find_elements_by_css_selector('input.choice-selector')
+        inputs = choices_list.find_elements_by_css_selector('.choice-selector input')
         self._selenium_bug_workaround_scroll_to(choices_list)
         inputs[0].click()
         inputs[1].click()

From 3f4ff281ce8cd011a111602daf1ce744c730e32c Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Mon, 30 Mar 2015 20:25:24 -0700
Subject: [PATCH 5/8] Fix: styles were not applied to Studio preview

---
 problem_builder/mentoring.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/problem_builder/mentoring.py b/problem_builder/mentoring.py
index 5a72d0a1..8d8177bb 100644
--- a/problem_builder/mentoring.py
+++ b/problem_builder/mentoring.py
@@ -559,7 +559,9 @@ def author_edit_view(self, context):
         """
         Add some HTML to the author view that allows authors to add child blocks.
         """
-        fragment = super(MentoringBlock, self).author_edit_view(context)
+        fragment = Fragment(u'<div class="mentoring">')  # This DIV is needed for CSS to apply to the previews
+        self.render_children(context, fragment, can_reorder=True, can_add=False)
+        fragment.add_content(u'</div>')
         fragment.add_content(loader.render_template('templates/html/mentoring_add_buttons.html', {}))
         fragment.add_content(loader.render_template('templates/html/mentoring_url_name.html', {
             "url_name": self.url_name

From 1eba407c404c9aad30b1a99a3f4b1a6f15253a54 Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 1 Apr 2015 23:22:50 -0700
Subject: [PATCH 6/8] Display choices as HTML in studio editor

---
 problem_builder/questionnaire.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py
index bca6c10c..8e26ad80 100644
--- a/problem_builder/questionnaire.py
+++ b/problem_builder/questionnaire.py
@@ -20,6 +20,7 @@
 
 # Imports ###########################################################
 
+from django.utils.safestring import mark_safe
 from lxml import etree
 from xblock.core import XBlock
 from xblock.fields import Scope, String, Float, List, UNIQUE_ID
@@ -160,7 +161,7 @@ def all_choice_values(self):
 
     @property
     def human_readable_choices(self):
-        return [{"display_name": c.content, "value": c.value} for c in self.custom_choices]
+        return [{"display_name": mark_safe(c.content), "value": c.value} for c in self.custom_choices]
 
     @staticmethod
     def choice_values_provider(question):

From 409214e49b1512abf896e777d8b5a39d213d3e3e Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Thu, 2 Apr 2015 21:40:06 -0700
Subject: [PATCH 7/8] Fix bugs in the upgrade script and change it to work w/
 mentoring v1 and v2 installed in parallel

---
 problem_builder/questionnaire.py              |   4 +
 .../v1/tests/xml/v1_upgrade_b_new.xml         |   2 +-
 .../v1/tests/xml/v1_upgrade_b_old.xml         |   2 +-
 problem_builder/v1/upgrade.py                 | 117 +++++++++++-------
 problem_builder/v1/xml_changes.py             |  27 ++--
 5 files changed, 92 insertions(+), 60 deletions(-)

diff --git a/problem_builder/questionnaire.py b/problem_builder/questionnaire.py
index 8e26ad80..74837e7b 100644
--- a/problem_builder/questionnaire.py
+++ b/problem_builder/questionnaire.py
@@ -21,6 +21,7 @@
 # Imports ###########################################################
 
 from django.utils.safestring import mark_safe
+import logging
 from lxml import etree
 from xblock.core import XBlock
 from xblock.fields import Scope, String, Float, List, UNIQUE_ID
@@ -99,6 +100,9 @@ def parse_xml(cls, node, runtime, keys, id_generator):
 
         # Load XBlock properties from the XML attributes:
         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]
             if isinstance(field, List) and not value.startswith('['):
                 # This list attribute is just a string of comma separated strings:
diff --git a/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml b/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml
index 1c571895..726d1e7c 100644
--- a/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml
+++ b/problem_builder/v1/tests/xml/v1_upgrade_b_new.xml
@@ -8,7 +8,7 @@
     <pb-choice value="yes">Yes</pb-choice>
     <pb-choice value="maybenot">Maybe not</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="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>
diff --git a/problem_builder/v1/tests/xml/v1_upgrade_b_old.xml b/problem_builder/v1/tests/xml/v1_upgrade_b_old.xml
index 5a088699..0487c291 100644
--- a/problem_builder/v1/tests/xml/v1_upgrade_b_old.xml
+++ b/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)
             <choice value="yes">Yes</choice>
             <choice value="maybenot">Maybe not</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="understand">
               <html>
diff --git a/problem_builder/v1/upgrade.py b/problem_builder/v1/upgrade.py
index a790a5d2..11232100 100644
--- a/problem_builder/v1/upgrade.py
+++ b/problem_builder/v1/upgrade.py
@@ -24,15 +24,16 @@
 optimized for editing in Studio.
 
 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
 from lxml import etree
 from mentoring import MentoringBlock
+from problem_builder import MentoringBlock as NewMentoringBlock
 from StringIO import StringIO
 import sys
+import warnings
 from courseware.models import StudentModule
-from xmodule.modulestore.exceptions import DuplicateItemError
 from .studio_xml_utils import studio_update_from_node
 from .xml_changes import convert_xml_v1_to_v2
 
@@ -42,14 +43,18 @@ def upgrade_block(block):
     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.
     """
-    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
     store = block.runtime.modulestore
     xml_content_str = block.xml_content
     parser = etree.XMLParser(remove_blank_text=True)
     root = etree.parse(StringIO(xml_content_str), parser=parser).getroot()
     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:
     try:
@@ -74,50 +79,41 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
     parent = block.get_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:
-    delete_on_success = None
-    if "url_name" in root.attrib:
-        url_name = root.attrib.pop("url_name")
-        if block.url_name != url_name:
-            print(" ➔ This block has two conflicting url_name values set. Attempting to fix...")
-            # Fix the url_name by replacing the block with a blank block with the correct url_name
-            parent_children = parent.children
-            old_usage_id = block.location
-            index = parent_children.index(old_usage_id)
-            try:
-                new_block = store.create_item(
-                    user_id=None,
-                    course_key=block.location.course_key,
-                    block_type="mentoring",
-                    block_id=url_name,
-                    fields={"xml_content": xml_content_str},
-                )
-                delete_on_success = block
-                parent_children[index] = new_block.location
-                parent.save()
-                store.update_item(parent, user_id=None)
-                block = new_block
-                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.
-                # Migrate it now:
-                student_data = StudentModule.objects.filter(module_state_key=old_usage_id)
-                num_entries = student_data.count()
-                if num_entries > 0:
-                    print(" ➔ Migrating {} student records to new url_name".format(num_entries))
-                    student_data.update(module_state_key=new_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
-                    )
-                )
+    old_usage_id = block.location
+    if old_usage_id.block_type != "problem-builder":
+        # We need to change the block type from "mentoring" to "problem-builder", which requires
+        # deleting then re-creating the block:
+        store.delete_item(old_usage_id, user_id=None)
+        parent_children = parent.children
+        index = parent_children.index(old_usage_id)
+
+        if "url_name" in root.attrib:
+            url_name = root.attrib.pop("url_name")
+            if unicode(old_usage_id.block_id) != url_name:
+                print(" ➔ This block has two conflicting url_name values!! Using the XML value : {}".format(url_name))
+        block = store.create_item(
+            user_id=None,
+            course_key=old_usage_id.course_key,
+            block_type="problem-builder",
+            block_id=url_name,
+            fields={"xml_content": xml_content_str},
+        )
+        parent_children[index] = block.location
+        parent.save()
+        store.update_item(parent, user_id=None)
+        print(" ➔ problem-builder created: {}".format(url_name))
+
+        # Now we've changed the block's block_type but in doing so we've disrupted the student data.
+        # Migrate it now:
+        student_data = StudentModule.objects.filter(module_state_key=old_usage_id)
+        num_entries = student_data.count()
+        if num_entries > 0:
+            print(" ➔ Migrating {} student records to new block".format(num_entries))
+            student_data.update(module_state_key=block.location)
 
     # Replace block with the new version and the new children:
     studio_update_from_node(block, root)
 
-    if delete_on_success:
-        store.delete_item(delete_on_success.location, user_id=None)
     if parent_was_published:
         store.publish(parent.location, user_id=None)
 
@@ -148,15 +144,46 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
     blocks_found = []
 
     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)
         elif block.has_children:
             for child_id in block.children:
                 find_mentoring_blocks(block.runtime.get_block(child_id))
     find_mentoring_blocks(course)
+
     total = len(blocks_found)
     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):
         count = 1
         for block_id in blocks_found:
diff --git a/problem_builder/v1/xml_changes.py b/problem_builder/v1/xml_changes.py
index 43c6e5e1..e7dea513 100644
--- a/problem_builder/v1/xml_changes.py
+++ b/problem_builder/v1/xml_changes.py
@@ -258,28 +258,29 @@ def add_to_list(list_name, value):
             else:
                 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 mode == "display":
+            if self.node.attrib.get("display"):
+                value = self.node.attrib.pop("display")
                 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)
-            elif mode != "reject":
-                warnings.warn("Invalid <tip> element: has {}={}".format(mode, value))
+            elif self.node.attrib.get("reject"):
+                value = self.node.attrib.pop("reject")
+            else:
+                warnings.warn("Invalid <tip> element found.")
                 return
         else:
             # 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)
-            elif mode != "reject":
-                warnings.warn("Invalid <tip> element: has {}={}".format(mode, value))
+            elif self.node.attrib.get("reject"):
+                value = self.node.attrib.pop("reject")
+            else:
+                warnings.warn("Invalid <tip> element found.")
                 return
         self.node.attrib["values"] = value
-        self.node.attrib.pop(mode)
 
 
 class SharedHeaderToHTML(Change):

From a4751871c9530ae31ed631f559fa1e2f334b9d1c Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Mon, 6 Apr 2015 16:18:57 -0700
Subject: [PATCH 8/8] Address review comments, fix bug when a unit contains two
 mentoring blocks.

---
 problem_builder/v1/upgrade.py     | 59 ++++++++++++++++---------------
 problem_builder/v1/xml_changes.py |  1 -
 2 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/problem_builder/v1/upgrade.py b/problem_builder/v1/upgrade.py
index 11232100..a6bf8dc0 100644
--- a/problem_builder/v1/upgrade.py
+++ b/problem_builder/v1/upgrade.py
@@ -54,7 +54,7 @@ def upgrade_block(block):
         warnings.simplefilter("always")
         convert_xml_v1_to_v2(root)
         for warning in warnings_caught:
-            print("    ➔ {}".format(str(warning.message)))
+            print(u"    ➔ {}".format(str(warning.message)))
 
     # We need some special-case handling to deal with HTML being an XModule and not a pure XBlock:
     try:
@@ -76,7 +76,7 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
     root.attrib["xml_content"] = xml_content_str
 
     # Was block already published?
-    parent = block.get_parent()
+    parent = block.runtime.get_block(block.parent)  # Don't use get_parent() as it may be an outdated cached version
     parent_was_published = not store.has_changes(parent)
 
     old_usage_id = block.location
@@ -87,10 +87,12 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
         parent_children = parent.children
         index = parent_children.index(old_usage_id)
 
+        url_name = unicode(old_usage_id.block_id)
         if "url_name" in root.attrib:
-            url_name = root.attrib.pop("url_name")
-            if unicode(old_usage_id.block_id) != url_name:
-                print(" ➔ This block has two conflicting url_name values!! Using the XML value : {}".format(url_name))
+            url_name_xml = root.attrib.pop("url_name")
+            if url_name != url_name_xml:
+                print(u"    ➔ Two conflicting url_name values! Using the 'real' one : {}".format(url_name))
+                print(u"    ➔ References to the old url_name ({}) need to be updated manually.".format(url_name_xml))
         block = store.create_item(
             user_id=None,
             course_key=old_usage_id.course_key,
@@ -101,14 +103,14 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
         parent_children[index] = block.location
         parent.save()
         store.update_item(parent, user_id=None)
-        print(" ➔ problem-builder created: {}".format(url_name))
+        print(u"    ➔ problem-builder created: {}".format(url_name))
 
         # Now we've changed the block's block_type but in doing so we've disrupted the student data.
         # Migrate it now:
         student_data = StudentModule.objects.filter(module_state_key=old_usage_id)
         num_entries = student_data.count()
         if num_entries > 0:
-            print(" ➔ Migrating {} student records to new block".format(num_entries))
+            print(u"    ➔ Migrating {} student records to new block".format(num_entries))
             student_data.update(module_state_key=block.location)
 
     # Replace block with the new version and the new children:
@@ -126,9 +128,9 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
     from opaque_keys.edx.keys import CourseKey
     from xmodule.modulestore.django import modulestore
 
-    print("┏━━━━━━━━━━━━━━━━━━━━━━━━━━┓")
-    print("┃ Mentoring Upgrade Script ┃")
-    print("┗━━━━━━━━━━━━━━━━━━━━━━━━━━┛")
+    print(u"┏━━━━━━━━━━━━━━━━━━━━━━━━━━┓")
+    print(u"┃ Mentoring Upgrade Script ┃")
+    print(u"┗━━━━━━━━━━━━━━━━━━━━━━━━━━┛")
 
     try:
         course_id = CourseKey.from_string(sys.argv[1])
@@ -139,8 +141,8 @@ def parse_xml_for_HtmlDescriptor(cls, node, runtime, keys, id_generator):
     course = store.get_course(course_id)
     if course is None:
         sys.exit(u"Course '{}' not found.".format(unicode(course_id)))
-    print(" ➔ Found course: {}".format(course.display_name))
-    print(" ➔ Searching for mentoring blocks")
+    print(u" ➔ Found course: {}".format(course.display_name))
+    print(u" ➔ Searching for mentoring blocks")
     blocks_found = []
 
     def find_mentoring_blocks(block):
@@ -157,39 +159,40 @@ def find_mentoring_blocks(block):
     find_mentoring_blocks(course)
 
     total = len(blocks_found)
-    print(" ➔ Found {} mentoring blocks".format(total))
+    print(u" ➔ Found {} mentoring blocks".format(total))
 
-    print(" ➔ Doing a quick sanity check of the url_names")
+    print(u" ➔ Doing a quick sanity check of the url_names")
     url_names = set()
     stop = False
     for block_id in blocks_found:
+        url_name = block_id.block_id
         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))
+        if url_name in url_names:
+            print(u" ➔ Mentoring block {} appears in the course in multiple places!".format(url_name))
             print(u'   (display_name: "{}", parent {}: "{}")'.format(
                 block.display_name, block.parent, block.get_parent().display_name
             ))
+            print(u'   To fix, you must delete the extra occurences.')
             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 block.url_name and block.url_name != unicode(block_id.block_id):
+            print(u" ➔ Warning: Mentoring block {} has a different url_name set in the XML.".format(url_name))
+            print(u"   If other blocks reference this block using the XML url_name '{}',".format(block.url_name))
+            print(u"   those blocks will need to be updated.")
+            if "--force" not in sys.argv:
+                print(u"   In order to force this upgrade to continue, add --force to the end of the command.")
+                stop = True
+        url_names.add(url_name)
 
     if stop:
-        print(" ➔ Exiting due to errors preventing the upgrade.")
-        sys.exit(1)
+        sys.exit(u" ➔ Exiting due to errors preventing the upgrade.")
 
     with store.bulk_operations(course.location.course_key):
         count = 1
         for block_id in blocks_found:
             block = course.runtime.get_block(block_id)
-            print(" ➔ Upgrading block {} of {} - \"{}\"".format(count, total, block.url_name))
+            print(u" ➔ Upgrading block {} of {} - \"{}\"".format(count, total, block.url_name))
             count += 1
             upgrade_block(block)
 
-    print(" ➔ Complete.")
+    print(u" ➔ Complete.")
diff --git a/problem_builder/v1/xml_changes.py b/problem_builder/v1/xml_changes.py
index e7dea513..a4c2d94d 100644
--- a/problem_builder/v1/xml_changes.py
+++ b/problem_builder/v1/xml_changes.py
@@ -184,7 +184,6 @@ def applies_to(node):
 
     def apply(self):
         self.node.tag = "pb-answer-recap"
-        self.node.attrib
         self.node.attrib.pop("read_only")
         for name in self.node.attrib:
             if name != "name":