diff --git a/grade/report/history/classes/helper.php b/grade/report/history/classes/helper.php index 1ff2d1eb33e..189cbf7b022 100644 --- a/grade/report/history/classes/helper.php +++ b/grade/report/history/classes/helper.php @@ -129,10 +129,13 @@ class helper { */ protected static function get_users_sql_and_params($context, $search = '', $count = false) { global $DB, $USER; - + $userfieldsapi = \core_user\fields::for_identity($context)->with_userpic()->including('username'); + $userfieldssql = $userfieldsapi->get_sql('u', true, '', '', false); // Fields we need from the user table. - // TODO Does not support custom user profile fields (MDL-70456). - $extrafields = \core_user\fields::get_identity_fields($context, false); + $extrafields = []; + foreach ($userfieldsapi->get_required_fields([\core_user\fields::PURPOSE_IDENTITY]) as $field) { + $extrafields[$field] = $userfieldssql->mappings[$field]; + } $params = array(); if (!empty($search)) { list($filtersql, $params) = users_search_sql($search, 'u', true, $extrafields); @@ -141,13 +144,12 @@ class helper { $filtersql = ''; } - $userfieldsapi = \core_user\fields::for_userpic()->including(...(array_merge($extrafields, ['username']))); - $ufields = $userfieldsapi->get_sql('u', false, '', '', false)->selects; + $userfieldjoinssql = $userfieldssql->joins; if ($count) { $select = "SELECT COUNT(DISTINCT u.id) "; $orderby = ""; } else { - $select = "SELECT DISTINCT $ufields "; + $select = "SELECT DISTINCT $userfieldssql->selects "; $orderby = " ORDER BY u.lastname ASC, u.firstname ASC"; } @@ -161,7 +163,6 @@ class helper { if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context)) { // Fetch the groups that the user can see. $groups = groups_get_all_groups($courseid, $USER->id, 0, 'g.id'); - // Add join condition to include users that only belong to the same group as the user. list($insql, $inparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED, 'gid', true, 0); $groupjoinsql = " JOIN {groups_members} gm ON gm.userid = u.id "; @@ -173,11 +174,12 @@ class helper { FROM {user} u JOIN {grade_grades_history} ggh ON u.id = ggh.userid JOIN {grade_items} gi ON gi.id = ggh.itemid + $userfieldjoinssql $groupjoinsql WHERE $filtersql gi.courseid = :courseid $groupwheresql"; $sql .= $orderby; $params['courseid'] = $courseid; - + $params = array_merge($userfieldssql->params, $params); return array($sql, $params); } diff --git a/grade/report/history/classes/output/tablelog.php b/grade/report/history/classes/output/tablelog.php index 663e29488f8..afacc119d35 100644 --- a/grade/report/history/classes/output/tablelog.php +++ b/grade/report/history/classes/output/tablelog.php @@ -140,8 +140,7 @@ class tablelog extends \table_sql implements \renderable { * Setup the headers for the html table. */ protected function define_table_columns() { - // TODO Does not support custom user profile fields (MDL-70456). - $extrafields = \core_user\fields::get_identity_fields($this->context, false); + $extrafields = \core_user\fields::get_identity_fields($this->context); // Define headers and columns. $cols = array( @@ -154,7 +153,7 @@ class tablelog extends \table_sql implements \renderable { if (get_string_manager()->string_exists($field, 'moodle')) { $cols[$field] = get_string($field); } else { - $cols[$field] = $field; + $cols[$field] = \core_user\fields::get_display_name($field); } } @@ -394,15 +393,24 @@ class tablelog extends \table_sql implements \renderable { ggh.source, ggh.overridden, ggh.locked, ggh.excluded, ggh.feedback, ggh.feedbackformat, gi.itemtype, gi.itemmodule, gi.iteminstance, gi.itemnumber, '; + $userfieldsapi = \core_user\fields::for_identity($this->context); + $userfieldssql = $userfieldsapi->get_sql('u', true, '', '', true); + $userfieldsselects = ''; + $userfieldsjoins = ''; + $userfieldsparams = []; + if (!$count) { + $userfieldsselects = $userfieldssql->selects; + $userfieldsjoins = $userfieldssql->joins; + $userfieldsparams = $userfieldssql->params; + } + // Add extra user fields that we need for the graded user. - // TODO Does not support custom user profile fields (MDL-70456). - $extrafields = \core_user\fields::get_identity_fields($this->context, false); - foreach ($extrafields as $field) { - $fields .= 'u.' . $field . ', '; + $extrafields = []; + foreach ($userfieldsapi->get_required_fields([\core_user\fields::PURPOSE_IDENTITY]) as $field) { + $extrafields[$field] = $userfieldssql->mappings[$field]; } $userfieldsapi = \core_user\fields::for_name(); - $gradeduserfields = $userfieldsapi->get_sql('u', false, '', '', false)->selects; - $fields .= $gradeduserfields . ', '; + $fields .= $userfieldsapi->get_sql('u', false, '', '', false)->selects . ', '; $groupby = $fields; // Add extra user fields that we need for the grader user. @@ -437,12 +445,14 @@ class tablelog extends \table_sql implements \renderable { list($where, $params) = $this->get_filters_sql_and_params(); - $sql = "SELECT $select + $sql = " SELECT $select $userfieldsselects FROM {grade_grades_history} ggh JOIN {grade_items} gi ON gi.id = ggh.itemid JOIN {user} u ON u.id = ggh.userid + $userfieldsjoins LEFT JOIN {user} ug ON ug.id = ggh.usermodified WHERE $where"; + $params = array_merge($userfieldsparams, $params); // As prevgrade is a dynamic field, we need to wrap the query. This is the only filtering // that should be defined outside the method self::get_filters_sql_and_params(). diff --git a/grade/report/history/tests/behat/basic_functionality.feature b/grade/report/history/tests/behat/basic_functionality.feature index bb5ecbc0b5c..25972ec3a9e 100644 --- a/grade/report/history/tests/behat/basic_functionality.feature +++ b/grade/report/history/tests/behat/basic_functionality.feature @@ -9,12 +9,15 @@ Feature: A teacher checks the grade history report in a course Given the following "courses" exist: | fullname | shortname | category | groupmode | | Course 1 | C1 | 0 | 1 | + And the following "custom profile fields" exist: + | datatype | shortname | name | + | text | food | Favourite food | And the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Teacher | 1 | teacher1@example.com | - | teacher2 | Teacher | 2 | teacher2@example.com | - | student1 | Student | 1 | student1@example.com | - | student2 | Student | 2 | student2@example.com | + | username | firstname | lastname | email | profile_field_food | + | teacher1 | Teacher | 1 | teacher1@example.com | | + | teacher2 | Teacher | 2 | teacher2@example.com | | + | student1 | Student | 1 | student1@example.com | apple | + | student2 | Student | 2 | student2@example.com | orange | And the following "course enrolments" exist: | user | course | role | | teacher1 | C1 | editingteacher | @@ -25,9 +28,13 @@ Feature: A teacher checks the grade history report in a course | activity | course | section | name | intro | | assign | C1 | 1 | The greatest assignment ever | Write a behat test for Moodle - it's amazing | | assign | C1 | 1 | Rewarding assignment | After writing your behat test go grab a beer! | + Given the following config values are set as admin: + | showuseridentity | email,profile_field_food | And I log in as "teacher1" And I am on "Course 1" course homepage with editing mode on And I navigate to "View > Grader report" in the course gradebook + And I should see "apple" in the "student1" "table_row" + And I should see "orange" in the "student2" "table_row" And I turn editing mode on And I give the grade "50.00" to the user "Student 1" for the grade item "The greatest assignment ever" And I give the grade "60.00" to the user "Student 1" for the grade item "Rewarding assignment" @@ -38,6 +45,8 @@ Feature: A teacher checks the grade history report in a course And I log in as "teacher2" And I am on "Course 1" course homepage And I navigate to "View > Grader report" in the course gradebook + And I should see "apple" in the "student1" "table_row" + And I should see "orange" in the "student2" "table_row" And I turn editing mode on And I give the grade "70.00" to the user "Student 1" for the grade item "The greatest assignment ever" And I give the grade "80.00" to the user "Student 1" for the grade item "Rewarding assignment" @@ -47,15 +56,15 @@ Feature: A teacher checks the grade history report in a course And I follow "Grade history" When I press "Submit" Then the following should exist in the "gradereport_history" table: - | First name/Surname | Grade item | Original grade | Revised grade | Grader | - | Student 1 | The greatest assignment ever | | 50.00 | Teacher 1 | - | Student 1 | Rewarding assignment | | 60.00 | Teacher 1 | - | Student 2 | The greatest assignment ever | | 50.00 | Teacher 1 | - | Student 2 | Rewarding assignment | | 60.00 | Teacher 1 | - | Student 1 | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | - | Student 1 | Rewarding assignment | 60.00 | 80.00 | Teacher 2 | - | Student 2 | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | - | Student 2 | Rewarding assignment | 60.00 | 80.00 | Teacher 2 | + | First name/Surname | Email address | Favourite food | Grade item | Original grade | Revised grade | Grader | + | Student 1 | student1@example.com | apple | The greatest assignment ever | | 50.00 | Teacher 1 | + | Student 1 | student1@example.com | apple | Rewarding assignment | | 60.00 | Teacher 1 | + | Student 2 | student2@example.com | orange | The greatest assignment ever | | 50.00 | Teacher 1 | + | Student 2 | student2@example.com | orange | Rewarding assignment | | 60.00 | Teacher 1 | + | Student 1 | student1@example.com | apple | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | + | Student 1 | student1@example.com | apple | Rewarding assignment | 60.00 | 80.00 | Teacher 2 | + | Student 2 | student2@example.com | orange | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | + | Student 2 | student2@example.com | orange | Rewarding assignment | 60.00 | 80.00 | Teacher 2 | # Test filtering by student. And I press "Select users" And I set the field with xpath "//form/input[@class='usp-search-field']" to "Student 1" @@ -69,6 +78,9 @@ Feature: A teacher checks the grade history report in a course | Student 1 | Rewarding assignment | | 60.00 | Teacher 1 | | Student 1 | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | | Student 1 | Rewarding assignment | 60.00 | 80.00 | Teacher 2 | + # Test for seeing custom fields contents in the rows. + And I should see "apple" in the "student1" "table_row" + And I should not see "orange" And the following should not exist in the "gradereport_history" table: | Student 2 | The greatest assignment ever | | 50.00 | Teacher 1 | | Student 2 | Rewarding assignment | | 60.00 | Teacher 1 | @@ -88,13 +100,13 @@ Feature: A teacher checks the grade history report in a course And I set the field "Grader" to "Teacher 1" And I press "Submit" And the following should exist in the "gradereport_history" table: - | First name/Surname | Grade item | Original grade | Revised grade | Grader | - | Student 1 | The greatest assignment ever | | 50.00 | Teacher 1 | + | First name/Surname | Email address | Favourite food | Grade item | Original grade | Revised grade | Grader | + | Student 1 | student1@example.com | apple | The greatest assignment ever | | 50.00 | Teacher 1 | And the following should not exist in the "gradereport_history" table: | Student 1 | The greatest assignment ever | 50.00 | 70.00 | Teacher 2 | # Test filtering by revised grades. And I click on "id_revisedonly" "checkbox" And I press "Submit" And the following should exist in the "gradereport_history" table: - | First name/Surname | Grade item | Original grade | Revised grade | Grader | - | Student 1 | The greatest assignment ever | | 50.00 | Teacher 1 | + | First name/Surname | Email address | Favourite food | Grade item | Original grade | Revised grade | Grader | + | Student 1 | student1@example.com | apple | The greatest assignment ever | | 50.00 | Teacher 1 | diff --git a/grade/report/history/tests/report_test.php b/grade/report/history/tests/report_test.php index 7a58ae1363e..24e407cbb39 100644 --- a/grade/report/history/tests/report_test.php +++ b/grade/report/history/tests/report_test.php @@ -241,6 +241,159 @@ class gradereport_history_report_testcase extends advanced_testcase { $this->assertEquals(1, \gradereport_history\helper::get_users_count($c1ctx, 'c')); } + /** + * Data provider for \gradereport_history_report_testcase::test_get_users_with_profile_fields() + * Testing get_users() and get_users_count() test cases. + * + * @return array List of data sets (test cases) + */ + public function get_users_with_profile_fields_provider(): array { + return [ + // User identity check boxes, 'email', 'profile_field_lang' and 'profile_field_height' are checked. + 'show email,lang and height;search for all users' => + ['email,profile_field_lang,profile_field_height', '', ['u1', 'u2', 'u3', 'u4']], + 'show email,lang and height;search for users on .org ' => + ['email,profile_field_lang,profile_field_height', '.org', ['u1', 'u2', 'u4']], + 'show email,lang and height;search for users on .com ' => + ['email,profile_field_lang,profile_field_height', '.com', []], + 'show email,lang and height;search for users on .uk ' => + ['email,profile_field_lang,profile_field_height', '.uk', ['u3']], + 'show email,lang and height,search for Spanish speakers' => + ['email,profile_field_lang,profile_field_height', 'spanish', ['u1', 'u4']], + 'show email,lang and height,search for Spanish speakers' => + ['email,profile_field_lang,profile_field_height', 'spa', ['u1', 'u4']], + 'show email,lang and height,search for German speakers' => + ['email,profile_field_lang,profile_field_height', 'german', ['u2']], + 'show email,lang and height,search for German speakers' => + ['email,profile_field_lang,profile_field_height', 'ger', ['u2']], + 'show email,lang and height,search for English speakers' => + ['email,profile_field_lang,profile_field_height', 'english', ['u3']], + 'show email,lang and height,search for English speakers' => + ['email,profile_field_lang,profile_field_height', 'eng', ['u3']], + 'show email,lang and height,search for English speakers' => + ['email,profile_field_lang,profile_field_height', 'ish', ['u3']], + 'show email,lang and height,search for users with height 180cm' => + ['email,profile_field_lang,profile_field_height', '180cm', ['u2', 'u3', 'u4']], + 'show email,lang and height,search for users with height 180cm' => + ['email,profile_field_lang,profile_field_height', '180', ['u2', 'u3', 'u4']], + 'show email,lang and height,search for users with height 170cm' => + ['email,profile_field_lang,profile_field_height', '170cm', ['u1']], + 'show email,lang and height,search for users with height 170cm' => + ['email,profile_field_lang,profile_field_height', '170', ['u1']], + + // User identity check boxes, 'email' and 'profile_field_height' are checked. + 'show email and height;search for users on .org' => + ['email,profile_field_height', '.org', ['u1', 'u2', 'u4']], + 'show email and height;search for users on .com' => + ['email,profile_field_height', '.com', []], + 'show email and height;search for users on .co' => + ['email,profile_field_height', '.co', ['u3']], + 'show email and height,search for Spanish/German/English speakers' => + ['email,profile_field_height', 'spanish', []], + 'show email and height,search for Spanish/German/English speakers' => + ['email,profile_field_height', 'german', []], + 'show email and height,search for Spanish/German/English speakers' => + ['email,profile_field_height', 'english', []], + 'show email,lang and height,search for English speakers' => + ['email,profile_field_height', 'english', []], + 'show email and height,search for English speakers' => + ['email,profile_field_height', 'eng', []], + 'show email and height,search for English speakers' => + ['email,profile_field_height', 'ish', []], + 'show email and height,search for users with height 180cm' => + ['email,profile_field_height', '180cm', ['u2', 'u3', 'u4']], + 'show email,lang and height,search for users with height 180cm' => + ['email,profile_field_height', '180', ['u2', 'u3', 'u4']], + 'show email,lang and height,search for users with height 170cm' => + ['email,profile_field_height', '170cm', ['u1']], + 'show email,lang and height,search for users with height 170cm' => + ['email,profile_field_height', '170', ['u1']], + + // User identity check boxes, only 'email' is checked. + 'show email only;search for users on .org' => ['email', '.org', ['u1', 'u2', 'u4']], + 'show email only;search for users on .com' => ['email', '.com', []], + 'show email only;search for users on .co.uk' => ['email', 'co.uk', ['u3']], + 'show email only;search for users on .uk' => ['email', '.uk', ['u3']], + 'show email only;search for users on .co' => ['email', '.co', ['u3']], + 'show email only;search for Spanish speakers' => ['email', 'spanish', []], + 'show email only;search for German speakers' => ['email', 'german', []], + 'show email only;search for English speakers' => ['email', 'english', []], + 'show email only;search for users with height 180cm' => ['email', '180cm', []], + 'show email only;search for users with height 180cm' => ['email', '180', []], + 'show email only;search for users with height 170cm' => ['email', '170cm', []], + 'show email only;search for users with height 170cm' => ['email', '170', []], + ]; + } + + /** + * Testing the search functionality on get_users() and get_users_count() and their inner methods. + * + * @dataProvider get_users_with_profile_fields_provider + * + * @param string $showuseridentity, list of user identities to be shown. + * @param string $searchstring, the string to be searched. + * @param array $expectedusernames, a list of expected usernames. + * @return void + */ + public function test_get_users_with_profile_fields(string $showuseridentity, string $searchstring, + array $expectedusernames): void { + global $CFG, $DB; + require_once($CFG->dirroot . '/user/profile/lib.php'); + $this->resetAfterTest(); + + // Create a couple of custom profile fields, which are in user identity. + $generator = $this->getDataGenerator(); + $generator->create_custom_profile_field(['datatype' => 'text', + 'shortname' => 'lang', 'name' => 'Language']); + $generator->create_custom_profile_field(['datatype' => 'text', + 'shortname' => 'height', 'name' => 'Height']); + + // Create a couple of test users. + $u1 = $generator->create_user(['firstname' => 'Eduardo', 'lastname' => 'Gomes', + 'username' => 'u1', 'email' => 'u1@x.org', 'profile_field_lang' => 'Spanish', + 'profile_field_height' => '170cm']); + $u2 = $generator->create_user(['firstname' => 'Dieter', 'lastname' => 'Schmitt', + 'username' => 'u2', 'email' => 'u2@x.org', 'profile_field_lang' => 'German', + 'profile_field_height' => '180cm']); + + $u3 = $generator->create_user(['firstname' => 'Peter', 'lastname' => 'Jones', + 'username' => 'u3', 'email' => 'u3@x.co.uk', 'profile_field_lang' => 'English', + 'profile_field_height' => '180cm']); + $u4 = $generator->create_user(['firstname' => 'Pedro', 'lastname' => 'Gomes', + 'username' => 'u4', 'email' => 'u3@x.org', 'profile_field_lang' => 'Spanish', + 'profile_field_height' => '180cm']); + + // Do this as admin user. + $this->setAdminUser(); + + // Making the setup. + $c1 = $this->getDataGenerator()->create_course(); + $c1ctx = context_course::instance($c1->id); + $c1m1 = $this->getDataGenerator()->create_module('assign', array('course' => $c1)); + + // Creating grade history for some users. + $gi = grade_item::fetch(array('iteminstance' => $c1m1->id, 'itemtype' => 'mod', 'itemmodule' => 'assign')); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u1->id)); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u2->id)); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u3->id)); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u4->id)); + + // Checking fetching some users with this config settings. + set_config('showuseridentity', $showuseridentity); + $numberofexpectedusers = count($expectedusernames); + $users = \gradereport_history\helper::get_users($c1ctx, $searchstring); + $userscount = \gradereport_history\helper::get_users_count($c1ctx, $searchstring); + $this->assertEquals($numberofexpectedusers, $userscount); + $this->assertCount($numberofexpectedusers, $users); + foreach ($users as $user) { + if (in_array($user->username, $expectedusernames)) { + $this->assertArrayHasKey($user->id, $users); + } else { + $this->assertArrayNotHasKey($user->id, $users); + } + } + } + /** * Data provider method for \gradereport_history_report_testcase::test_get_users_with_groups() */ diff --git a/grade/report/history/users_ajax.php b/grade/report/history/users_ajax.php index 038cf0274d4..ede28aebc3b 100644 --- a/grade/report/history/users_ajax.php +++ b/grade/report/history/users_ajax.php @@ -51,8 +51,8 @@ $users = \gradereport_history\helper::get_users($context, $search, $page, 25); $outcome->response = array('users' => array()); $outcome->response['totalusers'] = \gradereport_history\helper::get_users_count($context, $search);; -// TODO Does not support custom user profile fields (MDL-70456). -$extrafields = \core_user\fields::get_identity_fields($context, false); +$userfieldsapi = \core_user\fields::for_identity($context)->with_userpic(); +$extrafields = $userfieldsapi->get_required_fields([\core_user\fields::PURPOSE_IDENTITY]); $useroptions = array('link' => false, 'visibletoscreenreaders' => false); // Format the user record. @@ -61,11 +61,11 @@ foreach ($users as $user) { $newuser->userid = $user->id; $newuser->picture = $OUTPUT->user_picture($user, $useroptions); $newuser->fullname = fullname($user); - $fieldvalues = array(); foreach ($extrafields as $field) { - $fieldvalues[] = s($user->{$field}); + if ($user->{$field}) { + $newuser->extrafields[] = $user->{$field}; + } } - $newuser->extrafields = implode(', ', $fieldvalues); $outcome->response['users'][] = $newuser; }