diff --git a/.upgradenotes/MDL-82687-2024080211282638.yml b/.upgradenotes/MDL-82687-2024080211282638.yml new file mode 100644 index 00000000000..19dc08a008b --- /dev/null +++ b/.upgradenotes/MDL-82687-2024080211282638.yml @@ -0,0 +1,10 @@ +issueNumber: MDL-82687 +notes: + core_form: + - message: + Previously, the 'duration' form field type would allow users to input positive or negative durations. + However looking at all the uses, everyone was expecting this input type to only accept times >= 0 seconds, + and almost no-one was bothering to write manual form validation, leading to subtle bugs. + So now, by default this field type will validate the input value is not negative. If you need the + previous behaviour, there is a new option 'allownegative' which you can set to true. (The default is false.) + type: improved diff --git a/lang/en/form.php b/lang/en/form.php index d4304482b5c..123ff4d5076 100644 --- a/lang/en/form.php +++ b/lang/en/form.php @@ -39,6 +39,7 @@ $string['err_minlength'] = 'You must enter at least {$a->format} characters here $string['err_nonzero'] = 'You must enter a number not starting with a 0 here.'; $string['err_nopunctuation'] = 'You must enter no punctuation characters here.'; $string['err_numeric'] = 'You must enter a number here.'; +$string['err_positiveduration'] = 'This duration cannot be negative.'; $string['err_positiveint'] = 'You must enter a whole number that is greater than 0.'; $string['err_rangelength'] = 'You must enter between {$a->format[0]} and {$a->format[1]} characters here.'; $string['err_required'] = 'You must supply a value here.'; diff --git a/lib/form/duration.php b/lib/form/duration.php index 0ff1aaf2ee2..e6a7e8319a2 100644 --- a/lib/form/duration.php +++ b/lib/form/duration.php @@ -36,6 +36,9 @@ require_once($CFG->libdir . '/form/text.php'); * HTML class for a length of time. For example, 30 minutes of 4 days. The * values returned to PHP is the duration in seconds (an int rounded to the nearest second). * + * By default, only durations >= 0 can be input. If you want to allow negative + * durations set the option allownegative. + * * @package core_form * @category form * @copyright 2009 Tim Hunt @@ -46,9 +49,10 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * Control the field names for form elements * optional => if true, show a checkbox beside the element to turn it on (or off) * defaultunit => which unit is default when the form is blank (default Minutes). + * allownegative => are durations < 0 allowed? (default false) * @var array */ - protected $_options = ['optional' => false, 'defaultunit' => MINSECS]; + protected $_options = ['optional' => false, 'defaultunit' => MINSECS, 'allownegative' => false]; /** @var array associative array of time units (days, hours, minutes, seconds) */ private $_units = null; @@ -64,6 +68,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { * the time is blank. If not specified, minutes is used. * 'units' => array containing some or all of 1, MINSECS, HOURSECS, DAYSECS and WEEKSECS * which unit choices to offer. + * 'allownegative' => true/false - are durations < 0 allowed? (default false) * @param mixed $attributes Either a typical HTML attribute string or an associative array */ public function __construct($elementName = null, $elementLabel = null, @@ -78,6 +83,7 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { $options = []; } $this->_options['optional'] = !empty($options['optional']); + $this->_options['allownegative'] = !empty($options['allownegative']); if (isset($options['defaultunit'])) { if (!array_key_exists($options['defaultunit'], $this->get_units())) { throw new coding_exception($options['defaultunit'] . @@ -248,6 +254,14 @@ class MoodleQuickForm_duration extends MoodleQuickForm_group { } } + #[\Override] + public function validateSubmitValue($values) { + if ($this->exportValue($values) < 0 && !$this->_options['allownegative']) { + return get_string('err_positiveduration', 'core_form'); + } + return null; + } + /** * Returns HTML for advchecbox form element. * diff --git a/lib/form/tests/duration_test.php b/lib/form/tests/duration_test.php index 5158f8f6391..2dd27de07ad 100644 --- a/lib/form/tests/duration_test.php +++ b/lib/form/tests/duration_test.php @@ -14,17 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -/** - * Unit tests for MoodleQuickForm_duration - * - * Contains test cases for testing MoodleQuickForm_duration - * - * @package core_form - * @category test - * @copyright 2009 Tim Hunt - * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later - */ - namespace core_form; use moodleform; @@ -44,6 +33,7 @@ require_once($CFG->libdir . '/form/duration.php'); * @category test * @copyright 2009 Tim Hunt * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \MoodleQuickForm_duration */ class duration_test extends \basic_testcase { @@ -54,7 +44,7 @@ class duration_test extends \basic_testcase { */ protected function get_test_form(): MoodleQuickForm { $form = new temp_form_duration(); - return $form->getform(); + return $form->get_form(); } /** @@ -192,6 +182,47 @@ class duration_test extends \basic_testcase { $this->assertEquals(['testel' => $expected], $el->exportValue($values, true)); $this->assertEquals($expected, $el->exportValue($values)); } + + /** + * Test cases for {@see test_validate_submit_value_negative_blocked()}. + * @return array[] test cases. + */ + public static function validate_submit_value_cases(): array { + return [ + [false, -10, MINSECS, false], + [false, 10, MINSECS, true], + [false, 0, MINSECS, true], + [true, -10, MINSECS, true], + [true, 10, MINSECS, true], + [true, 0, MINSECS, true], + ]; + } + + /** + * Test for {@see MoodleQuickForm_duration::validateSubmitValue()}. + * + * @dataProvider validate_submit_value_cases + * @param bool $allownegative whether the element should be created to allow negative values. + * @param int $number the number submitted. + * @param int $unit the unit submitted. + * @param bool $isvalid whether this submission is valid. + */ + public function test_validate_submit_value(bool $allownegative, int $number, int $unit, bool $isvalid): void { + $form = new temp_form_duration(null, null, 'post', '', null, true); + /** @var \MoodleQuickForm_duration $element */ + $element = $form->get_form()->addElement('duration', 'testel', '', ['allownegative' => $allownegative]); + + $values = ['testel' => ['number' => $number, 'timeunit' => $unit]]; + + if ($isvalid) { + $this->assertNull($element->validateSubmitValue($values)); + } else { + $this->assertEquals( + get_string('err_positiveduration', 'core_form'), + $element->validateSubmitValue($values), + ); + } + } } /** @@ -209,7 +240,7 @@ class temp_form_duration extends moodleform { * Returns form reference * @return MoodleQuickForm */ - public function getform() { + public function get_form() { $mform = $this->_form; // Set submitted flag, to simulate submission. $mform->_flagSubmitted = true; diff --git a/mod/quiz/tests/behat/settings_form_validation.feature b/mod/quiz/tests/behat/settings_form_validation.feature new file mode 100644 index 00000000000..974f99aee8c --- /dev/null +++ b/mod/quiz/tests/behat/settings_form_validation.feature @@ -0,0 +1,28 @@ +@mod @mod_quiz +Feature: Settings form fields are validated + To help me avoid mistakes + As a teacher + I need the quiz settings to be validated + + Background: + Given the following "users" exist: + | username | firstname | + | teacher | Teach | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher | C1 | editingteacher | + And the following "activities" exist: + | activity | course | section | name | + | quiz | C1 | 1 | Test quiz 1 | + + Scenario: Negative time limits are not allowed + When I am on the "Test quiz 1" "quiz activity editing" page logged in as teacher + And I expand all fieldsets + And I set the following fields to these values: + | id_timelimit_enabled | 1 | + | id_timelimit_number | -10 | + And I press "Save and display" + Then I should see "This duration cannot be negative"