From 143aacaea0fb3e73c74f7d2c92f1f55679912426 Mon Sep 17 00:00:00 2001 From: Adrian Greeve Date: Mon, 8 Sep 2014 14:14:17 +0800 Subject: [PATCH 1/4] MDL-46921 lib: Add sorting on additional name fields A thank you to Yishe Choden for creating a patch for this issue. --- lib/tablelib.php | 7 ++++--- user/index.php | 6 ++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/tablelib.php b/lib/tablelib.php index d2ec1e38642..426731560b0 100644 --- a/lib/tablelib.php +++ b/lib/tablelib.php @@ -448,7 +448,8 @@ class flexible_table { if (($sortcol = optional_param($this->request[TABLE_VAR_SORT], '', PARAM_ALPHANUMEXT)) && $this->is_sortable($sortcol) && empty($this->sess->collapse[$sortcol]) && - (isset($this->columns[$sortcol]) || in_array($sortcol, array('firstname', 'lastname')) && isset($this->columns['fullname']))) { + (isset($this->columns[$sortcol]) || in_array($sortcol, get_all_user_name_fields()) + && isset($this->columns['fullname']))) { if (array_key_exists($sortcol, $this->sess->sortby)) { // This key already exists somewhere. Change its sortorder and bring it to the top. @@ -571,7 +572,7 @@ class flexible_table { if (isset($this->columns[$column])) { continue; // This column is OK. } - if (in_array($column, array('firstname', 'lastname')) && + if (in_array($column, get_all_user_name_fields()) && isset($this->columns['fullname'])) { continue; // This column is OK. } @@ -1186,7 +1187,7 @@ class flexible_table { if ($nameformat == 'language') { $nameformat = get_string('fullnamedisplay'); } - $requirednames = order_in_string(array('firstname', 'lastname'), $nameformat); + $requirednames = order_in_string(get_all_user_name_fields(), $nameformat); if (!empty($requirednames)) { if ($this->is_sortable($column)) { diff --git a/user/index.php b/user/index.php index e8875bd16b1..5db6f95ff14 100644 --- a/user/index.php +++ b/user/index.php @@ -410,11 +410,9 @@ list($esql, $params) = get_enrolled_sql($context, null, $currentgroup, true); $joins = array("FROM {user} u"); $wheres = array(); -$extrasql = get_extra_user_fields_sql($context, 'u', '', array( - 'id', 'username', 'firstname', 'lastname', 'email', 'city', 'country', - 'picture', 'lang', 'timezone', 'maildisplay', 'imagealt', 'lastaccess')); - $mainuserfields = user_picture::fields('u', array('username', 'email', 'city', 'country', 'lang', 'timezone', 'maildisplay')); +$alreadyretrievedfields = explode(',', $mainuserfields); +$extrasql = get_extra_user_fields_sql($context, 'u', '', $alreadyretrievedfields); if ($isfrontpage) { $select = "SELECT $mainuserfields, u.lastaccess$extrasql"; From e622b1a14b696ad0894b228f72c122ccf412e991 Mon Sep 17 00:00:00 2001 From: Adrian Greeve Date: Tue, 9 Sep 2014 14:52:01 +0800 Subject: [PATCH 2/4] MDL-46921 lib: Update get_all_user_name_fields() plus unit tests. --- lib/moodlelib.php | 18 +++++++++++++++++- lib/tests/moodlelib_test.php | 15 +++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index e44954f90c0..360116fa475 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -3672,9 +3672,12 @@ function fullname($user, $override=false) { * @param string $tableprefix table query prefix to use in front of each field. * @param string $prefix prefix added to the name fields e.g. authorfirstname. * @param string $fieldprefix sql field prefix e.g. id AS userid. + * @param bool $order moves firstname and lastname to the top of the array / start of the string. * @return array|string All name fields. */ -function get_all_user_name_fields($returnsql = false, $tableprefix = null, $prefix = null, $fieldprefix = null) { +function get_all_user_name_fields($returnsql = false, $tableprefix = null, $prefix = null, $fieldprefix = null, $order = false) { + // This array is provided in this order because when called by fullname() (above) if firstname is before + // firstnamephonetic str_replace() will change the wrong placeholder. $alternatenames = array('firstnamephonetic' => 'firstnamephonetic', 'lastnamephonetic' => 'lastnamephonetic', 'middlename' => 'middlename', @@ -3689,6 +3692,19 @@ function get_all_user_name_fields($returnsql = false, $tableprefix = null, $pref } } + // If we want the end result to have firstname and lastname at the front / top of the result. + if ($order) { + // Move the last two elements (firstname, lastname) off the array and put them at the top. + for ($i = 0; $i < 2; $i++) { + // Get the last element. + $lastelement = end($alternatenames); + // Remove it from the array. + unset($alternatenames[$lastelement]); + // Put the element back on the top of the array. + $alternatenames = array_merge(array($lastelement => $lastelement), $alternatenames); + } + } + // Create an sql field snippet if requested. if ($returnsql) { if ($tableprefix) { diff --git a/lib/tests/moodlelib_test.php b/lib/tests/moodlelib_test.php index 3738cfc5350..40953cd7bec 100644 --- a/lib/tests/moodlelib_test.php +++ b/lib/tests/moodlelib_test.php @@ -2486,6 +2486,21 @@ class core_moodlelib_testcase extends advanced_testcase { // Additional name fields with an alias and a title - string. $teststring = 'u.firstnamephonetic AS authorfirstnamephonetic,u.lastnamephonetic AS authorlastnamephonetic,u.middlename AS authormiddlename,u.alternatename AS authoralternatename,u.firstname AS authorfirstname,u.lastname AS authorlastname'; $this->assertEquals($teststring, get_all_user_name_fields(true, 'u', null, 'author')); + + // Test the order parameter of the function. + // Returning an array. + $testarray = array('firstname' => 'firstname', + 'lastname' => 'lastname', + 'firstnamephonetic' => 'firstnamephonetic', + 'lastnamephonetic' => 'lastnamephonetic', + 'middlename' => 'middlename', + 'alternatename' => 'alternatename' + ); + $this->assertEquals($testarray, get_all_user_name_fields(false, null, null, null, true)); + + // Returning a string. + $teststring = 'firstname,lastname,firstnamephonetic,lastnamephonetic,middlename,alternatename'; + $this->assertEquals($teststring, get_all_user_name_fields(true, null, null, null, true)); } public function test_order_in_string() { From 95065006c6399bb6a61a2725d3a1372cbadbedc6 Mon Sep 17 00:00:00 2001 From: Adrian Greeve Date: Mon, 8 Sep 2014 16:21:53 +0800 Subject: [PATCH 3/4] MDL-46921 lib: Sorting on additional fields, admin, enrolment pages. --- admin/user.php | 45 +++++++++++++++++++++++++++------------------ enrol/locallib.php | 12 ++---------- enrol/renderer.php | 7 ++++--- enrol/users.php | 24 +++++++++++++++++++----- 4 files changed, 52 insertions(+), 36 deletions(-) diff --git a/admin/user.php b/admin/user.php index 184ada69bb0..88e37a48b94 100644 --- a/admin/user.php +++ b/admin/user.php @@ -46,6 +46,8 @@ $returnurl = new moodle_url('/admin/user.php', array('sort' => $sort, 'dir' => $dir, 'perpage' => $perpage, 'page'=>$page)); + // The $user variable is also used outside of these if statements. + $user = null; if ($confirmuser and confirm_sesskey()) { require_capability('moodle/user:update', $sitecontext); if (!$user = $DB->get_record('user', array('id'=>$confirmuser, 'mnethostid'=>$CFG->mnet_localhost_id))) { @@ -158,8 +160,9 @@ // Carry on with the user listing $context = context_system::instance(); $extracolumns = get_extra_user_fields($context); - $columns = array_merge(array('firstname', 'lastname'), $extracolumns, - array('city', 'country', 'lastaccess')); + // Get all user name fields as an array. + $allusernamefields = get_all_user_name_fields(false, null, null, null, true); + $columns = array_merge($allusernamefields, $extracolumns, array('city', 'country', 'lastaccess')); foreach ($columns as $column) { $string[$column] = get_user_field_name($column); @@ -183,22 +186,28 @@ $$column = "".$string[$column]."$columnicon"; } - $override = new stdClass(); - $override->firstname = 'firstname'; - $override->lastname = 'lastname'; - $fullnamelanguage = get_string('fullnamedisplay', '', $override); - if (($CFG->fullnamedisplay == 'firstname lastname') or - ($CFG->fullnamedisplay == 'firstname') or - ($CFG->fullnamedisplay == 'language' and $fullnamelanguage == 'firstname lastname' )) { - $fullnamedisplay = "$firstname / $lastname"; - if ($sort == "name") { // If sort has already been set to something else then ignore. - $sort = "firstname"; - } - } else { // ($CFG->fullnamedisplay == 'language' and $fullnamelanguage == 'lastname firstname'). - $fullnamedisplay = "$lastname / $firstname"; - if ($sort == "name") { // This should give the desired sorting based on fullnamedisplay. - $sort = "lastname"; - } + // We need to check that alternativefullnameformat is not set to '' or language. + // We don't need to check the fullnamedisplay setting here as the fullname function call further down has + // the override parameter set to true. + $fullnamesetting = $CFG->alternativefullnameformat; + // If we are using language or it is empty, then retrieve all of the user's names. + if ($fullnamesetting == 'language' || empty($fullnamesetting)) { + $fullnamesetting = implode(' ', $allusernamefields); + } + + // Order in string will ensure that the name columns are in the correct order. + $usernames = order_in_string($allusernamefields, $fullnamesetting); + $fullnamedisplay = array(); + foreach ($usernames as $name) { + // Use the link from $$column for sorting on the user's name. + $fullnamedisplay[] = ${$name}; + } + // All of the names are in one column. Put them into a string and separate them with a /. + $fullnamedisplay = implode(' / ', $fullnamedisplay); + // If $sort = name then it is the default for the setting and we should use the first name to sort by. + if ($sort == "name") { + // Use the first item in the array. + $sort = reset($usernames); } list($extrasql, $params) = $ufiltering->get_sql_filter(); diff --git a/enrol/locallib.php b/enrol/locallib.php index 5e2a928f02d..433f6851ad6 100644 --- a/enrol/locallib.php +++ b/enrol/locallib.php @@ -243,16 +243,8 @@ class course_enrolment_manager { JOIN {enrol} e ON (e.id = ue.enrolid) LEFT JOIN {user_lastaccess} ul ON (ul.courseid = e.courseid AND ul.userid = u.id) LEFT JOIN {groups_members} gm ON u.id = gm.userid - WHERE $filtersql"; - if ($sort === 'firstname') { - $sql .= " ORDER BY u.firstname $direction, u.lastname $direction"; - } else if ($sort === 'lastname') { - $sql .= " ORDER BY u.lastname $direction, u.firstname $direction"; - } else if ($sort === 'email') { - $sql .= " ORDER BY u.email $direction, u.lastname $direction, u.firstname $direction"; - } else if ($sort === 'lastseen') { - $sql .= " ORDER BY ul.timeaccess $direction, u.lastname $direction, u.firstname $direction"; - } + WHERE $filtersql + ORDER BY u.$sort $direction"; $this->users[$key] = $DB->get_records_sql($sql, $params, $page*$perpage, $perpage); } return $this->users[$key]; diff --git a/enrol/renderer.php b/enrol/renderer.php index e957b8a91b1..22401992c06 100644 --- a/enrol/renderer.php +++ b/enrol/renderer.php @@ -420,8 +420,8 @@ class course_enrolment_table extends html_table implements renderable { * @static * @var array */ - protected static $sortablefields = array('firstname', 'lastname', 'idnumber', 'email', - 'phone1', 'phone2', 'institution', 'department' ); + protected static $sortablefields = array('firstname', 'lastname', 'firstnamephonetic', 'lastnamephonetic', 'middlename', + 'alternatename', 'idnumber', 'email', 'phone1', 'phone2', 'institution', 'department' ); /** * Constructs the table @@ -709,7 +709,8 @@ class course_enrolment_users_table extends course_enrolment_table { * @static * @var array */ - protected static $sortablefields = array('firstname', 'lastname', 'email', 'lastaccess'); + protected static $sortablefields = array('firstname', 'lastname', 'firstnamephonetic', 'lastnamephonetic', 'middlename', + 'alternatename', 'email', 'lastaccess'); } /** diff --git a/enrol/users.php b/enrol/users.php index 49421ef0d50..fb585b446a3 100644 --- a/enrol/users.php +++ b/enrol/users.php @@ -174,11 +174,25 @@ if ($action) { $renderer = $PAGE->get_renderer('core_enrol'); -$userdetails = array ( - 'picture' => false, - 'firstname' => get_string('firstname'), - 'lastname' => get_string('lastname'), -); +$userdetails = array('picture' => false); +// Get all the user names in a reasonable default order. +$allusernames = get_all_user_name_fields(false, null, null, null, true); +// Initialise the variable for the user's names in the table header. +$usernameheader = null; +// Get the alternative full name format for users with the viewfullnames capability. +$fullusernames = $CFG->alternativefullnameformat; +// If fullusernames is empty or accidentally set to language then fall back on the $allusernames set up. +if ($fullusernames == 'language' || empty($fullusernames)) { + $usernameheader = $allusernames; +} else { + // If everything is as expected then put them in the order specified by the alternative full name format setting. + $usernameheader = order_in_string($allusernames, $fullusernames); +} + +// Loop through each name and return the language string. +foreach ($usernameheader as $key => $username) { + $userdetails[$username] = get_string($username); +} $extrafields = get_extra_user_fields($context); foreach ($extrafields as $field) { $userdetails[$field] = get_user_field_name($field); From 28127d88970c9186efd6211c639cf34c9e081e06 Mon Sep 17 00:00:00 2001 From: Adrian Greeve Date: Mon, 29 Sep 2014 14:48:45 +0800 Subject: [PATCH 4/4] MDL-46921 lib: behat tests for alternate name sorting. --- user/tests/behat/table_sorting.feature | 55 ++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 user/tests/behat/table_sorting.feature diff --git a/user/tests/behat/table_sorting.feature b/user/tests/behat/table_sorting.feature new file mode 100644 index 00000000000..f6cb5cdf350 --- /dev/null +++ b/user/tests/behat/table_sorting.feature @@ -0,0 +1,55 @@ +@core +Feature: Tables can be sorted by additional names + In order to sort fields by additional names + As a user + I need to browse to a page with users in a table. + + Background: + Given the following "users" exist: + | username | firstname | lastname | middlename | alternatename | email | idnumber | + | student1 | Annie | Edison | Faith | Anne | student1@mail.com | s1 | + | student2 | George | Bradley | David | gman | student2@mail.com | s2 | + | student3 | Travis | Sutcliff | Peter | Mr T | student3@mail.com | s3 | + And I log in as "admin" + And I navigate to "User policies" node in "Site administration > Users > Permissions" + And I set the following administration settings values: + | Full name format | firstname middlename lastname | + | Alternative full name format | firstname middlename alternatename lastname | + + @javascript + Scenario: All user names are show and sortable in the administration user list. + Given I navigate to "Browse list of users" node in "Site administration > Users > Accounts" + # And I should see "First name / Middle name / Alternate name / Surname" + Then the following should exist in the "users" table: + | First name / Middle name / Alternate name / Surname | Email address | + | Admin User | moodle@moodlemoodle.com | + | Annie Faith Anne Edison | student1@mail.com | + | George David gman Bradley | student2@mail.com | + | Travis Peter Mr T Sutcliff | student3@mail.com | + And "Admin User" "table_row" should appear before "Annie Faith Anne Edison" "table_row" + And "Annie Faith Anne Edison" "table_row" should appear before "George David gman Bradley" "table_row" + And "George David gman Bradley" "table_row" should appear before "Travis Peter Mr T Sutcliff" "table_row" + And I follow "Middle name" + And "George David gman Bradley" "table_row" should appear before "Annie Faith Anne Edison" "table_row" + And "Annie Faith Anne Edison" "table_row" should appear before "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear before "Admin User" "table_row" + And I follow "Middle name" + And "George David gman Bradley" "table_row" should appear after "Annie Faith Anne Edison" "table_row" + And "Annie Faith Anne Edison" "table_row" should appear after "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear after "Admin User" "table_row" + And I follow "Alternate name" + And "Annie Faith Anne Edison" "table_row" should appear before "George David gman Bradley" "table_row" + And "George David gman Bradley" "table_row" should appear before "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear before "Admin User" "table_row" + And I follow "Alternate name" + And "Annie Faith Anne Edison" "table_row" should appear after "George David gman Bradley" "table_row" + And "George David gman Bradley" "table_row" should appear after "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear after "Admin User" "table_row" + And I follow "Surname" + And "George David gman Bradley" "table_row" should appear before "Annie Faith Anne Edison" "table_row" + And "Annie Faith Anne Edison" "table_row" should appear before "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear before "Admin User" "table_row" + And I follow "Surname" + And "George David gman Bradley" "table_row" should appear after "Annie Faith Anne Edison" "table_row" + And "Annie Faith Anne Edison" "table_row" should appear after "Travis Peter Mr T Sutcliff" "table_row" + And "Travis Peter Mr T Sutcliff" "table_row" should appear after "Admin User" "table_row"