MDL-62947 mod_feedback: fix feedback so it correctly uses forms API
authorJake Dallimore <jake@moodle.com>
Thu, 26 Jul 2018 01:56:00 +0000 (09:56 +0800)
committerJun Pataleta <jun@moodle.com>
Wed, 5 Sep 2018 04:12:20 +0000 (12:12 +0800)
Feedback code was doing the following, which is incompatible with the
sec-patched quickforms lib:
- appending '[0]' to non-group element names, meaning type checks in
getCleanType() were falling back to PARAM_RAW instead of PARAM_INT,
and meaning _findValue() was comparing elements it thought to be arrays
with scalar submit values (worked with eval() implementation as the 0th
element of a string was the character, but not working when patched).
- external unit tests incorrectly testing multichoice questions in the
process_page tests - patched lib highlighted failures.

It was also doing the following:
- actively setting null as default element values instead of 0 for
radio buttons and dropdowns (for 'not selected'). This is incompatible
with more recent quickforms lib (see MDL-63070).
- creating groups using addElement instead of addGroup - not recommended

This patch addresses the above.

mod/feedback/classes/complete_form.php
mod/feedback/item/multichoice/lib.php
mod/feedback/tests/external_test.php

index 44480a2..7bf40ab 100644 (file)
@@ -293,15 +293,22 @@ class mod_feedback_complete_form extends moodleform {
      */
     public function add_form_element($item, $element, $addrequiredrule = true, $setdefaultvalue = true) {
         global $OUTPUT;
-        // Add element to the form.
-        if (is_array($element)) {
-            if ($this->is_frozen() && $element[0] === 'text') {
-                // Convert 'text' element to 'static' when freezing for better display.
-                $element = ['static', $element[1], $element[2]];
+
+        if (is_array($element) && $element[0] == 'group') {
+            // For groups, use the mforms addGroup API.
+            // $element looks like: ['group', $groupinputname, $name, $objects, $separator, $appendname],
+            $element = $this->_form->addGroup($element[3], $element[1], $element[2], $element[4], $element[5]);
+        } else {
+            // Add non-group element to the form.
+            if (is_array($element)) {
+                if ($this->is_frozen() && $element[0] === 'text') {
+                    // Convert 'text' element to 'static' when freezing for better display.
+                    $element = ['static', $element[1], $element[2]];
+                }
+                $element = call_user_func_array(array($this->_form, 'createElement'), $element);
             }
-            $element = call_user_func_array(array($this->_form, 'createElement'), $element);
+            $element = $this->_form->addElement($element);
         }
-        $element = $this->_form->addElement($element);
 
         // Prepend standard CSS classes to the element classes.
         $attributes = $element->getAttributes();
index 9dea3fe..0c0286b 100644 (file)
@@ -316,14 +316,19 @@ class feedback_item_multichoice extends feedback_item_base {
         $inputname = $item->typ . '_' . $item->id;
         $options = $this->get_options($item);
         $separator = !empty($info->horizontal) ? ' ' : '<br>';
-        $tmpvalue = $form->get_item_value($item);
+        $tmpvalue = $form->get_item_value($item) ?? 0; // Used for element defaults, so must be a valid value (not null).
 
+        // Subtypes:
+        // r = radio
+        // c = checkbox
+        // d = dropdown.
         if ($info->subtype === 'd' || ($info->subtype === 'r' && $form->is_frozen())) {
             // Display as a dropdown in the complete form or a single value in the response view.
             $element = $form->add_form_element($item,
-                    ['select', $inputname.'[0]', $name, array(0 => '') + $options, array('class' => $class)],
+                    ['select', $inputname, $name, array(0 => '') + $options, array('class' => $class)],
                     false, false);
-            $form->set_element_default($inputname.'[0]', $tmpvalue);
+            $form->set_element_default($inputname, $tmpvalue);
+            $form->set_element_type($inputname, PARAM_INT);
         } else if ($info->subtype === 'c' && $form->is_frozen()) {
             // Display list of checkbox values in the response view.
             $objs = [];
@@ -354,26 +359,26 @@ class feedback_item_multichoice extends feedback_item_base {
             } else {
                 // Radio.
                 if (!array_key_exists(0, $options)) {
-                    // Always add '0' as hidden element, otherwise form submit data may not have this element.
-                    $objs[] = ['hidden', $inputname.'[0]'];
+                    // Always add a hidden element to the group to guarantee we get a value in the submit data.
+                    $objs[] = ['hidden', $inputname, 0];
                 }
                 foreach ($options as $idx => $label) {
-                    $objs[] = ['radio', $inputname.'[0]', '', $label, $idx];
+                    $objs[] = ['radio', $inputname, '', $label, $idx];
                 }
                 // Span to hold the element id. The id is used for drag and drop reordering.
                 $objs[] = ['static', '', '', html_writer::span('', '', ['id' => 'feedback_item_' . $item->id])];
                 $element = $form->add_form_group_element($item, 'group_'.$inputname, $name, $objs, $separator, $class);
-                $form->set_element_default($inputname.'[0]', $tmpvalue);
-                $form->set_element_type($inputname.'[0]', PARAM_INT);
+                $form->set_element_default($inputname, $tmpvalue);
+                $form->set_element_type($inputname, PARAM_INT);
             }
         }
 
         // Process 'required' rule.
         if ($item->required) {
             $elementname = $element->getName();
-            $form->add_validation_rule(function($values, $files) use ($elementname, $item) {
+            $form->add_validation_rule(function($values) use ($elementname, $item) {
                 $inputname = $item->typ . '_' . $item->id;
-                return empty($values[$inputname]) || !array_filter($values[$inputname]) ?
+                return empty($values[$inputname]) || (is_array($values[$inputname]) && !array_filter($values[$inputname])) ?
                     array($elementname => get_string('required')) : true;
             });
         }
@@ -385,6 +390,9 @@ class feedback_item_multichoice extends feedback_item_base {
      * @return string
      */
     public function create_value($value) {
+        // Could be an array (multichoice checkbox) or single value (multichoice radio or dropdown).
+        $value = is_array($value) ? $value : [$value];
+
         $value = array_unique(array_filter($value));
         return join(FEEDBACK_MULTICHOICE_LINE_SEP, $value);
     }
index 3ab5d19..9aff6c5 100644 (file)
@@ -518,7 +518,7 @@ class mod_feedback_external_testcase extends externallib_advanced_testcase {
         $this->assertCount(7, $tmpitems);   // 2 from the first page + 5 from the second page.
 
         // And finally, save everything! We are going to modify one previous recorded value.
-        $data[2]['value'] = 'b';
+        $data[2]['value'] = 2; // 2 is value of the option 'b'.
         $secondpagedata = [$data[2], $data[3], $data[4], $data[5], $data[6]];
         $result = mod_feedback_external::process_page($this->feedback->id, 1, $secondpagedata);
         $result = external_api::clean_returnvalue(mod_feedback_external::process_page_returns(), $result);
@@ -530,7 +530,9 @@ class mod_feedback_external_testcase extends externallib_advanced_testcase {
         // Check if the one we modified was correctly saved.
         $itemid = $itemscreated[4]->id;
         $itemsaved = $DB->get_field('feedback_value', 'value', array('item' => $itemid));
-        $this->assertEquals('b', $itemsaved);
+        $mcitem = new feedback_item_multichoice();
+        $itemval = $mcitem->get_printval($itemscreated[4], (object) ['value' => $itemsaved]);
+        $this->assertEquals('b', $itemval);
 
         // Check that the answers are saved for course 0.
         foreach ($items as $item) {
@@ -593,7 +595,7 @@ class mod_feedback_external_testcase extends externallib_advanced_testcase {
         $this->assertFalse($result['completed']);
 
         // Process second page.
-        $data[2]['value'] = 'b';
+        $data[2]['value'] = 2; // 2 is value of the option 'b';
         $secondpagedata = [$data[2], $data[3], $data[4], $data[5], $data[6]];
         $result = mod_feedback_external::process_page($this->feedback->id, 1, $secondpagedata, false, $this->course->id);
         $result = external_api::clean_returnvalue(mod_feedback_external::process_page_returns(), $result);
@@ -605,7 +607,9 @@ class mod_feedback_external_testcase extends externallib_advanced_testcase {
         // Check if the one we modified was correctly saved.
         $itemid = $itemscreated[4]->id;
         $itemsaved = $DB->get_field('feedback_value', 'value', array('item' => $itemid));
-        $this->assertEquals('b', $itemsaved);
+        $mcitem = new feedback_item_multichoice();
+        $itemval = $mcitem->get_printval($itemscreated[4], (object) ['value' => $itemsaved]);
+        $this->assertEquals('b', $itemval);
 
         // Check that the answers are saved for the correct course.
         foreach ($items as $item) {