From 28273694e5ea283a24aaca04248d2dbc5a0c683b Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 17 Jan 2014 11:56:55 +0000 Subject: [PATCH 1/3] MDL-43246 give unit tests class a better name. --- .../tests/{datalib_test.php => qubaid_condition_test.php} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename question/engine/tests/{datalib_test.php => qubaid_condition_test.php} (98%) diff --git a/question/engine/tests/datalib_test.php b/question/engine/tests/qubaid_condition_test.php similarity index 98% rename from question/engine/tests/datalib_test.php rename to question/engine/tests/qubaid_condition_test.php index 79ffb4efce1..123f38700f9 100644 --- a/question/engine/tests/datalib_test.php +++ b/question/engine/tests/qubaid_condition_test.php @@ -31,12 +31,12 @@ require_once(dirname(__FILE__) . '/../lib.php'); /** - * Unit tests for some of the code in ../datalib.php. + * Unit tests for qubaid_condition and subclasses. * * @copyright 2009 The Open University * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class qubaid_condition_test extends advanced_testcase { +class qubaid_condition_testcase extends advanced_testcase { protected function normalize_sql($sql, $params) { $newparams = array(); @@ -158,4 +158,4 @@ class qubaid_condition_test extends advanced_testcase { WHERE qa.questionusageid IN (SELECT ot.usageid FROM {other_table} ot WHERE 1 = 1)", array()); } -} \ No newline at end of file +} From 5b0a31bf20f2773f800bdfc07b20d82e1c376ac3 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 17 Jan 2014 15:36:19 +0000 Subject: [PATCH 2/3] MDL-43246 some question data mapper unit tests With fixes for the issues unearthed by these tests. --- question/engine/datalib.php | 20 +- question/engine/tests/helpers.php | 2 +- .../question_engine_data_mapper_test.php | 314 ++++++++++++++++++ question/type/essay/tests/helper.php | 25 ++ 4 files changed, 351 insertions(+), 10 deletions(-) create mode 100644 question/engine/tests/question_engine_data_mapper_test.php diff --git a/question/engine/datalib.php b/question/engine/datalib.php index d5be452db5f..ba757e6d44e 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -363,7 +363,7 @@ ORDER BY * * @param qubaid_condition $qubaids used to restrict which usages are included * in the query. See {@link qubaid_condition}. - * @param array $slots A list of slots for the questions you want to konw about. + * @param array $slots A list of slots for the questions you want to know about. * @param string|null $fields * @return array of records. See the SQL in this function to see the fields available. */ @@ -510,7 +510,7 @@ ORDER BY */ public function load_questions_usages_where_question_in_state( qubaid_condition $qubaids, $summarystate, $slot, $questionid = null, - $orderby = 'random', $params, $limitfrom = 0, $limitnum = null) { + $orderby = 'random', $params = array(), $limitfrom = 0, $limitnum = null) { $extrawhere = ''; if ($questionid) { @@ -531,14 +531,16 @@ ORDER BY $sqlorderby = ''; } - // We always want the total count, as well as the partcular list of ids, - // based on the paging and sort order. Becuase the list of ids is never - // going to be too rediculously long. My worst-case scenario is - // 10,000 students in the coures, each doing 5 quiz attempts. That + // We always want the total count, as well as the partcular list of ids + // based on the paging and sort order. Because the list of ids is never + // going to be too ridiculously long. My worst-case scenario is + // 10,000 students in the course, each doing 5 quiz attempts. That // is a 50,000 element int => int array, which PHP seems to use 5MB - // memeory to store on a 64 bit server. + // memory to store on a 64 bit server. + $qubaidswhere = $qubaids->where(); // Must call this before params. $params += $qubaids->from_where_params(); $params['slot'] = $slot; + $qubaids = $this->db->get_records_sql_menu(" SELECT qa.questionusageid, @@ -550,7 +552,7 @@ JOIN {question_attempt_steps} qas ON JOIN {question} q ON q.id = qa.questionid WHERE - {$qubaids->where()} AND + {$qubaidswhere} AND qa.slot = :slot $extrawhere @@ -587,7 +589,7 @@ $sqlorderby $slotwhere = " AND qa.slot $slottest"; } else { $slotwhere = ''; - $params = array(); + $slotsparams = array(); } list($statetest, $stateparams) = $this->db->get_in_or_equal(array( diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 69c02d08f33..93c4e3fd9a5 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -251,7 +251,7 @@ class test_question_maker { array($qtype, $which), $methodtemplate); if (!method_exists($helper, $method)) { - throw new coding_exception('Method ' . $method . ' does not exist on the' . + throw new coding_exception('Method ' . $method . ' does not exist on the ' . $qtype . ' question type test helper class.'); } diff --git a/question/engine/tests/question_engine_data_mapper_test.php b/question/engine/tests/question_engine_data_mapper_test.php new file mode 100644 index 00000000000..932576c0396 --- /dev/null +++ b/question/engine/tests/question_engine_data_mapper_test.php @@ -0,0 +1,314 @@ +. + +/** + * This file contains tests for the autosave code in the question_usage class. + * + * @package moodlecore + * @subpackage questionengine + * @copyright 2013 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once(dirname(__FILE__) . '/../lib.php'); +require_once(dirname(__FILE__) . '/helpers.php'); + + +/** + * Unit tests for the autosave parts of the {@link question_usage} class. + * + * Note that many of the methods used when attempting questions, like + * load_questions_usage_by_activity, insert_question_*, delete_steps are + * tested elsewhere, e.g. by {@link question_usage_autosave_test}. We do not + * re-test them here. + * + * @copyright 2013 The Open University + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class question_engine_data_mapper_testcase extends qbehaviour_walkthrough_test_base { + + /** @var question_engine_data_mapper */ + protected $dm; + + /** @var qtype_shortanswer_question */ + protected $sa; + + /** @var qtype_essay_question */ + protected $essay; + + /** @var array */ + protected $usageids = array(); + + /** @var qubaid_condition */ + protected $bothusages; + + /** @var array */ + protected $allslots = array(); + + /** + * Test the various methods that load data for reporting. + * + * Since these methods need an expensive set-up, and then only do read-only + * operations on the data, we use a single method to do the set-up, which + * calls diffents methods to test each query. + */ + public function test_reporting_queries() { + // We create two usages, each with two questions, a short-answer marked + // out of 5, and and essay marked out of 10. + // + // In the first usage, the student answers the short-answer + // question correctly, and enters something in the essay. + // + // In the second useage, the student answers the short-answer question + // wrongly, and leaves the essay blank. + $this->resetAfterTest(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + $cat = $generator->create_question_category(); + $this->sa = $generator->create_question('shortanswer', null, + array('category' => $cat->id)); + $this->essay = $generator->create_question('essay', null, + array('category' => $cat->id)); + + $this->usageids = array(); + + // Create the first usage. + $q = question_bank::load_question($this->sa->id); + $this->start_attempt_at_question($q, 'interactive', 5); + $this->allslots[] = $this->slot; + $this->process_submission(array('answer' => 'cat')); + $this->process_submission(array('answer' => 'frog', '-submit' => 1)); + + $q = question_bank::load_question($this->essay->id); + $this->start_attempt_at_question($q, 'interactive', 10); + $this->allslots[] = $this->slot; + $this->process_submission(array('answer' => '

The cat sat on the mat.

', 'answerformat' => FORMAT_HTML)); + + $this->finish(); + $this->save_quba(); + $this->usageids[] = $this->quba->get_id(); + + // Create the second usage. + $this->quba = question_engine::make_questions_usage_by_activity('unit_test', + context_system::instance()); + + $q = question_bank::load_question($this->sa->id); + $this->start_attempt_at_question($q, 'interactive', 5); + $this->process_submission(array('answer' => 'fish')); + + $q = question_bank::load_question($this->essay->id); + $this->start_attempt_at_question($q, 'interactive', 10); + + $this->finish(); + $this->save_quba(); + $this->usageids[] = $this->quba->get_id(); + + // Set up some things the tests will need. + $this->dm = new question_engine_data_mapper(); + $this->bothusages = new qubaid_list($this->usageids); + + // Now test the various queries. + $this->dotest_load_questions_usages_latest_steps(); + $this->dotest_load_questions_usages_question_state_summary(); + $this->dotest_load_questions_usages_where_question_in_state(); + $this->dotest_load_average_marks(); + $this->dotest_sum_usage_marks_subquery(); + $this->dotest_question_attempt_latest_state_view(); + } + + protected function dotest_load_questions_usages_latest_steps() { + $rawstates = $this->dm->load_questions_usages_latest_steps($this->bothusages, $this->allslots, + 'qa.id AS questionattemptid, qa.questionusageid, qa.slot, ' . + 'qa.questionid, qa.maxmark, qas.sequencenumber, qas.state'); + + $states = array(); + foreach ($rawstates as $state) { + $states[$state->questionusageid][$state->slot] = $state; + unset($state->questionattemptid); + unset($state->questionusageid); + unset($state->slot); + } + + $state = $states[$this->usageids[0]][$this->allslots[0]]; + $this->assertEquals((object) array( + 'questionid' => $this->sa->id, + 'maxmark' => '5.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$gradedright, + ), $state); + + $state = $states[$this->usageids[0]][$this->allslots[1]]; + $this->assertEquals((object) array( + 'questionid' => $this->essay->id, + 'maxmark' => '10.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$needsgrading, + ), $state); + + $state = $states[$this->usageids[1]][$this->allslots[0]]; + $this->assertEquals((object) array( + 'questionid' => $this->sa->id, + 'maxmark' => '5.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$gradedwrong, + ), $state); + + $state = $states[$this->usageids[1]][$this->allslots[1]]; + $this->assertEquals((object) array( + 'questionid' => $this->essay->id, + 'maxmark' => '10.0000000', + 'sequencenumber' => 1, + 'state' => (string) question_state::$gaveup, + ), $state); + } + + protected function dotest_load_questions_usages_question_state_summary() { + $summary = $this->dm->load_questions_usages_question_state_summary( + $this->bothusages, $this->allslots); + + $this->assertEquals($summary[$this->allslots[0] . ',' . $this->sa->id], + (object) array( + 'slot' => $this->allslots[0], + 'questionid' => $this->sa->id, + 'name' => $this->sa->name, + 'inprogress' => 0, + 'needsgrading' => 0, + 'autograded' => 2, + 'manuallygraded' => 0, + 'all' => 2, + )); + $this->assertEquals($summary[$this->allslots[1] . ',' . $this->essay->id], + (object) array( + 'slot' => $this->allslots[1], + 'questionid' => $this->essay->id, + 'name' => $this->essay->name, + 'inprogress' => 0, + 'needsgrading' => 1, + 'autograded' => 1, + 'manuallygraded' => 0, + 'all' => 2, + )); + } + + protected function dotest_load_questions_usages_where_question_in_state() { + $this->assertEquals( + array(array($this->usageids[0], $this->usageids[1]), 2), + $this->dm->load_questions_usages_where_question_in_state($this->bothusages, + 'all', $this->allslots[1], null, 'questionusageid')); + + $this->assertEquals( + array(array($this->usageids[0], $this->usageids[1]), 2), + $this->dm->load_questions_usages_where_question_in_state($this->bothusages, + 'autograded', $this->allslots[0], null, 'questionusageid')); + + $this->assertEquals( + array(array($this->usageids[0]), 1), + $this->dm->load_questions_usages_where_question_in_state($this->bothusages, + 'needsgrading', $this->allslots[1], null, 'questionusageid')); + } + + protected function dotest_load_average_marks() { + $averages = $this->dm->load_average_marks($this->bothusages); + + $this->assertEquals(array( + $this->allslots[0] => (object) array( + 'slot' => $this->allslots[0], + 'averagefraction' => 0.5, + 'numaveraged' => 2, + ), + $this->allslots[1] => (object) array( + 'slot' => $this->allslots[1], + 'averagefraction' => 0, + 'numaveraged' => 1, + ), + ), $averages); + } + + protected function dotest_sum_usage_marks_subquery() { + global $DB; + + $totals = $DB->get_records_sql_menu("SELECT qu.id, ({$this->dm->sum_usage_marks_subquery('qu.id')}) AS totalmark + FROM {question_usages} qu + WHERE qu.id IN ({$this->usageids[0]}, {$this->usageids[1]})"); + + $this->assertNull($totals[$this->usageids[0]]); // Since a question requires grading. + + $this->assertNotNull($totals[$this->usageids[1]]); // Grrr! PHP null == 0 makes this hard. + $this->assertEquals(0, $totals[$this->usageids[1]]); + } + + protected function dotest_question_attempt_latest_state_view() { + global $DB; + + list($inlineview, $viewparams) = $this->dm->question_attempt_latest_state_view( + 'lateststate', $this->bothusages); + + $rawstates = $DB->get_records_sql(" + SELECT lateststate.questionattemptid, + qu.id AS questionusageid, + lateststate.slot, + lateststate.questionid, + lateststate.maxmark, + lateststate.sequencenumber, + lateststate.state + FROM {question_usages} qu + LEFT JOIN $inlineview ON lateststate.questionusageid = qu.id + WHERE qu.id IN ({$this->usageids[0]}, {$this->usageids[1]})", $viewparams); + + $states = array(); + foreach ($rawstates as $state) { + $states[$state->questionusageid][$state->slot] = $state; + unset($state->questionattemptid); + unset($state->questionusageid); + unset($state->slot); + } + + $state = $states[$this->usageids[0]][$this->allslots[0]]; + $this->assertEquals((object) array( + 'questionid' => $this->sa->id, + 'maxmark' => '5.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$gradedright, + ), $state); + + $state = $states[$this->usageids[0]][$this->allslots[1]]; + $this->assertEquals((object) array( + 'questionid' => $this->essay->id, + 'maxmark' => '10.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$needsgrading, + ), $state); + + $state = $states[$this->usageids[1]][$this->allslots[0]]; + $this->assertEquals((object) array( + 'questionid' => $this->sa->id, + 'maxmark' => '5.0000000', + 'sequencenumber' => 2, + 'state' => (string) question_state::$gradedwrong, + ), $state); + + $state = $states[$this->usageids[1]][$this->allslots[1]]; + $this->assertEquals((object) array( + 'questionid' => $this->essay->id, + 'maxmark' => '10.0000000', + 'sequencenumber' => 1, + 'state' => (string) question_state::$gaveup, + ), $state); + } +} diff --git a/question/type/essay/tests/helper.php b/question/type/essay/tests/helper.php index 0ce0ba44282..c63668495eb 100644 --- a/question/type/essay/tests/helper.php +++ b/question/type/essay/tests/helper.php @@ -68,6 +68,31 @@ class qtype_essay_test_helper extends question_test_helper { return $this->initialise_essay_question(); } + /** + * Make the data what would be received from the editing form for an essay + * question using the HTML editor allowing embedded files as input, and up + * to three attachments. + * + * @return stdClass the data that would be returned by $form->get_gata(); + */ + public function get_essay_question_form_data_editor() { + $fromform = new stdClass(); + + $fromform->name = 'Essay question (HTML editor)'; + $fromform->questiontext = array('text' => 'Please write a story about a frog.', 'format' => FORMAT_HTML); + $fromform->defaultmark = 1.0; + $fromform->generalfeedback = array('text' => 'I hope your story had a beginning, a middle and an end.', 'format' => FORMAT_HTML); + $fromform->responseformat = 'editor'; + $fromform->responserequired = 1; + $fromform->responsefieldlines = 10; + $fromform->attachments = 0; + $fromform->attachmentsrequired = 0; + $fromform->graderinfo = array('text' => '', 'format' => FORMAT_HTML); + $fromform->responsetemplate = array('text' => '', 'format' => FORMAT_HTML); + + return $fromform; + } + /** * Makes an essay question using the HTML editor allowing embedded files as * input, and up to three attachments. From 84b3710842e539d33552645e27331b5c007b9e63 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 8 Jan 2014 18:22:20 +0000 Subject: [PATCH 3/3] MDL-43246 question engine: avoid order-by id. This was breaking with oracle master/master replication. Fortunately all the places that needed to be changed were private to datalib.php. There is still some ordering by id there, but only places where we want a consitent, rather than meaningful, order, so that is OK. The queries changed by this patch all have subqueries in aggregate queries that pull out the latest step for a question_attempt. Those queries used to look for MAX(id) but now they look for MAX(sequencenumber). This is equivalent (for databases where ids always increase with time, except for auto-saved steps. In the past, an auto-saved step might have been considered latest. Now the latest step will always be one that has been properly processed. You can aruge that this change is an improvement. Anyway, it is a moot point. All these queries are only used in reports which are run on completed attempts, where there will not be any autosaved data. --- question/engine/datalib.php | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/question/engine/datalib.php b/question/engine/datalib.php index ba757e6d44e..9694851a77f 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -400,8 +400,8 @@ SELECT {$fields} FROM {$qubaids->from_question_attempts('qa')} -JOIN {question_attempt_steps} qas ON - qas.id = {$this->latest_step_for_qa_subquery()} +JOIN {question_attempt_steps} qas ON qas.questionattemptid = qa.id + AND qas.sequencenumber = {$this->latest_step_for_qa_subquery()} WHERE {$qubaids->where()} AND @@ -438,8 +438,8 @@ SELECT COUNT(1) AS numattempts FROM {$qubaids->from_question_attempts('qa')} -JOIN {question_attempt_steps} qas ON - qas.id = {$this->latest_step_for_qa_subquery()} +JOIN {question_attempt_steps} qas ON qas.questionattemptid = qa.id + AND qas.sequencenumber = {$this->latest_step_for_qa_subquery()} JOIN {question} q ON q.id = qa.questionid WHERE @@ -547,8 +547,8 @@ SELECT 1 FROM {$qubaids->from_question_attempts('qa')} -JOIN {question_attempt_steps} qas ON - qas.id = {$this->latest_step_for_qa_subquery()} +JOIN {question_attempt_steps} qas ON qas.questionattemptid = qa.id + AND qas.sequencenumber = {$this->latest_step_for_qa_subquery()} JOIN {question} q ON q.id = qa.questionid WHERE @@ -609,8 +609,8 @@ SELECT COUNT(1) AS numaveraged FROM {$qubaids->from_question_attempts('qa')} -JOIN {question_attempt_steps} qas ON - qas.id = {$this->latest_step_for_qa_subquery()} +JOIN {question_attempt_steps} qas ON qas.questionattemptid = qa.id + AND qas.sequencenumber = {$this->latest_step_for_qa_subquery()} WHERE {$qubaids->where()} @@ -926,10 +926,11 @@ ORDER BY // NULL total into a 0. return "SELECT COALESCE(SUM(qa.maxmark * qas.fraction), 0) FROM {question_attempts} qa - JOIN {question_attempt_steps} qas ON qas.id = ( - SELECT MAX(summarks_qas.id) - FROM {question_attempt_steps} summarks_qas - WHERE summarks_qas.questionattemptid = qa.id + JOIN {question_attempt_steps} qas ON qas.questionattemptid = qa.id + AND qas.sequencenumber = ( + SELECT MAX(summarks_qas.sequencenumber) + FROM {question_attempt_steps} summarks_qas + WHERE summarks_qas.questionattemptid = qa.id ) WHERE qa.questionusageid = $qubaid HAVING COUNT(CASE @@ -971,15 +972,15 @@ ORDER BY {$alias}qas.userid FROM {$qubaids->from_question_attempts($alias . 'qa')} - JOIN {question_attempt_steps} {$alias}qas ON - {$alias}qas.id = {$this->latest_step_for_qa_subquery($alias . 'qa.id')} + JOIN {question_attempt_steps} {$alias}qas ON {$alias}qas.questionattemptid = {$alias}qa.id + AND {$alias}qas.sequencenumber = {$this->latest_step_for_qa_subquery($alias . 'qa.id')} WHERE {$qubaids->where()} ) $alias", $qubaids->from_where_params()); } protected function latest_step_for_qa_subquery($questionattemptid = 'qa.id') { return "( - SELECT MAX(id) + SELECT MAX(sequencenumber) FROM {question_attempt_steps} WHERE questionattemptid = $questionattemptid )";