From 5537e29ee8e94381427b1628c7f210a6e5140eec Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Jan 2019 09:39:27 -0800 Subject: [PATCH] Move "Welcome" mail generation out of PhabricatorUser Summary: Ref PHI1027. Currently, `PhabricatorUser` has a couple of mail-related methods which shouldn't really be there in the long term. Immediately, I want to make some adjusments to the welcome email. Move "Welcome" mail generation to a separate class and consolidate all the error handling. (Eventually, "invite" and "verify address" email should move to similar subclasses, too.) Previously, a bunch of errors/conditions got checked in multiple places. The only functional change is that we no longer allow you to send welcome mail to disabled users. Test Plan: - Used "Send Welcome Mail" from profile pages to send mail. - Hit "not admin", "disabled user", "bot/mailing list" errors. - Used `scripts/user/add_user.php` to send welcome mail. - Used "Create New User" to send welcome mail. - Verified mail with `bin/mail show-outbound`. (Cleaned up a couple of minor display issues here.) Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19989 --- scripts/user/add_user.php | 7 +- src/__phutil_library_map__.php | 6 ++ ...atorMailManagementShowOutboundWorkflow.php | 8 ++ .../PhabricatorPeopleNewController.php | 9 +- ...abricatorPeopleProfileManageController.php | 5 +- .../PhabricatorPeopleWelcomeController.php | 29 ++++--- .../mail/PhabricatorPeopleMailEngine.php | 61 ++++++++++++++ .../PhabricatorPeopleMailEngineException.php | 24 ++++++ .../PhabricatorPeopleWelcomeMailEngine.php | 83 +++++++++++++++++++ .../people/storage/PhabricatorUser.php | 50 ----------- 10 files changed, 218 insertions(+), 64 deletions(-) create mode 100644 src/applications/people/mail/PhabricatorPeopleMailEngine.php create mode 100644 src/applications/people/mail/PhabricatorPeopleMailEngineException.php create mode 100644 src/applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php diff --git a/scripts/user/add_user.php b/scripts/user/add_user.php index 4c598e47e2..2554ab3ddc 100755 --- a/scripts/user/add_user.php +++ b/scripts/user/add_user.php @@ -59,7 +59,12 @@ id(new PhabricatorUserEditor()) ->setActor($admin) ->createNewUser($user, $email_object); -$user->sendWelcomeEmail($admin); +$welcome_engine = id(new PhabricatorPeopleWelcomeMailEngine()) + ->setSender($admin) + ->setRecipient($user); +if ($welcome_engine->canSendMail()) { + $welcome_engine->sendMail(); +} echo pht( "Created user '%s' (realname='%s', email='%s').\n", diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 648a8cd894..82a639f61f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3814,6 +3814,8 @@ phutil_register_library_map(array( 'PhabricatorPeopleLogQuery' => 'applications/people/query/PhabricatorPeopleLogQuery.php', 'PhabricatorPeopleLogSearchEngine' => 'applications/people/query/PhabricatorPeopleLogSearchEngine.php', 'PhabricatorPeopleLogsController' => 'applications/people/controller/PhabricatorPeopleLogsController.php', + 'PhabricatorPeopleMailEngine' => 'applications/people/mail/PhabricatorPeopleMailEngine.php', + 'PhabricatorPeopleMailEngineException' => 'applications/people/mail/PhabricatorPeopleMailEngineException.php', 'PhabricatorPeopleManageProfileMenuItem' => 'applications/people/menuitem/PhabricatorPeopleManageProfileMenuItem.php', 'PhabricatorPeopleManagementWorkflow' => 'applications/people/management/PhabricatorPeopleManagementWorkflow.php', 'PhabricatorPeopleNewController' => 'applications/people/controller/PhabricatorPeopleNewController.php', @@ -3841,6 +3843,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleUserFunctionDatasource' => 'applications/people/typeahead/PhabricatorPeopleUserFunctionDatasource.php', 'PhabricatorPeopleUserPHIDType' => 'applications/people/phid/PhabricatorPeopleUserPHIDType.php', 'PhabricatorPeopleWelcomeController' => 'applications/people/controller/PhabricatorPeopleWelcomeController.php', + 'PhabricatorPeopleWelcomeMailEngine' => 'applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php', 'PhabricatorPhabricatorAuthProvider' => 'applications/auth/provider/PhabricatorPhabricatorAuthProvider.php', 'PhabricatorPhameApplication' => 'applications/phame/application/PhabricatorPhameApplication.php', 'PhabricatorPhameBlogPHIDType' => 'applications/phame/phid/PhabricatorPhameBlogPHIDType.php', @@ -9730,6 +9733,8 @@ phutil_register_library_map(array( 'PhabricatorPeopleLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorPeopleLogSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorPeopleLogsController' => 'PhabricatorPeopleController', + 'PhabricatorPeopleMailEngine' => 'Phobject', + 'PhabricatorPeopleMailEngineException' => 'Exception', 'PhabricatorPeopleManageProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorPeopleManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorPeopleNewController' => 'PhabricatorPeopleController', @@ -9757,6 +9762,7 @@ phutil_register_library_map(array( 'PhabricatorPeopleUserFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorPeopleUserPHIDType' => 'PhabricatorPHIDType', 'PhabricatorPeopleWelcomeController' => 'PhabricatorPeopleController', + 'PhabricatorPeopleWelcomeMailEngine' => 'PhabricatorPeopleMailEngine', 'PhabricatorPhabricatorAuthProvider' => 'PhabricatorOAuth2AuthProvider', 'PhabricatorPhameApplication' => 'PhabricatorApplication', 'PhabricatorPhameBlogPHIDType' => 'PhabricatorPHIDType', diff --git a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php index d462314342..f29a63c2eb 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementShowOutboundWorkflow.php @@ -115,7 +115,14 @@ final class PhabricatorMailManagementShowOutboundWorkflow $info[] = $this->newSectionHeader(pht('HEADERS')); $headers = $message->getDeliveredHeaders(); + if (!$headers) { + $headers = array(); + } + $unfiltered = $message->getUnfilteredHeaders(); + if (!$unfiltered) { + $unfiltered = array(); + } $header_map = array(); foreach ($headers as $header) { @@ -201,6 +208,7 @@ final class PhabricatorMailManagementShowOutboundWorkflow $info[] = null; } else { $info[] = pht('(This message has no HTML body.)'); + $info[] = null; } $console->writeOut('%s', implode("\n", $info)); diff --git a/src/applications/people/controller/PhabricatorPeopleNewController.php b/src/applications/people/controller/PhabricatorPeopleNewController.php index 3521fb840f..44dfe0e8a6 100644 --- a/src/applications/people/controller/PhabricatorPeopleNewController.php +++ b/src/applications/people/controller/PhabricatorPeopleNewController.php @@ -107,8 +107,13 @@ final class PhabricatorPeopleNewController ->makeMailingListUser($user, true); } - if ($welcome_checked && !$is_bot && !$is_list) { - $user->sendWelcomeEmail($admin); + if ($welcome_checked) { + $welcome_engine = id(new PhabricatorPeopleWelcomeMailEngine()) + ->setSender($admin) + ->setRecipient($user); + if ($welcome_engine->canSendMail()) { + $welcome_engine->sendMail(); + } } $response = id(new AphrontRedirectResponse()) diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php index 046726f39e..3e6c4ffa6e 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php @@ -92,8 +92,11 @@ final class PhabricatorPeopleProfileManageController PeopleDisableUsersCapability::CAPABILITY); $can_disable = ($has_disable && !$is_self); - $can_welcome = ($is_admin && $user->canEstablishWebSessions()); + $welcome_engine = id(new PhabricatorPeopleWelcomeMailEngine()) + ->setSender($viewer) + ->setRecipient($user); + $can_welcome = $welcome_engine->canSendMail(); $curtain = $this->newCurtainView($user); $curtain->addAction( diff --git a/src/applications/people/controller/PhabricatorPeopleWelcomeController.php b/src/applications/people/controller/PhabricatorPeopleWelcomeController.php index 14b1544b7f..73ee5fd740 100644 --- a/src/applications/people/controller/PhabricatorPeopleWelcomeController.php +++ b/src/applications/people/controller/PhabricatorPeopleWelcomeController.php @@ -3,6 +3,13 @@ final class PhabricatorPeopleWelcomeController extends PhabricatorPeopleController { + public function shouldRequireAdmin() { + // You need to be an administrator to actually send welcome email, but + // we let anyone hit this page so they can get a nice error dialog + // explaining the issue. + return false; + } + public function handleRequest(AphrontRequest $request) { $admin = $this->getViewer(); @@ -14,22 +21,24 @@ final class PhabricatorPeopleWelcomeController return new Aphront404Response(); } - $profile_uri = '/p/'.$user->getUsername().'/'; + $id = $user->getID(); + $profile_uri = "/people/manage/{$id}/"; - if (!$user->canEstablishWebSessions()) { + $welcome_engine = id(new PhabricatorPeopleWelcomeMailEngine()) + ->setSender($admin) + ->setRecipient($user); + + try { + $welcome_engine->validateMail(); + } catch (PhabricatorPeopleMailEngineException $ex) { return $this->newDialog() - ->setTitle(pht('Not a Normal User')) - ->appendParagraph( - pht( - 'You can not send this user a welcome mail because they are not '. - 'a normal user and can not log in to the web interface. Special '. - 'users (like bots and mailing lists) are unable to establish web '. - 'sessions.')) + ->setTitle($ex->getTitle()) + ->appendParagraph($ex->getBody()) ->addCancelButton($profile_uri, pht('Done')); } if ($request->isFormPost()) { - $user->sendWelcomeEmail($admin); + $welcome_engine->sendMail(); return id(new AphrontRedirectResponse())->setURI($profile_uri); } diff --git a/src/applications/people/mail/PhabricatorPeopleMailEngine.php b/src/applications/people/mail/PhabricatorPeopleMailEngine.php new file mode 100644 index 0000000000..8f8a22b12e --- /dev/null +++ b/src/applications/people/mail/PhabricatorPeopleMailEngine.php @@ -0,0 +1,61 @@ +sender = $sender; + return $this; + } + + final public function getSender() { + if (!$this->sender) { + throw new PhutilInvalidStateException('setSender'); + } + return $this->sender; + } + + final public function setRecipient(PhabricatorUser $recipient) { + $this->recipient = $recipient; + return $this; + } + + final public function getRecipient() { + if (!$this->recipient) { + throw new PhutilInvalidStateException('setRecipient'); + } + return $this->recipient; + } + + final public function canSendMail() { + try { + $this->validateMail(); + return true; + } catch (PhabricatorPeopleMailEngineException $ex) { + return false; + } + } + + final public function sendMail() { + $this->validateMail(); + $mail = $this->newMail(); + + $mail + ->setForceDelivery(true) + ->save(); + + return $mail; + } + + abstract public function validateMail(); + abstract protected function newMail(); + + + final protected function throwValidationException($title, $body) { + throw new PhabricatorPeopleMailEngineException($title, $body); + } + +} diff --git a/src/applications/people/mail/PhabricatorPeopleMailEngineException.php b/src/applications/people/mail/PhabricatorPeopleMailEngineException.php new file mode 100644 index 0000000000..fa19bdfa98 --- /dev/null +++ b/src/applications/people/mail/PhabricatorPeopleMailEngineException.php @@ -0,0 +1,24 @@ +title = $title; + $this->body = $body; + + parent::__construct(pht('%s: %s', $title, $body)); + } + + public function getTitle() { + return $this->title; + } + + public function getBody() { + return $this->body; + } + +} diff --git a/src/applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php b/src/applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php new file mode 100644 index 0000000000..b52de9d519 --- /dev/null +++ b/src/applications/people/mail/PhabricatorPeopleWelcomeMailEngine.php @@ -0,0 +1,83 @@ +getSender(); + $recipient = $this->getRecipient(); + + if (!$sender->getIsAdmin()) { + $this->throwValidationException( + pht('Not an Administrator'), + pht( + 'You can not send welcome mail because you are not an '. + 'administrator. Only administrators may send welcome mail.')); + } + + if ($recipient->getIsDisabled()) { + $this->throwValidationException( + pht('User is Disabled'), + pht( + 'You can not send welcome mail to this user because their account '. + 'is disabled.')); + } + + if (!$recipient->canEstablishWebSessions()) { + $this->throwValidationException( + pht('Not a Normal User'), + pht( + 'You can not send this user welcome mail because they are not '. + 'a normal user and can not log in to the web interface. Special '. + 'users (like bots and mailing lists) are unable to establish '. + 'web sessions.')); + } + } + + protected function newMail() { + $sender = $this->getSender(); + $recipient = $this->getRecipient(); + + $sender_username = $sender->getUserName(); + $sender_realname = $sender->getRealName(); + + $recipient_username = $recipient->getUserName(); + $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); + + $base_uri = PhabricatorEnv::getProductionURI('/'); + + $engine = new PhabricatorAuthSessionEngine(); + + $uri = $engine->getOneTimeLoginURI( + $recipient, + $recipient->loadPrimaryEmail(), + PhabricatorAuthSessionEngine::ONETIME_WELCOME); + + $body = pht( + "Welcome to Phabricator!\n\n". + "%s (%s) has created an account for you.\n\n". + " Username: %s\n\n". + "To login to Phabricator, follow this link and set a password:\n\n". + " %s\n\n". + "After you have set a password, you can login in the future by ". + "going here:\n\n". + " %s\n", + $sender_username, + $sender_realname, + $recipient_username, + $uri, + $base_uri); + + if (!$is_serious) { + $body .= sprintf( + "\n%s\n", + pht("Love,\nPhabricator")); + } + + return id(new PhabricatorMetaMTAMail()) + ->addTos(array($recipient->getPHID())) + ->setSubject(pht('[Phabricator] Welcome to Phabricator')) + ->setBody($body); + } + +} diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index bb3b52f5f5..e24024d96d 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -555,56 +555,6 @@ final class PhabricatorUser } } - public function sendWelcomeEmail(PhabricatorUser $admin) { - if (!$this->canEstablishWebSessions()) { - throw new Exception( - pht( - 'Can not send welcome mail to users who can not establish '. - 'web sessions!')); - } - - $admin_username = $admin->getUserName(); - $admin_realname = $admin->getRealName(); - $user_username = $this->getUserName(); - $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); - - $base_uri = PhabricatorEnv::getProductionURI('/'); - - $engine = new PhabricatorAuthSessionEngine(); - $uri = $engine->getOneTimeLoginURI( - $this, - $this->loadPrimaryEmail(), - PhabricatorAuthSessionEngine::ONETIME_WELCOME); - - $body = pht( - "Welcome to Phabricator!\n\n". - "%s (%s) has created an account for you.\n\n". - " Username: %s\n\n". - "To login to Phabricator, follow this link and set a password:\n\n". - " %s\n\n". - "After you have set a password, you can login in the future by ". - "going here:\n\n". - " %s\n", - $admin_username, - $admin_realname, - $user_username, - $uri, - $base_uri); - - if (!$is_serious) { - $body .= sprintf( - "\n%s\n", - pht("Love,\nPhabricator")); - } - - $mail = id(new PhabricatorMetaMTAMail()) - ->addTos(array($this->getPHID())) - ->setForceDelivery(true) - ->setSubject(pht('[Phabricator] Welcome to Phabricator')) - ->setBody($body) - ->saveAndSend(); - } - public function sendUsernameChangeEmail( PhabricatorUser $admin, $old_username) {