From a18a00bd2dbb3ffc9e223b581d96ec7ea36fbbb5 Mon Sep 17 00:00:00 2001 From: "Eloy Lafuente (stronk7)" Date: Tue, 19 Nov 2019 19:08:20 +0100 Subject: [PATCH] MDL-66259 qtypes: get_question_options() always call parent:: To get the question->options initialised, children must call parent::get_question_options() always. Also, it is just general good practice. Subclasses are meant to be adaptations of the base class, not something completely different. Note, there are some changes in the data structure produced (see changes in the tests) but these changes are not wrong. --- question/type/calculated/questiontype.php | 1 + .../calculated/tests/questiontype_test.php | 5 ++-- question/type/multianswer/questiontype.php | 1 + .../multianswer/tests/questiontype_test.php | 8 +++-- question/type/random/questiontype.php | 1 + .../type/random/tests/questiontype_test.php | 29 ++++++++++--------- question/type/truefalse/questiontype.php | 1 + .../truefalse/tests/questiontype_test.php | 7 ++++- 8 files changed, 33 insertions(+), 20 deletions(-) diff --git a/question/type/calculated/questiontype.php b/question/type/calculated/questiontype.php index a6d321da2dd..39f293d1590 100644 --- a/question/type/calculated/questiontype.php +++ b/question/type/calculated/questiontype.php @@ -61,6 +61,7 @@ class qtype_calculated extends question_type { // First get the datasets and default options. // The code is used for calculated, calculatedsimple and calculatedmulti qtypes. global $CFG, $DB, $OUTPUT; + parent::get_question_options($question); if (!$question->options = $DB->get_record('question_calculated_options', array('question' => $question->id))) { $question->options = new stdClass(); diff --git a/question/type/calculated/tests/questiontype_test.php b/question/type/calculated/tests/questiontype_test.php index b4cb2dd5f35..1292760e43c 100644 --- a/question/type/calculated/tests/questiontype_test.php +++ b/question/type/calculated/tests/questiontype_test.php @@ -112,9 +112,8 @@ class qtype_calculated_test extends advanced_testcase { // Options. $this->assertEquals($questiondata->id, $questiondata->options->question); $this->assertEquals([], $questiondata->options->units); - // TODO: Are this four correct? The first one doesn't match, hence commented out. - // $this->assertEquals($fromform->unitgradingtypes, $questiondata->options->unitgradingtype); - $this->assertEquals($fromform->unitrole, $questiondata->options->showunits); + $this->assertEquals(qtype_numerical::UNITNONE, $questiondata->options->showunits); + $this->assertEquals(0, $questiondata->options->unitgradingtype); // Unit role is none, so this is 0. $this->assertEquals($fromform->unitpenalty, $questiondata->options->unitpenalty); $this->assertEquals($fromform->unitsleft, $questiondata->options->unitsleft); diff --git a/question/type/multianswer/questiontype.php b/question/type/multianswer/questiontype.php index 79811ce851c..a1f01f5554f 100644 --- a/question/type/multianswer/questiontype.php +++ b/question/type/multianswer/questiontype.php @@ -45,6 +45,7 @@ class qtype_multianswer extends question_type { public function get_question_options($question) { global $DB, $OUTPUT; + parent::get_question_options($question); // Get relevant data indexed by positionkey from the multianswers table. $sequence = $DB->get_field('question_multianswer', 'sequence', array('question' => $question->id), MUST_EXIST); diff --git a/question/type/multianswer/tests/questiontype_test.php b/question/type/multianswer/tests/questiontype_test.php index b35645f17c3..22616347453 100644 --- a/question/type/multianswer/tests/questiontype_test.php +++ b/question/type/multianswer/tests/questiontype_test.php @@ -131,7 +131,8 @@ class qtype_multianswer_test extends advanced_testcase { $question->qtype = 'multianswer'; $question->createdby = 0; - // TODO: Just check why is not consistent with others, $question should came back modified. + // Note, $question gets modified during save because of the way subquestions + // are extracted. $question = $this->qtype->save_question($question, $fromform); $questiondata = question_bank::load_question_data($question->id); @@ -181,9 +182,12 @@ class qtype_multianswer_test extends advanced_testcase { $this->assertEquals($expectedhints, array_values($gothints)); // Options. - $this->assertEquals(['questions'], array_keys(get_object_vars($questiondata->options))); + $this->assertEquals(['answers', 'questions'], array_keys(get_object_vars($questiondata->options))); $this->assertEquals(count($fromform->options->questions), count($questiondata->options->questions)); + // Option answers. + $this->assertEquals([], $questiondata->options->answers); + // Build the expected questions. We aren't going deeper to subquestion answers, options... that's another qtype job. $expectedquestions = []; foreach ($fromform->options->questions as $key => $value) { diff --git a/question/type/random/questiontype.php b/question/type/random/questiontype.php index e318641c12e..b3999863088 100644 --- a/question/type/random/questiontype.php +++ b/question/type/random/questiontype.php @@ -116,6 +116,7 @@ class qtype_random extends question_type { } public function get_question_options($question) { + parent::get_question_options($question); return true; } diff --git a/question/type/random/tests/questiontype_test.php b/question/type/random/tests/questiontype_test.php index e29486719af..289966fdc15 100644 --- a/question/type/random/tests/questiontype_test.php +++ b/question/type/random/tests/questiontype_test.php @@ -82,25 +82,26 @@ class qtype_random_test extends advanced_testcase { $this->assertEquals(['id', 'category', 'parent', 'name', 'questiontext', 'questiontextformat', 'generalfeedback', 'generalfeedbackformat', 'defaultmark', 'penalty', 'qtype', 'length', 'stamp', 'version', 'hidden', 'timecreated', 'timemodified', - 'createdby', 'modifiedby', 'idnumber', 'contextid', 'categoryobject'], + 'createdby', 'modifiedby', 'idnumber', 'contextid', 'options', 'hints', 'categoryobject'], array_keys(get_object_vars($questiondata))); $this->assertEquals($category->id, $questiondata->category); - // TODO: All this is nosense (in my brain, requires review. - $this->assertEquals($questiondata->id, $questiondata->parent); // parent points to self? - // $this->assertEquals($fromform->name, $questiondata->name); // fromform has empty name. - $this->assertEquals($fromform->questiontext, $questiondata->questiontext); // questiontext has 0 ? - $this->assertEquals($fromform->questiontextformat, $questiondata->questiontextformat); - $this->assertEquals('', $questiondata->generalfeedback); - $this->assertEquals(0, $questiondata->generalfeedbackformat); - $this->assertEquals(1, $questiondata->defaultmark); // Nothing @ fromform? - $this->assertEquals(0, $questiondata->penalty); + + // Random questions are not real questions. This is signaled by parent + // being non-zero - and in fact equal to question id. + $this->assertEquals($questiondata->id, $questiondata->parent); + $this->assertEquals('Random (' . $category->name . ')', $questiondata->name); + $this->assertEquals(0, $questiondata->questiontext); // Used to store 'Select from subcategories'. $this->assertEquals('random', $questiondata->qtype); $this->assertEquals(1, $questiondata->length); $this->assertEquals(0, $questiondata->hidden); - $this->assertEquals($question->createdby, $questiondata->createdby); - $this->assertEquals($question->createdby, $questiondata->modifiedby); - $this->assertEquals('', $questiondata->idnumber); - $this->assertEquals($syscontext->id, $questiondata->contextid); + $this->assertEquals($category->contextid, $questiondata->contextid); + + // Options - not used. + $this->assertEquals(['answers'], array_keys(get_object_vars($questiondata->options))); + $this->assertEquals([], $questiondata->options->answers); + + // Hints - not used. + $this->assertEquals([], $questiondata->hints); } public function test_get_possible_responses() { diff --git a/question/type/truefalse/questiontype.php b/question/type/truefalse/questiontype.php index c883a4c8283..1969ae3ecb4 100644 --- a/question/type/truefalse/questiontype.php +++ b/question/type/truefalse/questiontype.php @@ -113,6 +113,7 @@ class qtype_truefalse extends question_type { */ public function get_question_options($question) { global $DB, $OUTPUT; + parent::get_question_options($question); // Get additional information from database // and attach it to the question object. if (!$question->options = $DB->get_record('question_truefalse', diff --git a/question/type/truefalse/tests/questiontype_test.php b/question/type/truefalse/tests/questiontype_test.php index 2607982c0b9..5f1a639f872 100644 --- a/question/type/truefalse/tests/questiontype_test.php +++ b/question/type/truefalse/tests/questiontype_test.php @@ -83,7 +83,7 @@ class qtype_truefalse_test extends advanced_testcase { $this->assertEquals(['id', 'category', 'parent', 'name', 'questiontext', 'questiontextformat', 'generalfeedback', 'generalfeedbackformat', 'defaultmark', 'penalty', 'qtype', 'length', 'stamp', 'version', 'hidden', 'timecreated', 'timemodified', - 'createdby', 'modifiedby', 'idnumber', 'contextid', 'options', 'categoryobject'], + 'createdby', 'modifiedby', 'idnumber', 'contextid', 'options', 'hints', 'categoryobject'], array_keys(get_object_vars($questiondata))); $this->assertEquals($category->id, $questiondata->category); $this->assertEquals(0, $questiondata->parent); @@ -101,6 +101,8 @@ class qtype_truefalse_test extends advanced_testcase { $this->assertEquals($question->createdby, $questiondata->modifiedby); $this->assertEquals('', $questiondata->idnumber); $this->assertEquals($syscontext->id, $questiondata->contextid); + + // Options. $this->assertEquals($questiondata->id, $questiondata->options->question); $this->assertEquals('True', $questiondata->options->answers[$questiondata->options->trueanswer]->answer); $this->assertEquals('False', $questiondata->options->answers[$questiondata->options->falseanswer]->answer); @@ -112,6 +114,9 @@ class qtype_truefalse_test extends advanced_testcase { $questiondata->options->answers[$questiondata->options->falseanswer]->feedback); $this->assertEquals(FORMAT_HTML, $questiondata->options->answers[$questiondata->options->trueanswer]->feedbackformat); $this->assertEquals(FORMAT_HTML, $questiondata->options->answers[$questiondata->options->falseanswer]->feedbackformat); + + // Hints. + $this->assertEquals([], $questiondata->hints); } public function test_get_possible_responses() {