Use standard rendering and controller for Differential subscriptions

Summary:
Ref T2222. Differential has custom code for managing subscriptions, but no longer requires it.

The one trick is that we don't have a hook for loading related data on the subscriptions workflow right now. Just glue that in for the moment; it's relatively harmless, and once Diffusion converts we'll have more context on how to best surface it properly.

Test Plan: Subscribed and unsubscribed from a revision. Viewed different revisions and saw correct subscription state.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8293

src/__phutil_library_map__.php

@@ -464,7 +464,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php',
     'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php',
     'DifferentialSearchIndexer' => 'applications/differential/search/DifferentialSearchIndexer.php',
-    'DifferentialSubscribeController' => 'applications/differential/controller/DifferentialSubscribeController.php',
     'DifferentialSubscribersField' => 'applications/differential/customfield/DifferentialSubscribersField.php',
     'DifferentialSummaryField' => 'applications/differential/customfield/DifferentialSummaryField.php',
     'DifferentialSummaryFieldSpecification' => 'applications/differential/field/specification/DifferentialSummaryFieldSpecification.php',
@@ -3014,7 +3013,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',
     'DifferentialRevisionViewController' => 'DifferentialController',
     'DifferentialSearchIndexer' => 'PhabricatorSearchDocumentIndexer',
-    'DifferentialSubscribeController' => 'DifferentialController',
     'DifferentialSubscribersField' => 'DifferentialCoreCustomField',
     'DifferentialSummaryField' => 'DifferentialCoreCustomField',
     'DifferentialSummaryFieldSpecification' => 'DifferentialFreeformFieldSpecification',

src/applications/differential/application/PhabricatorApplicationDifferential.php

@@ -63,8 +63,6 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication {
               => 'DifferentialInlineCommentEditController',
           ),
         ),
-        'subscribe/(?P<action>add|rem)/(?P<id>[1-9]\d*)/'
-          => 'DifferentialSubscribeController',
         'preview/' => 'PhabricatorMarkupPreviewController',
       ),
     );

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -459,23 +459,17 @@ final class DifferentialRevisionViewController extends DifferentialController {
   }
 
   private function getRevisionActions(DifferentialRevision $revision) {
-    $user = $this->getRequest()->getUser();
+    $viewer = $this->getRequest()->getUser();
-    $viewer_phid = $user->getPHID();
-    $viewer_is_owner = ($revision->getAuthorPHID() == $viewer_phid);
-    $viewer_is_reviewer = in_array($viewer_phid, $revision->getReviewers());
-    $viewer_is_cc = in_array($viewer_phid, $revision->getCCPHIDs());
-    $logged_in = $this->getRequest()->getUser()->isLoggedIn();
-    $status = $revision->getStatus();
     $revision_id = $revision->getID();
     $revision_phid = $revision->getPHID();
 
-    $links = array();
-
     $can_edit = PhabricatorPolicyFilter::hasCapability(
-      $user,
+      $viewer,
       $revision,
       PhabricatorPolicyCapability::CAN_EDIT);
 
+    $links = array();
+
     $links[] = array(
       'icon'  =>  'edit',
       'href'  => "/differential/revision/edit/{$revision_id}/",
@@ -484,24 +478,6 @@ final class DifferentialRevisionViewController extends DifferentialController {
       'sigil' => $can_edit ? null : 'workflow',
     );
 
-    if (!$viewer_is_owner && !$viewer_is_reviewer) {
-      $action = $viewer_is_cc ? 'rem' : 'add';
-      $links[] = array(
-        'icon'    => $viewer_is_cc ? 'disable' : 'check',
-        'href'    => "/differential/subscribe/{$action}/{$revision_id}/",
-        'name'    => $viewer_is_cc ? pht('Unsubscribe') : pht('Subscribe'),
-        'instant' => $logged_in,
-        'disabled' => !$logged_in,
-        'sigil' => $can_edit ? null : 'workflow',
-      );
-    } else {
-      $links[] = array(
-        'icon'     => 'enable',
-        'name'     => pht('Automatically Subscribed'),
-        'disabled' => true,
-      );
-    }
-
     $this->requireResource('phabricator-object-selector-css');
     $this->requireResource('javelin-behavior-phabricator-object-selector');
 

src/applications/differential/controller/DifferentialSubscribeController.php

@@ -1,80 +0,0 @@
-<?php
-
-final class DifferentialSubscribeController extends DifferentialController {
-
-  private $id;
-  private $action;
-
-  public function willProcessRequest(array $data) {
-    $this->id = $data['id'];
-    $this->action = $data['action'];
-  }
-
-  public function processRequest() {
-
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    $revision = id(new DifferentialRevisionQuery())
-      ->withIDs(array($this->id))
-      ->setViewer($request->getUser())
-      ->needRelationships(true)
-      ->needReviewerStatus(true)
-      ->executeOne();
-    if (!$revision) {
-      return new Aphront404Response();
-    }
-
-    if (!$request->isFormPost()) {
-      $dialog = new AphrontDialogView();
-
-      switch ($this->action) {
-        case 'add':
-          $button = pht('Subscribe');
-          $title = pht('Subscribe to Revision');
-          $prompt = pht('Really subscribe to this revision?');
-          break;
-        case 'rem':
-          $button = pht('Unsubscribe');
-          $title = pht('Unsubscribe from Revision');
-          $prompt = pht('Really unsubscribe from this revision? Herald will '.
-                    'not resubscribe you to a revision you unsubscribe '.
-                    'from.');
-          break;
-        default:
-          return new Aphront400Response();
-      }
-
-      $dialog
-        ->setUser($user)
-        ->setTitle($title)
-        ->appendChild(phutil_tag('p', array(), $prompt))
-        ->setSubmitURI($request->getRequestURI())
-        ->addSubmitButton($button)
-        ->addCancelButton('/D'.$revision->getID());
-
-      return id(new AphrontDialogResponse())->setDialog($dialog);
-    }
-
-    $phid = $user->getPHID();
-
-    switch ($this->action) {
-      case 'add':
-        DifferentialRevisionEditor::addCCAndUpdateRevision(
-          $revision,
-          $phid,
-          $user);
-        break;
-      case 'rem':
-        DifferentialRevisionEditor::removeCCAndUpdateRevision(
-          $revision,
-          $phid,
-          $user);
-        break;
-      default:
-        return new Aphront400Response();
-    }
-
-    return id(new AphrontRedirectResponse())->setURI('/D'.$revision->getID());
-  }
-}

src/applications/differential/storage/DifferentialRevision.php

@@ -444,20 +444,41 @@ final class DifferentialRevision extends DifferentialDAO
 
 
   public function isAutomaticallySubscribed($phid) {
-    // TODO: Reviewers are also automatically subscribed, but that data may
+    if ($phid == $this->getAuthorPHID()) {
-    // not always be loaded, so be conservative for now. All the editing code
+      return true;
-    // has checks around this already.
+    }
-    return ($phid == $this->getAuthorPHID());
+
+    // TODO: This only happens when adding or removing CCs, and is safe from a
+    // policy perspective, but the subscription pathway should have some
+    // opportunity to load this data properly. For now, this is the only case
+    // where implicit subscription is not an intrinsic property of the object.
+    if ($this->reviewerStatus == self::ATTACHABLE) {
+      $reviewers = id(new DifferentialRevisionQuery())
+        ->setViewer(PhabricatorUser::getOmnipotentUser())
+        ->withPHIDs(array($this->getPHID()))
+        ->needReviewerStatus(true)
+        ->executeOne()
+        ->getReviewerStatus();
+    } else {
+      $reviewers = $this->getReviewerStatus();
+    }
+
+    foreach ($reviewers as $reviewer) {
+      if ($reviewer->getReviewerPHID() == $phid) {
+        return true;
+      }
+    }
+
+    return false;
   }
 
   public function shouldShowSubscribersProperty() {
-    // TODO: For now, Differential has its own stuff.
+    // TODO: Differential does its own thing for now.
     return false;
   }
 
   public function shouldAllowSubscription($phid) {
-    // TODO: For now, Differential has its own stuff.
+    return true;
-    return false;
   }