diff --git a/question/engine/datalib.php b/question/engine/datalib.php index 751a90b7aa0..0c0f2eb8494 100644 --- a/question/engine/datalib.php +++ b/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 @@ class question_engine_data_mapper { * 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 @@ class question_engine_data_mapper { } /** - * 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,22 +138,64 @@ class question_engine_data_mapper { $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); } } + /** + * 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. @@ -727,29 +787,27 @@ ORDER BY /** * 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,29 +1011,35 @@ 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. - */ - protected $attemptstodeletestepsfor = array(); - - /** - * @var array list of array(question_attempt_step, question_attempt id, seq number) + * @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 $stepsadded = array(); + /** + * @var array of array(question_attempt_step, question_attempt id, seq number) + * of steps that have been modified in their attempt. + */ + 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. * @param question_usage_by_activity $quba the usage to track. @@ -989,9 +1053,9 @@ class question_engine_unit_of_work implements question_usage_observer { } 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 @@ class question_engine_unit_of_work implements question_usage_observer { $this->attemptsadded[$qa->get_slot()] = $qa; } - public function notify_delete_attempt_steps(question_attempt $qa) { - - 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]); - } - } - - $this->attemptstodeletestepsfor[$qaid] = 1; - } - public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) { 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) { + 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; + } + + 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_deleted(question_attempt_step $step, question_attempt $qa) { + if (array_key_exists($qa->get_slot(), $this->attemptsadded)) { + return; + } + + 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 @@ class question_engine_unit_of_work implements question_usage_observer { * @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; diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index f31ff3a59e5..bde5115d062 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -187,7 +187,7 @@ class question_attempt { * 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 @@ class question_attempt { $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 @@ class question_attempt { * @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 @@ class question_attempt { $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 @@ class question_attempt { * @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 @@ class question_attempt { 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 @@ class question_attempt { * * @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 @@ class question_attempt { $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 @@ class question_attempt_with_restricted_history extends question_attempt { 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) { diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index 9f0b5c05bcd..4cd813a1132 100644 --- a/question/engine/questionattemptstep.php +++ b/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 @@ class question_attempt_step { } 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. */ diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index 33aeb5fd019..46ea5cf3912 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -124,11 +124,6 @@ class question_usage_by_activity { 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 @@ class question_usage_by_activity { } } + /** @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 @@ class question_usage_by_activity { } 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 @@ class question_usage_by_activity { $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 @@ class question_usage_by_activity { * * @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(); @@ -807,20 +818,29 @@ interface question_usage_observer { */ 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. - */ - public function notify_delete_attempt_steps(question_attempt $qa); - /** * 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. + * @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_step_added(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 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_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 @@ class question_usage_null_observer implements question_usage_observer { } 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) { + } } diff --git a/question/engine/simpletest/testquestionattempt.php b/question/engine/simpletest/testquestionattempt.php index 13873368837..84a4d4679ad 100644 --- a/question/engine/simpletest/testquestionattempt.php +++ b/question/engine/simpletest/testquestionattempt.php @@ -70,8 +70,8 @@ class question_attempt_test extends UnitTestCase { $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 @@ class question_attempt_test extends UnitTestCase { } 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); diff --git a/question/engine/simpletest/testquestionusagebyactivity.php b/question/engine/simpletest/testquestionusagebyactivity.php index bad18547e61..c317b7c3d0f 100644 --- a/question/engine/simpletest/testquestionusagebyactivity.php +++ b/question/engine/simpletest/testquestionusagebyactivity.php @@ -158,4 +158,66 @@ class question_usage_by_activity_test extends UnitTestCase { $this->expectException('question_out_of_sequence_exception'); $quba->process_all_actions($slot, $postdata); } -} \ No newline at end of file +} + +/** + * 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()); + } +} diff --git a/question/engine/simpletest/testunitofwork.php b/question/engine/simpletest/testunitofwork.php new file mode 100644 index 00000000000..e76085c1b64 --- /dev/null +++ b/question/engine/simpletest/testunitofwork.php @@ -0,0 +1,296 @@ +. + +/** + * 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())); + } +}