From 2260738de9e07834a89b45b292920250779190a7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Mar 2019 15:13:25 -0800 Subject: [PATCH 01/25] When dragging nodes between different columns on an ordered board, don't reorder them by making secondary edits Summary: Ref T10334. When a workboard is ordered by priority, dragging from column "A" to a particular place in column "B" currently means "move this task to column B, and adjust its priority so that it naturally sorts into the location under my mouse cursor". Users frequently find this confusing / undesirable. To begin improving this, make "drag from column A to column B" and "drag from somewhere in column A to somewhere else in column A" into different operations. The first operation, a movement between columns, no longer implies an ordering change. The second action still does. So if you actually want to change the priority of a task, you drag it within its current column. If you just want to move it to a different column, you drag it between columns. This creates some possible problems: - Some users may love the current behavior and just not be very vocal about it. I doubt it, but presumably we'll hear from them if we break it. - If you actualy want to move + reorder, it's a bit more cumbersome now. We could possibly add something like "shift + drag" for this if there's feedback. - The new behavior is probably less surprising, but may not be much more obvious. Future changes (for example, in T10335) should help make it more clear. - When you mouse cursor goes over column B, the card dashed-rectangle preview target thing jumps to the correct position in the column -- but that may not be under your mouse cursor. This feels pretty much fine if the whole column fits on screen. It may not be so great if the column does not fit on screen and the dashed-rectangle-thing has vanished. This is just a UI feedback issue and we could refine this later (scroll/highlight the column). Test Plan: - Created several tasks at different priority levels, sorted a board by priority, dragged tasks between columns. Dragging from "A" to "B" no longer causes a priority edit. - Also, dragged within a column. This still performs priority edits. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10334 Differential Revision: https://secure.phabricator.com/D20242 --- resources/celerity/map.php | 56 +++++++++---------- .../js/application/projects/WorkboardBoard.js | 4 ++ .../application/projects/WorkboardColumn.js | 27 +++++++-- webroot/rsrc/js/core/DraggableList.js | 32 ++++++++++- 4 files changed, 84 insertions(+), 35 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 7f9aaa4995..bc81717e06 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', - 'core.pkg.js' => '2cda17a4', + 'core.pkg.js' => '9eb1254b', 'differential.pkg.css' => '1755a478', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -409,9 +409,9 @@ 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' => '45d0b2b1', + 'rsrc/js/application/projects/WorkboardBoard.js' => '3a8c42a3', 'rsrc/js/application/projects/WorkboardCard.js' => '9a513421', - 'rsrc/js/application/projects/WorkboardColumn.js' => '8573dc1b', + 'rsrc/js/application/projects/WorkboardColumn.js' => 'b451fd4c', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/behavior-project-boards.js' => '05c74d65', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', @@ -434,7 +434,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '29819b75', 'rsrc/js/core/Busy.js' => '5202e831', 'rsrc/js/core/DragAndDropFileUpload.js' => '4370900d', - 'rsrc/js/core/DraggableList.js' => '3c6bd549', + 'rsrc/js/core/DraggableList.js' => 'd594c805', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', 'rsrc/js/core/Hovercard.js' => '074f0783', @@ -727,9 +727,9 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => '45d0b2b1', + 'javelin-workboard-board' => '3a8c42a3', 'javelin-workboard-card' => '9a513421', - 'javelin-workboard-column' => '8573dc1b', + 'javelin-workboard-column' => 'b451fd4c', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', @@ -755,7 +755,7 @@ return array( 'phabricator-diff-changeset-list' => '04023d82', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', - 'phabricator-draggable-list' => '3c6bd549', + 'phabricator-draggable-list' => 'd594c805', 'phabricator-fatal-config-template-css' => '20babf50', 'phabricator-favicon' => '7930776a', 'phabricator-feed-css' => 'd8b6e3f8', @@ -1188,18 +1188,19 @@ return array( 'javelin-install', 'javelin-dom', ), + '3a8c42a3' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + ), '3b4899b0' => array( 'javelin-behavior', 'phabricator-prefab', ), - '3c6bd549' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), '3dc5ad43' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1256,15 +1257,6 @@ return array( '43bc9360' => array( 'javelin-install', ), - '45d0b2b1' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - ), '46116c01' => array( 'javelin-request', 'javelin-behavior', @@ -1565,10 +1557,6 @@ return array( 'javelin-workflow', 'phabricator-draggable-list', ), - '8573dc1b' => array( - 'javelin-install', - 'javelin-workboard-card', - ), '87428eb2' => array( 'javelin-behavior', 'javelin-diffusion-locate-file-source', @@ -1855,6 +1843,10 @@ return array( 'b347a301' => array( 'javelin-behavior', ), + 'b451fd4c' => array( + 'javelin-install', + 'javelin-workboard-card', + ), 'b517bfa0' => array( 'phui-oi-list-view-css', ), @@ -1994,6 +1986,14 @@ return array( 'd3799cb4' => array( 'javelin-install', ), + 'd594c805' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), 'd8a86cfb' => array( 'javelin-behavior', 'javelin-dom', diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index cac35c2d9a..422ee70418 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -118,6 +118,10 @@ JX.install('WorkboardBoard', { .setCanDragX(true) .setHasInfiniteHeight(true); + if (this.getOrder() !== 'natural') { + list.setCompareHandler(JX.bind(column, column.compareHandler)); + } + list.listen('didDrop', JX.bind(this, this._onmovecard, list)); lists.push(list); diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index 9973648593..28bff862da 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -175,6 +175,19 @@ JX.install('WorkboardColumn', { this._dirty = false; }, + compareHandler: function(src_list, src_node, dst_list, dst_node) { + var board = this.getBoard(); + var order = board.getOrder(); + + var src_phid = JX.Stratcom.getData(src_node).objectPHID; + var dst_phid = JX.Stratcom.getData(dst_node).objectPHID; + + var u_vec = this.getBoard().getOrderVector(src_phid, order); + var v_vec = this.getBoard().getOrderVector(dst_phid, order); + + return this._compareVectors(u_vec, v_vec); + }, + _getCardsSortedNaturally: function() { var list = []; @@ -200,15 +213,19 @@ JX.install('WorkboardColumn', { }, _sortCards: function(order, u, v) { - var ud = this.getBoard().getOrderVector(u.getPHID(), order); - var vd = this.getBoard().getOrderVector(v.getPHID(), order); + var u_vec = this.getBoard().getOrderVector(u.getPHID(), order); + var v_vec = this.getBoard().getOrderVector(v.getPHID(), order); - for (var ii = 0; ii < ud.length; ii++) { - if (ud[ii] > vd[ii]) { + return this._compareVectors(u_vec, v_vec); + }, + + _compareVectors: function(u_vec, v_vec) { + for (var ii = 0; ii < u_vec.length; ii++) { + if (u_vec[ii] > v_vec[ii]) { return 1; } - if (ud[ii] < vd[ii]) { + if (u_vec[ii] < v_vec[ii]) { return -1; } } diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index a545ed7272..a65489be41 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -39,6 +39,7 @@ JX.install('DraggableList', { properties : { findItemsHandler: null, + compareHandler: null, canDragX: false, outerContainer: null, hasInfiniteHeight: false @@ -367,8 +368,29 @@ JX.install('DraggableList', { return this; }, + _getOrderedTarget: function(src_list, src_node) { + var targets = this._getTargets(); + + // NOTE: The targets are ordered from the bottom of the column to the + // top, so we're looking for the first node that we sort below. If we + // don't find one, we'll sort to the head of the column. + + for (var ii = 0; ii < targets.length; ii++) { + var target = targets[ii]; + if (this._compareTargets(src_list, src_node, target.item) > 0) { + return target.item; + } + } + + return null; + }, + + _compareTargets: function(src_list, src_node, dst_node) { + var dst_list = this; + return this.getCompareHandler()(src_list, src_node, dst_list, dst_node); + }, + _getCurrentTarget : function(p) { - var ghost = this.getGhostNode(); var targets = this._getTargets(); var dragging = this._dragging; @@ -461,9 +483,15 @@ JX.install('DraggableList', { // Compute the size and position of the drop target indicator, because we // need to update our static position computations to account for it. + var compare_handler = this.getCompareHandler(); + var cur_target = false; if (target_list) { - cur_target = target_list._getCurrentTarget(p); + if (compare_handler && (target_list !== this)) { + cur_target = target_list._getOrderedTarget(this, this._dragging); + } else { + cur_target = target_list._getCurrentTarget(p); + } } // If we've selected a new target, update the UI to show where we're From be1e3b2cc0a8773531ad8565a2b0590d0ea75cca Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Mar 2019 07:45:51 -0800 Subject: [PATCH 02/25] When a user drags a card over a column, highlight the column border Summary: Ref T10334. Partly, this just improves visual feedback for all drag operations. After D20242, we can have cases where you (for example) drag a low-priority node to a very tall column on a priority-ordered workboard. In this case, the actual dashed-border-drop-target may not be on screen. We might make the column scroll or put some kind of hint in the UI in this case, but an easy starting point is just to make the "yes, you're targeting this column" state a bit more clear. Test Plan: Dragged tasks between columns, saw the border higlight on the target columns. This is very tricky to take a screenshot of. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10334 Differential Revision: https://secure.phabricator.com/D20245 --- resources/celerity/map.php | 66 +++++++++---------- .../css/phui/workboards/phui-workpanel.css | 8 +++ .../js/application/projects/WorkboardBoard.js | 3 +- .../application/projects/WorkboardColumn.js | 5 ++ webroot/rsrc/js/core/DraggableList.js | 17 ++++- 5 files changed, 63 insertions(+), 36 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index bc81717e06..5118dc741d 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', - 'core.pkg.js' => '9eb1254b', + 'core.pkg.js' => 'b96c872e', 'differential.pkg.css' => '1755a478', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -178,7 +178,7 @@ return array( 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', 'rsrc/css/phui/workboards/phui-workboard.css' => '74fc9d98', 'rsrc/css/phui/workboards/phui-workcard.css' => '8c536f90', - 'rsrc/css/phui/workboards/phui-workpanel.css' => 'bd546a49', + 'rsrc/css/phui/workboards/phui-workpanel.css' => '7e12d43c', 'rsrc/css/sprite-login.css' => '18b368a6', 'rsrc/css/sprite-tokens.css' => 'f1896dc5', 'rsrc/css/syntax/syntax-default.css' => '055fc231', @@ -409,9 +409,9 @@ 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' => '3a8c42a3', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'fd96a6e8', 'rsrc/js/application/projects/WorkboardCard.js' => '9a513421', - 'rsrc/js/application/projects/WorkboardColumn.js' => 'b451fd4c', + 'rsrc/js/application/projects/WorkboardColumn.js' => '1f71e559', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/behavior-project-boards.js' => '05c74d65', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', @@ -434,7 +434,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '29819b75', 'rsrc/js/core/Busy.js' => '5202e831', 'rsrc/js/core/DragAndDropFileUpload.js' => '4370900d', - 'rsrc/js/core/DraggableList.js' => 'd594c805', + 'rsrc/js/core/DraggableList.js' => '8437c663', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', 'rsrc/js/core/Hovercard.js' => '074f0783', @@ -727,9 +727,9 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => '3a8c42a3', + 'javelin-workboard-board' => 'fd96a6e8', 'javelin-workboard-card' => '9a513421', - 'javelin-workboard-column' => 'b451fd4c', + 'javelin-workboard-column' => '1f71e559', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', @@ -755,7 +755,7 @@ return array( 'phabricator-diff-changeset-list' => '04023d82', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', - 'phabricator-draggable-list' => 'd594c805', + 'phabricator-draggable-list' => '8437c663', 'phabricator-fatal-config-template-css' => '20babf50', 'phabricator-favicon' => '7930776a', 'phabricator-feed-css' => 'd8b6e3f8', @@ -854,7 +854,7 @@ return array( 'phui-workboard-color-css' => 'e86de308', 'phui-workboard-view-css' => '74fc9d98', 'phui-workcard-view-css' => '8c536f90', - 'phui-workpanel-view-css' => 'bd546a49', + 'phui-workpanel-view-css' => '7e12d43c', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', 'phuix-autocomplete' => '8f139ef0', @@ -1034,6 +1034,10 @@ return array( 'javelin-behavior', 'javelin-dom', ), + '1f71e559' => array( + 'javelin-install', + 'javelin-workboard-card', + ), '1ff278aa' => array( 'phui-button-css', ), @@ -1188,15 +1192,6 @@ return array( 'javelin-install', 'javelin-dom', ), - '3a8c42a3' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - ), '3b4899b0' => array( 'javelin-behavior', 'phabricator-prefab', @@ -1540,6 +1535,9 @@ return array( 'javelin-install', 'javelin-dom', ), + '7e12d43c' => array( + 'phui-workcard-view-css', + ), '80bff3af' => array( 'javelin-install', 'javelin-typeahead-source', @@ -1557,6 +1555,14 @@ return array( 'javelin-workflow', 'phabricator-draggable-list', ), + '8437c663' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), '87428eb2' => array( 'javelin-behavior', 'javelin-diffusion-locate-file-source', @@ -1843,10 +1849,6 @@ return array( 'b347a301' => array( 'javelin-behavior', ), - 'b451fd4c' => array( - 'javelin-install', - 'javelin-workboard-card', - ), 'b517bfa0' => array( 'phui-oi-list-view-css', ), @@ -1885,9 +1887,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'bd546a49' => array( - 'phui-workcard-view-css', - ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1986,14 +1985,6 @@ return array( 'd3799cb4' => array( 'javelin-install', ), - 'd594c805' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), 'd8a86cfb' => array( 'javelin-behavior', 'javelin-dom', @@ -2129,6 +2120,15 @@ return array( 'javelin-magical-init', 'javelin-util', ), + 'fd96a6e8' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + ), 'fdc13e4e' => array( 'javelin-install', ), diff --git a/webroot/rsrc/css/phui/workboards/phui-workpanel.css b/webroot/rsrc/css/phui/workboards/phui-workpanel.css index 617ff5aa6d..fb7415ff20 100644 --- a/webroot/rsrc/css/phui/workboards/phui-workpanel.css +++ b/webroot/rsrc/css/phui/workboards/phui-workpanel.css @@ -137,3 +137,11 @@ .phui-workpanel-view.project-panel-over-limit .phui-header-shell { border-color: {$red}; } + +.phui-workpanel-view .phui-box-grey { + border: 1px solid transparent; +} + +.phui-workpanel-view.workboard-column-drop-target .phui-box-grey { + border-color: {$lightblueborder}; +} diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index 422ee70418..b3f8e585d6 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -116,7 +116,8 @@ JX.install('WorkboardBoard', { .setOuterContainer(this.getRoot()) .setFindItemsHandler(JX.bind(column, column.getCardNodes)) .setCanDragX(true) - .setHasInfiniteHeight(true); + .setHasInfiniteHeight(true) + .setIsDropTargetHandler(JX.bind(column, column.setIsDropTarget)); if (this.getOrder() !== 'natural') { list.setCompareHandler(JX.bind(column, column.compareHandler)); diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index 28bff862da..a94604a470 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -188,6 +188,11 @@ JX.install('WorkboardColumn', { return this._compareVectors(u_vec, v_vec); }, + setIsDropTarget: function(is_target) { + var node = this.getWorkpanelNode(); + JX.DOM.alterClass(node, 'workboard-column-drop-target', is_target); + }, + _getCardsSortedNaturally: function() { var list = []; diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index a65489be41..598856581f 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -40,6 +40,7 @@ JX.install('DraggableList', { properties : { findItemsHandler: null, compareHandler: null, + isDropTargetHandler: null, canDragX: false, outerContainer: null, hasInfiniteHeight: false @@ -318,7 +319,7 @@ JX.install('DraggableList', { } } - JX.DOM.alterClass(root, 'drag-target-list', is_target); + group[ii]._setIsDropTarget(is_target); } } else { target_list = this; @@ -368,6 +369,18 @@ JX.install('DraggableList', { return this; }, + _setIsDropTarget: function(is_target) { + var root = this.getRootNode(); + JX.DOM.alterClass(root, 'drag-target-list', is_target); + + var handler = this.getIsDropTargetHandler(); + if (handler) { + handler(is_target); + } + + return this; + }, + _getOrderedTarget: function(src_list, src_node) { var targets = this._getTargets(); @@ -633,7 +646,7 @@ JX.install('DraggableList', { var group = this._group; for (var ii = 0; ii < group.length; ii++) { - JX.DOM.alterClass(group[ii].getRootNode(), 'drag-target-list', false); + group[ii]._setIsDropTarget(false); group[ii]._clearTarget(); } From 14a433c77355fab99e0de71653ef3c7824f8795c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Mar 2019 06:00:12 -0800 Subject: [PATCH 03/25] Add priority group headers to workboard columns (display only) Summary: Ref T10333. When workboards are ordered (for example, by priority), add headers to the various groups. Major goals are: - Allow users to drag-and-drop to set values that no cards currently have: for example, you can change a card priority to "normal" by dragging it under the "normal" header, even if no other cards in the column are currently "Normal". - Make future orderings more useful, particularly "order by assignee". We don't really have room to put the username on every card and it would create a fair amount of clutter, but we can put usernames in these headers and then reference them with just the profile picture. This also allows you to assign to users who are not currently assigned anything in a given column. - Make the drag-and-drop behavior more obvious by showing what it will do more clearly (see T8135). - Make things a little easier to scan in general: because space on cards is limited, some information isn't conveyed very clearly (for example, priority information is currently conveyed //only// through color, which can be hard to pick out visually and is probably not functional for users who need vision accommodations). - Maybe do "swimlanes": this is pretty much a "swimlanes" UI if we add whitespace at the bottom of each group so that the headers line up across all the columns (e.g., "Normal" is at the same y-axis position in every column as you scroll down the page). Not sold on this being useful, but it's just a UI adjustment if we do want to try it. NOTE: This only makes these headers work for display. They aren't yet recognized as targets by the drag list UI, so you can't drag cards into an empty group. I'll tackle that in a followup. Test Plan: {F6257686} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20247 --- resources/celerity/map.php | 84 ++++++++------- .../maniphest/storage/ManiphestTask.php | 1 + .../PhabricatorProjectBoardViewController.php | 40 +++++++ .../css/phui/workboards/phui-workpanel.css | 13 +++ .../js/application/projects/WorkboardBoard.js | 47 ++++++++ .../js/application/projects/WorkboardCard.js | 4 + .../application/projects/WorkboardColumn.js | 101 +++++++++++++----- .../application/projects/WorkboardHeader.js | 38 +++++++ .../projects/WorkboardHeaderTemplate.js | 28 +++++ .../projects/behavior-project-boards.js | 10 ++ 10 files changed, 306 insertions(+), 60 deletions(-) create mode 100644 webroot/rsrc/js/application/projects/WorkboardHeader.js create mode 100644 webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 5118dc741d..9bfb432f42 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -178,7 +178,7 @@ return array( 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', 'rsrc/css/phui/workboards/phui-workboard.css' => '74fc9d98', 'rsrc/css/phui/workboards/phui-workcard.css' => '8c536f90', - 'rsrc/css/phui/workboards/phui-workpanel.css' => '7e12d43c', + 'rsrc/css/phui/workboards/phui-workpanel.css' => 'bc16cf33', 'rsrc/css/sprite-login.css' => '18b368a6', 'rsrc/css/sprite-tokens.css' => 'f1896dc5', 'rsrc/css/syntax/syntax-default.css' => '055fc231', @@ -409,11 +409,13 @@ 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' => 'fd96a6e8', - 'rsrc/js/application/projects/WorkboardCard.js' => '9a513421', - 'rsrc/js/application/projects/WorkboardColumn.js' => '1f71e559', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'e4e2d107', + 'rsrc/js/application/projects/WorkboardCard.js' => 'c23ddfde', + 'rsrc/js/application/projects/WorkboardColumn.js' => 'fd9cb972', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', - 'rsrc/js/application/projects/behavior-project-boards.js' => '05c74d65', + 'rsrc/js/application/projects/WorkboardHeader.js' => '354c5c0e', + 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '9b86cd0d', + 'rsrc/js/application/projects/behavior-project-boards.js' => 'a3f6b67f', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', 'rsrc/js/application/projects/behavior-reorder-columns.js' => '8ac32fd9', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', @@ -655,7 +657,7 @@ return array( 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', - 'javelin-behavior-project-boards' => '05c74d65', + 'javelin-behavior-project-boards' => 'a3f6b67f', 'javelin-behavior-project-create' => '34c53422', 'javelin-behavior-quicksand-blacklist' => '5a6f6a06', 'javelin-behavior-read-only-warning' => 'b9109f8f', @@ -727,10 +729,12 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'fd96a6e8', - 'javelin-workboard-card' => '9a513421', - 'javelin-workboard-column' => '1f71e559', + 'javelin-workboard-board' => 'e4e2d107', + 'javelin-workboard-card' => 'c23ddfde', + 'javelin-workboard-column' => 'fd9cb972', 'javelin-workboard-controller' => '42c7a5a7', + 'javelin-workboard-header' => '354c5c0e', + 'javelin-workboard-header-template' => '9b86cd0d', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', @@ -854,7 +858,7 @@ return array( 'phui-workboard-color-css' => 'e86de308', 'phui-workboard-view-css' => '74fc9d98', 'phui-workcard-view-css' => '8c536f90', - 'phui-workpanel-view-css' => '7e12d43c', + 'phui-workpanel-view-css' => 'bc16cf33', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', 'phuix-autocomplete' => '8f139ef0', @@ -915,15 +919,6 @@ return array( 'javelin-dom', 'javelin-workflow', ), - '05c74d65' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-workboard-controller', - ), '05d290ef' => array( 'javelin-install', 'javelin-util', @@ -1034,10 +1029,6 @@ return array( 'javelin-behavior', 'javelin-dom', ), - '1f71e559' => array( - 'javelin-install', - 'javelin-workboard-card', - ), '1ff278aa' => array( 'phui-button-css', ), @@ -1172,6 +1163,9 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), + '354c5c0e' => array( + 'javelin-install', + ), '37b8a04a' => array( 'javelin-install', 'javelin-util', @@ -1535,9 +1529,6 @@ return array( 'javelin-install', 'javelin-dom', ), - '7e12d43c' => array( - 'phui-workcard-view-css', - ), '80bff3af' => array( 'javelin-install', 'javelin-typeahead-source', @@ -1701,9 +1692,6 @@ return array( 'javelin-dom', 'javelin-router', ), - '9a513421' => array( - 'javelin-install', - ), '9aae2b66' => array( 'javelin-install', 'javelin-util', @@ -1713,6 +1701,9 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + '9b86cd0d' => array( + 'javelin-install', + ), '9cec214e' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1737,6 +1728,15 @@ return array( 'a241536a' => array( 'javelin-install', ), + 'a3f6b67f' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-workboard-controller', + ), 'a4356cde' => array( 'javelin-install', 'javelin-dom', @@ -1887,6 +1887,9 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'bc16cf33' => array( + 'phui-workcard-view-css', + ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1903,6 +1906,9 @@ return array( 'javelin-stratcom', 'javelin-uri', ), + 'c23ddfde' => array( + 'javelin-install', + ), 'c2c500a7' => array( 'javelin-install', 'javelin-dom', @@ -2019,6 +2025,16 @@ return array( 'javelin-dom', 'javelin-history', ), + 'e4e2d107' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + 'javelin-workboard-header-template', + ), 'e562708c' => array( 'javelin-install', ), @@ -2120,14 +2136,10 @@ return array( 'javelin-magical-init', 'javelin-util', ), - 'fd96a6e8' => array( + 'fd9cb972' => array( 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', + 'javelin-workboard-card', + 'javelin-workboard-header', ), 'fdc13e4e' => array( 'javelin-install', diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 400bace650..88ade9c35a 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -306,6 +306,7 @@ final class ManiphestTask extends ManiphestDAO return array( 'status' => $this->getStatus(), 'points' => (double)$this->getPoints(), + 'priority' => $this->getPriority(), ); } diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index f2965892d7..857004caf0 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -621,6 +621,45 @@ final class PhabricatorProjectBoardViewController $board->addPanel($panel); } + // It's possible for tasks to have an invalid/unknown priority in the + // database. We still want to generate a header for these tasks so we + // don't break the workboard. + $priorities = + ManiphestTaskPriority::getTaskPriorityMap() + + mpull($all_tasks, null, 'getPriority'); + $priorities = array_keys($priorities); + + $headers = array(); + foreach ($priorities as $priority) { + $header_key = sprintf('priority(%s)', $priority); + + $priority_name = ManiphestTaskPriority::getTaskPriorityName($priority); + $priority_color = ManiphestTaskPriority::getTaskPriorityColor($priority); + $priority_icon = ManiphestTaskPriority::getTaskPriorityIcon($priority); + + $icon_view = id(new PHUIIconView()) + ->setIcon("{$priority_icon} {$priority_color}"); + + $template = phutil_tag( + 'li', + array( + 'class' => 'workboard-group-header', + ), + array( + $icon_view, + $priority_name, + )); + + $headers[] = array( + 'order' => 'priority', + 'key' => $header_key, + 'template' => hsprintf('%s', $template), + 'vector' => array( + (int)-$priority, + ), + ); + } + $behavior_config = array( 'moveURI' => $this->getApplicationURI('move/'.$project->getID().'/'), 'uploadURI' => '/file/dropupload/', @@ -630,6 +669,7 @@ final class PhabricatorProjectBoardViewController 'boardPHID' => $project->getPHID(), 'order' => $this->sortKey, + 'headers' => $headers, 'templateMap' => $templates, 'columnMaps' => $column_maps, 'orderMaps' => mpull($all_tasks, 'getWorkboardOrderVectors'), diff --git a/webroot/rsrc/css/phui/workboards/phui-workpanel.css b/webroot/rsrc/css/phui/workboards/phui-workpanel.css index fb7415ff20..2dac6b2233 100644 --- a/webroot/rsrc/css/phui/workboards/phui-workpanel.css +++ b/webroot/rsrc/css/phui/workboards/phui-workpanel.css @@ -145,3 +145,16 @@ .phui-workpanel-view.workboard-column-drop-target .phui-box-grey { border-color: {$lightblueborder}; } + +.workboard-group-header { + background: rgba({$alphablue}, 0.10); + padding: 4px 8px; + margin: 0 0 8px -8px; + border-bottom: 1px solid {$lightgreyborder}; + font-weight: bold; + color: {$darkgreytext}; +} + +.workboard-group-header .phui-icon-view { + margin-right: 8px; +} diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index b3f8e585d6..2ea38b07b2 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -7,6 +7,7 @@ * javelin-workflow * phabricator-draggable-list * javelin-workboard-column + * javelin-workboard-header-template * @javelin */ @@ -20,6 +21,7 @@ JX.install('WorkboardBoard', { this._templates = {}; this._orderMaps = {}; this._propertiesMap = {}; + this._headers = {}; this._buildColumns(); }, @@ -36,6 +38,7 @@ JX.install('WorkboardBoard', { _templates: null, _orderMaps: null, _propertiesMap: null, + _headers: null, getRoot: function() { return this._root; @@ -58,6 +61,36 @@ JX.install('WorkboardBoard', { return this; }, + getHeaderTemplate: function(header_key) { + if (!this._headers[header_key]) { + this._headers[header_key] = new JX.WorkboardHeaderTemplate(header_key); + } + + return this._headers[header_key]; + }, + + getHeaderTemplatesForOrder: function(order) { + var templates = []; + + for (var k in this._headers) { + var header = this._headers[k]; + + if (header.getOrder() !== order) { + continue; + } + + templates.push(header); + } + + templates.sort(JX.bind(this, this._sortHeaderTemplates)); + + return templates; + }, + + _sortHeaderTemplates: function(u, v) { + return this.compareVectors(u.getVector(), v.getVector()); + }, + setObjectProperties: function(phid, properties) { this._propertiesMap[phid] = properties; return this; @@ -84,6 +117,20 @@ JX.install('WorkboardBoard', { return this._orderMaps[phid][key]; }, + compareVectors: function(u_vec, v_vec) { + for (var ii = 0; ii < u_vec.length; ii++) { + if (u_vec[ii] > v_vec[ii]) { + return 1; + } + + if (u_vec[ii] < v_vec[ii]) { + return -1; + } + } + + return 0; + }, + start: function() { this._setupDragHandlers(); diff --git a/webroot/rsrc/js/application/projects/WorkboardCard.js b/webroot/rsrc/js/application/projects/WorkboardCard.js index b506e655c1..753eca40f1 100644 --- a/webroot/rsrc/js/application/projects/WorkboardCard.js +++ b/webroot/rsrc/js/application/projects/WorkboardCard.js @@ -40,6 +40,10 @@ JX.install('WorkboardCard', { return this.getProperties().status; }, + getPriority: function(order) { + return this.getProperties().priority; + }, + getNode: function() { if (!this._root) { var phid = this.getPHID(); diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index a94604a470..fdb165f589 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -2,6 +2,7 @@ * @provides javelin-workboard-column * @requires javelin-install * javelin-workboard-card + * javelin-workboard-header * @javelin */ @@ -21,6 +22,8 @@ JX.install('WorkboardColumn', { 'column-points-content'); this._cards = {}; + this._headers = {}; + this._objects = []; this._naturalOrder = []; }, @@ -29,11 +32,13 @@ JX.install('WorkboardColumn', { _root: null, _board: null, _cards: null, + _headers: null, _naturalOrder: null, _panel: null, _pointsNode: null, _pointsContentNode: null, _dirty: true, + _objects: null, getPHID: function() { return this._phid; @@ -148,24 +153,85 @@ JX.install('WorkboardColumn', { return this._dirty; }, + getHeader: function(key) { + if (!this._headers[key]) { + this._headers[key] = new JX.WorkboardHeader(this, key); + } + return this._headers[key]; + }, + + _getCardHeaderKey: function(card, order) { + switch (order) { + case 'priority': + return 'priority(' + card.getPriority() + ')'; + default: + return null; + } + }, + redraw: function() { var board = this.getBoard(); var order = board.getOrder(); var list; + var has_headers; if (order == 'natural') { list = this._getCardsSortedNaturally(); + has_headers = false; } else { list = this._getCardsSortedByKey(order); + has_headers = true; } - var content = []; - for (var ii = 0; ii < list.length; ii++) { + var ii; + var objects = []; + + var header_keys = []; + var seen_headers = {}; + if (has_headers) { + var header_templates = board.getHeaderTemplatesForOrder(order); + for (var k in header_templates) { + header_keys.push(header_templates[k].getHeaderKey()); + } + header_keys.reverse(); + } + + for (ii = 0; ii < list.length; ii++) { var card = list[ii]; - var node = card.getNode(); - content.push(node); + // If a column has a "High" priority card and a "Low" priority card, + // we need to add the "Normal" header in between them. This allows + // you to change priority to "Normal" even if there are no "Normal" + // cards in a column. + if (has_headers) { + var header_key = this._getCardHeaderKey(card, order); + if (!seen_headers[header_key]) { + while (header_keys.length) { + var next = header_keys.pop(); + + var header = this.getHeader(next); + objects.push(header); + seen_headers[header_key] = true; + + if (next === header_key) { + break; + } + } + } + } + + objects.push(card); + } + + this._objects = objects; + + var content = []; + for (ii = 0; ii < this._objects.length; ii++) { + var object = this._objects[ii]; + + var node = object.getNode(); + content.push(node); } JX.DOM.setContent(this.getRoot(), content); @@ -182,10 +248,10 @@ JX.install('WorkboardColumn', { var src_phid = JX.Stratcom.getData(src_node).objectPHID; var dst_phid = JX.Stratcom.getData(dst_node).objectPHID; - var u_vec = this.getBoard().getOrderVector(src_phid, order); - var v_vec = this.getBoard().getOrderVector(dst_phid, order); + var u_vec = board.getOrderVector(src_phid, order); + var v_vec = board.getOrderVector(dst_phid, order); - return this._compareVectors(u_vec, v_vec); + return board.compareVectors(u_vec, v_vec); }, setIsDropTarget: function(is_target) { @@ -218,24 +284,11 @@ JX.install('WorkboardColumn', { }, _sortCards: function(order, u, v) { - var u_vec = this.getBoard().getOrderVector(u.getPHID(), order); - var v_vec = this.getBoard().getOrderVector(v.getPHID(), order); + var board = this.getBoard(); + var u_vec = board.getOrderVector(u.getPHID(), order); + var v_vec = board.getOrderVector(v.getPHID(), order); - return this._compareVectors(u_vec, v_vec); - }, - - _compareVectors: function(u_vec, v_vec) { - for (var ii = 0; ii < u_vec.length; ii++) { - if (u_vec[ii] > v_vec[ii]) { - return 1; - } - - if (u_vec[ii] < v_vec[ii]) { - return -1; - } - } - - return 0; + return board.compareVectors(u_vec, v_vec); }, _redrawFrame: function() { diff --git a/webroot/rsrc/js/application/projects/WorkboardHeader.js b/webroot/rsrc/js/application/projects/WorkboardHeader.js new file mode 100644 index 0000000000..d6cfd137d0 --- /dev/null +++ b/webroot/rsrc/js/application/projects/WorkboardHeader.js @@ -0,0 +1,38 @@ +/** + * @provides javelin-workboard-header + * @requires javelin-install + * @javelin + */ + +JX.install('WorkboardHeader', { + + construct: function(column, header_key) { + this._column = column; + this._headerKey = header_key; + }, + + members: { + _root: null, + _column: null, + _headerKey: null, + + getColumn: function() { + return this._column; + }, + + getHeaderKey: function() { + return this._headerKey; + }, + + getNode: function() { + if (!this._root) { + var header_key = this.getHeaderKey(); + var board = this.getColumn().getBoard(); + var template = board.getHeaderTemplate(header_key).getTemplate(); + this._root = JX.$H(template).getFragment().firstChild; + } + return this._root; + } + } + +}); diff --git a/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js new file mode 100644 index 0000000000..c08652bed0 --- /dev/null +++ b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js @@ -0,0 +1,28 @@ +/** + * @provides javelin-workboard-header-template + * @requires javelin-install + * @javelin + */ + +JX.install('WorkboardHeaderTemplate', { + + construct: function(header_key) { + this._headerKey = header_key; + }, + + properties: { + template: null, + order: null, + vector: null + }, + + members: { + _headerKey: null, + + getHeaderKey: function() { + return this._headerKey; + } + + } + +}); diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index 83f41787ab..fd2c1a0fe6 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -105,6 +105,16 @@ JX.behavior('project-boards', function(config, statics) { board.setObjectProperties(property_phid, property_maps[property_phid]); } + var headers = config.headers; + for (var jj = 0; jj < headers.length; jj++) { + var header = headers[jj]; + + board.getHeaderTemplate(header.key) + .setOrder(header.order) + .setTemplate(header.template) + .setVector(header.vector); + } + board.start(); }); From 40af472ff59517258005bd2e8c06ec5ddd68d4d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 5 Mar 2019 07:38:35 -0800 Subject: [PATCH 04/25] Make drag-and-drop on workboards interact with priority column headers Summary: Ref T10333. Ref T8135. Depends on D20247. Allow users to drag-and-drop cards on a priority-sorted workboard under headers, even if the header has no other cards. As of D20247, headers show up but they aren't really interactive. Now, you can drag cards directly underneath a header (instead of only between other cards). For example, if a column has only one "Wishlist" task, you may drag it under the "High", "Normal", or "Low" priority headers to select a specific priority. (Some of this code still feels a little rough, but I think it will generalize once other types of sorting are available.) Test Plan: Dragged cards within and between priority groups, saw appropriate priority edits applied in every case I could come up with. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333, T8135 Differential Revision: https://secure.phabricator.com/D20248 --- resources/celerity/map.php | 80 +++++++++---------- .../maniphest/storage/ManiphestTask.php | 1 + .../PhabricatorProjectBoardViewController.php | 6 +- .../PhabricatorProjectMoveController.php | 77 ++++++++++++++---- .../storage/PhabricatorProjectColumn.php | 3 + .../js/application/projects/WorkboardBoard.js | 43 ++++++++-- .../js/application/projects/WorkboardCard.js | 4 + .../application/projects/WorkboardColumn.js | 61 +++++++++++--- .../application/projects/WorkboardHeader.js | 6 ++ .../projects/WorkboardHeaderTemplate.js | 3 +- .../projects/behavior-project-boards.js | 3 +- 11 files changed, 208 insertions(+), 79 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9bfb432f42..00a62f41d6 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -409,13 +409,13 @@ 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' => 'e4e2d107', - 'rsrc/js/application/projects/WorkboardCard.js' => 'c23ddfde', - 'rsrc/js/application/projects/WorkboardColumn.js' => 'fd9cb972', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'a4f1e85d', + 'rsrc/js/application/projects/WorkboardCard.js' => '887ef74f', + 'rsrc/js/application/projects/WorkboardColumn.js' => 'ca444dca', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', - 'rsrc/js/application/projects/WorkboardHeader.js' => '354c5c0e', - 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '9b86cd0d', - 'rsrc/js/application/projects/behavior-project-boards.js' => 'a3f6b67f', + 'rsrc/js/application/projects/WorkboardHeader.js' => '6e75daea', + 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '2d641f7d', + 'rsrc/js/application/projects/behavior-project-boards.js' => 'e2730b90', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', 'rsrc/js/application/projects/behavior-reorder-columns.js' => '8ac32fd9', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', @@ -657,7 +657,7 @@ return array( 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', - 'javelin-behavior-project-boards' => 'a3f6b67f', + 'javelin-behavior-project-boards' => 'e2730b90', 'javelin-behavior-project-create' => '34c53422', 'javelin-behavior-quicksand-blacklist' => '5a6f6a06', 'javelin-behavior-read-only-warning' => 'b9109f8f', @@ -729,12 +729,12 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'e4e2d107', - 'javelin-workboard-card' => 'c23ddfde', - 'javelin-workboard-column' => 'fd9cb972', + 'javelin-workboard-board' => 'a4f1e85d', + 'javelin-workboard-card' => '887ef74f', + 'javelin-workboard-column' => 'ca444dca', 'javelin-workboard-controller' => '42c7a5a7', - 'javelin-workboard-header' => '354c5c0e', - 'javelin-workboard-header-template' => '9b86cd0d', + 'javelin-workboard-header' => '6e75daea', + 'javelin-workboard-header-template' => '2d641f7d', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', @@ -1125,6 +1125,9 @@ return array( 'javelin-dom', 'phabricator-keyboard-shortcut', ), + '2d641f7d' => array( + 'javelin-install', + ), '2e255291' => array( 'javelin-install', 'javelin-util', @@ -1163,9 +1166,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '354c5c0e' => array( - 'javelin-install', - ), '37b8a04a' => array( 'javelin-install', 'javelin-util', @@ -1458,6 +1458,9 @@ return array( 'javelin-install', 'javelin-util', ), + '6e75daea' => array( + 'javelin-install', + ), 70245195 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1566,6 +1569,9 @@ return array( 'javelin-install', 'javelin-dom', ), + '887ef74f' => array( + 'javelin-install', + ), '89a1ae3a' => array( 'javelin-dom', 'javelin-util', @@ -1701,9 +1707,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - '9b86cd0d' => array( - 'javelin-install', - ), '9cec214e' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1728,15 +1731,6 @@ return array( 'a241536a' => array( 'javelin-install', ), - 'a3f6b67f' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-workboard-controller', - ), 'a4356cde' => array( 'javelin-install', 'javelin-dom', @@ -1762,6 +1756,16 @@ return array( 'javelin-request', 'javelin-util', ), + 'a4f1e85d' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + 'javelin-workboard-header-template', + ), 'a5257c4e' => array( 'javelin-install', 'javelin-dom', @@ -1906,9 +1910,6 @@ return array( 'javelin-stratcom', 'javelin-uri', ), - 'c23ddfde' => array( - 'javelin-install', - ), 'c2c500a7' => array( 'javelin-install', 'javelin-dom', @@ -1959,6 +1960,11 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + 'ca444dca' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', @@ -2025,15 +2031,14 @@ return array( 'javelin-dom', 'javelin-history', ), - 'e4e2d107' => array( - 'javelin-install', + 'e2730b90' => array( + 'javelin-behavior', 'javelin-dom', 'javelin-util', + 'javelin-vector', 'javelin-stratcom', 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - 'javelin-workboard-header-template', + 'javelin-workboard-controller', ), 'e562708c' => array( 'javelin-install', @@ -2136,11 +2141,6 @@ return array( 'javelin-magical-init', 'javelin-util', ), - 'fd9cb972' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), 'fdc13e4e' => array( 'javelin-install', ), diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 88ade9c35a..8d45ed45fa 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -252,6 +252,7 @@ final class ManiphestTask extends ManiphestDAO return array( PhabricatorProjectColumn::ORDER_PRIORITY => array( (int)-$this->getPriority(), + PhabricatorProjectColumn::NODETYPE_CARD, (double)-$this->getSubpriority(), (int)-$this->getID(), ), diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 857004caf0..32374ad8be 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -651,11 +651,15 @@ final class PhabricatorProjectBoardViewController )); $headers[] = array( - 'order' => 'priority', + 'order' => PhabricatorProjectColumn::ORDER_PRIORITY, 'key' => $header_key, 'template' => hsprintf('%s', $template), 'vector' => array( (int)-$priority, + PhabricatorProjectColumn::NODETYPE_HEADER, + ), + 'editProperties' => array( + PhabricatorProjectColumn::ORDER_PRIORITY => (int)$priority, ), ); } diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 29b70cfafc..09fca4692d 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -15,6 +15,14 @@ final class PhabricatorProjectMoveController $before_phid = $request->getStr('beforePHID'); $order = $request->getStr('order', PhabricatorProjectColumn::DEFAULT_ORDER); + $edit_header = null; + $raw_header = $request->getStr('header'); + if (strlen($raw_header)) { + $edit_header = phutil_json_decode($raw_header); + } else { + $edit_header = array(); + } + $project = id(new PhabricatorProjectQuery()) ->setViewer($viewer) ->requireCapabilities( @@ -87,10 +95,14 @@ final class PhabricatorProjectMoveController )); if ($order == PhabricatorProjectColumn::ORDER_PRIORITY) { + $header_priority = idx( + $edit_header, + PhabricatorProjectColumn::ORDER_PRIORITY); $priority_xactions = $this->getPriorityTransactions( $object, $after_phid, - $before_phid); + $before_phid, + $header_priority); foreach ($priority_xactions as $xaction) { $xactions[] = $xaction; } @@ -110,13 +122,33 @@ final class PhabricatorProjectMoveController private function getPriorityTransactions( ManiphestTask $task, $after_phid, - $before_phid) { + $before_phid, + $header_priority) { + + $xactions = array(); + $must_move = false; + + if ($header_priority !== null) { + if ($task->getPriority() !== $header_priority) { + $task = id(clone $task) + ->setPriority($header_priority); + + $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); + $keyword = head(idx($keyword_map, $header_priority)); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType( + ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) + ->setNewValue($keyword); + + $must_move = true; + } + } list($after_task, $before_task) = $this->loadPriorityTasks( $after_phid, $before_phid); - $must_move = false; if ($after_task && !$task->isLowerPriorityThan($after_task)) { $must_move = true; } @@ -125,10 +157,10 @@ final class PhabricatorProjectMoveController $must_move = true; } - // The move doesn't require a priority change to be valid, so don't - // change the priority since we are not being forced to. + // The move doesn't require a subpriority change to be valid, so don't + // change the subpriority since we are not being forced to. if (!$must_move) { - return array(); + return $xactions; } $try = array( @@ -139,28 +171,41 @@ final class PhabricatorProjectMoveController $pri = null; $sub = null; foreach ($try as $spec) { - list($task, $is_after) = $spec; + list($nearby_task, $is_after) = $spec; - if (!$task) { + if (!$nearby_task) { continue; } list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $task, + $nearby_task, $is_after); + // If we drag under a "Low" header between a "Normal" task and a "Low" + // task, we don't want to accept a subpriority assignment which changes + // our priority to "Normal". Only accept a subpriority that keeps us in + // the right primary priority. + if ($header_priority !== null) { + if ($pri !== $header_priority) { + continue; + } + } + // If we find a priority on the first try, don't keep going. break; } - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $pri)); - - $xactions = array(); if ($pri !== null) { - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); + if ($header_priority === null) { + $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); + $keyword = head(idx($keyword_map, $pri)); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType( + ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) + ->setNewValue($keyword); + } + $xactions[] = id(new ManiphestTransaction()) ->setTransactionType( ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) diff --git a/src/applications/project/storage/PhabricatorProjectColumn.php b/src/applications/project/storage/PhabricatorProjectColumn.php index 756c356ee1..03b9cbff70 100644 --- a/src/applications/project/storage/PhabricatorProjectColumn.php +++ b/src/applications/project/storage/PhabricatorProjectColumn.php @@ -16,6 +16,9 @@ final class PhabricatorProjectColumn const ORDER_NATURAL = 'natural'; const ORDER_PRIORITY = 'priority'; + const NODETYPE_HEADER = 0; + const NODETYPE_CARD = 1; + protected $name; protected $status; protected $projectPHID; diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index 2ea38b07b2..b0bcfe97c6 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -161,11 +161,15 @@ JX.install('WorkboardBoard', { var list = new JX.DraggableList('project-card', column.getRoot()) .setOuterContainer(this.getRoot()) - .setFindItemsHandler(JX.bind(column, column.getCardNodes)) + .setFindItemsHandler(JX.bind(column, column.getDropTargetNodes)) .setCanDragX(true) .setHasInfiniteHeight(true) .setIsDropTargetHandler(JX.bind(column, column.setIsDropTarget)); + var default_handler = list.getGhostHandler(); + list.setGhostHandler( + JX.bind(column, column.handleDragGhost, default_handler)); + if (this.getOrder() !== 'natural') { list.setCompareHandler(JX.bind(column, column.compareHandler)); } @@ -198,16 +202,39 @@ JX.install('WorkboardBoard', { order: this.getOrder() }; - if (after_node) { - data.afterPHID = JX.Stratcom.getData(after_node).objectPHID; + var after_data; + var after_card = after_node; + while (after_card) { + after_data = JX.Stratcom.getData(after_card); + if (after_data.objectPHID) { + break; + } + after_card = after_card.previousSibling; } - var before_node = item.nextSibling; - if (before_node) { - var before_phid = JX.Stratcom.getData(before_node).objectPHID; - if (before_phid) { - data.beforePHID = before_phid; + if (after_data) { + data.afterPHID = after_data.objectPHID; + } + + var before_data; + var before_card = item.nextSibling; + while (before_card) { + before_data = JX.Stratcom.getData(before_card); + if (before_data.objectPHID) { + break; } + before_card = before_card.nextSibling; + } + + if (before_data) { + data.beforePHID = before_data.objectPHID; + } + + var header_key = JX.Stratcom.getData(after_node).headerKey; + if (header_key) { + var properties = this.getHeaderTemplate(header_key) + .getEditProperties(); + data.header = JX.JSON.stringify(properties); } var visible_phids = []; diff --git a/webroot/rsrc/js/application/projects/WorkboardCard.js b/webroot/rsrc/js/application/projects/WorkboardCard.js index 753eca40f1..9da3b7e69f 100644 --- a/webroot/rsrc/js/application/projects/WorkboardCard.js +++ b/webroot/rsrc/js/application/projects/WorkboardCard.js @@ -55,6 +55,10 @@ JX.install('WorkboardCard', { return this._root; }, + isWorkboardHeader: function() { + return false; + }, + redraw: function() { var old_node = this._root; this._root = null; diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index fdb165f589..262e80c2da 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -52,6 +52,10 @@ JX.install('WorkboardColumn', { return this._cards; }, + _getObjects: function() { + return this._objects; + }, + getCard: function(phid) { return this._cards[phid]; }, @@ -126,12 +130,13 @@ JX.install('WorkboardColumn', { return this; }, - getCardNodes: function() { - var cards = this.getCards(); + getDropTargetNodes: function() { + var objects = this._getObjects(); var nodes = []; - for (var k in cards) { - nodes.push(cards[k].getNode()); + for (var ii = 0; ii < objects.length; ii++) { + var object = objects[ii]; + nodes.push(object.getNode()); } return nodes; @@ -160,6 +165,32 @@ JX.install('WorkboardColumn', { return this._headers[key]; }, + handleDragGhost: function(default_handler, ghost, node) { + // If the column has headers, don't let the user drag a card above + // the topmost header: for example, you can't change a task to have + // a priority higher than the highest possible priority. + + if (this._hasColumnHeaders()) { + if (!node) { + return false; + } + } + + return default_handler(ghost, node); + }, + + _hasColumnHeaders: function() { + var board = this.getBoard(); + var order = board.getOrder(); + + switch (order) { + case 'natural': + return false; + } + + return true; + }, + _getCardHeaderKey: function(card, order) { switch (order) { case 'priority': @@ -174,18 +205,16 @@ JX.install('WorkboardColumn', { var order = board.getOrder(); var list; - var has_headers; if (order == 'natural') { list = this._getCardsSortedNaturally(); - has_headers = false; } else { list = this._getCardsSortedByKey(order); - has_headers = true; } var ii; var objects = []; + var has_headers = this._hasColumnHeaders(); var header_keys = []; var seen_headers = {}; if (has_headers) { @@ -245,15 +274,23 @@ JX.install('WorkboardColumn', { var board = this.getBoard(); var order = board.getOrder(); - var src_phid = JX.Stratcom.getData(src_node).objectPHID; - var dst_phid = JX.Stratcom.getData(dst_node).objectPHID; - - var u_vec = board.getOrderVector(src_phid, order); - var v_vec = board.getOrderVector(dst_phid, order); + var u_vec = this._getNodeOrderVector(src_node, order); + var v_vec = this._getNodeOrderVector(dst_node, order); return board.compareVectors(u_vec, v_vec); }, + _getNodeOrderVector: function(node, order) { + var board = this.getBoard(); + var data = JX.Stratcom.getData(node); + + if (data.objectPHID) { + return board.getOrderVector(data.objectPHID, order); + } + + return board.getHeaderTemplate(data.headerKey).getVector(); + }, + setIsDropTarget: function(is_target) { var node = this.getWorkpanelNode(); JX.DOM.alterClass(node, 'workboard-column-drop-target', is_target); diff --git a/webroot/rsrc/js/application/projects/WorkboardHeader.js b/webroot/rsrc/js/application/projects/WorkboardHeader.js index d6cfd137d0..0a8f4d9681 100644 --- a/webroot/rsrc/js/application/projects/WorkboardHeader.js +++ b/webroot/rsrc/js/application/projects/WorkboardHeader.js @@ -30,8 +30,14 @@ JX.install('WorkboardHeader', { var board = this.getColumn().getBoard(); var template = board.getHeaderTemplate(header_key).getTemplate(); this._root = JX.$H(template).getFragment().firstChild; + + JX.Stratcom.getData(this._root).headerKey = header_key; } return this._root; + }, + + isWorkboardHeader: function() { + return true; } } diff --git a/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js index c08652bed0..add37d9c25 100644 --- a/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js +++ b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js @@ -13,7 +13,8 @@ JX.install('WorkboardHeaderTemplate', { properties: { template: null, order: null, - vector: null + vector: null, + editProperties: null }, members: { diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index fd2c1a0fe6..6427e1de4c 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -112,7 +112,8 @@ JX.behavior('project-boards', function(config, statics) { board.getHeaderTemplate(header.key) .setOrder(header.order) .setTemplate(header.template) - .setVector(header.vector); + .setVector(header.vector) + .setEditProperties(header.editProperties); } board.start(); From 46ab71f834dc75d0a37ded7645ea17143a7e3ee1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Mar 2019 11:52:05 -0700 Subject: [PATCH 05/25] Fix "abou" typo of "about" in SearchEngine API methods Summary: See . Test Plan: Read carefully. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20280 --- .../search/engine/PhabricatorSearchEngineAPIMethod.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index 510ad91864..f4e2dc918f 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -403,7 +403,7 @@ EOTEXT $info = pht(<< Date: Thu, 7 Mar 2019 14:04:27 -0800 Subject: [PATCH 06/25] Remove opacity effects for left-side / right-side diff text selection Summary: These effects feel like they're possibly overkill, since other CSS rules make the selection reticle behave correctly and the implementation is relatively intuitive. Or not, either way. Test Plan: Selected text on either side of a 2-up diff, no more opacity effects. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20264 --- resources/celerity/map.php | 12 ++++++------ .../css/application/differential/changeset-view.css | 5 ----- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 00a62f41d6..0e963428bc 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', 'core.pkg.js' => 'b96c872e', - 'differential.pkg.css' => '1755a478', + 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '4193eeff', + 'rsrc/css/application/differential/changeset-view.css' => 'bde53589', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -542,7 +542,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => '4193eeff', + 'differential-changeset-view-css' => 'bde53589', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1210,9 +1210,6 @@ return array( 'javelin-behavior', 'javelin-uri', ), - '4193eeff' => array( - 'phui-inline-comment-view-css', - ), '4234f572' => array( 'syntax-default-css', ), @@ -1901,6 +1898,9 @@ return array( 'javelin-vector', 'javelin-stratcom', ), + 'bde53589' => array( + 'phui-inline-comment-view-css', + ), 'c03f2fb4' => array( 'javelin-install', ), diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 844690abd3..233ac4cca5 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -451,7 +451,6 @@ unselectable. */ -ms-user-select: none; -webkit-user-select: none; user-select: none; - opacity: 0.5; } .differential-diff.copy-l > tbody > tr > td:nth-child(2) { @@ -459,7 +458,6 @@ unselectable. */ -ms-user-select: auto; -webkit-user-select: auto; user-select: auto; - opacity: 1; } .differential-diff.copy-l > tbody > tr > td.show-more:nth-child(2) { @@ -467,7 +465,6 @@ unselectable. */ -ms-user-select: none; -webkit-user-select: none; user-select: none; - opacity: 0.5; } .differential-diff.copy-r > tbody > tr > td:nth-child(5) { @@ -475,7 +472,6 @@ unselectable. */ -ms-user-select: auto; -webkit-user-select: auto; user-select: auto; - opacity: 1; } .differential-diff.copy-l > tbody > tr.inline > td, @@ -484,5 +480,4 @@ unselectable. */ -ms-user-select: none; -webkit-user-select: none; user-select: none; - opacity: 0.5; } From 00543f0620d1c9ecfbbf89bfef92fa0d1a002d62 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Mar 2019 10:55:57 -0800 Subject: [PATCH 07/25] Remove the ability to drag tasks up and down on (non-Workboard) priority list views Summary: Ref T13074. Today, in normal task list views in Maniphest (not workboards), you can (sometimes) reorder tasks if the view is priority-sorted. I suspect no one ever does this, few users know it's supported, and that it was basically rendered obsolete the day we shipped workboards. This also means that we need to maintain a global "subpriority" for tasks, which distinguishes between different tasks at the same priority level (e.g., "High") and maintains a consistent ordering on workboards. As we move toward making workboards more flexible (e.g., group by author / owner / custom fields), I'd like to try moving away from "subpriority" and possibly removing it entirely, in favor of "natural order", which basically means "we kind of remember where you put the card and it works a bit like a sticky note". Currently, the "natural order" and "subpriority" systems are sort of similar but also sort of in conflict, and the "subpriority" system can't really be extended while the "natural order / column position" system can. The only real reason to have a global "subpriority" is to support the list-view drag-and-drop. It's possible I'm wrong about this and a bunch of users love this feature, but we can re-evaluate if we get feedback in this vein. (This just removes UI, the actual subpriority system is still intact and still used on workboards.) Test Plan: Viewed task lists, was no longer able to drag stuff. Grepped for affected symbols. Dragged stuff in remaining grippable lists, like "Edit Forms" in EditEngine config. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13074 Differential Revision: https://secure.phabricator.com/D20263 --- resources/celerity/map.php | 28 +++----- resources/celerity/packages.php | 1 - src/__phutil_library_map__.php | 2 - .../PhabricatorManiphestApplication.php | 1 - .../controller/ManiphestController.php | 1 - .../ManiphestSubpriorityController.php | 70 ------------------ .../ManiphestTaskEditController.php | 1 - .../query/ManiphestTaskSearchEngine.php | 3 - .../maniphest/view/ManiphestTaskListView.php | 14 +--- .../view/ManiphestTaskResultListView.php | 30 -------- src/view/phui/PHUIObjectItemView.php | 20 ++---- .../maniphest/behavior-batch-selector.js | 13 ---- .../maniphest/behavior-subpriorityeditor.js | 72 ------------------- 13 files changed, 16 insertions(+), 240 deletions(-) delete mode 100644 src/applications/maniphest/controller/ManiphestSubpriorityController.php delete mode 100644 webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0e963428bc..58aa364de5 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -16,7 +16,7 @@ return array( 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', - 'maniphest.pkg.js' => '286955ae', + 'maniphest.pkg.js' => 'c9308721', 'rsrc/audio/basic/alert.mp3' => '17889334', 'rsrc/audio/basic/bing.mp3' => 'a817a0c3', 'rsrc/audio/basic/pock.mp3' => '0fa843d0', @@ -395,10 +395,9 @@ return array( 'rsrc/js/application/herald/HeraldRuleEditor.js' => '27daef73', 'rsrc/js/application/herald/PathTypeahead.js' => 'ad486db3', 'rsrc/js/application/herald/herald-rule-editor.js' => '0922e81d', - 'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'cffd39b4', + 'rsrc/js/application/maniphest/behavior-batch-selector.js' => '139ef688', 'rsrc/js/application/maniphest/behavior-line-chart.js' => 'c8147a20', 'rsrc/js/application/maniphest/behavior-list-edit.js' => 'c687e867', - 'rsrc/js/application/maniphest/behavior-subpriorityeditor.js' => '8400307c', 'rsrc/js/application/owners/OwnersPathEditor.js' => '2a8b62d9', 'rsrc/js/application/owners/owners-path-editor.js' => 'ff688a7a', 'rsrc/js/application/passphrase/passphrase-credential-control.js' => '48fe33d0', @@ -619,9 +618,8 @@ return array( 'javelin-behavior-lightbox-attachments' => 'c7e748bf', 'javelin-behavior-line-chart' => 'c8147a20', 'javelin-behavior-linked-container' => '74446546', - 'javelin-behavior-maniphest-batch-selector' => 'cffd39b4', + 'javelin-behavior-maniphest-batch-selector' => '139ef688', 'javelin-behavior-maniphest-list-editor' => 'c687e867', - 'javelin-behavior-maniphest-subpriority-editor' => '8400307c', 'javelin-behavior-owners-path-editor' => 'ff688a7a', 'javelin-behavior-passphrase-credential-control' => '48fe33d0', 'javelin-behavior-phabricator-active-nav' => '7353f43d', @@ -998,6 +996,12 @@ return array( 'javelin-uri', 'phabricator-keyboard-shortcut', ), + '139ef688' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + ), '1c850a26' => array( 'javelin-install', 'javelin-util', @@ -1539,13 +1543,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - '8400307c' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - ), '8437c663' => array( 'javelin-install', 'javelin-dom', @@ -1970,12 +1967,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'cffd39b4' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - ), 'd0a85a85' => array( 'javelin-dom', 'javelin-util', @@ -2362,7 +2353,6 @@ return array( ), 'maniphest.pkg.js' => array( 'javelin-behavior-maniphest-batch-selector', - 'javelin-behavior-maniphest-subpriority-editor', 'javelin-behavior-maniphest-list-editor', ), ), diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index deef3633a8..6dbb662288 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -218,7 +218,6 @@ return array( ), 'maniphest.pkg.js' => array( 'javelin-behavior-maniphest-batch-selector', - 'javelin-behavior-maniphest-subpriority-editor', 'javelin-behavior-maniphest-list-editor', ), ); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 651fad9d12..3cab91717a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1709,7 +1709,6 @@ phutil_register_library_map(array( 'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php', 'ManiphestStatusSearchConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestStatusSearchConduitAPIMethod.php', 'ManiphestStatusesConfigType' => 'applications/maniphest/config/ManiphestStatusesConfigType.php', - 'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php', 'ManiphestSubtypesConfigType' => 'applications/maniphest/config/ManiphestSubtypesConfigType.php', 'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php', 'ManiphestTaskAssignHeraldAction' => 'applications/maniphest/herald/ManiphestTaskAssignHeraldAction.php', @@ -7406,7 +7405,6 @@ phutil_register_library_map(array( 'ManiphestStatusEmailCommand' => 'ManiphestEmailCommand', 'ManiphestStatusSearchConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestStatusesConfigType' => 'PhabricatorJSONConfigType', - 'ManiphestSubpriorityController' => 'ManiphestController', 'ManiphestSubtypesConfigType' => 'PhabricatorJSONConfigType', 'ManiphestTask' => array( 'ManiphestDAO', diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index ec732791fa..8ed20416bb 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -54,7 +54,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { => 'ManiphestTaskEditController', 'subtask/(?P[1-9]\d*)/' => 'ManiphestTaskSubtaskController', ), - 'subpriority/' => 'ManiphestSubpriorityController', 'graph/(?P[1-9]\d*)/' => 'ManiphestTaskGraphController', ), ); diff --git a/src/applications/maniphest/controller/ManiphestController.php b/src/applications/maniphest/controller/ManiphestController.php index 872d3f7b38..51a243afac 100644 --- a/src/applications/maniphest/controller/ManiphestController.php +++ b/src/applications/maniphest/controller/ManiphestController.php @@ -53,7 +53,6 @@ abstract class ManiphestController extends PhabricatorController { $view = id(new ManiphestTaskListView()) ->setUser($user) - ->setShowSubpriorityControls(!$request->getStr('ungrippable')) ->setShowBatchControls(true) ->setHandles($handles) ->setTasks(array($task)); diff --git a/src/applications/maniphest/controller/ManiphestSubpriorityController.php b/src/applications/maniphest/controller/ManiphestSubpriorityController.php deleted file mode 100644 index 8869b6a327..0000000000 --- a/src/applications/maniphest/controller/ManiphestSubpriorityController.php +++ /dev/null @@ -1,70 +0,0 @@ -getViewer(); - - if (!$request->validateCSRF()) { - return new Aphront403Response(); - } - - $task = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withIDs(array($request->getInt('task'))) - ->needProjectPHIDs(true) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->executeOne(); - if (!$task) { - return new Aphront404Response(); - } - - if ($request->getInt('after')) { - $after_task = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withIDs(array($request->getInt('after'))) - ->executeOne(); - if (!$after_task) { - return new Aphront404Response(); - } - list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $after_task, - $is_after = true); - } else { - list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( - $request->getInt('priority'), - $is_end = false); - } - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $pri)); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($viewer) - ->setContinueOnMissingFields(true) - ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); - - $editor->applyTransactions($task, $xactions); - - return id(new AphrontAjaxResponse())->setContent( - array( - 'tasks' => $this->renderSingleTask($task), - )); - } - -} diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 9483529138..341997e325 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -5,7 +5,6 @@ final class ManiphestTaskEditController extends ManiphestController { public function handleRequest(AphrontRequest $request) { return id(new ManiphestEditEngine()) ->setController($this) - ->addContextParameter('ungrippable') ->addContextParameter('responseType') ->addContextParameter('columnPHID') ->addContextParameter('order') diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index f4a1f2b344..4c69c604e4 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -366,10 +366,8 @@ final class ManiphestTaskSearchEngine $viewer = $this->requireViewer(); if ($this->isPanelContext()) { - $can_edit_priority = false; $can_bulk_edit = false; } else { - $can_edit_priority = true; $can_bulk_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $this->getApplication(), @@ -380,7 +378,6 @@ final class ManiphestTaskSearchEngine ->setUser($viewer) ->setTasks($tasks) ->setSavedQuery($saved) - ->setCanEditPriority($can_edit_priority) ->setCanBatchEdit($can_bulk_edit) ->setShowBatchControls($this->showBatchControls); diff --git a/src/applications/maniphest/view/ManiphestTaskListView.php b/src/applications/maniphest/view/ManiphestTaskListView.php index 6bf5daf29b..f9ad9e6046 100644 --- a/src/applications/maniphest/view/ManiphestTaskListView.php +++ b/src/applications/maniphest/view/ManiphestTaskListView.php @@ -5,7 +5,6 @@ final class ManiphestTaskListView extends ManiphestView { private $tasks; private $handles; private $showBatchControls; - private $showSubpriorityControls; private $noDataString; public function setTasks(array $tasks) { @@ -25,11 +24,6 @@ final class ManiphestTaskListView extends ManiphestView { return $this; } - public function setShowSubpriorityControls($show_subpriority_controls) { - $this->showSubpriorityControls = $show_subpriority_controls; - return $this; - } - public function setNoDataString($text) { $this->noDataString = $text; return $this; @@ -102,10 +96,7 @@ final class ManiphestTaskListView extends ManiphestView { phabricator_datetime($task->getDateModified(), $this->getUser())); } - if ($this->showSubpriorityControls) { - $item->setGrippable(true); - } - if ($this->showSubpriorityControls || $this->showBatchControls) { + if ($this->showBatchControls) { $item->addSigil('maniphest-task'); } @@ -134,9 +125,6 @@ final class ManiphestTaskListView extends ManiphestView { if ($this->showBatchControls) { $href = new PhutilURI('/maniphest/task/edit/'.$task->getID().'/'); - if (!$this->showSubpriorityControls) { - $href->replaceQueryParam('ungrippable', 'true'); - } $item->addAction( id(new PHUIListItemView()) ->setIcon('fa-pencil') diff --git a/src/applications/maniphest/view/ManiphestTaskResultListView.php b/src/applications/maniphest/view/ManiphestTaskResultListView.php index 6aafcbdccb..cc2a135292 100644 --- a/src/applications/maniphest/view/ManiphestTaskResultListView.php +++ b/src/applications/maniphest/view/ManiphestTaskResultListView.php @@ -4,7 +4,6 @@ final class ManiphestTaskResultListView extends ManiphestView { private $tasks; private $savedQuery; - private $canEditPriority; private $canBatchEdit; private $showBatchControls; @@ -18,11 +17,6 @@ final class ManiphestTaskResultListView extends ManiphestView { return $this; } - public function setCanEditPriority($can_edit_priority) { - $this->canEditPriority = $can_edit_priority; - return $this; - } - public function setCanBatchEdit($can_batch_edit) { $this->canBatchEdit = $can_batch_edit; return $this; @@ -54,28 +48,12 @@ final class ManiphestTaskResultListView extends ManiphestView { $group_parameter, $handles); - $can_edit_priority = $this->canEditPriority; - - $can_drag = ($order_parameter == 'priority') && - ($can_edit_priority) && - ($group_parameter == 'none' || $group_parameter == 'priority'); - - if (!$viewer->isLoggedIn()) { - // TODO: (T7131) Eventually, we conceivably need to make each task - // draggable individually, since the user may be able to edit some but - // not others. - $can_drag = false; - } - $result = array(); $lists = array(); foreach ($groups as $group => $list) { $task_list = new ManiphestTaskListView(); $task_list->setShowBatchControls($this->showBatchControls); - if ($can_drag) { - $task_list->setShowSubpriorityControls(true); - } $task_list->setUser($viewer); $task_list->setTasks($list); $task_list->setHandles($handles); @@ -91,14 +69,6 @@ final class ManiphestTaskResultListView extends ManiphestView { } - if ($can_drag) { - Javelin::initBehavior( - 'maniphest-subpriority-editor', - array( - 'uri' => '/maniphest/subpriority/', - )); - } - return array( $lists, $this->showBatchControls ? $this->renderBatchEditor($query) : null, diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php index e1c67c7f32..463f34a2a0 100644 --- a/src/view/phui/PHUIObjectItemView.php +++ b/src/view/phui/PHUIObjectItemView.php @@ -414,25 +414,17 @@ final class PHUIObjectItemView extends AphrontTagView { )); } - // Wrap the header content in a with the "slippery" sigil. This - // prevents us from beginning a drag if you click the text (like "T123"), - // but not if you click the white space after the header. $header = phutil_tag( 'div', array( 'class' => 'phui-oi-name', ), - javelin_tag( - 'span', - array( - 'sigil' => 'slippery', - ), - array( - $this->headIcons, - $header_name, - $header_link, - $description_tag, - ))); + array( + $this->headIcons, + $header_name, + $header_link, + $description_tag, + )); $icons = array(); if ($this->icons) { diff --git a/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js b/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js index b62f40a589..b64abc3503 100644 --- a/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js +++ b/webroot/rsrc/js/application/maniphest/behavior-batch-selector.js @@ -41,19 +41,6 @@ JX.behavior('maniphest-batch-selector', function(config) { update(); }; - var redraw = function (task) { - var selected = is_selected(task); - change(task, selected); - }; - JX.Stratcom.listen( - 'subpriority-changed', - null, - function (e) { - e.kill(); - var data = e.getData(); - redraw(data.task); - }); - // Change all tasks to some state (used by "select all" / "clear selection" // buttons). diff --git a/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js b/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js deleted file mode 100644 index 82f16854f8..0000000000 --- a/webroot/rsrc/js/application/maniphest/behavior-subpriorityeditor.js +++ /dev/null @@ -1,72 +0,0 @@ -/** - * @provides javelin-behavior-maniphest-subpriority-editor - * @requires javelin-behavior - * javelin-dom - * javelin-stratcom - * javelin-workflow - * phabricator-draggable-list - */ - -JX.behavior('maniphest-subpriority-editor', function(config) { - - var draggable = new JX.DraggableList('maniphest-task') - .setFindItemsHandler(function() { - var tasks = JX.DOM.scry(document.body, 'li', 'maniphest-task'); - var heads = JX.DOM.scry(document.body, 'div', 'task-group'); - return tasks.concat(heads); - }) - .setGhostHandler(function(ghost, target) { - if (!target) { - // The user is trying to drag a task above the first group header; - // don't permit that since it doesn't make sense. - return false; - } - - if (target.nextSibling) { - if (JX.DOM.isType(target, 'div')) { - target.nextSibling.insertBefore(ghost, target.nextSibling.firstChild); - } else { - target.parentNode.insertBefore(ghost, target.nextSibling); - } - } else { - target.parentNode.appendChild(ghost); - } - }); - - draggable.listen('shouldBeginDrag', function(e) { - if (e.getNode('slippery') || e.getNode('maniphest-edit-task')) { - JX.Stratcom.context().kill(); - } - }); - - draggable.listen('didDrop', function(node, after) { - var data = { - task: JX.Stratcom.getData(node).taskID - }; - - if (JX.DOM.isType(after, 'div')) { - data.priority = JX.Stratcom.getData(after).priority; - } else { - data.after = JX.Stratcom.getData(after).taskID; - } - - draggable.lock(); - JX.DOM.alterClass(node, 'drag-sending', true); - - var onresponse = function(r) { - var nodes = JX.$H(r.tasks).getFragment().firstChild; - var task = JX.DOM.find(nodes, 'li', 'maniphest-task'); - JX.DOM.replace(node, task); - draggable.unlock(); - JX.Stratcom.invoke( - 'subpriority-changed', - null, - { 'task' : task }); - }; - - new JX.Workflow(config.uri, data) - .setHandler(onresponse) - .start(); - }); - -}); From 46ed8d4a5ea7adcd5c8000e58891f9b9bae2d4b4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 08:43:55 -0700 Subject: [PATCH 08/25] On Workboards, sort groups by "natural order", not subpriority Summary: Depends on D20263. Ref T10333. I want to add groups like "Assignee" to workboards. This means you may have several tasks grouped under, say, "Alice". When you drag the bottom-most task under "Alice" to the top, what does that mean? Today, the only grouping is "Priority", and it means "change the task's secret/hidden global subpriority". However, this seems to generally be a somewhat-bad answer, and is quite complex. It also doesn't make much sense for an author grouping, since one task can't really be "more assigned" to Alice than another task. Users likely intend this operation to mean "move it, visually, with no other effects" -- that is, user intent is to shuffle sticky notes around on a board, not edit anything substantive. The meaning is probably something like "this is similar to other nearby tasks" or "maybe this is a good place to start", which we can't really capture with any top-level attribute. We could extend "subpriority" and give tasks a secret/hidden "sub-assignment strength" and so on, but this seems like a bad road to walk down. We'll also run into trouble later when subproject columns may appear on the board, and a user could want to put a task in different positions on different subprojects, conceivably. In the "Natural" order view, we already have what is probably a generally better approach for this: a task display order particular to the column, that just remembers where you put the sticky notes. Move away from "subpriority", and toward a world where we mostly keep sticky notes where you stuck them and move them around only when we have to. With no grouping, we still sort by "natural" order, as before. With priority grouping, we now sort by ``. When you drag stuff around inside a priority group, we update the natural order. This means that moving cards around on a "priority" board will also move them around on a "natural" board, at least somewhat. I think this is okay. If it's not intuitive, we could give every ordering its own separate "natural" view, so we remember where you stuck stuff on the "priority" board but that doesn't affect the "Natural" board. But I suspect we won't need to. Test Plan: - Viewed and dragged a natural board. - Viewed and dragged a priority board. - Dragged within and between groups of 0, 1, and multiple items. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20265 --- resources/celerity/map.php | 38 ++-- .../maniphest/storage/ManiphestTask.php | 3 - .../PhabricatorProjectBoardViewController.php | 8 - .../PhabricatorProjectMoveController.php | 169 +++--------------- .../js/application/projects/WorkboardBoard.js | 44 ++++- .../application/projects/WorkboardColumn.js | 80 ++++++--- 6 files changed, 141 insertions(+), 201 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 58aa364de5..30d6587bac 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -408,9 +408,9 @@ 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' => 'a4f1e85d', + 'rsrc/js/application/projects/WorkboardBoard.js' => '902a1551', 'rsrc/js/application/projects/WorkboardCard.js' => '887ef74f', - 'rsrc/js/application/projects/WorkboardColumn.js' => 'ca444dca', + 'rsrc/js/application/projects/WorkboardColumn.js' => '01ea93b3', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/WorkboardHeader.js' => '6e75daea', 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '2d641f7d', @@ -727,9 +727,9 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'a4f1e85d', + 'javelin-workboard-board' => '902a1551', 'javelin-workboard-card' => '887ef74f', - 'javelin-workboard-column' => 'ca444dca', + 'javelin-workboard-column' => '01ea93b3', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workboard-header' => '6e75daea', 'javelin-workboard-header-template' => '2d641f7d', @@ -889,6 +889,11 @@ return array( 'javelin-uri', 'phabricator-notification', ), + '01ea93b3' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), '022516b4' => array( 'javelin-install', 'javelin-util', @@ -1615,6 +1620,16 @@ return array( 'javelin-workflow', 'javelin-stratcom', ), + '902a1551' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'phabricator-draggable-list', + 'javelin-workboard-column', + 'javelin-workboard-header-template', + ), 91863989 => array( 'javelin-install', 'javelin-stratcom', @@ -1750,16 +1765,6 @@ return array( 'javelin-request', 'javelin-util', ), - 'a4f1e85d' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - 'javelin-workboard-header-template', - ), 'a5257c4e' => array( 'javelin-install', 'javelin-dom', @@ -1957,11 +1962,6 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), - 'ca444dca' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 8d45ed45fa..c19e75604c 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -252,9 +252,6 @@ final class ManiphestTask extends ManiphestDAO return array( PhabricatorProjectColumn::ORDER_PRIORITY => array( (int)-$this->getPriority(), - PhabricatorProjectColumn::NODETYPE_CARD, - (double)-$this->getSubpriority(), - (int)-$this->getID(), ), ); } diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 32374ad8be..6df43addd9 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -522,13 +522,6 @@ final class PhabricatorProjectBoardViewController $column->getPHID()); $column_tasks = array_select_keys($tasks, $task_phids); - - // If we aren't using "natural" order, reorder the column by the original - // query order. - if ($this->sortKey != PhabricatorProjectColumn::ORDER_NATURAL) { - $column_tasks = array_select_keys($column_tasks, array_keys($tasks)); - } - $column_phid = $column->getPHID(); $visible_columns[$column_phid] = $column; @@ -684,7 +677,6 @@ final class PhabricatorProjectBoardViewController ); $this->initBehavior('project-boards', $behavior_config); - $sort_menu = $this->buildSortMenu( $viewer, $project, diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 09fca4692d..7a771ea7e8 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -71,20 +71,14 @@ final class PhabricatorProjectMoveController ->setObjectPHIDs(array($object_phid)) ->executeLayout(); - $columns = $engine->getObjectColumns($board_phid, $object_phid); - $old_column_phids = mpull($columns, 'getPHID'); - - $xactions = array(); - $order_params = array(); - if ($order == PhabricatorProjectColumn::ORDER_NATURAL) { - if ($after_phid) { - $order_params['afterPHID'] = $after_phid; - } else if ($before_phid) { - $order_params['beforePHID'] = $before_phid; - } + if ($after_phid) { + $order_params['afterPHID'] = $after_phid; + } else if ($before_phid) { + $order_params['beforePHID'] = $before_phid; } + $xactions = array(); $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_COLUMNS) ->setNewValue( @@ -94,18 +88,12 @@ final class PhabricatorProjectMoveController ) + $order_params, )); - if ($order == PhabricatorProjectColumn::ORDER_PRIORITY) { - $header_priority = idx( - $edit_header, - PhabricatorProjectColumn::ORDER_PRIORITY); - $priority_xactions = $this->getPriorityTransactions( - $object, - $after_phid, - $before_phid, - $header_priority); - foreach ($priority_xactions as $xaction) { - $xactions[] = $xaction; - } + $header_xactions = $this->newHeaderTransactions( + $object, + $order, + $edit_header); + foreach ($header_xactions as $header_xaction) { + $xactions[] = $header_xaction; } $editor = id(new ManiphestTransactionEditor()) @@ -119,137 +107,30 @@ final class PhabricatorProjectMoveController return $this->newCardResponse($board_phid, $object_phid); } - private function getPriorityTransactions( + private function newHeaderTransactions( ManiphestTask $task, - $after_phid, - $before_phid, - $header_priority) { + $order, + array $header) { $xactions = array(); - $must_move = false; - if ($header_priority !== null) { - if ($task->getPriority() !== $header_priority) { - $task = id(clone $task) - ->setPriority($header_priority); + switch ($order) { + case PhabricatorProjectColumn::ORDER_PRIORITY: + $new_priority = idx($header, $order); - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $header_priority)); + if ($task->getPriority() !== $new_priority) { + $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); + $keyword = head(idx($keyword_map, $new_priority)); - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType( - ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $must_move = true; - } - } - - list($after_task, $before_task) = $this->loadPriorityTasks( - $after_phid, - $before_phid); - - if ($after_task && !$task->isLowerPriorityThan($after_task)) { - $must_move = true; - } - - if ($before_task && !$task->isHigherPriorityThan($before_task)) { - $must_move = true; - } - - // The move doesn't require a subpriority change to be valid, so don't - // change the subpriority since we are not being forced to. - if (!$must_move) { - return $xactions; - } - - $try = array( - array($after_task, true), - array($before_task, false), - ); - - $pri = null; - $sub = null; - foreach ($try as $spec) { - list($nearby_task, $is_after) = $spec; - - if (!$nearby_task) { - continue; - } - - list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $nearby_task, - $is_after); - - // If we drag under a "Low" header between a "Normal" task and a "Low" - // task, we don't want to accept a subpriority assignment which changes - // our priority to "Normal". Only accept a subpriority that keeps us in - // the right primary priority. - if ($header_priority !== null) { - if ($pri !== $header_priority) { - continue; + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType( + ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) + ->setNewValue($keyword); } - } - - // If we find a priority on the first try, don't keep going. - break; - } - - if ($pri !== null) { - if ($header_priority === null) { - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $pri)); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType( - ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - } - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType( - ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); + break; } return $xactions; } - private function loadPriorityTasks($after_phid, $before_phid) { - $viewer = $this->getViewer(); - - $task_phids = array(); - - if ($after_phid) { - $task_phids[] = $after_phid; - } - if ($before_phid) { - $task_phids[] = $before_phid; - } - - if (!$task_phids) { - return array(null, null); - } - - $tasks = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->withPHIDs($task_phids) - ->execute(); - $tasks = mpull($tasks, null, 'getPHID'); - - if ($after_phid) { - $after_task = idx($tasks, $after_phid); - } else { - $after_task = null; - } - - if ($before_phid) { - $before_task = idx($tasks, $before_phid); - } else { - $before_task = null; - } - - return array($after_task, $before_task); - } - } diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index b0bcfe97c6..c49e4c859f 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -202,6 +202,14 @@ JX.install('WorkboardBoard', { order: this.getOrder() }; + // We're going to send an "afterPHID" and a "beforePHID" if the card + // was dropped immediately adjacent to another card. If a card was + // dropped before or after a header, we don't send a PHID for the card + // on the other side of the header. + + // If the view has headers, we always send the header the card was + // dropped under. + var after_data; var after_card = after_node; while (after_card) { @@ -209,11 +217,16 @@ JX.install('WorkboardBoard', { if (after_data.objectPHID) { break; } + if (after_data.headerKey) { + break; + } after_card = after_card.previousSibling; } if (after_data) { - data.afterPHID = after_data.objectPHID; + if (after_data.objectPHID) { + data.afterPHID = after_data.objectPHID; + } } var before_data; @@ -223,18 +236,35 @@ JX.install('WorkboardBoard', { if (before_data.objectPHID) { break; } + if (before_data.headerKey) { + break; + } before_card = before_card.nextSibling; } if (before_data) { - data.beforePHID = before_data.objectPHID; + if (before_data.objectPHID) { + data.beforePHID = before_data.objectPHID; + } } - var header_key = JX.Stratcom.getData(after_node).headerKey; - if (header_key) { - var properties = this.getHeaderTemplate(header_key) - .getEditProperties(); - data.header = JX.JSON.stringify(properties); + var header_data; + var header_node = after_node; + while (header_node) { + header_data = JX.Stratcom.getData(header_node); + if (header_data.headerKey) { + break; + } + header_node = header_node.previousSibling; + } + + if (header_data) { + var header_key = header_data.headerKey; + if (header_key) { + var properties = this.getHeaderTemplate(header_key) + .getEditProperties(); + data.header = JX.JSON.stringify(properties); + } } var visible_phids = []; diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index 262e80c2da..e2ec18ae75 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -34,6 +34,7 @@ JX.install('WorkboardColumn', { _cards: null, _headers: null, _naturalOrder: null, + _orderVectors: null, _panel: null, _pointsNode: null, _pointsContentNode: null, @@ -66,6 +67,7 @@ JX.install('WorkboardColumn', { setNaturalOrder: function(order) { this._naturalOrder = order; + this._orderVectors = null; return this; }, @@ -86,6 +88,7 @@ JX.install('WorkboardColumn', { this._cards[phid] = card; this._naturalOrder.push(phid); + this._orderVectors = null; return card; }, @@ -97,6 +100,7 @@ JX.install('WorkboardColumn', { for (var ii = 0; ii < this._naturalOrder.length; ii++) { if (this._naturalOrder[ii] == phid) { this._naturalOrder.splice(ii, 1); + this._orderVectors = null; break; } } @@ -127,6 +131,8 @@ JX.install('WorkboardColumn', { this._naturalOrder.splice(index, 0, phid); } + this._orderVectors = null; + return this; }, @@ -204,12 +210,7 @@ JX.install('WorkboardColumn', { var board = this.getBoard(); var order = board.getOrder(); - var list; - if (order == 'natural') { - list = this._getCardsSortedNaturally(); - } else { - list = this._getCardsSortedByKey(order); - } + var list = this._getCardsSortedByKey(order); var ii; var objects = []; @@ -285,7 +286,7 @@ JX.install('WorkboardColumn', { var data = JX.Stratcom.getData(node); if (data.objectPHID) { - return board.getOrderVector(data.objectPHID, order); + return this._getOrderVector(data.objectPHID, order); } return board.getHeaderTemplate(data.headerKey).getVector(); @@ -296,17 +297,6 @@ JX.install('WorkboardColumn', { JX.DOM.alterClass(node, 'workboard-column-drop-target', is_target); }, - _getCardsSortedNaturally: function() { - var list = []; - - for (var ii = 0; ii < this._naturalOrder.length; ii++) { - var phid = this._naturalOrder[ii]; - list.push(this.getCard(phid)); - } - - return list; - }, - _getCardsSortedByKey: function(order) { var cards = this.getCards(); @@ -322,12 +312,62 @@ JX.install('WorkboardColumn', { _sortCards: function(order, u, v) { var board = this.getBoard(); - var u_vec = board.getOrderVector(u.getPHID(), order); - var v_vec = board.getOrderVector(v.getPHID(), order); + var u_vec = this._getOrderVector(u.getPHID(), order); + var v_vec = this._getOrderVector(v.getPHID(), order); return board.compareVectors(u_vec, v_vec); }, + _getOrderVector: function(phid, order) { + if (!this._orderVectors) { + this._orderVectors = {}; + } + + if (!this._orderVectors[order]) { + var board = this.getBoard(); + var cards = this.getCards(); + var vectors = {}; + + for (var k in cards) { + var card_phid = cards[k].getPHID(); + var vector = board.getOrderVector(card_phid, order); + vectors[card_phid] = [].concat(vector); + + // Push a "card" type, so cards always sort after headers; headers + // have a "0" in this position. + vectors[card_phid].push(1); + } + + for (var ii = 0; ii < this._naturalOrder.length; ii++) { + var natural_phid = this._naturalOrder[ii]; + if (vectors[natural_phid]) { + vectors[natural_phid].push(ii); + } + } + + this._orderVectors[order] = vectors; + } + + if (!this._orderVectors[order][phid]) { + // In this case, we're comparing a card being dragged in from another + // column to the cards already in this column. We're just going to + // build a temporary vector for it. + var incoming_vector = this.getBoard().getOrderVector(phid, order); + incoming_vector = [].concat(incoming_vector); + + // Add a "card" type to sort this after headers. + incoming_vector.push(1); + + // Add a "0" for the natural ordering to put this on top. A new card + // has no natural ordering on a column it isn't part of yet. + incoming_vector.push(0); + + return incoming_vector; + } + + return this._orderVectors[order][phid]; + }, + _redrawFrame: function() { var cards = this.getCards(); var board = this.getBoard(); From 4bad6bc42af9022a648964f1c80718dd8e4fb27e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 08:44:15 -0700 Subject: [PATCH 09/25] Remove all readers/writers for task "subpriority" Summary: Depends on D20265. Ref T10333. Now that neither task lists nor workboards use subpriority, we can remove all the readers and writers. I'm not actually getting rid of the column data yet, but anticipate doing that in a future change. Note that the subpriority algorithm (removed here) is possibly better than the "natural order" algorithm still in use. It's a bit more clever, and likely performs far fewer writes. I might make the "natural order" code use an algorithm more similar to the "subpriority" algorithm in the future. Test Plan: Grepped for `subpriority`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20266 --- src/__phutil_library_map__.php | 2 - .../__tests__/ManiphestTaskTestCase.php | 255 ------------------ .../editor/ManiphestTransactionEditor.php | 245 ----------------- ...bricatorManiphestTaskTestDataGenerator.php | 5 - .../maniphest/query/ManiphestTaskQuery.php | 15 +- .../maniphest/storage/ManiphestTask.php | 34 --- .../ManiphestTaskSubpriorityTransaction.php | 7 +- 7 files changed, 5 insertions(+), 558 deletions(-) delete mode 100644 src/applications/maniphest/__tests__/ManiphestTaskTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3cab91717a..713d3e5f29 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1782,7 +1782,6 @@ phutil_register_library_map(array( 'ManiphestTaskSubpriorityTransaction' => 'applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php', 'ManiphestTaskSubtaskController' => 'applications/maniphest/controller/ManiphestTaskSubtaskController.php', 'ManiphestTaskSubtypeDatasource' => 'applications/maniphest/typeahead/ManiphestTaskSubtypeDatasource.php', - 'ManiphestTaskTestCase' => 'applications/maniphest/__tests__/ManiphestTaskTestCase.php', 'ManiphestTaskTitleHeraldField' => 'applications/maniphest/herald/ManiphestTaskTitleHeraldField.php', 'ManiphestTaskTitleTransaction' => 'applications/maniphest/xaction/ManiphestTaskTitleTransaction.php', 'ManiphestTaskTransactionType' => 'applications/maniphest/xaction/ManiphestTaskTransactionType.php', @@ -7501,7 +7500,6 @@ phutil_register_library_map(array( 'ManiphestTaskSubpriorityTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskSubtaskController' => 'ManiphestController', 'ManiphestTaskSubtypeDatasource' => 'PhabricatorTypeaheadDatasource', - 'ManiphestTaskTestCase' => 'PhabricatorTestCase', 'ManiphestTaskTitleHeraldField' => 'ManiphestTaskHeraldField', 'ManiphestTaskTitleTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskTransactionType' => 'PhabricatorModularTransactionType', diff --git a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php b/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php deleted file mode 100644 index 58190f6a89..0000000000 --- a/src/applications/maniphest/__tests__/ManiphestTaskTestCase.php +++ /dev/null @@ -1,255 +0,0 @@ - true, - ); - } - - public function testTaskReordering() { - $viewer = $this->generateNewTestUser(); - - $t1 = $this->newTask($viewer, pht('Task 1')); - $t2 = $this->newTask($viewer, pht('Task 2')); - $t3 = $this->newTask($viewer, pht('Task 3')); - - $auto_base = min(mpull(array($t1, $t2, $t3), 'getID')); - - - // Default order should be reverse creation. - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 to the middle. - $this->moveTask($viewer, $t3, $t2, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 3, 1), array_keys($tasks)); - - - // Move T3 to the end. - $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Repeat the move above, there should be no overall change in order. - $this->moveTask($viewer, $t3, $t1, true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Move T3 to the first slot in the priority. - $this->movePriority($viewer, $t3, $t3->getPriority(), false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 to the last slot in the priority. - $this->movePriority($viewer, $t3, $t3->getPriority(), true); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 1, 3), array_keys($tasks)); - - - // Move T3 before T2. - $this->moveTask($viewer, $t3, $t2, false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(3, 2, 1), array_keys($tasks)); - - - // Move T3 before T1. - $this->moveTask($viewer, $t3, $t1, false); - $tasks = $this->loadTasks($viewer, $auto_base); - $t1 = $tasks[1]; - $t2 = $tasks[2]; - $t3 = $tasks[3]; - $this->assertEqual(array(2, 3, 1), array_keys($tasks)); - - } - - public function testTaskAdjacentBlocks() { - $viewer = $this->generateNewTestUser(); - - $t = array(); - for ($ii = 1; $ii < 10; $ii++) { - $t[$ii] = $this->newTask($viewer, pht('Task Block %d', $ii)); - - // This makes sure this test remains meaningful if we begin assigning - // subpriorities when tasks are created. - $t[$ii]->setSubpriority(0)->save(); - } - - $auto_base = min(mpull($t, 'getID')); - - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(9, 8, 7, 6, 5, 4, 3, 2, 1), - array_keys($tasks)); - - $this->moveTask($viewer, $t[9], $t[8], true); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 9, 7, 6, 5, 4, 3, 2, 1), - array_keys($tasks)); - - // When there is a large block of tasks which all have the same - // subpriority, they should be assigned distinct subpriorities as a - // side effect of having a task moved into the block. - - $subpri = mpull($tasks, 'getSubpriority'); - $unique_subpri = array_unique($subpri); - $this->assertEqual( - 9, - count($subpri), - pht('Expected subpriorities to be distributed.')); - - // Move task 9 to the end. - $this->moveTask($viewer, $t[9], $t[1], true); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 3, 2, 1, 9), - array_keys($tasks)); - - // Move task 3 to the beginning. - $this->moveTask($viewer, $t[3], $t[8], false); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(3, 8, 7, 6, 5, 4, 2, 1, 9), - array_keys($tasks)); - - // Move task 3 to the end. - $this->moveTask($viewer, $t[3], $t[9], true); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 2, 1, 9, 3), - array_keys($tasks)); - - // Move task 5 to before task 4 (this is its current position). - $this->moveTask($viewer, $t[5], $t[4], false); - $tasks = $this->loadTasks($viewer, $auto_base); - $this->assertEqual( - array(8, 7, 6, 5, 4, 2, 1, 9, 3), - array_keys($tasks)); - } - - private function newTask(PhabricatorUser $viewer, $title) { - $task = ManiphestTask::initializeNewTask($viewer); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskTitleTransaction::TRANSACTIONTYPE) - ->setNewValue($title); - - - $this->applyTaskTransactions($viewer, $task, $xactions); - - return $task; - } - - private function loadTasks(PhabricatorUser $viewer, $auto_base) { - $tasks = id(new ManiphestTaskQuery()) - ->setViewer($viewer) - ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) - ->execute(); - - // NOTE: AUTO_INCREMENT changes survive ROLLBACK, and we can't throw them - // away without committing the current transaction, so we adjust the - // apparent task IDs as though the first one had been ID 1. This makes the - // tests a little easier to understand. - - $map = array(); - foreach ($tasks as $task) { - $map[($task->getID() - $auto_base) + 1] = $task; - } - - return $map; - } - - private function moveTask(PhabricatorUser $viewer, $src, $dst, $is_after) { - list($pri, $sub) = ManiphestTransactionEditor::getAdjacentSubpriority( - $dst, - $is_after); - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head($keyword_map[$pri]); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - return $this->applyTaskTransactions($viewer, $src, $xactions); - } - - private function movePriority( - PhabricatorUser $viewer, - $src, - $target_priority, - $is_end) { - - list($pri, $sub) = ManiphestTransactionEditor::getEdgeSubpriority( - $target_priority, - $is_end); - - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head($keyword_map[$pri]); - - $xactions = array(); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType(ManiphestTaskSubpriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($sub); - - return $this->applyTaskTransactions($viewer, $src, $xactions); - } - - private function applyTaskTransactions( - PhabricatorUser $viewer, - ManiphestTask $task, - array $xactions) { - - $content_source = $this->newContentSource(); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($viewer) - ->setContentSource($content_source) - ->setContinueOnNoEffect(true) - ->applyTransactions($task, $xactions); - - return $task; - } - -} diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 1748e5e84e..9a95bbdbb8 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -297,251 +297,6 @@ final class ManiphestTransactionEditor return $copy; } - /** - * Get priorities for moving a task to a new priority. - */ - public static function getEdgeSubpriority( - $priority, - $is_end) { - - $query = id(new ManiphestTaskQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPriorities(array($priority)) - ->setLimit(1); - - if ($is_end) { - $query->setOrderVector(array('-priority', '-subpriority', '-id')); - } else { - $query->setOrderVector(array('priority', 'subpriority', 'id')); - } - - $result = $query->executeOne(); - $step = (double)(2 << 32); - - if ($result) { - $base = $result->getSubpriority(); - if ($is_end) { - $sub = ($base - $step); - } else { - $sub = ($base + $step); - } - } else { - $sub = 0; - } - - return array($priority, $sub); - } - - - /** - * Get priorities for moving a task before or after another task. - */ - public static function getAdjacentSubpriority( - ManiphestTask $dst, - $is_after) { - - $query = id(new ManiphestTaskQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->setOrder(ManiphestTaskQuery::ORDER_PRIORITY) - ->withPriorities(array($dst->getPriority())) - ->setLimit(1); - - if ($is_after) { - $query->setAfterID($dst->getID()); - } else { - $query->setBeforeID($dst->getID()); - } - - $adjacent = $query->executeOne(); - - $base = $dst->getSubpriority(); - $step = (double)(2 << 32); - - // If we find an adjacent task, we average the two subpriorities and - // return the result. - if ($adjacent) { - $epsilon = 1.0; - - // If the adjacent task has a subpriority that is identical or very - // close to the task we're looking at, we're going to spread out all - // the nearby tasks. - - $adjacent_sub = $adjacent->getSubpriority(); - if ((abs($adjacent_sub - $base) < $epsilon)) { - $base = self::disperseBlock( - $dst, - $epsilon * 2); - if ($is_after) { - $sub = $base - $epsilon; - } else { - $sub = $base + $epsilon; - } - } else { - $sub = ($adjacent_sub + $base) / 2; - } - } else { - // Otherwise, we take a step away from the target's subpriority and - // use that. - if ($is_after) { - $sub = ($base - $step); - } else { - $sub = ($base + $step); - } - } - - return array($dst->getPriority(), $sub); - } - - /** - * Distribute a cluster of tasks with similar subpriorities. - */ - private static function disperseBlock( - ManiphestTask $task, - $spacing) { - - $conn = $task->establishConnection('w'); - - // Find a block of subpriority space which is, on average, sparse enough - // to hold all the tasks that are inside it with a reasonable level of - // separation between them. - - // We'll start by looking near the target task for a range of numbers - // which has more space available than tasks. For example, if the target - // task has subpriority 33 and we want to separate each task by at least 1, - // we might start by looking in the range [23, 43]. - - // If we find fewer than 20 tasks there, we have room to reassign them - // with the desired level of separation. We space them out, then we're - // done. - - // However: if we find more than 20 tasks, we don't have enough room to - // distribute them. We'll widen our search and look in a bigger range, - // maybe [13, 53]. This range has more space, so if we find fewer than - // 40 tasks in this range we can spread them out. If we still find too - // many tasks, we keep widening the search. - - $base = $task->getSubpriority(); - - $scale = 4.0; - while (true) { - $range = ($spacing * $scale) / 2.0; - $min = ($base - $range); - $max = ($base + $range); - - $result = queryfx_one( - $conn, - 'SELECT COUNT(*) N FROM %T WHERE priority = %d AND - subpriority BETWEEN %f AND %f', - $task->getTableName(), - $task->getPriority(), - $min, - $max); - - $count = $result['N']; - if ($count < $scale) { - // We have found a block which we can make sparse enough, so bail and - // continue below with our selection. - break; - } - - // This block had too many tasks for its size, so try again with a - // bigger block. - $scale *= 2.0; - } - - $rows = queryfx_all( - $conn, - 'SELECT id FROM %T WHERE priority = %d AND - subpriority BETWEEN %f AND %f - ORDER BY priority, subpriority, id', - $task->getTableName(), - $task->getPriority(), - $min, - $max); - - $task_id = $task->getID(); - $result = null; - - // NOTE: In strict mode (which we encourage enabling) we can't structure - // this bulk update as an "INSERT ... ON DUPLICATE KEY UPDATE" unless we - // provide default values for ALL of the columns that don't have defaults. - - // This is gross, but we may be moving enough rows that individual - // queries are unreasonably slow. An alternate construction which might - // be worth evaluating is to use "CASE". Another approach is to disable - // strict mode for this query. - - $default_str = qsprintf($conn, '%s', ''); - $default_int = qsprintf($conn, '%d', 0); - - $extra_columns = array( - 'phid' => $default_str, - 'authorPHID' => $default_str, - 'status' => $default_str, - 'priority' => $default_int, - 'title' => $default_str, - 'description' => $default_str, - 'dateCreated' => $default_int, - 'dateModified' => $default_int, - 'mailKey' => $default_str, - 'viewPolicy' => $default_str, - 'editPolicy' => $default_str, - 'ownerOrdering' => $default_str, - 'spacePHID' => $default_str, - 'bridgedObjectPHID' => $default_str, - 'properties' => $default_str, - 'points' => $default_int, - 'subtype' => $default_str, - ); - - $sql = array(); - $offset = 0; - - // Often, we'll have more room than we need in the range. Distribute the - // tasks evenly over the whole range so that we're less likely to end up - // with tasks spaced exactly the minimum distance apart, which may - // get shifted again later. We have one fewer space to distribute than we - // have tasks. - $divisor = (double)(count($rows) - 1.0); - if ($divisor > 0) { - $available_distance = (($max - $min) / $divisor); - } else { - $available_distance = 0.0; - } - - foreach ($rows as $row) { - $subpriority = $min + ($offset * $available_distance); - - // If this is the task that we're spreading out relative to, keep track - // of where it is ending up so we can return the new subpriority. - $id = $row['id']; - if ($id == $task_id) { - $result = $subpriority; - } - - $sql[] = qsprintf( - $conn, - '(%d, %LQ, %f)', - $id, - $extra_columns, - $subpriority); - - $offset++; - } - - foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { - queryfx( - $conn, - 'INSERT INTO %T (id, %LC, subpriority) VALUES %LQ - ON DUPLICATE KEY UPDATE subpriority = VALUES(subpriority)', - $task->getTableName(), - array_keys($extra_columns), - $chunk); - } - - return $result; - } - protected function validateAllTransactions( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php index 3fc1957b4f..ef0644ddd2 100644 --- a/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php +++ b/src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php @@ -14,7 +14,6 @@ final class PhabricatorManiphestTaskTestDataGenerator $author = id(new PhabricatorUser()) ->loadOneWhere('phid = %s', $author_phid); $task = ManiphestTask::initializeNewTask($author) - ->setSubPriority($this->generateTaskSubPriority()) ->setTitle($this->generateTitle()); $content_source = $this->getLipsumContentSource(); @@ -106,10 +105,6 @@ final class PhabricatorManiphestTaskTestDataGenerator return $keyword; } - public function generateTaskSubPriority() { - return rand(2 << 16, 2 << 32); - } - public function generateTaskStatus() { $statuses = array_keys(ManiphestTaskStatus::getTaskStatusMap()); // Make sure 4/5th of all generated Tasks are open diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 9fb4ecb68c..1033bfe333 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -435,13 +435,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $this->priorities); } - if ($this->subpriorities !== null) { - $where[] = qsprintf( - $conn, - 'task.subpriority IN (%Lf)', - $this->subpriorities); - } - if ($this->bridgedObjectPHIDs !== null) { $where[] = qsprintf( $conn, @@ -844,7 +837,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { public function getBuiltinOrders() { $orders = array( 'priority' => array( - 'vector' => array('priority', 'subpriority', 'id'), + 'vector' => array('priority', 'id'), 'name' => pht('Priority'), 'aliases' => array(self::ORDER_PRIORITY), ), @@ -919,11 +912,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { 'type' => 'string', 'reverse' => true, ), - 'subpriority' => array( - 'table' => 'task', - 'column' => 'subpriority', - 'type' => 'float', - ), 'updated' => array( 'table' => 'task', 'column' => 'dateModified', @@ -948,7 +936,6 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $map = array( 'id' => $task->getID(), 'priority' => $task->getPriority(), - 'subpriority' => $task->getSubpriority(), 'owner' => $task->getOwnerOrdering(), 'status' => $task->getStatus(), 'title' => $task->getTitle(), diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index c19e75604c..d6e3ec6d6c 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -267,39 +267,6 @@ final class ManiphestTask extends ManiphestDAO return ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD; } - private function comparePriorityTo(ManiphestTask $other) { - $upri = $this->getPriority(); - $vpri = $other->getPriority(); - - if ($upri != $vpri) { - return ($upri - $vpri); - } - - $usub = $this->getSubpriority(); - $vsub = $other->getSubpriority(); - - if ($usub != $vsub) { - return ($usub - $vsub); - } - - $uid = $this->getID(); - $vid = $other->getID(); - - if ($uid != $vid) { - return ($uid - $vid); - } - - return 0; - } - - public function isLowerPriorityThan(ManiphestTask $other) { - return ($this->comparePriorityTo($other) < 0); - } - - public function isHigherPriorityThan(ManiphestTask $other) { - return ($this->comparePriorityTo($other) > 0); - } - public function getWorkboardProperties() { return array( 'status' => $this->getStatus(), @@ -540,7 +507,6 @@ final class ManiphestTask extends ManiphestDAO $priority_value = (int)$this->getPriority(); $priority_info = array( 'value' => $priority_value, - 'subpriority' => (double)$this->getSubpriority(), 'name' => ManiphestTaskPriority::getTaskPriorityName($priority_value), 'color' => ManiphestTaskPriority::getTaskPriorityColor($priority_value), ); diff --git a/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php index 49d227b7f1..c88ee8aa0c 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskSubpriorityTransaction.php @@ -6,16 +6,17 @@ final class ManiphestTaskSubpriorityTransaction const TRANSACTIONTYPE = 'subpriority'; public function generateOldValue($object) { - return $object->getSubpriority(); + return null; } public function applyInternalEffects($object, $value) { - $object->setSubpriority($value); + // This transaction is obsolete, but we're keeping the class around so it + // is hidden from timelines until we destroy the actual transaction data. + throw new PhutilMethodNotImplementedException(); } public function shouldHide() { return true; } - } From 7d849afd16c5e76081f6a8da2a52db374fe4c1c2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 19:53:25 -0700 Subject: [PATCH 10/25] Add a "WorkboardCardTemplate" class to make workboard client code easier to reason about Summary: Depends on D20266. Boards currently have several `whateverMap stuff>` properties, but we can just move these all down into a `CardTemplate`, similar to the recently introduced `HeaderTemplate`. The `CardTemplate` holds all the global information for a card, and then `Card` is specific for a particular copy in a column. Today, each `CardTemplate` has one `Card`, but a `CardTemplate` may have more than one card in the future (when we add subproject columns). Test Plan: Viewed workboards in different sort orders and dragged stuff around, grepped for all affected symbols. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20267 --- resources/celerity/map.php | 76 ++++++++++--------- .../js/application/projects/WorkboardBoard.js | 70 +++++++---------- .../js/application/projects/WorkboardCard.js | 14 +++- .../projects/WorkboardCardTemplate.js | 47 ++++++++++++ .../application/projects/WorkboardColumn.js | 10 ++- .../projects/behavior-project-boards.js | 11 ++- 6 files changed, 140 insertions(+), 88 deletions(-) create mode 100644 webroot/rsrc/js/application/projects/WorkboardCardTemplate.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 30d6587bac..825a5cebc9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -408,13 +408,14 @@ 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' => '902a1551', - 'rsrc/js/application/projects/WorkboardCard.js' => '887ef74f', - 'rsrc/js/application/projects/WorkboardColumn.js' => '01ea93b3', + 'rsrc/js/application/projects/WorkboardBoard.js' => '2181739b', + 'rsrc/js/application/projects/WorkboardCard.js' => 'bc92741f', + 'rsrc/js/application/projects/WorkboardCardTemplate.js' => 'b0b5f90a', + 'rsrc/js/application/projects/WorkboardColumn.js' => '6461f58b', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/WorkboardHeader.js' => '6e75daea', 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '2d641f7d', - 'rsrc/js/application/projects/behavior-project-boards.js' => 'e2730b90', + 'rsrc/js/application/projects/behavior-project-boards.js' => 'cca3f5f8', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', 'rsrc/js/application/projects/behavior-reorder-columns.js' => '8ac32fd9', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', @@ -655,7 +656,7 @@ return array( 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', - 'javelin-behavior-project-boards' => 'e2730b90', + 'javelin-behavior-project-boards' => 'cca3f5f8', 'javelin-behavior-project-create' => '34c53422', 'javelin-behavior-quicksand-blacklist' => '5a6f6a06', 'javelin-behavior-read-only-warning' => 'b9109f8f', @@ -727,9 +728,10 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => '902a1551', - 'javelin-workboard-card' => '887ef74f', - 'javelin-workboard-column' => '01ea93b3', + 'javelin-workboard-board' => '2181739b', + 'javelin-workboard-card' => 'bc92741f', + 'javelin-workboard-card-template' => 'b0b5f90a', + 'javelin-workboard-column' => '6461f58b', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workboard-header' => '6e75daea', 'javelin-workboard-header-template' => '2d641f7d', @@ -889,11 +891,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - '01ea93b3' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), '022516b4' => array( 'javelin-install', 'javelin-util', @@ -1051,6 +1048,17 @@ return array( 'javelin-behavior', 'javelin-request', ), + '2181739b' => 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', + ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1419,6 +1427,11 @@ return array( '60cd9241' => array( 'javelin-behavior', ), + '6461f58b' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), '65bb0011' => array( 'javelin-behavior', 'javelin-dom', @@ -1568,9 +1581,6 @@ return array( 'javelin-install', 'javelin-dom', ), - '887ef74f' => array( - 'javelin-install', - ), '89a1ae3a' => array( 'javelin-dom', 'javelin-util', @@ -1620,16 +1630,6 @@ return array( 'javelin-workflow', 'javelin-stratcom', ), - '902a1551' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'phabricator-draggable-list', - 'javelin-workboard-column', - 'javelin-workboard-header-template', - ), 91863989 => array( 'javelin-install', 'javelin-stratcom', @@ -1839,6 +1839,9 @@ return array( 'javelin-behavior-device', 'javelin-vector', ), + 'b0b5f90a' => array( + 'javelin-install', + ), 'b105a3a6' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1893,6 +1896,9 @@ return array( 'bc16cf33' => array( 'phui-workcard-view-css', ), + 'bc92741f' => array( + 'javelin-install', + ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1962,6 +1968,15 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + 'cca3f5f8' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-workboard-controller', + ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', @@ -2022,15 +2037,6 @@ return array( 'javelin-dom', 'javelin-history', ), - 'e2730b90' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-workboard-controller', - ), 'e562708c' => array( 'javelin-install', ), diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index c49e4c859f..a746461325 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -8,6 +8,7 @@ * phabricator-draggable-list * javelin-workboard-column * javelin-workboard-header-template + * javelin-workboard-card-template * @javelin */ @@ -18,10 +19,9 @@ JX.install('WorkboardBoard', { this._phid = phid; this._root = root; - this._templates = {}; - this._orderMaps = {}; - this._propertiesMap = {}; this._headers = {}; + this._cards = {}; + this._buildColumns(); }, @@ -35,10 +35,8 @@ JX.install('WorkboardBoard', { _phid: null, _root: null, _columns: null, - _templates: null, - _orderMaps: null, - _propertiesMap: null, _headers: null, + _cards: null, getRoot: function() { return this._root; @@ -56,9 +54,12 @@ JX.install('WorkboardBoard', { return this._phid; }, - setCardTemplate: function(phid, template) { - this._templates[phid] = template; - return this; + getCardTemplate: function(phid) { + if (!this._cards[phid]) { + this._cards[phid] = new JX.WorkboardCardTemplate(phid); + } + + return this._cards[phid]; }, getHeaderTemplate: function(header_key) { @@ -91,32 +92,10 @@ JX.install('WorkboardBoard', { return this.compareVectors(u.getVector(), v.getVector()); }, - setObjectProperties: function(phid, properties) { - this._propertiesMap[phid] = properties; - return this; - }, - - getObjectProperties: function(phid) { - return this._propertiesMap[phid]; - }, - - getCardTemplate: function(phid) { - return this._templates[phid]; - }, - getController: function() { return this._controller; }, - setOrderMap: function(phid, map) { - this._orderMaps[phid] = map; - return this; - }, - - getOrderVector: function(phid, key) { - return this._orderMaps[phid][key]; - }, - compareVectors: function(u_vec, v_vec) { for (var ii = 0; ii < u_vec.length; ii++) { if (u_vec[ii] > v_vec[ii]) { @@ -310,25 +289,29 @@ JX.install('WorkboardBoard', { var columns = this.getColumns(); var phid = response.objectPHID; + var card = this.getCardTemplate(phid); - if (!this._templates[phid]) { - for (var add_phid in response.columnMaps) { - var target_column = this.getColumn(add_phid); + for (var add_phid in response.columnMaps) { + var target_column = this.getColumn(add_phid); - if (!target_column) { - // If the column isn't visible, don't try to add a card to it. - continue; - } - - target_column.newCard(phid); + if (!target_column) { + // If the column isn't visible, don't try to add a card to it. + continue; } + + target_column.newCard(phid); } - this.setCardTemplate(phid, response.cardHTML); + card.setNodeHTMLTemplate(response.cardHTML); var order_maps = response.orderMaps; for (var order_phid in order_maps) { - this.setOrderMap(order_phid, order_maps[order_phid]); + var card_template = this.getCardTemplate(order_phid); + for (var order_key in order_maps[order_phid]) { + card_template.setSortVector( + order_key, + order_maps[order_phid][order_key]); + } } var column_maps = response.columnMaps; @@ -348,7 +331,8 @@ JX.install('WorkboardBoard', { var property_maps = response.propertyMaps; for (var property_phid in property_maps) { - this.setObjectProperties(property_phid, property_maps[property_phid]); + this.getCardTemplate(property_phid) + .setObjectProperties(property_maps[property_phid]); } for (var column_phid in columns) { diff --git a/webroot/rsrc/js/application/projects/WorkboardCard.js b/webroot/rsrc/js/application/projects/WorkboardCard.js index 9da3b7e69f..e0eab6b53b 100644 --- a/webroot/rsrc/js/application/projects/WorkboardCard.js +++ b/webroot/rsrc/js/application/projects/WorkboardCard.js @@ -29,7 +29,8 @@ JX.install('WorkboardCard', { }, getProperties: function() { - return this.getColumn().getBoard().getObjectProperties(this.getPHID()); + return this.getColumn().getBoard().getCardTemplate(this.getPHID()) + .getObjectProperties(); }, getPoints: function() { @@ -47,11 +48,16 @@ JX.install('WorkboardCard', { getNode: function() { if (!this._root) { var phid = this.getPHID(); - var template = this.getColumn().getBoard().getCardTemplate(phid); - this._root = JX.$H(template).getFragment().firstChild; - JX.Stratcom.getData(this._root).objectPHID = this.getPHID(); + var root = this.getColumn().getBoard() + .getCardTemplate(phid) + .newNode(); + + JX.Stratcom.getData(root).objectPHID = phid; + + this._root = root; } + return this._root; }, diff --git a/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js b/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js new file mode 100644 index 0000000000..70080b5364 --- /dev/null +++ b/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js @@ -0,0 +1,47 @@ +/** + * @provides javelin-workboard-card-template + * @requires javelin-install + * @javelin + */ + +JX.install('WorkboardCardTemplate', { + + construct: function(phid) { + this._phid = phid; + this._vectors = {}; + + this.setObjectProperties({}); + }, + + properties: { + objectProperties: null + }, + + members: { + _phid: null, + _vectors: null, + + getPHID: function() { + return this._phid; + }, + + setNodeHTMLTemplate: function(html) { + this._html = html; + return this; + }, + + setSortVector: function(order, vector) { + this._vectors[order] = vector; + return this; + }, + + getSortVector: function(order) { + return this._vectors[order]; + }, + + newNode: function() { + return JX.$H(this._html).getFragment().firstChild; + } + } + +}); diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index e2ec18ae75..1c5d6f1a59 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -319,18 +319,21 @@ JX.install('WorkboardColumn', { }, _getOrderVector: function(phid, order) { + var board = this.getBoard(); + if (!this._orderVectors) { this._orderVectors = {}; } if (!this._orderVectors[order]) { - var board = this.getBoard(); var cards = this.getCards(); var vectors = {}; for (var k in cards) { var card_phid = cards[k].getPHID(); - var vector = board.getOrderVector(card_phid, order); + var vector = board.getCardTemplate(card_phid) + .getSortVector(order); + vectors[card_phid] = [].concat(vector); // Push a "card" type, so cards always sort after headers; headers @@ -352,7 +355,8 @@ JX.install('WorkboardColumn', { // In this case, we're comparing a card being dragged in from another // column to the cards already in this column. We're just going to // build a temporary vector for it. - var incoming_vector = this.getBoard().getOrderVector(phid, order); + var incoming_vector = board.getCardTemplate(phid) + .getSortVector(order); incoming_vector = [].concat(incoming_vector); // Add a "card" type to sort this after headers. diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index 6427e1de4c..da82c02040 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -83,7 +83,8 @@ JX.behavior('project-boards', function(config, statics) { var templates = config.templateMap; for (var k in templates) { - board.setCardTemplate(k, templates[k]); + board.getCardTemplate(k) + .setNodeHTMLTemplate(templates[k]); } var column_maps = config.columnMaps; @@ -97,12 +98,16 @@ JX.behavior('project-boards', function(config, statics) { var order_maps = config.orderMaps; for (var object_phid in order_maps) { - board.setOrderMap(object_phid, order_maps[object_phid]); + var order_card = board.getCardTemplate(object_phid); + for (var order_key in order_maps[object_phid]) { + order_card.setSortVector(order_key, order_maps[object_phid][order_key]); + } } var property_maps = config.propertyMaps; for (var property_phid in property_maps) { - board.setObjectProperties(property_phid, property_maps[property_phid]); + board.getCardTemplate(property_phid) + .setObjectProperties(property_maps[property_phid]); } var headers = config.headers; From 9a8019d4a98bd06868289039496d57080286e03e Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 21:49:12 -0700 Subject: [PATCH 11/25] Modularize workboard column orders Summary: Depends on D20267. Depends on D20268. Ref T10333. Currently, we support "Natural" and "Priority" orders, but a lot of the particulars are pretty hard-coded, including some logic in `ManiphestTask`. Although it's not clear that we'll ever put other types of objects on workboards, it seems generally bad that you need to modify `ManiphestTask` to get a new ordering. Pull the ordering logic out into a `ProjectColumnOrder` hierarchy instead, and let each ordering define the things it needs to work (name, icon, what headers look like, how different objects are sorted, and how to apply an edit when you drop an object under a header). Then move the existing "Natural" and "Priority" orders into this new hierarchy. This has a minor bug where using the "Edit" workflow to change a card's priority on a priority-ordered board doesn't fully refresh card/header order since the response isn't ordering-aware. I'll fix that in an upcoming change. Test Plan: Grouped workboards by "Natural" and "Priority", dragged stuff around within and between columns, grepped for all touched symbols. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20269 --- resources/celerity/map.php | 102 +++++----- src/__phutil_library_map__.php | 8 + .../maniphest/storage/ManiphestTask.php | 16 -- .../PhabricatorProjectBoardViewController.php | 104 ++++------- .../PhabricatorProjectController.php | 17 +- .../PhabricatorProjectMoveController.php | 41 ++-- .../engine/PhabricatorBoardResponseEngine.php | 70 ++++++- .../order/PhabricatorProjectColumnHeader.php | 94 ++++++++++ .../PhabricatorProjectColumnNaturalOrder.php | 12 ++ .../order/PhabricatorProjectColumnOrder.php | 176 ++++++++++++++++++ .../PhabricatorProjectColumnPriorityOrder.php | 91 +++++++++ .../storage/PhabricatorProjectColumn.php | 7 - .../js/application/projects/WorkboardBoard.js | 48 +++-- .../js/application/projects/WorkboardCard.js | 7 +- .../projects/WorkboardCardTemplate.js | 17 ++ .../application/projects/WorkboardColumn.js | 16 +- .../application/projects/WorkboardHeader.js | 12 +- .../projects/WorkboardHeaderTemplate.js | 10 + .../projects/behavior-project-boards.js | 8 +- 19 files changed, 638 insertions(+), 218 deletions(-) create mode 100644 src/applications/project/order/PhabricatorProjectColumnHeader.php create mode 100644 src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php create mode 100644 src/applications/project/order/PhabricatorProjectColumnOrder.php create mode 100644 src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 825a5cebc9..c96d03896c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -408,14 +408,14 @@ 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' => '2181739b', - 'rsrc/js/application/projects/WorkboardCard.js' => 'bc92741f', - 'rsrc/js/application/projects/WorkboardCardTemplate.js' => 'b0b5f90a', - 'rsrc/js/application/projects/WorkboardColumn.js' => '6461f58b', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'fc1664ff', + 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', + 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', + 'rsrc/js/application/projects/WorkboardColumn.js' => '533dd592', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', - 'rsrc/js/application/projects/WorkboardHeader.js' => '6e75daea', - 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => '2d641f7d', - 'rsrc/js/application/projects/behavior-project-boards.js' => 'cca3f5f8', + 'rsrc/js/application/projects/WorkboardHeader.js' => '111bfd2d', + 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => 'b65351bd', + 'rsrc/js/application/projects/behavior-project-boards.js' => '285c337a', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', 'rsrc/js/application/projects/behavior-reorder-columns.js' => '8ac32fd9', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', @@ -656,7 +656,7 @@ return array( 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', - 'javelin-behavior-project-boards' => 'cca3f5f8', + 'javelin-behavior-project-boards' => '285c337a', 'javelin-behavior-project-create' => '34c53422', 'javelin-behavior-quicksand-blacklist' => '5a6f6a06', 'javelin-behavior-read-only-warning' => 'b9109f8f', @@ -728,13 +728,13 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => '2181739b', - 'javelin-workboard-card' => 'bc92741f', - 'javelin-workboard-card-template' => 'b0b5f90a', - 'javelin-workboard-column' => '6461f58b', + 'javelin-workboard-board' => 'fc1664ff', + 'javelin-workboard-card' => '0392a5d8', + 'javelin-workboard-card-template' => '2a61f8d4', + 'javelin-workboard-column' => '533dd592', 'javelin-workboard-controller' => '42c7a5a7', - 'javelin-workboard-header' => '6e75daea', - 'javelin-workboard-header-template' => '2d641f7d', + 'javelin-workboard-header' => '111bfd2d', + 'javelin-workboard-header-template' => 'b65351bd', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', @@ -909,6 +909,9 @@ return array( 'javelin-uri', 'javelin-util', ), + '0392a5d8' => array( + 'javelin-install', + ), '04023d82' => array( 'javelin-install', 'phuix-button-view', @@ -993,6 +996,9 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), + '111bfd2d' => array( + 'javelin-install', + ), '1325b731' => array( 'javelin-behavior', 'javelin-uri', @@ -1048,17 +1054,6 @@ return array( 'javelin-behavior', 'javelin-request', ), - '2181739b' => 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', - ), '225bbb98' => array( 'javelin-install', 'javelin-reactor', @@ -1110,6 +1105,15 @@ return array( 'javelin-json', 'phabricator-prefab', ), + '285c337a' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-workboard-controller', + ), '289bf236' => array( 'javelin-install', 'javelin-util', @@ -1119,6 +1123,9 @@ return array( 'javelin-stratcom', 'javelin-behavior', ), + '2a61f8d4' => array( + 'javelin-install', + ), '2a8b62d9' => array( 'multirow-row-manager', 'javelin-install', @@ -1142,9 +1149,6 @@ return array( 'javelin-dom', 'phabricator-keyboard-shortcut', ), - '2d641f7d' => array( - 'javelin-install', - ), '2e255291' => array( 'javelin-install', 'javelin-util', @@ -1343,6 +1347,11 @@ return array( 'javelin-dom', 'javelin-fx', ), + '533dd592' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), '534f1757' => array( 'phui-oi-list-view-css', ), @@ -1427,11 +1436,6 @@ return array( '60cd9241' => array( 'javelin-behavior', ), - '6461f58b' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), '65bb0011' => array( 'javelin-behavior', 'javelin-dom', @@ -1477,9 +1481,6 @@ return array( 'javelin-install', 'javelin-util', ), - '6e75daea' => array( - 'javelin-install', - ), 70245195 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1839,9 +1840,6 @@ return array( 'javelin-behavior-device', 'javelin-vector', ), - 'b0b5f90a' => array( - 'javelin-install', - ), 'b105a3a6' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1875,6 +1873,9 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + 'b65351bd' => array( + 'javelin-install', + ), 'b7b73831' => array( 'javelin-behavior', 'javelin-dom', @@ -1896,9 +1897,6 @@ return array( 'bc16cf33' => array( 'phui-workcard-view-css', ), - 'bc92741f' => array( - 'javelin-install', - ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1968,15 +1966,6 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), - 'cca3f5f8' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-workboard-controller', - ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', @@ -2134,6 +2123,17 @@ return array( 'phabricator-keyboard-shortcut', 'conpherence-thread-manager', ), + 'fc1664ff' => 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', + ), 'fce5d170' => array( 'javelin-magical-init', 'javelin-util', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 713d3e5f29..bdb1fee462 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4052,10 +4052,14 @@ phutil_register_library_map(array( 'PhabricatorProjectColumn' => 'applications/project/storage/PhabricatorProjectColumn.php', 'PhabricatorProjectColumnDetailController' => 'applications/project/controller/PhabricatorProjectColumnDetailController.php', 'PhabricatorProjectColumnEditController' => 'applications/project/controller/PhabricatorProjectColumnEditController.php', + 'PhabricatorProjectColumnHeader' => 'applications/project/order/PhabricatorProjectColumnHeader.php', 'PhabricatorProjectColumnHideController' => 'applications/project/controller/PhabricatorProjectColumnHideController.php', + 'PhabricatorProjectColumnNaturalOrder' => 'applications/project/order/PhabricatorProjectColumnNaturalOrder.php', + 'PhabricatorProjectColumnOrder' => 'applications/project/order/PhabricatorProjectColumnOrder.php', 'PhabricatorProjectColumnPHIDType' => 'applications/project/phid/PhabricatorProjectColumnPHIDType.php', 'PhabricatorProjectColumnPosition' => 'applications/project/storage/PhabricatorProjectColumnPosition.php', 'PhabricatorProjectColumnPositionQuery' => 'applications/project/query/PhabricatorProjectColumnPositionQuery.php', + 'PhabricatorProjectColumnPriorityOrder' => 'applications/project/order/PhabricatorProjectColumnPriorityOrder.php', 'PhabricatorProjectColumnQuery' => 'applications/project/query/PhabricatorProjectColumnQuery.php', 'PhabricatorProjectColumnSearchEngine' => 'applications/project/query/PhabricatorProjectColumnSearchEngine.php', 'PhabricatorProjectColumnTransaction' => 'applications/project/storage/PhabricatorProjectColumnTransaction.php', @@ -10132,13 +10136,17 @@ phutil_register_library_map(array( ), 'PhabricatorProjectColumnDetailController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnEditController' => 'PhabricatorProjectBoardController', + 'PhabricatorProjectColumnHeader' => 'Phobject', 'PhabricatorProjectColumnHideController' => 'PhabricatorProjectBoardController', + 'PhabricatorProjectColumnNaturalOrder' => 'PhabricatorProjectColumnOrder', + 'PhabricatorProjectColumnOrder' => 'Phobject', 'PhabricatorProjectColumnPHIDType' => 'PhabricatorPHIDType', 'PhabricatorProjectColumnPosition' => array( 'PhabricatorProjectDAO', 'PhabricatorPolicyInterface', ), 'PhabricatorProjectColumnPositionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorProjectColumnPriorityOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectColumnSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectColumnTransaction' => 'PhabricatorApplicationTransaction', diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index d6e3ec6d6c..d2700895ce 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -248,14 +248,6 @@ final class ManiphestTask extends ManiphestDAO return idx($this->properties, 'cover.thumbnailPHID'); } - public function getWorkboardOrderVectors() { - return array( - PhabricatorProjectColumn::ORDER_PRIORITY => array( - (int)-$this->getPriority(), - ), - ); - } - public function getPriorityKeyword() { $priority = $this->getPriority(); @@ -267,14 +259,6 @@ final class ManiphestTask extends ManiphestDAO return ManiphestTaskPriority::UNKNOWN_PRIORITY_KEYWORD; } - public function getWorkboardProperties() { - return array( - 'status' => $this->getStatus(), - 'points' => (double)$this->getPoints(), - 'priority' => $this->getPriority(), - ); - } - /* -( PhabricatorSubscribableInterface )----------------------------------- */ diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 6df43addd9..43599fa369 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -614,49 +614,25 @@ final class PhabricatorProjectBoardViewController $board->addPanel($panel); } - // It's possible for tasks to have an invalid/unknown priority in the - // database. We still want to generate a header for these tasks so we - // don't break the workboard. - $priorities = - ManiphestTaskPriority::getTaskPriorityMap() + - mpull($all_tasks, null, 'getPriority'); - $priorities = array_keys($priorities); + $order_key = $this->sortKey; - $headers = array(); - foreach ($priorities as $priority) { - $header_key = sprintf('priority(%s)', $priority); + $ordering_map = PhabricatorProjectColumnOrder::getAllOrders(); + $ordering = id(clone $ordering_map[$order_key]) + ->setViewer($viewer); - $priority_name = ManiphestTaskPriority::getTaskPriorityName($priority); - $priority_color = ManiphestTaskPriority::getTaskPriorityColor($priority); - $priority_icon = ManiphestTaskPriority::getTaskPriorityIcon($priority); + $headers = $ordering->getHeadersForObjects($all_tasks); + $headers = mpull($headers, 'toDictionary'); - $icon_view = id(new PHUIIconView()) - ->setIcon("{$priority_icon} {$priority_color}"); - - $template = phutil_tag( - 'li', - array( - 'class' => 'workboard-group-header', - ), - array( - $icon_view, - $priority_name, - )); - - $headers[] = array( - 'order' => PhabricatorProjectColumn::ORDER_PRIORITY, - 'key' => $header_key, - 'template' => hsprintf('%s', $template), - 'vector' => array( - (int)-$priority, - PhabricatorProjectColumn::NODETYPE_HEADER, - ), - 'editProperties' => array( - PhabricatorProjectColumn::ORDER_PRIORITY => (int)$priority, - ), - ); + $vectors = $ordering->getSortVectorsForObjects($all_tasks); + $vector_map = array(); + foreach ($vectors as $task_phid => $vector) { + $vector_map[$task_phid][$order_key] = $vector; } + $header_keys = $ordering->getHeaderKeysForObjects($all_tasks); + + $properties = array(); + $behavior_config = array( 'moveURI' => $this->getApplicationURI('move/'.$project->getID().'/'), 'uploadURI' => '/file/dropupload/', @@ -667,10 +643,11 @@ final class PhabricatorProjectBoardViewController 'boardPHID' => $project->getPHID(), 'order' => $this->sortKey, 'headers' => $headers, + 'headerKeys' => $header_keys, 'templateMap' => $templates, 'columnMaps' => $column_maps, - 'orderMaps' => mpull($all_tasks, 'getWorkboardOrderVectors'), - 'propertyMaps' => mpull($all_tasks, 'getWorkboardProperties'), + 'orderMaps' => $vector_map, + 'propertyMaps' => $properties, 'boardID' => $board_id, 'projectPHID' => $project->getPHID(), @@ -680,7 +657,8 @@ final class PhabricatorProjectBoardViewController $sort_menu = $this->buildSortMenu( $viewer, $project, - $this->sortKey); + $this->sortKey, + $ordering_map); $filter_menu = $this->buildFilterMenu( $viewer, @@ -775,7 +753,7 @@ final class PhabricatorProjectBoardViewController return $default_sort; } - return PhabricatorProjectColumn::DEFAULT_ORDER; + return PhabricatorProjectColumnNaturalOrder::ORDERKEY; } private function getDefaultFilter(PhabricatorProject $project) { @@ -789,41 +767,37 @@ final class PhabricatorProjectBoardViewController } private function isValidSort($sort) { - switch ($sort) { - case PhabricatorProjectColumn::ORDER_NATURAL: - case PhabricatorProjectColumn::ORDER_PRIORITY: - return true; - } - - return false; + $map = PhabricatorProjectColumnOrder::getAllOrders(); + return isset($map[$sort]); } private function buildSortMenu( PhabricatorUser $viewer, PhabricatorProject $project, - $sort_key) { - - $sort_icon = id(new PHUIIconView()) - ->setIcon('fa-sort-amount-asc bluegrey'); - - $named = array( - PhabricatorProjectColumn::ORDER_NATURAL => pht('Natural'), - PhabricatorProjectColumn::ORDER_PRIORITY => pht('Sort by Priority'), - ); + $sort_key, + array $ordering_map) { $base_uri = $this->getURIWithState(); $items = array(); - foreach ($named as $key => $name) { - $is_selected = ($key == $sort_key); + foreach ($ordering_map as $key => $ordering) { + // TODO: It would be desirable to build a real "PHUIIconView" here, but + // the pathway for threading that through all the view classes ends up + // being fairly complex, since some callers read the icon out of other + // views. For now, just stick with a string. + $ordering_icon = $ordering->getMenuIconIcon(); + $ordering_name = $ordering->getDisplayName(); + + $is_selected = ($key === $sort_key); if ($is_selected) { - $active_order = $name; + $active_name = $ordering_name; + $active_icon = $ordering_icon; } $item = id(new PhabricatorActionView()) - ->setIcon('fa-sort-amount-asc') + ->setIcon($ordering_icon) ->setSelected($is_selected) - ->setName($name); + ->setName($ordering_name); $uri = $base_uri->alter('order', $key); $item->setHref($uri); @@ -856,8 +830,8 @@ final class PhabricatorProjectBoardViewController } $sort_button = id(new PHUIListItemView()) - ->setName($active_order) - ->setIcon('fa-sort-amount-asc') + ->setName($active_name) + ->setIcon($active_icon) ->setHref('#') ->addSigil('boards-dropdown-menu') ->setMetadata( diff --git a/src/applications/project/controller/PhabricatorProjectController.php b/src/applications/project/controller/PhabricatorProjectController.php index 7c344b0b8e..850dfa2268 100644 --- a/src/applications/project/controller/PhabricatorProjectController.php +++ b/src/applications/project/controller/PhabricatorProjectController.php @@ -149,7 +149,11 @@ abstract class PhabricatorProjectController extends PhabricatorController { return $this; } - protected function newCardResponse($board_phid, $object_phid) { + protected function newCardResponse( + $board_phid, + $object_phid, + PhabricatorProjectColumnOrder $ordering = null) { + $viewer = $this->getViewer(); $request = $this->getRequest(); @@ -158,12 +162,17 @@ abstract class PhabricatorProjectController extends PhabricatorController { $visible_phids = array(); } - return id(new PhabricatorBoardResponseEngine()) + $engine = id(new PhabricatorBoardResponseEngine()) ->setViewer($viewer) ->setBoardPHID($board_phid) ->setObjectPHID($object_phid) - ->setVisiblePHIDs($visible_phids) - ->buildResponse(); + ->setVisiblePHIDs($visible_phids); + + if ($ordering) { + $engine->setOrdering($ordering); + } + + return $engine->buildResponse(); } public function renderHashtags(array $tags) { diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 7a771ea7e8..6d7902a733 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -13,7 +13,15 @@ final class PhabricatorProjectMoveController $object_phid = $request->getStr('objectPHID'); $after_phid = $request->getStr('afterPHID'); $before_phid = $request->getStr('beforePHID'); - $order = $request->getStr('order', PhabricatorProjectColumn::DEFAULT_ORDER); + + $order = $request->getStr('order'); + if (!strlen($order)) { + $order = PhabricatorProjectColumnNaturalOrder::ORDERKEY; + } + + $ordering = PhabricatorProjectColumnOrder::getOrderByKey($order); + $ordering = id(clone $ordering) + ->setViewer($viewer); $edit_header = null; $raw_header = $request->getStr('header'); @@ -88,9 +96,8 @@ final class PhabricatorProjectMoveController ) + $order_params, )); - $header_xactions = $this->newHeaderTransactions( + $header_xactions = $ordering->getColumnTransactions( $object, - $order, $edit_header); foreach ($header_xactions as $header_xaction) { $xactions[] = $header_xaction; @@ -104,33 +111,7 @@ final class PhabricatorProjectMoveController $editor->applyTransactions($object, $xactions); - return $this->newCardResponse($board_phid, $object_phid); - } - - private function newHeaderTransactions( - ManiphestTask $task, - $order, - array $header) { - - $xactions = array(); - - switch ($order) { - case PhabricatorProjectColumn::ORDER_PRIORITY: - $new_priority = idx($header, $order); - - if ($task->getPriority() !== $new_priority) { - $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); - $keyword = head(idx($keyword_map, $new_priority)); - - $xactions[] = id(new ManiphestTransaction()) - ->setTransactionType( - ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) - ->setNewValue($keyword); - } - break; - } - - return $xactions; + return $this->newCardResponse($board_phid, $object_phid, $ordering); } } diff --git a/src/applications/project/engine/PhabricatorBoardResponseEngine.php b/src/applications/project/engine/PhabricatorBoardResponseEngine.php index 969dfa3bc8..36c5e81150 100644 --- a/src/applications/project/engine/PhabricatorBoardResponseEngine.php +++ b/src/applications/project/engine/PhabricatorBoardResponseEngine.php @@ -6,6 +6,7 @@ final class PhabricatorBoardResponseEngine extends Phobject { private $boardPHID; private $objectPHID; private $visiblePHIDs; + private $ordering; public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; @@ -43,10 +44,20 @@ final class PhabricatorBoardResponseEngine extends Phobject { return $this->visiblePHIDs; } + public function setOrdering(PhabricatorProjectColumnOrder $ordering) { + $this->ordering = $ordering; + return $this; + } + + public function getOrdering() { + return $this->ordering; + } + public function buildResponse() { $viewer = $this->getViewer(); $object_phid = $this->getObjectPHID(); $board_phid = $this->getBoardPHID(); + $ordering = $this->getOrdering(); // Load all the other tasks that are visible in the affected columns and // perform layout for them. @@ -74,10 +85,17 @@ final class PhabricatorBoardResponseEngine extends Phobject { ->setViewer($viewer) ->withPHIDs($visible_phids) ->execute(); + $all_visible = mpull($all_visible, null, 'getPHID'); - $order_maps = array(); - foreach ($all_visible as $visible) { - $order_maps[$visible->getPHID()] = $visible->getWorkboardOrderVectors(); + if ($ordering) { + $vectors = $ordering->getSortVectorsForObjects($all_visible); + $header_keys = $ordering->getHeaderKeysForObjects($all_visible); + $headers = $ordering->getHeadersForObjects($all_visible); + $headers = mpull($headers, 'toDictionary'); + } else { + $vectors = array(); + $header_keys = array(); + $headers = array(); } $object = id(new ManiphestTaskQuery()) @@ -91,14 +109,50 @@ final class PhabricatorBoardResponseEngine extends Phobject { $template = $this->buildTemplate($object); + $cards = array(); + foreach ($all_visible as $card_phid => $object) { + $card = array( + 'vectors' => array(), + 'headers' => array(), + 'properties' => array(), + 'nodeHTMLTemplate' => null, + ); + + if ($ordering) { + $order_key = $ordering->getColumnOrderKey(); + + $vector = idx($vectors, $card_phid); + if ($vector !== null) { + $card['vectors'][$order_key] = $vector; + } + + $header = idx($header_keys, $card_phid); + if ($header !== null) { + $card['headers'][$order_key] = $header; + } + + $card['properties'] = array( + 'points' => (double)$object->getPoints(), + 'status' => $object->getStatus(), + ); + } + + if ($card_phid === $object_phid) { + $card['nodeHTMLTemplate'] = hsprintf('%s', $template); + } + + $card['vectors'] = (object)$card['vectors']; + $card['headers'] = (object)$card['headers']; + $card['properties'] = (object)$card['properties']; + + $cards[$card_phid] = $card; + } + $payload = array( 'objectPHID' => $object_phid, - 'cardHTML' => $template, 'columnMaps' => $natural, - 'orderMaps' => $order_maps, - 'propertyMaps' => array( - $object_phid => $object->getWorkboardProperties(), - ), + 'cards' => $cards, + 'headers' => $headers, ); return id(new AphrontAjaxResponse()) diff --git a/src/applications/project/order/PhabricatorProjectColumnHeader.php b/src/applications/project/order/PhabricatorProjectColumnHeader.php new file mode 100644 index 0000000000..432b78279b --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnHeader.php @@ -0,0 +1,94 @@ +orderKey = $order_key; + return $this; + } + + public function getOrderKey() { + return $this->orderKey; + } + + public function setHeaderKey($header_key) { + $this->headerKey = $header_key; + return $this; + } + + public function getHeaderKey() { + return $this->headerKey; + } + + public function setSortVector(array $sort_vector) { + $this->sortVector = $sort_vector; + return $this; + } + + public function getSortVector() { + return $this->sortVector; + } + + public function setName($name) { + $this->name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setIcon(PHUIIconView$icon) { + $this->icon = $icon; + return $this; + } + + public function getIcon() { + return $this->icon; + } + + public function setEditProperties(array $edit_properties) { + $this->editProperties = $edit_properties; + return $this; + } + + public function getEditProperties() { + return $this->editProperties; + } + + public function toDictionary() { + return array( + 'order' => $this->getOrderKey(), + 'key' => $this->getHeaderKey(), + 'template' => hsprintf('%s', $this->newView()), + 'vector' => $this->getSortVector(), + 'editProperties' => $this->getEditProperties(), + ); + } + + private function newView() { + $icon_view = $this->getIcon(); + $name = $this->getName(); + + $template = phutil_tag( + 'li', + array( + 'class' => 'workboard-group-header', + ), + array( + $icon_view, + $name, + )); + + return $template; + } + +} diff --git a/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php new file mode 100644 index 0000000000..26f4d28601 --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php @@ -0,0 +1,12 @@ +viewer = $viewer; + return $this; + } + + final public function getViewer() { + return $this->viewer; + } + + final public function getColumnOrderKey() { + return $this->getPhobjectClassConstant('ORDERKEY'); + } + + final public static function getAllOrders() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass(__CLASS__) + ->setUniqueMethod('getColumnOrderKey') + ->execute(); + } + + final public static function getOrderByKey($key) { + $map = self::getAllOrders(); + + if (!isset($map[$key])) { + throw new Exception( + pht( + 'No column ordering exists with key "%s".', + $key)); + } + + return $map[$key]; + } + + final public function getColumnTransactions($object, array $header) { + $result = $this->newColumnTransactions($object, $header); + + if (!is_array($result) && !is_null($result)) { + throw new Exception( + pht( + 'Expected "newColumnTransactions()" on "%s" to return "null" or a '. + 'list of transactions, but got "%s".', + get_class($this), + phutil_describe_type($result))); + } + + if ($result === null) { + $result = array(); + } + + assert_instances_of($result, 'PhabricatorApplicationTransaction'); + + return $result; + } + + final public function getMenuIconIcon() { + return $this->newMenuIconIcon(); + } + + protected function newMenuIconIcon() { + return 'fa-sort-amount-asc'; + } + + abstract public function getDisplayName(); + + protected function newColumnTransactions($object, array $header) { + return array(); + } + + final public function getHeadersForObjects(array $objects) { + $headers = $this->newHeadersForObjects($objects); + + if (!is_array($headers)) { + throw new Exception( + pht( + 'Expected "newHeadersForObjects()" on "%s" to return a list '. + 'of headers, but got "%s".', + get_class($this), + phutil_describe_type($headers))); + } + + assert_instances_of($headers, 'PhabricatorProjectColumnHeader'); + + // Add a "0" to the end of each header. This makes them sort above object + // cards in the same group. + foreach ($headers as $header) { + $vector = $header->getSortVector(); + $vector[] = 0; + $header->setSortVector($vector); + } + + return $headers; + } + + protected function newHeadersForObjects(array $objects) { + return array(); + } + + final public function getSortVectorsForObjects(array $objects) { + $vectors = $this->newSortVectorsForObjects($objects); + + if (!is_array($vectors)) { + throw new Exception( + pht( + 'Expected "newSortVectorsForObjects()" on "%s" to return a '. + 'map of vectors, but got "%s".', + get_class($this), + phutil_describe_type($vectors))); + } + + assert_same_keys($objects, $vectors); + + return $vectors; + } + + protected function newSortVectorsForObjects(array $objects) { + $vectors = array(); + + foreach ($objects as $key => $object) { + $vectors[$key] = $this->newSortVectorForObject($object); + } + + return $vectors; + } + + protected function newSortVectorForObject($object) { + return array(); + } + + final public function getHeaderKeysForObjects(array $objects) { + $header_keys = $this->newHeaderKeysForObjects($objects); + + if (!is_array($header_keys)) { + throw new Exception( + pht( + 'Expected "newHeaderKeysForObject()" on "%s" to return a '. + 'map of header keys, but got "%s".', + get_class($this), + phutil_describe_type($header_keys))); + } + + assert_same_keys($objects, $header_keys); + + return $header_keys; + } + + protected function newHeaderKeysForObjects(array $objects) { + $header_keys = array(); + + foreach ($objects as $key => $object) { + $header_keys[$key] = $this->newHeaderKeyForObject($object); + } + + return $header_keys; + } + + protected function newHeaderKeyForObject($object) { + return null; + } + + final protected function newTransaction($object) { + return $object->getApplicationTransactionTemplate(); + } + + final protected function newHeader() { + return id(new PhabricatorProjectColumnHeader()) + ->setOrderKey($this->getColumnOrderKey()); + } + +} diff --git a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php new file mode 100644 index 0000000000..e08e7c07ef --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php @@ -0,0 +1,91 @@ +newHeaderKeyForPriority($object->getPriority()); + } + + private function newHeaderKeyForPriority($priority) { + return sprintf('priority(%d)', $priority); + } + + protected function newSortVectorForObject($object) { + return $this->newSortVectorForPriority($object->getPriority()); + } + + private function newSortVectorForPriority($priority) { + return array( + (int)-$priority, + ); + } + + protected function newHeadersForObjects(array $objects) { + $priorities = ManiphestTaskPriority::getTaskPriorityMap(); + + // It's possible for tasks to have an invalid/unknown priority in the + // database. We still want to generate a header for these tasks so we + // don't break the workboard. + $priorities = $priorities + mpull($objects, null, 'getPriority'); + + $priorities = array_keys($priorities); + + $headers = array(); + foreach ($priorities as $priority) { + $header_key = $this->newHeaderKeyForPriority($priority); + $sort_vector = $this->newSortVectorForPriority($priority); + + $priority_name = ManiphestTaskPriority::getTaskPriorityName($priority); + $priority_color = ManiphestTaskPriority::getTaskPriorityColor($priority); + $priority_icon = ManiphestTaskPriority::getTaskPriorityIcon($priority); + + $icon_view = id(new PHUIIconView()) + ->setIcon($priority_icon, $priority_color); + + $header = $this->newHeader() + ->setHeaderKey($header_key) + ->setSortVector($sort_vector) + ->setName($priority_name) + ->setIcon($icon_view) + ->setEditProperties( + array( + 'value' => (int)$priority, + )); + + $headers[] = $header; + } + + return $headers; + } + + protected function newColumnTransactions($object, array $header) { + $new_priority = idx($header, 'value'); + + if ($object->getPriority() === $new_priority) { + return null; + } + + $keyword_map = ManiphestTaskPriority::getTaskPriorityKeywordsMap(); + $keyword = head(idx($keyword_map, $new_priority)); + + $xactions = array(); + $xactions[] = $this->newTransaction($object) + ->setTransactionType(ManiphestTaskPriorityTransaction::TRANSACTIONTYPE) + ->setNewValue($keyword); + + return $xactions; + } + + +} diff --git a/src/applications/project/storage/PhabricatorProjectColumn.php b/src/applications/project/storage/PhabricatorProjectColumn.php index 03b9cbff70..febb2eb647 100644 --- a/src/applications/project/storage/PhabricatorProjectColumn.php +++ b/src/applications/project/storage/PhabricatorProjectColumn.php @@ -12,13 +12,6 @@ final class PhabricatorProjectColumn const STATUS_ACTIVE = 0; const STATUS_HIDDEN = 1; - const DEFAULT_ORDER = 'natural'; - const ORDER_NATURAL = 'natural'; - const ORDER_PRIORITY = 'priority'; - - const NODETYPE_HEADER = 0; - const NODETYPE_CARD = 1; - protected $name; protected $status; protected $projectPHID; diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index a746461325..cda48bde11 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -289,7 +289,6 @@ JX.install('WorkboardBoard', { var columns = this.getColumns(); var phid = response.objectPHID; - var card = this.getCardTemplate(phid); for (var add_phid in response.columnMaps) { var target_column = this.getColumn(add_phid); @@ -302,18 +301,6 @@ JX.install('WorkboardBoard', { target_column.newCard(phid); } - card.setNodeHTMLTemplate(response.cardHTML); - - var order_maps = response.orderMaps; - for (var order_phid in order_maps) { - var card_template = this.getCardTemplate(order_phid); - for (var order_key in order_maps[order_phid]) { - card_template.setSortVector( - order_key, - order_maps[order_phid][order_key]); - } - } - var column_maps = response.columnMaps; var natural_column; for (var natural_phid in column_maps) { @@ -329,10 +316,37 @@ JX.install('WorkboardBoard', { natural_column.setNaturalOrder(column_maps[natural_phid]); } - var property_maps = response.propertyMaps; - for (var property_phid in property_maps) { - this.getCardTemplate(property_phid) - .setObjectProperties(property_maps[property_phid]); + for (var card_phid in response.cards) { + var card_data = response.cards[card_phid]; + var card_template = this.getCardTemplate(card_phid); + + if (card_data.nodeHTMLTemplate) { + card_template.setNodeHTMLTemplate(card_data.nodeHTMLTemplate); + } + + var order; + for (order in card_data.vectors) { + card_template.setSortVector(order, card_data.vectors[order]); + } + + for (order in card_data.headers) { + card_template.setHeaderKey(order, card_data.headers[order]); + } + + for (var key in card_data.properties) { + card_template.setObjectProperty(key, card_data.properties[key]); + } + } + + var headers = response.headers; + for (var jj = 0; jj < headers.length; jj++) { + var header = headers[jj]; + + this.getHeaderTemplate(header.key) + .setOrder(header.order) + .setNodeHTMLTemplate(header.template) + .setVector(header.vector) + .setEditProperties(header.editProperties); } for (var column_phid in columns) { diff --git a/webroot/rsrc/js/application/projects/WorkboardCard.js b/webroot/rsrc/js/application/projects/WorkboardCard.js index e0eab6b53b..4a3be2a51d 100644 --- a/webroot/rsrc/js/application/projects/WorkboardCard.js +++ b/webroot/rsrc/js/application/projects/WorkboardCard.js @@ -29,7 +29,8 @@ JX.install('WorkboardCard', { }, getProperties: function() { - return this.getColumn().getBoard().getCardTemplate(this.getPHID()) + return this.getColumn().getBoard() + .getCardTemplate(this.getPHID()) .getObjectProperties(); }, @@ -41,10 +42,6 @@ JX.install('WorkboardCard', { return this.getProperties().status; }, - getPriority: function(order) { - return this.getProperties().priority; - }, - getNode: function() { if (!this._root) { var phid = this.getPHID(); diff --git a/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js b/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js index 70080b5364..58f3f9e97f 100644 --- a/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js +++ b/webroot/rsrc/js/application/projects/WorkboardCardTemplate.js @@ -9,6 +9,7 @@ JX.install('WorkboardCardTemplate', { construct: function(phid) { this._phid = phid; this._vectors = {}; + this._headerKeys = {}; this.setObjectProperties({}); }, @@ -19,7 +20,9 @@ JX.install('WorkboardCardTemplate', { members: { _phid: null, + _html: null, _vectors: null, + _headerKeys: null, getPHID: function() { return this._phid; @@ -39,8 +42,22 @@ JX.install('WorkboardCardTemplate', { return this._vectors[order]; }, + setHeaderKey: function(order, key) { + this._headerKeys[order] = key; + return this; + }, + + getHeaderKey: function(order) { + return this._headerKeys[order]; + }, + newNode: function() { return JX.$H(this._html).getFragment().firstChild; + }, + + setObjectProperty: function(key, value) { + this.getObjectProperties()[key] = value; + return this; } } diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index 1c5d6f1a59..a560234a2a 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -189,6 +189,9 @@ JX.install('WorkboardColumn', { var board = this.getBoard(); var order = board.getOrder(); + // TODO: This should be modularized into "ProjectColumnOrder" classes, + // but is currently hard-coded. + switch (order) { case 'natural': return false; @@ -197,15 +200,6 @@ JX.install('WorkboardColumn', { return true; }, - _getCardHeaderKey: function(card, order) { - switch (order) { - case 'priority': - return 'priority(' + card.getPriority() + ')'; - default: - return null; - } - }, - redraw: function() { var board = this.getBoard(); var order = board.getOrder(); @@ -235,7 +229,9 @@ JX.install('WorkboardColumn', { // cards in a column. if (has_headers) { - var header_key = this._getCardHeaderKey(card, order); + var header_key = board.getCardTemplate(card.getPHID()) + .getHeaderKey(order); + if (!seen_headers[header_key]) { while (header_keys.length) { var next = header_keys.pop(); diff --git a/webroot/rsrc/js/application/projects/WorkboardHeader.js b/webroot/rsrc/js/application/projects/WorkboardHeader.js index 0a8f4d9681..a0cbfc13c7 100644 --- a/webroot/rsrc/js/application/projects/WorkboardHeader.js +++ b/webroot/rsrc/js/application/projects/WorkboardHeader.js @@ -27,12 +27,16 @@ JX.install('WorkboardHeader', { getNode: function() { if (!this._root) { var header_key = this.getHeaderKey(); - var board = this.getColumn().getBoard(); - var template = board.getHeaderTemplate(header_key).getTemplate(); - this._root = JX.$H(template).getFragment().firstChild; - JX.Stratcom.getData(this._root).headerKey = header_key; + var root = this.getColumn().getBoard() + .getHeaderTemplate(header_key) + .newNode(); + + JX.Stratcom.getData(root).headerKey = header_key; + + this._root = root; } + return this._root; }, diff --git a/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js index add37d9c25..8376359270 100644 --- a/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js +++ b/webroot/rsrc/js/application/projects/WorkboardHeaderTemplate.js @@ -19,9 +19,19 @@ JX.install('WorkboardHeaderTemplate', { members: { _headerKey: null, + _html: null, getHeaderKey: function() { return this._headerKey; + }, + + setNodeHTMLTemplate: function(html) { + this._html = html; + return this; + }, + + newNode: function() { + return JX.$H(this._html).getFragment().firstChild; } } diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index da82c02040..51c067925f 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -116,11 +116,17 @@ JX.behavior('project-boards', function(config, statics) { board.getHeaderTemplate(header.key) .setOrder(header.order) - .setTemplate(header.template) + .setNodeHTMLTemplate(header.template) .setVector(header.vector) .setEditProperties(header.editProperties); } + var header_keys = config.headerKeys; + for (var header_phid in header_keys) { + board.getCardTemplate(header_phid) + .setHeaderKey(config.order, header_keys[header_phid]); + } + board.start(); }); From 2fdab434faf3c5f825985afa071b328a6b0b6bce Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 21:49:18 -0700 Subject: [PATCH 12/25] Implement "Group by Owner" on Workboards Summary: Depends on D20269. Ref T10333. Now that orderings are modularized, this is fairly easy to implement. This isn't super fancy for now (e.g., no profile images) but I'll touch it up in a general polish followup. Test Plan: {F6264596} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20270 --- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectColumnOwnerOrder.php | 163 ++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bdb1fee462..e79ae6ea68 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4056,6 +4056,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnHideController' => 'applications/project/controller/PhabricatorProjectColumnHideController.php', 'PhabricatorProjectColumnNaturalOrder' => 'applications/project/order/PhabricatorProjectColumnNaturalOrder.php', 'PhabricatorProjectColumnOrder' => 'applications/project/order/PhabricatorProjectColumnOrder.php', + 'PhabricatorProjectColumnOwnerOrder' => 'applications/project/order/PhabricatorProjectColumnOwnerOrder.php', 'PhabricatorProjectColumnPHIDType' => 'applications/project/phid/PhabricatorProjectColumnPHIDType.php', 'PhabricatorProjectColumnPosition' => 'applications/project/storage/PhabricatorProjectColumnPosition.php', 'PhabricatorProjectColumnPositionQuery' => 'applications/project/query/PhabricatorProjectColumnPositionQuery.php', @@ -10140,6 +10141,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnHideController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnNaturalOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnOrder' => 'Phobject', + 'PhabricatorProjectColumnOwnerOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnPHIDType' => 'PhabricatorPHIDType', 'PhabricatorProjectColumnPosition' => array( 'PhabricatorProjectDAO', diff --git a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php new file mode 100644 index 0000000000..c98ee61715 --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php @@ -0,0 +1,163 @@ +newHeaderKeyForOwnerPHID($object->getOwnerPHID()); + } + + private function newHeaderKeyForOwnerPHID($owner_phid) { + if ($owner_phid === null) { + $owner_phid = ''; + } + + return sprintf('owner(%s)', $owner_phid); + } + + protected function newSortVectorsForObjects(array $objects) { + $owner_phids = mpull($objects, null, 'getOwnerPHID'); + $owner_phids = array_keys($owner_phids); + $owner_phids = array_filter($owner_phids); + + if ($owner_phids) { + $owner_users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($owner_phids) + ->execute(); + $owner_users = mpull($owner_users, null, 'getPHID'); + } else { + $owner_users = array(); + } + + $vectors = array(); + foreach ($objects as $vector_key => $object) { + $owner_phid = $object->getOwnerPHID(); + if (!$owner_phid) { + $vector = $this->newSortVectorForUnowned(); + } else { + $owner = idx($owner_users, $owner_phid); + if ($owner) { + $vector = $this->newSortVectorForOwner($owner); + } else { + $vector = $this->newSortVectorForOwnerPHID($owner_phid); + } + } + + $vectors[$vector_key] = $vector; + } + + return $vectors; + } + + private function newSortVectorForUnowned() { + // Always put unasssigned tasks at the top. + return array( + 0, + ); + } + + private function newSortVectorForOwner(PhabricatorUser $user) { + // Put assigned tasks with a valid owner after "Unassigned", but above + // assigned tasks with an invalid owner. Sort these tasks by the owner's + // username. + return array( + 1, + $user->getUsername(), + ); + } + + private function newSortVectorForOwnerPHID($owner_phid) { + // If we have tasks with a nonempty owner but can't load the associated + // "User" object, move them to the bottom. We can only sort these by the + // PHID. + return array( + 2, + $owner_phid, + ); + } + + protected function newHeadersForObjects(array $objects) { + $owner_phids = mpull($objects, null, 'getOwnerPHID'); + $owner_phids = array_keys($owner_phids); + $owner_phids = array_filter($owner_phids); + + if ($owner_phids) { + $owner_users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($owner_phids) + ->execute(); + $owner_users = mpull($owner_users, null, 'getPHID'); + } else { + $owner_users = array(); + } + + array_unshift($owner_phids, null); + + $headers = array(); + foreach ($owner_phids as $owner_phid) { + $header_key = $this->newHeaderKeyForOwnerPHID($owner_phid); + + if ($owner_phid === null) { + $owner = null; + $sort_vector = $this->newSortVectorForUnowned(); + $owner_name = pht('Not Assigned'); + } else { + $owner = idx($owner_users, $owner_phid); + if ($owner) { + $sort_vector = $this->newSortVectorForOwner($owner); + $owner_name = $owner->getUsername(); + } else { + $sort_vector = $this->newSortVectorForOwnerPHID($owner_phid); + $owner_name = pht('Unknown User ("%s")', $owner_phid); + } + } + + $owner_icon = 'fa-user'; + $owner_color = 'bluegrey'; + + $icon_view = id(new PHUIIconView()) + ->setIcon($owner_icon, $owner_color); + + $header = $this->newHeader() + ->setHeaderKey($header_key) + ->setSortVector($sort_vector) + ->setName($owner_name) + ->setIcon($icon_view) + ->setEditProperties( + array( + 'value' => $owner_phid, + )); + + $headers[] = $header; + } + + return $headers; + } + + protected function newColumnTransactions($object, array $header) { + $new_owner = idx($header, 'value'); + + if ($object->getOwnerPHID() === $new_owner) { + return null; + } + + $xactions = array(); + $xactions[] = $this->newTransaction($object) + ->setTransactionType(ManiphestTaskOwnerTransaction::TRANSACTIONTYPE) + ->setNewValue($new_owner); + + return $xactions; + } + +} From 21dd79b35af2c07b3f689092822c4237bd8a65f5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 22:09:23 -0700 Subject: [PATCH 13/25] When creating or editing a card on a sorted/grouped workboard, adjust headers appropriately Summary: Depends on D20270. Ref T10333. If you create a task with a new owner, or edit a task and change the priority/owner, we want to move it (and possibly create a new header) when the response comes back. Make sure the response includes the appropriate details about the object's header and position. Test Plan: - Grouped by Owner. - Created a new task with a new owner, saw the header appear. - Edited a task and changed it to give it a new owner, saw the header appear. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20271 --- .../controller/ManiphestController.php | 23 ---------- .../maniphest/editor/ManiphestEditEngine.php | 44 +++++++++---------- 2 files changed, 22 insertions(+), 45 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestController.php b/src/applications/maniphest/controller/ManiphestController.php index 51a243afac..970095009b 100644 --- a/src/applications/maniphest/controller/ManiphestController.php +++ b/src/applications/maniphest/controller/ManiphestController.php @@ -37,29 +37,6 @@ abstract class ManiphestController extends PhabricatorController { return $crumbs; } - public function renderSingleTask(ManiphestTask $task) { - $request = $this->getRequest(); - $user = $request->getUser(); - - $phids = $task->getProjectPHIDs(); - if ($task->getOwnerPHID()) { - $phids[] = $task->getOwnerPHID(); - } - - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($user) - ->withPHIDs($phids) - ->execute(); - - $view = id(new ManiphestTaskListView()) - ->setUser($user) - ->setShowBatchControls(true) - ->setHandles($handles) - ->setTasks(array($task)); - - return $view; - } - final protected function newTaskGraphDropdownMenu( ManiphestTask $task, $has_parents, diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php index dc9c56f840..76c2276df0 100644 --- a/src/applications/maniphest/editor/ManiphestEditEngine.php +++ b/src/applications/maniphest/editor/ManiphestEditEngine.php @@ -379,7 +379,10 @@ EODOCS $object, array $xactions) { - if ($request->isAjax()) { + $response_type = $request->getStr('responseType'); + $is_card = ($response_type === 'card'); + + if ($is_card) { // Reload the task to make sure we pick up the final task state. $viewer = $this->getViewer(); $task = id(new ManiphestTaskQuery()) @@ -389,29 +392,12 @@ EODOCS ->needProjectPHIDs(true) ->executeOne(); - switch ($request->getStr('responseType')) { - case 'card': - return $this->buildCardResponse($task); - default: - return $this->buildListResponse($task); - } - + return $this->buildCardResponse($task); } return parent::newEditResponse($request, $object, $xactions); } - private function buildListResponse(ManiphestTask $task) { - $controller = $this->getController(); - - $payload = array( - 'tasks' => $controller->renderSingleTask($task), - 'data' => array(), - ); - - return id(new AphrontAjaxResponse())->setContent($payload); - } - private function buildCardResponse(ManiphestTask $task) { $controller = $this->getController(); $request = $controller->getRequest(); @@ -435,12 +421,26 @@ EODOCS $board_phid = $column->getProjectPHID(); $object_phid = $task->getPHID(); - return id(new PhabricatorBoardResponseEngine()) + $order = $request->getStr('order'); + if ($order) { + $ordering = PhabricatorProjectColumnOrder::getOrderByKey($order); + $ordering = id(clone $ordering) + ->setViewer($viewer); + } else { + $ordering = null; + } + + $engine = id(new PhabricatorBoardResponseEngine()) ->setViewer($viewer) ->setBoardPHID($board_phid) ->setObjectPHID($object_phid) - ->setVisiblePHIDs($visible_phids) - ->buildResponse(); + ->setVisiblePHIDs($visible_phids); + + if ($ordering) { + $engine->setOrdering($ordering); + } + + return $engine->buildResponse(); } private function getColumnMap(ManiphestTask $task) { From 1bdf446a802602a156c9bfd2decdfd5cdaf7ee51 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 22:22:59 -0700 Subject: [PATCH 14/25] Improve rendering of empty workboard columns in header views Summary: Depends on D20271. Ref T10333. When a column is empty but a board is grouped (by priority, owner, etc) render the headers properly. When a column has headers, don't apply the "empty" style even if it has no cards. This style just makes some empty space so you can drag-and-drop more easily, but headers do the same thing. Test Plan: {F6264611} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20272 --- resources/celerity/map.php | 14 ++++----- .../application/projects/WorkboardColumn.js | 31 +++++++++++++++++-- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c96d03896c..efa2e80f17 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -411,7 +411,7 @@ return array( 'rsrc/js/application/projects/WorkboardBoard.js' => 'fc1664ff', 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', - 'rsrc/js/application/projects/WorkboardColumn.js' => '533dd592', + 'rsrc/js/application/projects/WorkboardColumn.js' => 'fd4c2069', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/WorkboardHeader.js' => '111bfd2d', 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => 'b65351bd', @@ -731,7 +731,7 @@ return array( 'javelin-workboard-board' => 'fc1664ff', 'javelin-workboard-card' => '0392a5d8', 'javelin-workboard-card-template' => '2a61f8d4', - 'javelin-workboard-column' => '533dd592', + 'javelin-workboard-column' => 'fd4c2069', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workboard-header' => '111bfd2d', 'javelin-workboard-header-template' => 'b65351bd', @@ -1347,11 +1347,6 @@ return array( 'javelin-dom', 'javelin-fx', ), - '533dd592' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), '534f1757' => array( 'phui-oi-list-view-css', ), @@ -2138,6 +2133,11 @@ return array( 'javelin-magical-init', 'javelin-util', ), + 'fd4c2069' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), 'fdc13e4e' => array( 'javelin-install', ), diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index a560234a2a..271bed467c 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -220,6 +220,8 @@ JX.install('WorkboardColumn', { header_keys.reverse(); } + var header_key; + var next; for (ii = 0; ii < list.length; ii++) { var card = list[ii]; @@ -229,12 +231,12 @@ JX.install('WorkboardColumn', { // cards in a column. if (has_headers) { - var header_key = board.getCardTemplate(card.getPHID()) + header_key = board.getCardTemplate(card.getPHID()) .getHeaderKey(order); if (!seen_headers[header_key]) { while (header_keys.length) { - var next = header_keys.pop(); + next = header_keys.pop(); var header = this.getHeader(next); objects.push(header); @@ -250,6 +252,20 @@ JX.install('WorkboardColumn', { objects.push(card); } + // Add any leftover headers at the bottom of the column which don't have + // any cards in them. In particular, empty columns don't have any cards + // but should still have headers. + + while (header_keys.length) { + next = header_keys.pop(); + + if (seen_headers[next]) { + continue; + } + + objects.push(this.getHeader(next)); + } + this._objects = objects; var content = []; @@ -431,9 +447,18 @@ JX.install('WorkboardColumn', { JX.DOM.setContent(content_node, display_value); - var is_empty = !this.getCardPHIDs().length; + // Only put the "empty" style on the column (which just adds some empty + // space so it's easier to drop cards into an empty column) if it has no + // cards and no headers. + + var is_empty = + (!this.getCardPHIDs().length) && + (!this._hasColumnHeaders()); + var panel = JX.DOM.findAbove(this.getRoot(), 'div', 'workpanel'); JX.DOM.alterClass(panel, 'project-panel-empty', is_empty); + + JX.DOM.alterClass(panel, 'project-panel-over-limit', over_limit); var color_map = { From 74de153e59be0f14f0cd1e49a81660e7e72f82af Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 22:31:35 -0700 Subject: [PATCH 15/25] Allow MFA task edits to go through on workboards Summary: Depends on D20272. Ref T13074. When a task requires MFA to edit, you currently get a fatal. Provide a cancel URI so the prompt works and the edit can go through. Test Plan: - Locked a task, dragged it on a workboard. - Before: fatal trying to build an MFA gate. - After: got MFA gated, answered prompt, action went through. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13074 Differential Revision: https://secure.phabricator.com/D20273 --- .../controller/PhabricatorProjectMoveController.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 6d7902a733..3cfd94894b 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -43,6 +43,13 @@ final class PhabricatorProjectMoveController return new Aphront404Response(); } + $cancel_uri = $this->getApplicationURI( + new PhutilURI( + urisprintf('board/%d/', $project->getID()), + array( + 'order' => $order, + ))); + $board_phid = $project->getPHID(); $object = id(new ManiphestTaskQuery()) @@ -107,7 +114,8 @@ final class PhabricatorProjectMoveController ->setActor($viewer) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); + ->setContentSourceFromRequest($request) + ->setCancelURI($cancel_uri); $editor->applyTransactions($object, $xactions); From 804be81f5dbbe08059c2ebe1a6ee96b89c32968f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 10 Mar 2019 22:56:54 -0700 Subject: [PATCH 16/25] Provide better UI feedback about cards that can't be dragged or edited Summary: Depends on D20273. Fixes T10722. Currently, we don't make it very clear when a card can't be edited. Long ago, some code made a weak attempt to do this (by hiding the "grip" on the card), but later UI changes hid the "grip" unconditionally so that mooted things. Instead: - Replace the edit pencil with a red lock. - Provide cursor hints for grabbable / not grabbable. - Don't let users pick up cards they can't edit. Test Plan: On a workboard with a mixture of editable and not-editable cards, hovered over the different cards and was able to figure out which ones I could drag or not drag pretty easily. Picked up cards I could pick up, wasn't able to drag cards I can't edit. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10722 Differential Revision: https://secure.phabricator.com/D20274 --- resources/celerity/map.php | 52 +++++++++---------- .../project/view/ProjectBoardTaskCard.php | 30 +++++++---- .../css/phui/workboards/phui-workcard.css | 35 ++++++++++--- .../js/application/projects/WorkboardBoard.js | 2 +- webroot/rsrc/js/core/DraggableList.js | 2 + 5 files changed, 77 insertions(+), 44 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index efa2e80f17..33d8b09bc8 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', - 'core.pkg.js' => 'b96c872e', + 'core.pkg.js' => '200a0a61', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -177,7 +177,7 @@ return array( 'rsrc/css/phui/phui-two-column-view.css' => '01e6991e', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', 'rsrc/css/phui/workboards/phui-workboard.css' => '74fc9d98', - 'rsrc/css/phui/workboards/phui-workcard.css' => '8c536f90', + 'rsrc/css/phui/workboards/phui-workcard.css' => '9e9eb0df', 'rsrc/css/phui/workboards/phui-workpanel.css' => 'bc16cf33', 'rsrc/css/sprite-login.css' => '18b368a6', 'rsrc/css/sprite-tokens.css' => 'f1896dc5', @@ -408,7 +408,7 @@ 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' => 'fc1664ff', + 'rsrc/js/application/projects/WorkboardBoard.js' => 'eb55f7e8', 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', 'rsrc/js/application/projects/WorkboardColumn.js' => 'fd4c2069', @@ -436,7 +436,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '29819b75', 'rsrc/js/core/Busy.js' => '5202e831', 'rsrc/js/core/DragAndDropFileUpload.js' => '4370900d', - 'rsrc/js/core/DraggableList.js' => '8437c663', + 'rsrc/js/core/DraggableList.js' => '91f40fbf', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', 'rsrc/js/core/Hovercard.js' => '074f0783', @@ -728,7 +728,7 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'fc1664ff', + 'javelin-workboard-board' => 'eb55f7e8', 'javelin-workboard-card' => '0392a5d8', 'javelin-workboard-card-template' => '2a61f8d4', 'javelin-workboard-column' => 'fd4c2069', @@ -759,7 +759,7 @@ return array( 'phabricator-diff-changeset-list' => '04023d82', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', - 'phabricator-draggable-list' => '8437c663', + 'phabricator-draggable-list' => '91f40fbf', 'phabricator-fatal-config-template-css' => '20babf50', 'phabricator-favicon' => '7930776a', 'phabricator-feed-css' => 'd8b6e3f8', @@ -857,7 +857,7 @@ return array( 'phui-two-column-view-css' => '01e6991e', 'phui-workboard-color-css' => 'e86de308', 'phui-workboard-view-css' => '74fc9d98', - 'phui-workcard-view-css' => '8c536f90', + 'phui-workcard-view-css' => '9e9eb0df', 'phui-workpanel-view-css' => 'bc16cf33', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', @@ -1557,14 +1557,6 @@ return array( 'javelin-dom', 'javelin-vector', ), - '8437c663' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), '87428eb2' => array( 'javelin-behavior', 'javelin-diffusion-locate-file-source', @@ -1643,6 +1635,14 @@ return array( 'javelin-workflow', 'javelin-stratcom', ), + '91f40fbf' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), '92388bae' => array( 'javelin-behavior', 'javelin-scrollbar', @@ -2051,6 +2051,17 @@ return array( 'javelin-install', 'javelin-event', ), + 'eb55f7e8' => 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', + ), 'ec4e31c0' => array( 'phui-timeline-view-css', ), @@ -2118,17 +2129,6 @@ return array( 'phabricator-keyboard-shortcut', 'conpherence-thread-manager', ), - 'fc1664ff' => 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', - ), 'fce5d170' => array( 'javelin-magical-init', 'javelin-util', diff --git a/src/applications/project/view/ProjectBoardTaskCard.php b/src/applications/project/view/ProjectBoardTaskCard.php index 3a7016ca74..bb1c8ca8c5 100644 --- a/src/applications/project/view/ProjectBoardTaskCard.php +++ b/src/applications/project/view/ProjectBoardTaskCard.php @@ -82,20 +82,32 @@ final class ProjectBoardTaskCard extends Phobject { $card = id(new PHUIObjectItemView()) ->setObject($task) ->setUser($viewer) - ->setObjectName('T'.$task->getID()) + ->setObjectName($task->getMonogram()) ->setHeader($task->getTitle()) - ->setGrippable($can_edit) - ->setHref('/T'.$task->getID()) + ->setHref($task->getURI()) ->addSigil('project-card') ->setDisabled($task->isClosed()) - ->addAction( - id(new PHUIListItemView()) - ->setName(pht('Edit')) - ->setIcon('fa-pencil') - ->addSigil('edit-project-card') - ->setHref('/maniphest/task/edit/'.$task->getID().'/')) ->setBarColor($bar_color); + if ($can_edit) { + $card + ->addSigil('draggable-card') + ->addClass('draggable-card'); + $edit_icon = 'fa-pencil'; + } else { + $card + ->addClass('not-editable') + ->addClass('undraggable-card'); + $edit_icon = 'fa-lock red'; + } + + $card->addAction( + id(new PHUIListItemView()) + ->setName(pht('Edit')) + ->setIcon($edit_icon) + ->addSigil('edit-project-card') + ->setHref('/maniphest/task/edit/'.$task->getID().'/')); + if ($owner) { $card->addHandleIcon($owner, $owner->getName()); } diff --git a/webroot/rsrc/css/phui/workboards/phui-workcard.css b/webroot/rsrc/css/phui/workboards/phui-workcard.css index e137e962bc..3c6a798fc8 100644 --- a/webroot/rsrc/css/phui/workboards/phui-workcard.css +++ b/webroot/rsrc/css/phui/workboards/phui-workcard.css @@ -59,14 +59,6 @@ vertical-align: top; } -.phui-workcard.phui-oi-grippable .phui-oi-frame { - padding-left: 0; -} - -.phui-workcard .phui-oi-grip { - display: none; -} - .device-desktop .phui-workcard .phui-list-item-icon { display: none; } @@ -88,6 +80,33 @@ opacity: 1; } +.device-desktop .phui-workcard.draggable-card { + cursor: grab; +} + +.jx-dragging .phui-workcard.draggable-card { + cursor: grabbing; +} + +.device-desktop .phui-workcard.undraggable-card { + cursor: not-allowed; +} + +.device-desktop .phui-workcard.phui-oi.not-editable:hover { + background: {$sh-redbackground}; +} + +.device-desktop .phui-workcard.phui-oi.not-editable:hover + .phui-list-item-href { + border-radius: 3px; + background: {$red}; +} + +.device-desktop .phui-workcard.phui-oi.not-editable:hover + .phui-list-item-href .phui-icon-view { + color: #fff; +} + .phui-workcard.phui-oi:hover .phui-list-item-icon { display: block; } diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index cda48bde11..a05777e2f2 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -138,7 +138,7 @@ JX.install('WorkboardBoard', { for (var k in columns) { var column = columns[k]; - var list = new JX.DraggableList('project-card', column.getRoot()) + var list = new JX.DraggableList('draggable-card', column.getRoot()) .setOuterContainer(this.getRoot()) .setFindItemsHandler(JX.bind(column, column.getDropTargetNodes)) .setCanDragX(true) diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index 598856581f..1c7ca766f2 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -240,6 +240,7 @@ JX.install('DraggableList', { frame.appendChild(clone); document.body.appendChild(frame); + JX.DOM.alterClass(document.body, 'jx-dragging', true); this._dragging = drag; this._clone = clone; @@ -618,6 +619,7 @@ JX.install('DraggableList', { this._autoscroller = null; JX.DOM.remove(this._frame); + JX.DOM.alterClass(document.body, 'jx-dragging', false); this._frame = null; this._clone = null; From c020f027bbe6c17c1ba4a38150a4935b7fb9668d Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Mar 2019 08:58:46 -0700 Subject: [PATCH 17/25] Add an "Sort by Creation Date" filter to workboards and modularize remaining order behaviors Summary: Depends on D20274. Ref T10578. This is en route to an ordering by points, it's just a simpler half-step on the way there. Allow columns to be sorted by creation date, so the newest tasks rise to the top. In this ordering you can never reposition cards, since editing a creation date by dragging makes no sense. This will be true of the "points" ordering too (although we could imagine doing something like prompting the user, some day). Test Plan: Viewed boards by "natural" (allows reordering both when dragging within and between columns), "priority" (reorder only within columns), and "creation date" (reorder never). Dragged cards around between and within columns, got apparently sensible behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10578 Differential Revision: https://secure.phabricator.com/D20275 --- resources/celerity/map.php | 90 ++++++++++--------- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectBoardViewController.php | 4 + .../PhabricatorProjectColumnCreatedOrder.php | 31 +++++++ .../PhabricatorProjectColumnNaturalOrder.php | 8 ++ .../order/PhabricatorProjectColumnOrder.php | 10 +++ .../PhabricatorProjectColumnOwnerOrder.php | 8 ++ .../PhabricatorProjectColumnPriorityOrder.php | 8 ++ .../js/application/projects/WorkboardBoard.js | 31 ++++++- .../application/projects/WorkboardColumn.js | 10 +-- .../projects/WorkboardOrderTemplate.js | 27 ++++++ .../projects/behavior-project-boards.js | 16 +++- webroot/rsrc/js/core/DraggableList.js | 25 +++++- 13 files changed, 212 insertions(+), 58 deletions(-) create mode 100644 src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php create mode 100644 webroot/rsrc/js/application/projects/WorkboardOrderTemplate.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 33d8b09bc8..014839c14a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '34ce1741', - 'core.pkg.js' => '200a0a61', + 'core.pkg.js' => 'f9c2509b', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -408,14 +408,15 @@ 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' => 'eb55f7e8', + 'rsrc/js/application/projects/WorkboardBoard.js' => '9d59f098', 'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8', 'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4', - 'rsrc/js/application/projects/WorkboardColumn.js' => 'fd4c2069', + 'rsrc/js/application/projects/WorkboardColumn.js' => 'ec5c5ce0', 'rsrc/js/application/projects/WorkboardController.js' => '42c7a5a7', 'rsrc/js/application/projects/WorkboardHeader.js' => '111bfd2d', 'rsrc/js/application/projects/WorkboardHeaderTemplate.js' => 'b65351bd', - 'rsrc/js/application/projects/behavior-project-boards.js' => '285c337a', + 'rsrc/js/application/projects/WorkboardOrderTemplate.js' => '03e8891f', + 'rsrc/js/application/projects/behavior-project-boards.js' => '412af9d4', 'rsrc/js/application/projects/behavior-project-create.js' => '34c53422', 'rsrc/js/application/projects/behavior-reorder-columns.js' => '8ac32fd9', 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', @@ -436,7 +437,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '29819b75', 'rsrc/js/core/Busy.js' => '5202e831', 'rsrc/js/core/DragAndDropFileUpload.js' => '4370900d', - 'rsrc/js/core/DraggableList.js' => '91f40fbf', + 'rsrc/js/core/DraggableList.js' => '8bc7d797', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', 'rsrc/js/core/Hovercard.js' => '074f0783', @@ -656,7 +657,7 @@ return array( 'javelin-behavior-phuix-example' => 'c2c500a7', 'javelin-behavior-policy-control' => '0eaa33a9', 'javelin-behavior-policy-rule-editor' => '9347f172', - 'javelin-behavior-project-boards' => '285c337a', + 'javelin-behavior-project-boards' => '412af9d4', 'javelin-behavior-project-create' => '34c53422', 'javelin-behavior-quicksand-blacklist' => '5a6f6a06', 'javelin-behavior-read-only-warning' => 'b9109f8f', @@ -728,13 +729,14 @@ return array( 'javelin-view-renderer' => '9aae2b66', 'javelin-view-visitor' => '308f9fe4', 'javelin-websocket' => 'fdc13e4e', - 'javelin-workboard-board' => 'eb55f7e8', + 'javelin-workboard-board' => '9d59f098', 'javelin-workboard-card' => '0392a5d8', 'javelin-workboard-card-template' => '2a61f8d4', - 'javelin-workboard-column' => 'fd4c2069', + 'javelin-workboard-column' => 'ec5c5ce0', 'javelin-workboard-controller' => '42c7a5a7', 'javelin-workboard-header' => '111bfd2d', 'javelin-workboard-header-template' => 'b65351bd', + 'javelin-workboard-order-template' => '03e8891f', 'javelin-workflow' => '958e9045', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', @@ -759,7 +761,7 @@ return array( 'phabricator-diff-changeset-list' => '04023d82', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', - 'phabricator-draggable-list' => '91f40fbf', + 'phabricator-draggable-list' => '8bc7d797', 'phabricator-fatal-config-template-css' => '20babf50', 'phabricator-favicon' => '7930776a', 'phabricator-feed-css' => 'd8b6e3f8', @@ -912,6 +914,9 @@ return array( '0392a5d8' => array( 'javelin-install', ), + '03e8891f' => array( + 'javelin-install', + ), '04023d82' => array( 'javelin-install', 'phuix-button-view', @@ -1105,15 +1110,6 @@ return array( 'javelin-json', 'phabricator-prefab', ), - '285c337a' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-workboard-controller', - ), '289bf236' => array( 'javelin-install', 'javelin-util', @@ -1231,6 +1227,15 @@ return array( 'javelin-behavior', 'javelin-uri', ), + '412af9d4' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-workboard-controller', + ), '4234f572' => array( 'syntax-default-css', ), @@ -1588,6 +1593,14 @@ return array( 'javelin-dom', 'javelin-typeahead-normalizer', ), + '8bc7d797' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), '8c2ed2bf' => array( 'javelin-behavior', 'javelin-dom', @@ -1635,14 +1648,6 @@ return array( 'javelin-workflow', 'javelin-stratcom', ), - '91f40fbf' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), '92388bae' => array( 'javelin-behavior', 'javelin-scrollbar', @@ -1720,6 +1725,18 @@ return array( 'javelin-uri', 'phabricator-textareautils', ), + '9d59f098' => 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', + ), '9f081f05' => array( 'javelin-behavior', 'javelin-dom', @@ -2051,20 +2068,14 @@ return array( 'javelin-install', 'javelin-event', ), - 'eb55f7e8' => 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', - ), 'ec4e31c0' => array( 'phui-timeline-view-css', ), + 'ec5c5ce0' => array( + 'javelin-install', + 'javelin-workboard-card', + 'javelin-workboard-header', + ), 'ee77366f' => array( 'aphront-dialog-view-css', ), @@ -2133,11 +2144,6 @@ return array( 'javelin-magical-init', 'javelin-util', ), - 'fd4c2069' => array( - 'javelin-install', - 'javelin-workboard-card', - 'javelin-workboard-header', - ), 'fdc13e4e' => array( 'javelin-install', ), diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e79ae6ea68..9716e7180e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4050,6 +4050,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColorTransaction' => 'applications/project/xaction/PhabricatorProjectColorTransaction.php', 'PhabricatorProjectColorsConfigType' => 'applications/project/config/PhabricatorProjectColorsConfigType.php', 'PhabricatorProjectColumn' => 'applications/project/storage/PhabricatorProjectColumn.php', + 'PhabricatorProjectColumnCreatedOrder' => 'applications/project/order/PhabricatorProjectColumnCreatedOrder.php', 'PhabricatorProjectColumnDetailController' => 'applications/project/controller/PhabricatorProjectColumnDetailController.php', 'PhabricatorProjectColumnEditController' => 'applications/project/controller/PhabricatorProjectColumnEditController.php', 'PhabricatorProjectColumnHeader' => 'applications/project/order/PhabricatorProjectColumnHeader.php', @@ -10135,6 +10136,7 @@ phutil_register_library_map(array( 'PhabricatorExtendedPolicyInterface', 'PhabricatorConduitResultInterface', ), + 'PhabricatorProjectColumnCreatedOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnDetailController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnEditController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnHeader' => 'Phobject', diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 43599fa369..8784847f37 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -631,6 +631,9 @@ final class PhabricatorProjectBoardViewController $header_keys = $ordering->getHeaderKeysForObjects($all_tasks); + $order_maps = array(); + $order_maps[] = $ordering->toDictionary(); + $properties = array(); $behavior_config = array( @@ -642,6 +645,7 @@ final class PhabricatorProjectBoardViewController 'boardPHID' => $project->getPHID(), 'order' => $this->sortKey, + 'orders' => $order_maps, 'headers' => $headers, 'headerKeys' => $header_keys, 'templateMap' => $templates, diff --git a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php new file mode 100644 index 0000000000..945445aecf --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php @@ -0,0 +1,31 @@ +getDateCreated(), + (int)-$object->getID(), + ); + } + +} diff --git a/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php index 26f4d28601..b21a554715 100644 --- a/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php @@ -9,4 +9,12 @@ final class PhabricatorProjectColumnNaturalOrder return pht('Natural'); } + public function getHasHeaders() { + return false; + } + + public function getCanReorder() { + return true; + } + } diff --git a/src/applications/project/order/PhabricatorProjectColumnOrder.php b/src/applications/project/order/PhabricatorProjectColumnOrder.php index 815ab19f80..f3a1ed86ca 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOrder.php @@ -68,6 +68,8 @@ abstract class PhabricatorProjectColumnOrder } abstract public function getDisplayName(); + abstract public function getHasHeaders(); + abstract public function getCanReorder(); protected function newColumnTransactions($object, array $header) { return array(); @@ -173,4 +175,12 @@ abstract class PhabricatorProjectColumnOrder ->setOrderKey($this->getColumnOrderKey()); } + final public function toDictionary() { + return array( + 'orderKey' => $this->getColumnOrderKey(), + 'hasHeaders' => $this->getHasHeaders(), + 'canReorder' => $this->getCanReorder(), + ); + } + } diff --git a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php index c98ee61715..a41b78a11c 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php @@ -13,6 +13,14 @@ final class PhabricatorProjectColumnOwnerOrder return 'fa-users'; } + public function getHasHeaders() { + return true; + } + + public function getCanReorder() { + return true; + } + protected function newHeaderKeyForObject($object) { return $this->newHeaderKeyForOwnerPHID($object->getOwnerPHID()); } diff --git a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php index e08e7c07ef..64a5934e26 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php @@ -13,6 +13,14 @@ final class PhabricatorProjectColumnPriorityOrder return 'fa-sort-numeric-asc'; } + public function getHasHeaders() { + return true; + } + + public function getCanReorder() { + return true; + } + protected function newHeaderKeyForObject($object) { return $this->newHeaderKeyForPriority($object->getPriority()); } diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index a05777e2f2..fa10b2a180 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -9,6 +9,7 @@ * javelin-workboard-column * javelin-workboard-header-template * javelin-workboard-card-template + * javelin-workboard-order-template * @javelin */ @@ -21,6 +22,7 @@ JX.install('WorkboardBoard', { this._headers = {}; this._cards = {}; + this._orders = {}; this._buildColumns(); }, @@ -70,6 +72,14 @@ JX.install('WorkboardBoard', { return this._headers[header_key]; }, + getOrderTemplate: function(order_key) { + if (!this._orders[order_key]) { + this._orders[order_key] = new JX.WorkboardOrderTemplate(order_key); + } + + return this._orders[order_key]; + }, + getHeaderTemplatesForOrder: function(order) { var templates = []; @@ -134,6 +144,10 @@ JX.install('WorkboardBoard', { _setupDragHandlers: function() { var columns = this.getColumns(); + var order_template = this.getOrderTemplate(this.getOrder()); + var has_headers = order_template.getHasHeaders(); + var can_reorder = order_template.getCanReorder(); + var lists = []; for (var k in columns) { var column = columns[k]; @@ -149,8 +163,21 @@ JX.install('WorkboardBoard', { list.setGhostHandler( JX.bind(column, column.handleDragGhost, default_handler)); - if (this.getOrder() !== 'natural') { - list.setCompareHandler(JX.bind(column, column.compareHandler)); + // The "compare handler" locks cards into a specific position in the + // column. + list.setCompareHandler(JX.bind(column, column.compareHandler)); + + // If the view has group headers, we lock cards into the right position + // when moving them between columns, but not within a column. + if (has_headers) { + list.setCompareOnMove(true); + } + + // If we can't reorder cards, we always lock them into their current + // position. + if (!can_reorder) { + list.setCompareOnMove(true); + list.setCompareOnReorder(true); } list.listen('didDrop', JX.bind(this, this._onmovecard, list)); diff --git a/webroot/rsrc/js/application/projects/WorkboardColumn.js b/webroot/rsrc/js/application/projects/WorkboardColumn.js index 271bed467c..709c52016a 100644 --- a/webroot/rsrc/js/application/projects/WorkboardColumn.js +++ b/webroot/rsrc/js/application/projects/WorkboardColumn.js @@ -189,15 +189,7 @@ JX.install('WorkboardColumn', { var board = this.getBoard(); var order = board.getOrder(); - // TODO: This should be modularized into "ProjectColumnOrder" classes, - // but is currently hard-coded. - - switch (order) { - case 'natural': - return false; - } - - return true; + return board.getOrderTemplate(order).getHasHeaders(); }, redraw: function() { diff --git a/webroot/rsrc/js/application/projects/WorkboardOrderTemplate.js b/webroot/rsrc/js/application/projects/WorkboardOrderTemplate.js new file mode 100644 index 0000000000..083dc78b50 --- /dev/null +++ b/webroot/rsrc/js/application/projects/WorkboardOrderTemplate.js @@ -0,0 +1,27 @@ +/** + * @provides javelin-workboard-order-template + * @requires javelin-install + * @javelin + */ + +JX.install('WorkboardOrderTemplate', { + + construct: function(order) { + this._orderKey = order; + }, + + properties: { + hasHeaders: false, + canReorder: false + }, + + members: { + _orderKey: null, + + getOrderKey: function() { + return this._orderKey; + } + + } + +}); diff --git a/webroot/rsrc/js/application/projects/behavior-project-boards.js b/webroot/rsrc/js/application/projects/behavior-project-boards.js index 51c067925f..3aa43722c4 100644 --- a/webroot/rsrc/js/application/projects/behavior-project-boards.js +++ b/webroot/rsrc/js/application/projects/behavior-project-boards.js @@ -87,11 +87,12 @@ JX.behavior('project-boards', function(config, statics) { .setNodeHTMLTemplate(templates[k]); } + var ii; var column_maps = config.columnMaps; for (var column_phid in column_maps) { var column = board.getColumn(column_phid); var column_map = column_maps[column_phid]; - for (var ii = 0; ii < column_map.length; ii++) { + for (ii = 0; ii < column_map.length; ii++) { column.newCard(column_map[ii]); } } @@ -111,8 +112,8 @@ JX.behavior('project-boards', function(config, statics) { } var headers = config.headers; - for (var jj = 0; jj < headers.length; jj++) { - var header = headers[jj]; + for (ii = 0; ii < headers.length; ii++) { + var header = headers[ii]; board.getHeaderTemplate(header.key) .setOrder(header.order) @@ -121,6 +122,15 @@ JX.behavior('project-boards', function(config, statics) { .setEditProperties(header.editProperties); } + var orders = config.orders; + for (ii = 0; ii < orders.length; ii++) { + var order = orders[ii]; + + board.getOrderTemplate(order.orderKey) + .setHasHeaders(order.hasHeaders) + .setCanReorder(order.canReorder); + } + var header_keys = config.headerKeys; for (var header_phid in header_keys) { board.getCardTemplate(header_phid) diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index 1c7ca766f2..64f57503b8 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -43,7 +43,9 @@ JX.install('DraggableList', { isDropTargetHandler: null, canDragX: false, outerContainer: null, - hasInfiniteHeight: false + hasInfiniteHeight: false, + compareOnMove: false, + compareOnReorder: false }, members : { @@ -501,7 +503,26 @@ JX.install('DraggableList', { var cur_target = false; if (target_list) { - if (compare_handler && (target_list !== this)) { + // Determine if we're going to use the compare handler or not: the + // compare hander locks items into a specific place in the list. For + // example, on Workboards, some operations permit the user to drag + // items between lists, but not to reorder items within a list. + + var should_compare = false; + + var is_reorder = (target_list === this); + var is_move = (target_list !== this); + + if (compare_handler) { + if (is_reorder && this.getCompareOnReorder()) { + should_compare = true; + } + if (is_move && this.getCompareOnMove()) { + should_compare = true; + } + } + + if (should_compare) { cur_target = target_list._getOrderedTarget(this, this._dragging); } else { cur_target = target_list._getCurrentTarget(p); From 03b7aca019d18e8056e3c3d90c5c8bdd8496dd24 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Mar 2019 09:38:55 -0700 Subject: [PATCH 18/25] Implement "Sort by Points" on workboards Summary: Depends on D20275. Fixes T10578. This is a static sorting (like "By Date Created") where you can't change point values by dragging. You can still drag cards between columns, or use the "Edit" icon to change point values. Test Plan: {F6265191} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10578 Differential Revision: https://secure.phabricator.com/D20276 --- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectBoardViewController.php | 16 +++++- .../PhabricatorProjectColumnCreatedOrder.php | 8 ++- .../PhabricatorProjectColumnNaturalOrder.php | 4 ++ .../order/PhabricatorProjectColumnOrder.php | 21 ++++++++ .../PhabricatorProjectColumnOwnerOrder.php | 4 ++ .../PhabricatorProjectColumnPointsOrder.php | 50 +++++++++++++++++++ .../PhabricatorProjectColumnPriorityOrder.php | 6 ++- 8 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 src/applications/project/order/PhabricatorProjectColumnPointsOrder.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9716e7180e..e3b6af7974 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4059,6 +4059,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnOrder' => 'applications/project/order/PhabricatorProjectColumnOrder.php', 'PhabricatorProjectColumnOwnerOrder' => 'applications/project/order/PhabricatorProjectColumnOwnerOrder.php', 'PhabricatorProjectColumnPHIDType' => 'applications/project/phid/PhabricatorProjectColumnPHIDType.php', + 'PhabricatorProjectColumnPointsOrder' => 'applications/project/order/PhabricatorProjectColumnPointsOrder.php', 'PhabricatorProjectColumnPosition' => 'applications/project/storage/PhabricatorProjectColumnPosition.php', 'PhabricatorProjectColumnPositionQuery' => 'applications/project/query/PhabricatorProjectColumnPositionQuery.php', 'PhabricatorProjectColumnPriorityOrder' => 'applications/project/order/PhabricatorProjectColumnPriorityOrder.php', @@ -10145,6 +10146,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnOrder' => 'Phobject', 'PhabricatorProjectColumnOwnerOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorProjectColumnPointsOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnPosition' => array( 'PhabricatorProjectDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 8784847f37..cc95958bf7 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -616,7 +616,7 @@ final class PhabricatorProjectBoardViewController $order_key = $this->sortKey; - $ordering_map = PhabricatorProjectColumnOrder::getAllOrders(); + $ordering_map = PhabricatorProjectColumnOrder::getEnabledOrders(); $ordering = id(clone $ordering_map[$order_key]) ->setViewer($viewer); @@ -635,6 +635,12 @@ final class PhabricatorProjectBoardViewController $order_maps[] = $ordering->toDictionary(); $properties = array(); + foreach ($all_tasks as $task) { + $properties[$task->getPHID()] = array( + 'points' => (double)$task->getPoints(), + 'status' => $task->getStatus(), + ); + } $behavior_config = array( 'moveURI' => $this->getApplicationURI('move/'.$project->getID().'/'), @@ -771,7 +777,7 @@ final class PhabricatorProjectBoardViewController } private function isValidSort($sort) { - $map = PhabricatorProjectColumnOrder::getAllOrders(); + $map = PhabricatorProjectColumnOrder::getEnabledOrders(); return isset($map[$sort]); } @@ -820,6 +826,9 @@ final class PhabricatorProjectBoardViewController $project, PhabricatorPolicyCapability::CAN_EDIT); + $items[] = id(new PhabricatorActionView()) + ->setType(PhabricatorActionView::TYPE_DIVIDER); + $items[] = id(new PhabricatorActionView()) ->setIcon('fa-floppy-o') ->setName(pht('Save as Default')) @@ -918,6 +927,9 @@ final class PhabricatorProjectBoardViewController $project, PhabricatorPolicyCapability::CAN_EDIT); + $items[] = id(new PhabricatorActionView()) + ->setType(PhabricatorActionView::TYPE_DIVIDER); + $items[] = id(new PhabricatorActionView()) ->setIcon('fa-floppy-o') ->setName(pht('Save as Default')) diff --git a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php index 945445aecf..9fd2145886 100644 --- a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php @@ -21,10 +21,14 @@ final class PhabricatorProjectColumnCreatedOrder return false; } + public function getMenuOrder() { + return 3000; + } + protected function newSortVectorForObject($object) { return array( - (int)-$object->getDateCreated(), - (int)-$object->getID(), + -(int)$object->getDateCreated(), + -(int)$object->getID(), ); } diff --git a/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php index b21a554715..be67d28bcc 100644 --- a/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnNaturalOrder.php @@ -17,4 +17,8 @@ final class PhabricatorProjectColumnNaturalOrder return true; } + public function getMenuOrder() { + return 0; + } + } diff --git a/src/applications/project/order/PhabricatorProjectColumnOrder.php b/src/applications/project/order/PhabricatorProjectColumnOrder.php index f3a1ed86ca..c2da400fb2 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOrder.php @@ -22,9 +22,22 @@ abstract class PhabricatorProjectColumnOrder return id(new PhutilClassMapQuery()) ->setAncestorClass(__CLASS__) ->setUniqueMethod('getColumnOrderKey') + ->setSortMethod('getMenuOrder') ->execute(); } + final public static function getEnabledOrders() { + $map = self::getAllOrders(); + + foreach ($map as $key => $order) { + if (!$order->isEnabled()) { + unset($map[$key]); + } + } + + return $map; + } + final public static function getOrderByKey($key) { $map = self::getAllOrders(); @@ -71,6 +84,14 @@ abstract class PhabricatorProjectColumnOrder abstract public function getHasHeaders(); abstract public function getCanReorder(); + public function getMenuOrder() { + return 9000; + } + + public function isEnabled() { + return true; + } + protected function newColumnTransactions($object, array $header) { return array(); } diff --git a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php index a41b78a11c..97ae0f24d4 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php @@ -21,6 +21,10 @@ final class PhabricatorProjectColumnOwnerOrder return true; } + public function getMenuOrder() { + return 2000; + } + protected function newHeaderKeyForObject($object) { return $this->newHeaderKeyForOwnerPHID($object->getOwnerPHID()); } diff --git a/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php new file mode 100644 index 0000000000..ad75f6135b --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php @@ -0,0 +1,50 @@ +getPoints(); + + // Put cards with no points on top. + $has_points = ($points !== null); + if (!$has_points) { + $overall_order = 0; + } else { + $overall_order = 1; + } + + return array( + $overall_order, + -(double)$points, + -(int)$object->getID(), + ); + } + +} diff --git a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php index 64a5934e26..1a73145148 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php @@ -21,6 +21,10 @@ final class PhabricatorProjectColumnPriorityOrder return true; } + public function getMenuOrder() { + return 1000; + } + protected function newHeaderKeyForObject($object) { return $this->newHeaderKeyForPriority($object->getPriority()); } @@ -35,7 +39,7 @@ final class PhabricatorProjectColumnPriorityOrder private function newSortVectorForPriority($priority) { return array( - (int)-$priority, + -(int)$priority, ); } From a400d82932a6c730df9a97f1e0b69a450f021c19 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Mar 2019 10:43:29 -0700 Subject: [PATCH 19/25] Add "Group by Status" to Workboards Summary: Depends on D20276. Ref T10333. This one is a little bit rough/experimental, and I'm sort of curious what feedback we get about it. Weird stuff: - All statuses are always shown, even if the filter prevents tasks in that status from appearing (which is the default, since views are "Open Tasks" by default). - Pro: you can close tasks by dragging them to a closed status. - Con: lots of empty groups. - The "Duplicate" status is shown. - Pro: Shows closed duplicate tasks. - Con: Dragging tasks to "Duplicate" works, but is silly. - Since boards show "open tasks" by default, dragging stuff to a closed status and then reloading the board causes it to vanish. This is kind of how everything works, but more obvious/defaulted on "Status". These issues might overwhelm its usefulness, but there isn't much cost to nuking it in the future if feedback is mostly negative/confused. Test Plan: Grouped a workboard by status, dragged stuff around. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20277 --- src/__phutil_library_map__.php | 2 + .../PhabricatorProjectColumnStatusOrder.php | 106 ++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 src/applications/project/order/PhabricatorProjectColumnStatusOrder.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e3b6af7974..8f776a2948 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4065,6 +4065,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnPriorityOrder' => 'applications/project/order/PhabricatorProjectColumnPriorityOrder.php', 'PhabricatorProjectColumnQuery' => 'applications/project/query/PhabricatorProjectColumnQuery.php', 'PhabricatorProjectColumnSearchEngine' => 'applications/project/query/PhabricatorProjectColumnSearchEngine.php', + 'PhabricatorProjectColumnStatusOrder' => 'applications/project/order/PhabricatorProjectColumnStatusOrder.php', 'PhabricatorProjectColumnTransaction' => 'applications/project/storage/PhabricatorProjectColumnTransaction.php', 'PhabricatorProjectColumnTransactionEditor' => 'applications/project/editor/PhabricatorProjectColumnTransactionEditor.php', 'PhabricatorProjectColumnTransactionQuery' => 'applications/project/query/PhabricatorProjectColumnTransactionQuery.php', @@ -10155,6 +10156,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnPriorityOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectColumnSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhabricatorProjectColumnStatusOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorProjectColumnTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorProjectColumnTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php new file mode 100644 index 0000000000..e9570bea05 --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php @@ -0,0 +1,106 @@ +newHeaderKeyForStatus($object->getStatus()); + } + + private function newHeaderKeyForStatus($status) { + return sprintf('status(%s)', $status); + } + + protected function newSortVectorsForObjects(array $objects) { + $status_sequence = $this->newStatusSequence(); + + $vectors = array(); + foreach ($objects as $object_key => $object) { + $vectors[$object_key] = array( + (int)idx($status_sequence, $object->getStatus(), 0), + ); + } + + return $vectors; + } + + private function newStatusSequence() { + $statuses = ManiphestTaskStatus::getTaskStatusMap(); + return array_combine( + array_keys($statuses), + range(1, count($statuses))); + } + + protected function newHeadersForObjects(array $objects) { + $headers = array(); + + $statuses = ManiphestTaskStatus::getTaskStatusMap(); + $sequence = $this->newStatusSequence(); + + foreach ($statuses as $status_key => $status_name) { + $header_key = $this->newHeaderKeyForStatus($status_key); + + $sort_vector = array( + (int)idx($sequence, $status_key, 0), + ); + + $status_icon = ManiphestTaskStatus::getStatusIcon($status_key); + $status_color = ManiphestTaskStatus::getStatusColor($status_key); + + $icon_view = id(new PHUIIconView()) + ->setIcon($status_icon, $status_color); + + $header = $this->newHeader() + ->setHeaderKey($header_key) + ->setSortVector($sort_vector) + ->setName($status_name) + ->setIcon($icon_view) + ->setEditProperties( + array( + 'value' => $status_key, + )); + + $headers[] = $header; + } + + return $headers; + } + + protected function newColumnTransactions($object, array $header) { + $new_status = idx($header, 'value'); + + if ($object->getStatus() === $new_status) { + return null; + } + + $xactions = array(); + $xactions[] = $this->newTransaction($object) + ->setTransactionType(ManiphestTaskStatusTransaction::TRANSACTIONTYPE) + ->setNewValue($new_status); + + return $xactions; + } + +} From 8d74492875094abba369ea6de4e61949bf04971f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Mar 2019 13:05:16 -0700 Subject: [PATCH 20/25] Make workboard sort-order inversions more clear by using "-1 * ..." instead of "(int)-(int)" Summary: Depends on D20279. See D20269. Agreed that explicit `-1` is probably more clear. Test Plan: Viewed boards in each sort/group order. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20281 --- .../project/order/PhabricatorProjectColumnCreatedOrder.php | 4 ++-- .../project/order/PhabricatorProjectColumnPointsOrder.php | 4 ++-- .../project/order/PhabricatorProjectColumnPriorityOrder.php | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php index 9fd2145886..5652a96731 100644 --- a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php @@ -27,8 +27,8 @@ final class PhabricatorProjectColumnCreatedOrder protected function newSortVectorForObject($object) { return array( - -(int)$object->getDateCreated(), - -(int)$object->getID(), + -1 * (int)$object->getDateCreated(), + -1 * (int)$object->getID(), ); } diff --git a/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php index ad75f6135b..3cf758cbc8 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php @@ -42,8 +42,8 @@ final class PhabricatorProjectColumnPointsOrder return array( $overall_order, - -(double)$points, - -(int)$object->getID(), + -1.0 * (double)$points, + -1 * (int)$object->getID(), ); } diff --git a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php index 1a73145148..10fcafad76 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPriorityOrder.php @@ -39,7 +39,7 @@ final class PhabricatorProjectColumnPriorityOrder private function newSortVectorForPriority($priority) { return array( - -(int)$priority, + -1 * (int)$priority, ); } From a6e17fb702d1de3b6a38ea951bd711f72232f0c2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Mar 2019 16:01:54 -0700 Subject: [PATCH 21/25] Improve workboard "Owner" grouping, add "Author" grouping and "Title" sort Summary: Depends on D20277. Ref T10333. - Put profile icons on "Group by Owner". - Add a similar "Group by Author". Probably not terribly useful, but cheap to implement now. - Add "Sort by Title". Very likely not terribly useful, but cheap to implement and sort of flexible? Test Plan: {F6265396} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T10333 Differential Revision: https://secure.phabricator.com/D20278 --- resources/celerity/map.php | 10 +- src/__phutil_library_map__.php | 4 + .../PhabricatorProjectColumnAuthorOrder.php | 139 ++++++++++++++++++ .../PhabricatorProjectColumnCreatedOrder.php | 2 +- .../order/PhabricatorProjectColumnHeader.php | 7 +- .../PhabricatorProjectColumnOwnerOrder.php | 12 +- .../PhabricatorProjectColumnPointsOrder.php | 2 +- .../PhabricatorProjectColumnStatusOrder.php | 2 +- .../PhabricatorProjectColumnTitleOrder.php | 34 +++++ .../css/phui/workboards/phui-workpanel.css | 24 ++- 10 files changed, 223 insertions(+), 13 deletions(-) create mode 100644 src/applications/project/order/PhabricatorProjectColumnAuthorOrder.php create mode 100644 src/applications/project/order/PhabricatorProjectColumnTitleOrder.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 014839c14a..b80ce31d1a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -178,7 +178,7 @@ return array( 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', 'rsrc/css/phui/workboards/phui-workboard.css' => '74fc9d98', 'rsrc/css/phui/workboards/phui-workcard.css' => '9e9eb0df', - 'rsrc/css/phui/workboards/phui-workpanel.css' => 'bc16cf33', + 'rsrc/css/phui/workboards/phui-workpanel.css' => 'c5b408ad', 'rsrc/css/sprite-login.css' => '18b368a6', 'rsrc/css/sprite-tokens.css' => 'f1896dc5', 'rsrc/css/syntax/syntax-default.css' => '055fc231', @@ -860,7 +860,7 @@ return array( 'phui-workboard-color-css' => 'e86de308', 'phui-workboard-view-css' => '74fc9d98', 'phui-workcard-view-css' => '9e9eb0df', - 'phui-workpanel-view-css' => 'bc16cf33', + 'phui-workpanel-view-css' => 'c5b408ad', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', 'phuix-autocomplete' => '8f139ef0', @@ -1906,9 +1906,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'bc16cf33' => array( - 'phui-workcard-view-css', - ), 'bdce4d78' => array( 'javelin-install', 'javelin-util', @@ -1939,6 +1936,9 @@ return array( 'phabricator-phtize', 'javelin-dom', ), + 'c5b408ad' => array( + 'phui-workcard-view-css', + ), 'c687e867' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8f776a2948..e07a23d733 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4050,6 +4050,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColorTransaction' => 'applications/project/xaction/PhabricatorProjectColorTransaction.php', 'PhabricatorProjectColorsConfigType' => 'applications/project/config/PhabricatorProjectColorsConfigType.php', 'PhabricatorProjectColumn' => 'applications/project/storage/PhabricatorProjectColumn.php', + 'PhabricatorProjectColumnAuthorOrder' => 'applications/project/order/PhabricatorProjectColumnAuthorOrder.php', 'PhabricatorProjectColumnCreatedOrder' => 'applications/project/order/PhabricatorProjectColumnCreatedOrder.php', 'PhabricatorProjectColumnDetailController' => 'applications/project/controller/PhabricatorProjectColumnDetailController.php', 'PhabricatorProjectColumnEditController' => 'applications/project/controller/PhabricatorProjectColumnEditController.php', @@ -4066,6 +4067,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnQuery' => 'applications/project/query/PhabricatorProjectColumnQuery.php', 'PhabricatorProjectColumnSearchEngine' => 'applications/project/query/PhabricatorProjectColumnSearchEngine.php', 'PhabricatorProjectColumnStatusOrder' => 'applications/project/order/PhabricatorProjectColumnStatusOrder.php', + 'PhabricatorProjectColumnTitleOrder' => 'applications/project/order/PhabricatorProjectColumnTitleOrder.php', 'PhabricatorProjectColumnTransaction' => 'applications/project/storage/PhabricatorProjectColumnTransaction.php', 'PhabricatorProjectColumnTransactionEditor' => 'applications/project/editor/PhabricatorProjectColumnTransactionEditor.php', 'PhabricatorProjectColumnTransactionQuery' => 'applications/project/query/PhabricatorProjectColumnTransactionQuery.php', @@ -10138,6 +10140,7 @@ phutil_register_library_map(array( 'PhabricatorExtendedPolicyInterface', 'PhabricatorConduitResultInterface', ), + 'PhabricatorProjectColumnAuthorOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnCreatedOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnDetailController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnEditController' => 'PhabricatorProjectBoardController', @@ -10157,6 +10160,7 @@ phutil_register_library_map(array( 'PhabricatorProjectColumnQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectColumnSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectColumnStatusOrder' => 'PhabricatorProjectColumnOrder', + 'PhabricatorProjectColumnTitleOrder' => 'PhabricatorProjectColumnOrder', 'PhabricatorProjectColumnTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorProjectColumnTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorProjectColumnTransactionQuery' => 'PhabricatorApplicationTransactionQuery', diff --git a/src/applications/project/order/PhabricatorProjectColumnAuthorOrder.php b/src/applications/project/order/PhabricatorProjectColumnAuthorOrder.php new file mode 100644 index 0000000000..9d6bac2aff --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnAuthorOrder.php @@ -0,0 +1,139 @@ +newHeaderKeyForAuthorPHID($object->getAuthorPHID()); + } + + private function newHeaderKeyForAuthorPHID($author_phid) { + return sprintf('author(%s)', $author_phid); + } + + protected function newSortVectorsForObjects(array $objects) { + $author_phids = mpull($objects, null, 'getAuthorPHID'); + $author_phids = array_keys($author_phids); + $author_phids = array_filter($author_phids); + + if ($author_phids) { + $author_users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($author_phids) + ->execute(); + $author_users = mpull($author_users, null, 'getPHID'); + } else { + $author_users = array(); + } + + $vectors = array(); + foreach ($objects as $vector_key => $object) { + $author_phid = $object->getAuthorPHID(); + $author = idx($author_users, $author_phid); + if ($author) { + $vector = $this->newSortVectorForAuthor($author); + } else { + $vector = $this->newSortVectorForAuthorPHID($author_phid); + } + + $vectors[$vector_key] = $vector; + } + + return $vectors; + } + + private function newSortVectorForAuthor(PhabricatorUser $user) { + return array( + 1, + $user->getUsername(), + ); + } + + private function newSortVectorForAuthorPHID($author_phid) { + return array( + 2, + $author_phid, + ); + } + + protected function newHeadersForObjects(array $objects) { + $author_phids = mpull($objects, null, 'getAuthorPHID'); + $author_phids = array_keys($author_phids); + $author_phids = array_filter($author_phids); + + if ($author_phids) { + $author_users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($author_phids) + ->needProfileImage(true) + ->execute(); + $author_users = mpull($author_users, null, 'getPHID'); + } else { + $author_users = array(); + } + + $headers = array(); + foreach ($author_phids as $author_phid) { + $header_key = $this->newHeaderKeyForAuthorPHID($author_phid); + + $author = idx($author_users, $author_phid); + if ($author) { + $sort_vector = $this->newSortVectorForAuthor($author); + $author_name = $author->getUsername(); + $author_image = $author->getProfileImageURI(); + } else { + $sort_vector = $this->newSortVectorForAuthorPHID($author_phid); + $author_name = pht('Unknown User ("%s")', $author_phid); + $author_image = null; + } + + $author_icon = 'fa-user'; + $author_color = 'bluegrey'; + + $icon_view = id(new PHUIIconView()); + + if ($author_image) { + $icon_view->setImage($author_image); + } else { + $icon_view->setIcon($author_icon, $author_color); + } + + $header = $this->newHeader() + ->setHeaderKey($header_key) + ->setSortVector($sort_vector) + ->setName($author_name) + ->setIcon($icon_view) + ->setEditProperties( + array( + 'value' => $author_phid, + )); + + $headers[] = $header; + } + + return $headers; + } + +} diff --git a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php index 5652a96731..05f25a3d6a 100644 --- a/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnCreatedOrder.php @@ -22,7 +22,7 @@ final class PhabricatorProjectColumnCreatedOrder } public function getMenuOrder() { - return 3000; + return 5000; } protected function newSortVectorForObject($object) { diff --git a/src/applications/project/order/PhabricatorProjectColumnHeader.php b/src/applications/project/order/PhabricatorProjectColumnHeader.php index 432b78279b..24d1e5c5ec 100644 --- a/src/applications/project/order/PhabricatorProjectColumnHeader.php +++ b/src/applications/project/order/PhabricatorProjectColumnHeader.php @@ -85,7 +85,12 @@ final class PhabricatorProjectColumnHeader ), array( $icon_view, - $name, + phutil_tag( + 'span', + array( + 'class' => 'workboard-group-header-name', + ), + $name), )); return $template; diff --git a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php index 97ae0f24d4..336411bac5 100644 --- a/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnOwnerOrder.php @@ -108,6 +108,7 @@ final class PhabricatorProjectColumnOwnerOrder $owner_users = id(new PhabricatorPeopleQuery()) ->setViewer($this->getViewer()) ->withPHIDs($owner_phids) + ->needProfileImage(true) ->execute(); $owner_users = mpull($owner_users, null, 'getPHID'); } else { @@ -120,6 +121,7 @@ final class PhabricatorProjectColumnOwnerOrder foreach ($owner_phids as $owner_phid) { $header_key = $this->newHeaderKeyForOwnerPHID($owner_phid); + $owner_image = null; if ($owner_phid === null) { $owner = null; $sort_vector = $this->newSortVectorForUnowned(); @@ -129,6 +131,7 @@ final class PhabricatorProjectColumnOwnerOrder if ($owner) { $sort_vector = $this->newSortVectorForOwner($owner); $owner_name = $owner->getUsername(); + $owner_image = $owner->getProfileImageURI(); } else { $sort_vector = $this->newSortVectorForOwnerPHID($owner_phid); $owner_name = pht('Unknown User ("%s")', $owner_phid); @@ -138,8 +141,13 @@ final class PhabricatorProjectColumnOwnerOrder $owner_icon = 'fa-user'; $owner_color = 'bluegrey'; - $icon_view = id(new PHUIIconView()) - ->setIcon($owner_icon, $owner_color); + $icon_view = id(new PHUIIconView()); + + if ($owner_image) { + $icon_view->setImage($owner_image); + } else { + $icon_view->setIcon($owner_icon, $owner_color); + } $header = $this->newHeader() ->setHeaderKey($header_key) diff --git a/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php index 3cf758cbc8..2e9be8e4bb 100644 --- a/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnPointsOrder.php @@ -26,7 +26,7 @@ final class PhabricatorProjectColumnPointsOrder } public function getMenuOrder() { - return 4000; + return 6000; } protected function newSortVectorForObject($object) { diff --git a/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php index e9570bea05..e58d05f655 100644 --- a/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php +++ b/src/applications/project/order/PhabricatorProjectColumnStatusOrder.php @@ -22,7 +22,7 @@ final class PhabricatorProjectColumnStatusOrder } public function getMenuOrder() { - return 3000; + return 4000; } protected function newHeaderKeyForObject($object) { diff --git a/src/applications/project/order/PhabricatorProjectColumnTitleOrder.php b/src/applications/project/order/PhabricatorProjectColumnTitleOrder.php new file mode 100644 index 0000000000..a281c75437 --- /dev/null +++ b/src/applications/project/order/PhabricatorProjectColumnTitleOrder.php @@ -0,0 +1,34 @@ +getTitle(), + ); + } + +} diff --git a/webroot/rsrc/css/phui/workboards/phui-workpanel.css b/webroot/rsrc/css/phui/workboards/phui-workpanel.css index 2dac6b2233..95db8021ef 100644 --- a/webroot/rsrc/css/phui/workboards/phui-workpanel.css +++ b/webroot/rsrc/css/phui/workboards/phui-workpanel.css @@ -148,13 +148,33 @@ .workboard-group-header { background: rgba({$alphablue}, 0.10); - padding: 4px 8px; + padding: 6px 8px; margin: 0 0 8px -8px; border-bottom: 1px solid {$lightgreyborder}; font-weight: bold; color: {$darkgreytext}; + position: relative; } .workboard-group-header .phui-icon-view { - margin-right: 8px; + position: absolute; + display: inline-block; + width: 24px; + padding: 5px 0 0 0; + height: 19px; + background-size: 100%; + border-radius: 3px; + background-repeat: no-repeat; + text-align: center; + background-color: {$lightgreybackground}; + border: 1px solid {$lightgreybackground}; +} + +.workboard-group-header .workboard-group-header-name { + display: block; + position: relative; + height: 24px; + line-height: 24px; + margin-left: 36px; + overflow: hidden; } From 04f9e72cbd101b278a9f9374600e10deadc0eafa Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Mar 2019 11:41:22 -0700 Subject: [PATCH 22/25] Don't subscribe bots implicitly when they act on objects, or when they are mentioned Summary: See PHI1098. When users comment on objects, they are automatically subscribed. And when `@alice` mentions `@bailey` on a task, that usually subscribes `@bailey`. These rules make less sense if the user is a bot. There's generally no reason for a bot to automatically subscribe to objects it acts on (it's not going to read email and follow up later), and it can always subscribe itself pretty easily if it wants (since everything is `*.edit` now and supports subscribe transactions). Also, don't subscribe bots when they're mentioned for similar reasons. If users really want to subscribe bots, they can do so explicitly. These rules seem slightly like "bad implicit magic" since it's not immediately obvious why `@abc` subscribes that user but `@xyz` may not, but some of these rules are fairly complicated already (e.g., `@xyz` doesn't subscribe them if they unsubscribed or are implicitly subscribed) and this feels like it gets the right/desired result almost-always. Test Plan: On a fresh task: - Mentioned a bot in a comment with `@bot`. - Before patch: bot got CC'd. - After patch: no CC. - Called `maniphest.edit` via the API to add a comment as a bot. - Before patch: bot got CC'd. - After patch: no CC. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20284 --- ...habricatorApplicationTransactionEditor.php | 67 +++++++++++++++---- 1 file changed, 53 insertions(+), 14 deletions(-) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 3a46784a33..24bff2bc57 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1816,31 +1816,52 @@ abstract class PhabricatorApplicationTransactionEditor $users = mpull($users, null, 'getPHID'); foreach ($phids as $key => $phid) { - // Do not subscribe mentioned users - // who do not have VIEW Permissions - if ($object instanceof PhabricatorPolicyInterface - && !PhabricatorPolicyFilter::hasCapability( - $users[$phid], - $object, - PhabricatorPolicyCapability::CAN_VIEW) - ) { + $user = idx($users, $phid); + + // Don't subscribe invalid users. + if (!$user) { unset($phids[$key]); - } else { - if ($object->isAutomaticallySubscribed($phid)) { + continue; + } + + // Don't subscribe bots that get mentioned. If users truly intend + // to subscribe them, they can add them explicitly, but it's generally + // not useful to subscribe bots to objects. + if ($user->getIsSystemAgent()) { + unset($phids[$key]); + continue; + } + + // Do not subscribe mentioned users who do not have permission to see + // the object. + if ($object instanceof PhabricatorPolicyInterface) { + $can_view = PhabricatorPolicyFilter::hasCapability( + $user, + $object, + PhabricatorPolicyCapability::CAN_VIEW); + if (!$can_view) { unset($phids[$key]); + continue; } } + + // Don't subscribe users who are already automatically subscribed. + if ($object->isAutomaticallySubscribed($phid)) { + unset($phids[$key]); + continue; + } } + $phids = array_values($phids); } - // No else here to properly return null should we unset all subscriber + if (!$phids) { return null; } - $xaction = newv(get_class(head($xactions)), array()); - $xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); - $xaction->setNewValue(array('+' => $phids)); + $xaction = $object->getApplicationTransactionTemplate() + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('+' => $phids)); return $xaction; } @@ -2876,6 +2897,24 @@ abstract class PhabricatorApplicationTransactionEditor } } + $actor = $this->getActor(); + + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($actor) + ->withPHIDs(array($actor_phid)) + ->executeOne(); + if (!$user) { + return $xactions; + } + + // When a bot acts (usually via the API), don't automatically subscribe + // them as a side effect. They can always subscribe explicitly if they + // want, and bot subscriptions normally just clutter things up since bots + // usually do not read email. + if ($user->getIsSystemAgent()) { + return $xactions; + } + $xaction = newv(get_class(head($xactions)), array()); $xaction->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS); $xaction->setNewValue(array('+' => array($actor_phid))); From df53d72e794c8f3eed9123b4b78d7b02ace77e8c Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Mar 2019 14:14:52 -0700 Subject: [PATCH 23/25] Allow "Move Tasks to Column..." to prompt for MFA Summary: Ref T13074. Currently, if you "Move Tasks to Column..." on a board and some of the tasks require MFA to edit, the workflow fatals out. After this change, it works properly. You still have to answer a separate MFA prompt for //each// task, which is a little ridiculous, but at least doable. A reasonable future refinement would be to batch these MFA prompts, but this is currently the only use case for that. Test Plan: Set a task to a "Require MFA" status, bulk-moved it with other tasks on a workboard. Was prompted, answered MFA prompt, got a move. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13074 Differential Revision: https://secure.phabricator.com/D20282 --- .../controller/PhabricatorProjectBoardViewController.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index cc95958bf7..a1dcd6ab68 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -328,7 +328,7 @@ final class PhabricatorProjectBoardViewController $columns = null; $errors = array(); - if ($request->isFormPost()) { + if ($request->isFormOrHiSecPost()) { $move_project_phid = head($request->getArr('moveProjectPHID')); if (!$move_project_phid) { $move_project_phid = $request->getStr('moveProjectPHID'); @@ -425,7 +425,8 @@ final class PhabricatorProjectBoardViewController ->setActor($viewer) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true) - ->setContentSourceFromRequest($request); + ->setContentSourceFromRequest($request) + ->setCancelURI($cancel_uri); $editor->applyTransactions($move_task, $xactions); } From 492b03628f19118d1828f1e68a693752a4ba665e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Mar 2019 14:47:14 -0700 Subject: [PATCH 24/25] Fix a typo in Drydock "Land" operations Summary: Misspelling. Test Plan: Careful reading. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D20290 --- .../drydock/operation/DrydockLandRepositoryOperation.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php index 1ccc82eb7b..acb48f6f0b 100644 --- a/src/applications/drydock/operation/DrydockLandRepositoryOperation.php +++ b/src/applications/drydock/operation/DrydockLandRepositoryOperation.php @@ -401,7 +401,7 @@ final class DrydockLandRepositoryOperation 'body' => pht( 'When this diff was generated, the server was running an older '. 'version of Phabricator which did not support staging areas for '. - 'this version control system, so the chagne was not pushed to '. + 'this version control system, so the change was not pushed to '. 'staging. Changes must be pushed to staging before they can be '. 'landed from the web.'), ); From b469a5134ddd4f6484ebdf9088126d56acf6d4b8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Mar 2019 06:37:00 -0700 Subject: [PATCH 25/25] Allow "SMTP" and "Sendmail" mailers to have "Message-ID" behavior configured in "cluster.mailers" Summary: Fixes T13265. See that task for discussion. Briefly: - For mailers that use other mailers (SMTP, Sendmail), optionally let administrators set `"message-id": false` to improve threading behavior if their local Postfix is ultimately sending through SES or some other mailer which will replace the "Message-ID" header. Also: - Postmark is currently marked as supporting "Message-ID", but it does not actually support "Message-ID" on `secure.phabricator.com` (mail arrives with a non-Phabricator message ID). I suspect this was just an oversight in building or refactoring the adapter; correct it. - Remove the "encoding" parameter from "sendmail". It think this was just missed in the cleanup a couple months ago; it is no longer used or documented. Test Plan: Added and ran unit tests. (These feel like overkill, but this is super hard to test on real code.) See T13265 for evidence that this overall approach improves behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13265 Differential Revision: https://secure.phabricator.com/D20285 --- src/__phutil_library_map__.php | 2 + .../adapter/PhabricatorMailAdapter.php | 33 +++++++ .../PhabricatorMailAmazonSESAdapter.php | 4 - .../PhabricatorMailPostmarkAdapter.php | 4 - .../adapter/PhabricatorMailSMTPAdapter.php | 6 +- .../PhabricatorMailSendmailAdapter.php | 9 +- .../PhabricatorMailAdapterTestCase.php | 96 +++++++++++++++++++ .../configuring_outbound_email.diviner | 57 ++++++++++- 8 files changed, 196 insertions(+), 15 deletions(-) create mode 100644 src/applications/metamta/adapter/__tests__/PhabricatorMailAdapterTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e07a23d733..82972d5a7d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3464,6 +3464,7 @@ phutil_register_library_map(array( 'PhabricatorMacroTransactionType' => 'applications/macro/xaction/PhabricatorMacroTransactionType.php', 'PhabricatorMacroViewController' => 'applications/macro/controller/PhabricatorMacroViewController.php', 'PhabricatorMailAdapter' => 'applications/metamta/adapter/PhabricatorMailAdapter.php', + 'PhabricatorMailAdapterTestCase' => 'applications/metamta/adapter/__tests__/PhabricatorMailAdapterTestCase.php', 'PhabricatorMailAmazonSESAdapter' => 'applications/metamta/adapter/PhabricatorMailAmazonSESAdapter.php', 'PhabricatorMailAmazonSNSAdapter' => 'applications/metamta/adapter/PhabricatorMailAmazonSNSAdapter.php', 'PhabricatorMailAttachment' => 'applications/metamta/message/PhabricatorMailAttachment.php', @@ -9425,6 +9426,7 @@ phutil_register_library_map(array( 'PhabricatorMacroTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorMacroViewController' => 'PhabricatorMacroController', 'PhabricatorMailAdapter' => 'Phobject', + 'PhabricatorMailAdapterTestCase' => 'PhabricatorTestCase', 'PhabricatorMailAmazonSESAdapter' => 'PhabricatorMailAdapter', 'PhabricatorMailAmazonSNSAdapter' => 'PhabricatorMailAdapter', 'PhabricatorMailAttachment' => 'Phobject', diff --git a/src/applications/metamta/adapter/PhabricatorMailAdapter.php b/src/applications/metamta/adapter/PhabricatorMailAdapter.php index 4fb262626d..8c1a6c0ba7 100644 --- a/src/applications/metamta/adapter/PhabricatorMailAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailAdapter.php @@ -137,4 +137,37 @@ abstract class PhabricatorMailAdapter abstract public function newDefaultOptions(); + final protected function guessIfHostSupportsMessageID($config, $host) { + // See T13265. Mailers like "SMTP" and "sendmail" usually allow us to + // set the "Message-ID" header to a value we choose, but we may not be + // able to if the mailer is being used as API glue and the outbound + // pathway ends up routing to a service with an SMTP API that selects + // its own "Message-ID" header, like Amazon SES. + + // If users configured a behavior explicitly, use that behavior. + if ($config !== null) { + return $config; + } + + // If the server we're connecting to is part of a service that we know + // does not support "Message-ID", guess that we don't support "Message-ID". + if ($host !== null) { + $host_blocklist = array( + '/\.amazonaws\.com\z/', + '/\.postmarkapp\.com\z/', + '/\.sendgrid\.net\z/', + ); + + $host = phutil_utf8_strtolower($host); + foreach ($host_blocklist as $regexp) { + if (preg_match($regexp, $host)) { + return false; + } + } + } + + return true; + } + + } diff --git a/src/applications/metamta/adapter/PhabricatorMailAmazonSESAdapter.php b/src/applications/metamta/adapter/PhabricatorMailAmazonSESAdapter.php index a289e5bc73..793cd56091 100644 --- a/src/applications/metamta/adapter/PhabricatorMailAmazonSESAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailAmazonSESAdapter.php @@ -11,10 +11,6 @@ final class PhabricatorMailAmazonSESAdapter ); } - public function supportsMessageIDHeader() { - return false; - } - protected function validateOptions(array $options) { PhutilTypeSpec::checkMap( $options, diff --git a/src/applications/metamta/adapter/PhabricatorMailPostmarkAdapter.php b/src/applications/metamta/adapter/PhabricatorMailPostmarkAdapter.php index d84d8f8bfa..2381ff04bf 100644 --- a/src/applications/metamta/adapter/PhabricatorMailPostmarkAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailPostmarkAdapter.php @@ -11,10 +11,6 @@ final class PhabricatorMailPostmarkAdapter ); } - public function supportsMessageIDHeader() { - return true; - } - protected function validateOptions(array $options) { PhutilTypeSpec::checkMap( $options, diff --git a/src/applications/metamta/adapter/PhabricatorMailSMTPAdapter.php b/src/applications/metamta/adapter/PhabricatorMailSMTPAdapter.php index a3c6298279..abbda40146 100644 --- a/src/applications/metamta/adapter/PhabricatorMailSMTPAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailSMTPAdapter.php @@ -12,7 +12,9 @@ final class PhabricatorMailSMTPAdapter } public function supportsMessageIDHeader() { - return true; + return $this->guessIfHostSupportsMessageID( + $this->getOption('message-id'), + $this->getOption('host')); } protected function validateOptions(array $options) { @@ -24,6 +26,7 @@ final class PhabricatorMailSMTPAdapter 'user' => 'string|null', 'password' => 'string|null', 'protocol' => 'string|null', + 'message-id' => 'bool|null', )); } @@ -34,6 +37,7 @@ final class PhabricatorMailSMTPAdapter 'user' => null, 'password' => null, 'protocol' => null, + 'message-id' => null, ); } diff --git a/src/applications/metamta/adapter/PhabricatorMailSendmailAdapter.php b/src/applications/metamta/adapter/PhabricatorMailSendmailAdapter.php index 05f3c909aa..a60c0e5a4e 100644 --- a/src/applications/metamta/adapter/PhabricatorMailSendmailAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailSendmailAdapter.php @@ -5,7 +5,6 @@ final class PhabricatorMailSendmailAdapter const ADAPTERTYPE = 'sendmail'; - public function getSupportedMessageTypes() { return array( PhabricatorMailEmailMessage::MESSAGETYPE, @@ -13,20 +12,22 @@ final class PhabricatorMailSendmailAdapter } public function supportsMessageIDHeader() { - return true; + return $this->guessIfHostSupportsMessageID( + $this->getOption('message-id'), + null); } protected function validateOptions(array $options) { PhutilTypeSpec::checkMap( $options, array( - 'encoding' => 'string', + 'message-id' => 'bool|null', )); } public function newDefaultOptions() { return array( - 'encoding' => 'base64', + 'message-id' => null, ); } diff --git a/src/applications/metamta/adapter/__tests__/PhabricatorMailAdapterTestCase.php b/src/applications/metamta/adapter/__tests__/PhabricatorMailAdapterTestCase.php new file mode 100644 index 0000000000..9c194f24c2 --- /dev/null +++ b/src/applications/metamta/adapter/__tests__/PhabricatorMailAdapterTestCase.php @@ -0,0 +1,96 @@ + 'test', + 'secret-key' => 'test', + 'endpoint' => 'test', + ), + ), + + array( + pht('Mailgun'), + true, + new PhabricatorMailMailgunAdapter(), + array( + 'api-key' => 'test', + 'domain' => 'test', + 'api-hostname' => 'test', + ), + ), + + array( + pht('Sendmail'), + true, + new PhabricatorMailSendmailAdapter(), + array(), + ), + + array( + pht('Sendmail (Explicit Config)'), + false, + new PhabricatorMailSendmailAdapter(), + array( + 'message-id' => false, + ), + ), + + array( + pht('SMTP (Local)'), + true, + new PhabricatorMailSMTPAdapter(), + array(), + ), + + array( + pht('SMTP (Local + Explicit)'), + false, + new PhabricatorMailSMTPAdapter(), + array( + 'message-id' => false, + ), + ), + + array( + pht('SMTP (AWS)'), + false, + new PhabricatorMailSMTPAdapter(), + array( + 'host' => 'test.amazonaws.com', + ), + ), + + array( + pht('SMTP (AWS + Explicit)'), + true, + new PhabricatorMailSMTPAdapter(), + array( + 'host' => 'test.amazonaws.com', + 'message-id' => true, + ), + ), + + ); + + foreach ($cases as $case) { + list($label, $expect, $mailer, $options) = $case; + + $defaults = $mailer->newDefaultOptions(); + $mailer->setOptions($options + $defaults); + + $actual = $mailer->supportsMessageIDHeader(); + + $this->assertEqual($expect, $actual, pht('Message-ID: %s', $label)); + } + } + + +} diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner index b77d761f80..884e4e7fdb 100644 --- a/src/docs/user/configuration/configuring_outbound_email.diviner +++ b/src/docs/user/configuration/configuring_outbound_email.diviner @@ -339,9 +339,11 @@ document. If you can already send outbound email from the command line or know how to configure it, this option is straightforward. If you have no idea how to do any of this, strongly consider using Postmark or Mailgun instead. -To use this mailer, set `type` to `sendmail`. There are no `options` to -configure. +To use this mailer, set `type` to `sendmail`, then configure these `options`: + - `message-id`: Optional bool. Set to `false` if Phabricator will not be + able to select a custom "Message-ID" header when sending mail via this + mailer. See "Message-ID Headers" below. Mailer: SMTP ============ @@ -361,6 +363,9 @@ To use this mailer, set `type` to `smtp`, then configure these `options`: - `password`: Optional string. Password for authentication. - `protocol`: Optional string. Set to `tls` or `ssl` if necessary. Use `ssl` for Gmail. + - `message-id`: Optional bool. Set to `false` if Phabricator will not be + able to select a custom "Message-ID" header when sending mail via this + mailer. See "Message-ID Headers" below. Disable Mail @@ -446,6 +451,54 @@ in any priority group, in the configured order. In this example there is only one such server, so it will try to send via Mailgun. +Message-ID Headers +================== + +Email has a "Message-ID" header which is important for threading messages +correctly in mail clients. Normally, Phabricator is free to select its own +"Message-ID" header values for mail it sends. + +However, some mailers (including Amazon SES) do not allow selection of custom +"Message-ID" values and will ignore or replace the "Message-ID" in mail that +is submitted through them. + +When Phabricator adds other mail headers which affect threading, like +"In-Reply-To", it needs to know if its "Message-ID" headers will be respected +or not to select header values which will produce good threading behavior. If +we guess wrong and think we can set a "Message-ID" header when we can't, you +may get poor threading behavior in mail clients. + +For most mailers (like Postmark, Mailgun, and Amazon SES), the correct setting +will be selected for you automatically, because the behavior of the mailer +is knowable ahead of time. For example, we know Amazon SES will never respect +our "Message-ID" headers. + +However, if you're sending mail indirectly through a mailer like SMTP or +Sendmail, the mail might or might not be routing through some mail service +which will ignore or replace the "Message-ID" header. + +For example, your local mailer might submit mail to Mailgun (so "Message-ID" +will work), or to Amazon SES (so "Message-ID" will not work), or to some other +mail service (which we may not know anything about). We can't make a reliable +guess about whether "Message-ID" will be respected or not based only on +the local mailer configuration. + +By default, we check if the mailer has a hostname we recognize as belonging +to a service which does not allow us to set a "Message-ID" header. If we don't +recognize the hostname (which is very common, since these services are most +often configured against the localhost or some other local machine), we assume +we can set a "Message-ID" header. + +If the outbound pathway does not actually allow selection of a "Message-ID" +header, you can set the `message-id` option on the mailer to `false` to tell +Phabricator that it should not assume it can select a value for this header. + +For example, if you are sending mail via a local Postfix server which then +forwards the mail to Amazon SES (a service which does not allow selection of +a "Message-ID" header), your `smtp` configuration in Phabricator should +specify `"message-id": false`. + + Next Steps ==========