From 6138e50962e62316f270a7a25c272eb47a92160a Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 25 Mar 2019 10:12:10 -0700 Subject: [PATCH] When moving cards on workboards, treat before/after cards as optional hints, not strict requirements 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 --- resources/celerity/map.php | 28 +-- .../maniphest/editor/ManiphestEditEngine.php | 15 +- .../editor/ManiphestTransactionEditor.php | 166 +++++++++--------- .../PhabricatorProjectCoreTestCase.php | 18 +- .../PhabricatorProjectMoveController.php | 18 +- .../engine/PhabricatorBoardLayoutEngine.php | 136 +++++++------- .../js/application/projects/WorkboardBoard.js | 2 +- 7 files changed, 194 insertions(+), 189 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 586e568f64..972a5c2b56 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -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', diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php index 76c2276df0..2a8730d5c6 100644 --- a/src/applications/maniphest/editor/ManiphestEditEngine.php +++ b/src/applications/maniphest/editor/ManiphestEditEngine.php @@ -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 ); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 5cb7cf91ba..5198f44572 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -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', + 'afterPHIDs' => 'optional list', + + // 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); + } + } diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index e50c83ab5a..186ac7dea4 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -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( diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index 950b1e90cd..273a068c06 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -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(); diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index dcbe2d4ee4..e614ec2f94 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php @@ -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; diff --git a/webroot/rsrc/js/application/projects/WorkboardBoard.js b/webroot/rsrc/js/application/projects/WorkboardBoard.js index 0cd8abb7d2..7f1383ac64 100644 --- a/webroot/rsrc/js/application/projects/WorkboardBoard.js +++ b/webroot/rsrc/js/application/projects/WorkboardBoard.js @@ -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;