From ffe4e082b089f75cad69ddb27a9a3243617dc748 Mon Sep 17 00:00:00 2001 From: Jean-Michel Vedrine Date: Mon, 18 Feb 2019 17:48:32 +0100 Subject: [PATCH] MDL-63905 qtype_multianswer: validate imported questions --- question/format/multianswer/format.php | 7 ++ .../tests/fixtures/broken_multianswer_1.txt | 1 + .../tests/fixtures/broken_multianswer_2.txt | 1 + .../tests/fixtures/broken_multianswer_3.txt | 1 + .../tests/fixtures/broken_multianswer_4.txt | 1 + .../tests/multianswerformat_test.php | 77 +++++++++++++++++++ question/format/xml/format.php | 19 +++-- .../multianswer/edit_multianswer_form.php | 52 +------------ .../multianswer/lang/en/qtype_multianswer.php | 1 + question/type/multianswer/questiontype.php | 63 +++++++++++++++ question/type/multianswer/version.php | 2 +- .../type/numerical/edit_numerical_form.php | 2 +- question/type/numerical/questiontype.php | 11 +++ .../numerical/tests/questiontype_test.php | 9 +++ 14 files changed, 190 insertions(+), 57 deletions(-) create mode 100644 question/format/multianswer/tests/fixtures/broken_multianswer_1.txt create mode 100644 question/format/multianswer/tests/fixtures/broken_multianswer_2.txt create mode 100644 question/format/multianswer/tests/fixtures/broken_multianswer_3.txt create mode 100644 question/format/multianswer/tests/fixtures/broken_multianswer_4.txt diff --git a/question/format/multianswer/format.php b/question/format/multianswer/format.php index b7c412ea0569f..2ab3109fabf16 100644 --- a/question/format/multianswer/format.php +++ b/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; diff --git a/question/format/multianswer/tests/fixtures/broken_multianswer_1.txt b/question/format/multianswer/tests/fixtures/broken_multianswer_1.txt new file mode 100644 index 0000000000000..e136ea6c4f1d1 --- /dev/null +++ b/question/format/multianswer/tests/fixtures/broken_multianswer_1.txt @@ -0,0 +1 @@ +Please select the fruits {1:MULTICHOICE:=Apple#Correct} \ No newline at end of file diff --git a/question/format/multianswer/tests/fixtures/broken_multianswer_2.txt b/question/format/multianswer/tests/fixtures/broken_multianswer_2.txt new file mode 100644 index 0000000000000..bb443ab574285 --- /dev/null +++ b/question/format/multianswer/tests/fixtures/broken_multianswer_2.txt @@ -0,0 +1 @@ +Please select the fruits {1:MULTICHOICE:Pear#Incorrect~%50%Apple#Correct} \ No newline at end of file diff --git a/question/format/multianswer/tests/fixtures/broken_multianswer_3.txt b/question/format/multianswer/tests/fixtures/broken_multianswer_3.txt new file mode 100644 index 0000000000000..f76aa66b802aa --- /dev/null +++ b/question/format/multianswer/tests/fixtures/broken_multianswer_3.txt @@ -0,0 +1 @@ +What grade would you give it? {3:NUMERICAL:=zero} \ No newline at end of file diff --git a/question/format/multianswer/tests/fixtures/broken_multianswer_4.txt b/question/format/multianswer/tests/fixtures/broken_multianswer_4.txt new file mode 100644 index 0000000000000..0783af81c6de0 --- /dev/null +++ b/question/format/multianswer/tests/fixtures/broken_multianswer_4.txt @@ -0,0 +1 @@ +Please select the fruits. \ No newline at end of file diff --git a/question/format/multianswer/tests/multianswerformat_test.php b/question/format/multianswer/tests/multianswerformat_test.php index 8e1d301f8bfed..13aae67f6d9ab 100644 --- a/question/format/multianswer/tests/multianswerformat_test.php +++ b/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); + } } diff --git a/question/format/xml/format.php b/question/format/xml/format.php index d3b9c82ae42f8..177e8d143eb8b 100644 --- a/question/format/xml/format.php +++ b/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); diff --git a/question/type/multianswer/edit_multianswer_form.php b/question/type/multianswer/edit_multianswer_form.php index 3644514c535c1..e6c466d446ec0 100644 --- a/question/type/multianswer/edit_multianswer_form.php +++ b/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 || diff --git a/question/type/multianswer/lang/en/qtype_multianswer.php b/question/type/multianswer/lang/en/qtype_multianswer.php index 9a32ef19e130b..505e145fa8800 100644 --- a/question/type/multianswer/lang/en/qtype_multianswer.php +++ b/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'; diff --git a/question/type/multianswer/questiontype.php b/question/type/multianswer/questiontype.php index 9f51c6dbe4a9b..c4a516d5758fa 100644 --- a/question/type/multianswer/questiontype.php +++ b/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; +} diff --git a/question/type/multianswer/version.php b/question/type/multianswer/version.php index 359212e49ff30..1f30906cae34d 100644 --- a/question/type/multianswer/version.php +++ b/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( diff --git a/question/type/numerical/edit_numerical_form.php b/question/type/numerical/edit_numerical_form.php index fd8eefdd61204..4803b392ca871 100644 --- a/question/type/numerical/edit_numerical_form.php +++ b/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); } /** diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index 4f0628a25ee2a..d72a79f42e5e7 100644 --- a/question/type/numerical/questiontype.php +++ b/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); diff --git a/question/type/numerical/tests/questiontype_test.php b/question/type/numerical/tests/questiontype_test.php index 1d19a2fadd954..90a087e0f2212 100644 --- a/question/type/numerical/tests/questiontype_test.php +++ b/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')); + } }