Fix maths output rendering.

There was a bug with how PRT feedback was displayed. We were calling
Moodle's format_text function before processing it as CAS text. That
does not work in situations where equations are being replaced by
images. (It does not matter when using MathJax.)

In order to fix this, there is now a restriction that all PRT node
feedback must have the same FORMAT_... type. In practice, this is not a
serious restriction, and the editing form now validates this.

edit_stack_form.php

@@ -866,11 +866,20 @@ class qtype_stack_edit_form extends question_edit_form {
 
         // Check the nodes.
         $nodes = $this->get_required_nodes_for_prt($prtname);
+        $textformat = null;
         foreach ($nodes as $nodekey) {
 
             // Check the fields the belong to this node individually.
             $errors = $this->validate_prt_node($errors, $fromform, $prtname, $nodekey);
 
+            if (is_null($textformat)) {
+                $textformat = $fromform[$prtname . 'truefeedback'][$nodekey]['format'];
+            }
+            if ($textformat != $fromform[$prtname . 'truefeedback'][$nodekey]['format']) {
+                $errors[$prtname . 'truefeedback[' . $nodekey . ']'][] =
+                        stack_string('allnodefeedbackmustusethesameformat');
+            }
+
             // Also build the $nextnodes graph structure array that we will need in a second.
             $nextnodes[$nodekey] = array();
 

lang/en/qtype_stack.php

@@ -36,6 +36,7 @@ $string['exceptionmessage'] = '{$a}';
 
 // Strings used on the editing form.
 $string['addanothernode'] = 'Add another node';
+$string['allnodefeedbackmustusethesameformat'] = 'All the feedback for all the nodes in a PRT must use the same text format.';
 $string['answernote'] = 'Answer note';
 $string['answernote_err'] = 'Answer notes may not contain the character |.  This character is inserted by STACK and is later used to split answer notes automatically.';
 $string['answernote_help'] = 'This is a tag which is key for reporting purposes.  It is designed to record the unique path through the tree, and the outcome of each answer test.  This is automatically generated, but can be changed to something meaningful.';

renderer.php

@@ -41,7 +41,10 @@ class qtype_stack_renderer extends qtype_renderer {
         $response = $qa->get_last_qt_data();
 
         $questiontext = $qa->get_last_qt_var('_questiontext');
-        $questiontext = stack_maths::process_display_castext($questiontext);
+        $questiontext = $question->format_text(
+                stack_maths::process_display_castext($questiontext),
+                $question->questiontextformat,
+                $qa, 'question', 'questiontext', $question->id);
 
         // Replace inputs.
         foreach ($question->inputs as $name => $input) {
@@ -78,13 +81,11 @@ class qtype_stack_renderer extends qtype_renderer {
                 $feedback = html_writer::nonempty_tag('div', $result->errors,
                         array('class' => 'stackprtfeedback stackprtfeedback-' . $name));
             }
-            $questiontext = str_replace("[[feedback:{$index}]]", stack_maths::process_display_castext($feedback), $questiontext);
+            $questiontext = str_replace("[[feedback:{$index}]]", $feedback, $questiontext);
         }
 
         $result = '';
-        $result .= $this->question_tests_link($question, $options);
-        $result .= $question->format_text($questiontext, $question->questiontextformat,
-                $qa, 'question', 'questiontext', $question->id);
+        $result .= $this->question_tests_link($question, $options) . $questiontext;
 
         if ($qa->get_state() == question_state::$invalid) {
             $result .= html_writer::nonempty_tag('div',
@@ -165,6 +166,10 @@ class qtype_stack_renderer extends qtype_renderer {
             return '';
         }
 
+        $feedbacktext = stack_maths::process_display_castext($feedbacktext);
+        $feedbacktext = $question->format_text($feedbacktext, $question->specificfeedbackformat,
+                $qa, 'qtype_stack', 'specificfeedback', $question->id);
+
         // Replace any PRT feedback.
         $allempty = true;
         foreach ($question->prts as $name => $prt) {
@@ -182,8 +187,7 @@ class qtype_stack_renderer extends qtype_renderer {
             return '';
         }
 
-        return $question->format_text($feedbacktext, $question->specificfeedbackformat,
-                $qa, 'qtype_stack', 'specificfeedback', $question->id);
+        return $feedbacktext;
     }
 
     /**
@@ -202,6 +206,10 @@ class qtype_stack_renderer extends qtype_renderer {
             return '';
         }
 
+        $feedbacktext = stack_maths::process_display_castext($feedbacktext);
+        $feedbacktext = $question->format_text($feedbacktext, $question->specificfeedbackformat,
+                $qa, 'qtype_stack', 'specificfeedback', $question->id);
+
         // Replace any PRT feedback.
         $allempty = true;
         foreach ($question->prts as $index => $prt) {
@@ -215,8 +223,7 @@ class qtype_stack_renderer extends qtype_renderer {
             return '';
         }
 
-        return $question->format_text($feedbacktext, $question->specificfeedbackformat,
-                $qa, 'qtype_stack', 'specificfeedback', $question->id);
+        return $feedbacktext;
     }
 
     /**
@@ -284,6 +291,24 @@ class qtype_stack_renderer extends qtype_renderer {
             $err = $result->errors;
         }
 
+        $feedback = '';
+        $feedbackbits = $result->get_feedback();
+        if ($feedbackbits) {
+            $feedback = array();
+            $format = reset($feedbackbits)->format;
+            foreach ($feedbackbits as $bit) {
+                $feedback[] = $qa->rewrite_pluginfile_urls(
+                        $bit->feedback, 'qtype_stack', $bit->filearea, $bit->itemid);
+                if ($bit->format != $format) {
+                    throw new coding_exception('Inconsistent feedback formats found in PRT ' . $name);
+                }
+            }
+
+            $feedback = $result->substitue_variables_in_feedback(implode(' ', $feedback));
+            $feedback = format_text(stack_maths::process_display_castext($feedback),
+                    $format, array('noclean' => true, 'para' => false));
+        }
+
         $gradingdetails = '';
         if (!$result->errors && $qa->get_behaviour_name() == 'adaptivemultipart') {
             // This is rather a hack, but it will probably work.
@@ -292,8 +317,6 @@ class qtype_stack_renderer extends qtype_renderer {
                     $qa->get_behaviour()->get_part_mark_details($name), $options);
         }
 
-        $feedback = $result->get_feedback($qa);
-
         return html_writer::nonempty_tag('div',
                 $this->standard_prt_feedback($qa, $question, $result) .
                 $err . $feedback . $gradingdetails,

stack/potentialresponsetreestate.class.php

@@ -28,25 +28,10 @@ class stack_potentialresponse_tree_state {
     public $_errors      = '';
 
     /**
-     * @var array of feedback strings for the student.
+     * @var array of stack_prt_feedback_element.
      */
     public $_feedback    = array();
 
-    /**
-     * @var array of FORMAT_... constants corresponding to the feedback strings.
-     */
-    public $_feedbackformat   = array();
-
-    /**
-     * @var array of file area names corresponding to the feedback strings.
-     */
-    public $_feedbackfilearea = array();
-
-    /**
-     * @var array of node ids corresponding to the feedback strings.
-     */
-    public $_feedbacknodeid   = array();
-
     /**
      * @var array of answernote strings for the teacher.
      */
@@ -162,33 +147,51 @@ class stack_potentialresponse_tree_state {
      * @param string $feedback the next bit of feedback.
      */
     public function add_feedback($feedback, $format = null, $filearea = null, $nodeid = null) {
-        $this->_feedback[] = $feedback;
-        $this->_feedbackformat[] = $format;
-        $this->_feedbackfilearea[] = $filearea;
-        $this->_feedbacknodeid[] = $nodeid;
+        $this->_feedback[] = new stack_prt_feedback_element($feedback, $format, $filearea, $nodeid);
     }
 
     /**
-     * Prepare the feedback for output.
-     * @return string HTML to output.
+     * Get the bits of feedback.
+     * @return array of stack_prt_feedback_element.
      */
-    public function get_feedback(question_attempt $qa) {
-
-        $formattedfeedback = array();
-        foreach ($this->_feedback as $key => $feedback) {
-            if (is_null($this->_feedbackformat[$key])) {
-                $formattedfeedback[] = $feedback;
-            } else {
-                $formattedfeedback[] = $qa->get_question()->format_text($feedback,
-                        $this->_feedbackformat[$key], $qa, 'qtype_stack',
-                        $this->_feedbackfilearea[$key], $this->_feedbacknodeid[$key]);
-            }
+    public function get_feedback() {
+        return $this->_feedback;
     }
 
-        $feedbackct = new stack_cas_text(implode(' ', $formattedfeedback), $this->cascontext, $this->seed, 't', false, false);
+    /**
+     * Subsitute variables into the feedback text.
+     * @param string $feedback the concatenated feedback text.
+     * @return string the feedback with question variables substituted.
+     */
+    public function substitue_variables_in_feedback($feedback) {
+        $feedbackct = new stack_cas_text($feedback, $this->cascontext, $this->seed, 't', false, false);
         $result = $feedbackct->get_display_castext();
         $this->_errors = trim($this->_errors . ' ' . $feedbackct->get_errors());
-
         return $result;
     }
 }
+
+
+/**
+ * Small class to encapsulate all the data for the feedback from one PRT node.
+ */
+class stack_prt_feedback_element {
+    /** @var string the feedback text. */
+    public $feedback;
+
+    /** @var int the feedback format. One of the FORMAT_... constants. */
+    public $format;
+
+    /** @var string feedback file area name. */
+    public $filearea;
+
+    /** @var int node id (used as the file area item id. */
+    public $itemid;
+
+    public function __construct($feedback, $format, $filearea, $itemid) {
+        $this->feedback = $feedback;
+        $this->format   = $format;
+        $this->filearea = $filearea;
+        $this->itemid   = $itemid;
+    }
+}

stack/tests/potentialresponsenode_test.php

@@ -64,7 +64,8 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
 
         $this->assertEquals(true, $result->valid);
         $this->assertEquals('', $result->errors);
-        $this->assertEquals(array('Yeah!'), $result->feedback);
+        $this->assertEquals(1, count($result->feedback));
+        $this->assertEquals('Yeah!', $result->feedback[0]->feedback);
         $this->assertEquals(3, $nextnode);
     }
 
@@ -82,7 +83,8 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
 
         $this->assertEquals(true, $result->valid);
         $this->assertEquals('', $result->errors);
-        $this->assertEquals(array('Boo!'), $result->feedback);
+        $this->assertEquals(1, count($result->feedback));
+        $this->assertEquals('Boo!', $result->feedback[0]->feedback);
         $this->assertEquals(-1, $nextnode);
     }
 
@@ -100,10 +102,12 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
 
         $this->assertEquals(false, $result->valid);
         $this->assertNotEquals('', $result->errors);
+        $this->assertEquals(2, count($result->feedback));
         // TODO this next line looks wrong. Presumably a regressions from Chris's recent changes.
-        $this->assertEquals(array('The answer test failed to execute correctly: ' .
-                'please alert your teacher. Division by 0part: invalid index of list or matrix.', 'Boo!'),
-                $result->feedback);
+        $this->assertEquals('The answer test failed to execute correctly: ' .
+                'please alert your teacher. Division by 0part: invalid index of list or matrix.',
+                $result->feedback[0]->feedback);
+        $this->assertEquals('Boo!', $result->feedback[1]->feedback);
         $this->assertEquals(-1, $nextnode);
     }
 
@@ -121,7 +125,8 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
 
         $this->assertEquals(true, $result->valid);
         $this->assertEquals('', $result->errors);
-        $this->assertEquals(array('Yeah!'), $result->feedback);
+        $this->assertEquals(1, count($result->feedback));
+        $this->assertEquals('Yeah!', $result->feedback[0]->feedback);
         $this->assertEquals(3, $nextnode);
     }
 
@@ -139,8 +144,10 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
 
         $this->assertEquals(true, $result->valid);
         $this->assertEquals('', $result->errors);
-        $this->assertEquals(array('Your answer is not factored. You need to take out a common factor.', 'Boo!'),
-                $result->feedback);
+        $this->assertEquals(2, count($result->feedback));
+        $this->assertEquals('Your answer is not factored. You need to take out a common factor.',
+                $result->feedback[0]->feedback);
+        $this->assertEquals('Boo!', $result->feedback[1]->feedback);
 
     }
 
@@ -157,8 +164,9 @@ class stack_potentialresponse_node_test extends qtype_stack_testcase {
         $result = new stack_potentialresponse_tree_state(1, true, 1);
         $nextnode = $node->do_test('3*x+6', '3*(x+2)', 'x', $options, $result);
 
-        $this->assertEquals(array('Boo! Your answer should be in factored form, i.e. @factor(ans1)@.'),
-                $result->feedback);
+        $this->assertEquals(1, count($result->feedback));
+        $this->assertEquals('Boo! Your answer should be in factored form, i.e. @factor(ans1)@.',
+                $result->feedback[0]->feedback);
 
         $this->assertEquals(1.5, $result->score);
 

stack/tests/potentialresponsetree_test.php

@@ -26,6 +26,7 @@ require_once(dirname(__FILE__) . '/../potentialresponsetree.class.php');
 require_once(dirname(__FILE__) . '/../cas/castext.class.php');
 require_once(dirname(__FILE__) . '/../cas/keyval.class.php');
 require_once(dirname(__FILE__) . '/../../locallib.php');
+require_once(dirname(__FILE__) . '/../../tests/test_base.php');
 
 
 /**
@@ -61,7 +62,8 @@ class stack_potentialresponsetree_test extends qtype_stack_testcase {
         $this->assertEquals('', $result->errors);
         $this->assertEquals(1, $result->score);
         $this->assertEquals(0, $result->penalty);
-        $this->assertEquals(array('Yeah!'), $result->feedback);
+        $this->assertEquals(1, count($result->feedback));
+        $this->assertEquals('Yeah!', $result->feedback[0]->feedback);
         $this->assertEquals(array('ATInt_true', '1-0-1'), $result->answernotes);
         $this->assertEquals(array('NULL' => 'NULL', '1-0-1' => '1-0-1', '1-0-0' => '1-0-0'), $tree->get_all_answer_notes());
     }
@@ -100,7 +102,9 @@ class stack_potentialresponsetree_test extends qtype_stack_testcase {
         $this->assertEquals('', $result->errors);
         $this->assertEquals(1, $result->score);
         $this->assertEquals(0, $result->penalty);
-        $this->assertEquals(array('Ok, you can diff.', 'Do not expand!'), $result->feedback);
+        $this->assertEquals(2, count($result->feedback));
+        $this->assertEquals('Ok, you can diff.', $result->feedback[0]->feedback);
+        $this->assertEquals('Do not expand!', $result->feedback[1]->feedback);
         $this->assertEquals(array('ATDiff_true', '1-0-1', 'ATFacForm_notfactored.', '1-1-0'), $result->answernotes);
 
         // Now have another attempt at the same PRT!
@@ -112,7 +116,10 @@ class stack_potentialresponsetree_test extends qtype_stack_testcase {
         $this->assertEquals('', $result->errors);
         $this->assertEquals(1, $result->score);
         $this->assertEquals(0, $result->penalty);
-        $this->assertEquals(array('Ok, you can diff.', 'Yeah!'), $result->feedback);
+        $this->assertEquals(2, count($result->feedback));
+        $this->assertEquals('Ok, you can diff.', $result->feedback[0]->feedback);
+        $this->assertEquals('Yeah!', $result->feedback[1]->feedback);
+
         $this->assertEquals(array('ATDiff_true', '1-0-1', 'ATFacForm_true', '1-1-1'), $result->answernotes);
         $this->assertEquals(array('NULL' => 'NULL', '1-0-1' => '1-0-1', '1-0-0' => '1-0-0',
                 '1-1-1' => '1-1-1', '1-1-0' => '1-1-0'), $tree->get_all_answer_notes());
@@ -166,7 +173,9 @@ class stack_potentialresponsetree_test extends qtype_stack_testcase {
         $this->assertEquals('', $result->errors);
         $this->assertEquals(0.3, $result->score);
         $this->assertEquals(0, $result->penalty);
-        $this->assertEquals(array('Test 1 true.', 'Test 2 false.'), $result->feedback);
+        $this->assertEquals(2, count($result->feedback));
+        $this->assertEquals('Test 1 true.', $result->feedback[0]->feedback);
+        $this->assertEquals('Test 2 false.', $result->feedback[1]->feedback);
         $this->assertEquals(array('1-0-1', 'ATFacForm_notfactored.', '1-1-0', '[PRT-CIRCULARITY]=0'), $result->answernotes);
 
         $this->assertEquals(array('sa1', 'ta'), $tree->get_required_variables(array('sa1', 'sa3', 'ta', 'ssa1', 'a1', 't')));