From 7a315780b4fe7c8327e4e541b7f5c951cf02cc5e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Jun 2016 11:37:38 -0700 Subject: [PATCH] When using the "Close as Duplicate" relationship action, limit the UI to 1 task Summary: Ref T4788. When closing a task as a duplicate of another task, you can only select one task, since it doesn't really make sense to merge one task into several other tasks (this operation is //possible//, but probably not what anyone ever wants to do, I think?). Make the UI understand this: after you select a task, disable all of the "select" buttons in the UI to make this clear. Test Plan: - Used "Close as Duplicate", only allowed to select 1 task. - Used other editors like "Merge Duplicates In", allowed to select lots of tasks. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4788 Differential Revision: https://secure.phabricator.com/D16203 --- resources/celerity/map.php | 18 +-- ...iphestTaskCloseAsDuplicateRelationship.php | 13 +- ...habricatorSearchRelationshipController.php | 13 ++ .../PhabricatorObjectRelationship.php | 4 + .../PhabricatorObjectSelectorDialog.php | 13 ++ .../rsrc/js/core/behavior-object-selector.js | 122 +++++++++++++----- 6 files changed, 130 insertions(+), 53 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index acff94bca3..b6385b9aeb 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'core.pkg.js' => 'f2139810', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '3e81ae60', - 'differential.pkg.js' => '01a010d6', + 'differential.pkg.js' => '634399e9', 'diffusion.pkg.css' => '91c5d3a6', 'diffusion.pkg.js' => '3a9a8bfa', 'maniphest.pkg.css' => '4845691a', @@ -495,7 +495,7 @@ return array( 'rsrc/js/core/behavior-lightbox-attachments.js' => 'f8ba29d7', 'rsrc/js/core/behavior-line-linker.js' => '1499a8cb', 'rsrc/js/core/behavior-more.js' => 'a80d0378', - 'rsrc/js/core/behavior-object-selector.js' => '9030ebef', + 'rsrc/js/core/behavior-object-selector.js' => 'e0ec7f2f', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', 'rsrc/js/core/behavior-phabricator-nav.js' => '56a1ca03', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '116cf19b', @@ -658,7 +658,7 @@ return array( 'javelin-behavior-phabricator-line-linker' => '1499a8cb', 'javelin-behavior-phabricator-nav' => '56a1ca03', 'javelin-behavior-phabricator-notification-example' => '8ce821c5', - 'javelin-behavior-phabricator-object-selector' => '9030ebef', + 'javelin-behavior-phabricator-object-selector' => 'e0ec7f2f', 'javelin-behavior-phabricator-oncopy' => '2926fff2', 'javelin-behavior-phabricator-remarkup-assist' => '116cf19b', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', @@ -1608,12 +1608,6 @@ return array( 'javelin-dom', 'javelin-request', ), - '9030ebef' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-request', - 'javelin-util', - ), '9196fb06' => array( 'javelin-install', 'javelin-dom', @@ -2036,6 +2030,12 @@ return array( 'df5e11d2' => array( 'javelin-install', ), + 'e0ec7f2f' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-request', + 'javelin-util', + ), 'e10f8e18' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/maniphest/relationship/ManiphestTaskCloseAsDuplicateRelationship.php b/src/applications/maniphest/relationship/ManiphestTaskCloseAsDuplicateRelationship.php index 6428c79b55..e973e86e76 100644 --- a/src/applications/maniphest/relationship/ManiphestTaskCloseAsDuplicateRelationship.php +++ b/src/applications/maniphest/relationship/ManiphestTaskCloseAsDuplicateRelationship.php @@ -53,16 +53,11 @@ final class ManiphestTaskCloseAsDuplicateRelationship return false; } + public function getMaximumSelectionSize() { + return 1; + } + public function willUpdateRelationships($object, array $add, array $rem) { - - // TODO: Communicate this in the UI before users hit this error. - if (count($add) > 1) { - throw new Exception( - pht( - 'A task can only be closed as a duplicate of exactly one other '. - 'task.')); - } - $task = head($add); return $this->newMergeIntoTransactions($task); } diff --git a/src/applications/search/controller/PhabricatorSearchRelationshipController.php b/src/applications/search/controller/PhabricatorSearchRelationshipController.php index 14c705ac62..d254969423 100644 --- a/src/applications/search/controller/PhabricatorSearchRelationshipController.php +++ b/src/applications/search/controller/PhabricatorSearchRelationshipController.php @@ -39,11 +39,23 @@ final class PhabricatorSearchRelationshipController $done_uri = $src_handle->getURI(); $initial_phids = $dst_phids; + $maximum = $relationship->getMaximumSelectionSize(); + if ($request->isFormPost()) { $phids = explode(';', $request->getStr('phids')); $phids = array_filter($phids); $phids = array_values($phids); + // The UI normally enforces this with Javascript, so this is just a + // sanity check and does not need to be particularly user-friendly. + if ($maximum && (count($phids) > $maximum)) { + throw new Exception( + pht( + 'Too many relationships (%s, of type "%s").', + phutil_count($phids), + $relationship->getRelationshipConstant())); + } + $initial_phids = $request->getStrList('initialPHIDs'); // Apply the changes as adds and removes relative to the original state @@ -175,6 +187,7 @@ final class PhabricatorSearchRelationshipController ->setHeader($dialog_header) ->setButtonText($dialog_button) ->setInstructions($dialog_instructions) + ->setMaximumSelectionSize($maximum) ->buildDialog(); } diff --git a/src/applications/search/relationship/PhabricatorObjectRelationship.php b/src/applications/search/relationship/PhabricatorObjectRelationship.php index 50b5299952..0827c1728c 100644 --- a/src/applications/search/relationship/PhabricatorObjectRelationship.php +++ b/src/applications/search/relationship/PhabricatorObjectRelationship.php @@ -104,6 +104,10 @@ abstract class PhabricatorObjectRelationship extends Phobject { return "/search/rel/{$type}/{$phid}/"; } + public function getMaximumSelectionSize() { + return null; + } + public function canUndoRelationship() { return true; } diff --git a/src/view/control/PhabricatorObjectSelectorDialog.php b/src/view/control/PhabricatorObjectSelectorDialog.php index 09d9582e8b..3455cbc8bd 100644 --- a/src/view/control/PhabricatorObjectSelectorDialog.php +++ b/src/view/control/PhabricatorObjectSelectorDialog.php @@ -11,6 +11,7 @@ final class PhabricatorObjectSelectorDialog extends Phobject { private $selectedFilter; private $excluded; private $initialPHIDs; + private $maximumSelectionSize; private $title; private $header; @@ -87,6 +88,15 @@ final class PhabricatorObjectSelectorDialog extends Phobject { return $this->initialPHIDs; } + public function setMaximumSelectionSize($maximum_selection_size) { + $this->maximumSelectionSize = $maximum_selection_size; + return $this; + } + + public function getMaximumSelectionSize() { + return $this->maximumSelectionSize; + } + public function buildDialog() { $user = $this->user; @@ -190,6 +200,8 @@ final class PhabricatorObjectSelectorDialog extends Phobject { $dialog->addHiddenInput('initialPHIDs', $initial_phids); } + $maximum = $this->getMaximumSelectionSize(); + Javelin::initBehavior( 'phabricator-object-selector', array( @@ -202,6 +214,7 @@ final class PhabricatorObjectSelectorDialog extends Phobject { 'exclude' => $this->excluded, 'uri' => $this->searchURI, 'handles' => $handle_views, + 'maximum' => $maximum, )); $dialog->setResizeX(true); diff --git a/webroot/rsrc/js/core/behavior-object-selector.js b/webroot/rsrc/js/core/behavior-object-selector.js index d4a542c840..686f0f8820 100644 --- a/webroot/rsrc/js/core/behavior-object-selector.js +++ b/webroot/rsrc/js/core/behavior-object-selector.js @@ -10,11 +10,13 @@ JX.behavior('phabricator-object-selector', function(config) { var n = 0; var phids = {}; + var display = []; + var handles = config.handles; for (var k in handles) { phids[k] = true; } - var button_list = {}; + var query_timer = null; var query_delay = 50; @@ -41,37 +43,82 @@ JX.behavior('phabricator-object-selector', function(config) { return; } - var display = []; - button_list = {}; + display = []; for (var k in r) { handles[r[k].phid] = r[k]; - display.push(renderHandle(r[k], true)); + display.push({phid: r[k].phid}); } - if (!display.length) { - display = renderNote('No results.'); - } - - JX.DOM.setContent(JX.$(config.results), display); + redrawList(true); } function redrawAttached() { - var display = []; + var attached = []; for (var k in phids) { - display.push(renderHandle(handles[k], false)); + attached.push(renderHandle(handles[k], false).item); } - if (!display.length) { - display = renderNote('Nothing attached.'); + if (!attached.length) { + attached = renderNote('Nothing attached.'); } - JX.DOM.setContent(JX.$(config.current), display); + JX.DOM.setContent(JX.$(config.current), attached); phid_input.value = JX.keys(phids).join(';'); } - function renderHandle(h, attach) { + function redrawList(rebuild) { + var ii; + var content; + if (rebuild) { + if (display.length) { + var handle; + + content = []; + for (ii = 0; ii < display.length; ii++) { + handle = handles[display[ii].phid]; + + display[ii].node = renderHandle(handle, true); + content.push(display[ii].node.item); + } + } else { + content = renderNote('No results.'); + } + + JX.DOM.setContent(JX.$(config.results), content); + } + + var phid; + var is_disabled; + var button; + + var at_maximum = !canSelectMore(); + + for (ii = 0; ii < display.length; ii++) { + phid = display[ii].phid; + + is_disabled = false; + + // If this object is already selected, you can not select it again. + if (phids.hasOwnProperty(phid)) { + is_disabled = true; + } + + // If the maximum number of objects are already selected, you can + // not select more. + if (at_maximum) { + is_disabled = true; + } + + button = display[ii].node.button; + JX.DOM.alterClass(button, 'disabled', is_disabled); + button.disabled = is_disabled; + } + + } + + function renderHandle(h, attach) { var some_icon = JX.$N( 'span', {className: 'phui-icon-view phui-font-fa ' + @@ -111,15 +158,10 @@ JX.behavior('phabricator-object-selector', function(config) { meta: {handle: h, table:table}}, cells)); - if (attach) { - button_list[h.phid] = select_object_button; - if (h.phid in phids) { - JX.DOM.alterClass(select_object_button, 'disabled', true); - select_object_button.disabled = true; - } - } - - return table; + return { + item: table, + button: select_object_button + }; } function renderNote(note) { @@ -138,6 +180,18 @@ JX.behavior('phabricator-object-selector', function(config) { .send(); } + function canSelectMore() { + if (!config.maximum) { + return true; + } + + if (JX.keys(phids).length < config.maximum) { + return true; + } + + return false; + } + JX.DOM.listen( JX.$(config.results), 'click', @@ -151,10 +205,13 @@ JX.behavior('phabricator-object-selector', function(config) { return; } - phids[phid] = true; - JX.DOM.alterClass(button_list[phid], 'disabled', true); - button_list[phid].disabled = true; + if (!canSelectMore()) { + return; + } + phids[phid] = true; + + redrawList(false); redrawAttached(); }); @@ -170,13 +227,7 @@ JX.behavior('phabricator-object-selector', function(config) { delete phids[phid]; - // NOTE: We may not have a button in the button list, if this result is - // not visible in the current search results. - if (button_list[phid]) { - JX.DOM.alterClass(button_list[phid], 'disabled', false); - button_list[phid].disabled = false; - } - + redrawList(false); redrawAttached(); }); @@ -205,6 +256,7 @@ JX.behavior('phabricator-object-selector', function(config) { }); sendQuery(); - redrawAttached(); + redrawList(true); + redrawAttached(); });