MDL-66259 qtypes: get_question_options() always call parent::
authorEloy Lafuente (stronk7) <stronk7@moodle.org>
Tue, 19 Nov 2019 18:08:20 +0000 (19:08 +0100)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Fri, 24 Apr 2020 13:56:23 +0000 (15:56 +0200)
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
question/type/calculated/tests/questiontype_test.php
question/type/multianswer/questiontype.php
question/type/multianswer/tests/questiontype_test.php
question/type/random/questiontype.php
question/type/random/tests/questiontype_test.php
question/type/truefalse/questiontype.php
question/type/truefalse/tests/questiontype_test.php

index a6d321d..39f293d 100644 (file)
@@ -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();
index b4cb2dd..1292760 100644 (file)
@@ -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);
 
index 79811ce..a1f01f5 100644 (file)
@@ -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);
index b35645f..2261634 100644 (file)
@@ -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) {
index e318641..b399986 100644 (file)
@@ -116,6 +116,7 @@ class qtype_random extends question_type {
     }
 
     public function get_question_options($question) {
+        parent::get_question_options($question);
         return true;
     }
 
index e294867..289966f 100644 (file)
@@ -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() {
index c883a4c..1969ae3 100644 (file)
@@ -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',
index 2607982..5f1a639 100644 (file)
@@ -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() {