From 35750b9c615a0ef2665797acaa7cc28012948aef Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 2 Jan 2017 11:17:27 -0800 Subject: [PATCH] Make some Differential comment actions (like "Accept" and "Reject") conflict with one another Summary: Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision. For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall. Test Plan: - Selected "Accept", then selected "Reject". One replaced the other. - Selected "Accept", then selected "Change Subscribers". Both co-existed happily. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17132 --- resources/celerity/map.php | 29 ++-- .../DifferentialRevisionActionTransaction.php | 12 ++ .../PhabricatorEditEngineCommentAction.php | 10 ++ .../editfield/PhabricatorApplyEditField.php | 13 +- ...catorApplicationTransactionCommentView.php | 1 + .../transactions/behavior-comment-actions.js | 155 +++++++++++------- 6 files changed, 147 insertions(+), 73 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index ab3b79889a..7c9c2aab6c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -396,6 +396,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '019f36c4', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', + 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/ChangesetViewManager.js' => 'a2828756', 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '2e3f9738', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', @@ -453,7 +454,7 @@ return array( 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', - 'rsrc/js/application/transactions/behavior-comment-actions.js' => 'c23ecb0b', + 'rsrc/js/application/transactions/behavior-comment-actions.js' => '8fd8b2b1', 'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243', 'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96', 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => '94c65b72', @@ -609,7 +610,7 @@ return array( 'javelin-behavior-bulk-job-reload' => 'edf8a145', 'javelin-behavior-calendar-month-view' => 'fe33e256', 'javelin-behavior-choose-control' => '327a00d1', - 'javelin-behavior-comment-actions' => 'c23ecb0b', + 'javelin-behavior-comment-actions' => '8fd8b2b1', 'javelin-behavior-config-reorder-fields' => 'b6993408', 'javelin-behavior-conpherence-menu' => '7524fcfa', 'javelin-behavior-conpherence-participant-pane' => '8604caa8', @@ -625,6 +626,7 @@ return array( 'javelin-behavior-desktop-notifications-control' => 'edd1ba66', 'javelin-behavior-detect-timezone' => '4c193c96', 'javelin-behavior-device' => 'bb1dd507', + 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-add-reviewers-and-ccs' => 'e10f8e18', 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', @@ -944,6 +946,11 @@ return array( 'javelin-dom', 'phabricator-keyboard-shortcut', ), + '051c7832' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + ), '05270951' => array( 'javelin-util', 'javelin-magical-init', @@ -1640,6 +1647,15 @@ return array( 'javelin-stratcom', 'javelin-util', ), + '8fd8b2b1' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-dom', + 'phuix-form-control-view', + 'phuix-icon-view', + 'javelin-behavior-phabricator-gesture', + ), '8ff5e24c' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1947,15 +1963,6 @@ return array( 'javelin-install', 'javelin-dom', ), - 'c23ecb0b' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-dom', - 'phuix-form-control-view', - 'phuix-icon-view', - 'javelin-behavior-phabricator-gesture', - ), 'c587b80f' => array( 'javelin-install', ), diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index e495fff8e7..14d7a1369a 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -73,6 +73,18 @@ abstract class DifferentialRevisionActionTransaction $group_key = $this->getRevisionActionGroupKey(); $field->setCommentActionGroupKey($group_key); + + // Currently, every revision action conflicts with every other + // revision action: for example, you can not simultaneously Accept and + // Reject a revision. + + // Under some configurations, some combinations of actions are sort of + // technically permissible. For example, you could reasonably Reject + // and Abandon a revision if "anyone can abandon anything" is enabled. + + // It's not clear that these combinations are actually useful, so just + // keep things simple for now. + $field->setActionConflictKey('revision.action'); } } diff --git a/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php index 1c1c0c0725..4d3b9ca5df 100644 --- a/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php +++ b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php @@ -8,6 +8,7 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject { private $initialValue; private $order; private $groupKey; + private $conflictKey; abstract public function getPHUIXControlType(); abstract public function getPHUIXControlSpecification(); @@ -30,6 +31,15 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject { return $this->groupKey; } + public function setConflictKey($conflict_key) { + $this->conflictKey = $conflict_key; + return $this; + } + + public function getConflictKey() { + return $this->conflictKey; + } + public function setLabel($label) { $this->label = $label; return $this; diff --git a/src/applications/transactions/editfield/PhabricatorApplyEditField.php b/src/applications/transactions/editfield/PhabricatorApplyEditField.php index f6d9783f46..a292a65021 100644 --- a/src/applications/transactions/editfield/PhabricatorApplyEditField.php +++ b/src/applications/transactions/editfield/PhabricatorApplyEditField.php @@ -4,6 +4,7 @@ final class PhabricatorApplyEditField extends PhabricatorEditField { private $actionDescription; + private $actionConflictKey; protected function newControl() { return null; @@ -18,6 +19,15 @@ final class PhabricatorApplyEditField return $this->actionDescription; } + public function setActionConflictKey($action_conflict_key) { + $this->actionConflictKey = $action_conflict_key; + return $this; + } + + public function getActionConflictKey() { + return $this->actionConflictKey; + } + protected function newHTTPParameterType() { return new AphrontBoolHTTPParameterType(); } @@ -34,7 +44,8 @@ final class PhabricatorApplyEditField protected function newCommentAction() { return id(new PhabricatorEditEngineStaticCommentAction()) - ->setDescription($this->getActionDescription()); + ->setDescription($this->getActionDescription()) + ->setConflictKey($this->getActionConflictKey()); } } diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index 501f43d852..f69a2cd1bd 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -301,6 +301,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'spec' => $comment_action->getPHUIXControlSpecification(), 'initialValue' => $comment_action->getInitialValue(), 'groupKey' => $comment_action->getGroupKey(), + 'conflictKey' => $comment_action->getConflictKey(), ); $type_map[$key] = $comment_action; diff --git a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js index 3383b26a24..d10de590d4 100644 --- a/webroot/rsrc/js/application/transactions/behavior-comment-actions.js +++ b/webroot/rsrc/js/application/transactions/behavior-comment-actions.js @@ -43,59 +43,13 @@ JX.behavior('comment-actions', function(config) { return null; } - function add_row(option) { - var action = action_map[option.value]; - if (!action) { - return; + function remove_action(key) { + var row = rows[key]; + if (row) { + JX.DOM.remove(row.node); + row.option.disabled = false; + delete rows[key]; } - - option.disabled = true; - - var icon = new JX.PHUIXIconView() - .setIcon('fa-times-circle'); - var remove = JX.$N('a', {href: '#'}, icon.getNode()); - - var control = new JX.PHUIXFormControl() - .setLabel(action.label) - .setError(remove) - .setControl(action.type, action.spec) - .setClass('phui-comment-action'); - var node = control.getNode(); - - JX.Stratcom.addSigil(node, 'touchable'); - - var remove_action = function() { - JX.DOM.remove(node); - delete rows[action.key]; - option.disabled = false; - }; - - JX.DOM.listen(node, 'gesture.swipe.end', null, function(e) { - var data = e.getData(); - - if (data.direction != 'left') { - // Didn't swipe left. - return; - } - - if (data.length <= (JX.Vector.getDim(node).x / 2)) { - // Didn't swipe far enough. - return; - } - - remove_action(); - }); - - rows[action.key] = control; - - JX.DOM.listen(remove, 'click', null, function(e) { - e.kill(); - remove_action(); - }); - - place_node.parentNode.insertBefore(node, place_node); - - return control; } function serialize_actions() { @@ -104,7 +58,7 @@ JX.behavior('comment-actions', function(config) { for (var k in rows) { data.push({ type: k, - value: rows[k].getValue(), + value: rows[k].control.getValue(), initialValue: action_map[k].initialValue || null }); } @@ -171,6 +125,91 @@ JX.behavior('comment-actions', function(config) { } } + function force_preview() { + if (!config.shouldPreview) { + return; + } + + new JX.Request(config.actionURI, onresponse) + .setData(get_data()) + .send(); + } + + function add_row(option) { + var action = action_map[option.value]; + if (!action) { + return; + } + + // Remove any conflicting actions. For example, "Accept Revision" conflicts + // with "Reject Revision". + var conflict_key = action.conflictKey || null; + if (conflict_key !== null) { + for (var k in action_map) { + if (k === action) { + continue; + } + if (action_map[k].conflictKey !== conflict_key) { + continue; + } + + if (!(k in rows)) { + continue; + } + + remove_action(k); + } + } + + option.disabled = true; + + var icon = new JX.PHUIXIconView() + .setIcon('fa-times-circle'); + var remove = JX.$N('a', {href: '#'}, icon.getNode()); + + var control = new JX.PHUIXFormControl() + .setLabel(action.label) + .setError(remove) + .setControl(action.type, action.spec) + .setClass('phui-comment-action'); + var node = control.getNode(); + + JX.Stratcom.addSigil(node, 'touchable'); + + JX.DOM.listen(node, 'gesture.swipe.end', null, function(e) { + var data = e.getData(); + + if (data.direction != 'left') { + // Didn't swipe left. + return; + } + + if (data.length <= (JX.Vector.getDim(node).x / 2)) { + // Didn't swipe far enough. + return; + } + + remove_action(action); + }); + + rows[action.key] = { + control: control, + node: node, + option: option + }; + + JX.DOM.listen(remove, 'click', null, function(e) { + e.kill(); + remove_action(action); + }); + + place_node.parentNode.insertBefore(node, place_node); + + force_preview(); + + return control; + } + JX.DOM.listen(form_node, ['submit', 'didSyntheticSubmit'], null, function() { input_node.value = serialize_actions(); }); @@ -185,13 +224,7 @@ JX.behavior('comment-actions', function(config) { JX.DOM.listen(form_node, 'keydown', null, trigger); - var always_trigger = function() { - new JX.Request(config.actionURI, onresponse) - .setData(get_data()) - .send(); - }; - - JX.DOM.listen(form_node, 'shouldRefresh', null, always_trigger); + JX.DOM.listen(form_node, 'shouldRefresh', null, force_preview); request.start(); var old_device = JX.Device.getDevice(); @@ -206,7 +239,7 @@ JX.behavior('comment-actions', function(config) { // Force an immediate refresh if we switched from another device type // to desktop. if (old_device != new_device) { - always_trigger(); + force_preview(); } } else { // On mobile, don't show live previews and only save drafts every