MDL-46803 course: Prevent unenrol users with multiple roles during reset

moodle · Mar 18, 2019 · f8c209c · f8c209c
1 parent f3ad037
commit f8c209c
Show file tree

Hide file tree

Showing 4 changed files with 124 additions and 2 deletions.
diff --git a/course/tests/courselib_test.php b/course/tests/courselib_test.php
@@ -3426,7 +3426,9 @@ public function course_dates_reset_provider() {
     }
 
     /**
-     * Test reset_course_userdata() with reset_roles_overrides enabled.
+     * Test reset_course_userdata()
+     *    - with reset_roles_overrides enabled
+     *    - with selective role unenrolments
      */
     public function test_course_roles_reset() {
         global $DB;
@@ -3441,6 +3443,7 @@ public function test_course_roles_reset() {
         $roleid = $DB->get_field('role', 'id', array('shortname' => 'student'), MUST_EXIST);
         $generator->enrol_user($user->id, $course->id, $roleid);
 
+        // Test case with reset_roles_overrides enabled.
         // Override course so it does NOT allow students 'mod/forum:viewdiscussion'.
         $coursecontext = context_course::instance($course->id);
         assign_capability('mod/forum:viewdiscussion', CAP_PREVENT, $roleid, $coursecontext->id);
@@ -3456,6 +3459,39 @@ public function test_course_roles_reset() {
 
         // Check new expected capabilities - override at the course level should be reset.
         $this->assertTrue(has_capability('mod/forum:viewdiscussion', $coursecontext, $user));
+
+        // Test case with selective role unenrolments.
+        $roles = array();
+        $roles['student'] = $DB->get_field('role', 'id', array('shortname' => 'student'), MUST_EXIST);
+        $roles['teacher'] = $DB->get_field('role', 'id', array('shortname' => 'teacher'), MUST_EXIST);
+
+        // We enrol a user with student and teacher roles.
+        $generator->enrol_user($user->id, $course->id, $roles['student']);
+        $generator->enrol_user($user->id, $course->id, $roles['teacher']);
+
+        // When we reset only student role, we expect to keep teacher role.
+        $resetdata = new stdClass();
+        $resetdata->id = $course->id;
+        $resetdata->unenrol_users = array($roles['student']);
+        reset_course_userdata($resetdata);
+
+        $usersroles = enrol_get_course_users_roles($course->id);
+        $this->assertArrayHasKey($user->id, $usersroles);
+        $this->assertArrayHasKey($roles['teacher'], $usersroles[$user->id]);
+        $this->assertArrayNotHasKey($roles['student'], $usersroles[$user->id]);
+        $this->assertCount(1, $usersroles[$user->id]);
+
+        // We reenrol user as student.
+        $generator->enrol_user($user->id, $course->id, $roles['student']);
+
+        // When we reset student and teacher roles, we expect no roles left.
+        $resetdata = new stdClass();
+        $resetdata->id = $course->id;
+        $resetdata->unenrol_users = array($roles['student'], $roles['teacher']);
+        reset_course_userdata($resetdata);
+
+        $usersroles = enrol_get_course_users_roles($course->id);
+        $this->assertEmpty($usersroles);
     }
 
     public function test_course_check_module_updates_since() {

diff --git a/enrol/tests/enrollib_test.php b/enrol/tests/enrollib_test.php
@@ -1028,4 +1028,56 @@ public function test_enrol_get_my_courses_sort_by_last_access(
 
         $this->assertEquals($expectedcourses, $actual);
     }
+
+    /**
+     * Test enrol_get_course_users_roles function.
+     *
+     * @return void
+     */
+    public function test_enrol_get_course_users_roles() {
+        global $DB;
+
+        $this->resetAfterTest();
+
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+        $course = $this->getDataGenerator()->create_course();
+        $context = context_course::instance($course->id);
+
+        $roles = array();
+        $roles['student'] = $DB->get_field('role', 'id', array('shortname' => 'student'), MUST_EXIST);
+        $roles['teacher'] = $DB->get_field('role', 'id', array('shortname' => 'teacher'), MUST_EXIST);
+
+        $manual = enrol_get_plugin('manual');
+        $this->assertNotEmpty($manual);
+
+        $enrol = $DB->get_record('enrol', array('courseid' => $course->id, 'enrol' => 'manual'), '*', MUST_EXIST);
+
+        // Test without enrolments.
+        $this->assertEmpty(enrol_get_course_users_roles($course->id));
+
+        // Test with 1 user, 1 role.
+        $manual->enrol_user($enrol, $user1->id, $roles['student']);
+        $return = enrol_get_course_users_roles($course->id);
+        $this->assertArrayHasKey($user1->id, $return);
+        $this->assertArrayHasKey($roles['student'], $return[$user1->id]);
+        $this->assertArrayNotHasKey($roles['teacher'], $return[$user1->id]);
+
+        // Test with 1 user, 2 role.
+        $manual->enrol_user($enrol, $user1->id, $roles['teacher']);
+        $return = enrol_get_course_users_roles($course->id);
+        $this->assertArrayHasKey($user1->id, $return);
+        $this->assertArrayHasKey($roles['student'], $return[$user1->id]);
+        $this->assertArrayHasKey($roles['teacher'], $return[$user1->id]);
+
+        // Test with another user, 1 role.
+        $manual->enrol_user($enrol, $user2->id, $roles['student']);
+        $return = enrol_get_course_users_roles($course->id);
+        $this->assertArrayHasKey($user1->id, $return);
+        $this->assertArrayHasKey($roles['student'], $return[$user1->id]);
+        $this->assertArrayHasKey($roles['teacher'], $return[$user1->id]);
+        $this->assertArrayHasKey($user2->id, $return);
+        $this->assertArrayHasKey($roles['student'], $return[$user2->id]);
+        $this->assertArrayNotHasKey($roles['teacher'], $return[$user2->id]);
+    }
 }
diff --git a/lib/enrollib.php b/lib/enrollib.php
@@ -868,6 +868,31 @@ function enrol_get_users_courses($userid, $onlyactive = false, $fields = null, $
 
 }
 
+/**
+ * Returns list of roles per users into course.
+ *
+ * @param int $courseid Course id.
+ * @return array Array[$userid][$roleid] = role_assignment.
+ */
+function enrol_get_course_users_roles(int $courseid) : array {
+    global $DB;
+
+    $context = context_course::instance($courseid);
+
+    $roles = array();
+
+    $records = $DB->get_recordset('role_assignments', array('contextid' => $context->id));
+    foreach ($records as $record) {
+        if (isset($roles[$record->userid]) === false) {
+            $roles[$record->userid] = array();
+        }
+        $roles[$record->userid][$record->roleid] = $record;
+    }
+    $records->close();
+
+    return $roles;
+}
+
 /**
  * Can user access at least one enrolled course?
  *

diff --git a/lib/moodlelib.php b/lib/moodlelib.php
@@ -5458,6 +5458,7 @@ function reset_course_userdata($data) {
             }
         }
 
+        $usersroles = enrol_get_course_users_roles($data->courseid);
         foreach ($data->unenrol_users as $withroleid) {
             if ($withroleid) {
                 $sql = "SELECT ue.*
@@ -5489,7 +5490,15 @@ function reset_course_userdata($data) {
                     continue;
                 }
 
-                $plugin->unenrol_user($instance, $ue->userid);
+                if ($withroleid && count($usersroles[$ue->userid]) > 1) {
+                    // If we don't remove all roles and user has more than one role, just remove this role.
+                    role_unassign($withroleid, $ue->userid, $context->id);
+
+                    unset($usersroles[$ue->userid][$withroleid]);
+                } else {
+                    // If we remove all roles or user has only one role, unenrol user from course.
+                    $plugin->unenrol_user($instance, $ue->userid);
+                }
                 $data->unenrolled[$ue->userid] = $ue->userid;
             }
             $rs->close();