L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-62947 mod_feedback: fix feedback so it correctly uses forms API

Browse Source 
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.
This commit is contained in:
Jake Dallimore
2018-09-05 12:12:20 +08:00
committed by Jun Pataleta
parent c87010baca
commit c96c9601af
3 changed files with 40 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mod/feedback/classes/complete_form.php
+14 -7
View 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();
mod/feedback/item/multichoice/lib.php
+18 -10
View 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);
}
mod/feedback/tests/external_test.php
+8 -4
View 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) {