From df8d4dff679b21573ac89adf6b7766f97ec4decb Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Mar 2018 16:01:46 -0700 Subject: [PATCH] Raise a warning when mentioning a user in a comment on a draft revision Summary: See PHI433. Ref T13102. Users in the wild have mixed expecations about exactly what "draft" means. Recent changes have tried to make behavior more clear. As part of clarifying messaging, make it explicit that `@mention` does not work on drafts by showing users a warning when they try to `@mention` a user. Test Plan: - Mentioned users on drafts, got a warning. - Posted normal comments on drafts, no warning. - Posted normal/mention comments on non-drafts, no warning. Maniphest Tasks: T13102 Differential Revision: https://secure.phabricator.com/D19210 --- src/__phutil_library_map__.php | 4 ++ .../editengine/PhabricatorEditEngine.php | 5 ++ ...habricatorApplicationTransactionEditor.php | 47 +++++++++++++++ ...ApplicationTransactionWarningException.php | 13 +++++ ...rApplicationTransactionWarningResponse.php | 58 +++++++++++++++++++ 5 files changed, 127 insertions(+) create mode 100644 src/applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php create mode 100644 src/applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7dc658b28a..4caa7f563d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2055,6 +2055,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionValidationResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionValidationResponse.php', 'PhabricatorApplicationTransactionValueController' => 'applications/transactions/controller/PhabricatorApplicationTransactionValueController.php', 'PhabricatorApplicationTransactionView' => 'applications/transactions/view/PhabricatorApplicationTransactionView.php', + 'PhabricatorApplicationTransactionWarningException' => 'applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php', + 'PhabricatorApplicationTransactionWarningResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php', 'PhabricatorApplicationUninstallController' => 'applications/meta/controller/PhabricatorApplicationUninstallController.php', 'PhabricatorApplicationsApplication' => 'applications/meta/application/PhabricatorApplicationsApplication.php', 'PhabricatorApplicationsController' => 'applications/meta/controller/PhabricatorApplicationsController.php', @@ -7484,6 +7486,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionValidationResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationTransactionValueController' => 'PhabricatorApplicationTransactionController', 'PhabricatorApplicationTransactionView' => 'AphrontView', + 'PhabricatorApplicationTransactionWarningException' => 'Exception', + 'PhabricatorApplicationTransactionWarningResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationUninstallController' => 'PhabricatorApplicationsController', 'PhabricatorApplicationsApplication' => 'PhabricatorApplication', 'PhabricatorApplicationsController' => 'PhabricatorController', diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index de8139b145..f0d2aae9ea 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1968,6 +1968,7 @@ abstract class PhabricatorEditEngine ->setContinueOnNoEffect($request->isContinueRequest()) ->setContinueOnMissingFields(true) ->setContentSourceFromRequest($request) + ->setRaiseWarnings(!$request->getBool('editEngine.warnings')) ->setIsPreview($is_preview); try { @@ -1980,6 +1981,10 @@ abstract class PhabricatorEditEngine return id(new PhabricatorApplicationTransactionNoEffectResponse()) ->setCancelURI($view_uri) ->setException($ex); + } catch (PhabricatorApplicationTransactionWarningException $ex) { + return id(new PhabricatorApplicationTransactionWarningResponse()) + ->setCancelURI($view_uri) + ->setException($ex); } if (!$is_preview) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 1d02d0fdb0..26be0f87af 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -48,6 +48,7 @@ abstract class PhabricatorApplicationTransactionEditor private $mentionedPHIDs; private $continueOnNoEffect; private $continueOnMissingFields; + private $raiseWarnings; private $parentMessageID; private $heraldAdapter; private $heraldTranscript; @@ -273,6 +274,15 @@ abstract class PhabricatorApplicationTransactionEditor return $this->applicationEmail; } + public function setRaiseWarnings($raise_warnings) { + $this->raiseWarnings = $raise_warnings; + return $this; + } + + public function getRaiseWarnings() { + return $this->raiseWarnings; + } + public function getTransactionTypesForObject($object) { $old = $this->object; try { @@ -919,6 +929,19 @@ abstract class PhabricatorApplicationTransactionEditor throw new PhabricatorApplicationTransactionValidationException($errors); } + if ($this->raiseWarnings) { + $warnings = array(); + foreach ($xactions as $xaction) { + if ($this->hasWarnings($object, $xaction)) { + $warnings[] = $xaction; + } + } + if ($warnings) { + throw new PhabricatorApplicationTransactionWarningException( + $warnings); + } + } + $this->willApplyTransactions($object, $xactions); if ($object->getID()) { @@ -4277,4 +4300,28 @@ abstract class PhabricatorApplicationTransactionEditor } } + private function hasWarnings($object, $xaction) { + // TODO: For the moment, this is a very un-modular hack to support + // exactly one type of warning (mentioning users on a draft revision) + // that we want to show. See PHI433. + + if (!($object instanceof DifferentialRevision)) { + return false; + } + + if (!$object->isDraft()) { + return false; + } + + $type = $xaction->getTransactionType(); + if ($type != PhabricatorTransactions::TYPE_SUBSCRIBERS) { + return false; + } + + // NOTE: This will currently warn even if you're only removing + // subscribers. + + return true; + } + } diff --git a/src/applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php b/src/applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php new file mode 100644 index 0000000000..e577071549 --- /dev/null +++ b/src/applications/transactions/exception/PhabricatorApplicationTransactionWarningException.php @@ -0,0 +1,13 @@ +xactions = $xactions; + parent::__construct(); + } + +} diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php new file mode 100644 index 0000000000..21b3ee61c8 --- /dev/null +++ b/src/applications/transactions/response/PhabricatorApplicationTransactionWarningResponse.php @@ -0,0 +1,58 @@ +cancelURI = $cancel_uri; + return $this; + } + + public function setException( + PhabricatorApplicationTransactionWarningException $exception) { + $this->exception = $exception; + return $this; + } + + protected function buildProxy() { + return new AphrontDialogResponse(); + } + + public function reduceProxyResponse() { + $request = $this->getRequest(); + + $title = pht('Warning: Unexpected Effects'); + + $head = pht( + 'This is a draft revision that will not publish any notifications '. + 'until the author requests review.'); + $tail = pht( + 'Mentioned or subscribed users will not be notified.'); + + $continue = pht('Tell No One'); + + $dialog = id(new AphrontDialogView()) + ->setViewer($request->getViewer()) + ->setTitle($title); + + $dialog->appendParagraph($head); + $dialog->appendParagraph($tail); + + $passthrough = $request->getPassthroughRequestParameters(); + foreach ($passthrough as $key => $value) { + $dialog->addHiddenInput($key, $value); + } + + $dialog + ->addHiddenInput('editEngine.warnings', 1) + ->addSubmitButton($continue) + ->addCancelButton($this->cancelURI); + + return $this->getProxy()->setDialog($dialog); + } + +}