MDL-74752 qtype_multichoice: fix regrading logic

The implements the new regrade-related hooks, and also has
tests for the changes to the core system, now that we have a question
type we can use for them.

mod/quiz/report/overview/tests/behat/regrading_not_possible.feature

@@ -0,0 +1,56 @@
+@mod @mod_quiz @quiz @quiz_overview @javascript
+Feature: Quiz regrade when not possible
+  In order avoid errors
+  As a teacher
+  I need the system to prevent impossible regrade scenarios
+
+  Background:
+    Given the following "users" exist:
+      | username | firstname | lastname  |
+      | teacher  | Mark      | Allwright |
+      | student  | Student   | One       |
+    And the following "courses" exist:
+      | fullname | shortname | category |
+      | Course 1 | C1        | 0        |
+    And the following "course enrolments" exist:
+      | user     | course | role           |
+      | teacher  | C1     | editingteacher |
+      | student  | C1     | student        |
+    And the following "activities" exist:
+      | activity   | name                       | course | idnumber |
+      | quiz       | Quiz for testing regrading | C1     | quiz1    |
+    And the following "question categories" exist:
+      | contextlevel    | reference | name           |
+      | Activity module | quiz1     | Test questions |
+    And the following "questions" exist:
+      | questioncategory | qtype       | template    | name  |
+      | Test questions   | multichoice | one_of_four | MC    |
+    And quiz "Quiz for testing regrading" contains the following questions:
+      | question | page | maxmark |
+      | MC       | 1    | 10.0    |
+    And user "student" has attempted "Quiz for testing regrading" with responses:
+      | slot | response |
+      |   1  | B        |
+
+  Scenario: Try a regrade after the question has been edited to have a different number of choices
+    # Edit the question so that V2 has the fourth choice removed.
+    Given I am on the "MC" "core_question > edit" page logged in as teacher
+    And I set the following fields to these values:
+      | Choice 4      |  |
+      | id_feedback_3 |  |
+    And I press "id_submitbutton"
+
+    # Try a regrade, and verify what happened is reported.
+    When I am on the "Quiz for testing regrading" "mod_quiz > grades report" page
+    And I press "Regrade all"
+
+    Then I should see "Quiz for testing regrading"
+    And I should see "The following questions could not be regraded in attempt 1 by Student One"
+    And I should see "Slot 1: The number of choices in the question has changed."
+    And I should see "Finished regrading (1/1)"
+    And I should see "Regrade completed"
+    And I press "Continue"
+
+    # These next tests just serve to check we got back to the report.
+    And I should see "Quiz for testing regrading"
+    And I should see "Overall number of students achieving grade ranges"

question/type/ddimageortext/lang/en/qtype_ddimageortext.php

@@ -28,6 +28,7 @@
 $string['bgimage'] = 'Background image';
 $string['blank'] = 'blank';
 $string['correctansweris'] = 'The correct answer is: {$a}';
+$string['deletedchoice'] = '[Deleted choice]';
 $string['draggableimage'] = 'Draggable image';
 $string['draggableitem'] = 'Draggable item';
 $string['draggableitems'] = 'Draggable items';
@@ -75,4 +76,3 @@
 $string['summariseplaceno'] = 'Drop zone {$a}';
 $string['xleft'] = 'Left';
 $string['ytop'] = 'Top';
-$string['deletedchoice'] = '[Deleted choice]';

question/type/multichoice/lang/en/qtype_multichoice.php

@@ -72,6 +72,7 @@
 $string['privacy:preference:shuffleanswers'] = 'Whether the answers should be automatically shuffled.';
 $string['privacy:preference:answernumbering'] = 'Which numbering style should be used (\'1, 2, 3, ...\', \'a, b, c, ...\' etc.).';
 $string['privacy:preference:showstandardinstruction'] = 'Whether showing standard instruction.';
+$string['regradeissuenumchoiceschanged'] = 'The number of choices in the question has changed.';
 $string['selectmulti'] = 'Select one or more:';
 $string['selectone'] = 'Select one:';
 $string['shuffleanswers'] = 'Shuffle the choices?';

question/type/multichoice/question.php

@@ -89,6 +89,34 @@ public function apply_attempt_state(question_attempt_step $step) {
         }
     }
 
+    public function validate_can_regrade_with_other_version(question_definition $otherversion): ?string {
+        $basemessage = parent::validate_can_regrade_with_other_version($otherversion);
+        if ($basemessage) {
+            return $basemessage;
+        }
+
+        if (count($this->answers) != count($otherversion->answers)) {
+            return get_string('regradeissuenumchoiceschanged', 'qtype_multichoice');
+        }
+
+        return null;
+    }
+
+    public function update_attempt_state_data_for_new_version(
+            question_attempt_step $oldstep, question_definition $otherversion) {
+        parent::update_attempt_state_data_for_new_version($oldstep, $otherversion);
+
+        $mapping = array_combine(array_keys($otherversion->answers), array_keys($this->answers));
+
+        $oldorder = explode(',', $oldstep->get_qt_var('_order'));
+        $neworder = [];
+        foreach ($oldorder as $oldid) {
+            $neworder[] = $mapping[$oldid] ?? $oldid;
+        }
+
+        return ['_order' => implode(',', $neworder)];
+    }
+
     public function get_question_summary() {
         $question = $this->html_to_text($this->questiontext, $this->questiontextformat);
         $choices = array();
@@ -203,9 +231,19 @@ public function summarise_response(array $response) {
         if (!$this->is_complete_response($response)) {
             return null;
         }
-        $ansid = $this->order[$response['answer']];
-        return $this->html_to_text($this->answers[$ansid]->answer,
-                $this->answers[$ansid]->answerformat);
+        $answerid = $this->order[$response['answer']];
+        return $this->html_to_text($this->answers[$answerid]->answer,
+                $this->answers[$answerid]->answerformat);
+    }
+
+    public function un_summarise_response(string $summary) {
+        foreach ($this->order as $key => $answerid) {
+            if ($summary === $this->html_to_text($this->answers[$answerid]->answer,
+                    $this->answers[$answerid]->answerformat)) {
+                return ['answer' => $key];
+            }
+        }
+        return [];
     }
 
     public function classify_response(array $response) {
@@ -370,6 +408,20 @@ public function summarise_response(array $response) {
         return implode('; ', $selectedchoices);
     }
 
+    public function un_summarise_response(string $summary) {
+        // This implementation is not perfect. It will fail if an answer contains '; ',
+        // but this method is only for testing, so it is good enough.
+        $selectedchoices = explode('; ', $summary);
+        $response = [];
+        foreach ($this->order as $key => $answerid) {
+            if (in_array($this->html_to_text($this->answers[$answerid]->answer,
+                    $this->answers[$answerid]->answerformat), $selectedchoices)) {
+                $response[$this->field($key)] = '1';
+            }
+        }
+        return $response;
+    }
+
     public function classify_response(array $response) {
         $selectedchoices = array();
         foreach ($this->order as $key => $ansid) {

question/type/multichoice/tests/question_multi_test.php

@@ -33,6 +33,7 @@
  *
  * @copyright 2009 The Open University
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers \qtype_multichoice_multi_question
  */
 class qtype_multichoice_multi_question_test extends advanced_testcase {
 
@@ -127,8 +128,7 @@ public function test_summarise_response() {
         $mc->shuffleanswers = false;
         $mc->start_attempt(new question_attempt_step(), 1);
 
-        $summary = $mc->summarise_response($mc->prepare_simulated_post_data(array('B' => 1, 'C' => 1)),
-                test_question_maker::get_a_qa($mc));
+        $summary = $mc->summarise_response($mc->prepare_simulated_post_data(['B' => 1, 'C' => 1]));
 
         $this->assertEquals('B; C', $summary);
     }
@@ -138,12 +138,21 @@ public function test_summarise_response_clearchoice() {
         $mc->shuffleanswers = false;
         $mc->start_attempt(new question_attempt_step(), 1);
 
-        $summary = $mc->summarise_response($mc->prepare_simulated_post_data(array('clearchoice' => -1)),
-            test_question_maker::get_a_qa($mc));
+        $summary = $mc->summarise_response($mc->prepare_simulated_post_data(['clearchoice' => -1]));
 
         $this->assertNull($summary);
     }
 
+    public function test_un_summarise_response() {
+        $mc = test_question_maker::make_a_multichoice_multi_question();
+        $mc->shuffleanswers = false;
+        $mc->start_attempt(new question_attempt_step(), 1);
+
+        $this->assertEquals(['choice1' => '1', 'choice2' => '1'], $mc->un_summarise_response('B; C'));
+
+        $this->assertEquals([], $mc->un_summarise_response(''));
+    }
+
     public function test_classify_response() {
         $mc = test_question_maker::make_a_multichoice_multi_question();
         $mc->start_attempt(new question_attempt_step(), 1);

question/type/multichoice/tests/question_single_test.php

@@ -33,6 +33,7 @@
  *
  * @copyright 2009 The Open University
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers \qtype_multichoice_single_question
  */
 class qtype_multichoice_single_question_test extends advanced_testcase {
 
@@ -159,8 +160,18 @@ public function test_summarise_response() {
 
         $this->assertEquals('A', $summary);
 
-        $this->assertNull($mc->summarise_response(array(), test_question_maker::get_a_qa($mc)));
-        $this->assertNull($mc->summarise_response(array('answer' => '-1'), test_question_maker::get_a_qa($mc)));
+        $this->assertNull($mc->summarise_response([]));
+        $this->assertNull($mc->summarise_response(['answer' => '-1']));
+    }
+
+    public function test_un_summarise_response() {
+        $mc = test_question_maker::make_a_multichoice_single_question();
+        $mc->shuffleanswers = false;
+        $mc->start_attempt(new question_attempt_step(), 1);
+
+        $this->assertEquals(['answer' => '1'], $mc->un_summarise_response('B'));
+
+        $this->assertEquals([], $mc->un_summarise_response(''));
     }
 
     public function test_classify_response() {
@@ -209,4 +220,61 @@ public function test_simulated_post_data() {
         $reconstucted = $mc->prepare_simulated_post_data($simulated);
         $this->assertEquals($originalresponse, $reconstucted);
     }
+
+    public function test_validate_can_regrade_with_other_version_bad() {
+        $mc = test_question_maker::make_a_multichoice_single_question();
+
+        $newmc = clone($mc);
+        $newmc->answers = array(
+            23 => new question_answer(13, 'A', 1, 'A is right', FORMAT_HTML),
+            24 => new question_answer(14, 'B', -0.3333333, 'B is wrong', FORMAT_HTML),
+        );
+
+        $this->assertEquals(get_string('regradeissuenumchoiceschanged', 'qtype_multichoice'),
+                $newmc->validate_can_regrade_with_other_version($mc));
+    }
+
+    public function test_validate_can_regrade_with_other_version_ok() {
+        $mc = test_question_maker::make_a_multichoice_single_question();
+
+        $newmc = clone($mc);
+        $newmc->answers = array(
+            23 => new question_answer(13, 'A', 1, 'A is right', FORMAT_HTML),
+            24 => new question_answer(14, 'B', -0.3333333, 'B is wrong', FORMAT_HTML),
+            25 => new question_answer(15, 'C', -0.3333333, 'C is wrong', FORMAT_HTML),
+        );
+
+        $this->assertNull($newmc->validate_can_regrade_with_other_version($mc));
+    }
+
+    public function test_update_attempt_state_date_from_old_version_bad() {
+        $mc = test_question_maker::make_a_multichoice_single_question();
+
+        $newmc = clone($mc);
+        $newmc->answers = array(
+            23 => new question_answer(13, 'A', 1, 'A is right', FORMAT_HTML),
+            24 => new question_answer(14, 'B', -0.3333333, 'B is wrong', FORMAT_HTML),
+        );
+
+        $oldstep = new question_attempt_step();
+        $oldstep->set_qt_var('_order', '14,13,15');
+        $this->expectExceptionMessage(get_string('regradeissuenumchoiceschanged', 'qtype_multichoice'));
+        $newmc->update_attempt_state_data_for_new_version($oldstep, $mc);
+    }
+
+    public function test_update_attempt_state_date_from_old_version_ok() {
+        $mc = test_question_maker::make_a_multichoice_single_question();
+
+        $newmc = clone($mc);
+        $newmc->answers = array(
+            23 => new question_answer(13, 'A', 1, 'A is right', FORMAT_HTML),
+            24 => new question_answer(14, 'B', -0.3333333, 'B is wrong', FORMAT_HTML),
+            25 => new question_answer(15, 'C', -0.3333333, 'C is wrong', FORMAT_HTML),
+        );
+
+        $oldstep = new question_attempt_step();
+        $oldstep->set_qt_var('_order', '14,13,15');
+        $this->assertEquals(['_order' => '24,23,25'],
+                $newmc->update_attempt_state_data_for_new_version($oldstep, $mc));
+    }
 }

question/type/multichoice/tests/walkthrough_test.php

@@ -69,6 +69,27 @@ public function test_deferredfeedback_feedback_multichoice_single() {
                 $this->get_contains_correct_expectation(),
                 new \question_pattern_expectation('/class="r0 correct"/'),
                 new \question_pattern_expectation('/class="r1"/'));
+
+        // Regrade with a new version of the question.
+        $newmc = \test_question_maker::make_a_multichoice_single_question();
+        $newmc->answers = [
+            23 => $newmc->answers[13],
+            24 => $newmc->answers[14],
+            25 => $newmc->answers[15],
+        ];
+        $newmc->answers[23]->fraction = 0.5;
+        $newmc->answers[23]->feedback = 'A is now only partially right';
+        $newmc->answers[24]->fraction = 1;
+        $newmc->answers[24]->answer = 'B is the new right answer';
+        $this->quba->regrade_question($this->slot, true, null, $newmc);
+
+        // Verify.
+        $this->check_current_mark(1.5);
+        $this->render();
+        $this->assertStringContainsString('A is now only partially right', $this->currentoutput);
+        $this->assertStringContainsString('B is the new right answer', $this->currentoutput);
+        $this->assertStringNotContainsString(
+                get_string('deletedchoice', 'qtype_multichoice'), $this->currentoutput);
     }
 
     public function test_deferredfeedback_feedback_multichoice_single_showstandardunstruction_yes() {