Commit 9a8019d4 authored by epriestley's avatar epriestley
Browse files

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
parent 7d849afd
......@@ -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',
......
......@@ -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',
......
......@@ -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 )----------------------------------- */
......
......@@ -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);
$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,
));
$order_key = $this->sortKey;
$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,
),
);
$ordering_map = PhabricatorProjectColumnOrder::getAllOrders();
$ordering = id(clone $ordering_map[$order_key])
->setViewer($viewer);
$headers = $ordering->getHeadersForObjects($all_tasks);
$headers = mpull($headers, 'toDictionary');
$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(
......
......@@ -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) {
......
......@@ -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);
}
}
......@@ -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();
$order_maps = array();
foreach ($all_visible as $visible) {
$order_maps[$visible->getPHID()] = $visible->getWorkboardOrderVectors();
$all_visible = mpull($all_visible, null, 'getPHID');
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