MDL-63905 qtype_multianswer: validate imported questions

question/format/multianswer/format.php

@@ -51,7 +51,14 @@ class qformat_multianswer extends qformat_default {
         $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 @@ The capital of France is {#5}.
         $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

@@ -240,7 +240,7 @@ class qformat_xml extends qformat_default {
 
         // 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'])) {
@@ -475,6 +475,11 @@ class qformat_xml extends qformat_default {
         $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';
@@ -483,8 +488,12 @@ class qformat_xml extends qformat_default {
         if (isset($this->course)) {
             $qo->course = $this->course;
         }
-
+        if (isset($question['#']['name'])) {
-        $qo->name = $this->clean_question_name($this->import_text($question['#']['name'][0]['#']['text']));
+            $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'])) {
@@ -514,7 +523,7 @@ class qformat_xml extends qformat_default {
 
         // 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'])) {
@@ -937,7 +946,7 @@ class qformat_xml extends qformat_default {
      * @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/format/xml/tests/fixtures/broken_cloze_questions.xml

@@ -0,0 +1,82 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<quiz>
+<!-- question: 1  -->
+  <question type="cloze">
+    <name>
+      <text>Multianswer question without subquestions</text>
+    </name>
+    <questiontext format="html">
+      <text>Please select the fruits.</text>
+    </questiontext>
+    <generalfeedback format="html">
+      <text></text>
+    </generalfeedback>
+    <penalty>0.3333333</penalty>
+    <hidden>0</hidden>
+    <idnumber></idnumber>
+  </question>
+
+  <!-- question: 2  -->
+  <question type="cloze">
+    <name>
+      <text>Multichoice subquestion without choices</text>
+    </name>
+    <questiontext format="html">
+      <text>Please select the fruits {1:MULTICHOICE}</text>
+    </questiontext>
+    <generalfeedback format="html">
+      <text></text>
+    </generalfeedback>
+    <penalty>0.3333333</penalty>
+    <hidden>0</hidden>
+    <idnumber></idnumber>
+  </question>
+
+  <!-- question: 3  -->
+  <question type="cloze">
+    <name>
+      <text>Multichoice subquestion with only one choice</text>
+    </name>
+    <questiontext format="html">
+      <text>Please select the fruits {1:MULTICHOICE:=Apple#Correct}</text>
+    </questiontext>
+    <generalfeedback format="html">
+      <text></text>
+    </generalfeedback>
+    <penalty>0.3333333</penalty>
+    <hidden>0</hidden>
+    <idnumber></idnumber>
+  </question>
+
+  <!-- question: 4  -->
+  <question type="cloze">
+    <name>
+      <text>Multichoice subquestion with no completely correct answer</text>
+    </name>
+    <questiontext format="html">
+      <text>Please select the fruits {1:MULTICHOICE:Pear#Incorrect~%50%Apple#Correct}</text>
+    </questiontext>
+    <generalfeedback format="html">
+      <text></text>
+    </generalfeedback>
+    <penalty>0.3333333</penalty>
+    <hidden>0</hidden>
+    <idnumber></idnumber>
+  </question>
+
+    <!-- question: 5  -->
+  <question type="cloze">
+    <name>
+      <text>Numerical subquestion with buggy answer</text>
+    </name>
+    <questiontext format="html">
+      <text>What grade would you give it? {3:NUMERICAL:=zero}</text>
+    </questiontext>
+    <generalfeedback format="html">
+      <text></text>
+    </generalfeedback>
+    <penalty>0.3333333</penalty>
+    <hidden>0</hidden>
+    <idnumber></idnumber>
+  </question>
+</quiz>

question/format/xml/tests/qformat_xml_import_export_test.php

@@ -422,4 +422,30 @@ class qformat_xml_import_export_test extends advanced_testcase {
         $expectedxml = file_get_contents(__DIR__ . '/fixtures/nested_categories_with_questions.xml');
         $this->assert_same_xml($expectedxml, $qformat->exportprocess());
     }
+
+    /**
+     * Test that bad multianswer questions are not imported.
+     */
+    public function test_import_broken_multianswer_questions() {
+        $lines = file(__DIR__ . '/fixtures/broken_cloze_questions.xml');
+        $importer = $qformat = new qformat_xml();
+
+        // 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);
+        $this->assertContains('The answer must be a number, for example -1.234 or 3e8, or \'*\'.', $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);
+        $this->assertContains('The question text must include at least one embedded answer.', $output);
+
+        // No question  have been imported.
+        $this->assertCount(0, $questions);
+    }
 }

question/type/multianswer/edit_multianswer_form.php

@@ -398,7 +398,7 @@ class qtype_multianswer_edit_form extends question_edit_form {
                             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'));
@@ -456,76 +456,12 @@ class qtype_multianswer_edit_form extends question_edit_form {
         parent::set_data($question);
     }
 
-    /**
-     * Validate that a string is a nubmer 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.
-     */
-    protected function is_valid_number($x) {
-        if (is_null($this->ap)) {
-            $this->ap = new qtype_numerical_answer_processor(array());
-        }
-
-        list($value, $unit) = $this->ap->apply_units($x);
-
-        return !is_null($value) && !$unit;
-    }
-
-
     public function validation($data, $files) {
         $errors = parent::validation($data, $files);
 
         $questiondisplay = qtype_multianswer_extract_question($data['questiontext']);
 
-        if (isset($questiondisplay->options->questions)) {
+        $errors = array_merge($errors, qtype_multianswer_validate_question($questiondisplay));
-            $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');
-            }
-        }
 
         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['confirmsave'] = 'Confirm then save {$a}';
 $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 @@ defined('MOODLE_INTERNAL') || die();
 
 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(stdClass $question) : array {
+    $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   = 2018120300;
+$plugin->version   = 2019022500;
 
 $plugin->requires  = 2018112800;
 $plugin->dependencies = array(

question/type/numerical/edit_numerical_form.php

@@ -319,22 +319,7 @@ class qtype_numerical_edit_form extends question_edit_form {
      * @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);
-    }
-
-    /**
-     * Validate that a string is a nubmer 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.
-     */
-    protected function is_valid_number($x) {
-        if (is_null($this->ap)) {
-            $this->ap = new qtype_numerical_answer_processor(array());
-        }
-
-        list($value, $unit) = $this->ap->apply_units($x);
-
-        return !is_null($value) && !$unit;
     }
 
     /**

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(string $x) : bool {
+        $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/form_test.php

@@ -1,81 +0,0 @@
-<?php
-// This file is part of Moodle - http://moodle.org/
-//
-// Moodle is free software: you can redistribute it and/or modify
-// it under the terms of the GNU General Public License as published by
-// the Free Software Foundation, either version 3 of the License, or
-// (at your option) any later version.
-//
-// Moodle is distributed in the hope that it will be useful,
-// but WITHOUT ANY WARRANTY; without even the implied warranty of
-// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-// GNU General Public License for more details.
-//
-// You should have received a copy of the GNU General Public License
-// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
-
-/**
- * Unit tests for (some of) question/type/numerical/edit_numerical_form.php.
- *
- * @package    qtype
- * @subpackage numerical
- * @copyright  2011 The Open University
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-
-
-defined('MOODLE_INTERNAL') || die();
-
-global $CFG;
-require_once($CFG->dirroot . '/question/type/numerical/edit_numerical_form.php');
-
-
-/**
- * Test sub-class, so we can force the locale.
- *
- * @copyright  2011 The Open University
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-class test_qtype_numerical_edit_form extends qtype_numerical_edit_form {
-    public function __construct() {
-        // Warning, avoid running the parent constructor. That means the form is
-        // not properly tested but for now that is OK, we are only testing a few
-        // methods.
-        $this->ap = new qtype_numerical_answer_processor(array(), false, ',', ' ');
-    }
-    public function is_valid_number($x) {
-        return parent::is_valid_number($x);
-    }
-}
-
-
-/**
- * Unit tests for question/type/numerical/edit_numerical_form.php.
- *
- * @copyright  2011 The Open University
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-class qtype_numerical_form_test extends advanced_testcase {
-    public static $includecoverage = array(
-        'question/type/numerical/edit_numerical_form.php'
-    );
-
-    protected $form;
-
-    protected function setUp() {
-        $this->form = new test_qtype_numerical_edit_form();
-    }
-
-    protected function tearDown() {
-        $this->form = null;
-    }
-
-    public function test_is_valid_number() {
-        $this->assertTrue($this->form->is_valid_number('1,001'));
-        $this->assertTrue($this->form->is_valid_number('1.001'));
-        $this->assertTrue($this->form->is_valid_number('1'));
-        $this->assertTrue($this->form->is_valid_number('1,e8'));
-        $this->assertFalse($this->form->is_valid_number('1001 xxx'));
-        $this->assertTrue($this->form->is_valid_number('1.e8'));
-    }
-}

question/type/numerical/tests/questiontype_test.php

@@ -169,4 +169,13 @@ class qtype_numerical_test extends advanced_testcase {
             }
         }
     }
+
+    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'));
+    }
 }

question/upgrade.txt

@@ -1,5 +1,12 @@
 This files describes API changes for code that uses the question API.
 
+=== 3.7 ===
+
+The code for the is_valid_number function that was duplicated in the
+qtype_numerical and qtype_multianswer plugins in the qtype_numerical_edit_form 
+and qtype_multianswer_edit_form classes has been moved to a static function
+in the qtype_numerical class of the qtype_numerical plugin.
+
 === 3.5 ===
 
 1) The question format exportprocess function now adds a