MDL-46148 qtype_calculated: fix validation when importing.
authorTim Hunt <T.J.Hunt@open.ac.uk>
Wed, 9 Jul 2014 12:35:09 +0000 (13:35 +0100)
committerTim Hunt <T.J.Hunt@open.ac.uk>
Wed, 9 Jul 2014 12:35:09 +0000 (13:35 +0100)
In order to do this in a sane way, I cleaned up a lot of old mess,
inclduing:

1. Previously, qtype_calcuated used ->answeres when importing, and
->answer when saving the form. This was crazy, so I fixed it, and
stripped out the code that made the alternative variable name work.

2. Similarly, it could handle ->answer being either an array, such as
you would get form the HTML editor, or a simple string, which is what
you get form the form. I simplified that too.

3. Finally, I made import use a transaction around saving each
question, so we don't get half questions in the database when an error
occurs.

question/format.php
question/format/webct/format.php
question/format/xml/format.php
question/type/calculated/questiontype.php
question/type/calculatedmulti/questiontype.php
question/type/calculatedsimple/questiontype.php

index e284d8c..ad6a136 100644 (file)
@@ -357,6 +357,7 @@ class qformat_default {
         $count = 0;
 
         foreach ($questions as $question) {   // Process and store each question
+            $transaction = $DB->start_delegated_transaction();
 
             // reset the php timeout
             core_php_time_limit::raise();
@@ -429,9 +430,14 @@ class qformat_default {
 
             if (!empty($result->error)) {
                 echo $OUTPUT->notification($result->error);
+                // Can't use $transaction->rollback(); since it requires an exception,
+                // and I don't want to rewrite this code to change the error handling now.
+                $DB->force_transaction_rollback();
                 return false;
             }
 
+            $transaction->allow_commit();
+
             if (!empty($result->notice)) {
                 echo $OUTPUT->notification($result->notice);
                 return true;
index 57c3175..2c003d8 100644 (file)
@@ -606,12 +606,9 @@ class qformat_webct extends qformat_default {
                 // Calculated Question.
                 $question = $this->defaultquestion();
                 $question->qtype = 'calculated';
-                $question->answers = array(); // No problem as they go as :FORMULA: from webct.
+                $question->answer = array(); // No problem as they go as :FORMULA: from webct.
                 $question->units = array();
                 $question->dataset = array();
-
-                // To make us pass the end-of-question sanity checks.
-                $question->answer = array('dummy');
                 $question->fraction = array('1.0');
                 $question->feedback = array();
 
@@ -739,7 +736,7 @@ class qformat_webct extends qformat_default {
             if (preg_match('~^:FORMULA:(.*)~i', $line, $webctoptions)) {
                 // Answer for a calculated question.
                 ++$currentchoice;
-                $question->answers[$currentchoice] =
+                $question->answer[$currentchoice] =
                         qformat_webct_convert_formula($webctoptions[1]);
 
                 // Default settings.
index 3fc9b13..1c55f34 100644 (file)
@@ -779,7 +779,7 @@ class qformat_xml extends qformat_default {
 
         // Get answers array.
         $answers = $question['#']['answer'];
-        $qo->answers = array();
+        $qo->answer = array();
         $qo->feedback = array();
         $qo->fraction = array();
         $qo->tolerance = array();
@@ -793,7 +793,7 @@ class qformat_xml extends qformat_default {
             if (empty($ans->answer['text'])) {
                 $ans->answer['text'] = '*';
             }
-            $qo->answers[] = $ans->answer;
+            $qo->answer[] = $ans->answer['text'];
             $qo->feedback[] = $ans->feedback;
             $qo->tolerance[] = $answer['#']['tolerance'][0]['#'];
             // Fraction as a tag is deprecated.
index db7cedd..84faed8 100644 (file)
@@ -131,11 +131,13 @@ class qtype_calculated extends question_type {
 
     public function save_question_options($question) {
         global $CFG, $DB;
+
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
+
         // The code is used for calculated, calculatedsimple and calculatedmulti qtypes.
         $context = $question->context;
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
+
         // Calculated options.
         $update = true;
         $options = $DB->get_record('question_calculated_options',
@@ -185,14 +187,7 @@ class qtype_calculated extends question_type {
             $units = $result->units;
         }
 
-        // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
-            if (is_array($answerdata)) {
-                $answerdata = $answerdata['text'];
-            }
+        foreach ($question->answer as $key => $answerdata) {
             if (trim($answerdata) == '') {
                 continue;
             }
@@ -471,6 +466,25 @@ class qtype_calculated extends question_type {
         }
     }
 
+    /**
+     * Validate data before save.
+     * @param stdClass $question data from the form / import file.
+     */
+    protected function validate_question_data($question) {
+        $this->validate_text($question->questiontext); // Yes, really no ['text'].
+
+        if (isset($question->generalfeedback['text'])) {
+            $this->validate_text($question->generalfeedback['text']);
+        } else if (isset($question->generalfeedback)) {
+            $this->validate_text($question->generalfeedback); // Because question import is weird.
+        }
+
+        foreach ($question->answer as $key => $answer) {
+            $this->validate_answer($answer);
+            $this->validate_text($question->feedback[$key]['text']);
+        }
+    }
+
     /**
      * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
      * so that they can be saved
@@ -483,13 +497,13 @@ class qtype_calculated extends question_type {
      * @param object $form
      * @param int $questionfromid default = '0'
      */
-    public function preparedatasets($form , $questionfromid = '0') {
+    public function preparedatasets($form, $questionfromid = '0') {
 
         // The dataset names present in the edit_question_form and edit_calculated_form
         // are retrieved.
         $possibledatasets = $this->find_dataset_names($form->questiontext);
         $mandatorydatasets = array();
-        foreach ($form->answers as $key => $answer) {
+        foreach ($form->answer as $key => $answer) {
             $mandatorydatasets += $this->find_dataset_names($answer);
         }
         // If there are identical datasetdefs already saved in the original question
@@ -579,12 +593,6 @@ class qtype_calculated extends question_type {
     public function save_question($question, $form) {
         global $DB;
 
-        if (isset($form->correctfeedback)) {
-            $this->validate_text($form->correctfeedback['text']);
-            $this->validate_text($form->partiallycorrectfeedback['text']);
-            $this->validate_text($form->incorrectfeedback['text']);
-        }
-
         if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
             $question = parent::save_question($question, $form);
             return $question;
@@ -605,14 +613,6 @@ class qtype_calculated extends question_type {
             case '' :
             case 'question': // Coming from the first page, creating the second.
                 if (empty($form->id)) { // or a new question $form->id is empty.
-                    // Make it impossible to save bad formulas anywhere.
-                    $this->validate_text($form->questiontext['text']);
-                    $this->validate_text($form->generalfeedback['text']);
-                    foreach ($form->answer as $key => $answer) {
-                        $this->validate_answer($answer);
-                        $this->validate_text($form->feedback[$key]['text']);
-                    }
-
                     $question = parent::save_question($question, $form);
                     // Prepare the datasets using default $questionfromid.
                     $this->preparedatasets($form);
index 283ccd4..2f90d7b 100644 (file)
@@ -42,6 +42,9 @@ class qtype_calculatedmulti extends qtype_calculated {
         global $CFG, $DB;
         $context = $question->context;
 
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
+
         // Calculated options.
         $update = true;
         $options = $DB->get_record('question_calculated_options',
@@ -72,10 +75,7 @@ class qtype_calculatedmulti extends qtype_calculated {
         }
 
         // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
+        foreach ($question->answer as $key => $answerdata) {
             if (is_array($answerdata)) {
                 $answerdata = $answerdata['text'];
             }
@@ -163,6 +163,13 @@ class qtype_calculatedmulti extends qtype_calculated {
         }
     }
 
+    protected function validate_question_data($question) {
+        parent::validate_question_data($question);
+        $this->validate_text($question->correctfeedback['text']);
+        $this->validate_text($question->partiallycorrectfeedback['text']);
+        $this->validate_text($question->incorrectfeedback['text']);
+    }
+
     protected function make_question_instance($questiondata) {
         question_bank::load_question_definition_classes($this->name());
         if ($questiondata->options->single) {
index a58a172..68f45cb 100644 (file)
@@ -43,11 +43,9 @@ class qtype_calculatedsimple extends qtype_calculated {
     public function save_question_options($question) {
         global $CFG, $DB;
         $context = $question->context;
-        // Get old answers.
 
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
 
         // Get old versions of the objects.
         if (!$oldanswers = $DB->get_records('question_answers',
@@ -69,10 +67,7 @@ class qtype_calculatedsimple extends qtype_calculated {
             $units = &$result->units;
         }
         // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
+        foreach ($question->answer as $key => $answerdata) {
             if (is_array($answerdata)) {
                 $answerdata = $answerdata['text'];
             }