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

@@ -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',
+    'DoorkeeperFeedWorker' => 'FeedPushWorker',
-    'DoorkeeperFeedWorkerJIRA' => 'FeedPushWorker',
+    'DoorkeeperFeedWorkerAsana' => 'DoorkeeperFeedWorker',
+    'DoorkeeperFeedWorkerJIRA' => 'DoorkeeperFeedWorker',
     'DoorkeeperImportEngine' => 'Phobject',
     'DoorkeeperObjectRef' => 'Phobject',
     'DoorkeeperRemarkupRule' => 'PhutilRemarkupRule',

src/applications/doorkeeper/worker/DoorkeeperFeedWorker.php

@@ -0,0 +1,196 @@
+<?php
+
+/**
+ * Publish events (like comments on a revision) to external objects which are
+ * linked through Doorkeeper (like a linked JIRA or Asana task).
+ *
+ * These workers are invoked by feed infrastructure during normal task queue
+ * operations. They read feed stories and publish information about them to
+ * external systems, generally mirroring comments and updates in Phabricator
+ * into remote systems by making API calls.
+ *
+ * @task publish  Publishing Stories
+ * @task context  Story Context
+ * @task internal Internals
+ */
+abstract class DoorkeeperFeedWorker extends FeedPushWorker {
+
+  private $publisher;
+  private $feedStory;
+  private $storyObject;
+
+
+/* -(  Publishing Stories  )------------------------------------------------- */
+
+
+  /**
+   * Actually publish the feed story. Subclasses will generally make API calls
+   * to publish some version of the story into external systems.
+   *
+   * @return void
+   * @task publish
+   */
+  abstract protected function publishFeedStory();
+
+
+  /**
+   * Enable or disable the worker. Normally, this checks configuration to
+   * see if Phabricator is linked to applicable external systems.
+   *
+   * @return bool True if this worker should try to publish stories.
+   * @task publish
+   */
+  abstract public function isEnabled();
+
+
+/* -(  Story Context  )------------------------------------------------------ */
+
+
+  /**
+   * Get the @{class:PhabricatorFeedStory} that should be published.
+   *
+   * @return PhabricatorFeedStory The story to publish.
+   * @task context
+   */
+  protected function getFeedStory() {
+    if (!$this->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);
+  }
+
+}

src/applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php

@@ -1,120 +1,30 @@
 <?php
 
-final class DoorkeeperFeedWorkerAsana extends FeedPushWorker {
+/**
+ * Publishes tasks representing work that needs to be done into Asana, and
+ * updates the tasks as the corresponding Phabricator objects are updated.
+ */
+final class DoorkeeperFeedWorkerAsana extends DoorkeeperFeedWorker {
 
   private $provider;
-  private $publisher;
-  private $workspaceID;
-  private $feedStory;
-  private $storyObject;
 
-  private function getProvider() {
+
-    if (!$this->provider) {
+/* -(  Publishing Stories  )------------------------------------------------- */
-      $provider = PhabricatorAuthProviderOAuthAsana::getAsanaProvider();
+
-      if (!$provider) {
+
-        throw new PhabricatorWorkerPermanentFailureException(
+  /**
-          'No Asana provider configured.');
+   * This worker is enabled when an Asana workspace ID is configured with
-      }
+   * `asana.workspace-id`.
-      $this->provider = $provider;
+   */
-    }
+  public function isEnabled() {
-    return $this->provider;
+    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) {
+   * Publish stories into Asana using the Asana API.
-      $story = $this->loadFeedStory();
+   */
-      $this->feedStory = $story;
+  protected function publishFeedStory() {
-    }
-    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() {
     $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();
+    $publisher = $this->getPublisher();
-
-    $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;
-    }
 
     // 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);
   }
 
-
 }

src/applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php

@@ -1,89 +1,34 @@
 <?php
 
-final class DoorkeeperFeedWorkerJIRA extends FeedPushWorker {
+/**
+ * Publishes feed stories into JIRA, using the "JIRA Issues" field to identify
+ * linked issues.
+ */
+final class DoorkeeperFeedWorkerJIRA extends DoorkeeperFeedWorker {
 
   private $provider;
-  private $publisher;
-  private $workspaceID;
-  private $feedStory;
-  private $storyObject;
 
-  private function getProvider() {
+
-    if (!$this->provider) {
+/* -(  Publishing Stories  )------------------------------------------------- */
-      $provider = PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider();
+
-      if (!$provider) {
+
-        throw new PhabricatorWorkerPermanentFailureException(
+  /**
-          'No JIRA provider configured.');
+   * This worker is enabled when a JIRA authentication provider is active.
-      }
+   */
-      $this->provider = $provider;
+  public function isEnabled() {
-    }
+    return (bool)PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider();
-    return $this->provider;
   }
 
-  private function getFeedStory() {
-    if (!$this->feedStory) {
-      $story = $this->loadFeedStory();
-      $this->feedStory = $story;
-    }
-    return $this->feedStory;
-  }
 
-  private function getViewer() {
+  /**
-    return PhabricatorUser::getOmnipotentUser();
+   * Publishes stories into JIRA using the JIRA API.
-  }
+   */
-
+  protected function publishFeedStory() {
-  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() {
     $story = $this->getFeedStory();
-    $data = $story->getStoryData();
-
     $viewer = $this->getViewer();
     $provider = $this->getProvider();
-
     $object = $this->getStoryObject();
-    $src_phid = $object->getPHID();
+    $publisher = $this->getPublisher();
-
-    $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;
-    }
 
     $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<phid> Candidate user PHIDs to act as when publishing this
+   *                    story.
+   * @task internal
+   */
   private function findUsersToPossess() {
     $object = $this->getStoryObject();
     $publisher = $this->getPublisher();

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(),
-        ));
-    }
-
   }
 
 

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;