Merge pull request #25 from open-craft/fix-author-changes

Test + fix for crash when block is deleted after students have used it

problem_builder/mentoring.py

@@ -232,22 +232,33 @@ class MentoringBlock(XBlock, StepParentMixin, StudioEditableXBlockMixin, StudioC
         """
         Create a JSON-dumpable object with readable key names from a list of student answers.
         """
-        return [
-            {
-                'number': self.get_question_number(answer[0]),
-                'id': answer[0],
-                'details': answer[1],
-                } for answer in self.student_results if answer[1]['status'] == answer_status
-        ]
+        answer_map = []
+        for answer in self.student_results:
+            if answer[1]['status'] == answer_status:
+                try:
+                    answer_map.append({
+                        'number': self.get_question_number(answer[0]),
+                        'id': answer[0],
+                        'details': answer[1],
+                    })
+                except ValueError:
+                    pass  # The question has been deleted since the student answered it.
+        return answer_map
 
     @property
     def score(self):
         """Compute the student score taking into account the weight of each step."""
-        weights = (float(self.runtime.get_block(step_id).weight) for step_id in self.steps)
-        total_child_weight = sum(weights)
+        steps = [self.runtime.get_block(step_id) for step_id in self.steps]
+        steps_map = {q.name: q for q in steps}
+        total_child_weight = sum(float(step.weight) for step in steps)
         if total_child_weight == 0:
             return Score(0, 0, [], [], [])
-        score = sum(r[1]['score'] * r[1]['weight'] for r in self.student_results) / total_child_weight
+        points_earned = 0
+        for q_name, q_details in self.student_results:
+            question = steps_map.get(q_name)
+            if question:
+                points_earned += q_details['score'] * question.weight
+        score = points_earned / total_child_weight
         correct = self.answer_mapper(CORRECT)
         incorrect = self.answer_mapper(INCORRECT)
         partially_correct = self.answer_mapper(PARTIAL)

problem_builder/tests/integration/base_test.py

@@ -60,24 +60,22 @@ class PopupCheckMixin(object):
             self.assertFalse(item_feedback_popup.is_displayed())
 
 
-class MentoringBaseTest(SeleniumBaseTest, PopupCheckMixin):
-    module_name = __name__
-    default_css_selector = 'div.mentoring'
-
-
-class MentoringBaseTemplateTest(SeleniumXBlockTest, PopupCheckMixin):
+class ProblemBuilderBaseTest(SeleniumXBlockTest, PopupCheckMixin):
     """
-    Base class for mentoring tests that use templated XML.
-    All new tests should inherit from this rather than MentoringBaseTest
+    The new base class for integration tests.
+    Scenarios can be loaded and edited on the fly.
     """
-    module_name = __name__
     default_css_selector = 'div.mentoring'
 
-    def load_scenario(self, xml_file, params=None):
+    def load_scenario(self, xml_file, params=None, load_immediately=True):
+        """
+        Given the name of an XML file in the xml_templates folder, load it into the workbench.
+        """
         params = params or {}
         scenario = loader.render_template("xml_templates/{}".format(xml_file), params)
         self.set_scenario_xml(scenario)
-        return self.go_to_view("student_view")
+        if load_immediately:
+            return self.go_to_view("student_view")
 
     def click_submit(self, mentoring):
         """ Click the submit button and wait for the response """
@@ -88,7 +86,12 @@ class MentoringBaseTemplateTest(SeleniumXBlockTest, PopupCheckMixin):
         self.wait_until_disabled(submit)
 
 
-class MentoringAssessmentBaseTest(MentoringBaseTemplateTest):
+class MentoringBaseTest(SeleniumBaseTest, PopupCheckMixin):
+    module_name = __name__
+    default_css_selector = 'div.mentoring'
+
+
+class MentoringAssessmentBaseTest(ProblemBuilderBaseTest):
     """
     Base class for tests of assessment mode
     """

problem_builder/tests/integration/test_author_changes.py

+"""
+If an author makes changes to the block after students have started using it, will bad things
+happen?
+"""
+from .base_test import ProblemBuilderBaseTest
+import re
+
+
+class AuthorChangesTest(ProblemBuilderBaseTest):
+    """
+    Test various scenarios involving author changes made to a block already in use by students
+    """
+    def setUp(self):
+        super(AuthorChangesTest, self).setUp()
+        self.load_scenario("author_changes.xml", load_immediately=False)
+        self.refresh_page()
+
+    def refresh_page(self):
+        """
+        [Re]load the page with our scenario
+        """
+        self.pb_block_dom = self.go_to_view("student_view")
+        self.reload_pb_block()
+
+    def reload_pb_block(self):
+        """
+        [Re]load the Problem Builder block, potentially with updated field data
+        """
+        vertical = self.load_root_xblock()
+        self.pb_block = vertical.runtime.get_block(vertical.children[0])
+
+    def submit_answers(self, q1_answer='yes', q2_answer='elegance', q3_answer="It's boring."):
+        """ Answer all three questions in the 'author_changes.xml' scenario correctly """
+        self.pb_block_dom.find_element_by_css_selector('input[name=q1][value={}]'.format(q1_answer)).click()
+        self.pb_block_dom.find_element_by_css_selector('input[name=q2][value={}]'.format(q2_answer)).click()
+        self.pb_block_dom.find_element_by_css_selector('textarea').send_keys(q3_answer)
+        self.click_submit(self.pb_block_dom)
+
+    def test_delete_question(self):
+        """ Test what the block behaves correctly when deleting a question """
+        # First, submit an answer to each of the three questions, but get the second question wrong:
+        self.submit_answers(q2_answer='bugs')
+        self.reload_pb_block()
+        self.assertEqual(self.pb_block.score.percentage, 67)
+
+        # Delete the second question:
+        self.pb_block.children = [self.pb_block.children[0], self.pb_block.children[2]]
+        self.pb_block.save()
+        self.reload_pb_block()
+
+        # Now that the wrong question is deleted, the student should have a perfect score:
+        self.assertEqual(self.pb_block.score.percentage, 100)
+        # NOTE: This is questionable, since the block does not send a new 'grade' event to the
+        # LMS. So the LMS 'grade' (based on the event sent when the student actually submitted
+        # the answers) and the block's current 'score' may be different.
+
+    def test_reweight_question(self):
+        """ Test what the block behaves correctly when changing the weight of a question """
+        # First, submit an answer to each of the three questions, but get the first question wrong:
+        self.submit_answers(q1_answer='no')
+        self.reload_pb_block()
+        self.assertEqual(self.pb_block.score.percentage, 67)
+
+        # Re-weight Q1 to '5':
+        q1 = self.pb_block.runtime.get_block(self.pb_block.children[0])
+        q1.weight = 5
+        q1.save()
+        self.reload_pb_block()
+        self.assertEqual(self.pb_block.score.percentage, 29)  # 29% is 2 out of 7 (5+1+1)
+
+        # Delete Q2 (the MRQ)
+        self.pb_block.children = [self.pb_block.children[0], self.pb_block.children[2]]
+        self.pb_block.save()
+        self.reload_pb_block()
+
+        # Now, the student's score should be 1 out of 6 (only q3 is correct):
+        self.assertEqual(self.pb_block.score.percentage, 17)

problem_builder/tests/integration/test_dashboard.py

@@ -18,7 +18,7 @@
 # "AGPLv3".  If not, see <http://www.gnu.org/licenses/>.
 #
 from mock import Mock, patch
-from .base_test import MentoringBaseTemplateTest
+from .base_test import ProblemBuilderBaseTest
 
 
 class MockSubmissionsAPI(object):
@@ -50,7 +50,7 @@ class MockSubmissionsAPI(object):
         return []
 
 
-class TestDashboardBlock(MentoringBaseTemplateTest):
+class TestDashboardBlock(ProblemBuilderBaseTest):
     """
     Test the Student View of a dashboard XBlock linked to some problem builder blocks
     """

problem_builder/tests/integration/test_messages.py

@@ -17,7 +17,7 @@
 # along with this program in a file in the toplevel directory called
 # "AGPLv3".  If not, see <http://www.gnu.org/licenses/>.
 #
-from .base_test import MentoringBaseTemplateTest
+from .base_test import ProblemBuilderBaseTest
 import ddt
 
 
@@ -32,7 +32,7 @@ MESSAGES = {
 
 
 @ddt.ddt
-class MessagesTest(MentoringBaseTemplateTest):
+class MessagesTest(ProblemBuilderBaseTest):
     """
     Test the various types of message that can be added to a problem.
     """

problem_builder/tests/integration/xml_templates/author_changes.xml

+<vertical_demo>
+    <problem-builder>
+
+        <pb-mcq name="q1" type="choices" correct_choices='["yes"]' question="So, do you like this Q1?">
+            <pb-choice value="yes">Yes</pb-choice>
+            <pb-choice value="no">No</pb-choice>
+        </pb-mcq>
+
+        <pb-mrq name="q2" required_choices='["elegance"]' question="What do you like in this Q2?">
+            <pb-choice value="elegance">Its elegance</pb-choice>
+            <pb-choice value="bugs">Its bugs</pb-choice>
+        </pb-mrq>
+
+        <pb-answer name="q3" question="What do you like about Q3?"/>
+
+    </problem-builder>
+</vertical_demo>

requirements.txt

 ddt
 mock
 unicodecsv==0.9.4
--e git+https://github.com/edx/xblock-utils.git@b2a17fa3793e98e67bdb86273317c41b6297dcbb#egg=xblock-utils
+-e git+https://github.com/edx/xblock-utils.git@a0e77eeb4eb971ac57243fe1056dd8db6806f514#egg=xblock-utils
 -e .

test_requirements.txt

--e git+https://github.com/edx/XBlock.git@496d3cb9aca1d9e0a18b0f5e73c7bede824e465f#egg=XBlock
\ No newline at end of file
+-e git+https://github.com/edx/XBlock.git@496d3cb9aca1d9e0a18b0f5e73c7bede824e465f#egg=XBlock