From a9e98e42f5ff65a3a71182ba3b30f502d0d40981 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Feb 2016 10:58:15 -0800 Subject: [PATCH] Continue lifting column layout logic out of ColumnPositionQuery Summary: Ref T10010. See D15174. This gets rid of the "actually apply the change" callsite and moves it to layout engine. Next up is to make the board view use the layout engine, then throw away all the whack code in ColumnPositionQuery, then move forward with D15171. Test Plan: - Dragged tasks within a column. - Dragged tasks between columns. - Dragged tasks to empty columns. - Created a task in a column. - Swapped board to priority sort, dragged a bunch of stuff all over. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10010 Differential Revision: https://secure.phabricator.com/D15175 --- .../editor/ManiphestTransactionEditor.php | 177 ++++++-------- .../engine/PhabricatorBoardLayoutEngine.php | 215 +++++++++++++++++- .../PhabricatorProjectColumnPosition.php | 2 +- 3 files changed, 280 insertions(+), 114 deletions(-) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 72979d3a4e..bc8b5d3d31 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -200,18 +200,6 @@ final class ManiphestTransactionEditor 'columnPHIDs')); } - $columns = id(new PhabricatorProjectColumnQuery()) - ->setViewer($this->requireActor()) - ->withPHIDs($new_phids) - ->execute(); - $columns = mpull($columns, null, 'getPHID'); - - $positions = id(new PhabricatorProjectColumnPositionQuery()) - ->setViewer($this->requireActor()) - ->withObjectPHIDs(array($object->getPHID())) - ->withBoardPHIDs(array($board_phid)) - ->execute(); - $before_phid = idx($xaction->getNewValue(), 'beforePHID'); $after_phid = idx($xaction->getNewValue(), 'afterPHID'); @@ -227,111 +215,76 @@ final class ManiphestTransactionEditor // object's position in the "natural" ordering, so we do need to update // some rows. + $object_phid = $object->getPHID(); + + // We're doing layout with the ominpotent viewer to make sure we don't + // remove positions in columns that exist, but which the actual actor + // can't see. + $omnipotent_viewer = PhabricatorUser::getOmnipotentUser(); + + $board_tasks = id(new ManiphestTaskQuery()) + ->setViewer($omnipotent_viewer) + ->withEdgeLogicPHIDs( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + PhabricatorQueryConstraint::OPERATOR_AND, + array($board_phid)) + ->execute(); + + $object_phids = mpull($board_tasks, 'getPHID'); + $object_phids[] = $object_phid; + + $engine = id(new PhabricatorBoardLayoutEngine()) + ->setViewer($omnipotent_viewer) + ->setBoardPHIDs(array($board_phid)) + ->setObjectPHIDs($object_phids) + ->executeLayout(); + + // TODO: This logic needs to be revised if we legitimately support + // multiple column positions. + + // NOTE: When a task is newly created, it's implicitly added to the + // backlog but we don't currently record that in the "$old_phids". Just + // clean it up for now. + $columns = $engine->getObjectColumns($board_phid, $object_phid); + foreach ($columns as $column) { + $engine->queueRemovePosition( + $board_phid, + $column->getPHID(), + $object_phid); + } + // Remove all existing column positions on the board. - - foreach ($positions as $position) { - $position->delete(); + foreach ($old_phids as $column_phid) { + $engine->queueRemovePosition( + $board_phid, + $column_phid, + $object_phid); } - // Add the new column positions. - - foreach ($new_phids as $phid) { - $column = idx($columns, $phid); - if (!$column) { - throw new Exception( - pht('No such column "%s" exists!', $phid)); - } - - // Load the other object positions in the column. Note that we must - // skip implicit column creation to avoid generating a new position - // if the target column is a backlog column. - - $other_positions = id(new PhabricatorProjectColumnPositionQuery()) - ->setViewer($this->requireActor()) - ->withColumns(array($column)) - ->withBoardPHIDs(array($board_phid)) - ->setSkipImplicitCreate(true) - ->execute(); - $other_positions = msort($other_positions, 'getOrderingKey'); - - // Set up the new position object. We're going to figure out the - // right sequence number and then persist this object with that - // sequence number. - $new_position = id(new PhabricatorProjectColumnPosition()) - ->setBoardPHID($board_phid) - ->setColumnPHID($column->getPHID()) - ->setObjectPHID($object->getPHID()); - - $updates = array(); - $sequence = 0; - - // If we're just dropping this into the column without any specific - // position information, put it at the top. - if (!$before_phid && !$after_phid) { - $new_position->setSequence($sequence)->save(); - $sequence++; - } - - foreach ($other_positions as $position) { - $object_phid = $position->getObjectPHID(); - - // If this is the object we're moving before and we haven't - // saved yet, insert here. - if (($before_phid == $object_phid) && !$new_position->getID()) { - $new_position->setSequence($sequence)->save(); - $sequence++; - } - - // This object goes here in the sequence; we might need to update - // the row. - if ($sequence != $position->getSequence()) { - $updates[$position->getID()] = $sequence; - } - $sequence++; - - // If this is the object we're moving after and we haven't saved - // yet, insert here. - if (($after_phid == $object_phid) && !$new_position->getID()) { - $new_position->setSequence($sequence)->save(); - $sequence++; - } - } - - // We should have found a place to put it. - if (!$new_position->getID()) { - throw new Exception( - pht('Unable to find a place to insert object on column!')); - } - - // If we changed other objects' column positions, bulk reorder them. - - if ($updates) { - $position = new PhabricatorProjectColumnPosition(); - $conn_w = $position->establishConnection('w'); - - $pairs = array(); - foreach ($updates as $id => $sequence) { - // This is ugly because MySQL gets upset with us if it is - // configured strictly and we attempt inserts which can't work. - // We'll never actually do these inserts since they'll always - // collide (triggering the ON DUPLICATE KEY logic), so we just - // provide dummy values in order to get there. - - $pairs[] = qsprintf( - $conn_w, - '(%d, %d, "", "", "")', - $id, - $sequence); - } - - queryfx( - $conn_w, - 'INSERT INTO %T (id, sequence, boardPHID, columnPHID, objectPHID) - VALUES %Q ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)', - $position->getTableName(), - implode(', ', $pairs)); + // Add new positions. + foreach ($new_phids as $column_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->applyPositionUpdates(); + break; default: break; diff --git a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php index b5b60c44e4..28cecd81ea 100644 --- a/src/applications/project/engine/PhabricatorBoardLayoutEngine.php +++ b/src/applications/project/engine/PhabricatorBoardLayoutEngine.php @@ -10,6 +10,9 @@ final class PhabricatorBoardLayoutEngine extends Phobject { private $objectColumnMap = array(); private $boardLayout = array(); + private $remQueue = array(); + private $addQueue = array(); + public function setViewer(PhabricatorUser $viewer) { $this->viewer = $viewer; return $this; @@ -76,6 +79,207 @@ final class PhabricatorBoardLayoutEngine extends Phobject { return array_select_keys($this->columnMap, $column_phids); } + public function queueRemovePosition( + $board_phid, + $column_phid, + $object_phid) { + + $board_layout = idx($this->boardLayout, $board_phid, array()); + $positions = idx($board_layout, $column_phid, array()); + $position = idx($positions, $object_phid); + + if ($position) { + $this->remQueue[] = $position; + + // If this position hasn't been saved yet, get it out of the add queue. + if (!$position->getID()) { + foreach ($this->addQueue as $key => $add_position) { + if ($add_position === $position) { + unset($this->addQueue[$key]); + } + } + } + } + + unset($this->boardLayout[$board_phid][$column_phid][$object_phid]); + + 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) { + + $board_layout = idx($this->boardLayout, $board_phid, array()); + $positions = idx($board_layout, $column_phid, array()); + + // Check if the object is already in the column, and remove it if it is. + $object_position = idx($positions, $object_phid); + unset($positions[$object_phid]); + + if (!$object_position) { + $object_position = id(new PhabricatorProjectColumnPosition()) + ->setBoardPHID($board_phid) + ->setColumnPHID($column_phid) + ->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; + + $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 ($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; + $positions = msort($positions, 'getOrderingKey'); + + $this->boardLayout[$board_phid][$column_phid] = $positions; + + return $this; + } + + public function applyPositionUpdates() { + foreach ($this->remQueue as $position) { + if ($position->getID()) { + $position->delete(); + } + } + $this->remQueue = array(); + + $adds = array(); + $updates = array(); + + foreach ($this->addQueue as $position) { + $id = $position->getID(); + if ($id) { + $updates[$id] = $position; + } else { + $adds[] = $position; + } + } + $this->addQueue = array(); + + $table = new PhabricatorProjectColumnPosition(); + $conn_w = $table->establishConnection('w'); + + $pairs = array(); + foreach ($updates as $id => $position) { + // This is ugly because MySQL gets upset with us if it is configured + // strictly and we attempt inserts which can't work. We'll never actually + // do these inserts since they'll always collide (triggering the ON + // DUPLICATE KEY logic), so we just provide dummy values in order to get + // there. + + $pairs[] = qsprintf( + $conn_w, + '(%d, %d, "", "", "")', + $id, + $position->getSequence()); + } + + if ($pairs) { + queryfx( + $conn_w, + 'INSERT INTO %T (id, sequence, boardPHID, columnPHID, objectPHID) + VALUES %Q ON DUPLICATE KEY UPDATE sequence = VALUES(sequence)', + $table->getTableName(), + implode(', ', $pairs)); + } + + foreach ($adds as $position) { + $position->save(); + } + + return $this; + } + private function loadBoards() { $viewer = $this->getViewer(); $board_phids = $this->getBoardPHIDs(); @@ -114,10 +318,15 @@ final class PhabricatorBoardLayoutEngine extends Phobject { private function loadPositions(array $boards) { $viewer = $this->getViewer(); + $object_phids = $this->getObjectPHIDs(); + if (!$object_phids) { + return array(); + } + $positions = id(new PhabricatorProjectColumnPositionQuery()) ->setViewer($viewer) ->withBoardPHIDs(array_keys($boards)) - ->withObjectPHIDs($this->getObjectPHIDs()) + ->withObjectPHIDs($object_phids) ->execute(); $positions = msort($positions, 'getOrderingKey'); $positions = mgroup($positions, 'getBoardPHID'); @@ -150,6 +359,7 @@ final class PhabricatorBoardLayoutEngine extends Phobject { foreach ($positions as $key => $position) { $column_phid = $position->getColumnPHID(); if (empty($columns[$column_phid])) { + $this->remQueue[] = $position; unset($positions[$key]); } } @@ -161,6 +371,9 @@ final class PhabricatorBoardLayoutEngine extends Phobject { ->setColumnPHID($default_phid) ->setObjectPHID($object_phid) ->setSequence(0); + + $this->addQueue[] = $new_position; + $positions = array( $new_position, ); diff --git a/src/applications/project/storage/PhabricatorProjectColumnPosition.php b/src/applications/project/storage/PhabricatorProjectColumnPosition.php index 9815db18e5..c59676f8e4 100644 --- a/src/applications/project/storage/PhabricatorProjectColumnPosition.php +++ b/src/applications/project/storage/PhabricatorProjectColumnPosition.php @@ -41,7 +41,7 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO } public function getOrderingKey() { - if (!$this->getID()) { + if (!$this->getID() && !$this->getSequence()) { return 0; }