From a039751aee646c9125d4a31d88f339b27cbf2622 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 20 Jun 2018 13:40:33 +0800 Subject: [PATCH 1/3] MDL-56881 mod_choice: fix bug with limits when saving existing choice The check determining whether a choice option's limit was exceeded was including the user's existing answers in its checks, meaning a user couldn't save an existing choice answer, or select further options, if all a choice option limit was reached. This patch fixes that. --- mod/choice/lib.php | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/mod/choice/lib.php b/mod/choice/lib.php index bf548a04ae6..3953d43b42b 100644 --- a/mod/choice/lib.php +++ b/mod/choice/lib.php @@ -312,7 +312,7 @@ function choice_modify_responses($userids, $answerids, $newoptionid, $choice, $c * Process user submitted answers for a choice, * and either updating them or saving new answers. * - * @param int $formanswer users submitted answers. + * @param int|array $formanswer the id(s) of the user submitted choice options. * @param object $choice the selected choice. * @param int $userid user identifier. * @param object $course current course. @@ -361,6 +361,12 @@ function choice_user_submit_response($formanswer, $choice, $userid, $course, $cm } $current = $DB->get_records('choice_answers', array('choiceid' => $choice->id, 'userid' => $userid)); + + // Array containing [answerid => optionid] mapping. + $existinganswers = array_map(function($answer) { + return $answer->optionid; + }, $current); + $context = context_module::instance($cm->id); $choicesexceeded = false; @@ -404,7 +410,13 @@ function choice_user_submit_response($formanswer, $choice, $userid, $course, $cm } } } + foreach ($countanswers as $opt => $count) { + // Ignore the user's existing answers when checking whether an answer count has been exceeded. + // A user may wish to update their response with an additional choice option and shouldn't be competing with themself! + if (in_array($opt, $existinganswers)) { + continue; + } if ($count >= $choice->maxanswers[$opt]) { $choicesexceeded = true; break; @@ -418,10 +430,8 @@ function choice_user_submit_response($formanswer, $choice, $userid, $course, $cm if (!($choice->limitanswers && $choicesexceeded)) { if ($current) { // Update an existing answer. - $existingchoices = array(); foreach ($current as $c) { if (in_array($c->optionid, $formanswers)) { - $existingchoices[] = $c->optionid; $DB->set_field('choice_answers', 'timemodified', time(), array('id' => $c->id)); } else { $deletedanswersnapshots[] = $c; @@ -431,7 +441,7 @@ function choice_user_submit_response($formanswer, $choice, $userid, $course, $cm // Add new ones. foreach ($formanswers as $f) { - if (!in_array($f, $existingchoices)) { + if (!in_array($f, $existinganswers)) { $newanswer = new stdClass(); $newanswer->optionid = $f; $newanswer->choiceid = $choice->id; @@ -460,14 +470,9 @@ function choice_user_submit_response($formanswer, $choice, $userid, $course, $cm } } } else { - // Check to see if current choice already selected - if not display error. - $currentids = array_keys($current); - - if (array_diff($currentids, $formanswers) || array_diff($formanswers, $currentids) ) { - // Release lock before error. - $choicelock->release(); - print_error('choicefull', 'choice', $continueurl); - } + // This is a choice with limited options, and one of the options selected has just run over its limit. + $choicelock->release(); + print_error('choicefull', 'choice', $continueurl); } // Release lock. From 03577962efbbf5444f709a7a63ab5a7041ce5aca Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 20 Jun 2018 15:37:10 +0800 Subject: [PATCH 2/3] MDL-56881 mod_choice: clarify lang string for limit-reached scenarios --- mod/choice/lang/en/choice.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/choice/lang/en/choice.php b/mod/choice/lang/en/choice.php index 8b2a52d4544..02bf3bd65ed 100644 --- a/mod/choice/lang/en/choice.php +++ b/mod/choice/lang/en/choice.php @@ -51,7 +51,7 @@ $string['choice:addinstance'] = 'Add a new choice'; $string['choiceclose'] = 'Allow responses until'; $string['choice:deleteresponses'] = 'Modify and delete responses'; $string['choice:downloadresponses'] = 'Download responses'; -$string['choicefull'] = 'This choice is full and there are no available places.'; +$string['choicefull'] = 'One or more of the options you have selected have already been filled. Your response has not been saved. Please make another selection.'; $string['choice:choose'] = 'Record a choice'; $string['choicecloseson'] = 'Choice closes on {$a}'; $string['choicename'] = 'Choice name'; From 5a8791336aea41d05d5b7ebb7490da36c2567aac Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 29 Jun 2018 10:08:36 +0800 Subject: [PATCH 3/3] MDL-56881 mod_choice: add unit tests for choice_user_submit_response Tests cover option limits and multiple option responses. --- mod/choice/tests/lib_test.php | 194 ++++++++++++++++++++++++++++++++++ 1 file changed, 194 insertions(+) diff --git a/mod/choice/tests/lib_test.php b/mod/choice/tests/lib_test.php index 950ea178e76..eb452e16dc1 100644 --- a/mod/choice/tests/lib_test.php +++ b/mod/choice/tests/lib_test.php @@ -785,4 +785,198 @@ class mod_choice_lib_testcase extends externallib_advanced_testcase { $this->assertNull($min); $this->assertNull($max); } + + /** + * Test choice_user_submit_response for a choice with specific options. + * Options: + * allowmultiple: false + * limitanswers: false + */ + public function test_choice_user_submit_response_no_multiple_no_limits() { + global $DB; + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $user = $generator->create_user(); + $user2 = $generator->create_user(); + + // User must be enrolled in the course for choice limits to be honoured properly. + $role = $DB->get_record('role', ['shortname' => 'student']); + $this->getDataGenerator()->enrol_user($user->id, $course->id, $role->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $role->id); + + // Create choice, with updates allowed and a two options both limited to 1 response each. + $choice = $generator->get_plugin_generator('mod_choice')->create_instance([ + 'course' => $course->id, + 'allowupdate' => false, + 'limitanswers' => false, + 'allowmultiple' => false, + 'option' => ['red', 'green'], + ]); + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Get the choice, with options and limits included. + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + // Now, save an response which includes the first option. + $this->assertNull(choice_user_submit_response($optionids[0], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that saving again without changing the selected option will not throw a 'choice full' exception. + $this->assertNull(choice_user_submit_response($optionids[1], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that saving a response for student 2 including the first option is allowed. + $this->assertNull(choice_user_submit_response($optionids[0], $choicewithoptions, $user2->id, $course, $cm)); + + // Confirm that trying to save multiple options results in an exception. + $this->expectException('moodle_exception'); + choice_user_submit_response([$optionids[1], $optionids[1]], $choicewithoptions, $user->id, $course, $cm); + } + + /** + * Test choice_user_submit_response for a choice with specific options. + * Options: + * allowmultiple: true + * limitanswers: false + */ + public function test_choice_user_submit_response_multiples_no_limits() { + global $DB; + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $user = $generator->create_user(); + $user2 = $generator->create_user(); + + // User must be enrolled in the course for choice limits to be honoured properly. + $role = $DB->get_record('role', ['shortname' => 'student']); + $this->getDataGenerator()->enrol_user($user->id, $course->id, $role->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $role->id); + + // Create choice, with updates allowed and a two options both limited to 1 response each. + $choice = $generator->get_plugin_generator('mod_choice')->create_instance([ + 'course' => $course->id, + 'allowupdate' => false, + 'allowmultiple' => true, + 'limitanswers' => false, + 'option' => ['red', 'green'], + ]); + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Get the choice, with options and limits included. + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + // Save a response which includes the first option only. + $this->assertNull(choice_user_submit_response([$optionids[0]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that adding an option to the response is allowed. + $this->assertNull(choice_user_submit_response([$optionids[0], $optionids[1]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that saving a response for student 2 including the first option is allowed. + $this->assertNull(choice_user_submit_response($optionids[0], $choicewithoptions, $user2->id, $course, $cm)); + + // Confirm that removing an option from the response is allowed. + $this->assertNull(choice_user_submit_response([$optionids[0]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that removing all options from the response is not allowed via this method. + $this->expectException('moodle_exception'); + choice_user_submit_response([], $choicewithoptions, $user->id, $course, $cm); + } + + /** + * Test choice_user_submit_response for a choice with specific options. + * Options: + * allowmultiple: false + * limitanswers: true + */ + public function test_choice_user_submit_response_no_multiples_limits() { + global $DB; + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $user = $generator->create_user(); + $user2 = $generator->create_user(); + + // User must be enrolled in the course for choice limits to be honoured properly. + $role = $DB->get_record('role', ['shortname' => 'student']); + $this->getDataGenerator()->enrol_user($user->id, $course->id, $role->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $role->id); + + // Create choice, with updates allowed and a two options both limited to 1 response each. + $choice = $generator->get_plugin_generator('mod_choice')->create_instance([ + 'course' => $course->id, + 'allowupdate' => false, + 'allowmultiple' => false, + 'limitanswers' => true, + 'option' => ['red', 'green'], + 'limit' => [1, 1] + ]); + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Get the choice, with options and limits included. + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + // Save a response which includes the first option only. + $this->assertNull(choice_user_submit_response($optionids[0], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that changing the option in the response is allowed. + $this->assertNull(choice_user_submit_response($optionids[1], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that limits are respected by trying to save the same option as another user. + $this->expectException('moodle_exception'); + choice_user_submit_response($optionids[1], $choicewithoptions, $user2->id, $course, $cm); + } + + /** + * Test choice_user_submit_response for a choice with specific options. + * Options: + * allowmultiple: true + * limitanswers: true + */ + public function test_choice_user_submit_response_multiples_limits() { + global $DB; + $this->resetAfterTest(true); + + $generator = $this->getDataGenerator(); + $course = $generator->create_course(); + $user = $generator->create_user(); + $user2 = $generator->create_user(); + + // User must be enrolled in the course for choice limits to be honoured properly. + $role = $DB->get_record('role', ['shortname' => 'student']); + $this->getDataGenerator()->enrol_user($user->id, $course->id, $role->id); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $role->id); + + // Create choice, with updates allowed and a two options both limited to 1 response each. + $choice = $generator->get_plugin_generator('mod_choice')->create_instance([ + 'course' => $course->id, + 'allowupdate' => false, + 'allowmultiple' => true, + 'limitanswers' => true, + 'option' => ['red', 'green'], + 'limit' => [1, 1] + ]); + $cm = get_coursemodule_from_instance('choice', $choice->id); + + // Get the choice, with options and limits included. + $choicewithoptions = choice_get_choice($choice->id); + $optionids = array_keys($choicewithoptions->option); + + // Now, save a response which includes the first option only. + $this->assertNull(choice_user_submit_response([$optionids[0]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that changing the option in the response is allowed. + $this->assertNull(choice_user_submit_response([$optionids[1]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that adding an option to the response is allowed. + $this->assertNull(choice_user_submit_response([$optionids[0], $optionids[1]], $choicewithoptions, $user->id, $course, $cm)); + + // Confirm that limits are respected by trying to save the same option as another user. + $this->expectException('moodle_exception'); + choice_user_submit_response($optionids[1], $choicewithoptions, $user2->id, $course, $cm); + } }