Skip to content

Commit 59a85e8

Browse files
author
epriestley
committed
Support natural ordering of workboards
Summary: Ref T4807. This is probably a complete fix, but I'd be surprised if there isn't a little cleanup I missed. When users drag tasks on a "natural"-ordered workboard, leave things where they put them. This isn't //too// bad since a lot of the existing work is completely reusable (e.g., we don't need any new JS). Test Plan: - Dragged a bunch of stuff around, it stayed where I put it after dropped and when reloaded. - Dragged stuff across priorities, no zany priority changes (in "natural" mode). - Created new tasks, they show up at the top. - Tagged new tasks, they show up at the top of backlog. - Swapped to "priority" mode and got sorting and the old priority-altering reordering. - Added tasks in priority mode. - Viewed task transactions for correctness/sanity. Reviewers: btrahan, chad Reviewed By: chad Subscribers: chad, epriestley Maniphest Tasks: T4807 Differential Revision: https://secure.phabricator.com/D10182
1 parent 043e0db commit 59a85e8

File tree

7 files changed

+202
-14
lines changed

7 files changed

+202
-14
lines changed

‎src/applications/maniphest/controller/ManiphestTaskEditController.php‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -387,10 +387,23 @@ public function processRequest() {
387387
->withPHIDs($task_phids)
388388
->execute();
389389

390-
$sort_map = mpull(
391-
$column_tasks,
392-
'getPrioritySortVector',
393-
'getPHID');
390+
if ($order == PhabricatorProjectColumn::ORDER_NATURAL) {
391+
// TODO: This is a little bit awkward, because PHP and JS use
392+
// slightly different sort order parameters to achieve the same
393+
// effect. It would be unify this a bit at some point.
394+
$sort_map = array();
395+
foreach ($positions as $position) {
396+
$sort_map[$position->getObjectPHID()] = array(
397+
-$position->getSequence(),
398+
$position->getID(),
399+
);
400+
}
401+
} else {
402+
$sort_map = mpull(
403+
$column_tasks,
404+
'getPrioritySortVector',
405+
'getPHID');
406+
}
394407

395408
$data = array(
396409
'sortMap' => $sort_map,

‎src/applications/maniphest/editor/ManiphestTransactionEditor.php‎

Lines changed: 117 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -190,34 +190,144 @@ protected function applyCustomExternalTransaction(
190190
pht("Expected 'projectPHID' in column transaction."));
191191
}
192192

193+
$old_phids = idx($xaction->getOldValue(), 'columnPHIDs', array());
193194
$new_phids = idx($xaction->getNewValue(), 'columnPHIDs', array());
194195
if (count($new_phids) !== 1) {
195196
throw new Exception(
196197
pht("Expected exactly one 'columnPHIDs' in column transaction."));
197198
}
198199

200+
$columns = id(new PhabricatorProjectColumnQuery())
201+
->setViewer($this->requireActor())
202+
->withPHIDs($new_phids)
203+
->execute();
204+
$columns = mpull($columns, null, 'getPHID');
205+
199206
$positions = id(new PhabricatorProjectColumnPositionQuery())
200207
->setViewer($this->requireActor())
201208
->withObjectPHIDs(array($object->getPHID()))
202209
->withBoardPHIDs(array($board_phid))
203210
->execute();
204211

212+
$before_phid = idx($xaction->getNewValue(), 'beforePHID');
213+
$after_phid = idx($xaction->getNewValue(), 'afterPHID');
214+
215+
if (!$before_phid && !$after_phid && ($old_phids == $new_phids)) {
216+
// If we are not moving the object between columns and also not
217+
// reordering the position, this is a move on some other order
218+
// (like priority). We can leave the positions untouched and just
219+
// bail, there's no work to be done.
220+
return;
221+
}
222+
223+
// Otherwise, we're either moving between columns or adjusting the
224+
// object's position in the "natural" ordering, so we do need to update
225+
// some rows.
226+
205227
// Remove all existing column positions on the board.
206228

207229
foreach ($positions as $position) {
208230
$position->delete();
209231
}
210232

211-
// Add the new column position.
233+
// Add the new column positions.
212234

213235
foreach ($new_phids as $phid) {
214-
id(new PhabricatorProjectColumnPosition())
236+
$column = idx($columns, $phid);
237+
if (!$column) {
238+
throw new Exception(
239+
pht('No such column "%s" exists!', $phid));
240+
}
241+
242+
// Load the other object positions in the column. Note that we must
243+
// skip implicit column creation to avoid generating a new position
244+
// if the target column is a backlog column.
245+
246+
$other_positions = id(new PhabricatorProjectColumnPositionQuery())
247+
->setViewer($this->requireActor())
248+
->withColumns(array($column))
249+
->withBoardPHIDs(array($board_phid))
250+
->setSkipImplicitCreate(true)
251+
->execute();
252+
$other_positions = msort($other_positions, 'getOrderingKey');
253+
254+
// Set up the new position object. We're going to figure out the
255+
// right sequence number and then persist this object with that
256+
// sequence number.
257+
$new_position = id(new PhabricatorProjectColumnPosition())
215258
->setBoardPHID($board_phid)
216-
->setColumnPHID($phid)
217-
->setObjectPHID($object->getPHID())
218-
// TODO: Do real sequence stuff.
219-
->setSequence(0)
220-
->save();
259+
->setColumnPHID($column->getPHID())
260+
->setObjectPHID($object->getPHID());
261+
262+
$updates = array();
263+
$sequence = 0;
264+
265+
// If we're just dropping this into the column without any specific
266+
// position information, put it at the top.
267+
if (!$before_phid && !$after_phid) {
268+
$new_position->setSequence($sequence)->save();
269+
$sequence++;
270+
}
271+
272+
foreach ($other_positions as $position) {
273+
$object_phid = $position->getObjectPHID();
274+
275+
// If this is the object we're moving before and we haven't
276+
// saved yet, insert here.
277+
if (($before_phid == $object_phid) && !$new_position->getID()) {
278+
$new_position->setSequence($sequence)->save();
279+
$sequence++;
280+
}
281+
282+
// This object goes here in the sequence; we might need to update
283+
// the row.
284+
if ($sequence != $position->getSequence()) {
285+
$updates[$position->getID()] = $sequence;
286+
}
287+
$sequence++;
288+
289+
// If this is the object we're moving after and we haven't saved
290+
// yet, insert here.
291+
if (($after_phid == $object_phid) && !$new_position->getID()) {
292+
$new_position->setSequence($sequence)->save();
293+
$sequence++;
294+
}
295+
}
296+
297+
// We should have found a place to put it.
298+
if (!$new_position->getID()) {
299+
throw new Exception(
300+
pht('Unable to find a place to insert object on column!'));
301+
}
302+
303+
// If we changed other objects' column positions, bulk reorder them.
304+
305+
if ($updates) {
306+
$position = new PhabricatorProjectColumnPosition();
307+
$conn_w = $position->establishConnection('w');
308+
309+
$pairs = array();
310+
foreach ($updates as $id => $sequence) {
311+
// This is ugly because MySQL gets upset with us if it is
312+
// configured strictly and we attempt inserts which can't work.
313+
// We'll never actually do these inserts since they'll always
314+
// collide (triggering the ON DUPLICATE KEY logic), so we just
315+
// provide dummy values in order to get there.
316+
317+
$pairs[] = qsprintf(
318+
$conn_w,
319+
'(%d, %d, "", "", "")',
320+
$id,
321+
$sequence);
322+
}
323+
324+
queryfx(
325+
$conn_w,
326+
'INSERT INTO %T (id, sequence, boardPHID, columnPHID, objectPHID)
327+
VALUES %Q ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)',
328+
$position->getTableName(),
329+
implode(', ', $pairs));
330+
}
221331
}
222332
break;
223333
default:

‎src/applications/maniphest/storage/ManiphestTransaction.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,16 @@ public function shouldHide() {
125125
break;
126126
case self::TYPE_SUBPRIORITY:
127127
return true;
128+
case self::TYPE_PROJECT_COLUMN:
129+
$old_cols = idx($this->getOldValue(), 'columnPHIDs');
130+
$new_cols = idx($this->getNewValue(), 'columnPHIDs');
131+
132+
$old_cols = array_values($old_cols);
133+
$new_cols = array_values($new_cols);
134+
sort($old_cols);
135+
sort($new_cols);
136+
137+
return ($old_cols === $new_cols);
128138
}
129139

130140
return parent::shouldHide();

‎src/applications/project/controller/PhabricatorProjectBoardViewController.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,23 @@ public function processRequest() {
178178
$task_map[$column_phid][] = $task_phid;
179179
}
180180

181+
// If we're showing the board in "natural" order, sort columns by their
182+
// column positions.
183+
if ($this->sortKey == PhabricatorProjectColumn::ORDER_NATURAL) {
184+
foreach ($task_map as $column_phid => $task_phids) {
185+
$order = array();
186+
foreach ($task_phids as $task_phid) {
187+
if (isset($positions[$task_phid])) {
188+
$order[$task_phid] = $positions[$task_phid]->getOrderingKey();
189+
} else {
190+
$order[$task_phid] = 0;
191+
}
192+
}
193+
asort($order);
194+
$task_map[$column_phid] = array_keys($order);
195+
}
196+
}
197+
181198
$task_can_edit_map = id(new PhabricatorPolicyFilter())
182199
->setViewer($viewer)
183200
->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT))

‎src/applications/project/controller/PhabricatorProjectMoveController.php‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ public function processRequest() {
1717
$object_phid = $request->getStr('objectPHID');
1818
$after_phid = $request->getStr('afterPHID');
1919
$before_phid = $request->getStr('beforePHID');
20+
$order = $request->getStr('order', PhabricatorProjectColumn::DEFAULT_ORDER);
21+
2022

2123
$project = id(new PhabricatorProjectQuery())
2224
->setViewer($viewer)
@@ -65,13 +67,22 @@ public function processRequest() {
6567

6668
$xactions = array();
6769

70+
if ($order == PhabricatorProjectColumn::ORDER_NATURAL) {
71+
$order_params = array(
72+
'afterPHID' => $after_phid,
73+
'beforePHID' => $before_phid,
74+
);
75+
} else {
76+
$order_params = array();
77+
}
78+
6879
$xactions[] = id(new ManiphestTransaction())
6980
->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN)
7081
->setNewValue(
7182
array(
7283
'columnPHIDs' => array($column->getPHID()),
7384
'projectPHID' => $column->getProjectPHID(),
74-
))
85+
) + $order_params)
7586
->setOldValue(
7687
array(
7788
'columnPHIDs' => mpull($positions, 'getColumnPHID'),
@@ -86,7 +97,7 @@ public function processRequest() {
8697
$task_phids[] = $before_phid;
8798
}
8899

89-
if ($task_phids) {
100+
if ($task_phids && ($order == PhabricatorProjectColumn::ORDER_PRIORITY)) {
90101
$tasks = id(new ManiphestTaskQuery())
91102
->setViewer($viewer)
92103
->withPHIDs($task_phids)

‎src/applications/project/query/PhabricatorProjectColumnPositionQuery.php‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ final class PhabricatorProjectColumnPositionQuery
99
private $columns;
1010

1111
private $needColumns;
12+
private $skipImplicitCreate;
1213

1314
public function withIDs(array $ids) {
1415
$this->ids = $ids;
@@ -46,6 +47,21 @@ public function needColumns($need_columns) {
4647
return $this;
4748
}
4849

50+
51+
/**
52+
* Skip implicit creation of column positions which are implied but do not
53+
* yet exist.
54+
*
55+
* This is primarily useful internally.
56+
*
57+
* @param bool True to skip implicit creation of column positions.
58+
* @return this
59+
*/
60+
public function setSkipImplicitCreate($skip) {
61+
$this->skipImplicitCreate = $skip;
62+
return $this;
63+
}
64+
4965
// NOTE: For now, boards are always attached to projects. However, they might
5066
// not be in the future. This generalization just anticipates a future where
5167
// we let other types of objects (like users) have boards, or let boards
@@ -93,7 +109,7 @@ protected function loadPage() {
93109
// column and put it in the default column.
94110

95111
$must_type_filter = false;
96-
if ($this->columns) {
112+
if ($this->columns && !$this->skipImplicitCreate) {
97113
$default_map = array();
98114
foreach ($this->columns as $column) {
99115
if ($column->isDefaultColumn()) {

‎src/applications/project/storage/PhabricatorProjectColumnPosition.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ public function attachColumn(PhabricatorProjectColumn $column) {
2525
return $this;
2626
}
2727

28+
public function getOrderingKey() {
29+
// Low sequence numbers go above high sequence numbers.
30+
// High position IDs go above low position IDs.
31+
// Broadly, this makes newly added stuff float to the top.
32+
33+
return sprintf(
34+
'~%012d%012d',
35+
$this->getSequence(),
36+
((1 << 31) - $this->getID()));
37+
}
38+
2839
/* -( PhabricatorPolicyInterface )----------------------------------------- */
2940

3041
public function getCapabilities() {

0 commit comments

Comments
 (0)