MDL-20636 numerical qtype: assorted changes

1. database upgrade to merge instructions into the questiontext field,
and remove the UNITDISPLAY option.

2. Changes to the validation in deferred feedback mode, so students
are warned about incomplete answers.

3. Using this to wark students if they enter an answer that is not
recognised as a number.

backup/moodle2/backup_qtype_plugin.class.php

@@ -101,8 +101,7 @@ protected function add_question_numerical_options($element) {
         // Define the elements
         $options = new backup_nested_element('numerical_options');
         $option = new backup_nested_element('numerical_option', array('id'), array(
-            'instructions', 'instructionsformat', 'showunits', 'unitsleft',
-            'unitgradingtype', 'unitpenalty'));
+            'showunits', 'unitsleft', 'unitgradingtype', 'unitpenalty'));
 
         // Build the tree
         $element->add_child($options);
@@ -192,15 +191,4 @@ public static function get_components_and_fileareas($filter = null) {
         }
         return $components;
     }
-
-    /**
-     * Returns one array with filearea => mappingname elements for the qtype
-     *
-     * Used by {@link get_components_and_fileareas} to know about all the qtype
-     * files to be processed both in backup and restore.
-     */
-    public static function get_qtype_fileareas() {
-        // By default, return empty array, only qtypes having own fileareas will override this
-        return array();
-    }
 }

question/behaviour/deferredfeedback/behaviour.php

@@ -68,6 +68,33 @@ public function process_action(question_attempt_pending_step $pendingstep) {
         }
     }
 
+    /*
+     * Like the parent method, except that when a respones is gradable, but not
+     * completely, we move it to the invalid state.
+     *
+     * TODO refactor, to remove the duplication.
+     */
+    public function process_save(question_attempt_pending_step $pendingstep) {
+        if ($this->qa->get_state()->is_finished()) {
+            return question_attempt::DISCARD;
+        } else if (!$this->qa->get_state()->is_active()) {
+            throw new coding_exception('Question is not active, cannot process_actions.');
+        }
+
+        if ($this->is_same_response($pendingstep)) {
+            return question_attempt::DISCARD;
+        }
+
+        if ($this->is_complete_response($pendingstep)) {
+            $pendingstep->set_state(question_state::$complete);
+        } else if ($this->question->is_gradable_response($pendingstep->get_qt_data())) {
+            $pendingstep->set_state(question_state::$invalid);
+        } else {
+            $pendingstep->set_state(question_state::$todo);
+        }
+        return question_attempt::KEEP;
+    }
+
     public function summarise_action(question_attempt_step $step) {
         if ($step->has_behaviour_var('comment')) {
             return $this->summarise_manual_comment($step);

question/format/gift/simpletest/testgiftformat.php

@@ -563,8 +563,6 @@ public function test_export_numerical() {
             'options' => (object) array(
                 'id' => 123,
                 'question' => 666,
-                'instructions' => '',
-                'instructionsformat' => FORMAT_MOODLE,
                 'showunits' => 0,
                 'unitsleft' => 0,
                 'showunits' => 2,

question/type/ddwtos/simpletest/testwalkthrough.php

@@ -198,7 +198,7 @@ public function test_deferred_feedback() {
         $this->process_submission(array('p1' => '1', 'p2' => '2'));
 
         // Verify.
-        $this->check_current_state(question_state::$todo);
+        $this->check_current_state(question_state::$invalid);
         $this->check_current_mark(null);
         $this->check_current_output(
                 $this->get_contains_drop_box_expectation('p1', 1, false),
@@ -338,7 +338,7 @@ public function test_deferred_feedback_partial_answer() {
         $this->process_submission(array('p1' => '1', 'p2' => '0', 'p3' => '0'));
 
         // Verify.
-        $this->check_current_state(question_state::$todo);
+        $this->check_current_state(question_state::$invalid);
         $this->check_current_mark(null);
         $this->check_current_output(
                 $this->get_contains_drop_box_expectation('p1', 1, false),

question/type/match/simpletest/testwalkthrough.php

@@ -124,7 +124,7 @@ public function test_deferred_feedback_partial_answer() {
                 'sub1' => $orderforchoice[2], 'sub2' => '0', 'sub3' => '0'));
 
         // Verify.
-        $this->check_current_state(question_state::$todo);
+        $this->check_current_state(question_state::$invalid);
         $this->check_current_mark(null);
         $this->check_current_output(
                 $this->get_contains_select_expectation('sub0', $choices, 1, true),

question/type/numerical/backup/moodle2/backup_qtype_numerical_plugin.class.php

@@ -83,6 +83,6 @@ protected function define_question_plugin_structure() {
      * files to be processed both in backup and restore.
      */
     public static function get_qtype_fileareas() {
-        return array('instruction' => 'question_created');
+        return array();
     }
 }

question/type/numerical/db/install.xml

@@ -22,10 +22,8 @@
     <TABLE NAME="question_numerical_options" COMMENT="Options for questions of type numerical This table is also used by the calculated question type" PREVIOUS="question_numerical" NEXT="question_numerical_units">
       <FIELDS>
         <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" UNSIGNED="true" SEQUENCE="true" NEXT="question"/>
-        <FIELD NAME="question" TYPE="int" LENGTH="10" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="id" NEXT="instructions"/>
-        <FIELD NAME="instructions" TYPE="text" LENGTH="small" NOTNULL="false" SEQUENCE="false" COMMENT="Feedback shown for any incorrect response." PREVIOUS="question" NEXT="instructionsformat"/>
-        <FIELD NAME="instructionsformat" TYPE="int" LENGTH="2" NOTNULL="true" UNSIGNED="false" DEFAULT="0" SEQUENCE="false" PREVIOUS="instructions" NEXT="showunits"/>
-        <FIELD NAME="showunits" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" COMMENT="display units as multichoice" PREVIOUS="instructionsformat" NEXT="unitsleft"/>
+        <FIELD NAME="question" TYPE="int" LENGTH="10" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" PREVIOUS="id" NEXT="showunits"/>
+        <FIELD NAME="showunits" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" COMMENT="display units as multichoice" PREVIOUS="question" NEXT="unitsleft"/>
         <FIELD NAME="unitsleft" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="false" DEFAULT="0" SEQUENCE="false" COMMENT="display the unit at left as in $1.00" PREVIOUS="showunits" NEXT="unitgradingtype"/>
         <FIELD NAME="unitgradingtype" TYPE="int" LENGTH="4" NOTNULL="true" UNSIGNED="true" DEFAULT="0" SEQUENCE="false" COMMENT="0 no penalty, 1 response grade, 2 total grade" PREVIOUS="unitsleft" NEXT="unitpenalty"/>
         <FIELD NAME="unitpenalty" TYPE="number" LENGTH="12" NOTNULL="true" UNSIGNED="true" DEFAULT="0.1" SEQUENCE="false" DECIMALS="7" PREVIOUS="unitgradingtype"/>

question/type/numerical/db/upgrade.php

@@ -59,7 +59,7 @@ function xmldb_qtype_numerical_upgrade($oldversion) {
         if (!$dbman->table_exists($table)) {
             // $dbman->create_table doesnt return a result, we just have to trust it
             $dbman->create_table($table);
-        }//else
+        }
         upgrade_plugin_savepoint(true, 2009100100, 'qtype', 'numerical');
     }
 
@@ -96,5 +96,176 @@ function xmldb_qtype_numerical_upgrade($oldversion) {
         upgrade_plugin_savepoint(true, 2009100101, 'qtype', 'numerical');
     }
 
+    if ($oldversion < 2011042600) {
+        // Get rid of the instructions field by adding it to the qestion
+        // text. Also, if the unit was set to be displayed beside the input,
+        // deal with that within the question text too.
+
+        // The hard-coded constants used here are:
+        // 2 = the old qtype_numerical::UNITDISPLAY for ->showunits
+        // 3 = qtype_numerical::UNITNONE
+
+        $fs = get_file_storage();
+
+        $rs = $DB->get_recordset_sql('
+                SELECT q.id AS questionid,
+                       q.questiontext,
+                       q.questiontextformat,
+                       qc.contextid,
+                       qno.id AS qnoid,
+                       qno.instructions,
+                       qno.instructionsformat,
+                       qno.showunits,
+                       qno.unitsleft,
+                       qnu.unit AS defaultunit
+
+                  FROM {question} q
+                  FROM {question_categories} qc ON qc.id = q.category
+                  JOIN {question_numerical_options} qno ON qno.question = q.id
+                  JOIN {question_numerical_units} qnu ON qnu.id = (
+                            SELECT min(id)
+                              FROM {question_numerical_units}
+                             WHERE question = q.id AND ABS(multiplier - 1) < 0.0000000001)');
+        foreach ($rs as $numericaloptions) {
+            if ($numericaloptions->showunits != 2 && empty($numericaloptions->instructions)) {
+                // Nothing to do for this question.
+                continue;
+            }
+
+            $ishtml = qtype_numerical_convert_text_format($numericaloptions);
+
+            $response = '_______________';
+            if ($numericaloptions->showunits == 2) {
+                if ($numericaloptions->unitsleft) {
+                    $response = $numericaloptions->defaultunit . ' _______________';
+                } else {
+                    $response = '_______________ ' . $numericaloptions->defaultunit;
+                }
+
+                $DB->set_field('question_numerical_options', 'showunits', 3,
+                        array('id' => $numericaloptions->qnoid));
+            }
+
+            if ($ishtml) {
+                $numericaloptions->questiontext .= '<p>' . $response . '</p>';
+            } else {
+                $numericaloptions->questiontext .= "\n\n" . $response;
+            }
+
+            if (!empty($numericaloptions->instructions)) {
+                if ($ishtml) {
+                    $numericaloptions->questiontext .= $numericaloptions->instructions;
+                } else {
+                    $numericaloptions->questiontext .= "\n\n" . $numericaloptions->instructions;
+                }
+
+                $oldfiles = $fs->get_area_files($numericaloptions->contextid,
+                        'qtype_numerical', 'instruction', $numericaloptions->questionid, 'id', false);
+                foreach ($oldfiles as $oldfile) {
+                    $filerecord = new stdClass();
+                    $filerecord->component = 'question';
+                    $filerecord->filearea = 'questiontext';
+                    $fs->create_file_from_storedfile($filerecord, $oldfile);
+                }
+
+                if ($oldfiles) {
+                    $fs->delete_area_files($numericaloptions->contextid,
+                        'qtype_numerical', 'instruction', $numericaloptions->questionid);
+                }
+            }
+
+            $updaterecord = new stdClass();
+            $updaterecord->id = $numericaloptions->questionid;
+            $updaterecord->questiontext = $numericaloptions->questiontext;
+            $updaterecord->questiontextformat = $numericaloptions->questiontextformat;
+            $DB->update_record('question', $updaterecord);
+        }
+        $rs->close();
+    }
+
+    if ($oldversion < 2011042601) {
+        // Define field instructions to be dropped from question_numerical_options
+        $table = new xmldb_table('question_numerical_options');
+        $field = new xmldb_field('instructions');
+
+        // Conditionally launch drop field instructions
+        if ($dbman->field_exists($table, $field)) {
+            $dbman->drop_field($table, $field);
+        }
+
+        // numerical savepoint reached
+        upgrade_plugin_savepoint(true, 2011042601, 'qtype', 'numerical');
+    }
+
+    if ($oldversion < 2011042602) {
+        // Define field instructionsformat to be dropped from question_numerical_options
+        $table = new xmldb_table('question_numerical_options');
+        $field = new xmldb_field('instructionsformat');
+
+        // Conditionally launch drop field instructionsformat
+        if ($dbman->field_exists($table, $field)) {
+            $dbman->drop_field($table, $field);
+        }
+
+        // numerical savepoint reached
+        upgrade_plugin_savepoint(true, 2011042602, 'qtype', 'numerical');
+    }
+
     return true;
 }
+
+
+/**
+ * Convert the ->questiontext and ->instructions fields to have the same text format.
+ * If they are already the same, do nothing. Otherwise, this method works by
+ * converting both to HTML.
+ * @param $numericaloptions the data to convert.
+ * @return bool true if the resulting fields are in HTML, as opposed to one of
+ * the text-based formats.
+ */
+function qtype_numerical_convert_text_format($numericaloptions) {
+    if ($numericaloptions->questiontextformat == $numericaloptions->instructionsformat) {
+        // Nothing to do:
+        return $numericaloptions->questiontextformat == FORMAT_HTML;
+    }
+
+    if ($numericaloptions->questiontextformat != FORMAT_HTML) {
+        $numericaloptions->questiontext = qtype_numerical_convert_to_html(
+                $numericaloptions->questiontext, $numericaloptions->questiontextformat);
+        $numericaloptions->questiontextformat = FORMAT_HTML;
+    }
+
+    if ($numericaloptions->instructionsformat != FORMAT_HTML) {
+        $numericaloptions->instructions = qtype_numerical_convert_to_html(
+                $numericaloptions->instructions, $numericaloptions->instructionsformat);
+        $numericaloptions->instructionsformat = FORMAT_HTML;
+    }
+
+    return true;
+}
+
+/**
+ * Convert some content to HTML.
+ * @param string $text the content to convert to HTML
+ * @param int $oldformat One of the FORMAT_... constants.
+ */
+function qtype_numerical_convert_to_html($text, $oldformat) {
+    switch ($oldformat) {
+        // Similar to format_text.
+
+        case FORMAT_PLAIN:
+            $text = s($text);
+            $text = str_replace('  ', '&nbsp; ', $text);
+            $text = nl2br($text);
+            return $text;
+
+        case FORMAT_MARKDOWN:
+            return markdown_to_html($text);
+
+        case FORMAT_MOODLE:
+            return text_to_html($text, null, $options['para'], $options['newlines']);
+
+        default:
+            throw new coding_exception('Unexpected text format when upgrading numerical questions.');
+    }
+}

question/type/numerical/edit_numerical_form.php

@@ -71,7 +71,6 @@ protected function add_unit_options($mform) {
 
         $unitoptions = array(
             qtype_numerical::UNITNONE     => get_string('onlynumerical', 'qtype_numerical'),
-            qtype_numerical::UNITDISPLAY  => get_string('oneunitshown', 'qtype_numerical'),
             qtype_numerical::UNITOPTIONAL => get_string('manynumerical', 'qtype_numerical'),
             qtype_numerical::UNITGRADED   => get_string('unitgraded', 'qtype_numerical'),
         );
@@ -112,19 +111,12 @@ protected function add_unit_options($mform) {
                 get_string('unitposition', 'qtype_numerical'), $unitsleftoptions);
         $mform->setDefault('unitsleft', 0);
 
-        $mform->addElement('editor', 'instructions',
-                get_string('instructions', 'qtype_numerical'), null, $this->editoroptions);
-        $mform->setType('instructions', PARAM_RAW);
-        $mform->addHelpButton('instructions', 'numericalinstructions', 'qtype_numerical');
-
         $mform->disabledIf('penaltygrp', 'unitrole', 'eq', qtype_numerical::UNITNONE);
-        $mform->disabledIf('penaltygrp', 'unitrole', 'eq', qtype_numerical::UNITDISPLAY);
         $mform->disabledIf('penaltygrp', 'unitrole', 'eq', qtype_numerical::UNITOPTIONAL);
 
         $mform->disabledIf('unitsleft', 'unitrole', 'eq', qtype_numerical::UNITNONE);
 
         $mform->disabledIf('multichoicedisplay', 'unitrole', 'eq', qtype_numerical::UNITNONE);
-        $mform->disabledIf('multichoicedisplay', 'unitrole', 'eq', qtype_numerical::UNITDISPLAY);
         $mform->disabledIf('multichoicedisplay', 'unitrole', 'eq', qtype_numerical::UNITOPTIONAL);
     }
 
@@ -234,20 +226,6 @@ protected function data_preprocessing_unit_options($question) {
             $question->unitrole = $question->options->showunits;
         }
 
-        // Instructions field.
-        $draftitemid = file_get_submitted_draft_itemid('instruction');
-        $question->instructions['text'] = file_prepare_draft_area(
-            $draftitemid,                    // draftid
-            $this->context->id,              // context
-            'qtype_' . $this->qtype(),       // component
-            'instruction',                   // filarea
-            !empty($question->id) ? (int) $question->id : null, // itemid
-            $this->fileoptions,              // options
-            $question->options->instructions // text
-        );
-        $question->instructions['itemid'] = $draftitemid ;
-        $question->instructions['format'] = $question->options->instructionsformat;
-
         return $question;
     }
 

question/type/numerical/lang/en/qtype_numerical.php

@@ -33,12 +33,13 @@
 $string['decfractionofquestiongrade'] = 'as a fraction (0-1) of the question grade';
 $string['decfractionofresponsegrade'] = 'as a fraction (0-1) of the response grade';
 $string['decimalformat'] = 'decimals';
-$string['editableunittext'] = 'a text input element';
+$string['editableunittext'] = 'the text input element';
 $string['editingnumerical'] = 'Editing a Numerical question';
 $string['errornomultiplier'] = 'You must specify a multiplier for this unit.';
 $string['errorrepeatedunit'] = 'You cannot have two units with the same name.';
 $string['geometric'] = 'Geometric';
-$string['instructions'] = 'Instructions ';
+$string['invalidnumber'] = 'You must enter a valid number.';
+$string['invalidnumbernounit'] = 'You must enter a valid number. Do not include a unit in your response.';
 $string['invalidnumericanswer'] = 'One of the answers you entered was not a valid number.';
 $string['invalidnumerictolerance'] = 'One of the tolerances you entered was not a valid number.';
 $string['leftexample'] = 'on the left, for example $1.00 or £1.00';
@@ -51,8 +52,6 @@
 $string['numerical_help'] = 'From the student perspective, a numerical question looks just like a short-answer question. The difference is that numerical answers are allowed to have an accepted error. This allows a fixed range of answers to be evaluated as one answer. For example, if the answer is 10 with an accepted error of 2, then any number between 8 and 12 will be accepted as correct. ';
 $string['numerical_link'] = 'question/type/numerical';
 $string['numericalsummary'] = 'Allows a numerical response, possibly with units, that is graded by comparing against various model answers, possibly with tolerances.';
-$string['numericalinstructions'] = 'Instructions';
-$string['numericalinstructions_help'] = 'Enter any instructions you would like to give the student about how to complete their response.';
 $string['numericalmultiplier'] = 'Multiplier';
 $string['numericalmultiplier_help'] = 'The multiplier is the factor by which the correct numerical response will be multiplied.
 
@@ -100,7 +99,7 @@
 * the wrong unit name is entered into the unit input, or
 * a unit is entered into the value input box';
 $string['unitappliedpenalty'] = 'These marks include a penalty of {$a} for bad unit.';
-$string['unitposition'] = 'Units are displayed';
+$string['unitposition'] = 'Units go';
 $string['unitnotselected'] = 'No unit selected';
 $string['unithandling'] = 'Unit handling';
 $string['validnumberformats'] = 'Valid number formats';