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

@@ -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',

src/applications/people/application/PhabricatorApplicationPeople.php

@@ -44,6 +44,7 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication {
         'approve/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleApproveController',
         'disable/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleDisableController',
         'delete/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleDeleteController',
+        'rename/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleRenameController',
         'edit/(?:(?P<id>[1-9]\d*)/(?:(?P<view>\w+)/)?)?'
           => 'PhabricatorPeopleEditController',
         'ldap/' => 'PhabricatorPeopleLdapController',

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.');

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(
-        '<p class="aphront-form-instructions">'.
-          '<strong>%s</strong> '.
-          '%s'.
-        '</p>'.
-        '<p class="aphront-form-instructions">'.
-          '%s'.
-        '</p>'.
-        '<p class="aphront-form-instructions">'.
-          '%s'.
-        '</p>', $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',

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')

src/applications/people/controller/PhabricatorPeopleRenameController.php

@@ -0,0 +1,112 @@
+<?php
+
+final class PhabricatorPeopleRenameController
+  extends PhabricatorPeopleController {
+
+  private $id;
+
+  public function willProcessRequest(array $data) {
+    $this->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);
+  }
+
+}