From 3a28f86a6e03b8fba018c06334b58820c2079a3d Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Sep 2013 15:22:01 -0700 Subject: [PATCH] Refactor shared code between JIRA + Asana publishers into a base class Summary: Ref T3687. See some discussion in D6892. The JIRA doorkeeper publisher shares a reasonable amount of code with the Asana publisher. Remedy this: - Create `DoorkeeperFeedWorker`, where shared functionality lives (mostly related to building story context objects). - Push responsibility for enabling/disabling a worker into this new layer, via `isEnabled()`. This allows `FeedPublisherWorker` to dynamically find and schedule doorkeeper publishers, so third parties can add additional doorkeeper publishers. - Some general cleanup/documentation. Test Plan: Used `bin/feed republish` to republish stories about objects with JIRA and Asana links. Verified that doorkeeper publishers activated properly, made calls, and published events into the remote systems. Reviewers: btrahan, akopanev22 Reviewed By: btrahan CC: aran Maniphest Tasks: T3687 Differential Revision: https://secure.phabricator.com/D6906 --- src/__phutil_library_map__.php | 6 +- .../worker/DoorkeeperFeedWorker.php | 196 +++++++++++++++ .../worker/DoorkeeperFeedWorkerAsana.php | 234 +++++++----------- .../worker/DoorkeeperFeedWorkerJIRA.php | 126 ++++------ .../feed/worker/FeedPublisherWorker.php | 24 +- .../feed/worker/FeedPushWorker.php | 2 +- 6 files changed, 351 insertions(+), 237 deletions(-) create mode 100644 src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9dc8368449..2e79ae1d04 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -554,6 +554,7 @@ phutil_register_library_map(array( 'DoorkeeperExternalObject' => 'applications/doorkeeper/storage/DoorkeeperExternalObject.php', 'DoorkeeperExternalObjectQuery' => 'applications/doorkeeper/query/DoorkeeperExternalObjectQuery.php', 'DoorkeeperFeedStoryPublisher' => 'applications/doorkeeper/engine/DoorkeeperFeedStoryPublisher.php', + 'DoorkeeperFeedWorker' => 'applications/doorkeeper/worker/DoorkeeperFeedWorker.php', 'DoorkeeperFeedWorkerAsana' => 'applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php', 'DoorkeeperFeedWorkerJIRA' => 'applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php', 'DoorkeeperImportEngine' => 'applications/doorkeeper/engine/DoorkeeperImportEngine.php', @@ -2603,8 +2604,9 @@ phutil_register_library_map(array( 1 => 'PhabricatorPolicyInterface', ), 'DoorkeeperExternalObjectQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', - 'DoorkeeperFeedWorkerAsana' => 'FeedPushWorker', - 'DoorkeeperFeedWorkerJIRA' => 'FeedPushWorker', + 'DoorkeeperFeedWorker' => 'FeedPushWorker', + 'DoorkeeperFeedWorkerAsana' => 'DoorkeeperFeedWorker', + 'DoorkeeperFeedWorkerJIRA' => 'DoorkeeperFeedWorker', 'DoorkeeperImportEngine' => 'Phobject', 'DoorkeeperObjectRef' => 'Phobject', 'DoorkeeperRemarkupRule' => 'PhutilRemarkupRule', diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php new file mode 100644 index 0000000000..f73451ee87 --- /dev/null +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php @@ -0,0 +1,196 @@ +feedStory) { + $story = $this->loadFeedStory(); + $this->feedStory = $story; + } + return $this->feedStory; + } + + + /** + * Get the viewer for the act of publishing. + * + * NOTE: Publishing currently uses the omnipotent viewer because it depends + * on loading external accounts. Possibly we should tailor this. See T3732. + * Using the actor for most operations might make more sense. + * + * @return PhabricatorUser Viewer. + * @task context + */ + protected function getViewer() { + return PhabricatorUser::getOmnipotentUser(); + } + + + /** + * Get the @{class:DoorkeeperFeedStoryPublisher} which handles this object. + * + * @return DoorkeeperFeedStoryPublisher Object publisher. + * @task context + */ + protected function getPublisher() { + return $this->publisher; + } + + + /** + * Get the primary object the story is about, like a + * @{class:DifferentialRevision} or @{class:ManiphestTask}. + * + * @return object Object which the story is about. + * @task context + */ + protected function getStoryObject() { + if (!$this->storyObject) { + $story = $this->getFeedStory(); + try { + $object = $story->getPrimaryObject(); + } catch (Exception $ex) { + throw new PhabricatorWorkerPermanentFailureException( + $ex->getMessage()); + } + $this->storyObject = $object; + } + return $this->storyObject; + } + + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * Load the @{class:DoorkeeperFeedStoryPublisher} which corresponds to this + * object. Publishers provide a common API for pushing object updates into + * foreign systems. + * + * @return DoorkeeperFeedStoryPublisher Publisher for the story's object. + * @task internal + */ + private function loadPublisher() { + $story = $this->getFeedStory(); + $viewer = $this->getViewer(); + $object = $this->getStoryObject(); + + $publishers = id(new PhutilSymbolLoader()) + ->setAncestorClass('DoorkeeperFeedStoryPublisher') + ->loadObjects(); + + foreach ($publishers as $publisher) { + if (!$publisher->canPublishStory($story, $object)) { + continue; + } + + $publisher + ->setViewer($viewer) + ->setFeedStory($story); + + $object = $publisher->willPublishStory($object); + $this->storyObject = $object; + + $this->publisher = $publisher; + break; + } + + return $this->publisher; + } + + +/* -( Inherited )---------------------------------------------------------- */ + + + /** + * Doorkeeper workers set up some context, then call + * @{method:publishFeedStory}. + */ + final protected function doWork() { + if (!$this->isEnabled()) { + $this->log("Doorkeeper worker '%s' is not enabled.\n", get_class($this)); + return; + } + + $publisher = $this->loadPublisher(); + if (!$publisher) { + $this->log("Story is about an unsupported object type.\n"); + return; + } else { + $this->log("Using publisher '%s'.\n", get_class($publisher)); + } + + $this->publishFeedStory(); + } + + + /** + * By default, Doorkeeper workers perform a small number of retries with + * exponential backoff. A consideration in this policy is that many of these + * workers are laden with side effects. + */ + public function getMaximumRetryCount() { + return 4; + } + + + /** + * See @{method:getMaximumRetryCount} for a description of Doorkeeper + * retry defaults. + */ + public function getWaitBeforeRetry(PhabricatorWorkerTask $task) { + $count = $task->getFailureCount(); + return (5 * 60) * pow(8, $count); + } + +} diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php index d56ddd1e08..a389df3bd7 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php @@ -1,120 +1,30 @@ provider) { - $provider = PhabricatorAuthProviderOAuthAsana::getAsanaProvider(); - if (!$provider) { - throw new PhabricatorWorkerPermanentFailureException( - 'No Asana provider configured.'); - } - $this->provider = $provider; - } - return $this->provider; + +/* -( Publishing Stories )------------------------------------------------- */ + + + /** + * This worker is enabled when an Asana workspace ID is configured with + * `asana.workspace-id`. + */ + public function isEnabled() { + return (bool)$this->getWorkspaceID(); } - private function getWorkspaceID() { - if (!$this->workspaceID) { - $workspace_id = PhabricatorEnv::getEnvConfig('asana.workspace-id'); - if (!$workspace_id) { - throw new PhabricatorWorkerPermanentFailureException( - 'No workspace Asana ID configured.'); - } - $this->workspaceID = $workspace_id; - } - return $this->workspaceID; - } - private function getFeedStory() { - if (!$this->feedStory) { - $story = $this->loadFeedStory(); - $this->feedStory = $story; - } - return $this->feedStory; - } - - private function getViewer() { - return PhabricatorUser::getOmnipotentUser(); - } - - private function getPublisher() { - return $this->publisher; - } - - private function getStoryObject() { - if (!$this->storyObject) { - $story = $this->getFeedStory(); - try { - $object = $story->getPrimaryObject(); - } catch (Exception $ex) { - throw new PhabricatorWorkerPermanentFailureException( - $ex->getMessage()); - } - $this->storyObject = $object; - } - return $this->storyObject; - } - - private function getAsanaTaskData($object) { - $publisher = $this->getPublisher(); - - $title = $publisher->getObjectTitle($object); - $uri = $publisher->getObjectURI($object); - $description = $publisher->getObjectDescription($object); - $is_completed = $publisher->isObjectClosed($object); - - $notes = array( - $description, - $uri, - $this->getSynchronizationWarning(), - ); - - $notes = implode("\n\n", $notes); - - return array( - 'name' => $title, - 'notes' => $notes, - 'completed' => $is_completed, - ); - } - - private function getAsanaSubtaskData($object) { - $publisher = $this->getPublisher(); - - $title = $publisher->getResponsibilityTitle($object); - $uri = $publisher->getObjectURI($object); - $description = $publisher->getObjectDescription($object); - - $notes = array( - $description, - $uri, - $this->getSynchronizationWarning(), - ); - - $notes = implode("\n\n", $notes); - - return array( - 'name' => $title, - 'notes' => $notes, - ); - } - - private function getSynchronizationWarning() { - return - "\xE2\x9A\xA0 DO NOT EDIT THIS TASK \xE2\x9A\xA0\n". - "\xE2\x98\xA0 Your changes will not be reflected in Phabricator.\n". - "\xE2\x98\xA0 Your changes will be destroyed the next time state ". - "is synchronized."; - } - - protected function doWork() { + /** + * Publish stories into Asana using the Asana API. + */ + protected function publishFeedStory() { $story = $this->getFeedStory(); $data = $story->getStoryData(); @@ -125,30 +35,7 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { $object = $this->getStoryObject(); $src_phid = $object->getPHID(); - $chronological_key = $story->getChronologicalKey(); - - $publishers = id(new PhutilSymbolLoader()) - ->setAncestorClass('DoorkeeperFeedStoryPublisher') - ->loadObjects(); - foreach ($publishers as $publisher) { - if ($publisher->canPublishStory($story, $object)) { - $publisher - ->setViewer($viewer) - ->setFeedStory($story); - - $object = $publisher->willPublishStory($object); - $this->storyObject = $object; - - $this->publisher = $publisher; - $this->log("Using publisher '%s'.\n", get_class($publisher)); - break; - } - } - - if (!$this->publisher) { - $this->log("Story is about an unsupported object type.\n"); - return; - } + $publisher = $this->getPublisher(); // Figure out all the users related to the object. Users go into one of // four buckets: @@ -539,6 +426,77 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { } } + +/* -( Internals )---------------------------------------------------------- */ + + private function getWorkspaceID() { + return PhabricatorEnv::getEnvConfig('asana.workspace-id'); + } + + private function getProvider() { + if (!$this->provider) { + $provider = PhabricatorAuthProviderOAuthAsana::getAsanaProvider(); + if (!$provider) { + throw new PhabricatorWorkerPermanentFailureException( + 'No Asana provider configured.'); + } + $this->provider = $provider; + } + return $this->provider; + } + + private function getAsanaTaskData($object) { + $publisher = $this->getPublisher(); + + $title = $publisher->getObjectTitle($object); + $uri = $publisher->getObjectURI($object); + $description = $publisher->getObjectDescription($object); + $is_completed = $publisher->isObjectClosed($object); + + $notes = array( + $description, + $uri, + $this->getSynchronizationWarning(), + ); + + $notes = implode("\n\n", $notes); + + return array( + 'name' => $title, + 'notes' => $notes, + 'completed' => $is_completed, + ); + } + + private function getAsanaSubtaskData($object) { + $publisher = $this->getPublisher(); + + $title = $publisher->getResponsibilityTitle($object); + $uri = $publisher->getObjectURI($object); + $description = $publisher->getObjectDescription($object); + + $notes = array( + $description, + $uri, + $this->getSynchronizationWarning(), + ); + + $notes = implode("\n\n", $notes); + + return array( + 'name' => $title, + 'notes' => $notes, + ); + } + + private function getSynchronizationWarning() { + return + "\xE2\x9A\xA0 DO NOT EDIT THIS TASK \xE2\x9A\xA0\n". + "\xE2\x98\xA0 Your changes will not be reflected in Phabricator.\n". + "\xE2\x98\xA0 Your changes will be destroyed the next time state ". + "is synchronized."; + } + private function lookupAsanaUserIDs($all_phids) { $phid_map = array(); @@ -658,15 +616,6 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { return $ref; } - public function getMaximumRetryCount() { - return 4; - } - - public function getWaitBeforeRetry(PhabricatorWorkerTask $task) { - $count = $task->getFailureCount(); - return (5 * 60) * pow(8, $count); - } - private function addFollowers( $oauth_token, $task_id, @@ -694,5 +643,4 @@ final class DoorkeeperFeedWorkerAsana extends FeedPushWorker { $data); } - } diff --git a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php index 29d968c480..e9e6a3fae9 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php +++ b/src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php @@ -1,89 +1,34 @@ provider) { - $provider = PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider(); - if (!$provider) { - throw new PhabricatorWorkerPermanentFailureException( - 'No JIRA provider configured.'); - } - $this->provider = $provider; - } - return $this->provider; + +/* -( Publishing Stories )------------------------------------------------- */ + + + /** + * This worker is enabled when a JIRA authentication provider is active. + */ + public function isEnabled() { + return (bool)PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider(); } - private function getFeedStory() { - if (!$this->feedStory) { - $story = $this->loadFeedStory(); - $this->feedStory = $story; - } - return $this->feedStory; - } - private function getViewer() { - return PhabricatorUser::getOmnipotentUser(); - } - - private function getPublisher() { - return $this->publisher; - } - - private function getStoryObject() { - if (!$this->storyObject) { - $story = $this->getFeedStory(); - try { - $object = $story->getPrimaryObject(); - } catch (Exception $ex) { - throw new PhabricatorWorkerPermanentFailureException( - $ex->getMessage()); - } - $this->storyObject = $object; - } - return $this->storyObject; - } - - protected function doWork() { + /** + * Publishes stories into JIRA using the JIRA API. + */ + protected function publishFeedStory() { $story = $this->getFeedStory(); - $data = $story->getStoryData(); - $viewer = $this->getViewer(); $provider = $this->getProvider(); - $object = $this->getStoryObject(); - $src_phid = $object->getPHID(); - - $chronological_key = $story->getChronologicalKey(); - - $publishers = id(new PhutilSymbolLoader()) - ->setAncestorClass('DoorkeeperFeedStoryPublisher') - ->loadObjects(); - foreach ($publishers as $publisher) { - if ($publisher->canPublishStory($story, $object)) { - $publisher - ->setViewer($viewer) - ->setFeedStory($story); - - $object = $publisher->willPublishStory($object); - $this->storyObject = $object; - - $this->publisher = $publisher; - $this->log("Using publisher '%s'.\n", get_class($publisher)); - break; - } - } - - if (!$this->publisher) { - $this->log("Story is about an unsupported object type.\n"); - return; - } + $publisher = $this->getPublisher(); $jira_issue_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( $object->getPHID(), @@ -148,15 +93,36 @@ final class DoorkeeperFeedWorkerJIRA extends FeedPushWorker { } } - public function getMaximumRetryCount() { - return 4; + +/* -( Internals )---------------------------------------------------------- */ + + + /** + * Get the active JIRA provider. + * + * @return PhabricatorAuthProviderOAuth1JIRA Active JIRA auth provider. + * @task internal + */ + private function getProvider() { + if (!$this->provider) { + $provider = PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider(); + if (!$provider) { + throw new PhabricatorWorkerPermanentFailureException( + 'No JIRA provider configured.'); + } + $this->provider = $provider; + } + return $this->provider; } - public function getWaitBeforeRetry(PhabricatorWorkerTask $task) { - $count = $task->getFailureCount(); - return (5 * 60) * pow(8, $count); - } + /** + * Get a list of users to act as when publishing into JIRA. + * + * @return list Candidate user PHIDs to act as when publishing this + * story. + * @task internal + */ private function findUsersToPossess() { $object = $this->getStoryObject(); $publisher = $this->getPublisher(); diff --git a/src/applications/feed/worker/FeedPublisherWorker.php b/src/applications/feed/worker/FeedPublisherWorker.php index 804e2435af..b2d8860679 100644 --- a/src/applications/feed/worker/FeedPublisherWorker.php +++ b/src/applications/feed/worker/FeedPublisherWorker.php @@ -15,22 +15,24 @@ final class FeedPublisherWorker extends FeedPushWorker { )); } - if (PhabricatorEnv::getEnvConfig('asana.workspace-id')) { + $argv = array( + array(), + ); + + // Find and schedule all the enabled Doorkeeper publishers. + $doorkeeper_workers = id(new PhutilSymbolLoader()) + ->setAncestorClass('DoorkeeperFeedWorker') + ->loadObjects($argv); + foreach ($doorkeeper_workers as $worker) { + if (!$worker->isEnabled()) { + continue; + } PhabricatorWorker::scheduleTask( - 'DoorkeeperFeedWorkerAsana', + get_class($worker), array( 'key' => $story->getChronologicalKey(), )); } - - if (PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider()) { - PhabricatorWorker::scheduleTask( - 'DoorkeeperFeedWorkerJIRA', - array( - 'key' => $story->getChronologicalKey(), - )); - } - } diff --git a/src/applications/feed/worker/FeedPushWorker.php b/src/applications/feed/worker/FeedPushWorker.php index 2a8b5723d2..b406b76ee0 100644 --- a/src/applications/feed/worker/FeedPushWorker.php +++ b/src/applications/feed/worker/FeedPushWorker.php @@ -13,7 +13,7 @@ abstract class FeedPushWorker extends PhabricatorWorker { if (!$story) { throw new PhabricatorWorkerPermanentFailureException( - 'Feed story does not exist..'); + 'Feed story does not exist.'); } return $story;