From bb1105ae31e317e818e6a3609a07bc8e574749d6 Mon Sep 17 00:00:00 2001 From: Jerome Mouneyrac Date: Wed, 6 Feb 2013 17:18:09 +0800 Subject: [PATCH] MDL-29938 Fix typo bypassing some code + remove misplaced code in deprecated function + some code cleaning --- user/externallib.php | 96 +++++++++++++++------------------ user/tests/externallib_test.php | 33 ++++-------- 2 files changed, 54 insertions(+), 75 deletions(-) diff --git a/user/externallib.php b/user/externallib.php index 84f1276b341..08d46ae3be3 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -438,7 +438,6 @@ class core_user_external extends external_api { // Finally retrieve each users information $returnedusers = array(); foreach ($users as $user) { - $userdetails = user_get_user_details_courses($user); // Return the user only if the searched field is returned @@ -463,10 +462,10 @@ class core_user_external extends external_api { /** - * Returns description of get_users() parameters + * Returns description of get_users() parameters. * * @return external_function_parameters - * @since Moodle 2.4 + * @since Moodle 2.5 */ public static function get_users_parameters() { return new external_function_parameters( @@ -474,18 +473,18 @@ class core_user_external extends external_api { 'criteria' => new external_multiple_structure( new external_single_structure( array( - 'key' => new external_value(PARAM_ALPHA, 'the user column to search, expected keys (value format) are: - "id" (int) matching user id, - "lastname" (string) user last name (Note: you can use % for searching but it can be slow!), - "firstname" (string) user first name (Note: you can use % for searching but it can be slow!), - "idnumber" (string) matching user idnumber, - "username" (string) matching user username, - "email" (string) user email (Note: you can use % for searching but it can be slow!), - "auth" (plugin) matching user auth plugin'), - 'value' => new external_value(PARAM_RAW, 'the value to search') + 'key' => new external_value(PARAM_ALPHA, 'the user column to search, expected keys (value format) are: + "id" (int) matching user id, + "lastname" (string) user last name (Note: you can use % for searching but it may be considerably slower!), + "firstname" (string) user first name (Note: you can use % for searching but it may be considerably slower!), + "idnumber" (string) matching user idnumber, + "username" (string) matching user username, + "email" (string) user email (Note: you can use % for searching but it may be considerably slower!), + "auth" (string) matching user auth plugin'), + 'value' => new external_value(PARAM_RAW, 'the value to search') ) ), 'the key/value pairs to be considered in user search. Values can not be empty. - Specifiy different keys only once (fullname => \'user1\', auth => \'manual\', ...) - + Specify different keys only once (fullname => \'user1\', auth => \'manual\', ...) - key occurences are ignored, only the last occurence is considered. The search is executed with AND operator on the criterias.' ) @@ -494,12 +493,11 @@ class core_user_external extends external_api { } /** - * Retrieve matching user + * Retrieve matching user. * - * @param string $field - * @param array $values - * @return array An array of arrays containg user profiles. - * @since Moodle 2.4 + * @param array $criteria the allowed array keys are id/lastname/firstname/idnumber/username/email/auth. + * @return array An array of arrays containing user profiles. + * @since Moodle 2.5 */ public static function get_users($criteria = array()) { global $CFG, $USER, $DB; @@ -509,8 +507,7 @@ class core_user_external extends external_api { $params = self::validate_parameters(self::get_users_parameters(), array('criteria' => $criteria)); - // Validate the criteria and retrieve the users - $cleanedvalues = array(); + // Validate the criteria and retrieve the users. $firstcriteria = true; $users = array(); $warnings = array(); @@ -518,8 +515,7 @@ class core_user_external extends external_api { $sqlparams = array(); foreach ($params['criteria'] as $criteria) { - - // Clean the parameters + // Clean the parameters. $paramtype = PARAM_RAW; switch ($criteria['key']) { case 'id': @@ -532,7 +528,7 @@ class core_user_external extends external_api { $paramtype = PARAM_RAW; break; case 'email': - // we use PARAM_RAW to allow searches with % + // We use PARAM_RAW to allow searches with %. $paramtype = PARAM_RAW; break; case 'auth': @@ -543,25 +539,25 @@ class core_user_external extends external_api { $paramtype = PARAM_TEXT; break; default: - // Send back a warning that this search key is not supported in this version - // This warning will make the function extandable without breaking clients + // Send back a warning that this search key is not supported in this version. + // This warning will make the function extandable without breaking clients. $warnings[] = array( 'item' => 'key', 'itemid' => $criteria['key'], 'warningcode' => 'invalidfieldparameter', - 'message' => 'The search key \'' . $$criteria['key'] . '\' is not supported, look at the web service documentation' + 'message' => 'The search key \'' . $criteria['key'] . '\' is not supported, look at the web service documentation' ); } $cleanedvalue = clean_param($criteria['value'], $paramtype); - // If first criteria do not add AND to the query + // If first criteria do not add AND to the query. if ($firstcriteria) { $firstcriteria = false; } else { $sql .= ' AND '; } - // Create the SQL + // Create the SQL. switch ($criteria['key']) { case 'id': case 'idnumber': @@ -573,7 +569,7 @@ class core_user_external extends external_api { case 'email': case 'lastname': case 'firstname': - $sql .= $DB->sql_like($criteria['key'], ':'.$criteria['key'], false); + $sql .= $DB->sql_like($criteria['key'], ':' . $criteria['key'], false); $sqlparams[$criteria['key']] = $cleanedvalue; break; default: @@ -583,10 +579,9 @@ class core_user_external extends external_api { $users = $DB->get_records_select('user', $sql, $sqlparams, 'id ASC'); - // Finally retrieve each users information + // Finally retrieve each users information. $returnedusers = array(); foreach ($users as $user) { - $userdetails = user_get_user_details_courses($user); // Return the user only if all the searched fields are returned. @@ -610,17 +605,17 @@ class core_user_external extends external_api { } /** - * Returns description of get_users result value + * Returns description of get_users result value. * * @return external_description - * @since Moodle 2.3 + * @since Moodle 2.5 */ public static function get_users_returns() { return new external_single_structure( array('users' => new external_multiple_structure( core_user_external::user_description() ), - 'warnings' => new external_warnings() + 'warnings' => new external_warnings('always set to \'key\'', 'faulty key name') ) ); } @@ -693,8 +688,6 @@ class core_user_external extends external_api { return $result; } - - /** * Returns description of method result value * @@ -702,10 +695,18 @@ class core_user_external extends external_api { * @since Moodle 2.2 */ public static function get_users_by_id_returns() { - return new external_multiple_structure( - core_user_external::user_description() - ); + $additionalfields = array ( + 'enrolledcourses' => new external_multiple_structure( + new external_single_structure( + array( + 'id' => new external_value(PARAM_INT, 'Id of the course'), + 'fullname' => new external_value(PARAM_RAW, 'Fullname of the course'), + 'shortname' => new external_value(PARAM_RAW, 'Shortname of the course') + ) + ), 'Courses where the user is enrolled - limited by which courses the user is able to see', VALUE_OPTIONAL)); + return new external_multiple_structure(core_user_external::user_description($additionalfields)); } + /** * Returns description of method parameters * @@ -829,10 +830,10 @@ class core_user_external extends external_api { /** * Create user return value description. * - * @param array $additionalfiels some additional field + * @param array $additionalfields some additional field * @return single_structure_description */ - public static function user_description($additionalfiels = array()) { + public static function user_description($additionalfields = array()) { $userfields = array( 'id' => new external_value(PARAM_INT, 'ID of the user'), 'username' => new external_value(PARAM_RAW, 'The username', VALUE_OPTIONAL), @@ -875,7 +876,7 @@ class core_user_external extends external_api { 'name' => new external_value(PARAM_RAW, 'The name of the custom field'), 'shortname' => new external_value(PARAM_RAW, 'The shortname of the custom field - to be able to build the field class in the code'), ) - ), 'User custom fields (also known as user profil fields)', VALUE_OPTIONAL), + ), 'User custom fields (also known as user profile fields)', VALUE_OPTIONAL), 'preferences' => new external_multiple_structure( new external_single_structure( array( @@ -1067,16 +1068,7 @@ class moodle_user_external extends external_api { * @see core_user_external::get_users_by_id_returns() */ public static function get_users_by_id_returns() { - $additionalfields = array ( - 'enrolledcourses' => new external_multiple_structure( - new external_single_structure( - array( - 'id' => new external_value(PARAM_INT, 'Id of the course'), - 'fullname' => new external_value(PARAM_RAW, 'Fullname of the course'), - 'shortname' => new external_value(PARAM_RAW, 'Shortname of the course') - ) - ), 'Courses where the user is enrolled - limited by which courses the user is able to see', VALUE_OPTIONAL)); - return core_user_external::get_users_by_id_returns($additionalfields); + return core_user_external::get_users_by_id_returns(); } /** * Returns description of method parameters diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index e0684acc48f..eed9fce10d8 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -42,6 +42,7 @@ class core_user_external_testcase extends externallib_advanced_testcase { $this->resetAfterTest(true); $course = self::getDataGenerator()->create_course(); + $user1 = array( 'username' => 'usernametest1', 'idnumber' => 'idnumbertest1', @@ -64,6 +65,7 @@ class core_user_external_testcase extends externallib_advanced_testcase { 'url' => 'http://moodle.org', 'country' => 'au' ); + $user1 = self::getDataGenerator()->create_user($user1); if (!empty($CFG->usetags)) { require_once($CFG->dirroot . '/user/editlib.php'); @@ -71,6 +73,7 @@ class core_user_external_testcase extends externallib_advanced_testcase { $user1->interests = array('Cinema', 'Tennis', 'Dance', 'Guitar', 'Cooking'); useredit_update_interests($user1, $user1->interests); } + $user2 = self::getDataGenerator()->create_user( array('username' => 'usernametest2', 'idnumber' => 'idnumbertest2')); @@ -82,18 +85,9 @@ class core_user_external_testcase extends externallib_advanced_testcase { $roleid = $this->assignUserCapability('moodle/user:viewdetails', $context->id); // Enrol the users in the course. - // We use the manual plugin. - $enrol = enrol_get_plugin('manual'); - $enrolinstances = enrol_get_instances($course->id, true); - foreach ($enrolinstances as $courseenrolinstance) { - if ($courseenrolinstance->enrol == "manual") { - $instance = $courseenrolinstance; - break; - } - } - $enrol->enrol_user($instance, $user1->id, $roleid); - $enrol->enrol_user($instance, $user2->id, $roleid); - $enrol->enrol_user($instance, $USER->id, $roleid); + $this->getDataGenerator()->enrol_user($user1->id, $course->id, $roleid); + $this->getDataGenerator()->enrol_user($user2->id, $course->id, $roleid); + $this->getDataGenerator()->enrol_user($USER->id, $course->id, $roleid); // call as admin and receive all possible fields. $this->setAdminUser(); @@ -105,6 +99,9 @@ class core_user_external_testcase extends externallib_advanced_testcase { // Call the external function. $result = core_user_external::get_users($searchparams); + // We need to execute the return values cleaning process to simulate the web service server + $result = external_api::clean_returnvalue(core_user_external::get_users_returns(), $result); + // Check we retrieve the good total number of enrolled users + no error on capability. $expectedreturnedusers = 1; $returnedusers = $result['users']; @@ -119,7 +116,7 @@ class core_user_external_testcase extends externallib_advanced_testcase { } $this->assertEquals($generateduser->firstname, $returneduser['firstname']); $this->assertEquals($generateduser->lastname, $returneduser['lastname']); - if ($generateduser->email != $USER->email) { //don't check the tmp modified $USER email + if ($generateduser->email != $USER->email) { // Don't check the tmp modified $USER email. $this->assertEquals($generateduser->email, $returneduser['email']); } if (!empty($generateduser->address)) { @@ -171,16 +168,6 @@ class core_user_external_testcase extends externallib_advanced_testcase { $this->assertEquals(implode(', ', $generateduser->interests), $returneduser['interests']); } } - - // Test that no result are returned for search by username if we are not admin - $this->setGuestUser(); - - // Call the external function. - $returnedusers = core_user_external::get_users_by_field('username', - array($USER->username, $user1->username, $user2->username)); - - // Only the own $USER username should be returned - $this->assertEquals(1, count($returnedusers)); } /**