From d8bbf559592f3509c680d6094c2889f51ca6ffc7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 14 Jan 2012 11:39:22 -0800 Subject: [PATCH] Improve behavior when user submits a no-op action in Differential Summary: See T730 and the slightly-less-pretty version of this in D1398. When a user takes an action in Differential that has no effect (for instance, accepting an already-accepted revision), prompt them: Action Has No Effect You can not accept this revision because it has already been accepted. Do you want to post the feedback anyway, as a normal comment? [Cancel] [Post as Comment] If they have no comment text, the dialog only says "Cancel". I think this is probably the best way to balance all the concerns here -- it might occasionally be a little annoying, but that should be rare, and it should never be confusing (the current workflow is extremely confusing). This also fixes the issue where you can add all sorts of CCs who are already part of the revision, either explicitly or via mentions. Test Plan: Posted some has-effect and has-no-effect comments, made different choices in the dialog, everything seems to work OK? Reviewers: vrana, btrahan, jungejason Reviewed By: vrana CC: aran, vrana Maniphest Tasks: T730 Differential Revision: https://secure.phabricator.com/D1403 --- src/__phutil_library_map__.php | 3 + .../DifferentialCommentSaveController.php | 45 +++- .../controller/commentsave/__init__.php | 4 + .../comment/DifferentialCommentEditor.php | 249 +++++++++++++----- .../differential/editor/comment/__init__.php | 1 + .../exception/base/DifferentialException.php | 21 ++ .../differential/exception/base/__init__.php | 10 + ...DifferentialActionHasNoEffectException.php | 24 ++ .../exception/noeffect/__init__.php | 12 + .../addcomment/DifferentialAddCommentView.php | 1 + src/view/dialog/AphrontDialogView.php | 10 +- 11 files changed, 298 insertions(+), 82 deletions(-) create mode 100644 src/applications/differential/exception/base/DifferentialException.php create mode 100644 src/applications/differential/exception/base/__init__.php create mode 100644 src/applications/differential/exception/noeffect/DifferentialActionHasNoEffectException.php create mode 100644 src/applications/differential/exception/noeffect/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 34c2578e90..34e8f179ca 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -160,6 +160,7 @@ phutil_register_library_map(array( 'DarkConsoleXHProfPluginAPI' => 'aphront/console/plugin/xhprof/api', 'DatabaseConfigurationProvider' => 'applications/base/storage/configuration', 'DifferentialAction' => 'applications/differential/constants/action', + 'DifferentialActionHasNoEffectException' => 'applications/differential/exception/noeffect', 'DifferentialAddCommentView' => 'applications/differential/view/addcomment', 'DifferentialAffectedPath' => 'applications/differential/storage/affectedpath', 'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch', @@ -191,6 +192,7 @@ phutil_register_library_map(array( 'DifferentialDiffProperty' => 'applications/differential/storage/diffproperty', 'DifferentialDiffTableOfContentsView' => 'applications/differential/view/difftableofcontents', 'DifferentialDiffViewController' => 'applications/differential/controller/diffview', + 'DifferentialException' => 'applications/differential/exception/base', 'DifferentialExceptionMail' => 'applications/differential/mail/exception', 'DifferentialExportPatchFieldSpecification' => 'applications/differential/field/specification/exportpatch', 'DifferentialFieldDataNotAvailableException' => 'applications/differential/field/exception/notavailable', @@ -908,6 +910,7 @@ phutil_register_library_map(array( 'DarkConsoleRequestPlugin' => 'DarkConsolePlugin', 'DarkConsoleServicesPlugin' => 'DarkConsolePlugin', 'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin', + 'DifferentialActionHasNoEffectException' => 'DifferentialException', 'DifferentialAddCommentView' => 'AphrontView', 'DifferentialAffectedPath' => 'DifferentialDAO', 'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification', diff --git a/src/applications/differential/controller/commentsave/DifferentialCommentSaveController.php b/src/applications/differential/controller/commentsave/DifferentialCommentSaveController.php index 4363f666cd..a26875d61d 100644 --- a/src/applications/differential/controller/commentsave/DifferentialCommentSaveController.php +++ b/src/applications/differential/controller/commentsave/DifferentialCommentSaveController.php @@ -46,13 +46,44 @@ class DifferentialCommentSaveController extends DifferentialController { 'ip' => $request->getRemoteAddr(), )); - $editor - ->setMessage($comment) - ->setContentSource($content_source) - ->setAttachInlineComments(true) - ->setAddedReviewers($reviewers) - ->setAddedCCs($ccs) - ->save(); + try { + $editor + ->setMessage($comment) + ->setContentSource($content_source) + ->setAttachInlineComments(true) + ->setAddedReviewers($reviewers) + ->setAddedCCs($ccs) + ->save(); + } catch (DifferentialActionHasNoEffectException $no_effect) { + $has_inlines = id(new DifferentialInlineComment())->loadAllWhere( + 'authorPHID = %s AND revisionID = %d AND commentID IS NULL', + $request->getUser()->getPHID(), + $revision->getID()); + + $dialog = new AphrontDialogView(); + $dialog->setUser($request->getUser()); + $dialog->addCancelButton('/D'.$revision_id); + + $dialog->addHiddenInput('revision_id', $revision_id); + $dialog->addHiddenInput('action', 'none'); + $dialog->addHiddenInput('reviewers', $reviewers); + $dialog->addHiddenInput('ccs', $ccs); + $dialog->addHiddenInput('comment', $comment); + + $dialog->setTitle('Action Has No Effect'); + $dialog->appendChild( + '

'.phutil_escape_html($no_effect->getMessage()).'

'); + + if (strlen($comment) || $has_inlines) { + $dialog->addSubmitButton('Post as Comment'); + $dialog->appendChild('
'); + $dialog->appendChild( + '

Do you want to post your feedback anyway, as a normal '. + 'comment?

'); + } + + return id(new AphrontDialogResponse())->setDialog($dialog); + } // TODO: Diff change detection? diff --git a/src/applications/differential/controller/commentsave/__init__.php b/src/applications/differential/controller/commentsave/__init__.php index e0f7759809..b0d8846cfe 100644 --- a/src/applications/differential/controller/commentsave/__init__.php +++ b/src/applications/differential/controller/commentsave/__init__.php @@ -7,13 +7,17 @@ phutil_require_module('phabricator', 'aphront/response/400'); +phutil_require_module('phabricator', 'aphront/response/dialog'); phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/differential/controller/base'); phutil_require_module('phabricator', 'applications/differential/editor/comment'); +phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment'); phutil_require_module('phabricator', 'applications/differential/storage/revision'); phutil_require_module('phabricator', 'applications/draft/storage/draft'); phutil_require_module('phabricator', 'applications/metamta/contentsource/source'); +phutil_require_module('phabricator', 'view/dialog'); +phutil_require_module('phutil', 'markup'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/differential/editor/comment/DifferentialCommentEditor.php b/src/applications/differential/editor/comment/DifferentialCommentEditor.php index afcd5de26e..bb9625f4a1 100644 --- a/src/applications/differential/editor/comment/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/comment/DifferentialCommentEditor.php @@ -105,40 +105,58 @@ class DifferentialCommentEditor { $metadata = array(); + $inline_comments = array(); + if ($this->attachInlineComments) { + $inline_comments = id(new DifferentialInlineComment())->loadAllWhere( + 'authorPHID = %s AND revisionID = %d AND commentID IS NULL', + $this->actorPHID, + $revision->getID()); + } + switch ($action) { case DifferentialAction::ACTION_COMMENT: + if (!$this->message && !$inline_comments) { + throw new DifferentialActionHasNoEffectException( + "You are submitting an empty comment with no action: ". + "you must act on the revision or post a comment."); + } break; case DifferentialAction::ACTION_RESIGN: if ($actor_is_author) { throw new Exception('You can not resign from your own revision!'); } - if (isset($reviewer_phids[$actor_phid])) { - DifferentialRevisionEditor::alterReviewers( - $revision, - $reviewer_phids, - $rem = array($actor_phid), - $add = array(), - $actor_phid); + if (empty($reviewer_phids[$actor_phid])) { + throw new DifferentialActionHasNoEffectException( + "You can not resign from this revision because you are not ". + "a reviewer."); } + DifferentialRevisionEditor::alterReviewers( + $revision, + $reviewer_phids, + $rem = array($actor_phid), + $add = array(), + $actor_phid); break; case DifferentialAction::ACTION_ABANDON: if (!($actor_is_author || $actor_is_admin)) { - throw new Exception('You can only abandon your revisions.'); - } - if ($revision_status == - ArcanistDifferentialRevisionStatus::COMMITTED) { - throw new Exception('You can not abandon a committed revision.'); - } - if ($revision_status == - ArcanistDifferentialRevisionStatus::ABANDONED) { - $action = DifferentialAction::ACTION_COMMENT; - break; + throw new Exception('You can only abandon your own revisions.'); } - $revision - ->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED); + if ($revision_status == ArcanistDifferentialRevisionStatus::COMMITTED) { + throw new DifferentialActionHasNoEffectException( + "You can not abandon this revision because it has already ". + "been committed."); + } + + if ($revision_status == ArcanistDifferentialRevisionStatus::ABANDONED) { + throw new DifferentialActionHasNoEffectException( + "You can not abandon this revision because it has already ". + "been abandoned."); + } + + $revision->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED); break; case DifferentialAction::ACTION_ACCEPT: @@ -149,8 +167,24 @@ class DifferentialCommentEditor { ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) && ($revision_status != ArcanistDifferentialRevisionStatus::NEEDS_REVISION)) { - $action = DifferentialAction::ACTION_COMMENT; - break; + + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + throw new DifferentialActionHasNoEffectException( + "You can not accept this revision because someone else ". + "already accepted it."); + case ArcanistDifferentialRevisionStatus::ABANDONED: + throw new DifferentialActionHasNoEffectException( + "You can not accept this revision because it has been ". + "abandoned."); + case ArcanistDifferentialRevisionStatus::COMMITTED: + throw new DifferentialActionHasNoEffectException( + "You can not accept this revision because it has already ". + "been committed."); + default: + throw new Exception( + "Unexpected revision state '{$revision_status}'!"); + } } $revision @@ -170,16 +204,30 @@ class DifferentialCommentEditor { if (!$actor_is_author) { throw new Exception('You must own a revision to request review.'); } - if (($revision_status != - ArcanistDifferentialRevisionStatus::NEEDS_REVISION) && - ($revision_status != - ArcanistDifferentialRevisionStatus::ACCEPTED)) { - $action = DifferentialAction::ACTION_COMMENT; - break; + + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + $revision->setStatus( + ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); + break; + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + throw new DifferentialActionHasNoEffectException( + "You can not request review of this revision because it has ". + "been abandoned."); + case ArcanistDifferentialRevisionStatus::ABANDONED: + throw new DifferentialActionHasNoEffectException( + "You can not request review of this revision because it has ". + "been abandoned."); + case ArcanistDifferentialRevisionStatus::COMMITTED: + throw new DifferentialActionHasNoEffectException( + "You can not request review of this revision because it has ". + "already been committed."); + default: + throw new Exception( + "Unexpected revision state '{$revision_status}'!"); } - $revision - ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); break; case DifferentialAction::ACTION_REJECT: @@ -187,12 +235,26 @@ class DifferentialCommentEditor { throw new Exception( 'You can not request changes to your own revision.'); } - if (($revision_status != - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) && - ($revision_status != - ArcanistDifferentialRevisionStatus::ACCEPTED)) { - $action = DifferentialAction::ACTION_COMMENT; - break; + + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + // NOTE: We allow you to reject an already-rejected revision + // because it doesn't create any ambiguity and avoids a rather + // needless dialog. + break; + case ArcanistDifferentialRevisionStatus::ABANDONED: + throw new DifferentialActionHasNoEffectException( + "You can not request changes to this revision because it has ". + "been abandoned."); + case ArcanistDifferentialRevisionStatus::COMMITTED: + throw new DifferentialActionHasNoEffectException( + "You can not request changes to this revision because it has ". + "already been committed."); + default: + throw new Exception( + "Unexpected revision state '{$revision_status}'!"); } if (!isset($reviewer_phids[$actor_phid])) { @@ -213,12 +275,23 @@ class DifferentialCommentEditor { throw new Exception( "You can not plan changes to somebody else's revision"); } - if (($revision_status != - ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) && - ($revision_status != - ArcanistDifferentialRevisionStatus::ACCEPTED)) { - $action = DifferentialAction::ACTION_COMMENT; - break; + + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + break; + case ArcanistDifferentialRevisionStatus::ABANDONED: + throw new DifferentialActionHasNoEffectException( + "You can not plan changes to this revision because it has ". + "been abandoned."); + case ArcanistDifferentialRevisionStatus::COMMITTED: + throw new DifferentialActionHasNoEffectException( + "You can not plan changes to this revision because it has ". + "already been committed."); + default: + throw new Exception( + "Unexpected revision state '{$revision_status}'!"); } $revision @@ -229,11 +302,13 @@ class DifferentialCommentEditor { if (!$actor_is_author) { throw new Exception('You can not reclaim a revision you do not own.'); } - if ($revision_status != - ArcanistDifferentialRevisionStatus::ABANDONED) { - $action = DifferentialAction::ACTION_COMMENT; - break; + + + if ($revision_status != ArcanistDifferentialRevisionStatus::ABANDONED) { + throw new DifferentialActionHasNoEffectException( + "You can not reclaim this revision because it is not abandoned."); } + $revision ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); break; @@ -245,6 +320,8 @@ class DifferentialCommentEditor { case DifferentialAction::ACTION_ADDREVIEWERS: $added_reviewers = $this->getAddedReviewers(); + $user_tried_to_add = count($added_reviewers); + foreach ($added_reviewers as $k => $user_phid) { if ($user_phid == $revision->getAuthorPHID()) { unset($added_reviewers[$k]); @@ -268,21 +345,27 @@ class DifferentialCommentEditor { $metadata[$key] = $added_reviewers; } else { - $action = DifferentialAction::ACTION_COMMENT; + if ($user_tried_to_add == 0) { + throw new DifferentialActionHasNoEffectException( + "You can not add reviewers, because you did not specify any ". + "reviewers."); + } else if ($user_tried_to_add == 1) { + throw new DifferentialActionHasNoEffectException( + "You can not add that reviewer, because they are already an ". + "author or reviewer."); + } else { + throw new DifferentialActionHasNoEffectException( + "You can not add those reviewers, because they are all already ". + "authors or reviewers."); + } } + break; case DifferentialAction::ACTION_ADDCCS: $added_ccs = $this->getAddedCCs(); + $user_tried_to_add = count($added_ccs); - $current_ccs = $revision->getCCPHIDs(); - if ($current_ccs) { - $current_ccs = array_fill_keys($current_ccs, true); - foreach ($added_ccs as $k => $cc) { - if (isset($current_ccs[$cc])) { - unset($added_ccs[$k]); - } - } - } + $added_ccs = $this->filterAddedCCs($added_ccs); if ($added_ccs) { foreach ($added_ccs as $cc) { @@ -296,7 +379,19 @@ class DifferentialCommentEditor { $metadata[$key] = $added_ccs; } else { - $action = DifferentialAction::ACTION_COMMENT; + if ($user_tried_to_add == 0) { + throw new DifferentialActionHasNoEffectException( + "You can not add CCs, because you did not specify any ". + "CCs."); + } else if ($user_tried_to_add == 1) { + throw new DifferentialActionHasNoEffectException( + "You can not add that CC, because they are already an ". + "author, reviewer or CC."); + } else { + throw new DifferentialActionHasNoEffectException( + "You can not add those CCs, because they are all already ". + "authors, reviewers or CCs."); + } } break; default: @@ -318,14 +413,6 @@ class DifferentialCommentEditor { $this->actorPHID); } - $inline_comments = array(); - if ($this->attachInlineComments) { - $inline_comments = id(new DifferentialInlineComment())->loadAllWhere( - 'authorPHID = %s AND revisionID = %d AND commentID IS NULL', - $this->actorPHID, - $revision->getID()); - } - $comment = id(new DifferentialComment()) ->setAuthorPHID($this->actorPHID) ->setRevisionID($revision->getID()) @@ -362,15 +449,7 @@ class DifferentialCommentEditor { $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions( $content_blocks); if ($mention_ccs) { - $current_ccs = $revision->getCCPHIDs(); - if ($current_ccs) { - $current_ccs = array_fill_keys($current_ccs, true); - foreach ($mention_ccs as $key => $mention_cc) { - if (isset($current_ccs[$mention_cc])) { - unset($mention_ccs[$key]); - } - } - } + $mention_ccs = $this->filterAddedCCs($mention_ccs); if ($mention_ccs) { $metadata = $comment->getMetadata(); $metacc = idx( @@ -447,4 +526,28 @@ class DifferentialCommentEditor { return $comment; } + private function filterAddedCCs(array $ccs) { + $revision = $this->revision; + + $current_ccs = $revision->getCCPHIDs(); + $current_ccs = array_fill_keys($current_ccs, true); + + $reviewer_phids = $revision->getReviewers(); + $reviewer_phids = array_fill_keys($reviewer_phids, true); + + foreach ($ccs as $key => $cc) { + if (isset($current_ccs[$cc])) { + unset($ccs[$key]); + } + if (isset($reviewer_phids[$cc])) { + unset($ccs[$key]); + } + if ($cc == $revision->getAuthorPHID()) { + unset($ccs[$key]); + } + } + + return $ccs; + } + } diff --git a/src/applications/differential/editor/comment/__init__.php b/src/applications/differential/editor/comment/__init__.php index c04541ffa8..e72ed086cc 100644 --- a/src/applications/differential/editor/comment/__init__.php +++ b/src/applications/differential/editor/comment/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus'); phutil_require_module('phabricator', 'applications/differential/constants/action'); phutil_require_module('phabricator', 'applications/differential/editor/revision'); +phutil_require_module('phabricator', 'applications/differential/exception/noeffect'); phutil_require_module('phabricator', 'applications/differential/mail/comment'); phutil_require_module('phabricator', 'applications/differential/storage/changeset'); phutil_require_module('phabricator', 'applications/differential/storage/comment'); diff --git a/src/applications/differential/exception/base/DifferentialException.php b/src/applications/differential/exception/base/DifferentialException.php new file mode 100644 index 0000000000..794efd7937 --- /dev/null +++ b/src/applications/differential/exception/base/DifferentialException.php @@ -0,0 +1,21 @@ +setWorkflow(true) ->setUser($this->user) ->setAction($this->actionURI) ->addHiddenInput('revision_id', $revision->getID()) diff --git a/src/view/dialog/AphrontDialogView.php b/src/view/dialog/AphrontDialogView.php index 1c040d81ca..b0d598336b 100644 --- a/src/view/dialog/AphrontDialogView.php +++ b/src/view/dialog/AphrontDialogView.php @@ -1,7 +1,7 @@ hidden[] = array($key, $value); + if (is_array($value)) { + foreach ($value as $hidden_key => $hidden_value) { + $this->hidden[] = array($key.'['.$hidden_key.']', $hidden_value); + } + } else { + $this->hidden[] = array($key, $value); + } return $this; }