From ee2187273fea18c22270f9e2c37957bb3733a362 Mon Sep 17 00:00:00 2001 From: Daniel Thee Roperto Date: Wed, 22 Nov 2017 16:13:03 +1100 Subject: [PATCH 1/5] MDL-57564 enrol: Change the default sort in enrol_get_my_courses() The new default value (null) now respects the navsortmycoursessort behaviour and should be consistently used whenever we are displaying the courses in the UI. --- lib/enrollib.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/enrollib.php b/lib/enrollib.php index d69094bdd33..aa50c1de537 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -554,15 +554,23 @@ function enrol_add_course_navigation(navigation_node $coursenode, $course) { * password). * * @param string|array $fields - * @param string $sort + * @param string|null $sort * @param int $limit max number of courses * @param array $courseids the list of course ids to filter by * @param bool $allaccessible Include courses user is not enrolled in, but can access * @return array */ -function enrol_get_my_courses($fields = null, $sort = 'visible DESC,sortorder ASC', +function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $courseids = [], $allaccessible = false) { - global $DB, $USER, $CFG; + global $CFG, $DB, $USER; + + if (is_null($sort)) { + if (empty($CFG->navsortmycoursessort)) { + $sort = 'visible DESC, sortorder ASC'; + } else { + $sort = 'visible DESC, '.$CFG->navsortmycoursessort.' ASC'; + } + } // Guest account does not have any enrolled courses. if (!$allaccessible && (isguestuser() or !isloggedin())) { From a3d003603ac5e6dd7bf2a41e4325822c3e2a6436 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 5 Dec 2017 21:53:55 +0100 Subject: [PATCH 2/5] MDL-57564 enrol: Change the default sort in other two functions Similarly to what the previous patch does, we now change the default sorting in functions enrol_get_users_courses() and enrol_get_all_users_courses() too. The patch also adds missing phpDocs for the functions, improves the readability of some existing bits and mentions the changes in the enrol/upgrade.txt file. --- enrol/upgrade.txt | 5 ++++ lib/enrollib.php | 62 ++++++++++++++++++++++++++--------------------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/enrol/upgrade.txt b/enrol/upgrade.txt index 528fe375c89..09ff3baf9bd 100644 --- a/enrol/upgrade.txt +++ b/enrol/upgrade.txt @@ -1,6 +1,11 @@ This files describes API changes in /enrol/* - plugins, information provided here is intended especially for developers. +=== 3.5 === + +* Default sorting in enrol_get_my_courses(), enrol_get_all_users_courses() and enrol_get_users_courses() now respects + the site setting "navsortmycoursessort" and should be consistently used when displaying the courses in the UI. + === 3.4 === * render_course_enrolment_users_table method has been removed from the renderer. The enrolled users page is now diff --git a/lib/enrollib.php b/lib/enrollib.php index aa50c1de537..d72133aea55 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -544,27 +544,25 @@ function enrol_add_course_navigation(navigation_node $coursenode, $course) { /** * Returns list of courses current $USER is enrolled in and can access * - * - $fields is an array of field names to ADD - * so name the fields you really need, which will - * be added and uniq'd + * The $fields param is a list of field names to ADD so name just the fields you really need, + * which will be added and uniq'd. * * If $allaccessible is true, this will additionally return courses that the current user is not * enrolled in, but can access because they are open to the user for other reasons (course view * permission, currently viewing course as a guest, or course allows guest access without * password). * - * @param string|array $fields - * @param string|null $sort + * @param string|array $fields Extra fields to be returned (array or comma-separated list). + * @param string|null $sort Comma separated list of fields to sort by, defaults to respecting navsortmycoursessort. * @param int $limit max number of courses * @param array $courseids the list of course ids to filter by * @param bool $allaccessible Include courses user is not enrolled in, but can access * @return array */ -function enrol_get_my_courses($fields = null, $sort = null, - $limit = 0, $courseids = [], $allaccessible = false) { - global $CFG, $DB, $USER; +function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $courseids = [], $allaccessible = false) { + global $DB, $USER, $CFG; - if (is_null($sort)) { + if ($sort === null) { if (empty($CFG->navsortmycoursessort)) { $sort = 'visible DESC, sortorder ASC'; } else { @@ -796,19 +794,19 @@ function enrol_get_course_description_texts($course) { /** * Returns list of courses user is enrolled into. - * (Note: use enrol_get_all_users_courses if you want to use the list wihtout any cap checks ) * - * - $fields is an array of fieldnames to ADD - * so name the fields you really need, which will - * be added and uniq'd + * Note: Use {@link enrol_get_all_users_courses()} if you need the list without any capability checks. * - * @param int $userid - * @param bool $onlyactive return only active enrolments in courses user may see - * @param string|array $fields - * @param string $sort + * The $fields param is a list of field names to ADD so name just the fields you really need, + * which will be added and uniq'd. + * + * @param int $userid User whose courses are returned, defaults to the current user. + * @param bool $onlyactive Return only active enrolments in courses user may see. + * @param string|array $fields Extra fields to be returned (array or comma-separated list). + * @param string|null $sort Comma separated list of fields to sort by, defaults to respecting navsortmycoursessort. * @return array */ -function enrol_get_users_courses($userid, $onlyactive = false, $fields = NULL, $sort = 'visible DESC,sortorder ASC') { +function enrol_get_users_courses($userid, $onlyactive = false, $fields = null, $sort = null) { global $DB; $courses = enrol_get_all_users_courses($userid, $onlyactive, $fields, $sort); @@ -885,19 +883,27 @@ function enrol_user_sees_own_courses($user = null) { } /** - * Returns list of courses user is enrolled into without any capability checks - * - $fields is an array of fieldnames to ADD - * so name the fields you really need, which will - * be added and uniq'd + * Returns list of courses user is enrolled into without performing any capability checks. * - * @param int $userid - * @param bool $onlyactive return only active enrolments in courses user may see - * @param string|array $fields - * @param string $sort + * The $fields param is a list of field names to ADD so name just the fields you really need, + * which will be added and uniq'd. + * + * @param int $userid User whose courses are returned, defaults to the current user. + * @param bool $onlyactive Return only active enrolments in courses user may see. + * @param string|array $fields Extra fields to be returned (array or comma-separated list). + * @param string|null $sort Comma separated list of fields to sort by, defaults to respecting navsortmycoursessort. * @return array */ -function enrol_get_all_users_courses($userid, $onlyactive = false, $fields = NULL, $sort = 'visible DESC,sortorder ASC') { - global $DB; +function enrol_get_all_users_courses($userid, $onlyactive = false, $fields = null, $sort = null) { + global $CFG, $DB; + + if ($sort === null) { + if (empty($CFG->navsortmycoursessort)) { + $sort = 'visible DESC, sortorder ASC'; + } else { + $sort = 'visible DESC, '.$CFG->navsortmycoursessort.' ASC'; + } + } // Guest account does not have any courses if (isguestuser($userid) or empty($userid)) { From c260117d1518e9f796e4ff420f0ffd4bcd42c9ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 6 Dec 2017 23:14:08 +0100 Subject: [PATCH 3/5] MDL-57564 enrol: Add unit tests for the new sorting behaviour --- enrol/tests/enrollib_test.php | 53 +++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 9 deletions(-) diff --git a/enrol/tests/enrollib_test.php b/enrol/tests/enrollib_test.php index d52dc6ff07a..1678462783e 100644 --- a/enrol/tests/enrollib_test.php +++ b/enrol/tests/enrollib_test.php @@ -55,10 +55,24 @@ class core_enrollib_testcase extends advanced_testcase { $category1 = $this->getDataGenerator()->create_category(array('visible'=>0)); $category2 = $this->getDataGenerator()->create_category(); - $course1 = $this->getDataGenerator()->create_course(array('category'=>$category1->id)); - $course2 = $this->getDataGenerator()->create_course(array('category'=>$category2->id)); - $course3 = $this->getDataGenerator()->create_course(array('category'=>$category2->id, 'visible'=>0)); - $course4 = $this->getDataGenerator()->create_course(array('category'=>$category2->id)); + + $course1 = $this->getDataGenerator()->create_course(array( + 'shortname' => 'Z', + 'category' => $category1->id, + )); + $course2 = $this->getDataGenerator()->create_course(array( + 'shortname' => 'X', + 'category' => $category2->id, + )); + $course3 = $this->getDataGenerator()->create_course(array( + 'shortname' => 'Y', + 'category' => $category2->id, + 'visible' => 0, + )); + $course4 = $this->getDataGenerator()->create_course(array( + 'shortname' => 'W', + 'category' => $category2->id, + )); $maninstance1 = $DB->get_record('enrol', array('courseid'=>$course1->id, 'enrol'=>'manual'), '*', MUST_EXIST); $DB->set_field('enrol', 'status', ENROL_INSTANCE_DISABLED, array('id'=>$maninstance1->id)); @@ -150,6 +164,18 @@ class core_enrollib_testcase extends advanced_testcase { $courses = enrol_get_all_users_courses($user2->id, false, null, 'id DESC'); $this->assertEquals(array($course3->id, $course2->id, $course1->id), array_keys($courses)); + + // Make sure that implicit sorting defined in navsortmycoursessort is respected. + + $CFG->navsortmycoursessort = 'shortname'; + + $courses = enrol_get_all_users_courses($user1->id); + $this->assertEquals(array($course2->id, $course3->id, $course1->id), array_keys($courses)); + + // But still the explicit sorting takes precedence over the implicit one. + + $courses = enrol_get_all_users_courses($user1->id, false, null, 'shortname DESC'); + $this->assertEquals(array($course1->id, $course3->id, $course2->id), array_keys($courses)); } public function test_enrol_user_sees_own_courses() { @@ -590,15 +616,15 @@ class core_enrollib_testcase extends advanced_testcase { // Create test user and 4 courses, two of which have guest access enabled. $user = $this->getDataGenerator()->create_user(); $course1 = $this->getDataGenerator()->create_course( - (object)array('shortname' => 'Z', + (object)array('shortname' => 'X', 'enrol_guest_status_0' => ENROL_INSTANCE_DISABLED, 'enrol_guest_password_0' => '')); $course2 = $this->getDataGenerator()->create_course( - (object)array('shortname' => 'Y', + (object)array('shortname' => 'Z', 'enrol_guest_status_0' => ENROL_INSTANCE_ENABLED, 'enrol_guest_password_0' => '')); $course3 = $this->getDataGenerator()->create_course( - (object)array('shortname' => 'X', + (object)array('shortname' => 'Y', 'enrol_guest_status_0' => ENROL_INSTANCE_ENABLED, 'enrol_guest_password_0' => 'frog')); $course4 = $this->getDataGenerator()->create_course( @@ -645,10 +671,19 @@ class core_enrollib_testcase extends advanced_testcase { $this->assertObjectHasAttribute('summary', $courses[$course3->id]); $this->assertObjectHasAttribute('summaryformat', $courses[$course3->id]); - // Check sort parameter still works. - $courses = enrol_get_my_courses(null, 'shortname', 0, [], true); + // By default, courses are ordered by sortorder - which by default is most recent first. + $courses = enrol_get_my_courses(null, null, 0, [], true); $this->assertEquals([$course3->id, $course2->id, $course1->id], array_keys($courses)); + // Make sure that implicit sorting defined in navsortmycoursessort is respected. + $CFG->navsortmycoursessort = 'shortname'; + $courses = enrol_get_my_courses(null, null, 0, [], true); + $this->assertEquals([$course1->id, $course3->id, $course2->id], array_keys($courses)); + + // But still the explicit sorting takes precedence over the implicit one. + $courses = enrol_get_my_courses(null, 'shortname DESC', 0, [], true); + $this->assertEquals([$course2->id, $course3->id, $course1->id], array_keys($courses)); + // Check filter parameter still works. $courses = enrol_get_my_courses(null, 'id', 0, [$course2->id, $course3->id, $course4->id], true); $this->assertEquals([$course2->id, $course3->id], array_keys($courses)); From 04985346a20c0e748b694755021ae65ee3f1317f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 5 Dec 2017 21:56:07 +0100 Subject: [PATCH 4/5] MDL-57564 enrol: Make use of the new default sort parameter Now when enrol_get_my_courses(), enrol_get_users_courses() and enrol_get_all_users_courses() methods automatically reflect the navsortmycoursessort setting by default, we do not need to manually compose the sorting any more. --- auth/mnet/auth.php | 2 +- blocks/course_list/block_course_list.php | 9 +-------- blocks/myoverview/classes/output/main.php | 2 +- calendar/externallib.php | 4 ++-- course/renderer.php | 8 +------- lib/myprofilelib.php | 2 +- lib/navigationlib.php | 9 +-------- 7 files changed, 8 insertions(+), 28 deletions(-) diff --git a/auth/mnet/auth.php b/auth/mnet/auth.php index 1f269f77ff2..b3b22f534b1 100644 --- a/auth/mnet/auth.php +++ b/auth/mnet/auth.php @@ -382,7 +382,7 @@ class auth_plugin_mnet extends auth_plugin_base { // with info so that the IDP can maintain mnetservice_enrol_enrolments $mnetrequest->add_param($remoteuser->username); $fields = 'id, category, sortorder, fullname, shortname, idnumber, summary, startdate, visible'; - $courses = enrol_get_users_courses($localuser->id, false, $fields, 'visible DESC,sortorder ASC'); + $courses = enrol_get_users_courses($localuser->id, false, $fields); if (is_array($courses) && !empty($courses)) { // Second request to do the JOINs that we'd have done // inside enrol_get_users_courses() if we had been allowed diff --git a/blocks/course_list/block_course_list.php b/blocks/course_list/block_course_list.php index 36003f7f3af..10cbf23d5e7 100644 --- a/blocks/course_list/block_course_list.php +++ b/blocks/course_list/block_course_list.php @@ -57,14 +57,7 @@ class block_course_list extends block_list { if (empty($CFG->disablemycourses) and isloggedin() and !isguestuser() and !(has_capability('moodle/course:update', context_system::instance()) and $adminseesall)) { // Just print My Courses - // As this is producing navigation sort order should default to $CFG->navsortmycoursessort instead - // of using the default. - if (!empty($CFG->navsortmycoursessort)) { - $sortorder = 'visible DESC, ' . $CFG->navsortmycoursessort . ' ASC'; - } else { - $sortorder = 'visible DESC, sortorder ASC'; - } - if ($courses = enrol_get_my_courses(NULL, $sortorder)) { + if ($courses = enrol_get_my_courses()) { foreach ($courses as $course) { $coursecontext = context_course::instance($course->id); $linkcss = $course->visible ? "" : " class=\"dimmed\" "; diff --git a/blocks/myoverview/classes/output/main.php b/blocks/myoverview/classes/output/main.php index 2435f54c4f7..28506375fb5 100644 --- a/blocks/myoverview/classes/output/main.php +++ b/blocks/myoverview/classes/output/main.php @@ -63,7 +63,7 @@ class main implements renderable, templatable { public function export_for_template(renderer_base $output) { global $USER; - $courses = enrol_get_my_courses('*', 'fullname ASC'); + $courses = enrol_get_my_courses('*'); $coursesprogress = []; foreach ($courses as $course) { diff --git a/calendar/externallib.php b/calendar/externallib.php index 049f13d7f35..7592536b90d 100644 --- a/calendar/externallib.php +++ b/calendar/externallib.php @@ -503,7 +503,7 @@ class core_calendar_external extends external_api { $params['aftereventid'] = null; } - $courses = enrol_get_my_courses('*', 'visible DESC,sortorder ASC', 0, [$courseid]); + $courses = enrol_get_my_courses('*', null, 0, [$courseid]); $courses = array_values($courses); if (empty($courses)) { @@ -588,7 +588,7 @@ class core_calendar_external extends external_api { } $renderer = $PAGE->get_renderer('core_calendar'); - $courses = enrol_get_my_courses('*', 'visible DESC,sortorder ASC', 0, $params['courseids']); + $courses = enrol_get_my_courses('*', null, 0, $params['courseids']); $courses = array_values($courses); if (empty($courses)) { diff --git a/course/renderer.php b/course/renderer.php index 64b2575d4a8..d75983726bc 100644 --- a/course/renderer.php +++ b/course/renderer.php @@ -1988,13 +1988,7 @@ class core_course_renderer extends plugin_renderer_base { } $output = ''; - if (!empty($CFG->navsortmycoursessort)) { - // sort courses the same as in navigation menu - $sortorder = 'visible DESC,'. $CFG->navsortmycoursessort.' ASC'; - } else { - $sortorder = 'visible DESC,sortorder ASC'; - } - $courses = enrol_get_my_courses('summary, summaryformat', $sortorder); + $courses = enrol_get_my_courses('summary, summaryformat'); $rhosts = array(); $rcourses = array(); if (!empty($CFG->mnet_dispatcher_mode) && $CFG->mnet_dispatcher_mode==='strict') { diff --git a/lib/myprofilelib.php b/lib/myprofilelib.php index c873280bc2b..5ce910486a6 100644 --- a/lib/myprofilelib.php +++ b/lib/myprofilelib.php @@ -225,7 +225,7 @@ function core_myprofile_navigation(core_user\output\myprofile\tree $tree, $user, if (!isset($hiddenfields['mycourses'])) { $showallcourses = optional_param('showallcourses', 0, PARAM_INT); - if ($mycourses = enrol_get_all_users_courses($user->id, true, null, 'visible DESC, sortorder ASC')) { + if ($mycourses = enrol_get_all_users_courses($user->id, true, null)) { $shown = 0; $courselisting = html_writer::start_tag('ul'); foreach ($mycourses as $mycourse) { diff --git a/lib/navigationlib.php b/lib/navigationlib.php index 9f22f184cea..16d7b63339b 100644 --- a/lib/navigationlib.php +++ b/lib/navigationlib.php @@ -2920,14 +2920,7 @@ class global_navigation extends navigation_node { $limit = (int) $CFG->navcourselimit; - $sortorder = 'visible DESC'; - // Prevent undefined $CFG->navsortmycoursessort errors. - if (empty($CFG->navsortmycoursessort)) { - $CFG->navsortmycoursessort = 'sortorder'; - } - // Append the chosen sortorder. - $sortorder = $sortorder . ',' . $CFG->navsortmycoursessort . ' ASC'; - $courses = enrol_get_my_courses('*', $sortorder); + $courses = enrol_get_my_courses('*'); $flatnavcourses = []; // Go through the courses and see which ones we want to display in the flatnav. From 9cefe2f2b2c50c0daee11ccf8c5e3f7b27004de3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 5 Dec 2017 21:59:04 +0100 Subject: [PATCH 5/5] MDL-57564 enrol: Fix typos and copy&paste mistake in the exception msg --- lib/enrollib.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/enrollib.php b/lib/enrollib.php index d72133aea55..1a73b2df51e 100644 --- a/lib/enrollib.php +++ b/lib/enrollib.php @@ -590,7 +590,7 @@ function enrol_get_my_courses($fields = null, $sort = null, $limit = 0, $coursei } else if (is_array($fields)) { $fields = array_unique(array_merge($basefields, $fields)); } else { - throw new coding_exception('Invalid $fileds parameter in enrol_get_my_courses()'); + throw new coding_exception('Invalid $fields parameter in enrol_get_my_courses()'); } if (in_array('*', $fields)) { $fields = array('*'); @@ -926,7 +926,7 @@ function enrol_get_all_users_courses($userid, $onlyactive = false, $fields = nul } else if (is_array($fields)) { $fields = array_unique(array_merge($basefields, $fields)); } else { - throw new coding_exception('Invalid $fileds parameter in enrol_get_my_courses()'); + throw new coding_exception('Invalid $fields parameter in enrol_get_all_users_courses()'); } if (in_array('*', $fields)) { $fields = array('*');