MDL-63456 question: Improve Aiken error handling and multichoice errors

question/format/aiken/format.php

@@ -59,39 +59,67 @@ public function provide_import() {
 
     public function readquestions($lines) {
         $questions = array();
-        $question = $this->defaultquestion();
+        $question = null;
         $endchar = chr(13);
+        $linenumber = 0;
         foreach ($lines as $line) {
             $stp = strpos($line, $endchar, 0);
             $newlines = explode($endchar, $line);
             $linescount = count($newlines);
             for ($i=0; $i < $linescount; $i++) {
+                $linenumber++;
                 $nowline = trim($newlines[$i]);
                 // Go through the array and build an object called $question
                 // When done, add $question to $questions.
                 if (strlen($nowline) < 2) {
                     continue;
                 }
-                if (preg_match('/^[A-Z][).][ \t]/', $nowline)) {
+                if (preg_match('/^[A-Z][).][ \t]?/', $nowline)) {
+                    if (is_null($question)) {
+                        // We have a response line, but we aren't currently in a question.
+                        $this->error(get_string('questionnotstarted', 'qformat_aiken', $linenumber));
+                        continue;
+                    }
+
                     // A choice. Trim off the label and space, then save.
                     $question->answer[] = $this->text_field(
                             htmlspecialchars(trim(substr($nowline, 2)), ENT_NOQUOTES));
                     $question->fraction[] = 0;
                     $question->feedback[] = $this->text_field('');
                 } else if (preg_match('/^ANSWER:/', $nowline)) {
+                    if (is_null($question)) {
+                        // We have an answer line, but we aren't currently in a question.
+                        $this->error(get_string('questionnotstarted', 'qformat_aiken', $linenumber));
+                        continue;
+                    }
+
                     // The line that indicates the correct answer. This question is finised.
                     $ans = trim(substr($nowline, strpos($nowline, ':') + 1));
                     $ans = substr($ans, 0, 1);
                     // We want to map A to 0, B to 1, etc.
                     $rightans = ord($ans) - ord('A');
+
+                    if (count($question->answer) < 2) {
+                        // The multichoice question requires at least 2 answers, or there will be a failure later.
+                        $this->error(get_string('questionmissinganswers', 'qformat_aiken', $linenumber), '', $question->name);
+                        $question = null;
+                        continue;
+                    }
+
                     $question->fraction[$rightans] = 1;
                     $questions[] = $question;
 
-                    // Clear array for next question set.
-                    $question = $this->defaultquestion();
+                    // Clear variable for next question set.
+                    $question = null;
                     continue;
                 } else {
                     // Must be the first line of a new question, since no recognised prefix.
+                    if (!is_null($question)) {
+                        // In this case, there was already an open question that we didn't complete. It is being discarded.
+                        $this->error(get_string('questionnotcomplete', 'qformat_aiken', $linenumber), '', $question->name);
+                    }
+
+                    $question = $this->defaultquestion();
                     $question->qtype = 'multichoice';
                     $question->name = $this->create_default_question_name($nowline, get_string('questionname', 'question'));
                     $question->questiontext = htmlspecialchars(trim($nowline), ENT_NOQUOTES);

question/format/aiken/lang/en/qformat_aiken.php

@@ -26,3 +26,6 @@
 $string['pluginname_help'] = 'This is a simple format for importing multiple choice questions from a text file.';
 $string['pluginname_link'] = 'qformat/aiken';
 $string['privacy:metadata'] = 'The Aiken question format plugin does not store any personal data.';
+$string['questionmissinganswers'] = 'Question must have at least 2 answers on line {$a}';
+$string['questionnotcomplete'] = 'Question not completed before next question start on line {$a}';
+$string['questionnotstarted'] = 'Question not started on line {$a}';

question/format/aiken/tests/aikenformat_test.php

@@ -0,0 +1,87 @@
+<?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 the Moodle Aiken format.
+ *
+ * @package    qformat_aiken
+ * @copyright  2018 Eric Merrill (eric.a.merrill@gmail.com)
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+
+defined('MOODLE_INTERNAL') || die();
+
+global $CFG;
+require_once($CFG->libdir . '/questionlib.php');
+require_once($CFG->dirroot . '/question/format.php');
+require_once($CFG->dirroot . '/question/format/aiken/format.php');
+require_once($CFG->dirroot . '/question/engine/tests/helpers.php');
+
+
+/**
+ * Unit tests for the matching question definition class.
+ *
+ * @copyright  2018 Eric Merrill (eric.a.merrill@gmail.com)
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class aikenformat_test extends question_testcase {
+    public function test_readquestions() {
+        global $CFG;
+
+        $lines = file($CFG->dirroot.'/question/format/aiken/tests/fixtures/aiken_errors.txt');
+        $importer = new qformat_aiken($lines);
+
+        // The importer echos 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 A question with too few answers', $output);
+        $this->assertContains('Question must have at least 2 answers on line 3', $output);
+        $this->assertContains('Question not started on line 5', $output);
+        $this->assertContains('Question not started on line 7', $output);
+        $this->assertContains('Error importing question A question started but not finished', $output);
+        $this->assertContains('Question not completed before next question start on line 18', $output);
+
+        // There are two expected questions.
+        $this->assertCount(2, $questions);
+
+        $q1 = null;
+        $q2 = null;
+        foreach ($questions as $question) {
+            if ($question->name === 'A good question') {
+                $q1 = $question;
+            } else if ($question->name === 'A second good question') {
+                $q2 = $question;
+            }
+        }
+
+        // Check the first good question.
+        $this->assertCount(2, $q1->answer);
+        $this->assertEquals(1, $q1->fraction[0]);
+        $this->assertEquals('Correct', $q1->answer[0]['text']);
+        $this->assertEquals('Incorrect', $q1->answer[1]['text']);
+
+        // Check the second good question.
+        $this->assertCount(2, $q2->answer);
+        $this->assertEquals(1, $q2->fraction[1]);
+        $this->assertEquals('Incorrect (No space)', $q2->answer[0]['text']);
+        $this->assertEquals('Correct (No space)', $q2->answer[1]['text']);
+    }
+}

question/format/aiken/tests/fixtures/aiken_errors.txt

@@ -0,0 +1,21 @@
+A question with too few answers
+A) Only answer
+ANSWER: A
+
+A) Question not started
+
+ANSWER: Question not started
+
+A good question
+A) Correct
+B) Incorrect
+ANSWER: A
+
+A question started but not finished
+A) Correct-ish
+B) Incorrect-ish
+
+A second good question
+A)Incorrect (No space)
+B)Correct (No space)
+ANSWER: B

question/type/multichoice/questiontype.php

@@ -60,7 +60,7 @@ public function save_question_options($question) {
             }
         }
         if ($answercount < 2) { // Check there are at lest 2 answers for multiple choice.
-            $result->notice = get_string('notenoughanswers', 'qtype_multichoice', '2');
+            $result->error = get_string('notenoughanswers', 'qtype_multichoice', '2');
             return $result;
         }