Skip to content

Commit

Permalink
Merge branch 'MDL-77464_400' of https://github.com/timhunt/moodle int…
Browse files Browse the repository at this point in the history 
…o MOODLE_400_STABLE
  • Loading branch information
@sarjona
sarjona committed Mar 8, 2023
2 parents 7601dd8 + 1397ce3 commit 4555270
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 33 deletions.
4 changes: 4 additions & 0 deletions mod/quiz/tests/behat/attempt_redo_questions.feature
View file
Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions question/engine/questionattempt.php
View file
Expand Up @@ -1472,11 +1472,16 @@ protected function get_attempt_state_data_to_regrade_with_version(question_attem
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;
}
Expand Down
66 changes: 36 additions & 30 deletions question/engine/questionattemptstep.php
View file
Expand Up @@ -28,19 +28,18 @@


/**
* 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.
*
* A step also tracks the time it was created, and the user responsible for
* 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
Expand All @@ -50,15 +49,21 @@
* 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
Expand All @@ -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;
Expand All @@ -91,16 +96,16 @@ 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. */
private $user = null;

/**
* 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.)
Expand Down Expand Up @@ -147,7 +152,7 @@ public function get_state() {

/**
* 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;
Expand Down Expand Up @@ -248,7 +253,8 @@ public function set_qt_var($name, $value) {
* 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)) {
Expand Down Expand Up @@ -300,7 +306,7 @@ public function prepare_response_files_draft_itemid_with_text($name, $contextid,
/**
* 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.
Expand Down Expand Up @@ -379,7 +385,7 @@ public function get_behaviour_data() {
/**
* 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.
*/
Expand All @@ -397,7 +403,7 @@ public function get_submitted_data() {
/**
* 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.
Expand All @@ -410,7 +416,7 @@ public function get_all_data() {
* 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.
Expand All @@ -423,7 +429,7 @@ public function set_metadata_var($name, $value) {
* 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.
Expand All @@ -436,7 +442,7 @@ public function has_metadata_var($name) {
* 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.
Expand Down Expand Up @@ -510,10 +516,10 @@ public static function load_from_records($records, $attemptstepid, $qtype = null


/**
* 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.
*
Expand Down Expand Up @@ -583,7 +589,7 @@ public function variant_number_changed() {


/**
* 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
Expand All @@ -605,8 +611,8 @@ public function set_behaviour_var($name, $value) {


/**
* 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
Expand All @@ -628,7 +634,7 @@ public function get_fraction() {


/**
* 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.
*
Expand Down Expand Up @@ -687,7 +693,7 @@ public function remove_prefix($field) {
* 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();
Expand Down
34 changes: 33 additions & 1 deletion question/engine/tests/walkthrough_test.php
View file
Expand Up @@ -134,7 +134,7 @@ public function test_action_author_with_display_options_testcase() {
*/
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();
Expand All @@ -156,4 +156,36 @@ public function test_regrading_an_interactive_attempt_while_in_progress() {
$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'));
}
}

0 comments on commit 4555270

Please sign in to comment.