From b53134bf32404b0bc720fb2452ddcda18cb0a9dd Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Apr 2014 12:05:19 -0700 Subject: [PATCH] Move "Change Username" from weird edit panel to standard object action Summary: Ref T4065. Make this work in a more standard way which administrators have a reasonable shot at finding and using. See D8662 for discussion. Test Plan: Changed a user's username. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4065 Differential Revision: https://secure.phabricator.com/D8663 --- src/__phutil_library_map__.php | 2 + .../PhabricatorApplicationPeople.php | 1 + .../PhabricatorPeopleDeleteController.php | 2 +- .../PhabricatorPeopleEditController.php | 95 +-------------- .../PhabricatorPeopleProfileController.php | 7 ++ .../PhabricatorPeopleRenameController.php | 112 ++++++++++++++++++ 6 files changed, 124 insertions(+), 95 deletions(-) create mode 100644 src/applications/people/controller/PhabricatorPeopleRenameController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 54dbf2c57f..27c25fafa1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1818,6 +1818,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleProfileEditController' => 'applications/people/controller/PhabricatorPeopleProfileEditController.php', 'PhabricatorPeopleProfilePictureController' => 'applications/people/controller/PhabricatorPeopleProfilePictureController.php', 'PhabricatorPeopleQuery' => 'applications/people/query/PhabricatorPeopleQuery.php', + 'PhabricatorPeopleRenameController' => 'applications/people/controller/PhabricatorPeopleRenameController.php', 'PhabricatorPeopleSearchEngine' => 'applications/people/query/PhabricatorPeopleSearchEngine.php', 'PhabricatorPeopleTestDataGenerator' => 'applications/people/lipsum/PhabricatorPeopleTestDataGenerator.php', 'PhabricatorPhameConfigOptions' => 'applications/phame/config/PhabricatorPhameConfigOptions.php', @@ -4626,6 +4627,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleProfileEditController' => 'PhabricatorPeopleController', 'PhabricatorPeopleProfilePictureController' => 'PhabricatorPeopleController', 'PhabricatorPeopleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorPeopleRenameController' => 'PhabricatorPeopleController', 'PhabricatorPeopleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorPeopleTestDataGenerator' => 'PhabricatorTestDataGenerator', 'PhabricatorPhameConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/people/application/PhabricatorApplicationPeople.php b/src/applications/people/application/PhabricatorApplicationPeople.php index 155e523bda..c937cad4e5 100644 --- a/src/applications/people/application/PhabricatorApplicationPeople.php +++ b/src/applications/people/application/PhabricatorApplicationPeople.php @@ -44,6 +44,7 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication { 'approve/(?P[1-9]\d*)/' => 'PhabricatorPeopleApproveController', 'disable/(?P[1-9]\d*)/' => 'PhabricatorPeopleDisableController', 'delete/(?P[1-9]\d*)/' => 'PhabricatorPeopleDeleteController', + 'rename/(?P[1-9]\d*)/' => 'PhabricatorPeopleRenameController', 'edit/(?:(?P[1-9]\d*)/(?:(?P\w+)/)?)?' => 'PhabricatorPeopleEditController', 'ldap/' => 'PhabricatorPeopleLdapController', diff --git a/src/applications/people/controller/PhabricatorPeopleDeleteController.php b/src/applications/people/controller/PhabricatorPeopleDeleteController.php index 2273dd3b1e..296c1e9295 100644 --- a/src/applications/people/controller/PhabricatorPeopleDeleteController.php +++ b/src/applications/people/controller/PhabricatorPeopleDeleteController.php @@ -34,7 +34,7 @@ final class PhabricatorPeopleDeleteController if ($request->isFormPost()) { $v_username = $request->getStr('username'); - if (!$v_username) { + if (!strlen($v_username)) { $errors[] = pht( 'You must type the username to confirm that you want to delete '. 'this user account.'); diff --git a/src/applications/people/controller/PhabricatorPeopleEditController.php b/src/applications/people/controller/PhabricatorPeopleEditController.php index bd8f429cad..4704dea640 100644 --- a/src/applications/people/controller/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleEditController.php @@ -39,9 +39,8 @@ final class PhabricatorPeopleEditController $nav->addFilter('cert', pht('Conduit Certificate')); $nav->addFilter('profile', pht('View Profile'), '/p/'.$user->getUsername().'/'); - $nav->addLabel(pht('Special')); - $nav->addFilter('rename', pht('Change Username')); if ($user->getIsSystemAgent()) { + $nav->addLabel(pht('Special')); $nav->addFilter('picture', pht('Set Account Picture')); } @@ -72,9 +71,6 @@ final class PhabricatorPeopleEditController case 'cert': $response = $this->processCertificateRequest($user); break; - case 'rename': - $response = $this->processRenameRequest($user); - break; case 'picture': $response = $this->processSetAccountPicture($user); break; @@ -480,95 +476,6 @@ final class PhabricatorPeopleEditController return array($form_box); } - private function processRenameRequest(PhabricatorUser $user) { - $request = $this->getRequest(); - $admin = $request->getUser(); - - $e_username = true; - $username = $user->getUsername(); - - $errors = array(); - if ($request->isFormPost()) { - - $username = $request->getStr('username'); - if (!strlen($username)) { - $e_username = pht('Required'); - $errors[] = pht('New username is required.'); - } else if ($username == $user->getUsername()) { - $e_username = pht('Invalid'); - $errors[] = pht('New username must be different from old username.'); - } else if (!PhabricatorUser::validateUsername($username)) { - $e_username = pht('Invalid'); - $errors[] = PhabricatorUser::describeValidUsername(); - } - - if (!$errors) { - try { - - id(new PhabricatorUserEditor()) - ->setActor($admin) - ->changeUsername($user, $username); - - return id(new AphrontRedirectResponse()) - ->setURI($request->getRequestURI()->alter('saved', true)); - } catch (AphrontQueryDuplicateKeyException $ex) { - $e_username = pht('Not Unique'); - $errors[] = pht('Another user already has that username.'); - } - } - } - - $inst1 = pht('Be careful when renaming users!'); - $inst2 = pht('The old username will no longer be tied to the user, so '. - 'anything which uses it (like old commit messages) will no longer '. - 'associate correctly. And if you give a user a username which some '. - 'other user used to have, username lookups will begin returning '. - 'the wrong user.'); - $inst3 = pht('It is generally safe to rename newly created users (and '. - 'test users and so on), but less safe to rename established users '. - 'and unsafe to reissue a username.'); - $inst4 = pht('Users who rely on password auth will need to reset their '. - 'passwordafter their username is changed (their username is part '. - 'of the salt in the password hash). They will receive an email '. - 'with instructions on how to do this.'); - - $form = new AphrontFormView(); - $form - ->setUser($admin) - ->setAction($request->getRequestURI()) - ->appendChild(hsprintf( - '

'. - '%s '. - '%s'. - '

'. - '

'. - '%s'. - '

'. - '

'. - '%s'. - '

', $inst1, $inst2, $inst3, $inst4)) - ->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('Old Username')) - ->setValue($user->getUsername())) - ->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('New Username')) - ->setValue($username) - ->setName('username') - ->setError($e_username)) - ->appendChild( - id(new AphrontFormSubmitControl()) - ->setValue(pht('Change Username'))); - - $form_box = id(new PHUIObjectBoxView()) - ->setHeaderText(pht('Change Username')) - ->setFormErrors($errors) - ->setForm($form); - - return array($form_box); - } - private function getRoleInstructions() { $roles_link = phutil_tag( 'a', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php index 1e3ebd3684..a8522b4595 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -61,6 +61,13 @@ final class PhabricatorPeopleProfileController ->setWorkflow(!$can_edit)); if ($viewer->getIsAdmin()) { + $actions->addAction( + id(new PhabricatorActionView()) + ->setIcon('tag') + ->setName(pht('Change Username')) + ->setWorkflow(true) + ->setHref($this->getApplicationURI('rename/'.$user->getID().'/'))); + $actions->addAction( id(new PhabricatorActionView()) ->setIcon('delete') diff --git a/src/applications/people/controller/PhabricatorPeopleRenameController.php b/src/applications/people/controller/PhabricatorPeopleRenameController.php new file mode 100644 index 0000000000..624f5e169b --- /dev/null +++ b/src/applications/people/controller/PhabricatorPeopleRenameController.php @@ -0,0 +1,112 @@ +id = $data['id']; + } + + public function processRequest() { + $request = $this->getRequest(); + $admin = $request->getUser(); + + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($admin) + ->withIDs(array($this->id)) + ->executeOne(); + if (!$user) { + return new Aphront404Response(); + } + + $profile_uri = '/p/'.$user->getUsername(); + + $errors = array(); + + $v_username = $user->getUsername(); + $e_username = true; + if ($request->isFormPost()) { + $v_username = $request->getStr('username'); + + + if (!strlen($v_username)) { + $e_username = pht('Required'); + $errors[] = pht('New username is required.'); + } else if ($v_username == $user->getUsername()) { + $e_username = pht('Invalid'); + $errors[] = pht('New username must be different from old username.'); + } else if (!PhabricatorUser::validateUsername($v_username)) { + $e_username = pht('Invalid'); + $errors[] = PhabricatorUser::describeValidUsername(); + } + + if (!$errors) { + try { + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->changeUsername($user, $v_username); + + $new_uri = '/p/'.$v_username.'/'; + + return id(new AphrontRedirectResponse())->setURI($new_uri); + } catch (AphrontQueryDuplicateKeyException $ex) { + $e_username = pht('Not Unique'); + $errors[] = pht('Another user already has that username.'); + } + } + } + + $inst1 = pht( + 'Be careful when renaming users!'); + + $inst2 = pht( + 'The old username will no longer be tied to the user, so anything '. + 'which uses it (like old commit messages) will no longer associate '. + 'correctly. (And, if you give a user a username which some other user '. + 'used to have, username lookups will begin returning the wrong user.)'); + + $inst3 = pht( + 'It is generally safe to rename newly created users (and test users '. + 'and so on), but less safe to rename established users and unsafe to '. + 'reissue a username.'); + + $inst4 = pht( + 'Users who rely on password authentication will need to reset their '. + 'password after their username is changed (their username is part of '. + 'the salt in the password hash). They will receive an email with '. + 'instructions on how to do this.'); + + $form = id(new AphrontFormView()) + ->setUser($admin) + ->appendChild( + id(new AphrontFormStaticControl()) + ->setLabel(pht('Old Username')) + ->setValue($user->getUsername())) + ->appendChild( + id(new AphrontFormTextControl()) + ->setLabel(pht('New Username')) + ->setValue($v_username) + ->setName('username') + ->setError($e_username)); + + if ($errors) { + $errors = id(new AphrontErrorView())->setErrors($errors); + } + + return $this->newDialog() + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->setTitle(pht('Change Username')) + ->appendChild($errors) + ->appendParagraph($inst1) + ->appendParagraph($inst2) + ->appendParagraph($inst3) + ->appendParagraph($inst4) + ->appendParagraph(null) + ->appendChild($form->buildLayoutView()) + ->addSubmitButton(pht('Rename User')) + ->addCancelButton($profile_uri); + } + +}