From 9ded266b4eb98c3615c64f739e0d726abd7a66d9 Mon Sep 17 00:00:00 2001
From: Paul Holden <pholden@greenhead.ac.uk>
Date: Fri, 8 Mar 2019 12:32:24 +0000
Subject: [PATCH] MDL-43130 access: fix user counting when retrieving
 assignable roles.

Previously users assigned the same role in a context via multiple
components would be counted multiple times.
---
 lib/accesslib.php            |  2 +-
 lib/tests/accesslib_test.php | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/lib/accesslib.php b/lib/accesslib.php
index ad1960b4f84..238feaf1449 100644
--- a/lib/accesslib.php
+++ b/lib/accesslib.php
@@ -3043,7 +3043,7 @@ function get_assignable_roles(context $context, $rolenamedisplay = ROLENAME_ALIA
     $extrafields = '';
 
     if ($withusercounts) {
-        $extrafields = ', (SELECT count(u.id)
+        $extrafields = ', (SELECT COUNT(DISTINCT u.id)
                              FROM {role_assignments} cra JOIN {user} u ON cra.userid = u.id
                             WHERE cra.roleid = r.id AND cra.contextid = :conid AND u.deleted = 0
                           ) AS usercount';
diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php
index 9951121065f..0d7f24b1968 100644
--- a/lib/tests/accesslib_test.php
+++ b/lib/tests/accesslib_test.php
@@ -1174,6 +1174,39 @@ class core_accesslib_testcase extends advanced_testcase {
         }
     }
 
+    /**
+     * Test user count of assignable roles in context where users are assigned the role via different components.
+     */
+    public function test_get_assignable_roles_distinct_usercount() {
+        global $DB;
+
+        $this->resetAfterTest(true);
+
+        $this->setAdminUser();
+
+        $course = $this->getDataGenerator()->create_course();
+        $context = \context_course::instance($course->id);
+
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+
+        $studentrole = $DB->get_record('role', ['shortname' => 'student']);
+
+        // Assign each user the student role in course.
+        role_assign($studentrole->id, $user1->id, $context->id);
+        role_assign($studentrole->id, $user2->id, $context->id);
+
+        list($rolenames, $rolecounts, $nameswithcounts) = get_assignable_roles($context, ROLENAME_SHORT, true);
+        $this->assertEquals(2, $rolecounts[$studentrole->id]);
+
+        // Assign first user the student role in course again (this time via 'enrol_self' component).
+        role_assign($studentrole->id, $user1->id, $context->id, 'enrol_self', 1);
+
+        // There are still only two distinct users.
+        list($rolenames, $rolecounts, $nameswithcounts) = get_assignable_roles($context, ROLENAME_SHORT, true);
+        $this->assertEquals(2, $rolecounts[$studentrole->id]);
+    }
+
     /**
      * Test getting of all switchable roles.
      */
-- 
2.17.1