diff --git a/question/format/multianswer/format.php b/question/format/multianswer/format.php index b7c412ea056..2ab3109fabf 100644 --- a/question/format/multianswer/format.php +++ b/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; 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 00000000000..e136ea6c4f1 --- /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 00000000000..bb443ab5742 --- /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 00000000000..f76aa66b802 --- /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 00000000000..0783af81c6d --- /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 8e1d301f8bf..13aae67f6d9 100644 --- a/question/format/multianswer/tests/multianswerformat_test.php +++ b/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); + } } diff --git a/question/format/xml/format.php b/question/format/xml/format.php index 6ea5f5f290b..98fe6d2c725 100644 --- a/question/format/xml/format.php +++ b/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; } - - $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'])) { @@ -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); diff --git a/question/format/xml/tests/fixtures/broken_cloze_questions.xml b/question/format/xml/tests/fixtures/broken_cloze_questions.xml new file mode 100644 index 00000000000..33a0f7ce0cf --- /dev/null +++ b/question/format/xml/tests/fixtures/broken_cloze_questions.xml @@ -0,0 +1,82 @@ + + + + + + Multianswer question without subquestions + + + Please select the fruits. + + + + + 0.3333333 + 0 + + + + + + + Multichoice subquestion without choices + + + Please select the fruits {1:MULTICHOICE} + + + + + 0.3333333 + 0 + + + + + + + Multichoice subquestion with only one choice + + + Please select the fruits {1:MULTICHOICE:=Apple#Correct} + + + + + 0.3333333 + 0 + + + + + + + Multichoice subquestion with no completely correct answer + + + Please select the fruits {1:MULTICHOICE:Pear#Incorrect~%50%Apple#Correct} + + + + + 0.3333333 + 0 + + + + + + + Numerical subquestion with buggy answer + + + What grade would you give it? {3:NUMERICAL:=zero} + + + + + 0.3333333 + 0 + + + \ No newline at end of file diff --git a/question/format/xml/tests/qformat_xml_import_export_test.php b/question/format/xml/tests/qformat_xml_import_export_test.php index 13724c39ac0..a0af0afd865 100644 --- a/question/format/xml/tests/qformat_xml_import_export_test.php +++ b/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); + } } diff --git a/question/type/multianswer/edit_multianswer_form.php b/question/type/multianswer/edit_multianswer_form.php index 3644514c535..56e1a55e87b 100644 --- a/question/type/multianswer/edit_multianswer_form.php +++ b/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)) { - $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 4b000de77f2..b51660ff1f3 100644 --- a/question/type/multianswer/lang/en/qtype_multianswer.php +++ b/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'; diff --git a/question/type/multianswer/questiontype.php b/question/type/multianswer/questiontype.php index c821490486b..79811ce851c 100644 --- a/question/type/multianswer/questiontype.php +++ b/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. @@ -515,3 +516,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; +} diff --git a/question/type/multianswer/version.php b/question/type/multianswer/version.php index d7d98ed9b19..832efc1c52e 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 = 2018120300; +$plugin->version = 2019022500; $plugin->requires = 2018112800; $plugin->dependencies = array( diff --git a/question/type/numerical/edit_numerical_form.php b/question/type/numerical/edit_numerical_form.php index fd8eefdd612..88027619d06 100644 --- a/question/type/numerical/edit_numerical_form.php +++ b/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); - } - - /** - * 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; + return $answer == '*' || qtype_numerical::is_valid_number($answer); } /** diff --git a/question/type/numerical/questiontype.php b/question/type/numerical/questiontype.php index 4f0628a25ee..82f6898acde 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(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); diff --git a/question/type/numerical/tests/form_test.php b/question/type/numerical/tests/form_test.php deleted file mode 100644 index 0bdae49cc36..00000000000 --- a/question/type/numerical/tests/form_test.php +++ /dev/null @@ -1,81 +0,0 @@ -. - -/** - * 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')); - } -} diff --git a/question/type/numerical/tests/questiontype_test.php b/question/type/numerical/tests/questiontype_test.php index 1d19a2fadd9..90a087e0f22 100644 --- a/question/type/numerical/tests/questiontype_test.php +++ b/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')); + } } diff --git a/question/upgrade.txt b/question/upgrade.txt index f34bd6a4626..c1a6e0bc9b1 100644 --- a/question/upgrade.txt +++ b/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