revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 16:22:43 +01:00
Code Issues Releases Wiki Activity

When moving cards on workboards, treat before/after cards as optional hints, not strict requirements

Browse source 
Summary:
Depends on D20320. Ref T12175. Ref T13074. Currently, when you move a card between columns, the internal transaction takes exactly one `afterPHID` or `beforePHID` and moves the card before or after the specified card.

This is a fairly strict interpretation and causes a number of practical issues, mostly because the user/client view of the board may be out of date and the card they're dragging before or after may no longer exist: another user might have moved or hidden it between the last client update and the current time.

In T13074, we also run into a more subtle issue where a card that incorrectly appears in multiple columns fatals when dropped before or after itself.

In all cases, a better behavior is just to complete the move and accept that the position may not end up exactly like the user specified. We could prompt the user instead:

> You tried to drop this card after card X, but that card has moved since you last loaded the board. Reload the board and try again.

...but this is pretty hostile and probably rarely/never what the user wants.

Instead, accept a list of before/after PHIDs and just try them until we find one that works, or accept a default position if none work. In essentially all cases, this means that the move "just works" like users expect it to instead of fataling in a confusing/disruptive/undesirable (but "technically correct") way.

(A followup will make the client JS send more beforePHIDs/afterPHIDs so this works more often.)

We could eventually add a "strict" mode in the API or something if there's some bot/API use case for precise behavior here, but I suspect none exist today or are (ever?) likely to exist in the future.

Test Plan:
  - (T13074) Inserted two conflicting rows to put a card on two columns on the same board. Dropped one version of it underneath the other version. Before: confusing fatal. After: cards merge sensibly into one consistent card.
  - (T12175) Opened two views of a board. Moved card A to a different column on the first view. On the second view, dropped card B under card A (still showing in the old column). Before: confusing fatal. After: card ended up in the right column in approximately the right place, very reasonably.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13074, T12175

Differential Revision: https://secure.phabricator.com/D20321
This commit is contained in:
epriestley 2019-03-25 10:12:10 -07:00
parent 6182193cf5
commit 6138e50962
7 changed files with 194 additions and 189 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
resources/celerity/map.php
View file

@ -409,7 +409,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' => '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 @@ return array(
'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 @@ return array(
'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 @@ return array(
'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',

15
src/applications/maniphest/editor/ManiphestEditEngine.php
View file

@ -123,22 +123,23 @@ information about the move, including an optional specific position within the
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
);

166
src/applications/maniphest/editor/ManiphestTransactionEditor.php
View file

@ -428,6 +428,7 @@ final class ManiphestTransactionEditor
private function buildMoveTransaction(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
$actor = $this->getActor();
$new = $xaction->getNewValue();
if (!is_array($new)) {
@ -435,7 +436,7 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
$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;
if (!empty($value['beforePHID'])) {
$nearby_phids[] = $value['beforePHID'];
}
$value = $value + array(
'beforePHIDs' => array(),
'afterPHIDs' => array(),
);
// Normalize the legacy keys "beforePHID" and "afterPHID" keys to the
// modern format.
if (!empty($value['afterPHID'])) {
$nearby_phids[] = $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']);
}
foreach ($value['beforePHIDs'] as $phid) {
$relative_phids[] = $phid;
}
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 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
// 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 @@ final class ManiphestTransactionEditor
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 @@ final class ManiphestTransactionEditor
$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 @@ final class ManiphestTransactionEditor
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);
}
}

18
src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php
View file

@ -1008,29 +1008,32 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
$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 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
array $expect,
PhabricatorUser $viewer,
PhabricatorProject $board,
PhabricatorProjectColumn $column) {
PhabricatorProjectColumn $column,
$label = null) {
$engine = id(new PhabricatorBoardLayoutEngine())
->setViewer($viewer)
@ -1346,7 +1350,7 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase {
$column->getPHID());
$object_phids = array_values($object_phids);
$this->assertEqual($expect, $object_phids);
$this->assertEqual($expect, $object_phids, $label);
}
private function addColumn(

18
src/applications/project/controller/PhabricatorProjectMoveController.php
View file

@ -11,9 +11,20 @@ final class PhabricatorProjectMoveController
$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 @@ final class PhabricatorProjectMoveController
$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();

136
src/applications/project/engine/PhabricatorBoardLayoutEngine.php
View file

@ -135,52 +135,12 @@ final class PhabricatorBoardLayoutEngine extends Phobject {
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 @@ final class PhabricatorBoardLayoutEngine extends Phobject {
->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);
// 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);
}
foreach ($before_phids as $before_phid) {
$search_phids[] = array($before_phid, true);
}
// 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);
}
if ($is_match) {
$found = true;
$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 ($is_match) {
$found = true;
$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;

2
webroot/rsrc/js/application/projects/WorkboardBoard.js
View file

@ -495,7 +495,7 @@ JX.install('WorkboardBoard', {
order: this.getOrder()
};
var context = this._getDropContext(after_node);
var context = this._getDropContext(after_node, item);
if (context.afterPHID) {
data.afterPHID = context.afterPHID;