Page MenuHomePhabricator
Files F14395157

D20321.diff
No OneTemporary
Actions

D20321.diff
View Options

diff --git a/resources/celerity/map.php b/resources/celerity/map.php
--- a/resources/celerity/map.php
+++ b/resources/celerity/map.php
@@ -409,7 +409,7 @@
'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' => '223af34e',
+ 'rsrc/js/application/projects/WorkboardBoard.js' => '106d870f',
'rsrc/js/application/projects/WorkboardCard.js' => '0392a5d8',
'rsrc/js/application/projects/WorkboardCardTemplate.js' => '2a61f8d4',
'rsrc/js/application/projects/WorkboardColumn.js' => 'c3d24e63',
@@ -737,7 +737,7 @@
'javelin-view-renderer' => '9aae2b66',
'javelin-view-visitor' => '308f9fe4',
'javelin-websocket' => 'fdc13e4e',
- 'javelin-workboard-board' => '223af34e',
+ 'javelin-workboard-board' => '106d870f',
'javelin-workboard-card' => '0392a5d8',
'javelin-workboard-card-template' => '2a61f8d4',
'javelin-workboard-column' => 'c3d24e63',
@@ -1015,6 +1015,18 @@
'javelin-workflow',
'phuix-icon-view',
),
+ '106d870f' => 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',
+ ),
'111bfd2d' => array(
'javelin-install',
),
@@ -1073,18 +1085,6 @@
'javelin-behavior',
'javelin-request',
),
- '223af34e' => 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',
- ),
'225bbb98' => array(
'javelin-install',
'javelin-reactor',
diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php
--- a/src/applications/maniphest/editor/ManiphestEditEngine.php
+++ b/src/applications/maniphest/editor/ManiphestEditEngine.php
@@ -123,22 +123,23 @@
column.
The target column should be identified as `columnPHID`, and you may select a
-position by passing either `beforePHID` or `afterPHID`, specifying the PHID of
-a task currently in the column that you want to move this task before or after:
+position by passing either `beforePHIDs` or `afterPHIDs`, specifying the PHIDs
+of tasks currently in the column that you want to move this task before or
+after:
```lang=json
[
{
"columnPHID": "PHID-PCOL-4444",
- "beforePHID": "PHID-TASK-5555"
+ "beforePHIDs": ["PHID-TASK-5555"]
}
]
```
-Note that this affects only the "natural" position of the task. The task
-position when the board is sorted by some other attribute (like priority)
-depends on that attribute value: change a task's priority to move it on
-priority-sorted boards.
+When you specify multiple PHIDs, the task will be moved adjacent to the first
+valid PHID found in either of the lists. This allows positional moves to
+generally work as users expect even if the client view of the board has fallen
+out of date and some of the nearby tasks have moved elsewhere.
EODOCS
);
diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
--- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php
@@ -428,6 +428,7 @@
private function buildMoveTransaction(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
+ $actor = $this->getActor();
$new = $xaction->getNewValue();
if (!is_array($new)) {
@@ -435,7 +436,7 @@
$new = array($new);
}
- $nearby_phids = array();
+ $relative_phids = array();
foreach ($new as $key => $value) {
if (!is_array($value)) {
$this->validateColumnPHID($value);
@@ -448,35 +449,83 @@
$value,
array(
'columnPHID' => 'string',
+ 'beforePHIDs' => 'optional list<string>',
+ 'afterPHIDs' => 'optional list<string>',
+
+ // Deprecated older variations of "beforePHIDs" and "afterPHIDs".
'beforePHID' => 'optional string',
'afterPHID' => 'optional string',
));
- $new[$key] = $value;
+ $value = $value + array(
+ 'beforePHIDs' => array(),
+ 'afterPHIDs' => array(),
+ );
+
+ // Normalize the legacy keys "beforePHID" and "afterPHID" keys to the
+ // modern format.
+ if (!empty($value['afterPHID'])) {
+ if ($value['afterPHIDs']) {
+ throw new Exception(
+ pht(
+ 'Transaction specifies both "afterPHID" and "afterPHIDs". '.
+ 'Specify only "afterPHIDs".'));
+ }
+ $value['afterPHIDs'] = array($value['afterPHID']);
+ unset($value['afterPHID']);
+ }
+
+ if (isset($value['beforePHID'])) {
+ if ($value['beforePHIDs']) {
+ throw new Exception(
+ pht(
+ 'Transaction specifies both "beforePHID" and "beforePHIDs". '.
+ 'Specify only "beforePHIDs".'));
+ }
+ $value['beforePHIDs'] = array($value['beforePHID']);
+ unset($value['beforePHID']);
+ }
- if (!empty($value['beforePHID'])) {
- $nearby_phids[] = $value['beforePHID'];
+ foreach ($value['beforePHIDs'] as $phid) {
+ $relative_phids[] = $phid;
}
- if (!empty($value['afterPHID'])) {
- $nearby_phids[] = $value['afterPHID'];
+ foreach ($value['afterPHIDs'] as $phid) {
+ $relative_phids[] = $phid;
}
+
+ $new[$key] = $value;
}
- if ($nearby_phids) {
- $nearby_objects = id(new PhabricatorObjectQuery())
- ->setViewer($this->getActor())
- ->withPHIDs($nearby_phids)
+ // We require that objects you specify in "beforePHIDs" or "afterPHIDs"
+ // are real objects which exist and which you have permission to view.
+ // If you provide other objects, we remove them from the specification.
+
+ if ($relative_phids) {
+ $objects = id(new PhabricatorObjectQuery())
+ ->setViewer($actor)
+ ->withPHIDs($relative_phids)
->execute();
- $nearby_objects = mpull($nearby_objects, null, 'getPHID');
+ $objects = mpull($objects, null, 'getPHID');
} else {
- $nearby_objects = array();
+ $objects = array();
+ }
+
+ foreach ($new as $key => $value) {
+ $value['afterPHIDs'] = $this->filterValidPHIDs(
+ $value['afterPHIDs'],
+ $objects);
+ $value['beforePHIDs'] = $this->filterValidPHIDs(
+ $value['beforePHIDs'],
+ $objects);
+
+ $new[$key] = $value;
}
$column_phids = ipull($new, 'columnPHID');
if ($column_phids) {
$columns = id(new PhabricatorProjectColumnQuery())
- ->setViewer($this->getActor())
+ ->setViewer($actor)
->withPHIDs($column_phids)
->execute();
$columns = mpull($columns, null, 'getPHID');
@@ -487,10 +536,9 @@
$board_phids = mpull($columns, 'getProjectPHID');
$object_phid = $object->getPHID();
- $object_phids = $nearby_phids;
-
// Note that we may not have an object PHID if we're creating a new
// object.
+ $object_phids = array();
if ($object_phid) {
$object_phids[] = $object_phid;
}
@@ -517,49 +565,6 @@
$board_phid = $column->getProjectPHID();
- $nearby = array();
-
- if (!empty($spec['beforePHID'])) {
- $nearby['beforePHID'] = $spec['beforePHID'];
- }
-
- if (!empty($spec['afterPHID'])) {
- $nearby['afterPHID'] = $spec['afterPHID'];
- }
-
- if (count($nearby) > 1) {
- throw new Exception(
- pht(
- 'Column move transaction moves object to multiple positions. '.
- 'Specify only "beforePHID" or "afterPHID", not both.'));
- }
-
- foreach ($nearby as $where => $nearby_phid) {
- if (empty($nearby_objects[$nearby_phid])) {
- throw new Exception(
- pht(
- 'Column move transaction specifies object "%s" as "%s", but '.
- 'there is no corresponding object with this PHID.',
- $object_phid,
- $where));
- }
-
- $nearby_columns = $layout_engine->getObjectColumns(
- $board_phid,
- $nearby_phid);
- $nearby_columns = mpull($nearby_columns, null, 'getPHID');
-
- if (empty($nearby_columns[$column_phid])) {
- throw new Exception(
- pht(
- 'Column move transaction specifies object "%s" as "%s" in '.
- 'column "%s", but this object is not in that column!',
- $nearby_phid,
- $where,
- $column_phid));
- }
- }
-
if ($object_phid) {
$old_columns = $layout_engine->getObjectColumns(
$board_phid,
@@ -578,8 +583,8 @@
// We can just drop this column change if it has no effect.
$from_map = array_fuse($spec['fromColumnPHIDs']);
$already_here = isset($from_map[$column_phid]);
- $is_reordering = (bool)$nearby;
+ $is_reordering = ($spec['afterPHIDs'] || $spec['beforePHIDs']);
if ($already_here && !$is_reordering) {
unset($new[$key]);
} else {
@@ -677,8 +682,9 @@
private function applyBoardMove($object, array $move) {
$board_phid = $move['boardPHID'];
$column_phid = $move['columnPHID'];
- $before_phid = idx($move, 'beforePHID');
- $after_phid = idx($move, 'afterPHID');
+
+ $before_phids = $move['beforePHIDs'];
+ $after_phids = $move['afterPHIDs'];
$object_phid = $object->getPHID();
@@ -730,24 +736,12 @@
$object_phid);
}
- if ($before_phid) {
- $engine->queueAddPositionBefore(
- $board_phid,
- $column_phid,
- $object_phid,
- $before_phid);
- } else if ($after_phid) {
- $engine->queueAddPositionAfter(
- $board_phid,
- $column_phid,
- $object_phid,
- $after_phid);
- } else {
- $engine->queueAddPosition(
- $board_phid,
- $column_phid,
- $object_phid);
- }
+ $engine->queueAddPosition(
+ $board_phid,
+ $column_phid,
+ $object_phid,
+ $after_phids,
+ $before_phids);
$engine->applyPositionUpdates();
}
@@ -849,4 +843,16 @@
return $errors;
}
+ private function filterValidPHIDs($phid_list, array $object_map) {
+ foreach ($phid_list as $key => $phid) {
+ if (isset($object_map[$phid])) {
+ continue;
+ }
+
+ unset($phid_list[$key]);
+ }
+
+ return array_values($phid_list);
+ }
+
}
diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
--- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
+++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
@@ -1008,29 +1008,32 @@
$task2->getPHID(),
$task1->getPHID(),
);
- $this->assertTasksInColumn($expect, $user, $board, $column);
+ $label = pht('Simple move');
+ $this->assertTasksInColumn($expect, $user, $board, $column, $label);
// Move the second task after the first task.
$options = array(
- 'afterPHID' => $task1->getPHID(),
+ 'afterPHIDs' => array($task1->getPHID()),
);
$this->moveToColumn($user, $board, $task2, $column, $column, $options);
$expect = array(
$task1->getPHID(),
$task2->getPHID(),
);
- $this->assertTasksInColumn($expect, $user, $board, $column);
+ $label = pht('With afterPHIDs');
+ $this->assertTasksInColumn($expect, $user, $board, $column, $label);
// Move the second task before the first task.
$options = array(
- 'beforePHID' => $task1->getPHID(),
+ 'beforePHIDs' => array($task1->getPHID()),
);
$this->moveToColumn($user, $board, $task2, $column, $column, $options);
$expect = array(
$task2->getPHID(),
$task1->getPHID(),
);
- $this->assertTasksInColumn($expect, $user, $board, $column);
+ $label = pht('With beforePHIDs');
+ $this->assertTasksInColumn($expect, $user, $board, $column, $label);
}
public function testMilestoneMoves() {
@@ -1333,7 +1336,8 @@
array $expect,
PhabricatorUser $viewer,
PhabricatorProject $board,
- PhabricatorProjectColumn $column) {
+ PhabricatorProjectColumn $column,
+ $label = null) {
$engine = id(new PhabricatorBoardLayoutEngine())
->setViewer($viewer)
@@ -1346,7 +1350,7 @@
$column->getPHID());
$object_phids = array_values($object_phids);
- $this->assertEqual($expect, $object_phids);
+ $this->assertEqual($expect, $object_phids, $label);
}
private function addColumn(
diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php
--- a/src/applications/project/controller/PhabricatorProjectMoveController.php
+++ b/src/applications/project/controller/PhabricatorProjectMoveController.php
@@ -11,9 +11,20 @@
$column_phid = $request->getStr('columnPHID');
$object_phid = $request->getStr('objectPHID');
+
$after_phid = $request->getStr('afterPHID');
$before_phid = $request->getStr('beforePHID');
+ $after_phids = array();
+ if ($after_phid) {
+ $after_phids[] = $after_phid;
+ }
+
+ $before_phids = array();
+ if ($before_phid) {
+ $before_phids[] = $before_phid;
+ }
+
$order = $request->getStr('order');
if (!strlen($order)) {
$order = PhabricatorProjectColumnNaturalOrder::ORDERKEY;
@@ -89,9 +100,10 @@
$order_params = array();
if ($after_phid) {
- $order_params['afterPHID'] = $after_phid;
- } else if ($before_phid) {
- $order_params['beforePHID'] = $before_phid;
+ $order_params['afterPHIDs'] = $after_phids;
+ }
+ if ($before_phid) {
+ $order_params['beforePHIDs'] = $before_phids;
}
$xactions = array();
diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php
--- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php
+++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php
@@ -135,52 +135,12 @@
return $this;
}
- public function queueAddPositionBefore(
- $board_phid,
- $column_phid,
- $object_phid,
- $before_phid) {
-
- return $this->queueAddPositionRelative(
- $board_phid,
- $column_phid,
- $object_phid,
- $before_phid,
- true);
- }
-
- public function queueAddPositionAfter(
- $board_phid,
- $column_phid,
- $object_phid,
- $after_phid) {
-
- return $this->queueAddPositionRelative(
- $board_phid,
- $column_phid,
- $object_phid,
- $after_phid,
- false);
- }
-
public function queueAddPosition(
- $board_phid,
- $column_phid,
- $object_phid) {
- return $this->queueAddPositionRelative(
- $board_phid,
- $column_phid,
- $object_phid,
- null,
- true);
- }
-
- private function queueAddPositionRelative(
$board_phid,
$column_phid,
$object_phid,
- $relative_phid,
- $is_before) {
+ array $after_phids,
+ array $before_phids) {
$board_layout = idx($this->boardLayout, $board_phid, array());
$positions = idx($board_layout, $column_phid, array());
@@ -196,54 +156,76 @@
->setObjectPHID($object_phid);
}
- $found = false;
if (!$positions) {
$object_position->setSequence(0);
} else {
- foreach ($positions as $position) {
- if (!$found) {
- if ($relative_phid === null) {
- $is_match = true;
- } else {
- $position_phid = $position->getObjectPHID();
- $is_match = ($relative_phid == $position_phid);
- }
-
- if ($is_match) {
- $found = true;
+ // The user's view of the board may fall out of date, so they might
+ // try to drop a card under a different card which is no longer where
+ // they thought it was.
+
+ // When this happens, we perform the move anyway, since this is almost
+ // certainly what users want when interacting with the UI. We'l try to
+ // fall back to another nearby card if the client provided us one. If
+ // we don't find any of the cards the client specified in the column,
+ // we'll just move the card to the default position.
+
+ $search_phids = array();
+ foreach ($after_phids as $after_phid) {
+ $search_phids[] = array($after_phid, false);
+ }
- $sequence = $position->getSequence();
+ foreach ($before_phids as $before_phid) {
+ $search_phids[] = array($before_phid, true);
+ }
- if (!$is_before) {
- $sequence++;
+ // This makes us fall back to the default position if we fail every
+ // candidate position. The default position counts as a "before" position
+ // because we want to put the new card at the top of the column.
+ $search_phids[] = array(null, true);
+
+ $found = false;
+ foreach ($search_phids as $search_position) {
+ list($relative_phid, $is_before) = $search_position;
+ foreach ($positions as $position) {
+ if (!$found) {
+ if ($relative_phid === null) {
+ $is_match = true;
+ } else {
+ $position_phid = $position->getObjectPHID();
+ $is_match = ($relative_phid === $position_phid);
}
- $object_position->setSequence($sequence++);
+ if ($is_match) {
+ $found = true;
- if (!$is_before) {
- // If we're inserting after this position, continue the loop so
- // we don't update it.
- continue;
+ $sequence = $position->getSequence();
+
+ if (!$is_before) {
+ $sequence++;
+ }
+
+ $object_position->setSequence($sequence++);
+
+ if (!$is_before) {
+ // If we're inserting after this position, continue the loop so
+ // we don't update it.
+ continue;
+ }
}
}
+
+ if ($found) {
+ $position->setSequence($sequence++);
+ $this->addQueue[] = $position;
+ }
}
if ($found) {
- $position->setSequence($sequence++);
- $this->addQueue[] = $position;
+ break;
}
}
}
- if ($relative_phid && !$found) {
- throw new Exception(
- pht(
- 'Unable to find object "%s" in column "%s" on board "%s".',
- $relative_phid,
- $column_phid,
- $board_phid));
- }
-
$this->addQueue[] = $object_position;
$positions[$object_phid] = $object_position;
diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js
--- a/webroot/rsrc/js/application/projects/WorkboardBoard.js
+++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js
@@ -495,7 +495,7 @@
order: this.getOrder()
};
- var context = this._getDropContext(after_node);
+ var context = this._getDropContext(after_node, item);
if (context.afterPHID) {
data.afterPHID = context.afterPHID;

File Metadata

Mime Type
text/plain
Expires
Mon, Dec 23, 3:14 AM (17 h, 47 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6920422
Default Alt Text
D20321.diff (19 KB)

Event Timeline

Phabricator · Built by Phacility