From 1df9a6e6b0702195f6542fc352874b7b78b6eb7e Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 2 Apr 2014 12:06:17 -0700 Subject: [PATCH] Move "Send Welcome Email" to profiles and nuke old weird edit UI Summary: Ref T4065. Moves the last of the weird alternate edit UI to profiles. The old "Edit" controller is now for creation only, and the funky pencil icon is gone. Test Plan: Created accounts; sent welcome email. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4065 Differential Revision: https://secure.phabricator.com/D8670 --- src/__phutil_library_map__.php | 2 + .../PhabricatorApplicationPeople.php | 4 +- .../PhabricatorPeopleDeleteController.php | 2 +- .../PhabricatorPeopleEditController.php | 256 +++++------------- .../PhabricatorPeopleEmpowerController.php | 2 +- .../PhabricatorPeopleListController.php | 5 - .../PhabricatorPeopleProfileController.php | 7 +- .../PhabricatorPeopleRenameController.php | 11 +- .../PhabricatorPeopleWelcomeController.php | 50 ++++ .../PhabricatorSettingsMainController.php | 1 - 10 files changed, 134 insertions(+), 206 deletions(-) create mode 100644 src/applications/people/controller/PhabricatorPeopleWelcomeController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0354ef878b..a778a489e8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1822,6 +1822,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleRenameController' => 'applications/people/controller/PhabricatorPeopleRenameController.php', 'PhabricatorPeopleSearchEngine' => 'applications/people/query/PhabricatorPeopleSearchEngine.php', 'PhabricatorPeopleTestDataGenerator' => 'applications/people/lipsum/PhabricatorPeopleTestDataGenerator.php', + 'PhabricatorPeopleWelcomeController' => 'applications/people/controller/PhabricatorPeopleWelcomeController.php', 'PhabricatorPhameConfigOptions' => 'applications/phame/config/PhabricatorPhameConfigOptions.php', 'PhabricatorPhamePHIDTypeBlog' => 'applications/phame/phid/PhabricatorPhamePHIDTypeBlog.php', 'PhabricatorPhamePHIDTypePost' => 'applications/phame/phid/PhabricatorPhamePHIDTypePost.php', @@ -4632,6 +4633,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleRenameController' => 'PhabricatorPeopleController', 'PhabricatorPeopleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorPeopleTestDataGenerator' => 'PhabricatorTestDataGenerator', + 'PhabricatorPeopleWelcomeController' => 'PhabricatorPeopleController', 'PhabricatorPhameConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorPhamePHIDTypeBlog' => 'PhabricatorPHIDType', 'PhabricatorPhamePHIDTypePost' => 'PhabricatorPHIDType', diff --git a/src/applications/people/application/PhabricatorApplicationPeople.php b/src/applications/people/application/PhabricatorApplicationPeople.php index 99f52c4d68..1fb30db3ca 100644 --- a/src/applications/people/application/PhabricatorApplicationPeople.php +++ b/src/applications/people/application/PhabricatorApplicationPeople.php @@ -49,8 +49,8 @@ final class PhabricatorApplicationPeople extends PhabricatorApplication { 'empower/(?P[1-9]\d*)/' => 'PhabricatorPeopleEmpowerController', 'delete/(?P[1-9]\d*)/' => 'PhabricatorPeopleDeleteController', 'rename/(?P[1-9]\d*)/' => 'PhabricatorPeopleRenameController', - 'edit/(?:(?P[1-9]\d*)/(?:(?P\w+)/)?)?' - => 'PhabricatorPeopleEditController', + 'welcome/(?P[1-9]\d*)/' => 'PhabricatorPeopleWelcomeController', + 'edit/' => 'PhabricatorPeopleEditController', 'ldap/' => 'PhabricatorPeopleLdapController', 'editprofile/(?P[1-9]\d*)/' => 'PhabricatorPeopleProfileEditController', diff --git a/src/applications/people/controller/PhabricatorPeopleDeleteController.php b/src/applications/people/controller/PhabricatorPeopleDeleteController.php index 296c1e9295..25ba5c751f 100644 --- a/src/applications/people/controller/PhabricatorPeopleDeleteController.php +++ b/src/applications/people/controller/PhabricatorPeopleDeleteController.php @@ -21,7 +21,7 @@ final class PhabricatorPeopleDeleteController return new Aphront404Response(); } - $profile_uri = '/p/'.$user->getUsername(); + $profile_uri = '/p/'.$user->getUsername().'/'; if ($user->getPHID() == $admin->getPHID()) { return $this->buildDeleteSelfResponse($profile_uri); diff --git a/src/applications/people/controller/PhabricatorPeopleEditController.php b/src/applications/people/controller/PhabricatorPeopleEditController.php index f447323a22..165892a568 100644 --- a/src/applications/people/controller/PhabricatorPeopleEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleEditController.php @@ -3,83 +3,31 @@ final class PhabricatorPeopleEditController extends PhabricatorPeopleController { - private $id; - private $view; - - public function willProcessRequest(array $data) { - $this->id = idx($data, 'id'); - $this->view = idx($data, 'view'); - } - public function processRequest() { $request = $this->getRequest(); $admin = $request->getUser(); $crumbs = $this->buildApplicationCrumbs($this->buildSideNavView()); - if ($this->id) { - $user = id(new PhabricatorUser())->load($this->id); - if (!$user) { - return new Aphront404Response(); - } - $base_uri = '/people/edit/'.$user->getID().'/'; - $crumbs->addTextCrumb(pht('Edit User'), '/people/edit/'); - $crumbs->addTextCrumb($user->getFullName(), $base_uri); - } else { - $user = new PhabricatorUser(); - $base_uri = '/people/edit/'; - $crumbs->addTextCrumb(pht('Create New User'), $base_uri); - } - $nav = new AphrontSideNavFilterView(); - $nav->setBaseURI(new PhutilURI($base_uri)); - $nav->addLabel(pht('User Information')); - $nav->addFilter('basic', pht('Basic Information')); - $nav->addFilter('profile', - pht('View Profile'), '/p/'.$user->getUsername().'/'); - - if (!$user->getID()) { - $this->view = 'basic'; - } - - $view = $nav->selectFilter($this->view, 'basic'); + $user = new PhabricatorUser(); + $base_uri = '/people/edit/'; + $crumbs->addTextCrumb(pht('Create New User'), $base_uri); $content = array(); - if ($request->getStr('saved')) { - $notice = new AphrontErrorView(); - $notice->setSeverity(AphrontErrorView::SEVERITY_NOTICE); - $notice->setTitle(pht('Changes Saved')); - $notice->appendChild( - phutil_tag('p', array(), pht('Your changes were saved.'))); - $content[] = $notice; - } - - switch ($view) { - case 'basic': - $response = $this->processBasicRequest($user); - break; - default: - return new Aphront404Response(); - } - + $response = $this->processBasicRequest($user); if ($response instanceof AphrontResponse) { return $response; } $content[] = $response; - if ($user->getID()) { - $nav->appendChild($content); - } else { - $nav = $this->buildSideNavView(); - $nav->selectFilter('edit'); - $nav->appendChild($content); - } - - $nav->setCrumbs($crumbs); return $this->buildApplicationPage( - $nav, + array( + $crumbs, + $content, + ), array( 'title' => pht('Edit User'), 'device' => true, @@ -102,23 +50,20 @@ final class PhabricatorPeopleEditController $request = $this->getRequest(); if ($request->isFormPost()) { $welcome_checked = $request->getInt('welcome'); - $is_new = !$user->getID(); - if ($is_new) { - $user->setUsername($request->getStr('username')); - - $new_email = $request->getStr('email'); - if (!strlen($new_email)) { - $errors[] = pht('Email is required.'); - $e_email = pht('Required'); - } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { - $e_email = pht('Invalid'); - $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); - } else { - $e_email = null; - } + $user->setUsername($request->getStr('username')); + $new_email = $request->getStr('email'); + if (!strlen($new_email)) { + $errors[] = pht('Email is required.'); + $e_email = pht('Required'); + } else if (!PhabricatorUserEmail::isAllowedAddress($new_email)) { + $e_email = pht('Invalid'); + $errors[] = PhabricatorUserEmail::describeAllowedAddresses(); + } else { + $e_email = null; } + $user->setRealName($request->getStr('realname')); if (!strlen($user->getUsername())) { @@ -141,28 +86,21 @@ final class PhabricatorPeopleEditController if (!$errors) { try { - if (!$is_new) { + $email = id(new PhabricatorUserEmail()) + ->setAddress($new_email) + ->setIsVerified(0); + + // Automatically approve the user, since an admin is creating them. + $user->setIsApproved(1); + + id(new PhabricatorUserEditor()) + ->setActor($admin) + ->createNewUser($user, $email); + + if ($request->getStr('role') == 'agent') { id(new PhabricatorUserEditor()) ->setActor($admin) - ->updateUser($user); - } else { - $email = id(new PhabricatorUserEmail()) - ->setAddress($new_email) - ->setIsVerified(0); - - // Automatically approve the user, since an admin is creating them. - $user->setIsApproved(1); - - id(new PhabricatorUserEditor()) - ->setActor($admin) - ->createNewUser($user, $email); - - if ($request->getStr('role') == 'agent') { - id(new PhabricatorUserEditor()) - ->setActor($admin) - ->makeSystemAgentUser($user, true); - } - + ->makeSystemAgentUser($user, true); } if ($welcome_checked) { @@ -170,7 +108,7 @@ final class PhabricatorPeopleEditController } $response = id(new AphrontRedirectResponse()) - ->setURI('/people/edit/'.$user->getID().'/?saved=true'); + ->setURI('/p/'.$user->getUsername().'/'); return $response; } catch (AphrontQueryDuplicateKeyException $ex) { $errors[] = pht('Username and email must be unique.'); @@ -193,17 +131,9 @@ final class PhabricatorPeopleEditController $form = new AphrontFormView(); $form->setUser($admin); - if ($user->getID()) { - $form->setAction('/people/edit/'.$user->getID().'/'); - } else { - $form->setAction('/people/edit/'); - } + $form->setAction('/people/edit/'); - if ($user->getID()) { - $is_immutable = true; - } else { - $is_immutable = false; - } + $is_immutable = false; $form ->appendChild( @@ -220,100 +150,46 @@ final class PhabricatorPeopleEditController ->setValue($user->getRealName()) ->setError($e_realname)); - if (!$user->getID()) { - $form->appendChild( - id(new AphrontFormTextControl()) - ->setLabel(pht('Email')) - ->setName('email') - ->setDisabled($is_immutable) - ->setValue($new_email) - ->setCaption(PhabricatorUserEmail::describeAllowedAddresses()) - ->setError($e_email)); - } else { - $email = $user->loadPrimaryEmail(); - if ($email) { - $status = $email->getIsVerified() ? - pht('Verified') : pht('Unverified'); - } else { - $status = pht('No Email Address'); - } - - $form->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('Email')) - ->setValue($status)); - - $form->appendChild( - id(new AphrontFormCheckboxControl()) - ->addCheckbox( - 'welcome', - 1, - pht('Re-send "Welcome to Phabricator" email.'), - false)); - - } + $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()); - if (!$user->getID()) { - $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.'))) - ->appendChild( - id(new AphrontFormCheckboxControl()) - ->addCheckbox( - 'welcome', - 1, - pht('Send "Welcome to Phabricator" email.'), - $welcome_checked)); - } else { - $roles = array(); - - if ($user->getIsSystemAgent()) { - $roles[] = pht('System Agent'); - } - if ($user->getIsAdmin()) { - $roles[] = pht('Admin'); - } - if ($user->getIsDisabled()) { - $roles[] = pht('Disabled'); - } - if (!$user->getIsApproved()) { - $roles[] = pht('Not Approved'); - } - if (!$roles) { - $roles[] = pht('Normal User'); - } - - $roles = implode(', ', $roles); - - $form->appendChild( - id(new AphrontFormStaticControl()) - ->setLabel(pht('Roles')) - ->setValue($roles)); - } + $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.'))) + ->appendChild( + id(new AphrontFormCheckboxControl()) + ->addCheckbox( + 'welcome', + 1, + pht('Send "Welcome to Phabricator" email.'), + $welcome_checked)); $form ->appendChild( id(new AphrontFormSubmitControl()) + ->addCancelButton($this->getApplicationURI()) ->setValue(pht('Save'))); - if ($user->getID()) { - $title = pht('Edit User'); - } else { - $title = pht('Create New User'); - } + $title = pht('Create New User'); $form_box = id(new PHUIObjectBoxView()) ->setHeaderText($title) diff --git a/src/applications/people/controller/PhabricatorPeopleEmpowerController.php b/src/applications/people/controller/PhabricatorPeopleEmpowerController.php index 743059905a..7ee8ffc6c2 100644 --- a/src/applications/people/controller/PhabricatorPeopleEmpowerController.php +++ b/src/applications/people/controller/PhabricatorPeopleEmpowerController.php @@ -21,7 +21,7 @@ final class PhabricatorPeopleEmpowerController return new Aphront404Response(); } - $profile_uri = '/p/'.$user->getUsername(); + $profile_uri = '/p/'.$user->getUsername().'/'; if ($user->getPHID() == $admin->getPHID()) { return $this->newDialog() diff --git a/src/applications/people/controller/PhabricatorPeopleListController.php b/src/applications/people/controller/PhabricatorPeopleListController.php index 0a035e1231..63b7c21c89 100644 --- a/src/applications/people/controller/PhabricatorPeopleListController.php +++ b/src/applications/people/controller/PhabricatorPeopleListController.php @@ -99,11 +99,6 @@ final class PhabricatorPeopleListController extends PhabricatorPeopleController ->setName(pht('Approve')) ->setWorkflow(true) ->setHref($this->getApplicationURI('approve/'.$user_id.'/'))); - } else { - $item->addAction( - id(new PHUIListItemView()) - ->setIcon('edit') - ->setHref($this->getApplicationURI('edit/'.$user_id.'/'))); } } diff --git a/src/applications/people/controller/PhabricatorPeopleProfileController.php b/src/applications/people/controller/PhabricatorPeopleProfileController.php index c6091fb187..0413127dce 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileController.php @@ -121,9 +121,10 @@ final class PhabricatorPeopleProfileController $actions->addAction( id(new PhabricatorActionView()) - ->setIcon('blame') - ->setName(pht('Administrate User')) - ->setHref($this->getApplicationURI('edit/'.$user->getID().'/'))); + ->setIcon('message') + ->setName(pht('Send Welcome Email')) + ->setWorkflow(true) + ->setHref($this->getApplicationURI('welcome/'.$user->getID().'/'))); } $properties = $this->buildPropertyView($user, $actions); diff --git a/src/applications/people/controller/PhabricatorPeopleRenameController.php b/src/applications/people/controller/PhabricatorPeopleRenameController.php index 624f5e169b..a785f4d249 100644 --- a/src/applications/people/controller/PhabricatorPeopleRenameController.php +++ b/src/applications/people/controller/PhabricatorPeopleRenameController.php @@ -21,7 +21,7 @@ final class PhabricatorPeopleRenameController return new Aphront404Response(); } - $profile_uri = '/p/'.$user->getUsername(); + $profile_uri = '/p/'.$user->getUsername().'/'; $errors = array(); @@ -75,8 +75,12 @@ final class PhabricatorPeopleRenameController $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.'); + 'the salt in the password hash).'); + + $inst5 = pht( + 'The user will receive an email notifying them that you changed their '. + 'username, with instructions for logging in and resetting their '. + 'password if necessary.'); $form = id(new AphrontFormView()) ->setUser($admin) @@ -103,6 +107,7 @@ final class PhabricatorPeopleRenameController ->appendParagraph($inst2) ->appendParagraph($inst3) ->appendParagraph($inst4) + ->appendParagraph($inst5) ->appendParagraph(null) ->appendChild($form->buildLayoutView()) ->addSubmitButton(pht('Rename User')) diff --git a/src/applications/people/controller/PhabricatorPeopleWelcomeController.php b/src/applications/people/controller/PhabricatorPeopleWelcomeController.php new file mode 100644 index 0000000000..7b0131a4b5 --- /dev/null +++ b/src/applications/people/controller/PhabricatorPeopleWelcomeController.php @@ -0,0 +1,50 @@ +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().'/'; + + if ($request->isFormPost()) { + $user->sendWelcomeEmail($admin); + return id(new AphrontRedirectResponse())->setURI($profile_uri); + } + + return $this->newDialog() + ->setTitle(pht('Send Welcome Email')) + ->appendParagraph( + pht( + 'This will send the user another copy of the "Welcome to '. + 'Phabricator" email that users normally receive when their '. + 'accounts are created.')) + ->appendParagraph( + pht( + 'The email contains a link to log in to their account. Sending '. + 'another copy of the email can be useful if the original was lost '. + 'or never sent.')) + ->appendParagraph( + pht( + 'The email will identify you as the sender.')) + ->addSubmitButton(pht('Send Email')) + ->addCancelButton($profile_uri); + } + +} diff --git a/src/applications/settings/controller/PhabricatorSettingsMainController.php b/src/applications/settings/controller/PhabricatorSettingsMainController.php index 572b087ff2..cf3792f675 100644 --- a/src/applications/settings/controller/PhabricatorSettingsMainController.php +++ b/src/applications/settings/controller/PhabricatorSettingsMainController.php @@ -51,7 +51,6 @@ final class PhabricatorSettingsMainController $key = $nav->selectFilter($this->key, head($panels)->getPanelKey()); - $panel = $panels[$key]; $panel->setUser($this->getUser()); $panel->setViewer($viewer);