Make user creation process simpler

Summary: Fixes T4065. This divides user creation into separate "Standard User" and "Script/Bot" workflows which show only relevant fields and provide guidance. This fixes the verification mess associated with script/bot users by verifying their email addresses automatically. Test Plan: - Created a standard user. - Created a script/bot. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4065 Differential Revision: https://secure.phabricator.com/D8674
2024-09-20 09:18:48 +02:00 · 2014-04-02 12:06:27 -07:00 · 2014-04-02 12:06:27 -07:00 · 3294649a02
commit 3294649a02
parent 1df9a6e6b0
5 changed files with 191 additions and 85 deletions
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@ -1805,14 +1805,15 @@ phutil_register_library_map(array(
    'PhabricatorPeopleApproveController' => 'applications/people/controller/PhabricatorPeopleApproveController.php',
    'PhabricatorPeopleCalendarController' => 'applications/people/controller/PhabricatorPeopleCalendarController.php',
    'PhabricatorPeopleController' => 'applications/people/controller/PhabricatorPeopleController.php',
+    'PhabricatorPeopleCreateController' => 'applications/people/controller/PhabricatorPeopleCreateController.php',
    'PhabricatorPeopleDeleteController' => 'applications/people/controller/PhabricatorPeopleDeleteController.php',
    'PhabricatorPeopleDisableController' => 'applications/people/controller/PhabricatorPeopleDisableController.php',
-    'PhabricatorPeopleEditController' => 'applications/people/controller/PhabricatorPeopleEditController.php',
    'PhabricatorPeopleEmpowerController' => 'applications/people/controller/PhabricatorPeopleEmpowerController.php',
    'PhabricatorPeopleHovercardEventListener' => 'applications/people/event/PhabricatorPeopleHovercardEventListener.php',
    'PhabricatorPeopleLdapController' => 'applications/people/controller/PhabricatorPeopleLdapController.php',
    'PhabricatorPeopleListController' => 'applications/people/controller/PhabricatorPeopleListController.php',
    'PhabricatorPeopleLogsController' => 'applications/people/controller/PhabricatorPeopleLogsController.php',
+    'PhabricatorPeopleNewController' => 'applications/people/controller/PhabricatorPeopleNewController.php',
    'PhabricatorPeoplePHIDTypeExternal' => 'applications/people/phid/PhabricatorPeoplePHIDTypeExternal.php',
    'PhabricatorPeoplePHIDTypeUser' => 'applications/people/phid/PhabricatorPeoplePHIDTypeUser.php',
    'PhabricatorPeopleProfileController' => 'applications/people/controller/PhabricatorPeopleProfileController.php',
@ -4612,9 +4613,9 @@ phutil_register_library_map(array(
    'PhabricatorPeopleApproveController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleCalendarController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleController' => 'PhabricatorController',
+    'PhabricatorPeopleCreateController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleDeleteController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleDisableController' => 'PhabricatorPeopleController',
-    'PhabricatorPeopleEditController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleEmpowerController' => 'PhabricatorPeopleController',
    'PhabricatorPeopleHovercardEventListener' => 'PhabricatorEventListener',
    'PhabricatorPeopleLdapController' => 'PhabricatorPeopleController',
@ -4624,6 +4625,7 @@ phutil_register_library_map(array(
      1 => 'PhabricatorApplicationSearchResultsControllerInterface',
    ),
    'PhabricatorPeopleLogsController' => 'PhabricatorPeopleController',
+    'PhabricatorPeopleNewController' => 'PhabricatorPeopleController',
    'PhabricatorPeoplePHIDTypeExternal' => 'PhabricatorPHIDType',
    'PhabricatorPeoplePHIDTypeUser' => 'PhabricatorPHIDType',
    'PhabricatorPeopleProfileController' => 'PhabricatorPeopleController',
--- a/src/applications/people/application/PhabricatorApplicationPeople.php
+++ b/src/applications/people/application/PhabricatorApplicationPeople.php
@ -50,7 +50,8 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication {
        'delete/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleDeleteController',
        'rename/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleRenameController',
        'welcome/(?P<id>[1-9]\d*)/' => 'PhabricatorPeopleWelcomeController',
-        'edit/' => 'PhabricatorPeopleEditController',
+        'create/' => 'PhabricatorPeopleCreateController',
+        'new/(?P<type>[^/]+)/' => 'PhabricatorPeopleNewController',
        'ldap/' => 'PhabricatorPeopleLdapController',
        'editprofile/(?P<id>[1-9]\d*)/' =>
          'PhabricatorPeopleProfileEditController',
@ -140,4 +141,20 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication {
    return $items;
  }

+
+  public function getQuickCreateItems(PhabricatorUser $viewer) {
+    $items = array();
+
+    if ($viewer->getIsAdmin()) {
+      $item = id(new PHUIListItemView())
+        ->setName(pht('User Account'))
+        ->setAppIcon('people-dark')
+        ->setHref($this->getBaseURI().'create/');
+      $items[] = $item;
+    }
+
+    return $items;
+  }
+
+
 }
--- a/src/applications/people/controller/PhabricatorPeopleController.php
+++ b/src/applications/people/controller/PhabricatorPeopleController.php
@ -41,7 +41,7 @@ abstract class PhabricatorPeopleController extends PhabricatorController {
      $crumbs->addAction(
        id(new PHUIListItemView())
          ->setName(pht('Create New User'))
-          ->setHref($this->getApplicationURI('edit'))
+          ->setHref($this->getApplicationURI('create/'))
          ->setIcon('create'));
    }

--- a/src/applications/people/controller/PhabricatorPeopleCreateController.php
+++ b/src/applications/people/controller/PhabricatorPeopleCreateController.php
@ -0,0 +1,82 @@
+<?php
+
+final class PhabricatorPeopleCreateController
+  extends PhabricatorPeopleController {
+
+  public function processRequest() {
+    $request = $this->getRequest();
+    $admin = $request->getUser();
+
+    $v_type = 'standard';
+    if ($request->isFormPost()) {
+      $v_type = $request->getStr('type');
+
+      if ($v_type == 'standard' || $v_type == 'bot') {
+        return id(new AphrontRedirectResponse())->setURI(
+          $this->getApplicationURI('new/'.$v_type.'/'));
+      }
+    }
+
+    $title = pht('Create New User');
+
+    $standard_caption = pht(
+      'Create a standard user account. These users can log in to Phabricator, '.
+      'use the web interface and API, and receive email.');
+
+    $standard_admin = pht(
+      'Administrators are limited in their ability to access or edit these '.
+      'accounts after account creation.');
+
+    $bot_caption = pht(
+      'Create a bot/script user account, to automate interactions with other '.
+      'systems. These users can not use the web interface, but can use the '.
+      'API.');
+
+    $bot_admin = pht(
+      'Administrators have greater access to edit these accounts.');
+
+    $form = id(new AphrontFormView())
+      ->setUser($admin)
+      ->appendRemarkupInstructions(
+        pht(
+          'Choose the type of user account to create. For a detailed '.
+          'explanation of user account types, see [[ %s | User Guide: '.
+          'Account Roles ]].',
+          PhabricatorEnv::getDoclink('User Guide: Account Roles')))
+      ->appendChild(
+        id(new AphrontFormRadioButtonControl())
+          ->setLabel(pht('Account Type'))
+          ->setName('type')
+          ->setValue($v_type)
+          ->addButton(
+            'standard',
+            pht('Create Standard User'),
+            hsprintf('%s<br /><br />%s', $standard_caption, $standard_admin))
+          ->addButton(
+            'bot',
+            pht('Create Bot/Script User'),
+            hsprintf('%s<br /><br />%s', $bot_caption, $bot_admin)))
+      ->appendChild(
+        id(new AphrontFormSubmitControl())
+          ->addCancelButton($this->getApplicationURI())
+          ->setValue(pht('Continue')));
+
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb($title);
+
+    $box = id(new PHUIObjectBoxView())
+      ->setHeaderText($title)
+      ->appendChild($form);
+
+    return $this->buildApplicationPage(
+      array(
+        $crumbs,
+        $box,
+      ),
+      array(
+        'title' => $title,
+        'device' => true,
+      ));
+  }
+
+}
--- a/src/applications/people/controller/PhabricatorPeopleEditController.php
+++ b/src/applications/people/controller/PhabricatorPeopleEditController.php
@ -1,43 +1,31 @@
 <?php

-final class PhabricatorPeopleEditController
+final class PhabricatorPeopleNewController
  extends PhabricatorPeopleController {

-  public function processRequest() {
+  private $type;

-    $request = $this->getRequest();
-    $admin = $request->getUser();
-
-    $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView());
-
-    $user = new PhabricatorUser();
-    $base_uri = '/people/edit/';
-    $crumbs->addTextCrumb(pht('Create New User'), $base_uri);
-
-    $content = array();
-
-    $response = $this->processBasicRequest($user);
-    if ($response instanceof AphrontResponse) {
-      return $response;
-    }
-
-    $content[] = $response;
-
-    return $this->buildApplicationPage(
-      array(
-        $crumbs,
-        $content,
-      ),
-      array(
-        'title' => pht('Edit User'),
-        'device' => true,
-      ));
+  public function willProcessRequest(array $data) {
+    $this->type = $data['type'];
  }

-  private function processBasicRequest(PhabricatorUser $user) {
+  public function processRequest() {
    $request = $this->getRequest();
    $admin = $request->getUser();

+    switch ($this->type) {
+      case 'standard':
+        $is_bot = false;
+        break;
+      case 'bot':
+        $is_bot = true;
+        break;
+      default:
+        return new Aphront404Response();
+    }
+
+    $user = new PhabricatorUser();
+
    $e_username = true;
    $e_realname = true;
    $e_email    = true;
@ -93,17 +81,22 @@ final class PhabricatorPeopleEditController
          // Automatically approve the user, since an admin is creating them.
          $user->setIsApproved(1);

+          // If the user is a bot, approve their email too.
+          if ($is_bot) {
+            $email->setIsVerified(1);
+          }
+
          id(new PhabricatorUserEditor())
            ->setActor($admin)
            ->createNewUser($user, $email);

-          if ($request->getStr('role') == 'agent') {
+          if ($is_bot) {
            id(new PhabricatorUserEditor())
              ->setActor($admin)
              ->makeSystemAgentUser($user, true);
          }

-          if ($welcome_checked) {
+          if ($welcome_checked && !$is_bot) {
            $user->sendWelcomeEmail($admin);
          }

@ -129,11 +122,18 @@ final class PhabricatorPeopleEditController
      }
    }

-    $form = new AphrontFormView();
-    $form->setUser($admin);
-    $form->setAction('/people/edit/');
+    $form = id(new AphrontFormView())
+      ->setUser($admin);

-    $is_immutable = false;
+    if ($is_bot) {
+      $form->appendRemarkupInstructions(
+        pht(
+          'You are creating a new **bot/script** user account.'));
+    } else {
+      $form->appendRemarkupInstructions(
+        pht(
+          'You are creating a new **standard** user account.'));
+    }

    $form
      ->appendChild(
@ -141,53 +141,63 @@ final class PhabricatorPeopleEditController
          ->setLabel(pht('Username'))
          ->setName('username')
          ->setValue($user->getUsername())
-          ->setError($e_username)
-          ->setDisabled($is_immutable))
+          ->setError($e_username))
      ->appendChild(
        id(new AphrontFormTextControl())
          ->setLabel(pht('Real Name'))
          ->setName('realname')
          ->setValue($user->getRealName())
-          ->setError($e_realname));
-
-    $form->appendChild(
-      id(new AphrontFormTextControl())
-        ->setLabel(pht('Email'))
-        ->setName('email')
-        ->setDisabled($is_immutable)
-        ->setValue($new_email)
-        ->setCaption(PhabricatorUserEmail::describeAllowedAddresses())
-        ->setError($e_email));
-
-    $form->appendChild($this->getRoleInstructions());
-
-    $form
-      ->appendChild(
-        id(new AphrontFormSelectControl())
-          ->setLabel(pht('Role'))
-          ->setName('role')
-          ->setValue('user')
-          ->setOptions(
-            array(
-              'user'  => pht('Normal User'),
-              'agent' => pht('System Agent'),
-            ))
-          ->setCaption(
-            pht('You can create a "system agent" account for bots, '.
-            'scripts, etc.')))
+          ->setError($e_realname))
      ->appendChild(
+        id(new AphrontFormTextControl())
+          ->setLabel(pht('Email'))
+          ->setName('email')
+          ->setValue($new_email)
+          ->setCaption(PhabricatorUserEmail::describeAllowedAddresses())
+          ->setError($e_email));
+
+    if (!$is_bot) {
+      $form->appendChild(
        id(new AphrontFormCheckboxControl())
          ->addCheckbox(
            'welcome',
            1,
-            pht('Send "Welcome to Phabricator" email.'),
+            pht('Send "Welcome to Phabricator" email with login instructions.'),
            $welcome_checked));
+    }

    $form
      ->appendChild(
        id(new AphrontFormSubmitControl())
          ->addCancelButton($this->getApplicationURI())
-          ->setValue(pht('Save')));
+          ->setValue(pht('Create User')));
+
+    if ($is_bot) {
+      $form
+        ->appendChild(id(new AphrontFormDividerControl()))
+        ->appendRemarkupInstructions(
+          pht(
+            '**Why do bot/script accounts need an email address?**'.
+            "\n\n".
+            'Although bots do not normally receive email from Phabricator, '.
+            'they can interact with other systems which require an email '.
+            'address. Examples include:'.
+            "\n\n".
+            "  - If the account takes actions which //send// email, we need ".
+            "    an address to use in the //From// header.\n".
+            "  - If the account creates commits, Git and Mercurial require ".
+            "    an email address for authorship.\n".
+            "  - If you send email //to// Phabricator on behalf of the ".
+            "    account, the address can identify the sender.\n".
+            "  - Some internal authentication functions depend on accounts ".
+            "    having an email address.\n".
+            "\n\n".
+            "The address will automatically be verified, so you do not need ".
+            "to be able to receive mail at this address, and can enter some ".
+            "invalid or nonexistent (but correctly formatted) address like ".
+            "`bot@yourcompany.com` if you prefer."));
+    }
+

    $title = pht('Create New User');

@ -196,23 +206,18 @@ final class PhabricatorPeopleEditController
      ->setFormErrors($errors)
      ->setForm($form);

-    return array($form_box);
-  }
+    $crumbs = $this->buildApplicationCrumbs();
+    $crumbs->addTextCrumb($title);

-  private function getRoleInstructions() {
-    $roles_link = phutil_tag(
-      'a',
+    return $this->buildApplicationPage(
      array(
-        'href'   => PhabricatorEnv::getDoclink(
-          'article/User_Guide_Account_Roles.html'),
-        'target' => '_blank',
+        $crumbs,
+        $form_box,
      ),
-      pht('User Guide: Account Roles'));
-
-    return phutil_tag(
-      'p',
-      array('class' => 'aphront-form-instructions'),
-      pht('For a detailed explanation of account roles, see %s.', $roles_link));
+      array(
+        'title' => $title,
+        'device' => true,
+      ));
  }

 }