From a5b3e33e3c2649d7c11a73c9c74e2729a66b8743 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Mar 2019 19:29:33 -0700 Subject: [PATCH] Don't show workboard action previews if the action won't have any effect Summary: Ref T10335. When you (for example) drag a "Resolved" task into a column with "Trigger: change status to resolved.", don't show a hint that the action will "Change status to resolved." since this isn't helpful and is somewhat confusing. For now, the only visibility operator is "!=" since all current actions are simple field comparisons, but some actions in the future (like "add subscriber" or "remove project") might need other conditions. Test Plan: Dragged cards in ways that previously provided useless hints: move from column A to column B on a "Group by Priority" board; drag a resolved task to a "Trigger: change status to as resolved" column. Saw a more accurate preview in both cases. Drags which actually cause effects still show the effects correctly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10335 Differential Revision: https://secure.phabricator.com/D20300 --- resources/celerity/map.php | 40 +++++++++---------- .../PhabricatorProjectBoardViewController.php | 2 + .../icon/PhabricatorProjectDropEffect.php | 16 ++++++++ .../PhabricatorProjectColumnOwnerOrder.php | 1 + .../PhabricatorProjectColumnPriorityOrder.php | 1 + .../PhabricatorProjectColumnStatusOrder.php | 1 + ...catorProjectTriggerManiphestStatusRule.php | 1 + .../js/application/projects/WorkboardBoard.js | 11 +++++ .../projects/WorkboardDropEffect.js | 32 ++++++++++++++- 9 files changed, 83 insertions(+), 22 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4d095ad2b1..4b732de524 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -408,12 +408,12 @@ return array( 'rsrc/js/application/phortune/phortune-credit-card-form.js' => 'd12d214f', 'rsrc/js/application/policy/behavior-policy-control.js' => '0eaa33a9', 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '9347f172', - 'rsrc/js/application/projects/WorkboardBoard.js' => 'ba6e36b0', + 'rsrc/js/application/projects/WorkboardBoard.js' => '2f893acd', 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', 'rsrc/js/application/projects/WorkboardColumn.js' => 'c344eb3c', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', - 'rsrc/js/application/projects/WorkboardDropEffect.js' => '101121be', + 'rsrc/js/application/projects/WorkboardDropEffect.js' => 'c808589e', 'rsrc/js/application/projects/WorkboardHeader.js' => '111bfd2d', 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => 'ebe83a6b', 'rsrc/js/application/projects/WorkboardOrderTemplate.js' => '03e8891f', @@ -730,12 +730,12 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'ba6e36b0', + 'javelin-workboard-board' => '2f893acd', 'javelin-workboard-card' => '0392a5d8', 'javelin-workboard-card-template' => '2a61f8d4', 'javelin-workboard-column' => 'c344eb3c', 'javelin-workboard-controller' => '42c7a5a7', - 'javelin-workboard-drop-effect' => '101121be', + 'javelin-workboard-drop-effect' => 'c808589e', 'javelin-workboard-header' => '111bfd2d', 'javelin-workboard-header-template' => 'ebe83a6b', 'javelin-workboard-order-template' => '03e8891f', @@ -1003,10 +1003,6 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), - '101121be' => array( - 'javelin-install', - 'javelin-dom', - ), '111bfd2d' => array( 'javelin-install', ), @@ -1170,6 +1166,18 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), + '2f893acd' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + 'javelin-workboard-header-template', + 'javelin-workboard-card-template', + 'javelin-workboard-order-template', + ), '308f9fe4' => array( 'javelin-install', 'javelin-util', @@ -1880,18 +1888,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'ba6e36b0' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - 'javelin-workboard-header-template', - 'javelin-workboard-card-template', - 'javelin-workboard-order-template', - ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1955,6 +1951,10 @@ return array( 'phuix-icon-view', 'phabricator-busy', ), + 'c808589e' => array( + 'javelin-install', + 'javelin-dom', + ), 'c8147a20' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 41981a5522..6d0c8721fc 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -655,6 +655,8 @@ final class PhabricatorProjectBoardViewController $properties[$task->getPHID()] = array( 'points' => (double)$task->getPoints(), 'status' => $task->getStatus(), + 'priority' => (int)$task->getPriority(), + 'owner' => $task->getOwnerPHID(), ); } diff --git a/src/applications/project/icon/PhabricatorProjectDropEffect.php b/src/applications/project/icon/PhabricatorProjectDropEffect.php index 33145eb039..8e68261fa4 100644 --- a/src/applications/project/icon/PhabricatorProjectDropEffect.php +++ b/src/applications/project/icon/PhabricatorProjectDropEffect.php @@ -6,6 +6,7 @@ final class PhabricatorProjectDropEffect private $icon; private $color; private $content; + private $conditions = array(); public function setIcon($icon) { $this->icon = $icon; @@ -39,7 +40,22 @@ final class PhabricatorProjectDropEffect 'icon' => $this->getIcon(), 'color' => $this->getColor(), 'content' => hsprintf('%s', $this->getContent()), + 'conditions' => $this->getConditions(), ); } + public function addCondition($field, $operator, $value) { + $this->conditions[] = array( + 'field' => $field, + 'operator' => $operator, + 'value' => $value, + ); + + return $this; + } + + public function getConditions() { + return $this->conditions; + } + } diff --git a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php index 920e87c9b7..48a6c394db 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php @@ -171,6 +171,7 @@ final class PhabricatorProjectColumnOwnerOrder $this->newEffect() ->setIcon($owner_icon) ->setColor($owner_color) + ->addCondition('owner', '!=', $owner_phid) ->setContent($effect_content)); } diff --git a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php index 8cffab91ae..42ccf96553 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php @@ -68,6 +68,7 @@ final class PhabricatorProjectColumnPriorityOrder $drop_effect = $this->newEffect() ->setIcon($priority_icon) ->setColor($priority_color) + ->addCondition('priority', '!=', $priority) ->setContent( pht( 'Change priority to %s.', diff --git a/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php index 419d7062c7..2cb156aa92 100644 --- a/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php @@ -75,6 +75,7 @@ final class PhabricatorProjectColumnStatusOrder $drop_effect = $this->newEffect() ->setIcon($status_icon) ->setColor($status_color) + ->addCondition('status', '!=', $status_key) ->setContent( pht( 'Change status to %s.', diff --git a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php index 575f51cd2b..58e8c227df 100644 --- a/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php +++ b/src/applications/project/trigger/PhabricatorProjectTriggerManiphestStatusRule.php @@ -51,6 +51,7 @@ final class PhabricatorProjectTriggerManiphestStatusRule $this->newEffect() ->setIcon($status_icon) ->setColor($status_color) + ->addCondition('status', '!=', $value) ->setContent($content), ); } diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index 6fab227c84..5a55a2d905 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -228,6 +228,17 @@ JX.install('WorkboardBoard', { effects = effects.concat(header.getDropEffects()); } + var card_phid = JX.Stratcom.getData(src_node).objectPHID; + var card = src_column.getCard(card_phid); + + var visible = []; + for (var ii = 0; ii < effects.length; ii++) { + if (effects[ii].isEffectVisibleForCard(card)) { + visible.push(effects[ii]); + } + } + effects = visible; + if (!effects.length) { JX.DOM.remove(node); return; diff --git a/webroot/rsrc/js/application/projects/WorkboardDropEffect.js b/webroot/rsrc/js/application/projects/WorkboardDropEffect.js index ecd18d0015..fc8b2eaa58 100644 --- a/webroot/rsrc/js/application/projects/WorkboardDropEffect.js +++ b/webroot/rsrc/js/application/projects/WorkboardDropEffect.js @@ -10,7 +10,8 @@ JX.install('WorkboardDropEffect', { properties: { icon: null, color: null, - content: null + content: null, + conditions: [] }, statics: { @@ -18,7 +19,8 @@ JX.install('WorkboardDropEffect', { return new JX.WorkboardDropEffect() .setIcon(map.icon) .setColor(map.color) - .setContent(JX.$H(map.content)); + .setContent(JX.$H(map.content)) + .setConditions(map.conditions || []); } }, @@ -30,6 +32,32 @@ JX.install('WorkboardDropEffect', { .getNode(); return JX.$N('li', {}, [icon, this.getContent()]); + }, + + isEffectVisibleForCard: function(card) { + var conditions = this.getConditions(); + + var properties = card.getProperties(); + for (var ii = 0; ii < conditions.length; ii++) { + var condition = conditions[ii]; + + var field = properties[condition.field]; + var value = condition.value; + + var result = true; + switch (condition.operator) { + case '!=': + result = (field !== value); + break; + } + + if (!result) { + return false; + } + } + + return true; } + } });