diff --git a/lang/en/question.php b/lang/en/question.php index 48b6a4b25cb..6c4c48dce0d 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -58,6 +58,7 @@ $string['cannotmovequestion'] = 'You can\'t use this script to move questions th $string['cannotopenforwriting'] = 'Cannot open for writing: {$a}'; $string['cannotpreview'] = 'You can\'t preview these questions!'; $string['cannotread'] = 'Cannot read import file (or file is empty)'; +$string['cannotregradedifferentqtype'] = 'Cannot regrade with a question of a different type.'; $string['cannotretrieveqcat'] = 'Could not retrieve question category'; $string['cannotunhidequestion'] = 'Failed to unhide the question.'; $string['cannotunzip'] = 'Could not unzip file.'; diff --git a/mod/quiz/report/overview/lang/en/quiz_overview.php b/mod/quiz/report/overview/lang/en/quiz_overview.php index 67024620999..852f6806877 100644 --- a/mod/quiz/report/overview/lang/en/quiz_overview.php +++ b/mod/quiz/report/overview/lang/en/quiz_overview.php @@ -63,11 +63,13 @@ $string['regradealldrydo'] = 'Regrade attempts marked as needing regrading ({$a} $string['regradealldrydogroup'] = 'Regrade attempts ({$a->countregradeneeded}) marked as needing regrading in group \'{$a->groupname}\''; $string['regradealldrygroup'] = 'Dry run a full regrade for group \'{$a->groupname}\''; $string['regradeallgroup'] = 'Full regrade for group \'{$a->groupname}\''; -$string['regradecomplete'] = 'Regrade completed successfully'; -$string['regradedsuccessfullyxofy'] = 'Successfully regraded ({$a->done}/{$a->count})'; +$string['regradecomplete'] = 'Regrade completed'; +$string['regradedsuccessfullyxofy'] = 'Finished regrading ({$a->done}/{$a->count})'; $string['regradeheader'] = 'Regrading'; $string['regradeselected'] = 'Regrade selected attempts'; +$string['regradingattemptissue'] = 'Slot {$a->slot}: {$a->reason}'; $string['regradingattemptxofy'] = 'Regrading attempt ({$a->done}/{$a->count})'; +$string['regradingattemptxofyproblem'] = 'The following questions could not be regraded in attempt {$a->attemptnum} by {$a->name} (id {$a->attemptid})'; $string['regradingattemptxofywithdetails'] = 'Regrading attempt ({$a->done}/{$a->count}) - Attempt {$a->attemptnum} by {$a->name} (id {$a->attemptid})'; $string['show'] = 'Show / download'; $string['showattempts'] = 'Only show / download attempts'; diff --git a/mod/quiz/report/overview/report.php b/mod/quiz/report/overview/report.php index b0f66d27853..e8fca33fd0e 100644 --- a/mod/quiz/report/overview/report.php +++ b/mod/quiz/report/overview/report.php @@ -337,8 +337,9 @@ class quiz_overview_report extends quiz_attempts_report { * @param bool $dryrun if true, do a pretend regrade, otherwise do it for real. * @param array $slots if null, regrade all questions, otherwise, just regrade * the questions with those slots. + * @return array messages array with keys slot number, and values reasons why that slot cannot be regraded. */ - public function regrade_attempt($attempt, $dryrun = false, $slots = null) { + public function regrade_attempt($attempt, $dryrun = false, $slots = null): array { global $DB; // Need more time for a quiz with many questions. core_php_time_limit::raise(300); @@ -351,12 +352,19 @@ class quiz_overview_report extends quiz_attempts_report { $slots = $quba->get_slots(); } + $messages = []; $finished = $attempt->state == quiz_attempt::FINISHED; foreach ($slots as $slot) { $qqr = new stdClass(); $qqr->oldfraction = $quba->get_question_fraction($slot); $otherquestionversion = $this->get_new_question_for_regrade($attempt, $quba, $slot); + $message = $quba->validate_can_regrade_with_other_version($slot, $otherquestionversion); + if ($message) { + $messages[$slot] = $message; + continue; + } + $quba->regrade_question($slot, $finished, null, $otherquestionversion); $qqr->newfraction = $quba->get_question_fraction($slot); @@ -391,6 +399,7 @@ class quiz_overview_report extends quiz_attempts_report { $quba = null; $transaction = null; gc_collect_cycles(); + return $messages; } /** @@ -555,6 +564,7 @@ class quiz_overview_report extends quiz_attempts_report { */ protected function regrade_batch_of_attempts($quiz, array $attempts, bool $dryrun, \core\dml\sql_join $groupstudentsjoins) { + global $OUTPUT; $this->clear_regrade_table($quiz, $groupstudentsjoins); $progressbar = new progress_bar('quiz_overview_regrade', 500, true); @@ -572,7 +582,17 @@ class quiz_overview_report extends quiz_attempts_report { } $progressbar->update($a['done'], $a['count'], get_string('regradingattemptxofywithdetails', 'quiz_overview', $a)); - $this->regrade_attempt($attempt, $dryrun, $attempt->regradeonlyslots); + $messages = $this->regrade_attempt($attempt, $dryrun, $attempt->regradeonlyslots); + if ($messages) { + $items = []; + foreach ($messages as $slot => $message) { + $items[] = get_string('regradingattemptissue', 'quiz_overview', + ['slot' => $slot, 'reason' => $message]); + } + echo $OUTPUT->notification( + html_writer::tag('p', get_string('regradingattemptxofyproblem', 'quiz_overview', $a)) . + html_writer::alist($items), \core\output\notification::NOTIFY_WARNING); + } } $progressbar->update($a['done'], $a['count'], get_string('regradedsuccessfullyxofy', 'quiz_overview', $a)); diff --git a/mod/quiz/report/overview/tests/behat/regrading.feature b/mod/quiz/report/overview/tests/behat/regrading.feature index d12594b2134..d6ab3f47ffd 100644 --- a/mod/quiz/report/overview/tests/behat/regrading.feature +++ b/mod/quiz/report/overview/tests/behat/regrading.feature @@ -52,8 +52,8 @@ Feature: Regrading quiz attempts using the Grades report # Also, nothing has changed in the quiz, so the regrade won't alter any scores, # but this is still a useful test that the regrade process completes without errors. Then I should see "Quiz for testing regrading" - And I should see "Successfully regraded (2/2)" - And I should see "Regrade completed successfully" + And I should see "Finished regrading (2/2)" + And I should see "Regrade completed" And I press "Continue" # These next tests just serve to check we got back to the report. @@ -67,8 +67,8 @@ Feature: Regrading quiz attempts using the Grades report And I press "Regrade selected attempts" Then I should see "Quiz for testing regrading" - And I should see "Successfully regraded (1/1)" - And I should see "Regrade completed successfully" + 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. @@ -88,8 +88,8 @@ Feature: Regrading quiz attempts using the Grades report # Note, the order is not defined, so we can only check part of the message. Then I should see "Quiz for testing regrading" - And I should see "Successfully regraded (2/2)" - And I should see "Regrade completed successfully" + And I should see "Finished regrading (2/2)" + And I should see "Regrade completed" And I press "Continue" And "Student One" row "Regrade" column of "attempts" table should not contain "Needed" @@ -100,8 +100,8 @@ Feature: Regrading quiz attempts using the Grades report And "Student TwoReview attempt" row "Grade/100.00Sort by Grade/100.00 Ascending" column of "attempts" table should contain "90.00/75.00" And I press "Regrade attempts marked as needing regrading (1)" And I should see "Quiz for testing regrading" - And I should see "Successfully regraded (1/1)" - And I should see "Regrade completed successfully" + 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. @@ -117,8 +117,8 @@ Feature: Regrading quiz attempts using the Grades report And I navigate to "Results" in current page administration When I press "Dry run a full regrade" Then I should see "Quiz for testing regrading" - And I should see "Successfully regraded (2/2)" - And I should see "Regrade completed successfully" + And I should see "Finished regrading (2/2)" + And I should see "Regrade completed" And I press "Continue" And I should see "Quiz for testing regrading" And I should see "Overall number of students achieving grade ranges" @@ -132,12 +132,12 @@ Feature: Regrading quiz attempts using the Grades report And I click on "v2 (latest)" "option" And I navigate to "Results" in current page administration And I press "Dry run a full regrade" - And I should see "Regrade completed successfully" + And I should see "Regrade completed" And I press "Continue" And "student1@example.com" row "Regrade" column of "attempts" table should contain "Needed" And "Correct" "icon" should appear before "50.00/0.00" "text" And I press "Regrade all" - And I should see "Regrade completed successfully" + And I should see "Regrade completed" And I press "Continue" Then "student1@example.com" row "Regrade" column of "attempts" table should contain "Done" And "Student OneReview attempt" row "Q. 1/50.00Sort by Q. 1/50.00 Ascending" column of "attempts" table should contain "50.00/0.00" @@ -177,8 +177,8 @@ Feature: Regrading quiz attempts using the Grades report And I click on "Always latest" "option" And I navigate to "Results" in current page administration And I press "Regrade all" - And I should see "Successfully regraded (1/1)" - And I should see "Regrade completed successfully" + And I should see "Finished regrading (1/1)" + And I should see "Regrade completed" And I press "Continue" Then "student3@example.com" row "Q. 1/50.00Sort by Q. 1/50.00 Ascending" column of "attempts" table should contain "50.00/0.00" And "Incorrect" "icon" should appear before "50.00/0.00" "text" @@ -218,8 +218,8 @@ Feature: Regrading quiz attempts using the Grades report And "student3@example.com" row "Q. 1/100.00Sort by Q. 1/100.00 Ascending" column of "attempts" table should contain "100.00" And "Correct" "icon" should be visible And I press "Regrade all" - And I should see "Successfully regraded (1/1)" - And I should see "Regrade completed successfully" + And I should see "Finished regrading (1/1)" + And I should see "Regrade completed" And I press "Continue" Then "student3@example.com" row "Q. 1/100.00Sort by Q. 1/100.00 Ascending" column of "attempts" table should contain "100.00/0.00" And "Incorrect" "icon" should be visible diff --git a/mod/quiz/upgrade.txt b/mod/quiz/upgrade.txt index 2419d36bf44..17a73e8d847 100644 --- a/mod/quiz/upgrade.txt +++ b/mod/quiz/upgrade.txt @@ -1,5 +1,13 @@ This files describes API changes in the quiz code. +=== 4.0.2, 4.1 === + +* No external code should be calling quiz_overview_report::regrade_attempt because it is an + internal method of the quiz_overview plugin. But if you are incorrectly using it, be aware + that the API changed slightly. It now returns an array listing any questions which could + not be regraded. + + === 4.0 === * The following API methods have a new parameter, $studentisonline, to define whether the student is currently interacting: diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index ba41f67118a..097465c7a42 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1396,6 +1396,16 @@ class question_attempt { $this->process_action(array('-finish' => 1), $timestamp, $userid); } + /** + * Verify if this question_attempt in can be regraded with that other question version. + * + * @param question_definition $otherversion a different version of the question to use in the regrade. + * @return string|null null if the regrade can proceed, else a reason why not. + */ + public function validate_can_regrade_with_other_version(question_definition $otherversion): ?string { + return $this->get_question(false)->validate_can_regrade_with_other_version($otherversion); + } + /** * Perform a regrade. This replays all the actions from $oldqa into this * attempt. @@ -1412,7 +1422,8 @@ class question_attempt { if ($first) { // First step of the attempt. $first = false; - $this->start($oldqa->behaviour, $oldqa->get_variant(), $step->get_all_data(), + $this->start($oldqa->behaviour, $oldqa->get_variant(), + $this->get_attempt_state_data_to_regrade_with_version($step, $oldqa->get_question()), $step->get_timecreated(), $step->get_user_id(), $step->get_id()); } else if ($step->has_behaviour_var('finish') && count($step->get_submitted_data()) > 1) { @@ -1446,6 +1457,27 @@ class question_attempt { $this->set_flagged($oldqa->is_flagged()); } + /** + * Helper used by regrading. + * + * Get the data from the first step of the old attempt and, if necessary, + * update it to be suitable for use with the other version of the question. + * + * @param question_attempt_step $oldstep First step at an attempt at $otherversion of this question. + * @param question_definition $otherversion Another version of the question being attempted. + * @return array updated data required to restart an attempt with the current version of this question. + */ + protected function get_attempt_state_data_to_regrade_with_version(question_attempt_step $oldstep, + question_definition $otherversion): array { + if ($this->get_question(false) === $otherversion) { + return $oldstep->get_all_data(); + } else { + $attemptstatedata = $this->get_question(false)->update_attempt_state_data_for_new_version( + $oldstep, $otherversion); + return array_merge($attemptstatedata, $oldstep->get_behaviour_data()); + } + } + /** * Change the max mark for this question_attempt. * @param float $maxmark the new max mark. diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index 291285684f5..116eca571de 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -875,9 +875,21 @@ class question_usage_by_activity { $this->observer->notify_attempt_modified($qa); } + /** + * Verify if the question_attempt in the given slot can be regraded with that other question version. + * + * @param int $slot the number used to identify this question within this usage. + * @param question_definition $otherversion a different version of the question to use in the regrade. + * @return string|null null if the regrade can proceed, else a reason why not. + */ + public function validate_can_regrade_with_other_version(int $slot, question_definition $otherversion): ?string { + return $this->get_question_attempt($slot)->validate_can_regrade_with_other_version($otherversion); + } + /** * Regrade a question in this usage. This replays the sequence of submitted * actions to recompute the outcomes. + * * @param int $slot the number used to identify this question within this usage. * @param bool $finished whether the question attempt should be forced to be finished * after the regrade, or whether it may still be in progress (default false). diff --git a/question/type/questionbase.php b/question/type/questionbase.php index e7d63600571..874b109406b 100644 --- a/question/type/questionbase.php +++ b/question/type/questionbase.php @@ -211,6 +211,53 @@ abstract class question_definition { public function apply_attempt_state(question_attempt_step $step) { } + /** + * Verify if an attempt at this question can be re-graded using the other question version. + * + * To put it another way, will {@see update_attempt_state_date_from_old_version()} be able to work? + * + * It is expected that this relationship is symmetrical, so if you can regrade from V1 to V3, then + * you can change back from V3 to V1. + * + * @param question_definition $otherversion a different version of the question to use in the regrade. + * @return string|null null if the regrade can proceed, else a reason why not. + */ + public function validate_can_regrade_with_other_version(question_definition $otherversion): ?string { + if (get_class($otherversion) !== get_class($this)) { + return get_string('cannotregradedifferentqtype', 'question'); + } + + return null; + } + + /** + * Update the data representing the initial state of an attempt another version of this question, to allow for the changes. + * + * What is required is probably most easily understood using an example. Think about multiple choice questions. + * The first step has a variable '_order' which is a comma-separated list of question_answer ids. + * A different version of the question will have different question_answers with different ids. However, the list of + * choices should be similar, and so we need to shuffle the new list of ids in the same way that the old one was. + * + * This method should only be called if {@see validate_can_regrade_with_other_version()} did not + * flag up a potential problem. So, this method will throw a {@see coding_exception} if it is not + * possible to work out a return value. + * + * @param question_attempt_step $oldstep the first step of a {@see question_attempt} at $oldquestion. + * @param question_definition $oldquestion the previous version of the question, which $oldstate comes from. + * @return array the submit data which can be passed to {@see apply_attempt_state} to start + * an attempt at this version of this question, corresponding to the attempt at the old question. + * @throws coding_exception if this can't be done. + */ + public function update_attempt_state_data_for_new_version( + question_attempt_step $oldstep, question_definition $oldquestion) { + $message = $this->validate_can_regrade_with_other_version($oldquestion); + if ($message) { + throw new coding_exception($message); + } + + return $oldstep->get_qt_data(); + } + /** * Generate a brief, plain-text, summary of this question. This is used by * various reports. This should show the particular variant of the question diff --git a/question/type/upgrade.txt b/question/type/upgrade.txt index 87f56205535..10d1a0c3ddc 100644 --- a/question/type/upgrade.txt +++ b/question/type/upgrade.txt @@ -1,17 +1,32 @@ This files describes API changes for question type plugins. +=== 4.0.2, 4.1 === + +* There was one issue caused by the changes in Moodle 4.0 which requires changes in question types, + and that is regrading. There are two new methods which some question types will need to implement, + - validate_can_regrade_with_other_version + - update_attempt_state_data_for_new_version + these methods are introduced in this commit, and there are details PHPdoc comments about what they + must do. Then the immediately following commits implement them in the core question types where + they are required. + Generally, you will need to implement one or both of these if you question type does something + significant in the apply_attempt_state method. If you have not implemented that method, then + almost certainly you don't need to worry about this. + + === 4.0 === -1) The major question bank changes should not affect most basic question type plugins. - The navigation changes may affect Behat tests. If you encounter this, - the best way to fix it is to use the new navigation steps in MDL-74130. +* The major question bank changes should not affect most basic question type plugins. + The navigation changes may affect Behat tests. If you encounter this, + the best way to fix it is to use the new navigation steps in MDL-74130. -2) The qualification 'most' is because some question types do more complex things, which - will require changes related to question versionning. Some examples that come to mind: +* The qualification 'most' is because some question types do more complex things, which + will require changes related to question versionning. Some examples that come to mind: - the way qtype_mulitanswer (or qtype_combined) aggregates several sub-questions into a parent question. - the way some contrib plugins (e.g. qtype_stack, qtype_pmatch) store additional data (question tests) linked to questions. That relationship will need to be updated. + === 3.11 === * Introducing the following \question_type base class methods to save/fetch the last form values @@ -42,6 +57,7 @@ This files describes API changes for question type plugins. use it in a question type at https://github.com/moodleou/moodle-qtype_pmatch/commit/2aefa8b5dcc7bab768f4707a4ffb7befcf4c2540. + === 3.8, 3.7.3, 3.6.7 === * Coming up in Moodle 3.8 are some changes to the question bank UI. These will break any