From 030e05aa4ca53737cf81989a16395142c3712f18 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Mar 2015 16:48:17 -0700 Subject: [PATCH] Remove reply handler instructions from email Summary: Ref T7199. Although this is useful for discovery, it's un-useful enough that we already have an option to disable it, and most applications do not provide any meaningful instructions. Throwing it away makes it easier to move forward and lets us get rid of a config option. This is becoming a more advanced/power-user feature anyway, and the new syntax will be significantly more complex and hard to explain with a one-liner. I'm currently thinking that I'll maybe make the "help" menu a dropdown and give it some options like: +---+ | O | +---+---------------------+ | Maniphest Documentation | | Maniphest Email Actions | +-------------------------+ Then you click the "Email Actions" thing and get a runtime-derived list of available options. Not sure if I'll actually build that, but I think we can fairly throw the in-mail instructions away even if we don't go in that specific direction. Test Plan: Grepped for `replyHandlerInstructions`, got no hits. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7199 Differential Revision: https://secure.phabricator.com/D12229 --- .../mail/PhabricatorAuditReplyHandler.php | 8 ---- .../PhabricatorExtraConfigSetupCheck.php | 2 + .../PhabricatorMetaMTAConfigOptions.php | 8 ---- .../mail/ConpherenceReplyHandler.php | 8 ---- .../mail/DifferentialReplyHandler.php | 47 ------------------- .../files/mail/FileReplyHandler.php | 8 ---- .../fund/mail/FundInitiativeReplyHandler.php | 9 ---- .../legalpad/mail/LegalpadReplyHandler.php | 8 ---- .../mail/PhabricatorMacroReplyHandler.php | 10 ---- .../maniphest/mail/ManiphestReplyHandler.php | 10 ---- .../PhabricatorMailReplyHandler.php | 1 - .../view/PhabricatorMetaMTAMailBody.php | 21 +-------- .../PhabricatorMetaMTAMailBodyTestCase.php | 31 ++---------- .../owners/mail/OwnersPackageReplyHandler.php | 4 -- .../paste/mail/PasteReplyHandler.php | 8 ---- .../pholio/mail/PholioReplyHandler.php | 10 ---- .../mail/PhortuneCartReplyHandler.php | 9 ---- .../phriction/mail/PhrictionReplyHandler.php | 9 ---- .../mail/PonderQuestionReplyHandler.php | 4 -- .../mail/ReleephRequestReplyHandler.php | 8 ---- .../PhabricatorRepositoryPushReplyHandler.php | 4 -- ...habricatorApplicationTransactionEditor.php | 4 -- 22 files changed, 6 insertions(+), 225 deletions(-) diff --git a/src/applications/audit/mail/PhabricatorAuditReplyHandler.php b/src/applications/audit/mail/PhabricatorAuditReplyHandler.php index db254fb4d1..7214943f76 100644 --- a/src/applications/audit/mail/PhabricatorAuditReplyHandler.php +++ b/src/applications/audit/mail/PhabricatorAuditReplyHandler.php @@ -22,14 +22,6 @@ final class PhabricatorAuditReplyHandler extends PhabricatorMailReplyHandler { 'metamta.diffusion.reply-handler-domain'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $commit = $this->getMailReceiver(); $actor = $this->getActor(); diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index 9339c571c3..550574a199 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -220,6 +220,8 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'security.allow-outbound-http' => pht( 'This option has been replaced with the more granular option '. '`security.outbound-blacklist`.'), + 'metamta.reply.show-hints' => pht( + 'Phabricator no longer shows reply hints in mail.'), ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php index 0a992d5ba9..7c0b990992 100644 --- a/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php +++ b/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php @@ -249,14 +249,6 @@ EODOC 'Domain used for reply email addresses. Some applications can '. 'override this configuration with a different domain.')) ->addExample('phabricator.example.com', ''), - $this->newOption('metamta.reply.show-hints', 'bool', true) - ->setBoolOptions( - array( - pht('Show Reply Handler Hints'), - pht('No Reply Handler Hints'), - )) - ->setSummary(pht('Show hints about reply handler actions in email.')) - ->setDescription($reply_hints_description), $this->newOption('metamta.herald.show-hints', 'bool', true) ->setBoolOptions( array( diff --git a/src/applications/conpherence/mail/ConpherenceReplyHandler.php b/src/applications/conpherence/mail/ConpherenceReplyHandler.php index c695cfbc39..5c55f7aa61 100644 --- a/src/applications/conpherence/mail/ConpherenceReplyHandler.php +++ b/src/applications/conpherence/mail/ConpherenceReplyHandler.php @@ -27,14 +27,6 @@ final class ConpherenceReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('Z'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment and attach files.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $conpherence = $this->getMailReceiver(); $user = $this->getActor(); diff --git a/src/applications/differential/mail/DifferentialReplyHandler.php b/src/applications/differential/mail/DifferentialReplyHandler.php index 7dd64b51c5..4a773dd2bc 100644 --- a/src/applications/differential/mail/DifferentialReplyHandler.php +++ b/src/applications/differential/mail/DifferentialReplyHandler.php @@ -29,53 +29,6 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler { 'metamta.differential.reply-handler-domain'); } - /* - * Generate text like the following from the supported commands. - * " - * - * ACTIONS - * Reply to comment, or !accept, !reject, !abandon, !resign, !reclaim. - * - * " - */ - public function getReplyHandlerInstructions() { - if (!$this->supportsReplies()) { - return null; - } - - $supported_commands = $this->getSupportedCommands(); - $text = ''; - if (empty($supported_commands)) { - return $text; - } - - $comment_command_printed = false; - if (in_array(DifferentialAction::ACTION_COMMENT, $supported_commands)) { - $text .= pht('Reply to comment'); - $comment_command_printed = true; - - $supported_commands = array_diff( - $supported_commands, array(DifferentialAction::ACTION_COMMENT)); - } - - if (!empty($supported_commands)) { - if ($comment_command_printed) { - $text .= ', or '; - } - - $modified_commands = array(); - foreach ($supported_commands as $command) { - $modified_commands[] = '!'.$command; - } - - $text .= implode(', ', $modified_commands); - } - - $text .= '.'; - - return $text; - } - public function getSupportedCommands() { $actions = array( DifferentialAction::ACTION_COMMENT, diff --git a/src/applications/files/mail/FileReplyHandler.php b/src/applications/files/mail/FileReplyHandler.php index 99da44afb3..aafd2fd43b 100644 --- a/src/applications/files/mail/FileReplyHandler.php +++ b/src/applications/files/mail/FileReplyHandler.php @@ -17,14 +17,6 @@ final class FileReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('F'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment or !unsubscribe.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $actor = $this->getActor(); $file = $this->getMailReceiver(); diff --git a/src/applications/fund/mail/FundInitiativeReplyHandler.php b/src/applications/fund/mail/FundInitiativeReplyHandler.php index a2bd2587df..0e5db2aab5 100644 --- a/src/applications/fund/mail/FundInitiativeReplyHandler.php +++ b/src/applications/fund/mail/FundInitiativeReplyHandler.php @@ -17,15 +17,6 @@ final class FundInitiativeReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('I'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - // TODO: Implement. - return null; - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // TODO: Implement. return null; diff --git a/src/applications/legalpad/mail/LegalpadReplyHandler.php b/src/applications/legalpad/mail/LegalpadReplyHandler.php index 08699d9bdd..9dd6cc8890 100644 --- a/src/applications/legalpad/mail/LegalpadReplyHandler.php +++ b/src/applications/legalpad/mail/LegalpadReplyHandler.php @@ -17,14 +17,6 @@ final class LegalpadReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('L'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment or !unsubscribe.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $actor = $this->getActor(); $document = $this->getMailReceiver(); diff --git a/src/applications/macro/mail/PhabricatorMacroReplyHandler.php b/src/applications/macro/mail/PhabricatorMacroReplyHandler.php index c2cf90352b..1b70b8872a 100644 --- a/src/applications/macro/mail/PhabricatorMacroReplyHandler.php +++ b/src/applications/macro/mail/PhabricatorMacroReplyHandler.php @@ -22,16 +22,6 @@ final class PhabricatorMacroReplyHandler extends PhabricatorMailReplyHandler { 'metamta.macro.reply-handler-domain'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - // TODO: Implement. - return null; - return pht('Reply to comment.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // TODO: Implement this. return null; diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php index 1c04093f4c..7f74c8a569 100644 --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -22,16 +22,6 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { 'metamta.maniphest.reply-handler-domain'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht( - 'Reply to comment or attach files, or !close, !claim, '. - '!unsubscribe or !assign .'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // NOTE: We'll drop in here on both the "reply to a task" and "create a // new task" workflows! Make sure you test both if you make changes! diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index a4d503cf8b..d3518fe65c 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -63,7 +63,6 @@ abstract class PhabricatorMailReplyHandler { 'metamta.reply-handler-domain'); } - abstract public function getReplyHandlerInstructions(); abstract protected function receiveEmail( PhabricatorMetaMTAReceivedMail $mail); diff --git a/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php index 8d6c82cfc2..03f89467f4 100644 --- a/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php +++ b/src/applications/metamta/view/PhabricatorMetaMTAMailBody.php @@ -159,26 +159,6 @@ final class PhabricatorMetaMTAMailBody { } - /** - * Add a section with reply handler instructions. - * - * @param string Reply handler instructions. - * @return this - * @task compose - */ - public function addReplySection($instructions) { - if (!PhabricatorEnv::getEnvConfig('metamta.reply.show-hints')) { - return $this; - } - if (!strlen($instructions)) { - return $this; - } - - $this->addTextSection(pht('REPLY HANDLER ACTIONS'), $instructions); - - return $this; - } - /** * Add a section with a link to email preferences. * @@ -197,6 +177,7 @@ final class PhabricatorMetaMTAMailBody { return $this; } + /** * Add an attachment. * diff --git a/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php index 021dd504dd..e8415c7675 100644 --- a/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php +++ b/src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php @@ -13,12 +13,9 @@ HEADER WHY DID I GET THIS EMAIL? http://test.com/xscript/ -REPLY HANDLER ACTIONS - pike - EOTEXT; - $this->assertEmail($expect, true, true); + $this->assertEmail($expect, true); } public function testBodyRenderNoHerald() { @@ -29,42 +26,20 @@ HEADER bass trout -REPLY HANDLER ACTIONS - pike - EOTEXT; - $this->assertEmail($expect, false, true); + $this->assertEmail($expect, false); } - - public function testBodyRenderNoReply() { - $expect = <<assertEmail($expect, true, false); - } - - private function assertEmail($expect, $herald_hints, $reply_hints) { + private function assertEmail($expect, $herald_hints) { $env = PhabricatorEnv::beginScopedEnv(); $env->overrideEnvConfig('phabricator.production-uri', 'http://test.com/'); $env->overrideEnvConfig('metamta.herald.show-hints', $herald_hints); - $env->overrideEnvConfig('metamta.reply.show-hints', $reply_hints); $body = new PhabricatorMetaMTAMailBody(); $body->addRawSection('salmon'); $body->addTextSection('HEADER', "bass\ntrout\n"); $body->addHeraldSection('/xscript/'); - $body->addReplySection('pike'); $this->assertEqual($expect, $body->render()); } diff --git a/src/applications/owners/mail/OwnersPackageReplyHandler.php b/src/applications/owners/mail/OwnersPackageReplyHandler.php index 5f2c7c5f12..3eb8a8a3c4 100644 --- a/src/applications/owners/mail/OwnersPackageReplyHandler.php +++ b/src/applications/owners/mail/OwnersPackageReplyHandler.php @@ -20,10 +20,6 @@ final class OwnersPackageReplyHandler extends PhabricatorMailReplyHandler { return null; } - public function getReplyHandlerInstructions() { - return null; - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { return; } diff --git a/src/applications/paste/mail/PasteReplyHandler.php b/src/applications/paste/mail/PasteReplyHandler.php index f94eceb09a..1cc5e91d70 100644 --- a/src/applications/paste/mail/PasteReplyHandler.php +++ b/src/applications/paste/mail/PasteReplyHandler.php @@ -17,14 +17,6 @@ final class PasteReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('P'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment or !unsubscribe.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $actor = $this->getActor(); $paste = $this->getMailReceiver(); diff --git a/src/applications/pholio/mail/PholioReplyHandler.php b/src/applications/pholio/mail/PholioReplyHandler.php index ce7578ff92..0e8249f78d 100644 --- a/src/applications/pholio/mail/PholioReplyHandler.php +++ b/src/applications/pholio/mail/PholioReplyHandler.php @@ -22,16 +22,6 @@ final class PholioReplyHandler extends PhabricatorMailReplyHandler { 'metamta.pholio.reply-handler-domain'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - // TODO: Implement. - return null; - return pht('Reply to comment.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // TODO: Implement this. return null; diff --git a/src/applications/phortune/mail/PhortuneCartReplyHandler.php b/src/applications/phortune/mail/PhortuneCartReplyHandler.php index c48d1ce264..09ae40f95c 100644 --- a/src/applications/phortune/mail/PhortuneCartReplyHandler.php +++ b/src/applications/phortune/mail/PhortuneCartReplyHandler.php @@ -17,15 +17,6 @@ final class PhortuneCartReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('CART'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - // TODO: Implement. - return null; - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // TODO: Implement. return null; diff --git a/src/applications/phriction/mail/PhrictionReplyHandler.php b/src/applications/phriction/mail/PhrictionReplyHandler.php index b458e13dc0..65df12aaf0 100644 --- a/src/applications/phriction/mail/PhrictionReplyHandler.php +++ b/src/applications/phriction/mail/PhrictionReplyHandler.php @@ -20,15 +20,6 @@ final class PhrictionReplyHandler extends PhabricatorMailReplyHandler { PhrictionDocumentPHIDType::TYPECONST); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - // TODO: Implement. - return null; - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // TODO: Implement. return null; diff --git a/src/applications/ponder/mail/PonderQuestionReplyHandler.php b/src/applications/ponder/mail/PonderQuestionReplyHandler.php index f5923f39dd..cc0ab8a939 100644 --- a/src/applications/ponder/mail/PonderQuestionReplyHandler.php +++ b/src/applications/ponder/mail/PonderQuestionReplyHandler.php @@ -17,10 +17,6 @@ final class PonderQuestionReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('Q'); } - public function getReplyHandlerInstructions() { - return null; - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { // ignore this entirely for now } diff --git a/src/applications/releeph/mail/ReleephRequestReplyHandler.php b/src/applications/releeph/mail/ReleephRequestReplyHandler.php index ec231b63c5..4c36078ea5 100644 --- a/src/applications/releeph/mail/ReleephRequestReplyHandler.php +++ b/src/applications/releeph/mail/ReleephRequestReplyHandler.php @@ -17,14 +17,6 @@ final class ReleephRequestReplyHandler extends PhabricatorMailReplyHandler { return $this->getDefaultPublicReplyHandlerEmailAddress('RERQ'); } - public function getReplyHandlerInstructions() { - if ($this->supportsReplies()) { - return pht('Reply to comment.'); - } else { - return null; - } - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $rq = $this->getMailReceiver(); $user = $this->getActor(); diff --git a/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php b/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php index 09f111868b..d902396d56 100644 --- a/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php +++ b/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php @@ -16,10 +16,6 @@ final class PhabricatorRepositoryPushReplyHandler return null; } - public function getReplyHandlerInstructions() { - return null; - } - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { return; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 19b94d9c5f..6da9c72d5a 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1983,10 +1983,6 @@ abstract class PhabricatorApplicationTransactionEditor $action = $this->getMailAction($object, $xactions); $reply_handler = $this->buildReplyHandler($object); - $reply_section = $reply_handler->getReplyHandlerInstructions(); - if ($reply_section !== null) { - $body->addReplySection($reply_section); - } $body->addEmailPreferenceSection();