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.', + ), + ), + ); }