From 1f00fdb0d22007531ca81c6655ef19667667d5bc Mon Sep 17 00:00:00 2001 From: Huong Nguyen Date: Tue, 27 Nov 2018 16:14:38 +0700 Subject: [PATCH] MDL-63938 Enrolments: course_enrolment_manager method improvement course_enrolment_manager method should not always do a count_records and a get_records --- enrol/locallib.php | 69 ++++++++++--- enrol/renderer.php | 1 + enrol/tests/course_enrolment_manager_test.php | 98 +++++++++++++++++++ enrol/upgrade.txt | 7 ++ .../otherusersmanager/otherusersmanager.js | 17 +++- lang/en/enrol.php | 1 + 6 files changed, 175 insertions(+), 18 deletions(-) diff --git a/enrol/locallib.php b/enrol/locallib.php index 35e61ea34bf..c77e1510c9f 100644 --- a/enrol/locallib.php +++ b/enrol/locallib.php @@ -419,21 +419,50 @@ class course_enrolment_manager { * @param int $page which page number of the results to show. * @param int $perpage number of users per page. * @param int $addedenrollment number of users added to enrollment. - * @return array with two elememts: - * int total number of users matching the search. - * array of user objects returned by the query. + * @param bool $returnexactcount Return the exact total users using count_record or not. + * @return array with two or three elements: + * int totalusers Number users matching the search. (This element only exist if $returnexactcount was set to true) + * array users List of user objects returned by the query. + * boolean moreusers True if there are still more users, otherwise is False. + * @throws dml_exception */ - protected function execute_search_queries($search, $fields, $countfields, $sql, array $params, $page, $perpage, $addedenrollment=0) { + protected function execute_search_queries($search, $fields, $countfields, $sql, array $params, $page, $perpage, + $addedenrollment = 0, $returnexactcount = false) { global $DB, $CFG; list($sort, $sortparams) = users_order_by_sql('u', $search, $this->get_context()); $order = ' ORDER BY ' . $sort; - $totalusers = $DB->count_records_sql($countfields . $sql, $params); - $availableusers = $DB->get_records_sql($fields . $sql . $order, - array_merge($params, $sortparams), ($page*$perpage) - $addedenrollment, $perpage); + $totalusers = 0; + $moreusers = false; + $results = []; - return array('totalusers' => $totalusers, 'users' => $availableusers); + $availableusers = $DB->get_records_sql($fields . $sql . $order, + array_merge($params, $sortparams), ($page * $perpage) - $addedenrollment, $perpage + 1); + if ($availableusers) { + $totalusers = count($availableusers); + $moreusers = $totalusers > $perpage; + + if ($moreusers) { + // We need to discard the last record. + array_pop($availableusers); + } + + if ($returnexactcount && $moreusers) { + // There is more data. We need to do the exact count. + $totalusers = $DB->count_records_sql($countfields . $sql, $params); + } + } + + $results['users'] = $availableusers; + $results['moreusers'] = $moreusers; + + if ($returnexactcount) { + // Include totalusers in result if $returnexactcount flag is true. + $results['totalusers'] = $totalusers; + } + + return $results; } /** @@ -446,9 +475,15 @@ class course_enrolment_manager { * @param int $page Defaults to 0 * @param int $perpage Defaults to 25 * @param int $addedenrollment Defaults to 0 - * @return array Array(totalusers => int, users => array) + * @param bool $returnexactcount Return the exact total users using count_record or not. + * @return array with two or three elements: + * int totalusers Number users matching the search. (This element only exist if $returnexactcount was set to true) + * array users List of user objects returned by the query. + * boolean moreusers True if there are still more users, otherwise is False. + * @throws dml_exception */ - public function get_potential_users($enrolid, $search='', $searchanywhere=false, $page=0, $perpage=25, $addedenrollment=0) { + public function get_potential_users($enrolid, $search = '', $searchanywhere = false, $page = 0, $perpage = 25, + $addedenrollment = 0, $returnexactcount = false) { global $DB; list($ufields, $params, $wherecondition) = $this->get_basic_search_conditions($search, $searchanywhere); @@ -461,7 +496,8 @@ class course_enrolment_manager { AND ue.id IS NULL"; $params['enrolid'] = $enrolid; - return $this->execute_search_queries($search, $fields, $countfields, $sql, $params, $page, $perpage, $addedenrollment); + return $this->execute_search_queries($search, $fields, $countfields, $sql, $params, $page, $perpage, $addedenrollment, + $returnexactcount); } /** @@ -472,9 +508,14 @@ class course_enrolment_manager { * @param bool $searchanywhere * @param int $page Starting at 0 * @param int $perpage - * @return array + * @param bool $returnexactcount Return the exact total users using count_record or not. + * @return array with two or three elements: + * int totalusers Number users matching the search. (This element only exist if $returnexactcount was set to true) + * array users List of user objects returned by the query. + * boolean moreusers True if there are still more users, otherwise is False. + * @throws dml_exception */ - public function search_other_users($search='', $searchanywhere=false, $page=0, $perpage=25) { + public function search_other_users($search = '', $searchanywhere = false, $page = 0, $perpage = 25, $returnexactcount = false) { global $DB, $CFG; list($ufields, $params, $wherecondition) = $this->get_basic_search_conditions($search, $searchanywhere); @@ -487,7 +528,7 @@ class course_enrolment_manager { AND ra.id IS NULL"; $params['contextid'] = $this->context->id; - return $this->execute_search_queries($search, $fields, $countfields, $sql, $params, $page, $perpage); + return $this->execute_search_queries($search, $fields, $countfields, $sql, $params, $page, $perpage, 0, $returnexactcount); } /** diff --git a/enrol/renderer.php b/enrol/renderer.php index a1bc2417fbf..32c5c10c30c 100644 --- a/enrol/renderer.php +++ b/enrol/renderer.php @@ -680,6 +680,7 @@ class course_enrolment_other_users_table extends course_enrolment_table { $this->manager->get_moodlepage()->requires->strings_for_js(array( 'ajaxoneuserfound', 'ajaxxusersfound', + 'ajaxxmoreusersfound', 'ajaxnext25', 'enrol', 'enrolmentoptions', diff --git a/enrol/tests/course_enrolment_manager_test.php b/enrol/tests/course_enrolment_manager_test.php index 8f56a442e95..9e9f0ed5690 100644 --- a/enrol/tests/course_enrolment_manager_test.php +++ b/enrol/tests/course_enrolment_manager_test.php @@ -104,6 +104,20 @@ class core_course_enrolment_manager_testcase extends advanced_testcase { $this->course = $course; $this->users = $users; $this->groups = $groups; + + // Make sample users and not enroll to any course. + $this->getDataGenerator()->create_user([ + 'username' => 'testapiuser1', + 'firstname' => 'testapiuser 1' + ]); + $this->getDataGenerator()->create_user([ + 'username' => 'testapiuser2', + 'firstname' => 'testapiuser 2' + ]); + $this->getDataGenerator()->create_user([ + 'username' => 'testapiuser3', + 'firstname' => 'testapiuser 3' + ]); } /** @@ -239,4 +253,88 @@ class core_course_enrolment_manager_testcase extends advanced_testcase { $this->assertCount(1, $users, 'Only suspended users must be returned when suspended users filtering is applied.'); $this->assertArrayHasKey($this->users['user22']->id, $users); } + + /** + * Test get_potential_users without returnexactcount param. + * + * @dataProvider search_users_provider + * + * @param int $perpage Number of users per page. + * @param bool $returnexactcount Return the exact count or not. + * @param int $expectedusers Expected number of users return. + * @param int $expectedtotalusers Expected total of users in database. + * @param bool $expectedmoreusers Expected for more users return or not. + */ + public function test_get_potential_users($perpage, $returnexactcount, $expectedusers, $expectedtotalusers, $expectedmoreusers) { + global $DB, $PAGE; + $this->resetAfterTest(); + $this->setAdminUser(); + + $enrol = $DB->get_record('enrol', array('courseid' => $this->course->id, 'enrol' => 'manual')); + $manager = new course_enrolment_manager($PAGE, $this->course); + $users = $manager->get_potential_users($enrol->id, + 'testapiuser', + true, + 0, + $perpage, + 0, + $returnexactcount); + + $this->assertCount($expectedusers, $users['users']); + $this->assertEquals($expectedmoreusers, $users['moreusers']); + if ($returnexactcount) { + $this->assertArrayHasKey('totalusers', $users); + $this->assertEquals($expectedtotalusers, $users['totalusers']); + } else { + $this->assertArrayNotHasKey('totalusers', $users); + } + } + + /** + * Test search_other_users with returnexactcount param. + * + * @dataProvider search_users_provider + * + * @param int $perpage Number of users per page. + * @param bool $returnexactcount Return the exact count or not. + * @param int $expectedusers Expected number of users return. + * @param int $expectedtotalusers Expected total of users in database. + * @param bool $expectedmoreusers Expected for more users return or not. + */ + public function test_search_other_users($perpage, $returnexactcount, $expectedusers, $expectedtotalusers, $expectedmoreusers) { + global $PAGE; + $this->resetAfterTest(); + $this->setAdminUser(); + + $manager = new course_enrolment_manager($PAGE, $this->course); + $users = $manager->search_other_users( + 'testapiuser', + true, + 0, + $perpage, + $returnexactcount); + + $this->assertCount($expectedusers, $users['users']); + $this->assertEquals($expectedmoreusers, $users['moreusers']); + if ($returnexactcount) { + $this->assertArrayHasKey('totalusers', $users); + $this->assertEquals($expectedtotalusers, $users['totalusers']); + } else { + $this->assertArrayNotHasKey('totalusers', $users); + } + } + + /** + * Test case for test_get_potential_users and test_search_other_users tests. + * + * @return array Dataset + */ + public function search_users_provider() { + return [ + [2, false, 2, 3, true], + [5, false, 3, 3, false], + [2, true, 2, 3, true], + [5, true, 3, 3, false] + ]; + } } diff --git a/enrol/upgrade.txt b/enrol/upgrade.txt index 3202160231a..df24f1f5e54 100644 --- a/enrol/upgrade.txt +++ b/enrol/upgrade.txt @@ -1,6 +1,13 @@ This files describes API changes in /enrol/* - plugins, information provided here is intended especially for developers. +=== 3.7 === + +* Functions get_potential_users() and search_other_users() now return more information to avoid extra count query: + - users: List of user objects returned by the query. + - moreusers: True if there are still more users, otherwise is False. + - totalusers: Number users matching the search. (This element only exists if the function is called with $returnexactcount param set to true). + === 3.6 === * External function core_enrol_external::get_users_courses now return more information to avoid multiple queries to build the diff --git a/enrol/yui/otherusersmanager/otherusersmanager.js b/enrol/yui/otherusersmanager/otherusersmanager.js index 23d8ba9c0a5..77daf94b745 100644 --- a/enrol/yui/otherusersmanager/otherusersmanager.js +++ b/enrol/yui/otherusersmanager/otherusersmanager.js @@ -160,8 +160,9 @@ YUI.add('moodle-enrol-otherusersmanager', function(Y) { this._loadingNode.addClass(CSS.HIDDEN); }, processSearchResults : function(tid, outcome, args) { + var result; try { - var result = Y.JSON.parse(outcome.responseText); + result = Y.JSON.parse(outcome.responseText); if (result.error) { return new M.core.ajaxException(result); } @@ -186,18 +187,26 @@ YUI.add('moodle-enrol-otherusersmanager', function(Y) { } this.set(USERCOUNT, count); if (!args.append) { - var usersstr = (result.response.totalusers == '1')?M.util.get_string('ajaxoneuserfound', 'enrol'):M.util.get_string('ajaxxusersfound','enrol', result.response.totalusers); + var usersstr = ''; + if (this.get(USERCOUNT) === 1) { + usersstr = M.util.get_string('ajaxoneuserfound', 'enrol'); + } else if (result.response.moreusers) { + usersstr = M.util.get_string('ajaxxmoreusersfound', 'enrol', this.get(USERCOUNT)); + } else { + usersstr = M.util.get_string('ajaxxusersfound', 'enrol', this.get(USERCOUNT)); + } + var content = Y.Node.create('
') .append(Y.Node.create('
'+usersstr+'
')) .append(usersnode); - if (result.response.totalusers > (this.get(PAGE)+1)*25) { + if (result.response.moreusers) { var fetchmore = Y.Node.create('
'+M.util.get_string('ajaxnext25', 'enrol')+'
'); fetchmore.on('click', this.getUsers, this, true); content.append(fetchmore) } this.setContent(content); } else { - if (result.response.totalusers <= (this.get(PAGE)+1)*25) { + if (!result.response.moreusers) { this.get(BASE).one('.'+CSS.MORERESULTS).remove(); } } diff --git a/lang/en/enrol.php b/lang/en/enrol.php index 8f452db9dd5..ea1bb8e32c5 100644 --- a/lang/en/enrol.php +++ b/lang/en/enrol.php @@ -28,6 +28,7 @@ $string['addinstance'] = 'Add method'; $string['addinstanceanother'] = 'Add method and create another'; $string['ajaxoneuserfound'] = '1 user found'; $string['ajaxxusersfound'] = '{$a} users found'; +$string['ajaxxmoreusersfound'] = 'More than {$a} users found'; $string['ajaxnext25'] = 'Next 25...'; $string['assignnotpermitted'] = 'You do not have permission or can not assign roles in this course.'; $string['bulkuseroperation'] = 'Bulk user operation';