Skip to content

Commit

Permalink
MDL-38538 question auto-save back end.
Browse files Browse the repository at this point in the history
1. Autosave works in some ways just like a normal save. We ultimately
call $behaviour->process_save() to do the work, and create a new step to
hold the data.

2. However, we come in through a completely different route through the
API, starting with separate auto-save methods. This keeps the auto-save
changes mostly separate, and so reduced the chance of breaking existing
working code.

3. When the time comes to store the auto-save step in the database, we
save it using a negative sequence number.

This is a clever trick that not only distinguises these steps, but also
avoids unique key errors when an auto-save and a real action happen
simultaneously. (There are unit tests for these tricky edge cases.)

4. When we load the data back from the database, most of the time the
auto-save steps are loaded back as if they were a real save, and so the
auto-saved data is used when the question is then rendered.

5. However, before we process another action, we remove the auto-saved
step, so it does not appear in the final history.
  • Loading branch information
timhunt committed Mar 28, 2013
1 parent eca230b commit 0a606a2
Show file tree
Hide file tree
Showing 8 changed files with 886 additions and 23 deletions.
36 changes: 36 additions & 0 deletions question/behaviour/behaviourbase.php
Expand Up @@ -435,6 +435,19 @@ protected function is_same_comment($pendingstep) {
*/ */
public abstract function process_action(question_attempt_pending_step $pendingstep); public abstract function process_action(question_attempt_pending_step $pendingstep);


/**
* Auto-saved data. By default this does nothing. interesting processing is
* done in {@link question_behaviour_with_save}.
*
* @param question_attempt_pending_step $pendingstep a partially initialised step
* containing all the information about the action that is being peformed. This
* information can be accessed using {@link question_attempt_step::get_behaviour_var()}.
* @return bool either {@link question_attempt::KEEP} or {@link question_attempt::DISCARD}
*/
public function process_autosave(question_attempt_pending_step $pendingstep) {
return question_attempt::DISCARD;
}

/** /**
* Implementation of processing a manual comment/grade action that should * Implementation of processing a manual comment/grade action that should
* be suitable for most subclasses. * be suitable for most subclasses.
Expand Down Expand Up @@ -570,6 +583,29 @@ protected function is_complete_response(question_attempt_step $pendingstep) {
return $this->question->is_complete_response($pendingstep->get_qt_data()); return $this->question->is_complete_response($pendingstep->get_qt_data());
} }


public function process_autosave(question_attempt_pending_step $pendingstep) {
// If already finished. Nothing to do.
if ($this->qa->get_state()->is_finished()) {
return question_attempt::DISCARD;
}

// If the new data is the same as we already have, then we don't need it.
if ($this->is_same_response($pendingstep)) {
return question_attempt::DISCARD;
}

// Repeat that test discarding any existing autosaved data.
if ($this->qa->has_autosaved_step()) {
$this->qa->discard_autosaved_step();
if ($this->is_same_response($pendingstep)) {
return question_attempt::DISCARD;
}
}

// OK, we need to save.
return $this->process_save($pendingstep);
}

/** /**
* Implementation of processing a save action that should be suitable for * Implementation of processing a save action that should be suitable for
* most subclasses. * most subclasses.
Expand Down
6 changes: 2 additions & 4 deletions question/engine/datalib.php
Expand Up @@ -63,7 +63,7 @@ class question_engine_data_mapper {
/** /**
* @param moodle_database $db a database connectoin. Defaults to global $DB. * @param moodle_database $db a database connectoin. Defaults to global $DB.
*/ */
public function __construct($db = null) { public function __construct(moodle_database $db = null) {
if (is_null($db)) { if (is_null($db)) {
global $DB; global $DB;
$this->db = $DB; $this->db = $DB;
Expand Down Expand Up @@ -613,12 +613,10 @@ public function load_average_marks(qubaid_condition $qubaids, $slots = null) {
* @return array of question_attempts. * @return array of question_attempts.
*/ */
public function load_attempts_at_question($questionid, qubaid_condition $qubaids) { public function load_attempts_at_question($questionid, qubaid_condition $qubaids) {
global $DB;

$params = $qubaids->from_where_params(); $params = $qubaids->from_where_params();
$params['questionid'] = $questionid; $params['questionid'] = $questionid;


$records = $DB->get_recordset_sql(" $records = $this->db->get_recordset_sql("
SELECT SELECT
quba.contextid, quba.contextid,
quba.preferredbehaviour, quba.preferredbehaviour,
Expand Down
10 changes: 6 additions & 4 deletions question/engine/lib.php
Expand Up @@ -71,10 +71,11 @@ public static function make_questions_usage_by_activity($component, $context) {
/** /**
* Load a {@link question_usage_by_activity} from the database, based on its id. * Load a {@link question_usage_by_activity} from the database, based on its id.
* @param int $qubaid the id of the usage to load. * @param int $qubaid the id of the usage to load.
* @param moodle_database $db a database connectoin. Defaults to global $DB.
* @return question_usage_by_activity loaded from the database. * @return question_usage_by_activity loaded from the database.
*/ */
public static function load_questions_usage_by_activity($qubaid) { public static function load_questions_usage_by_activity($qubaid, moodle_database $db = null) {
$dm = new question_engine_data_mapper(); $dm = new question_engine_data_mapper($db);
return $dm->load_questions_usage_by_activity($qubaid); return $dm->load_questions_usage_by_activity($qubaid);
} }


Expand All @@ -83,9 +84,10 @@ public static function load_questions_usage_by_activity($qubaid) {
* if the usage was newly created by {@link make_questions_usage_by_activity()} * if the usage was newly created by {@link make_questions_usage_by_activity()}
* or loaded from the database using {@link load_questions_usage_by_activity()} * or loaded from the database using {@link load_questions_usage_by_activity()}
* @param question_usage_by_activity the usage to save. * @param question_usage_by_activity the usage to save.
* @param moodle_database $db a database connectoin. Defaults to global $DB.
*/ */
public static function save_questions_usage_by_activity(question_usage_by_activity $quba) { public static function save_questions_usage_by_activity(question_usage_by_activity $quba, moodle_database $db = null) {
$dm = new question_engine_data_mapper(); $dm = new question_engine_data_mapper($db);
$observer = $quba->get_observer(); $observer = $quba->get_observer();
if ($observer instanceof question_engine_unit_of_work) { if ($observer instanceof question_engine_unit_of_work) {
$observer->save($dm); $observer->save($dm);
Expand Down
93 changes: 87 additions & 6 deletions question/engine/questionattempt.php
Expand Up @@ -118,6 +118,12 @@ class question_attempt {
/** @var array of {@link question_attempt_step}s. The steps in this attempt. */ /** @var array of {@link question_attempt_step}s. The steps in this attempt. */
protected $steps = array(); protected $steps = array();


/**
* @var question_attempt_step if, when we loaded the step from the DB, there was
* an autosaved step, we save a pointer to it here. (It is also added to the $steps array.)
*/
protected $autosavedstep = null;

/** @var boolean whether the user has flagged this attempt within the usage. */ /** @var boolean whether the user has flagged this attempt within the usage. */
protected $flagged = false; protected $flagged = false;


Expand Down Expand Up @@ -362,6 +368,14 @@ public function get_last_step() {
return end($this->steps); return end($this->steps);
} }


/**
* @return boolean whether this question_attempt has autosaved data from
* some time in the past.
*/
public function has_autosaved_step() {
return !is_null($this->autosavedstep);
}

/** /**
* @return question_attempt_step_iterator for iterating over the steps in * @return question_attempt_step_iterator for iterating over the steps in
* this attempt, in order. * this attempt, in order.
Expand Down Expand Up @@ -788,6 +802,31 @@ protected function add_step(question_attempt_step $step) {
$this->observer->notify_step_added($step, $this, key($this->steps)); $this->observer->notify_step_added($step, $this, key($this->steps));
} }


/**
* Add an auto-saved step to this question attempt. We mark auto-saved steps by
* changing saving the step number with a - sign.
* @param question_attempt_step $step the new step.
*/
protected function add_autosaved_step(question_attempt_step $step) {
$this->steps[] = $step;
$this->autosavedstep = $step;
end($this->steps);
$this->observer->notify_step_added($step, $this, -key($this->steps));
}

/**
* Discard any auto-saved data belonging to this question attempt.
*/
public function discard_autosaved_step() {
if (!$this->has_autosaved_step()) {
return;
}

$autosaved = array_pop($this->steps);
$this->autosavedstep = null;
$this->observer->notify_step_deleted($autosaved, $this);
}

/** /**
* Use a strategy to pick a variant. * Use a strategy to pick a variant.
* @param question_variant_selection_strategy $variantstrategy a strategy. * @param question_variant_selection_strategy $variantstrategy a strategy.
Expand Down Expand Up @@ -1045,10 +1084,12 @@ public function get_right_answer_summary() {
* Perform the action described by $submitteddata. * Perform the action described by $submitteddata.
* @param array $submitteddata the submitted data the determines the action. * @param array $submitteddata the submitted data the determines the action.
* @param int $timestamp the time to record for the action. (If not given, use now.) * @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.) * @param int $userid the user to attribute the action to. (If not given, use the current user.)
* @param int $existingstepid used by the regrade code.
*/ */
public function process_action($submitteddata, $timestamp = null, $userid = null, $existingstepid = null) { public function process_action($submitteddata, $timestamp = null, $userid = null, $existingstepid = null) {
$pendingstep = new question_attempt_pending_step($submitteddata, $timestamp, $userid, $existingstepid); $pendingstep = new question_attempt_pending_step($submitteddata, $timestamp, $userid, $existingstepid);
$this->discard_autosaved_step();
if ($this->behaviour->process_action($pendingstep) == self::KEEP) { if ($this->behaviour->process_action($pendingstep) == self::KEEP) {
$this->add_step($pendingstep); $this->add_step($pendingstep);
if ($pendingstep->response_summary_changed()) { if ($pendingstep->response_summary_changed()) {
Expand All @@ -1057,6 +1098,22 @@ public function process_action($submitteddata, $timestamp = null, $userid = null
} }
} }


/**
* Process an autosave.
* @param array $submitteddata the submitted data the determines the action.
* @param int $timestamp the time to record for the action. (If not given, use now.)
* @param int $userid the user to attribute the action to. (If not given, use the current user.)
* @return bool whether anything was saved.
*/
public function process_autosave($submitteddata, $timestamp = null, $userid = null) {
$pendingstep = new question_attempt_pending_step($submitteddata, $timestamp, $userid);
if ($this->behaviour->process_autosave($pendingstep) == self::KEEP) {
$this->add_autosaved_step($pendingstep);
return true;
}
return false;
}

/** /**
* Perform a finish action on this question attempt. This corresponds to an * Perform a finish action on this question attempt. This corresponds to an
* external finish action, for example the user pressing Submit all and finish * external finish action, for example the user pressing Submit all and finish
Expand Down Expand Up @@ -1212,6 +1269,7 @@ public static function load_from_records($records, $questionattemptid,


$qa->behaviour = question_engine::make_behaviour( $qa->behaviour = question_engine::make_behaviour(
$record->behaviour, $qa, $preferredbehaviour); $record->behaviour, $qa, $preferredbehaviour);
$qa->observer = $observer;


// If attemptstepid is null (which should not happen, but has happened // If attemptstepid is null (which should not happen, but has happened
// due to corrupt data, see MDL-34251) then the current pointer in $records // due to corrupt data, see MDL-34251) then the current pointer in $records
Expand All @@ -1223,20 +1281,43 @@ public static function load_from_records($records, $questionattemptid,
} }


$i = 0; $i = 0;
$autosavedstep = null;
$autosavedsequencenumber = null;
while ($record && $record->questionattemptid == $questionattemptid && !is_null($record->attemptstepid)) { while ($record && $record->questionattemptid == $questionattemptid && !is_null($record->attemptstepid)) {
$qa->steps[$i] = question_attempt_step::load_from_records($records, $record->attemptstepid); $sequencenumber = $record->sequencenumber;
if ($i == 0) { $nextstep = question_attempt_step::load_from_records($records, $record->attemptstepid);
$question->apply_attempt_state($qa->steps[0]);
if ($sequencenumber < 0) {
if (!$autosavedstep) {
$autosavedstep = $nextstep;
$autosavedsequencenumber = -$sequencenumber;
} else {
// Old redundant data. Mark it for deletion.
$qa->observer->notify_step_deleted($nextstep, $qa);
}
} else {
$qa->steps[$i] = $nextstep;
if ($i == 0) {
$question->apply_attempt_state($qa->steps[0]);
}
$i++;
} }
$i++;
if ($records->valid()) { if ($records->valid()) {
$record = $records->current(); $record = $records->current();
} else { } else {
$record = false; $record = false;
} }
} }


$qa->observer = $observer; if ($autosavedstep) {
if ($autosavedsequencenumber >= $i) {
$qa->autosavedstep = $autosavedstep;
$qa->steps[$i] = $qa->autosavedstep;
} else {
$qa->observer->notify_step_deleted($autosavedstep, $qa);
}
}


return $qa; return $qa;
} }
Expand Down
65 changes: 56 additions & 9 deletions question/engine/questionusage.php
Expand Up @@ -511,7 +511,49 @@ public function start_question_based_on($slot, question_attempt $oldqa) {
* instead of the data from $_POST. * instead of the data from $_POST.
*/ */
public function process_all_actions($timestamp = null, $postdata = null) { public function process_all_actions($timestamp = null, $postdata = null) {
// note: we must not use "question_attempt::get_submitted_var()" because there is no attempt instance!!! foreach ($this->get_slots_in_request($postdata) as $slot) {
if (!$this->validate_sequence_number($slot, $postdata)) {
continue;
}
$submitteddata = $this->extract_responses($slot, $postdata);
$this->process_action($slot, $submitteddata, $timestamp);
}
$this->update_question_flags($postdata);
}

/**
* Process all the question autosave data in the current request.
*
* If there is a parameter slots included in the post data, then only
* those question numbers will be processed, otherwise all questions in this
* useage will be.
*
* This function also does {@link update_question_flags()}.
*
* @param int $timestamp optional, use this timestamp as 'now'.
* @param array $postdata optional, only intended for testing. Use this data
* instead of the data from $_POST.
*/
public function process_all_autosaves($timestamp = null, $postdata = null) {
foreach ($this->get_slots_in_request($postdata) as $slot) {
if (!$this->is_autosave_required($slot, $postdata)) {
continue;
}
$submitteddata = $this->extract_responses($slot, $postdata);
$this->process_autosave($slot, $submitteddata, $timestamp);
}
$this->update_question_flags($postdata);
}

/**
* Get the list of slot numbers that should be processed as part of processing
* the current request.
* @param array $postdata optional, only intended for testing. Use this data
* instead of the data from $_POST.
* @return array of slot numbers.
*/
protected function get_slots_in_request($postdata = null) {
// Note: we must not use "question_attempt::get_submitted_var()" because there is no attempt instance!!!
if (is_null($postdata)) { if (is_null($postdata)) {
$slots = optional_param('slots', null, PARAM_SEQUENCE); $slots = optional_param('slots', null, PARAM_SEQUENCE);
} else if (array_key_exists('slots', $postdata)) { } else if (array_key_exists('slots', $postdata)) {
Expand All @@ -526,14 +568,7 @@ public function process_all_actions($timestamp = null, $postdata = null) {
} else { } else {
$slots = explode(',', $slots); $slots = explode(',', $slots);
} }
foreach ($slots as $slot) { return $slots;
if (!$this->validate_sequence_number($slot, $postdata)) {
continue;
}
$submitteddata = $this->extract_responses($slot, $postdata);
$this->process_action($slot, $submitteddata, $timestamp);
}
$this->update_question_flags($postdata);
} }


/** /**
Expand All @@ -560,6 +595,18 @@ public function process_action($slot, $submitteddata, $timestamp = null) {
$this->observer->notify_attempt_modified($qa); $this->observer->notify_attempt_modified($qa);
} }


/**
* Process an autosave action on a specific question.
* @param int $slot the number used to identify this question within this usage.
* @param $submitteddata the submitted data that constitutes the action.
*/
public function process_autosave($slot, $submitteddata, $timestamp = null) {
$qa = $this->get_question_attempt($slot);
if ($qa->process_autosave($submitteddata, $timestamp)) {
$this->observer->notify_attempt_modified($qa);
}
}

/** /**
* Check that the sequence number, that detects weird things like the student * Check that the sequence number, that detects weird things like the student
* clicking back, is OK. If the sequence check variable is not present, returns * clicking back, is OK. If the sequence check variable is not present, returns
Expand Down

0 comments on commit 0a606a2

Please sign in to comment.