Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

MDL-30484 question engine: don't lose response files when regrading.

The problem was mostly that, in the past, we did not worry if
question_attempt_step.id changed during regrade (because we deleted the
old step row and inserted a new one). However, now that steps can have
associated files, we can't be that slack, becuase the step id is used as
the file itemid.

So, now, we have to update the existing rows during a regrade. We do
this by having the question engine tell the question_engine_unit_of_work
that the step has first been deleted, and then added back. Then we make
the unit-of-work spot that delete + add = update.

This also means that during regrading, we have to pass around some extra
ids so that new steps know the id of the step they are replacing.

Naturally, this requires some quite trickly logic, so I finally got
around to writing unit tests for question_engine_unit_of_work, which is
a good thing.

Along the way I also got around to renaming
question_attempt->set_number_in_usage, which got missed out when
everthing else was renamed to slot ages ago.

Finally, while working on this code, I noticed and fixed some PHPdoc
comments.
  • Loading branch information...
commit 699f075ad246dec5283ce2e8ab6fba790eac1b1a 1 parent d4b3034
@timhunt timhunt authored
View
251 question/engine/datalib.php
@@ -17,6 +17,25 @@
/**
* Code for loading and saving question attempts to and from the database.
*
+ * A note for future reference. This code is pretty efficient but there are two
+ * potential optimisations that could be contemplated, at the cost of making the
+ * code more complex:
+ *
+ * 1. (This is the easier one, but probably not worth doing.) In the unit-of-work
+ * save method, we could get all the ids for steps due to be deleted or modified,
+ * and delete all the question_attempt_step_data for all of those steps in one
+ * query. That would save one DB query for each ->stepsupdated. However that number
+ * is 0 except when re-grading, and when regrading, there are many more inserts
+ * into question_attempt_step_data than deletes, so it is really hardly worth it.
+ *
+ * 2. A more significant optimisation would be to write an efficient
+ * $DB->insert_records($arrayofrecords) method (for example using functions
+ * like pg_copy_from) and then whenever we save stuff (unit_of_work->save and
+ * insert_questions_usage_by_activity) collect together all the records that
+ * need to be inserted into question_attempt_step_data, and insert them with
+ * a single call to $DB->insert_records. This is likely to be the biggest win.
+ * We do a lot of separate inserts into question_attempt_step_data.
+ *
* @package moodlecore
* @subpackage questionengine
* @copyright 2009 The Open University
@@ -76,7 +95,7 @@ public function insert_questions_usage_by_activity(question_usage_by_activity $q
* Store an entire {@link question_attempt} in the database,
* including all the question_attempt_steps that comprise it.
* @param question_attempt $qa the question attempt to store.
- * @param object $context the context of the owning question_usage_by_activity.
+ * @param context $context the context of the owning question_usage_by_activity.
*/
public function insert_question_attempt(question_attempt $qa, $context) {
$record = new stdClass();
@@ -105,14 +124,13 @@ public function insert_question_attempt(question_attempt $qa, $context) {
}
/**
- * Store a {@link question_attempt_step} in the database.
- * @param question_attempt_step $qa the step to store.
+ * Helper method used by insert_question_attempt_step and update_question_attempt_step
+ * @param question_attempt_step $step the step to store.
* @param int $questionattemptid the question attept id this step belongs to.
* @param int $seq the sequence number of this stop.
- * @param object $context the context of the owning question_usage_by_activity.
+ * @return stdClass data to insert into the database.
*/
- public function insert_question_attempt_step(question_attempt_step $step,
- $questionattemptid, $seq, $context) {
+ protected function make_step_record(question_attempt_step $step, $questionattemptid, $seq) {
$record = new stdClass();
$record->questionattemptid = $questionattemptid;
$record->sequencenumber = $seq;
@@ -120,16 +138,23 @@ public function insert_question_attempt_step(question_attempt_step $step,
$record->fraction = $step->get_fraction();
$record->timecreated = $step->get_timecreated();
$record->userid = $step->get_user_id();
+ return $record;
+ }
- $record->id = $this->db->insert_record('question_attempt_steps', $record);
-
+ /**
+ * Helper method used by insert_question_attempt_step and update_question_attempt_step
+ * @param question_attempt_step $step the step to store.
+ * @param int $stepid the id of the step.
+ * @param context $context the context of the owning question_usage_by_activity.
+ */
+ protected function insert_step_data(question_attempt_step $step, $stepid, $context) {
foreach ($step->get_all_data() as $name => $value) {
if ($value instanceof question_file_saver) {
- $value->save_files($record->id, $context);
+ $value->save_files($stepid, $context);
}
$data = new stdClass();
- $data->attemptstepid = $record->id;
+ $data->attemptstepid = $stepid;
$data->name = $name;
$data->value = $value;
$this->db->insert_record('question_attempt_step_data', $data, false);
@@ -137,6 +162,41 @@ public function insert_question_attempt_step(question_attempt_step $step,
}
/**
+ * Store a {@link question_attempt_step} in the database.
+ * @param question_attempt_step $step the step to store.
+ * @param int $questionattemptid the question attept id this step belongs to.
+ * @param int $seq the sequence number of this stop.
+ * @param context $context the context of the owning question_usage_by_activity.
+ */
+ public function insert_question_attempt_step(question_attempt_step $step,
+ $questionattemptid, $seq, $context) {
+
+ $record = $this->make_step_record($step, $questionattemptid, $seq);
+ $record->id = $this->db->insert_record('question_attempt_steps', $record);
+
+ $this->insert_step_data($step, $record->id, $context);
+ }
+
+ /**
+ * Update a {@link question_attempt_step} in the database.
+ * @param question_attempt_step $qa the step to store.
+ * @param int $questionattemptid the question attept id this step belongs to.
+ * @param int $seq the sequence number of this stop.
+ * @param context $context the context of the owning question_usage_by_activity.
+ */
+ public function update_question_attempt_step(question_attempt_step $step,
+ $questionattemptid, $seq, $context) {
+
+ $record = $this->make_step_record($step, $questionattemptid, $seq);
+ $record->id = $step->get_id();
+ $this->db->update_record('question_attempt_steps', $record);
+
+ $this->db->delete_records('question_attempt_step_data',
+ array('attemptstepid' => $record->id));
+ $this->insert_step_data($step, $record->id, $context);
+ }
+
+ /**
* Load a {@link question_attempt_step} from the database.
* @param int $stepid the id of the step to load.
* @param question_attempt_step the step that was loaded.
@@ -727,29 +787,27 @@ protected function delete_attempt_steps_for_mysql($test, $params) {
/**
* Delete all the steps for a question attempt.
* @param int $qaids question_attempt id.
+ * @param context $context the context that the $quba belongs to.
*/
- public function delete_steps_for_question_attempts($qaids, $context) {
- if (empty($qaids)) {
+ public function delete_steps($stepids, $context) {
+ if (empty($stepids)) {
return;
}
- list($test, $params) = $this->db->get_in_or_equal($qaids, SQL_PARAMS_NAMED);
+ list($test, $params) = $this->db->get_in_or_equal($stepids, SQL_PARAMS_NAMED);
- $this->delete_response_files($context->id, "IN (
- SELECT id
- FROM {question_attempt_steps}
- WHERE questionattemptid $test)", $params);
+ if ($deletefiles) {
+ $this->delete_response_files($context->id, $test, $params);
+ }
if ($this->db->get_dbfamily() == 'mysql') {
$this->delete_attempt_steps_for_mysql($test, $params);
return;
}
- $this->db->delete_records_select('question_attempt_step_data', "attemptstepid IN (
- SELECT qas.id
- FROM {question_attempt_steps} qas
- WHERE questionattemptid $test)", $params);
+ $this->db->delete_records_select('question_attempt_step_data',
+ "attemptstepid $test", $params);
$this->db->delete_records_select('question_attempt_steps',
- 'questionattemptid ' . $test, $params);
+ "attemptstepid $test", $params);
}
/**
@@ -953,28 +1011,34 @@ class question_engine_unit_of_work implements question_usage_observer {
protected $modified = false;
/**
- * @var array list of number in usage => {@link question_attempt}s that
+ * @var array list of slot => {@link question_attempt}s that
* were already in the usage, and which have been modified.
*/
protected $attemptsmodified = array();
/**
- * @var array list of number in usage => {@link question_attempt}s that
+ * @var array list of slot => {@link question_attempt}s that
* have been added to the usage.
*/
protected $attemptsadded = array();
/**
- * @var array list of question attempt ids to delete the steps for, before
- * inserting new steps.
+ * @var array of array(question_attempt_step, question_attempt id, seq number)
+ * of steps that have been added to question attempts in this usage.
*/
- protected $attemptstodeletestepsfor = array();
+ protected $stepsadded = array();
/**
- * @var array list of array(question_attempt_step, question_attempt id, seq number)
- * of steps that have been added to question attempts in this usage.
+ * @var array of array(question_attempt_step, question_attempt id, seq number)
+ * of steps that have been modified in their attempt.
*/
- protected $stepsadded = array();
+ protected $stepsmodified = array();
+
+ /**
+ * @var array list of question_attempt_step.id => question_attempt_step of steps
+ * that were previously stored in the database, but which are no longer required.
+ */
+ protected $stepsdeleted = array();
/**
* Constructor.
@@ -989,9 +1053,9 @@ public function notify_modified() {
}
public function notify_attempt_modified(question_attempt $qa) {
- $no = $qa->get_slot();
- if (!array_key_exists($no, $this->attemptsadded)) {
- $this->attemptsmodified[$no] = $qa;
+ $slot = $qa->get_slot();
+ if (!array_key_exists($slot, $this->attemptsadded)) {
+ $this->attemptsmodified[$slot] = $qa;
}
}
@@ -999,27 +1063,121 @@ public function notify_attempt_added(question_attempt $qa) {
$this->attemptsadded[$qa->get_slot()] = $qa;
}
- public function notify_delete_attempt_steps(question_attempt $qa) {
-
+ public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) {
if (array_key_exists($qa->get_slot(), $this->attemptsadded)) {
return;
}
- $qaid = $qa->get_database_id();
- foreach ($this->stepsadded as $key => $stepinfo) {
- if ($stepinfo[1] == $qaid) {
- unset($this->stepsadded[$key]);
+ if (($key = $this->is_step_added($step)) !== false) {
+ return;
+ }
+
+ if (($key = $this->is_step_modified($step)) !== false) {
+ throw new coding_exception('Cannot add a step that has already been modified.');
+ }
+
+ if (($key = $this->is_step_deleted($step)) !== false) {
+ unset($this->stepsdeleted[$step->get_id()]);
+ $this->stepsmodified[] = array($step, $qa->get_database_id(), $seq);
+ return;
+ }
+
+ $stepid = $step->get_id();
+ if ($stepid) {
+ if (array_key_exists($stepid, $this->stepsdeleted)) {
+ unset($this->stepsdeleted[$stepid]);
}
+ $this->stepsmodified[] = array($step, $qa->get_database_id(), $seq);
+
+ } else {
+ $this->stepsadded[] = array($step, $qa->get_database_id(), $seq);
+ }
+ }
+
+ public function notify_step_modified(question_attempt_step $step, question_attempt $qa, $seq) {
+ if (array_key_exists($qa->get_slot(), $this->attemptsadded)) {
+ return;
}
- $this->attemptstodeletestepsfor[$qaid] = 1;
+ if (($key = $this->is_step_added($step)) !== false) {
+ return;
+ }
+
+ if (($key = $this->is_step_deleted($step)) !== false) {
+ throw new coding_exception('Cannot modify a step after it has been deleted.');
+ }
+
+ $stepid = $step->get_id();
+ if (empty($stepid)) {
+ throw new coding_exception('Cannot modify a step that has never been stored in the database.');
+ }
+
+ $this->stepsmodified[] = array($step, $qa->get_database_id(), $seq);
}
- public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) {
+ public function notify_step_deleted(question_attempt_step $step, question_attempt $qa) {
if (array_key_exists($qa->get_slot(), $this->attemptsadded)) {
return;
}
- $this->stepsadded[] = array($step, $qa->get_database_id(), $seq);
+
+ if (($key = $this->is_step_added($step)) !== false) {
+ unset($this->stepsadded[$key]);
+ return;
+ }
+
+ if (($key = $this->is_step_modified($step)) !== false) {
+ unset($this->stepsmodified[$key]);
+ }
+
+ $stepid = $step->get_id();
+ if (empty($stepid)) {
+ return; // Was never in the database.
+ }
+
+ $this->stepsdeleted[$stepid] = $step;
+ }
+
+ /**
+ * @param question_attempt_step $step a step
+ * @return int|false if the step is in the list of steps to be added, return
+ * the key, otherwise return false.
+ */
+ protected function is_step_added(question_attempt_step $step) {
+ foreach ($this->stepsadded as $key => $data) {
+ list($addedstep, $qaid, $seq) = $data;
+ if ($addedstep === $step) {
+ return $key;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * @param question_attempt_step $step a step
+ * @return int|false if the step is in the list of steps to be modified, return
+ * the key, otherwise return false.
+ */
+ protected function is_step_modified(question_attempt_step $step) {
+ foreach ($this->stepsmodified as $key => $data) {
+ list($modifiedstep, $qaid, $seq) = $data;
+ if ($modifiedstep === $step) {
+ return $key;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * @param question_attempt_step $step a step
+ * @return bool whether the step is in the list of steps to be deleted.
+ */
+ protected function is_step_deleted(question_attempt_step $step) {
+ foreach ($this->stepsdeleted as $deletedstep) {
+ if ($deletedstep === $step) {
+ return true;
+ }
+ }
+ return false;
}
/**
@@ -1027,8 +1185,13 @@ public function notify_step_added(question_attempt_step $step, question_attempt
* @param question_engine_data_mapper $dm the mapper to use to update the database.
*/
public function save(question_engine_data_mapper $dm) {
- $dm->delete_steps_for_question_attempts(array_keys($this->attemptstodeletestepsfor),
- $this->quba->get_owning_context());
+ $dm->delete_steps(array_keys($this->stepsdeleted), $this->quba->get_owning_context());
+
+ foreach ($this->stepsmodified as $stepinfo) {
+ list($step, $questionattemptid, $seq) = $stepinfo;
+ $dm->update_question_attempt_step($step, $questionattemptid, $seq,
+ $this->quba->get_owning_context());
+ }
foreach ($this->stepsadded as $stepinfo) {
list($step, $questionattemptid, $seq) = $stepinfo;
View
32 question/engine/questionattempt.php
@@ -187,7 +187,7 @@ public function get_variant() {
* For internal use only.
* @param int $slot
*/
- public function set_number_in_usage($slot) {
+ public function set_slot($slot) {
$this->slot = $slot;
}
@@ -213,6 +213,15 @@ public function set_database_id($id) {
$this->id = $id;
}
+ /**
+ * You should almost certainly not call this method from your code. It is for
+ * internal use only.
+ * @param question_usage_observer that should be used to tracking changes made to this qa.
+ */
+ public function set_observer($observer) {
+ $this->observer = $observer;
+ }
+
/** @return int|string the id of the {@link question_usage_by_activity} we belong to. */
public function get_usage_id() {
return $this->usageid;
@@ -802,9 +811,11 @@ public function select_variant(question_variant_selection_strategy $variantstrat
* @param array $submitteddata optional, used when re-starting to keep the same initial state.
* @param int $timestamp optional, the timstamp to record for this action. Defaults to now.
* @param int $userid optional, the user to attribute this action to. Defaults to the current user.
+ * @param int $existingstepid optional, if this step is going to replace an existing step
+ * (for example, during a regrade) this is the id of the previous step we are replacing.
*/
public function start($preferredbehaviour, $variant, $submitteddata = array(),
- $timestamp = null, $userid = null) {
+ $timestamp = null, $userid = null, $existingstepid = null) {
// Initialise the behaviour.
$this->variant = $variant;
@@ -820,7 +831,7 @@ public function start($preferredbehaviour, $variant, $submitteddata = array(),
$this->minfraction = $this->behaviour->get_min_fraction();
// Initialise the first step.
- $firststep = new question_attempt_step($submitteddata, $timestamp, $userid);
+ $firststep = new question_attempt_step($submitteddata, $timestamp, $userid, $existingstepid);
$firststep->set_state(question_state::$todo);
if ($submitteddata) {
$this->question->apply_attempt_state($firststep);
@@ -1041,8 +1052,8 @@ public function get_right_answer_summary() {
* @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.)
*/
- public function process_action($submitteddata, $timestamp = null, $userid = null) {
- $pendingstep = new question_attempt_pending_step($submitteddata, $timestamp, $userid);
+ public function process_action($submitteddata, $timestamp = null, $userid = null, $existingstepid = null) {
+ $pendingstep = new question_attempt_pending_step($submitteddata, $timestamp, $userid, $existingstepid);
if ($this->behaviour->process_action($pendingstep) == self::KEEP) {
$this->add_step($pendingstep);
if ($pendingstep->response_summary_changed()) {
@@ -1074,13 +1085,14 @@ public function finish($timestamp = null, $userid = null) {
public function regrade(question_attempt $oldqa, $finished) {
$first = true;
foreach ($oldqa->get_step_iterator() as $step) {
+ $this->observer->notify_step_deleted($step, $this);
if ($first) {
$first = false;
$this->start($oldqa->behaviour, $oldqa->get_variant(), $step->get_all_data(),
- $step->get_timecreated(), $step->get_user_id());
+ $step->get_timecreated(), $step->get_user_id(), $step->get_id());
} else {
$this->process_action($step->get_submitted_data(),
- $step->get_timecreated(), $step->get_user_id());
+ $step->get_timecreated(), $step->get_user_id(), $step->get_id());
}
}
if ($finished) {
@@ -1147,7 +1159,7 @@ public function classify_response() {
*
* @param Iterator $records Raw records loaded from the database.
* @param int $questionattemptid The id of the question_attempt to extract.
- * @return question_attempt The newly constructed question_attempt_step.
+ * @return question_attempt The newly constructed question_attempt.
*/
public static function load_from_records($records, $questionattemptid,
question_usage_observer $observer, $preferredbehaviour) {
@@ -1172,7 +1184,7 @@ public static function load_from_records($records, $questionattemptid,
$qa = new question_attempt($question, $record->questionusageid,
null, $record->maxmark + 0);
$qa->set_database_id($record->questionattemptid);
- $qa->set_number_in_usage($record->slot);
+ $qa->set_slot($record->slot);
$qa->variant = $record->variant + 0;
$qa->minfraction = $record->minfraction + 0;
$qa->set_flagged($record->flagged);
@@ -1278,7 +1290,7 @@ public function set_database_id($id) {
public function set_flagged($flagged) {
coding_exception('Cannot modify a question_attempt_with_restricted_history.');
}
- public function set_number_in_usage($slot) {
+ public function set_slot($slot) {
coding_exception('Cannot modify a question_attempt_with_restricted_history.');
}
public function set_question_summary($questionsummary) {
View
17 question/engine/questionattemptstep.php
@@ -98,8 +98,11 @@ class question_attempt_step {
* @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.)
+ * @param int $existingstepid if this step is going to replace an existing step
+ * (for example, during a regrade) this is the id of the previous step we are replacing.
*/
- public function __construct($data = array(), $timecreated = null, $userid = null) {
+ public function __construct($data = array(), $timecreated = null, $userid = null,
+ $existingstepid = null) {
global $USER;
if (!is_array($data)) {
@@ -117,6 +120,18 @@ public function __construct($data = array(), $timecreated = null, $userid = null
} else {
$this->userid = $userid;
}
+
+ if (!is_null($existingstepid)) {
+ $this->id = $existingstepid;
+ }
+ }
+
+ /**
+ * @return int|null The id of this step in the database. null if this step
+ * is not stored in the database.
+ */
+ public function get_id() {
+ return $this->id;
}
/** @return question_state The state after this step. */
View
62 question/engine/questionusage.php
@@ -124,11 +124,6 @@ public function get_id() {
return $this->id;
}
- /** @return question_usage_observer that is tracking changes made to this usage. */
- public function get_observer() {
- return $this->observer;
- }
-
/**
* For internal use only. Used by {@link question_engine_data_mapper} to set
* the id when a usage is saved to the database.
@@ -141,6 +136,23 @@ public function set_id_from_database($id) {
}
}
+ /** @return question_usage_observer that is tracking changes made to this usage. */
+ public function get_observer() {
+ return $this->observer;
+ }
+
+ /**
+ * You should almost certainly not call this method from your code. It is for
+ * internal use only.
+ * @param question_usage_observer that should be used to tracking changes made to this usage.
+ */
+ public function set_observer($observer) {
+ $this->observer = $observer;
+ foreach ($this->questionattempts as $qa) {
+ $qa->set_observer($observer);
+ }
+ }
+
/**
* Add another question to this usage.
*
@@ -159,7 +171,7 @@ public function add_question(question_definition $question, $maxmark = null) {
} else {
$this->questionattempts[] = $qa;
}
- $qa->set_number_in_usage(end(array_keys($this->questionattempts)));
+ $qa->set_slot(end(array_keys($this->questionattempts)));
$this->observer->notify_attempt_added($qa);
return $qa->get_slot();
}
@@ -647,11 +659,10 @@ public function regrade_question($slot, $finished = false, $newmaxmark = null) {
$newmaxmark = $oldqa->get_max_mark();
}
- $this->observer->notify_delete_attempt_steps($oldqa);
-
$newqa = new question_attempt($oldqa->get_question(), $oldqa->get_usage_id(),
$this->observer, $newmaxmark);
$newqa->set_database_id($oldqa->get_database_id());
+ $newqa->set_slot($oldqa->get_slot());
$newqa->regrade($oldqa, $finished);
$this->questionattempts[$slot] = $newqa;
@@ -676,7 +687,7 @@ public function regrade_all_questions($finished = false) {
*
* @param Iterator $records Raw records loaded from the database.
* @param int $questionattemptid The id of the question_attempt to extract.
- * @return question_attempt The newly constructed question_attempt_step.
+ * @return question_usage_by_activity The newly constructed usage.
*/
public static function load_from_records($records, $qubaid) {
$record = $records->current();
@@ -808,19 +819,28 @@ public function notify_attempt_modified(question_attempt $qa);
public function notify_attempt_added(question_attempt $qa);
/**
- * Called we want to delete the old step records for an attempt, prior to
- * inserting newones. This is used by regrading.
- * @param question_attempt $qa the question attempt to delete the steps for.
+ * Called when a new step is added to a question attempt in this usage.
+ * @param question_attempt_step $step the new step.
+ * @param question_attempt $qa the usage it is being added to.
+ * @param int $seq the sequence number of the new step.
*/
- public function notify_delete_attempt_steps(question_attempt $qa);
+ public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq);
/**
- * Called when a new step is added to a question attempt in this usage.
- * @param $step the new step.
- * @param $qa the usage it is being added to.
- * @param $seq the sequence number of the new step.
+ * Called when a new step is updated in a question attempt in this usage.
+ * @param question_attempt_step $step the step that was updated.
+ * @param question_attempt $qa the usage it is being added to.
+ * @param int $seq the sequence number of the new step.
*/
- public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq);
+ public function notify_step_modified(question_attempt_step $step, question_attempt $qa, $seq);
+
+ /**
+ * Called when a new step is updated in a question attempt in this usage.
+ * @param question_attempt_step $step the step to delete.
+ * @param question_attempt $qa the usage it is being added to.
+ */
+ public function notify_step_deleted(question_attempt_step $step, question_attempt $qa);
+
}
@@ -838,8 +858,10 @@ public function notify_attempt_modified(question_attempt $qa) {
}
public function notify_attempt_added(question_attempt $qa) {
}
- public function notify_delete_attempt_steps(question_attempt $qa) {
- }
public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) {
}
+ public function notify_step_modified(question_attempt_step $step, question_attempt $qa, $seq) {
+ }
+ public function notify_step_deleted(question_attempt_step $step, question_attempt $qa) {
+ }
}
View
6 question/engine/simpletest/testquestionattempt.php
@@ -70,8 +70,8 @@ public function test_maxmark_beats_default_mark() {
$this->assertEqual(2, $qa->get_max_mark());
}
- public function test_get_set_number_in_usage() {
- $this->qa->set_number_in_usage(7);
+ public function test_get_set_slot() {
+ $this->qa->set_slot(7);
$this->assertEqual(7, $this->qa->get_slot());
}
@@ -97,7 +97,7 @@ public function test_get_behaviour_field_name() {
}
public function test_get_field_prefix() {
- $this->qa->set_number_in_usage(7);
+ $this->qa->set_slot(7);
$name = $this->qa->get_field_prefix();
$this->assertPattern('/' . preg_quote($this->usageid) . '/', $name);
$this->assertPattern('/' . preg_quote($this->qa->get_slot()) . '/', $name);
View
64 question/engine/simpletest/testquestionusagebyactivity.php
@@ -158,4 +158,66 @@ public function test_access_out_of_sequence_throws_exception() {
$this->expectException('question_out_of_sequence_exception');
$quba->process_all_actions($slot, $postdata);
}
-}
+}
+
+/**
+ * Unit tests for loading data into the {@link question_usage_by_activity} class.
+ *
+ * @copyright 2012 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class question_usage_db_test extends data_loading_method_test_base {
+ public function test_load() {
+ $records = new test_recordset(array(
+ array('qubaid', 'contextid', 'component', 'preferredbehaviour',
+ 'questionattemptid', 'contextid', 'questionusageid', 'slot',
+ 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
+ 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
+ 'attemptstepid', 'sequencenumber', 'state', 'fraction',
+ 'timecreated', 'userid', 'name', 'value'),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'),
+ ));
+
+ $question = test_question_maker::make_question('truefalse', 'true');
+ $question->id = -1;
+
+ question_bank::start_unit_test();
+ question_bank::load_test_question_data($question);
+ $quba = question_usage_by_activity::load_from_records($records, 1);
+ question_bank::end_unit_test();
+
+ $this->assertEqual('unit_test', $quba->get_owning_component());
+ $this->assertEqual(1, $quba->get_id());
+ $this->assertIsA($quba->get_observer(), 'question_engine_unit_of_work');
+ $this->assertEqual('interactive', $quba->get_preferred_behaviour());
+
+ $qa = $quba->get_question_attempt(1);
+
+ $this->assertEqual($question->questiontext, $qa->get_question()->questiontext);
+
+ $this->assertEqual(3, $qa->get_num_steps());
+
+ $step = $qa->get_step(0);
+ $this->assertEqual(question_state::$todo, $step->get_state());
+ $this->assertNull($step->get_fraction());
+ $this->assertEqual(1256233700, $step->get_timecreated());
+ $this->assertEqual(1, $step->get_user_id());
+ $this->assertEqual(array(), $step->get_all_data());
+
+ $step = $qa->get_step(1);
+ $this->assertEqual(question_state::$todo, $step->get_state());
+ $this->assertNull($step->get_fraction());
+ $this->assertEqual(1256233705, $step->get_timecreated());
+ $this->assertEqual(1, $step->get_user_id());
+ $this->assertEqual(array('answer' => '1'), $step->get_all_data());
+
+ $step = $qa->get_step(2);
+ $this->assertEqual(question_state::$gradedright, $step->get_state());
+ $this->assertEqual(1, $step->get_fraction());
+ $this->assertEqual(1256233720, $step->get_timecreated());
+ $this->assertEqual(1, $step->get_user_id());
+ $this->assertEqual(array('-finish' => '1'), $step->get_all_data());
+ }
+}
View
296 question/engine/simpletest/testunitofwork.php
@@ -0,0 +1,296 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * This file contains tests for the question_engine_unit_of_work class.
+ *
+ * @package moodlecore
+ * @subpackage questionengine
+ * @copyright 2012 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+
+defined('MOODLE_INTERNAL') || die();
+
+require_once(dirname(__FILE__) . '/../lib.php');
+require_once(dirname(__FILE__) . '/helpers.php');
+
+
+/**
+ * Test subclass to allow access to some protected data so that the correct
+ * behaviour can be verified.
+ *
+ * @copyright 2012 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class testable_question_engine_unit_of_work extends question_engine_unit_of_work {
+ public function get_modified() {
+ return $this->modified;
+ }
+
+ public function get_attempts_added() {
+ return $this->attemptsadded;
+ }
+
+ public function get_attempts_modified() {
+ return $this->attemptsmodified;
+ }
+
+ public function get_steps_added() {
+ return $this->stepsadded;
+ }
+
+ public function get_steps_modified() {
+ return $this->stepsmodified;
+ }
+
+ public function get_steps_deleted() {
+ return $this->stepsdeleted;
+ }
+}
+
+
+/**
+ * Unit tests for the {@link question_engine_unit_of_work} class.
+ *
+ * @copyright 2012 The Open University
+ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class question_engine_unit_of_work_test extends data_loading_method_test_base {
+ /** @var question_usage_by_activity the test question usage. */
+ protected $quba;
+
+ /** @var int the slot number of the one qa in the test usage.*/
+ protected $slot;
+
+ /** @var testable_question_engine_unit_of_work the unit of work we are testing. */
+ protected $observer;
+
+ public function setUp() {
+ // Create a usage in an initial state, with one shortanswer question added,
+ // and attempted in interactive mode submitted responses 'toad' then 'frog'.
+ // Then set it to use a new unit of work for any subsequent changes.
+ // Create a short answer question.
+ $question = test_question_maker::make_question('shortanswer');
+ $question->hints = array(
+ new question_hint(0, 'This is the first hint.', FORMAT_HTML),
+ new question_hint(0, 'This is the second hint.', FORMAT_HTML),
+ );
+ $question->id = -1;
+ question_bank::start_unit_test();
+ question_bank::load_test_question_data($question);
+
+ $this->setup_initial_test_state($this->get_test_data());
+ }
+
+ public function testDown() {
+ question_bank::end_unit_test();
+ }
+
+ protected function setup_initial_test_state($testdata) {
+ $records = new test_recordset($testdata);
+
+ $this->quba = question_usage_by_activity::load_from_records($records, 1);
+
+ $this->slot = 1;
+ $this->observer = new testable_question_engine_unit_of_work($this->quba);
+ $this->quba->set_observer($this->observer);
+ }
+
+ protected function get_test_data() {
+ return array(
+ array('qubaid', 'contextid', 'component', 'preferredbehaviour',
+ 'questionattemptid', 'contextid', 'questionusageid', 'slot',
+ 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
+ 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
+ 'attemptstepid', 'sequencenumber', 'state', 'fraction',
+ 'timecreated', 'userid', 'name', 'value'),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, '-_triesleft', 3),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233720, 1, 'answer', 'toad'),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233720, 1, '-submit', 1),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233720, 1, '-_triesleft', 1),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 3, 2, 'todo', null, 1256233740, 1, '-tryagain', 1),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', null, 1256233790, 1, 'answer', 'frog'),
+ array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', 1.0000000, 1256233790, 1, '-finish', 1),
+ );
+ }
+
+ public function test_initial_state() {
+ $this->assertFalse($this->observer->get_modified());
+ $this->assertEqual(0, count($this->observer->get_attempts_added()));
+ $this->assertEqual(0, count($this->observer->get_attempts_modified()));
+ $this->assertEqual(0, count($this->observer->get_steps_added()));
+ $this->assertEqual(0, count($this->observer->get_steps_modified()));
+ $this->assertEqual(0, count($this->observer->get_steps_deleted()));
+ }
+
+ public function test_update_usage() {
+
+ $this->quba->set_preferred_behaviour('deferredfeedback');
+
+ $this->assertTrue($this->observer->get_modified());
+ }
+
+ public function test_add_question() {
+
+ $slot = $this->quba->add_question(test_question_maker::make_question('truefalse'));
+
+ $newattempts = $this->observer->get_attempts_added();
+ $this->assertEqual(1, count($newattempts));
+ $this->assertIdentical($this->quba->get_question_attempt($slot), reset($newattempts));
+ $this->assertIdentical($slot, key($newattempts));
+ }
+
+ public function test_add_and_start_question() {
+
+ $slot = $this->quba->add_question(test_question_maker::make_question('truefalse'));
+ $this->quba->start_question($slot);
+
+ // The point here is that, although we have added a step, it is not listed
+ // separately becuase it is part of a newly added attempt, and all steps
+ // for a newly added attempt are automatically added to the DB, so it does
+ // not need to be tracked separately.
+ $newattempts = $this->observer->get_attempts_added();
+ $this->assertEqual(1, count($newattempts));
+ $this->assertIdentical($this->quba->get_question_attempt($slot),
+ reset($newattempts));
+ $this->assertIdentical($slot, key($newattempts));
+ $this->assertEqual(0, count($this->observer->get_steps_added()));
+ }
+
+ public function test_process_action() {
+
+ $this->quba->manual_grade($this->slot, 'Acutally, that is not quite right', 0.5);
+
+ // Here, however, were we are adding a step to an existing qa, we do need to track that.
+ $this->assertEqual(0, count($this->observer->get_attempts_added()));
+
+ $updatedattempts = $this->observer->get_attempts_modified();
+ $this->assertEqual(1, count($updatedattempts));
+
+ $updatedattempt = reset($updatedattempts);
+ $this->assertIdentical($this->quba->get_question_attempt($this->slot), $updatedattempt);
+ $this->assertIdentical($this->slot, key($updatedattempts));
+
+ $newsteps = $this->observer->get_steps_added();
+ $this->assertEqual(1, count($newsteps));
+
+ list($newstep, $qaid, $seq) = reset($newsteps);
+ $this->assertIdentical($this->quba->get_question_attempt($this->slot)->get_last_step(), $newstep);
+ }
+
+ public function test_regrade_same_steps() {
+
+ // Change the question in a minor way and regrade.
+ $this->quba->get_question($this->slot)->answer[14]->fraction = 0.5;
+ $this->quba->regrade_all_questions();
+
+ // Here, the qa, and all the steps, should be marked as updated.
+ // Here, however, were we are adding a step to an existing qa, we do need to track that.
+ $this->assertEqual(0, count($this->observer->get_attempts_added()));
+ $this->assertEqual(0, count($this->observer->get_steps_added()));
+ $this->assertEqual(0, count($this->observer->get_steps_deleted()));
+
+ $updatedattempts = $this->observer->get_attempts_modified();
+ $this->assertEqual(1, count($updatedattempts));
+
+ $updatedattempt = reset($updatedattempts);
+ $this->assertIdentical($this->quba->get_question_attempt($this->slot), $updatedattempt);
+
+ $updatedsteps = $this->observer->get_steps_modified();
+ $this->assertEqual($updatedattempt->get_num_steps(), count($updatedsteps));
+
+ foreach ($updatedattempt->get_step_iterator() as $seq => $step) {
+ $this->assertIdentical(array($step, $updatedattempt->get_database_id(), $seq),
+ $updatedsteps[$seq]);
+ }
+ }
+
+ public function test_regrade_losing_steps() {
+
+ // Change the question so that 'toad' is also right, and regrade. This
+ // will mean that the try again, and second try states are no longer
+ // needed, so they should be dropped.
+ $this->quba->get_question($this->slot)->answers[14]->fraction = 1;
+ $this->quba->regrade_all_questions();
+
+ $this->assertEqual(0, count($this->observer->get_attempts_added()));
+ $this->assertEqual(0, count($this->observer->get_steps_added()));
+
+ $updatedattempts = $this->observer->get_attempts_modified();
+ $this->assertEqual(1, count($updatedattempts));
+
+ $updatedattempt = reset($updatedattempts);
+ $this->assertIdentical($this->quba->get_question_attempt($this->slot), $updatedattempt);
+
+ $updatedsteps = $this->observer->get_steps_modified();
+ $this->assertEqual($updatedattempt->get_num_steps(), count($updatedsteps));
+
+ foreach ($updatedattempt->get_step_iterator() as $seq => $step) {
+ $this->assertIdentical(array($step, $updatedattempt->get_database_id(), $seq),
+ $updatedsteps[$seq]);
+ }
+
+ $deletedsteps = $this->observer->get_steps_deleted();
+ $this->assertEqual(2, count($deletedsteps));
+
+ $firstdeletedstep = reset($deletedsteps);
+ $this->assertEqual(array('-tryagain' => 1), $firstdeletedstep->get_all_data());
+
+ $seconddeletedstep = end($deletedsteps);
+ $this->assertEqual(array('answer' => 'frog', '-finish' => 1),
+ $seconddeletedstep->get_all_data());
+ }
+
+ public function test_tricky_regrade() {
+
+ // The tricky thing here is that we take a half-complete question-attempt,
+ // and then as one transaction, we submit some more responses, and then
+ // change the question attempt as in test_regrade_losing_steps, and regrade
+ // before the steps are even written to the database the first time.
+ $somedata = $this->get_test_data();
+ $somedata = array_slice($somedata, 0, 5);
+ $this->setup_initial_test_state($somedata);
+
+ $this->quba->process_action($this->slot, array('-tryagain' => 1));
+ $this->quba->process_action($this->slot, array('answer' => 'frog', '-submit' => 1));
+ $this->quba->finish_all_questions();
+
+ $this->quba->get_question($this->slot)->answers[14]->fraction = 1;
+ $this->quba->regrade_all_questions();
+
+ $this->assertEqual(0, count($this->observer->get_attempts_added()));
+
+ $updatedattempts = $this->observer->get_attempts_modified();
+ $this->assertEqual(1, count($updatedattempts));
+
+ $updatedattempt = reset($updatedattempts);
+ $this->assertIdentical($this->quba->get_question_attempt($this->slot), $updatedattempt);
+
+ $this->assertEqual(0, count($this->observer->get_steps_added()));
+
+ $updatedsteps = $this->observer->get_steps_modified();
+ $this->assertEqual($updatedattempt->get_num_steps(), count($updatedsteps));
+
+ foreach ($updatedattempt->get_step_iterator() as $seq => $step) {
+ $this->assertIdentical(array($step, $updatedattempt->get_database_id(), $seq),
+ $updatedsteps[$seq]);
+ }
+
+ $this->assertEqual(0, count($this->observer->get_steps_deleted()));
+ }
+}
Please sign in to comment.
Something went wrong with that request. Please try again.