diff --git a/user/externallib.php b/user/externallib.php index 86381b25fea..0f402dfa9a0 100644 --- a/user/externallib.php +++ b/user/externallib.php @@ -564,106 +564,143 @@ class core_user_external extends external_api { 'maxfiles' => 1, 'accepted_types' => 'optimised_image'); - $transaction = $DB->start_delegated_transaction(); - + $warnings = array(); foreach ($params['users'] as $user) { - // First check the user exists. - if (!$existinguser = core_user::get_user($user['id'])) { - continue; - } - // Check if we are trying to update an admin. - if ($existinguser->id != $USER->id and is_siteadmin($existinguser) and !is_siteadmin($USER)) { - continue; - } - // Other checks (deleted, remote or guest users). - if ($existinguser->deleted or is_mnet_remote_user($existinguser) or isguestuser($existinguser->id)) { - continue; - } - // Check duplicated emails. - if (isset($user['email']) && $user['email'] !== $existinguser->email) { - if (!validate_email($user['email'])) { - continue; - } else if (empty($CFG->allowaccountssameemail)) { - // Make a case-insensitive query for the given email address and make sure to exclude the user being updated. - $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid AND id <> :userid'; - $params = array( - 'email' => $user['email'], - 'mnethostid' => $CFG->mnet_localhost_id, - 'userid' => $user['id'] - ); - // Skip if there are other user(s) that already have the same email. - if ($DB->record_exists_select('user', $select, $params)) { - continue; + // Catch any exception while updating a user and return it as a warning. + try { + $transaction = $DB->start_delegated_transaction(); + + // First check the user exists. + if (!$existinguser = core_user::get_user($user['id'])) { + throw new moodle_exception('invaliduserid', '', '', null, + 'Invalid user ID'); + } + // Check if we are trying to update an admin. + if ($existinguser->id != $USER->id and is_siteadmin($existinguser) and !is_siteadmin($USER)) { + throw new moodle_exception('usernotupdatedadmin', '', '', null, + 'Cannot update admin accounts'); + } + // Other checks (deleted, remote or guest users). + if ($existinguser->deleted) { + throw new moodle_exception('usernotupdateddeleted', '', '', null, + 'User is a deleted user'); + } + if (is_mnet_remote_user($existinguser)) { + throw new moodle_exception('usernotupdatedremote', '', '', null, + 'User is a remote user'); + } + if (isguestuser($existinguser->id)) { + throw new moodle_exception('usernotupdatedguest', '', '', null, + 'Cannot update guest account'); + } + // Check duplicated emails. + if (isset($user['email']) && $user['email'] !== $existinguser->email) { + if (!validate_email($user['email'])) { + throw new moodle_exception('useremailinvalid', '', '', null, + 'Invalid email address'); + } else if (empty($CFG->allowaccountssameemail)) { + // Make a case-insensitive query for the given email address + // and make sure to exclude the user being updated. + $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid AND id <> :userid'; + $params = array( + 'email' => $user['email'], + 'mnethostid' => $CFG->mnet_localhost_id, + 'userid' => $user['id'] + ); + // Skip if there are other user(s) that already have the same email. + if ($DB->record_exists_select('user', $select, $params)) { + throw new moodle_exception('useremailduplicate', '', '', null, + 'Duplicate email address'); + } } } - } - user_update_user($user, true, false); + user_update_user($user, true, false); - $userobject = (object)$user; + $userobject = (object)$user; - // Update user picture if it was specified for this user. - if (empty($CFG->disableuserimages) && isset($user['userpicture'])) { - $userobject->deletepicture = null; + // Update user picture if it was specified for this user. + if (empty($CFG->disableuserimages) && isset($user['userpicture'])) { + $userobject->deletepicture = null; - if ($user['userpicture'] == 0) { - $userobject->deletepicture = true; - } else { - $userobject->imagefile = $user['userpicture']; + if ($user['userpicture'] == 0) { + $userobject->deletepicture = true; + } else { + $userobject->imagefile = $user['userpicture']; + } + + core_user::update_picture($userobject, $filemanageroptions); } - core_user::update_picture($userobject, $filemanageroptions); - } - - // Update user interests. - if (!empty($user['interests'])) { - $trimmedinterests = array_map('trim', explode(',', $user['interests'])); - $interests = array_filter($trimmedinterests, function($value) { - return !empty($value); - }); - useredit_update_interests($userobject, $interests); - } - - // Update user custom fields. - if (!empty($user['customfields'])) { - - foreach ($user['customfields'] as $customfield) { - // Profile_save_data() saves profile file it's expecting a user with the correct id, - // and custom field to be named profile_field_"shortname". - $user["profile_field_".$customfield['type']] = $customfield['value']; + // Update user interests. + if (!empty($user['interests'])) { + $trimmedinterests = array_map('trim', explode(',', $user['interests'])); + $interests = array_filter($trimmedinterests, function($value) { + return !empty($value); + }); + useredit_update_interests($userobject, $interests); } - profile_save_data((object) $user); - } - // Trigger event. - \core\event\user_updated::create_from_userid($user['id'])->trigger(); + // Update user custom fields. + if (!empty($user['customfields'])) { - // Preferences. - if (!empty($user['preferences'])) { - $userpref = clone($existinguser); - foreach ($user['preferences'] as $preference) { - $userpref->{'preference_'.$preference['type']} = $preference['value']; + foreach ($user['customfields'] as $customfield) { + // Profile_save_data() saves profile file it's expecting a user with the correct id, + // and custom field to be named profile_field_"shortname". + $user["profile_field_".$customfield['type']] = $customfield['value']; + } + profile_save_data((object) $user); + } + + // Trigger event. + \core\event\user_updated::create_from_userid($user['id'])->trigger(); + + // Preferences. + if (!empty($user['preferences'])) { + $userpref = clone($existinguser); + foreach ($user['preferences'] as $preference) { + $userpref->{'preference_'.$preference['type']} = $preference['value']; + } + useredit_update_user_preference($userpref); + } + if (isset($user['suspended']) and $user['suspended']) { + \core\session\manager::kill_user_sessions($user['id']); + } + + $transaction->allow_commit(); + } catch (Exception $e) { + try { + $transaction->rollback($e); + } catch (Exception $e) { + $warning = []; + $warning['item'] = 'user'; + $warning['itemid'] = $user['id']; + if ($e instanceof moodle_exception) { + $warning['warningcode'] = $e->errorcode; + } else { + $warning['warningcode'] = $e->getCode(); + } + $warning['message'] = $e->getMessage(); + $warnings[] = $warning; } - useredit_update_user_preference($userpref); - } - if (isset($user['suspended']) and $user['suspended']) { - \core\session\manager::kill_user_sessions($user['id']); } } - $transaction->allow_commit(); - - return null; + return ['warnings' => $warnings]; } /** * Returns description of method result value * - * @return null + * @return external_description * @since Moodle 2.2 */ public static function update_users_returns() { - return null; + return new external_single_structure( + array( + 'warnings' => new external_warnings() + ) + ); } /** diff --git a/user/tests/externallib_test.php b/user/tests/externallib_test.php index f2a6176745c..fd9f5cb1693 100644 --- a/user/tests/externallib_test.php +++ b/user/tests/externallib_test.php @@ -715,6 +715,7 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { global $USER, $CFG, $DB; $this->resetAfterTest(true); + $this->preventResetByRollback(); $wsuser = self::getDataGenerator()->create_user(); self::setUser($wsuser); @@ -780,8 +781,20 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $user4['id'] = $userdeleted->id; user_delete_user($userdeleted); + $user5 = self::getDataGenerator()->create_user(); + $user5 = array('id' => $user5->id, 'email' => $user5->email); + // Call the external function. - core_user_external::update_users(array($user1, $user2, $user3, $user4)); + $returnvalue = core_user_external::update_users(array($user1, $user2, $user3, $user4)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + + // Check warnings. + $this->assertEquals($user2['id'], $returnvalue['warnings'][0]['itemid']); // Guest user. + $this->assertEquals('usernotupdatedguest', $returnvalue['warnings'][0]['warningcode']); + $this->assertEquals($user3['id'], $returnvalue['warnings'][1]['itemid']); // Admin user. + $this->assertEquals('usernotupdatedadmin', $returnvalue['warnings'][1]['warningcode']); + $this->assertEquals($user4['id'], $returnvalue['warnings'][2]['itemid']); // Deleted user. + $this->assertEquals('usernotupdateddeleted', $returnvalue['warnings'][2]['warningcode']); $dbuser2 = $DB->get_record('user', array('id' => $user2['id'])); $this->assertNotEquals($dbuser2->username, $user2['username']); @@ -824,6 +837,39 @@ class core_user_externallib_testcase extends externallib_advanced_testcase { $dbuserdelpic = $DB->get_record('user', array('id' => $user1['id'])); $this->assertEquals(0, $dbuserdelpic->picture, 'Picture must be deleted when sent as 0.'); + // Updating user with an invalid email. + $user5['email'] = 'bogus'; + $returnvalue = core_user_external::update_users(array($user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('useremailinvalid', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Invalid email address', + $returnvalue['warnings'][0]['message']); + + // Updating user with a duplicate email. + $user5['email'] = $user1['email']; + $returnvalue = core_user_external::update_users(array($user1, $user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('useremailduplicate', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Duplicate email address', + $returnvalue['warnings'][0]['message']); + + // Updating a user that does not exist. + $user5['id'] = -1; + $returnvalue = core_user_external::update_users(array($user5)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('invaliduserid', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('Invalid user ID', + $returnvalue['warnings'][0]['message']); + + // Updating a remote user. + $user1['mnethostid'] = 5; + user_update_user($user1); // Update user not using webservice. + unset($user1['mnethostid']); // The mnet host ID field is not in the allowed field list for the webservice. + $returnvalue = core_user_external::update_users(array($user1)); + $returnvalue = external_api::clean_returnvalue(core_user_external::update_users_returns(), $returnvalue); + $this->assertEquals('usernotupdatedremote', $returnvalue['warnings'][0]['warningcode']); + $this->assertStringContainsString('User is a remote user', + $returnvalue['warnings'][0]['message']); // Call without required capability. $this->unassignUserCapability('moodle/user:update', $context->id, $roleid); diff --git a/user/upgrade.txt b/user/upgrade.txt index d20af1afffd..51d0ee8f5de 100644 --- a/user/upgrade.txt +++ b/user/upgrade.txt @@ -1,5 +1,12 @@ This files describes API changes for code that uses the user API. +=== 4.0 === + +* External function core_user_external::update_users() will now fail on a per user basis. Previously if one user + update failed all users in the operation would fail. +* External function core_user_external::update_users() now returns an error code and message to why a user update + action failed. + === 3.11 === * Added new core_user/form_user_selector JS module that can be used as the 'ajax' handler for the autocomplete form