diff --git a/resources/sql/autopatches/20140805.boardcol.1.sql b/resources/sql/autopatches/20140805.boardcol.1.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20140805.boardcol.1.sql @@ -0,0 +1,10 @@ +CREATE TABLE {$NAMESPACE}_project.project_columnposition ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + boardPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + columnPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + sequence INT UNSIGNED NOT NULL, + UNIQUE KEY (boardPHID, columnPHID, objectPHID), + KEY (objectPHID, boardPHID), + KEY (boardPHID, columnPHID, sequence) +) ENGINE=InnoDB, COLLATE utf8_general_ci; diff --git a/resources/sql/autopatches/20140805.boardcol.2.php b/resources/sql/autopatches/20140805.boardcol.2.php new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20140805.boardcol.2.php @@ -0,0 +1,53 @@ +establishConnection('w'); + +$rows = queryfx_all( + $conn_w, + 'SELECT src, dst FROM %T WHERE type = %d', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + $type_has_object); + +$cols = array(); +foreach ($rows as $row) { + $cols[$row['src']][] = $row['dst']; +} + +$sql = array(); +foreach ($cols as $col_phid => $obj_phids) { + echo "Migrating column '{$col_phid}'...\n"; + $column = id(new PhabricatorProjectColumn())->loadOneWhere( + 'phid = %s', + $col_phid); + if (!$column) { + echo "Column '{$col_phid}' does not exist.\n"; + continue; + } + + $sequence = 0; + foreach ($obj_phids as $obj_phid) { + $sql[] = qsprintf( + $conn_w, + '(%s, %s, %s, %d)', + $column->getProjectPHID(), + $column->getPHID(), + $obj_phid, + $sequence++); + } +} + +echo "Inserting rows...\n"; +foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn_w, + 'INSERT INTO %T (boardPHID, columnPHID, objectPHID, sequence) + VALUES %Q', + id(new PhabricatorProjectColumnPosition())->getTableName(), + $chunk); +} + +echo "Done.\n"; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1932,6 +1932,8 @@ 'PhabricatorProjectColumn' => 'applications/project/storage/PhabricatorProjectColumn.php', 'PhabricatorProjectColumnDetailController' => 'applications/project/controller/PhabricatorProjectColumnDetailController.php', 'PhabricatorProjectColumnPHIDType' => 'applications/project/phid/PhabricatorProjectColumnPHIDType.php', + 'PhabricatorProjectColumnPosition' => 'applications/project/storage/PhabricatorProjectColumnPosition.php', + 'PhabricatorProjectColumnPositionQuery' => 'applications/project/query/PhabricatorProjectColumnPositionQuery.php', 'PhabricatorProjectColumnQuery' => 'applications/project/query/PhabricatorProjectColumnQuery.php', 'PhabricatorProjectColumnTransaction' => 'applications/project/storage/PhabricatorProjectColumnTransaction.php', 'PhabricatorProjectColumnTransactionEditor' => 'applications/project/editor/PhabricatorProjectColumnTransactionEditor.php', @@ -4764,6 +4766,11 @@ ), 'PhabricatorProjectColumnDetailController' => 'PhabricatorProjectBoardController', 'PhabricatorProjectColumnPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorProjectColumnPosition' => array( + 'PhabricatorProjectDAO', + 'PhabricatorPolicyInterface', + ), + 'PhabricatorProjectColumnPositionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectColumnQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorProjectColumnTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorProjectColumnTransactionEditor' => 'PhabricatorApplicationTransactionEditor', diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -534,27 +534,13 @@ if ($project_phids) { require_celerity_resource('maniphest-task-summary-css'); - // If we end up with real-world projects with many hundreds of columns, it - // might be better to just load all the edges, then load those columns and - // work backward that way, or denormalize this data more. - - $columns = id(new PhabricatorProjectColumnQuery()) + $positions = id(new PhabricatorProjectColumnPositionQuery()) ->setViewer($viewer) - ->withProjectPHIDs($project_phids) + ->withBoardPHIDs($project_phids) + ->withObjectPHIDs(array($task->getPHID())) + ->needColumns(true) ->execute(); - $columns = mpull($columns, null, 'getPHID'); - - $column_edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_COLUMN; - $all_column_phids = array_keys($columns); - - $column_edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array($task->getPHID())) - ->withEdgeTypes(array($column_edge_type)) - ->withDestinationPHIDs($all_column_phids); - $column_edge_query->execute(); - $in_column_phids = array_fuse($column_edge_query->getDestinationPHIDs()); - - $column_groups = mgroup($columns, 'getProjectPHID'); + $positions = mpull($positions, null, 'getBoardPHID'); $project_handles = array(); $project_annotations = array(); @@ -562,9 +548,10 @@ $handle = $this->getHandle($project_phid); $project_handles[] = $handle; - $columns = idx($column_groups, $project_phid, array()); - $column = head(array_intersect_key($columns, $in_column_phids)); - if ($column) { + $position = idx($positions, $project_phid); + if ($position) { + $column = $position->getColumn(); + $column_name = pht('(%s)', $column->getDisplayName()); $column_link = phutil_tag( 'a', diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php --- a/src/applications/maniphest/controller/ManiphestTaskEditController.php +++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php @@ -339,6 +339,7 @@ if ($parent_task) { + // TODO: This should be transactional now. id(new PhabricatorEdgeEditor()) ->addEdge( $parent_task->getPHID(), @@ -364,47 +365,26 @@ ->setOwner($owner) ->setCanEdit(true) ->getItem(); - $column_phid = $request->getStr('columnPHID'); + $column = id(new PhabricatorProjectColumnQuery()) ->setViewer($user) - ->withPHIDs(array($column_phid)) + ->withPHIDs(array($request->getStr('columnPHID'))) ->executeOne(); - if ($column->isDefaultColumn()) { - $column_tasks = array(); - $potential_col_tasks = id(new ManiphestTaskQuery()) - ->setViewer($user) - ->withAllProjects(array($column->getProjectPHID())) - ->execute(); - $potential_col_tasks = mpull( - $potential_col_tasks, - null, - 'getPHID'); - $potential_task_phids = array_keys($potential_col_tasks); - if ($potential_task_phids) { - $edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_COLUMN; - $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($potential_task_phids) - ->withEdgeTypes(array($edge_type)); - $edges = $edge_query->execute(); - foreach ($potential_col_tasks as $task_phid => $curr_task) { - $curr_column_phids = $edges[$task_phid][$edge_type]; - $curr_column_phid = head_key($curr_column_phids); - if (!$curr_column_phid || - $curr_column_phid == $column_phid) { - $column_tasks[] = $curr_task; - } - } - } - } else { - $column_task_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $column_phid, - PhabricatorEdgeConfig::TYPE_COLUMN_HAS_OBJECT); - $column_tasks = id(new ManiphestTaskQuery()) - ->setViewer($user) - ->withPHIDs($column_task_phids) - ->execute(); + if (!$column) { + return new Aphront404Response(); } + $positions = id(new PhabricatorProjectColumnPositionQuery()) + ->setViewer($user) + ->withColumns(array($column)) + ->execute(); + $task_phids = mpull($positions, 'getObjectPHID'); + + $column_tasks = id(new ManiphestTaskQuery()) + ->setViewer($user) + ->withPHIDs($task_phids) + ->execute(); + $sort_map = mpull( $column_tasks, 'getPrioritySortVector', diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -184,43 +184,45 @@ switch ($xaction->getTransactionType()) { case ManiphestTransaction::TYPE_PROJECT_COLUMN: - $new = $xaction->getNewValue(); - $old = $xaction->getOldValue(); - $src = $object->getPHID(); - $dst = head($new['columnPHIDs']); - $edges = $old['columnPHIDs']; - $edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_COLUMN; - // NOTE: Normally, we expect only one edge to exist, but this works in - // a general way so it will repair any stray edges. - $remove = array(); - $edge_missing = true; - foreach ($edges as $phid) { - if ($phid == $dst) { - $edge_missing = false; - } else { - $remove[] = $phid; - } + $board_phid = idx($xaction->getNewValue(), 'projectPHID'); + if (!$board_phid) { + throw new Exception( + pht("Expected 'projectPHID' in column transaction.")); } - $add = array(); - if ($edge_missing) { - $add[] = $dst; + $new_phids = idx($xaction->getNewValue(), 'columnPHIDs', array()); + if (count($new_phids) !== 1) { + throw new Exception( + pht("Expected exactly one 'columnPHIDs' in column transaction.")); } - // This should never happen because of the code in - // transactionHasEffect, but keep it for maximum conservativeness - if (!$add && !$remove) { - return; - } + $positions = id(new PhabricatorProjectColumnPositionQuery()) + ->setViewer($this->requireActor()) + ->withObjectPHIDs(array($object->getPHID())) + ->withBoardPHIDs(array($board_phid)) + ->execute(); + + // Remove all existing column positions on the board. - $editor = new PhabricatorEdgeEditor(); - foreach ($add as $phid) { - $editor->addEdge($src, $edge_type, $phid); + foreach ($positions as $position) { + if (!$position->getID()) { + // This is an ephemeral position, so don't try to destroy it. + continue; + } + $position->delete(); } - foreach ($remove as $phid) { - $editor->removeEdge($src, $edge_type, $phid); + + // Add the new column position. + + foreach ($new_phids as $phid) { + id(new PhabricatorProjectColumnPosition()) + ->setBoardPHID($board_phid) + ->setColumnPHID($phid) + ->setObjectPHID($object->getPHID()) + // TODO: Do real sequence stuff. + ->setSequence(0) + ->save(); } - $editor->save(); break; default: break; diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -135,26 +135,28 @@ ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) ->setViewer($viewer) ->execute(); - $tasks = mpull($tasks, null, 'getPHID'); - $task_phids = array_keys($tasks); - - if ($task_phids) { - $edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_COLUMN; - $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($task_phids) - ->withEdgeTypes(array($edge_type)) - ->withDestinationPHIDs(mpull($columns, 'getPHID')); - $edge_query->execute(); + + if ($tasks) { + $positions = id(new PhabricatorProjectColumnPositionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(mpull($tasks, 'getPHID')) + ->withColumns($columns) + ->execute(); + $positions = mpull($positions, null, 'getObjectPHID'); + } else { + $positions = array(); } $task_map = array(); $default_phid = $columns[0]->getPHID(); foreach ($tasks as $task) { $task_phid = $task->getPHID(); - $column_phids = $edge_query->getDestinationPHIDs(array($task_phid)); - $column_phid = head($column_phids); + $column_phid = null; + if (isset($positions[$task_phid])) { + $column_phid = $positions[$task_phid]->getColumnPHID(); + } $column_phid = nonempty($column_phid, $default_phid); $task_map[$column_phid][] = $task_phid; diff --git a/src/applications/project/controller/PhabricatorProjectMoveController.php b/src/applications/project/controller/PhabricatorProjectMoveController.php --- a/src/applications/project/controller/PhabricatorProjectMoveController.php +++ b/src/applications/project/controller/PhabricatorProjectMoveController.php @@ -57,27 +57,26 @@ return new Aphront404Response(); } - $xactions = array(); - - $edge_type = PhabricatorEdgeConfig::TYPE_OBJECT_HAS_COLUMN; - - $query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(array($object->getPHID())) - ->withEdgeTypes(array($edge_type)) - ->withDestinationPHIDs(array_keys($columns)); - - $query->execute(); + $positions = id(new PhabricatorProjectColumnPositionQuery()) + ->setViewer($viewer) + ->withColumns($columns) + ->withObjectPHIDs(array($object_phid)) + ->execute(); - $edge_phids = $query->getDestinationPHIDs(); + $xactions = array(); $xactions[] = id(new ManiphestTransaction()) ->setTransactionType(ManiphestTransaction::TYPE_PROJECT_COLUMN) - ->setNewValue(array( - 'columnPHIDs' => array($column->getPHID()), - 'projectPHID' => $column->getProjectPHID())) - ->setOldValue(array( - 'columnPHIDs' => $edge_phids, - 'projectPHID' => $column->getProjectPHID())); + ->setNewValue( + array( + 'columnPHIDs' => array($column->getPHID()), + 'projectPHID' => $column->getProjectPHID(), + )) + ->setOldValue( + array( + 'columnPHIDs' => mpull($positions, 'getColumnPHID'), + 'projectPHID' => $column->getProjectPHID(), + )); $task_phids = array(); if ($after_phid) { @@ -86,6 +85,7 @@ if ($before_phid) { $task_phids[] = $before_phid; } + if ($task_phids) { $tasks = id(new ManiphestTaskQuery()) ->setViewer($viewer) diff --git a/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php b/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/project/query/PhabricatorProjectColumnPositionQuery.php @@ -0,0 +1,258 @@ +ids = $ids; + return $this; + } + + public function withBoardPHIDs(array $board_phids) { + $this->boardPHIDs = $board_phids; + return $this; + } + + public function withObjectPHIDs(array $object_phids) { + $this->objectPHIDs = $object_phids; + return $this; + } + + /** + * Find objects in specific columns. + * + * NOTE: Using this method activates logic which constructs virtual + * column positions for objects not in any column, if you pass a default + * column. Normally these results are not returned. + * + * @param list Columns to look for objects in. + * @return this + */ + public function withColumns(array $columns) { + assert_instances_of($columns, 'PhabricatorProjectColumn'); + $this->columns = $columns; + return $this; + } + + public function needColumns($need_columns) { + $this->needColumns = true; + 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 + // contain other types of objects. + + private function newPositionObject() { + return new PhabricatorProjectColumnPosition(); + } + + private function newColumnQuery() { + return new PhabricatorProjectColumnQuery(); + } + + private function getBoardMembershipEdgeTypes() { + return array( + PhabricatorProjectProjectHasObjectEdgeType::EDGECONST, + ); + } + + private function getBoardMembershipPHIDTypes() { + return array( + ManiphestTaskPHIDType::TYPECONST, + ); + } + + protected function loadPage() { + $table = $this->newPositionObject(); + $conn_r = $table->establishConnection('r'); + + // We're going to find results by combining two queries: one query finds + // objects on a board column, while the other query finds objects not on + // any board column and virtually puts them on the default column. + + $unions = array(); + + // First, find all the stuff that's actually on a column. + + $unions[] = qsprintf( + $conn_r, + 'SELECT * FROM %T %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r)); + + // If we have a default column, find all the stuff that's not in any + // column and put it in the default column. + + $must_type_filter = false; + if ($this->columns) { + $default_map = array(); + foreach ($this->columns as $column) { + if ($column->isDefaultColumn()) { + $default_map[$column->getProjectPHID()] = $column->getPHID(); + } + } + + if ($default_map) { + $where = array(); + + // Find the edges attached to the boards we have default columns for. + + $where[] = qsprintf( + $conn_r, + 'e.src IN (%Ls)', + array_keys($default_map)); + + // Find only edges which describe a board relationship. + + $where[] = qsprintf( + $conn_r, + 'e.type IN (%Ld)', + $this->getBoardMembershipEdgeTypes()); + + if ($this->boardPHIDs !== null) { + // This should normally be redundant, but construct it anyway if + // the caller has told us to. + $where[] = qsprintf( + $conn_r, + 'e.src IN (%Ls)', + $this->boardPHIDs); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'e.dst IN (%Ls)', + $this->objectPHIDs); + } + + $where[] = qsprintf( + $conn_r, + 'p.id IS NULL'); + + $where = $this->formatWhereClause($where); + + $unions[] = qsprintf( + $conn_r, + 'SELECT NULL id, e.src boardPHID, NULL columnPHID, e.dst objectPHID, + 0 sequence + FROM %T e LEFT JOIN %T p + ON e.src = p.boardPHID AND e.dst = p.objectPHID + %Q', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + $table->getTableName(), + $where); + + $must_type_filter = true; + } + } + + $data = queryfx_all( + $conn_r, + '%Q %Q %Q', + implode(' UNION ALL ', $unions), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + // If we've picked up objects not in any column, we need to filter out any + // matched objects which have the wrong edge type. + if ($must_type_filter) { + $allowed_types = array_fuse($this->getBoardMembershipPHIDTypes()); + foreach ($data as $id => $row) { + if ($row['columnPHID'] === null) { + $object_phid = $row['objectPHID']; + if (empty($allowed_types[phid_get_type($object_phid)])) { + unset($data[$id]); + } + } + } + } + + $positions = $table->loadAllFromArray($data); + + foreach ($positions as $position) { + if ($position->getColumnPHID() === null) { + $position->makeEphemeral(); + $column_phid = idx($default_map, $position->getBoardPHID()); + $position->setColumnPHID($column_phid); + } + } + + return $positions; + } + + protected function willFilterPage(array $page) { + + if ($this->needColumns) { + $column_phids = mpull($page, 'getColumnPHID'); + $columns = $this->newColumnQuery() + ->setParentQuery($this) + ->setViewer($this->getViewer()) + ->withPHIDs($column_phids) + ->execute(); + $columns = mpull($columns, null, 'getPHID'); + + foreach ($page as $key => $position) { + $column = idx($columns, $position->getColumnPHID()); + if (!$column) { + unset($page[$key]); + continue; + } + + $position->attachColumn($column); + } + } + + return $page; + } + + private function buildWhereClause($conn_r) { + $where = array(); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn_r, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->boardPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'boardPHID IN (%Ls)', + $this->boardPHIDs); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + if ($this->columns !== null) { + $where[] = qsprintf( + $conn_r, + 'columnPHID IN (%Ls)', + mpull($this->columns, 'getPHID')); + } + + // NOTE: Explicitly not building the paging clause here, since it won't + // work with the UNION. + + return $this->formatWhereClause($where); + } + + public function getQueryApplicationClass() { + return 'PhabricatorProjectApplication'; + } + +} diff --git a/src/applications/project/storage/PhabricatorProjectColumnPosition.php b/src/applications/project/storage/PhabricatorProjectColumnPosition.php new file mode 100644 --- /dev/null +++ b/src/applications/project/storage/PhabricatorProjectColumnPosition.php @@ -0,0 +1,51 @@ + false, + ) + parent::getConfiguration(); + } + + public function getColumn() { + return $this->assertAttached($this->column); + } + + public function attachColumn(PhabricatorProjectColumn $column) { + $this->column = $column; + return $this; + } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return PhabricatorPolicies::getMostOpenPolicy(); + } + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + + public function describeAutomaticCapability($capability) { + return null; + } + +} diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -57,9 +57,6 @@ const TYPE_OBJECT_USES_CREDENTIAL = 39; const TYPE_CREDENTIAL_USED_BY_OBJECT = 40; - const TYPE_OBJECT_HAS_COLUMN = 43; - const TYPE_COLUMN_HAS_OBJECT = 44; - const TYPE_DASHBOARD_HAS_PANEL = 45; const TYPE_PANEL_HAS_DASHBOARD = 46; @@ -105,6 +102,10 @@ range(1, 50), array(9000), range(80000, 80005)); + + $exclude[] = 43; // Was TYPE_OBJECT_HAS_COLUMN + $exclude[] = 44; // Was TYPE_COLUMN_HAS_OBJECT + $consts = array_diff($consts, $exclude); $map = array(); @@ -190,9 +191,6 @@ self::TYPE_OBJECT_USES_CREDENTIAL => self::TYPE_CREDENTIAL_USED_BY_OBJECT, self::TYPE_CREDENTIAL_USED_BY_OBJECT => self::TYPE_OBJECT_USES_CREDENTIAL, - self::TYPE_OBJECT_HAS_COLUMN => self::TYPE_COLUMN_HAS_OBJECT, - self::TYPE_COLUMN_HAS_OBJECT => self::TYPE_OBJECT_HAS_COLUMN, - self::TYPE_PANEL_HAS_DASHBOARD => self::TYPE_DASHBOARD_HAS_PANEL, self::TYPE_DASHBOARD_HAS_PANEL => self::TYPE_PANEL_HAS_DASHBOARD,