Merge branch 'MDL-56881-35' of https://github.com/snake/moodle into M…

…OODLE_35_STABLE

mod/choice/lang/en/choice.php

@@ -51,7 +51,7 @@
 $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';

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.

mod/choice/tests/lib_test.php

@@ -785,4 +785,198 @@ public function test_mod_choice_core_calendar_get_valid_event_timestart_range_cl
         $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);
+    }
 }