diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index a4086281dd..6438c47d93 100644 --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -387,10 +387,23 @@ final class ManiphestTaskEditController extends ManiphestController { ->withPHIDs($task_phids) ->execute(); - $sort_map = mpull( - $column_tasks, - 'getPrioritySortVector', - 'getPHID'); + if ($order == PhabricatorProjectColumn::ORDER_NATURAL) { + // TODO: This is a little bit awkward, because PHP and JS use + // slightly different sort order parameters to achieve the same + // effect. It would be unify this a bit at some point. + $sort_map = array(); + foreach ($positions as $position) { + $sort_map[$position->getObjectPHID()] = array( + -$position->getSequence(), + $position->getID(), + ); + } + } else { + $sort_map = mpull( + $column_tasks, + 'getPrioritySortVector', + 'getPHID'); + } $data = array( 'sortMap' => $sort_map, diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index f4069bf183..aa3e07b4f6 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -190,34 +190,144 @@ final class ManiphestTransactionEditor pht("Expected 'projectPHID' in column transaction.")); } + $old_phids = idx($xaction->getOldValue(), 'columnPHIDs', array()); $new_phids = idx($xaction->getNewValue(), 'columnPHIDs', array()); if (count($new_phids) !== 1) { throw new Exception( pht("Expected exactly one 'columnPHIDs' in column transaction.")); } + $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'); + + if (!$before_phid && !$after_phid && ($old_phids == $new_phids)) { + // If we are not moving the object between columns and also not + // reordering the position, this is a move on some other order + // (like priority). We can leave the positions untouched and just + // bail, there's no work to be done. + return; + } + + // Otherwise, we're either moving between columns or adjusting the + // object's position in the "natural" ordering, so we do need to update + // some rows. + // Remove all existing column positions on the board. foreach ($positions as $position) { $position->delete(); } - // Add the new column position. + // Add the new column positions. foreach ($new_phids as $phid) { - id(new PhabricatorProjectColumnPosition()) + $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($phid) - ->setObjectPHID($object->getPHID()) - // TODO: Do real sequence stuff. - ->setSequence(0) - ->save(); + ->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)); + } } break; default: diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 4e297534ed..ac0ada0178 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -125,6 +125,16 @@ final class ManiphestTransaction break; case self::TYPE_SUBPRIORITY: return true; + case self::TYPE_PROJECT_COLUMN: + $old_cols = idx($this->getOldValue(), 'columnPHIDs'); + $new_cols = idx($this->getNewValue(), 'columnPHIDs'); + + $old_cols = array_values($old_cols); + $new_cols = array_values($new_cols); + sort($old_cols); + sort($new_cols); + + return ($old_cols === $new_cols); } return parent::shouldHide(); diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index 25ffbeae37..66cb1b5de4 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -178,6 +178,23 @@ final class PhabricatorProjectBoardViewController $task_map[$column_phid][] = $task_phid; } + // If we're showing the board in "natural" order, sort columns by their + // column positions. + if ($this->sortKey == PhabricatorProjectColumn::ORDER_NATURAL) { + foreach ($task_map as $column_phid => $task_phids) { + $order = array(); + foreach ($task_phids as $task_phid) { + if (isset($positions[$task_phid])) { + $order[$task_phid] = $positions[$task_phid]->getOrderingKey(); + } else { + $order[$task_phid] = 0; + } + } + asort($order); + $task_map[$column_phid] = array_keys($order); + } + } + $task_can_edit_map = id(new PhabricatorPolicyFilter()) ->setViewer($viewer) ->requireCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT)) diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php index d6c6b44118..5302877093 100644 --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -17,6 +17,8 @@ final class PhabricatorProjectMoveController $object_phid = $request->getStr('objectPHID'); $after_phid = $request->getStr('afterPHID'); $before_phid = $request->getStr('beforePHID'); + $order = $request->getStr('order', PhabricatorProjectColumn::DEFAULT_ORDER); + $project = id(new PhabricatorProjectQuery()) ->setViewer($viewer) @@ -65,13 +67,22 @@ final class PhabricatorProjectMoveController $xactions = array(); + if ($order == PhabricatorProjectColumn::ORDER_NATURAL) { + $order_params = array( + 'afterPHID' => $after_phid, + 'beforePHID' => $before_phid, + ); + } else { + $order_params = array(); + } + $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN) ->setNewValue( array( 'columnPHIDs' => array($column->getPHID()), 'projectPHID' => $column->getProjectPHID(), - )) + ) + $order_params) ->setOldValue( array( 'columnPHIDs' => mpull($positions, 'getColumnPHID'), @@ -86,7 +97,7 @@ final class PhabricatorProjectMoveController $task_phids[] = $before_phid; } - if ($task_phids) { + if ($task_phids && ($order == PhabricatorProjectColumn::ORDER_PRIORITY)) { $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) ->withPHIDs($task_phids) diff --git a/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php b/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php index 60e9b2de58..87bd87ffd3 100644 --- a/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php +++ b/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php @@ -9,6 +9,7 @@ final class PhabricatorProjectColumnPositionQuery private $columns; private $needColumns; + private $skipImplicitCreate; public function withIDs(array $ids) { $this->ids = $ids; @@ -46,6 +47,21 @@ final class PhabricatorProjectColumnPositionQuery return $this; } + + /** + * Skip implicit creation of column positions which are implied but do not + * yet exist. + * + * This is primarily useful internally. + * + * @param bool True to skip implicit creation of column positions. + * @return this + */ + public function setSkipImplicitCreate($skip) { + $this->skipImplicitCreate = $skip; + return $this; + } + // NOTE: For now, boards are always attached to projects. However, they might // not be in the future. This generalization just anticipates a future where // we let other types of objects (like users) have boards, or let boards @@ -93,7 +109,7 @@ final class PhabricatorProjectColumnPositionQuery // column and put it in the default column. $must_type_filter = false; - if ($this->columns) { + if ($this->columns && !$this->skipImplicitCreate) { $default_map = array(); foreach ($this->columns as $column) { if ($column->isDefaultColumn()) { diff --git a/src/applications/project/storage/PhabricatorProjectColumnPosition.php b/src/applications/project/storage/PhabricatorProjectColumnPosition.php index 1b229efdfb..0f84cb532f 100644 --- a/src/applications/project/storage/PhabricatorProjectColumnPosition.php +++ b/src/applications/project/storage/PhabricatorProjectColumnPosition.php @@ -25,6 +25,17 @@ final class PhabricatorProjectColumnPosition extends PhabricatorProjectDAO return $this; } + public function getOrderingKey() { + // Low sequence numbers go above high sequence numbers. + // High position IDs go above low position IDs. + // Broadly, this makes newly added stuff float to the top. + + return sprintf( + '~%012d%012d', + $this->getSequence(), + ((1 << 31) - $this->getID())); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ public function getCapabilities() {