From d81d0c3ea099589e4717a2be8cfa794670d80568 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Jul 2019 11:00:27 -0700 Subject: [PATCH] Fix an issue where editing cards on a workboard with implicit column ordering could reorder cards improperly Summary: Depends on D20680. Ref T4900. The "BoardLayoutEngine" operates on PHIDs without knowledge of the underlying objects, but this means it has to be sensitive to PHID input order when falling back to a default layout order. We use "default layout order" on workboards which are sorted by "Natual" order but which have one or more cards which no user has ever reordered. For example, if you add 10 tasks to a project, then create a board, there's no existing order for those tasks in the "Backlog" column. The layout engine uses the input order to place them in the column, with the expectation that input order is ID/creation order, so new cards will end up on top. I think this code never really made an explicit effort to guarantee that the LayoutEngine received objects in ID order, and it just sort of happened to by coincidence and good fortune. Some recent change has disrupted this, so the edit operation can end up with the PHIDs arranged in arbitrary order. Explicitly put them in ID order so we always get an implicit default layout order to fall back to. Also, update to `msortv()`. Test Plan: - Tagged several tasks with project X, a project without a board yet. - Created the project X workboard. - (Did not drag any tasks around on the project X board!) - Viewed the board in "Natural" order. This creates a view of the board where tasks are ordered by implicit/virtual/input order. The expectation, and "view" behavior of this board, is that this order is "newest on top". - Edited one of the cards on the board, changing the title (don't reorder it!) - Before: page state synchronized with cards in arbitrary/random/different order. - After: page state synchronized with cards in the same order as before ("newest on top"). Reviewers: amckinley Maniphest Tasks: T4900 Differential Revision: https://secure.phabricator.com/D20681 --- .../project/engine/PhabricatorBoardLayoutEngine.php | 6 +++--- .../project/engine/PhabricatorBoardResponseEngine.php | 9 ++++++++- .../storage/PhabricatorProjectColumnPosition.php | 11 +++++------ 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index e614ec2f94..83fb943b76 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php @@ -229,7 +229,7 @@ final class PhabricatorBoardLayoutEngine extends Phobject { $this->addQueue[] = $object_position; $positions[$object_phid] = $object_position; - $positions = msort($positions, 'getOrderingKey'); + $positions = msortv($positions, 'newColumnPositionOrderVector'); $this->boardLayout[$board_phid][$column_phid] = $positions; @@ -404,7 +404,7 @@ final class PhabricatorBoardLayoutEngine extends Phobject { ->withBoardPHIDs(array_keys($boards)) ->withObjectPHIDs($object_phids) ->execute(); - $positions = msort($positions, 'getOrderingKey'); + $positions = msortv($positions, 'newColumnPositionOrderVector'); $positions = mgroup($positions, 'getBoardPHID'); return $positions; @@ -581,7 +581,7 @@ final class PhabricatorBoardLayoutEngine extends Phobject { } foreach ($layout as $column_phid => $map) { - $map = msort($map, 'getOrderingKey'); + $map = msortv($map, 'newColumnPositionOrderVector'); $layout[$column_phid] = $map; foreach ($map as $object_phid => $position) { diff --git a/src/applications/project/engine/PhabricatorBoardResponseEngine.php b/src/applications/project/engine/PhabricatorBoardResponseEngine.php index 96a6d8c457..81e56d2116 100644 --- a/src/applications/project/engine/PhabricatorBoardResponseEngine.php +++ b/src/applications/project/engine/PhabricatorBoardResponseEngine.php @@ -100,10 +100,17 @@ final class PhabricatorBoardResponseEngine extends Phobject { $all_objects = mpull($all_objects, null, 'getPHID'); } + // NOTE: The board layout engine is sensitive to PHID input order, and uses + // the input order as a component of the "natural" column ordering if no + // explicit ordering is specified. Rearrange the PHIDs in ID order. + + $all_objects = msort($all_objects, 'getID'); + $ordered_phids = mpull($all_objects, 'getPHID'); + $layout_engine = id(new PhabricatorBoardLayoutEngine()) ->setViewer($viewer) ->setBoardPHIDs(array($board_phid)) - ->setObjectPHIDs($all_phids) + ->setObjectPHIDs($ordered_phids) ->executeLayout(); $natural = array(); diff --git a/src/applications/project/storage/PhabricatorProjectColumnPosition.php b/src/applications/project/storage/PhabricatorProjectColumnPosition.php index 0bd9be6d4a..1a094dec37 100644 --- a/src/applications/project/storage/PhabricatorProjectColumnPosition.php +++ b/src/applications/project/storage/PhabricatorProjectColumnPosition.php @@ -46,7 +46,7 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO return $this; } - public function getOrderingKey() { + public function newColumnPositionOrderVector() { // We're ordering both real positions and "virtual" positions which we have // created but not saved yet. @@ -61,11 +61,10 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO // Broadly, this collectively makes newly added stuff float to the top. - return sprintf( - '~%012d%012d%012d', - $this->getSequence(), - ((1 << 31) - $this->viewSequence), - ((1 << 31) - $this->getID())); + return id(new PhutilSortVector()) + ->addInt($this->getSequence()) + ->addInt(-1 * $this->viewSequence) + ->addInt(-1 * $this->getID()); } /* -( PhabricatorPolicyInterface )----------------------------------------- */ -- 2.51.2