MDL-63905 qtype_multianswer: validate imported questions

question/format/multianswer/format.php

@@ -51,7 +51,14 @@ public function readquestions($lines) {
         $questiontext['text'] = implode('', $lines);
         $questiontext['format'] = FORMAT_MOODLE;
         $questiontext['itemid'] = '';
+
         $question = qtype_multianswer_extract_question($questiontext);
+        $errors = qtype_multianswer_validate_question($question);
+        if ($errors) {
+            $this->error(get_string('invalidmultianswerquestion', 'qtype_multianswer', implode(' ', $errors)));
+            return array();
+        }
+
         $question->questiontext = $question->questiontext['text'];
         $question->questiontextformat = 0;
 

question/format/multianswer/tests/fixtures/broken_multianswer_1.txt

@@ -0,0 +1 @@
+Please select the fruits {1:MULTICHOICE:=Apple#Correct}

question/format/multianswer/tests/fixtures/broken_multianswer_2.txt

@@ -0,0 +1 @@
+Please select the fruits {1:MULTICHOICE:Pear#Incorrect~%50%Apple#Correct}

question/format/multianswer/tests/fixtures/broken_multianswer_3.txt

@@ -0,0 +1 @@
+What grade would you give it? {3:NUMERICAL:=zero}

question/format/multianswer/tests/fixtures/broken_multianswer_4.txt

@@ -0,0 +1 @@
+Please select the fruits.

question/format/multianswer/tests/multianswerformat_test.php

@@ -75,4 +75,81 @@ public function test_import() {
         $this->assertEquals('multichoice', $qs[0]->options->questions[4]->qtype);
         $this->assertEquals('shortanswer', $qs[0]->options->questions[5]->qtype);
     }
+
+    public function test_read_brokencloze_1() {
+        $lines = file(__DIR__ . '/fixtures/broken_multianswer_1.txt');
+        $importer = new qformat_multianswer();
+
+        // The importer echoes some errors, so we need to capture and check that.
+        ob_start();
+        $questions = $importer->readquestions($lines);
+        $output = ob_get_contents();
+        ob_end_clean();
+
+        // Check that there were some expected errors.
+        $this->assertContains('Error importing question', $output);
+        $this->assertContains('Invalid embedded answers (Cloze) question', $output);
+        $this->assertContains('This type of question requires at least 2 choices', $output);
+
+        // No question  have been imported.
+        $this->assertCount(0, $questions);
+    }
+
+    public function test_read_brokencloze_2() {
+        $lines = file(__DIR__ . '/fixtures/broken_multianswer_2.txt');
+        $importer = new qformat_multianswer();
+
+        // The importer echoes some errors, so we need to capture and check that.
+        ob_start();
+        $questions = $importer->readquestions($lines);
+        $output = ob_get_contents();
+        ob_end_clean();
+
+        // Check that there were some expected errors.
+        $this->assertContains('Error importing question', $output);
+        $this->assertContains('Invalid embedded answers (Cloze) question', $output);
+        $this->assertContains('One of the answers should have a score of 100% so it is possible to get full marks for this question.',
+                $output);
+
+        // No question  have been imported.
+        $this->assertCount(0, $questions);
+    }
+
+    public function test_read_brokencloze_3() {
+        $lines = file(__DIR__ . '/fixtures/broken_multianswer_3.txt');
+        $importer = new qformat_multianswer();
+
+        // The importer echoes some errors, so we need to capture and check that.
+        ob_start();
+        $questions = $importer->readquestions($lines);
+        $output = ob_get_contents();
+        ob_end_clean();
+
+        // Check that there were some expected errors.
+        $this->assertContains('Error importing question', $output);
+        $this->assertContains('Invalid embedded answers (Cloze) question', $output);
+        $this->assertContains('The answer must be a number, for example -1.234 or 3e8, or \'*\'.', $output);
+
+        // No question  have been imported.
+        $this->assertCount(0, $questions);
+    }
+
+    public function test_read_brokencloze_4() {
+        $lines = file(__DIR__ . '/fixtures/broken_multianswer_4.txt');
+        $importer = new qformat_multianswer();
+
+        // The importer echoes some errors, so we need to capture and check that.
+        ob_start();
+        $questions = $importer->readquestions($lines);
+        $output = ob_get_contents();
+        ob_end_clean();
+
+        // Check that there were some expected errors.
+        $this->assertContains('Error importing question', $output);
+        $this->assertContains('Invalid embedded answers (Cloze) question', $output);
+        $this->assertContains('The question text must include at least one embedded answer.', $output);
+
+        // No question  have been imported.
+        $this->assertCount(0, $questions);
+    }
 }

question/format/xml/format.php

@@ -237,7 +237,7 @@ public function import_headers($question) {
 
         // Restore files in generalfeedback.
         $generalfeedback = $this->import_text_with_files($question,
-                array('#', 'generalfeedback', 0), $qo->generalfeedback, $this->get_format($qo->questiontextformat));
+                array('#', 'generalfeedback', 0), '', $this->get_format($qo->questiontextformat));
         $qo->generalfeedback = $generalfeedback['text'];
         $qo->generalfeedbackformat = $generalfeedback['format'];
         if (!empty($generalfeedback['itemid'])) {
@@ -472,6 +472,11 @@ public function import_multianswer($question) {
         $questiontext = $this->import_text_with_files($question,
                 array('#', 'questiontext', 0));
         $qo = qtype_multianswer_extract_question($questiontext);
+        $errors = qtype_multianswer_validate_question($qo);
+        if ($errors) {
+            $this->error(get_string('invalidmultianswerquestion', 'qtype_multianswer', implode(' ', $errors)));
+            return null;
+        }
 
         // Header parts particular to multianswer.
         $qo->qtype = 'multianswer';
@@ -480,8 +485,12 @@ public function import_multianswer($question) {
         if (isset($this->course)) {
             $qo->course = $this->course;
         }
-
-        $qo->name = $this->clean_question_name($this->import_text($question['#']['name'][0]['#']['text']));
+        if (isset($question['#']['name'])) {
+            $qo->name = $this->clean_question_name($this->import_text($question['#']['name'][0]['#']['text']));
+        } else {
+            $qo->name = $this->create_default_question_name($qo->questiontext['text'],
+                    get_string('questionname', 'question'));
+        }
         $qo->questiontextformat = $questiontext['format'];
         $qo->questiontext = $qo->questiontext['text'];
         if (!empty($questiontext['itemid'])) {
@@ -511,7 +520,7 @@ public function import_multianswer($question) {
 
         // Restore files in generalfeedback.
         $generalfeedback = $this->import_text_with_files($question,
-                array('#', 'generalfeedback', 0), $qo->generalfeedback, $this->get_format($qo->questiontextformat));
+                array('#', 'generalfeedback', 0), '', $this->get_format($qo->questiontextformat));
         $qo->generalfeedback = $generalfeedback['text'];
         $qo->generalfeedbackformat = $generalfeedback['format'];
         if (!empty($generalfeedback['itemid'])) {
@@ -926,7 +935,7 @@ protected function import_category($question) {
      * @param stdClass $context
      * @return array (of objects) question objects.
      */
-    protected function readquestions($lines) {
+    public function readquestions($lines) {
         // We just need it as one big string.
         $lines = implode('', $lines);
 

question/type/multianswer/edit_multianswer_form.php

@@ -398,7 +398,7 @@ public function set_data($question) {
                             if ($trimmedanswer !== '') {
                                 $answercount++;
                                 if ($subquestion->qtype == 'numerical' &&
-                                        !($this->is_valid_number($trimmedanswer) || $trimmedanswer == '*')) {
+                                        !(qtype_numerical::is_valid_number($trimmedanswer) || $trimmedanswer == '*')) {
                                     $this->_form->setElementError($prefix.'answer['.$key.']',
                                             get_string('answermustbenumberorstar',
                                                     'qtype_numerical'));
@@ -477,55 +477,7 @@ public function validation($data, $files) {
 
         $questiondisplay = qtype_multianswer_extract_question($data['questiontext']);
 
-        if (isset($questiondisplay->options->questions)) {
-            $subquestions = fullclone($questiondisplay->options->questions);
-            if (count($subquestions)) {
-                $sub = 1;
-                foreach ($subquestions as $subquestion) {
-                    $prefix = 'sub_'.$sub.'_';
-                    $answercount = 0;
-                    $maxgrade = false;
-                    $maxfraction = -1;
-
-                    foreach ($subquestion->answer as $key => $answer) {
-                        if (is_array($answer)) {
-                            $answer = $answer['text'];
-                        }
-                        $trimmedanswer = trim($answer);
-                        if ($trimmedanswer !== '') {
-                            $answercount++;
-                            if ($subquestion->qtype == 'numerical' &&
-                                    !($this->is_valid_number($trimmedanswer) || $trimmedanswer == '*')) {
-                                $errors[$prefix.'answer['.$key.']'] =
-                                        get_string('answermustbenumberorstar', 'qtype_numerical');
-                            }
-                            if ($subquestion->fraction[$key] == 1) {
-                                $maxgrade = true;
-                            }
-                            if ($subquestion->fraction[$key] > $maxfraction) {
-                                $maxfraction = $subquestion->fraction[$key];
-                            }
-                            // For 'multiresponse' we are OK if there is at least one fraction > 0.
-                            if ($subquestion->qtype == 'multichoice' && $subquestion->single == 0 &&
-                                $subquestion->fraction[$key] > 0) {
-                                $maxgrade = true;
-                            }
-                        }
-                    }
-                    if ($subquestion->qtype == 'multichoice' && $answercount < 2) {
-                        $errors[$prefix.'answer[0]'] = get_string('notenoughanswers', 'qtype_multichoice', 2);
-                    } else if ($answercount == 0) {
-                        $errors[$prefix.'answer[0]'] = get_string('notenoughanswers', 'question', 1);
-                    }
-                    if ($maxgrade == false) {
-                        $errors[$prefix.'fraction[0]'] = get_string('fractionsnomax', 'question');
-                    }
-                    $sub++;
-                }
-            } else {
-                $errors['questiontext'] = get_string('questionsmissing', 'qtype_multianswer');
-            }
-        }
+        $errors = array_merge($errors, qtype_multianswer_validate_question($questiondisplay));
 
         if (($this->negativediff > 0 || $this->usedinquiz &&
                 ($this->negativediff > 0 || $this->negativediff < 0 ||

question/type/multianswer/lang/en/qtype_multianswer.php

@@ -28,6 +28,7 @@
 $string['correctanswer'] = 'Correct answer';
 $string['correctanswerandfeedback'] = 'Correct answer and feedback';
 $string['decodeverifyquestiontext'] = 'Decode and verify the question text';
+$string['invalidmultianswerquestion'] = 'Invalid embedded answers (Cloze) question ({$a}).';
 $string['layout'] = 'Layout';
 $string['layouthorizontal'] = 'Horizontal row of radio-buttons';
 $string['layoutmultiple_horizontal'] = 'Horizontal row of checkboxes';

question/type/multianswer/questiontype.php

@@ -28,6 +28,7 @@
 
 require_once($CFG->dirroot . '/question/type/questiontypebase.php');
 require_once($CFG->dirroot . '/question/type/multichoice/question.php');
+require_once($CFG->dirroot . '/question/type/numerical/questiontype.php');
 
 /**
  * The multi-answer question type class.
@@ -509,3 +510,65 @@ function qtype_multianswer_extract_question($text) {
     }
     return $question;
 }
+
+/**
+ * Validate a multianswer question.
+ *
+ * @param object $question  The multianswer question to validate as returned by qtype_multianswer_extract_question
+ * @return array Array of error messages with questions field names as keys.
+ */
+function qtype_multianswer_validate_question($question) {
+    $errors = array();
+    if (!isset($question->options->questions)) {
+        $errors['questiontext'] = get_string('questionsmissing', 'qtype_multianswer');
+    } else {
+        $subquestions = fullclone($question->options->questions);
+        if (count($subquestions)) {
+            $sub = 1;
+            foreach ($subquestions as $subquestion) {
+                $prefix = 'sub_'.$sub.'_';
+                $answercount = 0;
+                $maxgrade = false;
+                $maxfraction = -1;
+
+                foreach ($subquestion->answer as $key => $answer) {
+                    if (is_array($answer)) {
+                        $answer = $answer['text'];
+                    }
+                    $trimmedanswer = trim($answer);
+                    if ($trimmedanswer !== '') {
+                        $answercount++;
+                        if ($subquestion->qtype == 'numerical' &&
+                                !(qtype_numerical::is_valid_number($trimmedanswer) || $trimmedanswer == '*')) {
+                            $errors[$prefix.'answer['.$key.']'] =
+                                    get_string('answermustbenumberorstar', 'qtype_numerical');
+                        }
+                        if ($subquestion->fraction[$key] == 1) {
+                            $maxgrade = true;
+                        }
+                        if ($subquestion->fraction[$key] > $maxfraction) {
+                            $maxfraction = $subquestion->fraction[$key];
+                        }
+                        // For 'multiresponse' we are OK if there is at least one fraction > 0.
+                        if ($subquestion->qtype == 'multichoice' && $subquestion->single == 0 &&
+                            $subquestion->fraction[$key] > 0) {
+                            $maxgrade = true;
+                        }
+                    }
+                }
+                if ($subquestion->qtype == 'multichoice' && $answercount < 2) {
+                    $errors[$prefix.'answer[0]'] = get_string('notenoughanswers', 'qtype_multichoice', 2);
+                } else if ($answercount == 0) {
+                    $errors[$prefix.'answer[0]'] = get_string('notenoughanswers', 'question', 1);
+                }
+                if ($maxgrade == false) {
+                    $errors[$prefix.'fraction[0]'] = get_string('fractionsnomax', 'question');
+                }
+                $sub++;
+            }
+        } else {
+            $errors['questiontext'] = get_string('questionsmissing', 'qtype_multianswer');
+        }
+    }
+    return $errors;
+}

question/type/multianswer/version.php

@@ -26,7 +26,7 @@
 defined('MOODLE_INTERNAL') || die();
 
 $plugin->component = 'qtype_multianswer';
-$plugin->version   = 2018051400;
+$plugin->version   = 2018051401;
 
 $plugin->requires  = 2018050800;
 $plugin->dependencies = array(

question/type/numerical/edit_numerical_form.php

@@ -319,7 +319,7 @@ protected function validate_answers($data, $errors) {
      * @return bool whether this is a valid answer.
      */
     protected function is_valid_answer($answer, $data) {
-        return $answer == '*' || $this->is_valid_number($answer);
+        return $answer == '*' || qtype_numerical::is_valid_number($answer);
     }
 
     /**

question/type/numerical/questiontype.php

@@ -52,6 +52,17 @@ class qtype_numerical extends question_type {
     const UNITGRADEDOUTOFMARK = 1;
     const UNITGRADEDOUTOFMAX = 2;
 
+    /**
+     * Validate that a string is a number formatted correctly for the current locale.
+     * @param string $x a string
+     * @return bool whether $x is a number that the numerical question type can interpret.
+     */
+    public static function is_valid_number($x) {
+        $ap = new qtype_numerical_answer_processor(array());
+        list($value, $unit) = $ap->apply_units($x);
+        return !is_null($value) && !$unit;
+    }
+
     public function get_question_options($question) {
         global $CFG, $DB, $OUTPUT;
         parent::get_question_options($question);

question/type/numerical/tests/questiontype_test.php

@@ -169,4 +169,13 @@ public function test_question_saving_pi() {
             }
         }
     }
+
+    public function test_is_valid_number() {
+        $this->assertTrue(qtype_numerical::is_valid_number('1,001'));
+        $this->assertTrue(qtype_numerical::is_valid_number('1.001'));
+        $this->assertTrue(qtype_numerical::is_valid_number('1'));
+        $this->assertTrue(qtype_numerical::is_valid_number('1,e8'));
+        $this->assertFalse(qtype_numerical::is_valid_number('1001 xxx'));
+        $this->assertTrue(qtype_numerical::is_valid_number('1.e8'));
+    }
 }