From 4041a7e0f66406dbaa31faa3807d50833b7c7efd Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 11 Dec 2012 17:27:40 -0800 Subject: [PATCH] Add ApplicationTransaction handling for transactions with no effect Summary: When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets: - User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only. - A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case. - User is submitting one action, which isn't effective. Tell them they're out of luck. - A special case of this is an empty comment. We provide tailored text in this case. By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally. Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings. Test Plan: - Submitted empty and nonempy comments in Macro and Pholio. - Submitted comments with new and existing "@mentions". - Submitted edits in both applications. Reviewers: btrahan, vrana Reviewed By: btrahan CC: aran Maniphest Tasks: T912, T2104 Differential Revision: https://secure.phabricator.com/D4160 --- src/__phutil_library_map__.php | 4 + .../PhabricatorMacroCommentController.php | 12 +- .../PhabricatorMacroDisableController.php | 5 +- .../PhabricatorMacroEditController.php | 6 +- .../PholioMockCommentController.php | 21 ++-- .../controller/PholioMockEditController.php | 3 +- ...habricatorApplicationTransactionEditor.php | 107 +++++++++++++++--- ...pplicationTransactionNoEffectException.php | 37 ++++++ ...ApplicationTransactionNoEffectResponse.php | 78 +++++++++++++ .../PhabricatorApplicationTransaction.php | 22 ++++ .../PhabricatorBaseEnglishTranslation.php | 21 ++++ 11 files changed, 284 insertions(+), 32 deletions(-) create mode 100644 src/applications/transactions/exception/PhabricatorApplicationTransactionNoEffectException.php create mode 100644 src/applications/transactions/response/PhabricatorApplicationTransactionNoEffectResponse.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 53f4763678..0fc2fe7e95 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -608,6 +608,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionController' => 'applications/transactions/controller/PhabricatorApplicationTransactionController.php', 'PhabricatorApplicationTransactionEditor' => 'applications/transactions/editor/PhabricatorApplicationTransactionEditor.php', 'PhabricatorApplicationTransactionFeedStory' => 'applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php', + 'PhabricatorApplicationTransactionNoEffectException' => 'applications/transactions/exception/PhabricatorApplicationTransactionNoEffectException.php', + 'PhabricatorApplicationTransactionNoEffectResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionNoEffectResponse.php', 'PhabricatorApplicationTransactionQuery' => 'applications/transactions/query/PhabricatorApplicationTransactionQuery.php', 'PhabricatorApplicationTransactionResponse' => 'applications/transactions/response/PhabricatorApplicationTransactionResponse.php', 'PhabricatorApplicationTransactionView' => 'applications/transactions/view/PhabricatorApplicationTransactionView.php', @@ -1887,6 +1889,8 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionController' => 'PhabricatorController', 'PhabricatorApplicationTransactionEditor' => 'PhabricatorEditor', 'PhabricatorApplicationTransactionFeedStory' => 'PhabricatorFeedStory', + 'PhabricatorApplicationTransactionNoEffectException' => 'Exception', + 'PhabricatorApplicationTransactionNoEffectResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationTransactionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorApplicationTransactionResponse' => 'AphrontProxyResponse', 'PhabricatorApplicationTransactionView' => 'AphrontView', diff --git a/src/applications/macro/controller/PhabricatorMacroCommentController.php b/src/applications/macro/controller/PhabricatorMacroCommentController.php index a52e720fb7..9b4fe7763d 100644 --- a/src/applications/macro/controller/PhabricatorMacroCommentController.php +++ b/src/applications/macro/controller/PhabricatorMacroCommentController.php @@ -34,13 +34,21 @@ final class PhabricatorMacroCommentController $editor = id(new PhabricatorMacroEditor()) ->setActor($user) + ->setContinueOnNoEffect($request->isContinueRequest()) ->setContentSource( PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_WEB, array( 'ip' => $request->getRemoteAddr(), - ))) - ->applyTransactions($macro, $xactions); + ))); + + try { + $xactions = $editor->applyTransactions($macro, $xactions); + } catch (PhabricatorApplicationTransactionNoEffectException $ex) { + return id(new PhabricatorApplicationTransactionNoEffectResponse()) + ->setCancelURI($view_uri) + ->setException($ex); + } if ($request->isAjax()) { return id(new PhabricatorApplicationTransactionResponse()) diff --git a/src/applications/macro/controller/PhabricatorMacroDisableController.php b/src/applications/macro/controller/PhabricatorMacroDisableController.php index 474285d77e..8be2409068 100644 --- a/src/applications/macro/controller/PhabricatorMacroDisableController.php +++ b/src/applications/macro/controller/PhabricatorMacroDisableController.php @@ -32,8 +32,9 @@ final class PhabricatorMacroDisableController PhabricatorContentSource::SOURCE_WEB, array( 'ip' => $request->getRemoteAddr(), - ))) - ->applyTransactions($macro, array($xaction)); + ))); + + $xactions = $editor->applyTransactions($macro, array($xaction)); return id(new AphrontRedirectResponse())->setURI($view_uri); } diff --git a/src/applications/macro/controller/PhabricatorMacroEditController.php b/src/applications/macro/controller/PhabricatorMacroEditController.php index ff9f9fcbf3..129dd26b7e 100644 --- a/src/applications/macro/controller/PhabricatorMacroEditController.php +++ b/src/applications/macro/controller/PhabricatorMacroEditController.php @@ -96,13 +96,15 @@ final class PhabricatorMacroEditController $editor = id(new PhabricatorMacroEditor()) ->setActor($user) + ->setContinueOnNoEffect(true) ->setContentSource( PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_WEB, array( 'ip' => $request->getRemoteAddr(), - ))) - ->applyTransactions($original, $xactions); + ))); + + $xactions = $editor->applyTransactions($original, $xactions); $view_uri = $this->getApplicationURI('/view/'.$original->getID().'/'); return id(new AphrontRedirectResponse())->setURI($view_uri); diff --git a/src/applications/pholio/controller/PholioMockCommentController.php b/src/applications/pholio/controller/PholioMockCommentController.php index 950332af70..cf1537f4e4 100644 --- a/src/applications/pholio/controller/PholioMockCommentController.php +++ b/src/applications/pholio/controller/PholioMockCommentController.php @@ -27,15 +27,6 @@ final class PholioMockCommentController extends PholioController { $mock_uri = '/M'.$mock->getID(); $comment = $request->getStr('comment'); - if (!strlen($comment)) { - $dialog = id(new AphrontDialogView()) - ->setUser($user) - ->setTitle(pht('Empty Comment')) - ->appendChild('You did not provide a comment!') - ->addCancelButton($mock_uri); - - return id(new AphrontDialogResponse())->setDialog($dialog); - } $content_source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_WEB, @@ -50,10 +41,18 @@ final class PholioMockCommentController extends PholioController { id(new PholioTransactionComment()) ->setContent($comment)); - id(new PholioMockEditor()) + $editor = id(new PholioMockEditor()) ->setActor($user) ->setContentSource($content_source) - ->applyTransactions($mock, $xactions); + ->setContinueOnException($request->isContinueRequest()); + + try { + $xactions = $editor->applyTransactions($mock, $xactions); + } catch (PhabricatorApplicationTransactionNoEffectException $ex) { + return id(new PhabricatorApplicationTransactionNoEffectResponse()) + ->setCancelURI($mock_uri) + ->setException($ex); + } if ($request->isAjax()) { return id(new PhabricatorApplicationTransactionResponse()) diff --git a/src/applications/pholio/controller/PholioMockEditController.php b/src/applications/pholio/controller/PholioMockEditController.php index 5f51366dd4..10d66fc652 100644 --- a/src/applications/pholio/controller/PholioMockEditController.php +++ b/src/applications/pholio/controller/PholioMockEditController.php @@ -120,9 +120,10 @@ final class PholioMockEditController extends PholioController { $mock->openTransaction(); $editor = id(new PholioMockEditor()) ->setContentSource($content_source) + ->setContinueOnException(true) ->setActor($user); - $editor->applyTransactions($mock, $xactions); + $xactions = $editor->applyTransactions($mock, $xactions); if ($images) { foreach ($images as $image) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index cf386a5303..5edd417cfe 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -13,6 +13,30 @@ abstract class PhabricatorApplicationTransactionEditor private $isNewObject; private $mentionedPHIDs; + private $continueOnNoEffect; + + + /** + * When the editor tries to apply transactions that have no effect, should + * it raise an exception (default) or drop them and continue? + * + * Generally, you will set this flag for edits coming from "Edit" interfaces, + * and leave it cleared for edits coming from "Comment" interfaces, so the + * user will get a useful error if they try to submit a comment that does + * nothing (e.g., empty comment with a status change that has already been + * performed by another user). + * + * @param bool True to drop transactions without effect and continue. + * @return this + */ + public function setContinueOnNoEffect($continue) { + $this->continueOnNoEffect = $continue; + return $this; + } + + public function getContinueOnNoEffect() { + return $this->continueOnNoEffect; + } protected function getIsNewObject() { return $this->isNewObject; @@ -94,6 +118,12 @@ abstract class PhabricatorApplicationTransactionEditor protected function transactionHasEffect( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + return $xaction->hasComment(); + } + return ($xaction->getOldValue() !== $xaction->getNewValue()); } @@ -192,19 +222,7 @@ abstract class PhabricatorApplicationTransactionEditor $this->adjustTransactionValues($object, $xaction); } - foreach ($xactions as $key => $xaction) { - if (!$this->transactionHasEffect($object, $xaction)) { - // TODO: Raise these to the user. - if ($xaction->getComment()) { - $xaction->setTransactionType( - PhabricatorTransactions::TYPE_COMMENT); - $xaction->setOldValue(null); - $xaction->setNewValue(null); - } else { - unset($xactions[$key]); - } - } - } + $xactions = $this->filterTransactions($object, $xactions); if (!$xactions) { return $this; @@ -262,7 +280,7 @@ abstract class PhabricatorApplicationTransactionEditor $this->didApplyTransactions($object, $xactions); - return $this; + return $xactions; } private function loadHandles(array $xactions) { @@ -370,6 +388,19 @@ abstract class PhabricatorApplicationTransactionEditor return null; } + if ($object->getPHID()) { + // Don't try to subscribe already-subscribed mentions: we want to generate + // a dialog about an action having no effect if the user explicitly adds + // existing CCs, but not if they merely mention existing subscribers. + $current = PhabricatorSubscribersQuery::loadSubscribersForPHID( + $object->getPHID()); + $phids = array_diff($phids, $current); + } + + if (!$phids) { + return null; + } + $xaction = newv(get_class(head($xactions)), array()); $xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); $xaction->setNewValue(array('+' => $phids)); @@ -530,6 +561,54 @@ abstract class PhabricatorApplicationTransactionEditor } + protected function filterTransactions( + PhabricatorLiskDAO $object, + array $xactions) { + + $type_comment = PhabricatorTransactions::TYPE_COMMENT; + + $no_effect = array(); + $has_comment = false; + $any_effect = false; + foreach ($xactions as $key => $xaction) { + if ($this->transactionHasEffect($object, $xaction)) { + if ($xaction->getTransactionType() != $type_comment) { + $any_effect = true; + } + } else { + $no_effect[$key] = $xaction; + } + if ($xaction->hasComment()) { + $has_comment = true; + } + } + + if (!$no_effect) { + return $xactions; + } + + if (!$this->getContinueOnNoEffect()) { + throw new PhabricatorApplicationTransactionNoEffectException( + $no_effect, + $any_effect, + $has_comment); + } + + foreach ($no_effect as $key => $xaction) { + if ($xaction->getComment()) { + $xaction->setTransactionType($type_comment); + $xaction->setOldValue(null); + $xaction->setNewValue(null); + } else { + unset($xactions[$key]); + } + } + + return $xactions; + } + + + /* -( Sending Mail )------------------------------------------------------- */ diff --git a/src/applications/transactions/exception/PhabricatorApplicationTransactionNoEffectException.php b/src/applications/transactions/exception/PhabricatorApplicationTransactionNoEffectException.php new file mode 100644 index 0000000000..dcccd84466 --- /dev/null +++ b/src/applications/transactions/exception/PhabricatorApplicationTransactionNoEffectException.php @@ -0,0 +1,37 @@ +transactions = $transactions; + $this->anyEffect = $any_effect; + + $message = array(); + $message[] = 'Transactions have no effect:'; + foreach ($this->transactions as $transaction) { + $message[] = ' - '.$transaction->getNoEffectDescription(); + } + + parent::__construct(implode("\n", $message)); + } + + public function getTransactions() { + return $this->transactions; + } + + public function hasAnyEffect() { + return $this->anyEffect; + } + + public function hasComment() { + return $this->hasComment; + } + +} diff --git a/src/applications/transactions/response/PhabricatorApplicationTransactionNoEffectResponse.php b/src/applications/transactions/response/PhabricatorApplicationTransactionNoEffectResponse.php new file mode 100644 index 0000000000..9c1caab76e --- /dev/null +++ b/src/applications/transactions/response/PhabricatorApplicationTransactionNoEffectResponse.php @@ -0,0 +1,78 @@ +cancelURI = $cancel_uri; + return $this; + } + + public function setException( + PhabricatorApplicationTransactionNoEffectException $exception) { + $this->exception = $exception; + return $this; + } + + protected function buildProxy() { + return new AphrontDialogResponse(); + } + + public function reduceProxyResponse() { + $request = $this->getRequest(); + + $ex = $this->exception; + $xactions = $ex->getTransactions(); + + $type_comment = PhabricatorTransactions::TYPE_COMMENT; + $only_empty_comment = (count($xactions) == 1) && + (head($xactions)->getTransactionType() == $type_comment); + + if ($ex->hasAnyEffect()) { + $title = pht('%d Action(s) With No Effect', count($xactions)); + $tail = pht('Apply remaining actions?'); + $continue = pht('Apply Other Actions'); + } else if ($ex->hasComment()) { + $title = pht('Post as Comment'); + $tail = pht('Do you want to post your comment anyway?'); + $continue = pht('Post Comment'); + } else if ($only_empty_comment) { + // Special case this since it's common and we can give the user a nicer + // dialog than "Action Has No Effect". + $title = pht('Empty Comment'); + $tail = null; + $continue = null; + } else { + $title = pht('%d Action(s) Have No Effect', count($xactions)); + $tail = null; + $continue = null; + } + + $dialog = id(new AphrontDialogView()) + ->setUser($request->getUser()) + ->setTitle($title); + + foreach ($xactions as $xaction) { + $dialog->appendChild('

'.$xaction->getNoEffectDescription().'

'); + } + $dialog->appendChild($tail); + + if ($continue) { + $passthrough = $request->getPassthroughRequestParameters(); + foreach ($passthrough as $key => $value) { + $dialog->addHiddenInput($key, $value); + } + $dialog->addHiddenInput('__continue__', 1); + $dialog->addSubmitButton($continue); + } + + $dialog->addCancelButton($this->cancelURI); + + return $this->getProxy()->setDialog($dialog); + } + +} diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 15f9d81fc4..053ace5e7a 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -170,6 +170,28 @@ abstract class PhabricatorApplicationTransaction return false; } + public function getNoEffectDescription() { + + switch ($this->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + return pht('You can not post an empty comment.'); + case PhabricatorTransactions::TYPE_VIEW_POLICY: + return pht( + 'This %s already has that view policy.', + $this->getApplicationObjectTypeName()); + case PhabricatorTransactions::TYPE_EDIT_POLICY: + return pht( + 'This %s already has that edit policy.', + $this->getApplicationObjectTypeName()); + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + return pht( + 'All users are already subscribed to this %s.', + $this->getApplicationObjectTypeName()); + } + + return pht('Transaction has no effect.'); + } + public function getTitle() { $author_phid = $this->getAuthorPHID(); diff --git a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php index f074aec819..dc0197fca3 100644 --- a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php @@ -172,6 +172,27 @@ abstract class PhabricatorBaseEnglishTranslation 'This is a binary file. It is %2$s bytes in length.', ), + '%d Action(s) Have No Effect' => array( + 'Action Has No Effect', + 'Actions Have No Effect', + ), + + '%d Action(s) With No Effect' => array( + 'Action With No Effect', + 'Actions With No Effect', + ), + + '%s added %d subscriber(s): %s.' => array( + array( + '%s added a subscriber: %3$s.', + '%s added subscribers: %3$s.', + ), + array( + '%s added a subscriber: %3$s.', + '%s added subscribers: %3$s.', + ), + ), + ); }