MDL-49395 accesslib: Make get_suspended_userids more efficient

lib/accesslib.php

@@ -2256,9 +2256,10 @@ function can_access_course(stdClass $course, $user = null, $withcapability = '',
  * @param string $withcapability
  * @param int $groupid 0 means ignore groups, any other value limits the result by group id
  * @param bool $onlyactive consider only active enrolments in enabled plugins and time restrictions
+ * @param bool $onlysuspended inverse of onlyactive, consider only suspended enrolments
  * @return array list($sql, $params)
  */
-function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0, $onlyactive = false) {
+function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0, $onlyactive = false, $onlysuspended = false) {
     global $DB, $CFG;
 
     // use unique prefix just in case somebody makes some SQL magic with the result
@@ -2271,6 +2272,13 @@ function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0,
 
     $isfrontpage = ($coursecontext->instanceid == SITEID);
 
+    if ($onlyactive && $onlysuspended) {
+        throw new coding_exception("Both onlyactive and onlysuspended are set, this is probably not what you want!");
+    }
+    if ($isfrontpage && $onlysuspended) {
+        throw new coding_exception("onlysuspended is not supported on frontpage; please add your own early-exit!");
+    }
+
     $joins  = array();
     $wheres = array();
     $params = array();
@@ -2387,13 +2395,28 @@ function get_enrolled_sql(context $context, $withcapability = '', $groupid = 0,
     if ($isfrontpage) {
         // all users are "enrolled" on the frontpage
     } else {
-        $joins[] = "JOIN {user_enrolments} {$prefix}ue ON {$prefix}ue.userid = {$prefix}u.id";
+        $where1 = "{$prefix}ue.status = :{$prefix}active AND {$prefix}e.status = :{$prefix}enabled";
-        $joins[] = "JOIN {enrol} {$prefix}e ON ({$prefix}e.id = {$prefix}ue.enrolid AND {$prefix}e.courseid = :{$prefix}courseid)";
+        $where2 = "{$prefix}ue.timestart < :{$prefix}now1 AND ({$prefix}ue.timeend = 0 OR {$prefix}ue.timeend > :{$prefix}now2)";
+        $ejoin = "JOIN {enrol} {$prefix}e ON ({$prefix}e.id = {$prefix}ue.enrolid AND {$prefix}e.courseid = :{$prefix}courseid)";
         $params[$prefix.'courseid'] = $coursecontext->instanceid;
 
-        if ($onlyactive) {
+        if (!$onlysuspended) {
-            $wheres[] = "{$prefix}ue.status = :{$prefix}active AND {$prefix}e.status = :{$prefix}enabled";
+            $joins[] = "JOIN {user_enrolments} {$prefix}ue ON {$prefix}ue.userid = {$prefix}u.id";
-            $wheres[] = "{$prefix}ue.timestart < :{$prefix}now1 AND ({$prefix}ue.timeend = 0 OR {$prefix}ue.timeend > :{$prefix}now2)";
+            $joins[] = $ejoin;
+            if ($onlyactive) {
+                $wheres[] = "$where1 AND $where2";
+            }
+        } else {
+            // Suspended only where there is enrolment but ALL are suspended.
+            // Consider multiple enrols where one is not suspended or plain role_assign.
+            $enrolselect = "SELECT DISTINCT {$prefix}ue.userid FROM {user_enrolments} {$prefix}ue $ejoin WHERE $where1 AND $where2";
+            $joins[] = "JOIN {user_enrolments} {$prefix}ue1 ON {$prefix}ue1.userid = {$prefix}u.id";
+            $joins[] = "JOIN {enrol} {$prefix}e1 ON ({$prefix}e1.id = {$prefix}ue1.enrolid AND {$prefix}e1.courseid = :{$prefix}_e1_courseid)";
+            $params["{$prefix}_e1_courseid"] = $coursecontext->instanceid;
+            $wheres[] = "{$prefix}u.id NOT IN ($enrolselect)";
+        }
+
+        if ($onlyactive || $onlysuspended) {
             $now = round(time(), -2); // rounding helps caching in DB
             $params = array_merge($params, array($prefix.'enabled'=>ENROL_INSTANCE_ENABLED,
                                                  $prefix.'active'=>ENROL_USER_ACTIVE,
@@ -7506,7 +7529,6 @@ function extract_suspended_users($context, &$users, $ignoreusers=array()) {
 function get_suspended_userids(context $context, $usecache = false) {
     global $DB;
 
-    // Check the cache first for performance reasons if enabled.
     if ($usecache) {
         $cache = cache::make('core', 'suspended_userids');
         $susers = $cache->get($context->id);
@@ -7515,21 +7537,14 @@ function get_suspended_userids(context $context, $usecache = false) {
         }
     }
 
-    // Get all enrolled users.
+    $coursecontext = $context->get_course_context();
-    list($sql, $params) = get_enrolled_sql($context);
-    $users = $DB->get_records_sql($sql, $params);
-
-    // Get active enrolled users.
-    list($sql, $params) = get_enrolled_sql($context, null, null, true);
-    $activeusers = $DB->get_records_sql($sql, $params);
-
     $susers = array();
-    if (sizeof($activeusers) != sizeof($users)) {
+
-        foreach ($users as $userid => $user) {
+    // Front page users are always enrolled, so suspended list is empty.
-            if (!array_key_exists($userid, $activeusers)) {
+    if ($coursecontext->instanceid != SITEID) {
-                $susers[$userid] = $userid;
+        list($sql, $params) = get_enrolled_sql($context, null, null, false, true);
-            }
+        $susers = $DB->get_fieldset_sql($sql, $params);
-        }
+        $susers = array_combine($susers, $susers);
     }
 
     // Cache results for the remainder of this request.
@@ -7537,6 +7552,5 @@ function get_suspended_userids(context $context, $usecache = false) {
         $cache->set($context->id, $susers);
     }
 
-    // Return.
     return $susers;
 }

lib/tests/accesslib_test.php

@@ -1763,6 +1763,68 @@ class core_accesslib_testcase extends advanced_testcase {
         }
     }
 
+    /**
+     * Test enrolled users SQL works via common paths.
+     * Including active/suspended only cases and multiple enrols.
+     */
+    public function test_get_enrolled_sql() {
+        global $DB, $CFG, $USER;
+
+        $this->resetAfterTest();
+
+        $course = $this->getDataGenerator()->create_course();
+        $context = context_course::instance($course->id);
+
+        $role1 = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
+        $role2 = $DB->get_record('role', array('shortname' => 'teacher'), '*', MUST_EXIST);
+        $user = $this->getDataGenerator()->create_user();
+
+        // This user should not appear anywhere, we're not interested in that context.
+        $user2 = $this->getDataGenerator()->create_user();
+        $course2 = $this->getDataGenerator()->create_course();
+        $this->getDataGenerator()->enrol_user($user2->id, $course2->id, $role1->id);
+
+        // Role assignment is not the same as course enrollment.
+        role_assign($role2->id, $user->id, $context->id);
+        $enrold = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, false);
+        $active = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, true);
+        $susped = get_suspended_userids($context);
+        $this->assertFalse(isset($enrold[$user->id]));
+        $this->assertFalse(isset($active[$user->id]));
+        $this->assertFalse(isset($susped[$user->id]));
+        $this->assertCount(0, $enrold);
+        $this->assertCount(0, $active);
+        $this->assertCount(0, $susped);
+
+        // Add a suspended enrol.
+        $this->getDataGenerator()->enrol_user($user->id, $course->id, $role1->id, 'self', 0, 0, ENROL_USER_SUSPENDED);
+
+        // Should be enrolled, but not active - user is suspended.
+        $enrold = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, false);
+        $active = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, true);
+        $susped = get_suspended_userids($context);
+        $this->assertTrue(isset($enrold[$user->id]));
+        $this->assertFalse(isset($active[$user->id]));
+        $this->assertTrue(isset($susped[$user->id]));
+        $this->assertCount(1, $enrold);
+        $this->assertCount(0, $active);
+        $this->assertCount(1, $susped);
+
+        // Add an active enrol for the user. Any active enrol makes them enrolled.
+        $this->getDataGenerator()->enrol_user($user->id, $course->id, $role1->id);
+
+        // User should be active now.
+        $enrold = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, false);
+        $active = get_enrolled_users($context, '', 0, 'u.id', null, 0, 0, true);
+        $susped = get_suspended_userids($context);
+        $this->assertTrue(isset($enrold[$user->id]));
+        $this->assertTrue(isset($active[$user->id]));
+        $this->assertFalse(isset($susped[$user->id]));
+        $this->assertCount(1, $enrold);
+        $this->assertCount(1, $active);
+        $this->assertCount(0, $susped);
+    }
+
     /**
      * A small functional test of permission evaluations.
      */