From 47dedfb152d0b83e936a857e83530d3754bfa335 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 Mar 2016 08:40:19 -0800 Subject: [PATCH] Introduce "bridged" objects Summary: Ref T10537. These are objects which are bound to some external object, like a Maniphest task which is a representation of a GitHub issue. This doesn't do much yet and may change, but my thinking is: - I'm putting these on-object instead of on edges because I think we want to actively change the UI for them (e.g., clearly call out that the object is bridged) but don't want every page to need to do extra queries in the common case where zero bridged objects exist anywhere in the system. - I'm making these one-to-one, more or less: an issue can't be bridged to a bunch of tasks, nor can a bunch of tasks be bridged to a single issue. Pretty sure this makes sense? I can't come up with any reasonable, realistic cases where you want a single GitHub issue to publish to multiple different tasks in Maniphest. - Technically, one type of each bridgable object could be bridged, but I expect this to never actually occur. Hopefully. Test Plan: Ran storage upgrade, loaded some pages. Reviewers: chad Reviewed By: chad Subscribers: Luke081515.2 Maniphest Tasks: T10537 Differential Revision: https://secure.phabricator.com/D15502 --- .../20160321.nuance.01.taskbridge.sql | 2 + src/__phutil_library_map__.php | 2 + .../DoorkeeperBridgedObjectInterface.php | 8 +++ .../maniphest/query/ManiphestTaskQuery.php | 13 +++++ .../maniphest/storage/ManiphestTask.php | 24 ++++++++- .../policy/PhabricatorPolicyAwareQuery.php | 51 +++++++++++++++++++ 6 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20160321.nuance.01.taskbridge.sql create mode 100644 src/applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php diff --git a/resources/sql/autopatches/20160321.nuance.01.taskbridge.sql b/resources/sql/autopatches/20160321.nuance.01.taskbridge.sql new file mode 100644 index 0000000000..53b80fe7d9 --- /dev/null +++ b/resources/sql/autopatches/20160321.nuance.01.taskbridge.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_maniphest.maniphest_task + ADD bridgedObjectPHID VARBINARY(64); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ad187d74f0..74dd5958eb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -845,6 +845,7 @@ phutil_register_library_map(array( 'DoorkeeperBridgeGitHubIssue' => 'applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php', 'DoorkeeperBridgeJIRA' => 'applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php', 'DoorkeeperBridgeJIRATestCase' => 'applications/doorkeeper/bridge/__tests__/DoorkeeperBridgeJIRATestCase.php', + 'DoorkeeperBridgedObjectInterface' => 'applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php', 'DoorkeeperDAO' => 'applications/doorkeeper/storage/DoorkeeperDAO.php', 'DoorkeeperExternalObject' => 'applications/doorkeeper/storage/DoorkeeperExternalObject.php', 'DoorkeeperExternalObjectQuery' => 'applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php', @@ -5636,6 +5637,7 @@ phutil_register_library_map(array( 'PhabricatorSpacesInterface', 'PhabricatorConduitResultInterface', 'PhabricatorFulltextInterface', + 'DoorkeeperBridgedObjectInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php new file mode 100644 index 0000000000..9d829642fc --- /dev/null +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgedObjectInterface.php @@ -0,0 +1,8 @@ +bridgedObjectPHIDs = $phids; + return $this; + } + public function newResultObject() { return new ManiphestTask(); } @@ -417,6 +423,13 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $this->subpriorityMax); } + if ($this->bridgedObjectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'task.bridgedObjectPHID IN (%Ls)', + $this->bridgedObjectPHIDs); + } + return $where; } diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 7718bdfcf8..9ac7190782 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -15,7 +15,8 @@ final class ManiphestTask extends ManiphestDAO PhabricatorProjectInterface, PhabricatorSpacesInterface, PhabricatorConduitResultInterface, - PhabricatorFulltextInterface { + PhabricatorFulltextInterface, + DoorkeeperBridgedObjectInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -36,6 +37,7 @@ final class ManiphestTask extends ManiphestDAO protected $ownerOrdering; protected $spacePHID; + protected $bridgedObjectPHID; protected $properties = array(); protected $points; @@ -43,6 +45,7 @@ final class ManiphestTask extends ManiphestDAO private $groupByProjectPHID = self::ATTACHABLE; private $customFields = self::ATTACHABLE; private $edgeProjectPHIDs = self::ATTACHABLE; + private $bridgedObject = self::ATTACHABLE; public static function initializeNewTask(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) @@ -82,6 +85,7 @@ final class ManiphestTask extends ManiphestDAO 'originalEmailSource' => 'text255?', 'subpriority' => 'double', 'points' => 'double?', + 'bridgedObjectPHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_phid' => null, @@ -116,6 +120,10 @@ final class ManiphestTask extends ManiphestDAO 'key_title' => array( 'columns' => array('title(64)'), ), + 'key_bridgedobject' => array( + 'columns' => array('bridgedObjectPHID'), + 'unique' => true, + ), ), ) + parent::getConfiguration(); } @@ -504,4 +512,18 @@ final class ManiphestTask extends ManiphestDAO return new ManiphestTaskFulltextEngine(); } + +/* -( DoorkeeperBridgedObjectInterface )----------------------------------- */ + + + public function getBridgedObject() { + return $this->assertAttached($this->bridgedObject); + } + + public function attachBridgedObject( + DoorkeeperExternalObject $object = null) { + $this->bridgedObject = $object; + return $this; + } + } diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index 9512acbc22..42d8001ee2 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -234,6 +234,9 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { if ($page) { $maybe_visible = $this->willFilterPage($page); + if ($maybe_visible) { + $maybe_visible = $this->applyWillFilterPageExtensions($maybe_visible); + } } else { $maybe_visible = array(); } @@ -672,4 +675,52 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { return PhabricatorApplication::isClassInstalledForViewer($class, $viewer); } + private function applyWillFilterPageExtensions(array $page) { + $bridges = array(); + foreach ($page as $key => $object) { + if ($object instanceof DoorkeeperBridgedObjectInterface) { + $bridges[$key] = $object; + } + } + + if ($bridges) { + $external_phids = array(); + foreach ($bridges as $bridge) { + $external_phid = $bridge->getBridgedObjectPHID(); + if ($external_phid) { + $external_phids[$key] = $external_phid; + } + } + + if ($external_phids) { + $external_objects = id(new DoorkeeperExternalObjectQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($external_phids) + ->execute(); + $external_objects = mpull($external_objects, null, 'getPHID'); + } else { + $external_objects = array(); + } + + foreach ($bridges as $key => $bridge) { + $external_phid = idx($external_phids, $key); + if (!$external_phid) { + $bridge->attachBridgedObject(null); + continue; + } + + $external_object = idx($external_objects, $external_phid); + if (!$external_object) { + $this->didRejectResult($bridge); + unset($page[$key]); + continue; + } + + $bridge->attachBridgedObject($external_object); + } + } + + return $page; + } + }