From 795e8ac70a0b098ae82ff3569e9e97e33aef5009 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 2 Mar 2023 15:12:49 +0000 Subject: [PATCH 1/2] MDL-77464 questions: update comments in question_attempt_step The class comment had some inaccuracies which I fixed. I also replaced all uses of @link with @see. --- question/engine/questionattemptstep.php | 66 ++++++++++++++----------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index 66969d46012..8b4c39501d7 100644 --- a/question/engine/questionattemptstep.php +++ b/question/engine/questionattemptstep.php @@ -28,10 +28,10 @@ defined('MOODLE_INTERNAL') || die(); /** - * Stores one step in a {@link question_attempt}. + * Stores one step in a {@see question_attempt}. * * The most important attributes of a step are the state, which is one of the - * {@link question_state} constants, the fraction, which may be null, or a + * {@see question_state} constants, the fraction, which may be null, or a * number bewteen the attempt's minfraction and maxfraction, and the array of submitted * data, about which more later. * @@ -39,8 +39,7 @@ defined('MOODLE_INTERNAL') || die(); * creating it. * * The submitted data is basically just an array of name => value pairs, with - * certain conventions about the to divide the variables into four = two times two - * categories. + * certain conventions about the to divide the variables into five = 2 x 2 + 1 categories. * * Variables may either belong to the behaviour, in which case the * name starts with a '-', or they may belong to the question type in which case @@ -50,15 +49,21 @@ defined('MOODLE_INTERNAL') || die(); * which case the name does not start with an _, or they are cached values that * were created during processing, in which case the name does start with an _. * - * That is, each name will start with one of '', '_'. '-' or '-_'. The remainder - * of the name should match the regex [a-z][a-z0-9]*. + * In addition, we can store 'metadata', typically only in the first step of a + * question attempt. These are stored with the initial characters ':_'. * - * These variables can be accessed with {@link get_behaviour_var()} and {@link get_qt_var()}, + * That is, each name will start with one of '', '_', '-', '-_' or ':_'. The remainder + * of the name was supposed to match the regex [a-z][a-z0-9]* - but this has never + * been enforced. Question types exist which break this rule. E.g. qtype_combined. + * Perhpas now, an accurate regex would be [a-z][a-z0-9_:]*. + * + * These variables can be accessed with {@see get_behaviour_var()} and {@see get_qt_var()}, * - to be clear, ->get_behaviour_var('x') gets the variable with name '-x' - - * and values whose names start with '_' can be set using {@link set_behaviour_var()} - * and {@link set_qt_var()}. There are some other methods like {@link has_behaviour_var()} - * to check wether a varaible with a particular name is set, and {@link get_behaviour_data()} - * to get all the behaviour data as an associative array. + * and values whose names start with '_' can be set using {@see set_behaviour_var()} + * and {@see set_qt_var()}. There are some other methods like {@see has_behaviour_var()} + * to check wether a varaible with a particular name is set, and {@see get_behaviour_data()} + * to get all the behaviour data as an associative array. There are also + * {@see get_metadata_var()}, {@see set_metadata_var()} and {@see has_metadata_var()}, * * @copyright 2009 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -71,7 +76,7 @@ class question_attempt_step { private $id = null; /** - * @var question_state one of the {@link question_state} constants. + * @var question_state one of the {@see question_state} constants. * The state after this step. */ private $state; @@ -91,7 +96,7 @@ class question_attempt_step { /** @var array name => value pairs. The submitted data. */ private $data; - /** @var array name => array of {@link stored_file}s. Caches the contents of file areas. */ + /** @var array name => array of {@see stored_file}s. Caches the contents of file areas. */ private $files = array(); /** @var stdClass User information. */ @@ -99,8 +104,8 @@ class question_attempt_step { /** * You should not need to call this constructor in your own code. Steps are - * normally created by {@link question_attempt} methods like - * {@link question_attempt::process_action()}. + * normally created by {@see question_attempt} methods like + * {@see question_attempt::process_action()}. * @param array $data the submitted data that defines this step. * @param int $timestamp the time to record for the action. (If not given, use now.) * @param int $userid the user to attribute the aciton to. (If not given, use the current user.) @@ -147,7 +152,7 @@ class question_attempt_step { /** * Set the state. Normally only called by behaviours. - * @param question_state $state one of the {@link question_state} constants. + * @param question_state $state one of the {@see question_state} constants. */ public function set_state($state) { $this->state = $state; @@ -248,7 +253,8 @@ class question_attempt_step { * type question_attempt::PARAM_FILES. * * @param string $name the name of the associated variable. - * @return array of {@link stored_files}. + * @param int $contextid contextid of the question attempt + * @return array of {@see stored_files}. */ public function get_qt_files($name, $contextid) { if (array_key_exists($name, $this->files)) { @@ -300,7 +306,7 @@ class question_attempt_step { /** * Rewrite the @@PLUGINFILE@@ tokens in a response variable from this step * that contains links to file. Normally you should probably call - * {@link question_attempt::rewrite_response_pluginfile_urls()} instead of + * {@see question_attempt::rewrite_response_pluginfile_urls()} instead of * calling this method directly. * * @param string $text the text to update the URLs in. @@ -379,7 +385,7 @@ class question_attempt_step { /** * Get all the submitted data, but not the cached data. behaviour * variables have the - at the start of their name. This is only really - * intended for use by {@link question_attempt::regrade()}, it should not + * intended for use by {@see question_attempt::regrade()}, it should not * be considered part of the public API. * @param array name => value pairs. */ @@ -397,7 +403,7 @@ class question_attempt_step { /** * Get all the data. behaviour variables have the - at the start of * their name. This is only intended for internal use, for example by - * {@link question_engine_data_mapper::insert_question_attempt_step()}, + * {@see question_engine_data_mapper::insert_question_attempt_step()}, * however, it can occasionally be useful in test code. It should not be * considered part of the public API of this class. * @param array name => value pairs. @@ -410,7 +416,7 @@ class question_attempt_step { * Set a metadata variable. * * Do not call this method directly from your code. It is for internal - * use only. You should call {@link question_usage::set_question_attempt_metadata()}. + * use only. You should call {@see question_usage::set_question_attempt_metadata()}. * * @param string $name the name of the variable to set. [a-z][a-z0-9]*. * @param string $value the value to set. @@ -423,7 +429,7 @@ class question_attempt_step { * Whether this step has a metadata variable. * * Do not call this method directly from your code. It is for internal - * use only. You should call {@link question_usage::get_question_attempt_metadata()}. + * use only. You should call {@see question_usage::get_question_attempt_metadata()}. * * @param string $name the name of the variable to set. [a-z][a-z0-9]*. * @return bool the value to set previously, or null if this variable was never set. @@ -436,7 +442,7 @@ class question_attempt_step { * Get a metadata variable. * * Do not call this method directly from your code. It is for internal - * use only. You should call {@link question_usage::get_question_attempt_metadata()}. + * use only. You should call {@see question_usage::get_question_attempt_metadata()}. * * @param string $name the name of the variable to set. [a-z][a-z0-9]*. * @return string the value to set previously, or null if this variable was never set. @@ -510,10 +516,10 @@ class question_attempt_step { /** - * A subclass of {@link question_attempt_step} used when processing a new submission. + * A subclass of {@see question_attempt_step} used when processing a new submission. * * When we are processing some new submitted data, which may or may not lead to - * a new step being added to the {@link question_usage_by_activity} we create an + * a new step being added to the {@see question_usage_by_activity} we create an * instance of this class. which is then passed to the question behaviour and question * type for processing. At the end of processing we then may, or may not, keep it. * @@ -583,7 +589,7 @@ class question_attempt_pending_step extends question_attempt_step { /** - * A subclass of {@link question_attempt_step} that cannot be modified. + * A subclass of {@see question_attempt_step} that cannot be modified. * * @copyright 2009 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -605,8 +611,8 @@ class question_attempt_step_read_only extends question_attempt_step { /** - * A null {@link question_attempt_step} returned from - * {@link question_attempt::get_last_step()} etc. when a an attempt has just been + * A null {@see question_attempt_step} returned from + * {@see question_attempt::get_last_step()} etc. when a an attempt has just been * created and there is no actual step. * * @copyright 2009 The Open University @@ -628,7 +634,7 @@ class question_null_step { /** - * This is an adapter class that wraps a {@link question_attempt_step} and + * This is an adapter class that wraps a {@see question_attempt_step} and * modifies the get/set_*_data methods so that they operate only on the parts * that belong to a particular subquestion, as indicated by an extra prefix. * @@ -687,7 +693,7 @@ class question_attempt_step_subquestion_adapter extends question_attempt_step { * Filter some data to keep only those entries where the key contains * extraprefix, and remove the extra prefix from the reutrned arrary. * @param array $data some of the data stored in this step. - * @return array the data with the keys ajusted using {@link remove_prefix()}. + * @return array the data with the keys ajusted using {@see remove_prefix()}. */ public function filter_array($data) { $result = array(); From bdcf29ab2917b338661025459c8a0f003b6ad64d Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 2 Mar 2023 15:13:56 +0000 Subject: [PATCH 2/2] MDL-77464 questions: regrading was losing question_attempt metadata This is very similar to MDL-77090, but at the time, I missed that this also needed to be handled. (Question metadata is, I think, only used by the quiz 'Try another question like this one' feature.) --- .../behat/attempt_redo_questions.feature | 4 +++ question/engine/questionattempt.php | 9 +++-- question/engine/tests/walkthrough_test.php | 34 ++++++++++++++++++- 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/mod/quiz/tests/behat/attempt_redo_questions.feature b/mod/quiz/tests/behat/attempt_redo_questions.feature index 58e8e950eca..7c68f6b1852 100644 --- a/mod/quiz/tests/behat/attempt_redo_questions.feature +++ b/mod/quiz/tests/behat/attempt_redo_questions.feature @@ -55,6 +55,10 @@ Feature: Allow students to redo questions in a practice quiz, without starting a Then I should see "Finished regrading (1/1)" And I should see "Regrade completed" And I press "Continue" + # Regrade a second time, to ensure the first regrade did not corrupt any data. + And I press "Regrade all" + And I should see "Finished regrading (1/1)" + And I should see "Regrade completed" @javascript Scenario: Start attempt, teacher edits question, redo picks up latest non-draft version diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 92f57afa620..e73ec8ad581 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1472,11 +1472,16 @@ class question_attempt { if ($this->get_question(false) === $otherversion) { return $oldstep->get_all_data(); } else { + // Update the data belonging to the question type by asking the question to do it. $attemptstatedata = $this->get_question(false)->update_attempt_state_data_for_new_version( $oldstep, $otherversion); - foreach ($oldstep->get_behaviour_data() as $name => $value) { - $attemptstatedata['-' . $name] = $value; + // Then copy over all the behaviour and metadata variables. + // This terminology is explained in the class comment on {@see question_attempt_step}. + foreach ($oldstep->get_all_data() as $name => $value) { + if (substr($name, 0, 1) === '-' || substr($name, 0, 2) === ':_') { + $attemptstatedata[$name] = $value; + } } return $attemptstatedata; } diff --git a/question/engine/tests/walkthrough_test.php b/question/engine/tests/walkthrough_test.php index 4dc699db9c9..a834b003aa4 100644 --- a/question/engine/tests/walkthrough_test.php +++ b/question/engine/tests/walkthrough_test.php @@ -134,7 +134,7 @@ class walkthrough_test extends \qbehaviour_walkthrough_test_base { */ public function test_regrading_an_interactive_attempt_while_in_progress() { - // Start at attempt at a matching question. + // Start an attempt at a matching question. $q = test_question_maker::make_question('match'); $this->start_attempt_at_question($q, 'interactive', 1); $this->save_quba(); @@ -156,4 +156,36 @@ class walkthrough_test extends \qbehaviour_walkthrough_test_base { $this->check_step_count(1); $this->check_current_output($this->get_tries_remaining_expectation(1)); } + + /** + * @covers \question_usage_by_activity::regrade_question + * @covers \question_attempt::regrade + * @covers \question_attempt::get_attempt_state_data_to_regrade_with_version + */ + public function test_regrading_does_not_lose_metadata() { + + // Start an attempt at a matching question. + $q = test_question_maker::make_question('match'); + $this->start_attempt_at_question($q, 'interactive', 1); + // Like in process_redo_question in mod_quiz. + $this->quba->set_question_attempt_metadata($this->slot, 'originalslot', 42); + $this->save_quba(); + + // Verify. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_step_count(1); + $this->check_current_output($this->get_tries_remaining_expectation(1)); + + // Regrade the attempt. + $reloadedquestion = clone($q); + $this->quba->regrade_question($this->slot, false, null, $reloadedquestion); + + // Verify. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_step_count(1); + $this->check_current_output($this->get_tries_remaining_expectation(1)); + $this->assertEquals(42, $this->quba->get_question_attempt_metadata($this->slot, 'originalslot')); + } }