Merge pull request #2637 from edx/nick/fix-shuffle-bug

Fix bug with multiple shuffled responses per problem

common/lib/capa/capa/responsetypes.py

@@ -825,8 +825,8 @@ class MultipleChoiceResponse(LoncapaResponse):
         using the regular (non-masked) names.
         Fails loudly if called on a response that is not masking.
         """
-        choicegroups = self.xml.xpath('//choicegroup')
-        return [self.unmask_name(choice.get("name")) for choice in choicegroups[0].getchildren()]
+        choices = self.xml.xpath('choicegroup/choice')
+        return [self.unmask_name(choice.get("name")) for choice in choices]
 
     def do_shuffle(self, tree):
         """
@@ -834,10 +834,12 @@ class MultipleChoiceResponse(LoncapaResponse):
         based on the seed. Otherwise does nothing.
         Does nothing if the tree has already been processed.
         """
-        choicegroups = tree.xpath('//choicegroup[@shuffle="true"]')
+        # The tree is already pared down to this <multichoiceresponse> so this query just
+        # gets the child choicegroup (i.e. no leading //)
+        choicegroups = tree.xpath('choicegroup[@shuffle="true"]')
         if choicegroups:
             if len(choicegroups) > 1:
-                raise LoncapaProblemError('We support at most one shuffled choicegroup')
+                raise LoncapaProblemError('We support one shuffled choicegroup per response')
             choicegroup = choicegroups[0]
             # Note that this tree has been processed.
             if choicegroup.get('shuffle-done') is not None:
@@ -890,11 +892,16 @@ class MultipleChoiceResponse(LoncapaResponse):
         pool size. If that attribute is zero or not present, no operation is performed.
         Calling this a second time does nothing.
         """
-        choicegroups = tree.xpath("//choicegroup[@answer-pool]")
+        choicegroups = tree.xpath("choicegroup[@answer-pool]")
 
         # Uses self.seed -- but want to randomize every time reaches this problem,
         # so problem's "randomization" should be set to "always"
         rnd = random.Random(self.context['seed'])
+        
+        # TODO: currently, each choicegroup uses the seed from the problem.
+        # This has the unfortunate effect of making choicegroups look similar
+        # when we have many in one problem. Could perturb the rnd a little
+        # per choicegroup so they look different.
 
         for choicegroup in choicegroups:
             num_str = choicegroup.get('answer-pool')

common/lib/capa/capa/tests/test_answer_pool.py

@@ -73,7 +73,7 @@ class CapaAnswerPoolTest(unittest.TestCase):
 
             <p>What is the correct answer?</p>
             <multiplechoiceresponse>
-              <choicegroup type="MultipleChoice" answer-pool="4">
+              <choicegroup type="MultipleChoice" answer-pool="4 ">
                 <choice correct="false">wrong-1</choice>
                 <choice correct="false">wrong-2</choice>
                 <choice correct="true" explanation-id="solution1">correct-1</choice>
@@ -361,9 +361,11 @@ class CapaAnswerPoolTest(unittest.TestCase):
 
         problem = new_loncapa_problem(xml_str, seed=723)
         the_html = problem.get_html()
+        print the_html
 
         str1 = r"<div>.*\[.*'wrong-3'.*'correct-2'.*'wrong-2'.*'wrong-4'.*\].*</div>"
-        str2 = r"<div>.*\[.*'wrong-2'.*'wrong-1'.*'correct-2'.*\].*</div>"
+        str2 = r"<div>.*\[.*'correct-2'.*'wrong-2'.*'wrong-3'.*\].*</div>"
+
         str3 = r"<div>\{.*'1_solution_2'.*\}</div>"
         str4 = r"<div>\{.*'1_solution_4'.*\}</div>"
 
@@ -447,9 +449,9 @@ class CapaAnswerPoolTest(unittest.TestCase):
         the_html = problem.get_html()
 
         str1 = r"<div>.*\[.*'wrong-4'.*'wrong-3'.*'correct-1'.*\].*</div>"
-        str2 = r"<div>.*\[.*'wrong-2'.*'wrong-3'.*'wrong-4'.*'correct-2'.*\].*</div>"
+        str2 = r"<div>.*\[.*'wrong-1'.*'wrong-4'.*'wrong-3'.*'correct-1'.*\].*</div>"
         str3 = r"<div>\{.*'1_solution_1'.*\}</div>"
-        str4 = r"<div>\{.*'1_solution_4'.*\}</div>"
+        str4 = r"<div>\{.*'1_solution_3'.*\}</div>"
 
         self.assertRegexpMatches(the_html, str1)
         self.assertRegexpMatches(the_html, str2)

common/lib/capa/capa/tests/test_shuffle.py

@@ -245,3 +245,37 @@ class CapaShuffleTest(unittest.TestCase):
         problem = new_loncapa_problem(xml_str, seed=0)
         the_html = problem.get_html()
         self.assertRegexpMatches(the_html, r"<div>.*\[.*'A'.*'Mid'.*'Mid'.*'C'.*'D'.*\].*</div>")
+
+    def test_multiple_shuffle_responses(self):
+        xml_str = textwrap.dedent("""
+            <problem>
+            <multiplechoiceresponse>
+              <choicegroup type="MultipleChoice" shuffle="true">
+                <choice correct="false">Apple</choice>
+                <choice correct="false">Banana</choice>
+                <choice correct="false">Chocolate</choice>
+                <choice correct ="true">Donut</choice>
+              </choicegroup>
+            </multiplechoiceresponse>
+            <p>Here is some text</p>
+            <multiplechoiceresponse>
+              <choicegroup type="MultipleChoice" shuffle="true">
+                <choice correct="false">A</choice>
+                <choice correct="false">B</choice>
+                <choice correct="false">C</choice>
+                <choice correct ="true">D</choice>
+              </choicegroup>
+            </multiplechoiceresponse>
+            </problem>
+        """)
+        problem = new_loncapa_problem(xml_str, seed=0)
+        orig_html = problem.get_html()
+        self.assertEqual(orig_html, problem.get_html(), 'should be able to call get_html() twice')
+        html = orig_html.replace('\n', ' ')  # avoid headaches with .* matching
+        self.assertRegexpMatches(html, r"<div>.*\[.*'Banana'.*'Apple'.*'Chocolate'.*'Donut'.*\].*</div>.*" +
+                                       r"<div>.*\[.*'B'.*'A'.*'C'.*'D'.*\].*</div>")
+        responses = problem.responders.values()
+        self.assertTrue(hasattr(responses[0], 'is_masked'))
+        self.assertTrue(hasattr(responses[1], 'is_masked'))
+        self.assertEqual(responses[0].unmask_order(), ['choice_1', 'choice_0', 'choice_2', 'choice_3'])
+        self.assertEqual(responses[1].unmask_order(), ['choice_1', 'choice_0', 'choice_2', 'choice_3'])

common/lib/xmodule/xmodule/capa_base.py

@@ -1044,7 +1044,9 @@ class CapaMixin(CapaFields):
                     event_info['state']['student_answers'][response.answer_id] = response.unmask_name(answer)
 
                 # 3. Record the shuffled ordering
-                event_info['display_order'] = {response.answer_id: response.unmask_order()}
+                if not 'display_order' in event_info:
+                    event_info['display_order'] = {}
+                event_info['display_order'][response.answer_id] = response.unmask_order()
 
     def pretty_print_seconds(self, num_seconds):
         """