Make subscribe/unsubscribe work properly on Revisions.

Summary:

Test Plan:

Reviewers:

CC:

src/__phutil_library_map__.php

@@ -138,6 +138,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionStatus' => 'applications/differential/constants/revisionstatus',
     'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/revisionupdatehistory',
     'DifferentialRevisionViewController' => 'applications/differential/controller/revisionview',
+    'DifferentialSubscribeController' => 'applications/differential/controller/subscribe',
     'DifferentialUnitStatus' => 'applications/differential/constants/unitstatus',
     'Javelin' => 'infrastructure/javelin/api',
     'LiskDAO' => 'storage/lisk/dao',
@@ -149,7 +150,6 @@ phutil_register_library_map(array(
     'ManiphestTaskListController' => 'applications/maniphest/controller/tasklist',
     'ManiphestTaskListView' => 'applications/maniphest/view/tasklist',
     'ManiphestTaskPriority' => 'applications/maniphest/constants/priority',
-    'ManiphestTaskSelectorController' => 'applications/maniphest/controller/taskselector',
     'ManiphestTaskSelectorSearchController' => 'applications/maniphest/controller/taskselectorsearch',
     'ManiphestTaskStatus' => 'applications/maniphest/constants/status',
     'ManiphestTaskSummaryView' => 'applications/maniphest/view/tasksummary',
@@ -382,6 +382,7 @@ phutil_register_library_map(array(
     'DifferentialRevisionListController' => 'DifferentialController',
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',
     'DifferentialRevisionViewController' => 'DifferentialController',
+    'DifferentialSubscribeController' => 'DifferentialController',
     'ManiphestController' => 'PhabricatorController',
     'ManiphestDAO' => 'PhabricatorLiskDAO',
     'ManiphestTask' => 'ManiphestDAO',
@@ -389,7 +390,6 @@ phutil_register_library_map(array(
     'ManiphestTaskDetailController' => 'ManiphestController',
     'ManiphestTaskListController' => 'ManiphestController',
     'ManiphestTaskListView' => 'AphrontView',
-    'ManiphestTaskSelectorController' => 'ManiphestController',
     'ManiphestTaskSelectorSearchController' => 'ManiphestController',
     'ManiphestTaskSummaryView' => 'AphrontView',
     'ManiphestTransaction' => 'ManiphestDAO',

src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php

@@ -93,6 +93,8 @@ class AphrontDefaultApplicationConfiguration
           ),
         ),
         'attach/(?P<id>\d+)/(?P<type>\w+)/$' => 'DifferentialAttachController',
+        'subscribe/(?P<action>add|rem)/(?P<id>\d+)/$'
+          => 'DifferentialSubscribeController',
       ),
 
       '/res/' => array(
@@ -144,7 +146,6 @@ class AphrontDefaultApplicationConfiguration
         'transaction/' => array(
           'save/' => 'ManiphestTransactionSaveController',
         ),
-        'select/$' => 'ManiphestTaskSelectorController',
         'select/search/$' => 'ManiphestTaskSelectorSearchController',
       ),
 
@@ -152,6 +153,7 @@ class AphrontDefaultApplicationConfiguration
 
       '/github-post-receive/(?P<id>\d+)/(?P<token>[^/]+)/$'
         => 'PhabricatorRepositoryGitHubPostReceiveController',
+
       '/repository/' => array(
         '$'                     => 'PhabricatorRepositoryListController',
         'create/$'              => 'PhabricatorRepositoryCreateController',

src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php

@@ -271,6 +271,7 @@ class DifferentialRevisionViewController extends DifferentialController {
         'class' => $viewer_is_cc ? 'subscribe-rem' : 'subscribe-add',
         'href'  => "/differential/subscribe/{$action}/{$revision_id}/",
         'name'  => $viewer_is_cc ? 'Unsubscribe' : 'Subscribe',
+        'sigil' => 'workflow',
       );
     } else {
       $links[] = array(
@@ -436,79 +437,6 @@ class DifferentialRevisionViewController extends DifferentialController {
 /*
 
 
-  protected function getRevisionActions(DifferentialRevision $revision) {
-
-    $viewer_id = $this->getRequest()->getViewerContext()->getUserID();
-    $viewer_is_owner = ($viewer_id == $revision->getOwnerID());
-    $viewer_is_reviewer =
-      ((array_search($viewer_id, $revision->getReviewers())) !== false);
-    $viewer_is_cc =
-      ((array_search($viewer_id, $revision->getCCFBIDs())) !== false);
-    $status = $revision->getStatus();
-
-    $links = array();
-
-    if (!$viewer_is_owner && !$viewer_is_reviewer) {
-      $action = $viewer_is_cc
-        ? 'rem'
-        : 'add';
-      $revision_id = $revision->getID();
-      $href = "/differential/subscribe/{$action}/{$revision_id}";
-      $links[] = array(
-        $viewer_is_cc ? 'subscribe-disabled' : 'subscribe-enabled',
-        <a href={$href}>{$viewer_is_cc ? 'Unsubscribe' : 'Subscribe'}</a>,
-      );
-    } else {
-      $links[] = array(
-        'subscribe-disabled unavailable',
-        <a>Automatically Subscribed</a>,
-      );
-    }
-
-    $blast_uri = RedirectURI(
-      '/intern/differential/?action=tasks&fbid='.$revision->getFBID())
-      ->setTier('intern');
-    $links[] = array(
-      'tasks',
-      <a href={$blast_uri}>Edit Tasks</a>,
-    );
-
-    $engineering_repository_id = RepositoryRef::getByCallsign('E')->getID();
-    $svn_revision = $revision->getSVNRevision();
-    if ($status == DifferentialConstants::COMMITTED &&
-        $svn_revision &&
-        $revision->getRepositoryID() == $engineering_repository_id) {
-      $href = '/intern/push/request.php?rev='.$svn_revision;
-      $href = RedirectURI($href)->setTier('intern');
-      $links[] = array(
-        'merge',
-        <a href={$href} id="ask_for_merge_link">Ask for Merge</a>,
-      );
-    }
-
-    $links[] = array(
-      'herald-transcript',
-      <a href={"/herald/transcript/?fbid=".$revision->getFBID()}
-        >Herald Transcripts</a>,
-    );
-    $links[] = array(
-      'metamta-transcript',
-      <a href={"/mail/?view=all&fbid=".$revision->getFBID()}
-        >MetaMTA Transcripts</a>,
-    );
-
-
-    $list = <ul class="differential-actions" />;
-    foreach ($links as $link) {
-      list($class, $tag) = $link;
-      $list->appendChild(<li class={$class}>{$tag}</li>);
-    }
-
-    return $list;
-
-
-
-
   protected function getSandcastleURI(Diff $diff) {
     $uri = $this->getDiffProperty($diff, 'facebook:sandcastle_uri');
     if (!$uri) {
@@ -561,8 +489,6 @@ class DifferentialRevisionViewController extends DifferentialController {
     }
 
 
-    $changesets = array_psort($changesets, 'getSortKey');
-
 
     $feedback = id(new DifferentialFeedback())->loadAllWithRevision($revision);
     $feedback = array_merge($implied_feedback, $feedback);
@@ -584,94 +510,11 @@ class DifferentialRevisionViewController extends DifferentialController {
       $hidden_changesets[$id] = $diff_map[$changeset->getDiffID()];
     }
 
-    $revision->loadRelationships();
-    $ccs = $revision->getCCFBIDs();
-    $reviewers = $revision->getReviewers();
-
-    $actors = array_pull($feedback, 'getUserID');
-    $actors[] = $revision->getOwnerID();
-
-    $tasks = array();
-    assoc_get_by_type(
-      $revision->getFBID(),
-      22284182462, // TODO: include issue, DIFFCAMP_TASK_ASSOC
-      $start = null,
-      $limit = null,
-      $pending = true,
-      $tasks);
-    memcache_dispatch();
-    $tasks = array_keys($tasks);
-
-    $preparer = new Preparer();
-      $fbids = array_merge_fast(
-        array($actors, array($viewer_id), $reviewers, $ccs, $tasks),
-        true);
-      $handles = array();
-      $handle_data = id(new ToolsHandleData($fbids, $handles))
-        ->needNames()
-        ->needAlternateNames()
-        ->needAlternateIDs()
-        ->needThumbnails();
-      $preparer->waitFor($handle_data);
-    $preparer->go();
-
-    $revision->attachTaskHandles(array_select_keys($handles, $tasks));
-
-    $inline_comments = array_group($inline_comments, 'getFeedbackID');
 
     $engine = new RemarkupEngine();
     $engine->enableFeature(RemarkupEngine::FEATURE_GUESS_IMAGES);
     $engine->enableFeature(RemarkupEngine::FEATURE_YOUTUBE);
     $engine->setCurrentSandcastle($this->getSandcastleURI($target_diff));
-    $feed = array();
-    foreach ($feedback as $comment) {
-      $inlines = null;
-      if (isset($inline_comments[$comment->getID()])) {
-        $inlines = $inline_comments[$comment->getID()];
-      }
-      $feed[] =
-        <differential:feedback
-            feedback={$comment}
-              handle={$handles[$comment->getUserID()]}
-              engine={$engine}
-              inline={$inlines}
-          changesets={$changesets}
-              hidden={$hidden_changesets} />;
-    }
-
-    $feed = $this->renderFeedbackList($feed, $feedback, $viewer_id);
-
-    $fields = $this->getDetailFields($revision, $diff, $handles);
-    $table = <table class="differential-revision-properties" />;
-    foreach ($fields as $key => $value) {
-      $table->appendChild(
-        <tr>
-          <th>{$key}:</th><td>{$value}</td>
-        </tr>);
-    }
-
-    $quick_links = $this->getQuickLinks($revision);
-
-
-    $info =
-      <div class="differential-revision-information">
-        <div class="differential-revision-actions">
-          {$quick_links}
-        </div>
-        <div class="differential-revision-detail">
-          <h1>{$revision->getName()}{$edit_link}</h1>
-          {$table}
-        </div>
-      </div>;
-
-    $actions = $this->getRevisionActions($revision);
-    $revision_id = $revision->getID();
-
-    $content = SavedCopy::loadData(
-      $viewer_id,
-      SavedCopy::Type_DifferentialRevisionFeedback,
-      $revision->getFBID());
-
 
     $syntax_link =
       <a href={'http://www.intern.facebook.com/intern/wiki/index.php' .
@@ -690,34 +533,6 @@ class DifferentialRevisionViewController extends DifferentialController {
         </tools:notice>;
     }
 
-  protected function getQuickLinks(DifferentialRevision $revision) {
-
-    $viewer_id = $this->getRequest()->getViewerContext()->getUserID();
-    $viewer_is_owner = ($viewer_id == $revision->getOwnerID());
-    $viewer_is_reviewer =
-      ((array_search($viewer_id, $revision->getReviewers())) !== false);
-    $viewer_is_cc =
-      ((array_search($viewer_id, $revision->getCCFBIDs())) !== false);
-    $status = $revision->getStatus();
-
-    $links = array();
-
-    if (!$viewer_is_owner && !$viewer_is_reviewer) {
-      $action = $viewer_is_cc
-        ? 'rem'
-        : 'add';
-      $revision_id = $revision->getID();
-      $href = "/differential/subscribe/{$action}/{$revision_id}";
-      $links[] = array(
-        $viewer_is_cc ? 'subscribe-disabled' : 'subscribe-enabled',
-        <a href={$href}>{$viewer_is_cc ? 'Unsubscribe' : 'Subscribe'}</a>,
-      );
-    } else {
-      $links[] = array(
-        'subscribe-disabled unavailable',
-        <a>Automatically Subscribed</a>,
-      );
-    }
 
     $blast_uri = RedirectURI(
       '/intern/differential/?action=blast&fbid='.$revision->getFBID())
@@ -727,23 +542,7 @@ class DifferentialRevisionViewController extends DifferentialController {
       <a href={$blast_uri}>Blast Revision</a>,
     );
 
-    $blast_uri = RedirectURI(
-      '/intern/differential/?action=tasks&fbid='.$revision->getFBID())
-      ->setTier('intern');
-    $links[] = array(
-      'tasks',
-      <a href={$blast_uri}>Edit Tasks</a>,
-    );
 
-    if ($viewer_is_owner && false) {
-      $perflab_uri = RedirectURI(
-        '/intern/differential/?action=perflab&fbid='.$revision->getFBID())
-        ->setTier('intern');
-      $links[] = array(
-        'perflab',
-        <a href={$perflab_uri}>Run in Perflab</a>,
-      );
-    }
 
     $engineering_repository_id = RepositoryRef::getByCallsign('E')->getID();
     $svn_revision = $revision->getSVNRevision();
@@ -763,20 +562,7 @@ class DifferentialRevisionViewController extends DifferentialController {
       <a href={"/herald/transcript/?fbid=".$revision->getFBID()}
         >Herald Transcripts</a>,
     );
-    $links[] = array(
-      'metamta-transcript',
-      <a href={"/mail/?view=all&fbid=".$revision->getFBID()}
-        >MetaMTA Transcripts</a>,
-    );
 
-
-    $list = <ul class="differential-actions" />;
-    foreach ($links as $link) {
-      list($class, $tag) = $link;
-      $list->appendChild(<li class={$class}>{$tag}</li>);
-    }
-
-    return $list;
   }
 
 
@@ -875,10 +661,6 @@ class DifferentialRevisionViewController extends DifferentialController {
     Diff $diff,
     array $handles) {
 
-    $fields = array();
-    $fields['Revision Status'] = $this->getRevisionStatusDisplay($revision);
-
-
     $sandcastle = $this->getSandcastleURI($diff);
     if ($sandcastle) {
       $fields['Sandcastle'] = <a href={$sandcastle}>{$sandcastle}</a>;
@@ -898,15 +680,6 @@ class DifferentialRevisionViewController extends DifferentialController {
       }
     }
 
-    $tasks = $revision->getTaskHandles();
-
-    if ($tasks) {
-      $links = array();
-      foreach ($tasks as $task) {
-        $links[] = <tools:handle handle={$task} link={true} />;
-      }
-      $fields['Tasks'] = array_implode(<br />, $links);
-    }
 
     $bugzilla_id = $revision->getBugzillaID();
     if ($bugzilla_id) {

src/applications/differential/controller/subscribe/DifferentialSubscribeController.php

@@ -0,0 +1,106 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+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 DifferentialRevision())->load($this->id);
+    if (!$revision) {
+      return new Aphront404Response();
+    }
+
+    if (!$request->isFormPost()) {
+      // TODO: This dialog is silly but we're CSRF-able otherwise.
+
+      $dialog = new AphrontDialogView();
+
+      switch ($this->action) {
+        case 'add':
+          $button = 'Subscribe';
+          $title = 'Subscribe to Revision';
+          $prompt = 'Really subscribe to this revision?';
+          break;
+        case 'rem':
+          $button = 'Unsubscribe';
+          $title = 'Unsubscribe from Revision';
+          // TODO: Once herald is in, add a notice about not getting any more
+          // herald notifications.
+          $prompt = 'Really unsubscribe from this revision?';
+          break;
+        default:
+          return new Aphront400Response();
+      }
+
+      $dialog
+        ->setUser($user)
+        ->setTitle($title)
+        ->appendChild('<p>'.$prompt.'</p>')
+        ->setSubmitURI($request->getRequestURI())
+        ->addSubmitButton($button)
+        ->addCancelButton('/D'.$revision->getID());
+
+      return id(new AphrontDialogResponse())->setDialog($dialog);
+    }
+
+    $revision->loadRelationships();
+    $phid = $user->getPHID();
+
+    switch ($this->action) {
+      case 'add':
+        DifferentialRevisionEditor::addCC(
+          $revision,
+          $phid,
+          $phid);
+        $unsubscribed = $revision->getUnsubscribed();
+        if (isset($unsubscribed[$phid])) {
+          unset($unsubscribed[$phid]);
+          $revision->setUnsubscribed($unsubscribed);
+          $revision->save();
+        }
+        break;
+      case 'rem':
+        DifferentialRevisionEditor::removeCC(
+          $revision,
+          $user->getPHID(),
+          $user->getPHID());
+        $unsubscribed = $revision->getUnsubscribed();
+        if (empty($unsubscribed[$phid])) {
+          $unsubscribed[$phid] = true;
+          $revision->setUnsubscribed($unsubscribed);
+          $revision->save();
+        }
+        break;
+      default:
+        return new Aphront400Response();
+    }
+
+    return id(new AphrontRedirectResponse())->setURI('/D'.$revision->getID());
+  }
+}

src/applications/differential/controller/subscribe/__init__.php

@@ -0,0 +1,21 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'aphront/response/400');
+phutil_require_module('phabricator', 'aphront/response/404');
+phutil_require_module('phabricator', 'aphront/response/dialog');
+phutil_require_module('phabricator', 'aphront/response/redirect');
+phutil_require_module('phabricator', 'applications/differential/controller/base');
+phutil_require_module('phabricator', 'applications/differential/editor/revision');
+phutil_require_module('phabricator', 'applications/differential/storage/revision');
+phutil_require_module('phabricator', 'view/dialog');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('DifferentialSubscribeController.php');

src/applications/differential/editor/revision/DifferentialRevisionEditor.php

@@ -471,6 +471,30 @@ class DifferentialRevisionEditor {
     }
   }
 
+  public static function addCC(
+    DifferentialRevision $revision,
+    $phid,
+    $reason) {
+    return self::alterCCs(
+      $revision,
+      $revision->getCCPHIDs(),
+      $rem = array(),
+      $add = array($phid),
+      $reason);
+  }
+
+  public static function removeCC(
+    DifferentialRevision $revision,
+    $phid,
+    $reason) {
+    return self::alterCCs(
+      $revision,
+      $revision->getCCPHIDs(),
+      $rem = array($phid),
+      $add = array(),
+      $reason);
+  }
+
   protected static function alterCCs(
     DifferentialRevision $revision,
     array $stable_phids,

src/applications/differential/storage/revision/DifferentialRevision.php

@@ -33,6 +33,7 @@ class DifferentialRevision extends DifferentialDAO {
 
   protected $lineCount;
   protected $attached = array();
+  protected $unsubscribed = array();
 
   private $relationships;
 
@@ -40,13 +41,13 @@ class DifferentialRevision extends DifferentialDAO {
 
   const RELATION_REVIEWER     = 'revw';
   const RELATION_SUBSCRIBED   = 'subd';
-  const RELATION_UNSUBSCRIBED = 'usub';
 
   public function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_SERIALIZATION => array(
         'attached'      => self::SERIALIZATION_JSON,
+        'unsubscribed'  => self::SERIALIZATION_JSON,
       ),
     ) + parent::getConfiguration();
   }

src/applications/maniphest/controller/taskselector/ManiphestTaskSelectorController.php

@@ -1,50 +0,0 @@
-<?php
-
-/*
- * Copyright 2011 Facebook, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-class ManiphestTaskSelectorController extends ManiphestController {
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    $phids = $request->getArr('phids');
-
-    $handles = id(new PhabricatorObjectHandleData($phids))
-      ->loadHandles();
-
-    $obj_dialog = new PhabricatorObjectSelectorDialog();
-    $obj_dialog
-      ->setUser($user)
-      ->setHandles($handles)
-      ->setFilters(array(
-        'assigned' => 'Assigned to Me',
-        'created'  => 'Created By Me',
-        'open'     => 'All Open Tasks',
-        'all'      => 'All Tasks',
-      ))
-      ->setCancelURI('#')
-      ->setSearchURI('/maniphest/select/search/')
-      ->setNoun('Tasks');
-
-    $dialog = $obj_dialog->buildDialog();
-
-
-    return id(new AphrontDialogResponse())->setDialog($dialog);
-  }
-
-}

src/applications/maniphest/controller/taskselector/__init__.php

@@ -1,17 +0,0 @@
-<?php
-/**
- * This file is automatically generated. Lint this module to rebuild it.
- * @generated
- */
-
-
-
-phutil_require_module('phabricator', 'aphront/response/dialog');
-phutil_require_module('phabricator', 'applications/maniphest/controller/base');
-phutil_require_module('phabricator', 'applications/phid/handle/data');
-phutil_require_module('phabricator', 'view/control/objectselector');
-
-phutil_require_module('phutil', 'utils');
-
-
-phutil_require_source('ManiphestTaskSelectorController.php');

webroot/rsrc/css/aphront/dialog-view.css

@@ -44,10 +44,10 @@
 }
 
 .jx-mask {
-  opacity:    .5;
+  opacity:    .75;
   -ms-filter: "progid:DXImageTransform.Microsoft.Alpha(Opacity=75)";
   filter:     alpha(opacity=75);
-  background: #999;
+  background: #222;
   position:   absolute;
   z-index:    5;
   top:        0;

webroot/rsrc/css/application/differential/revision-detail.css

@@ -55,6 +55,10 @@
   background-image: url(/rsrc/image/icon/unsubscribe.png);
 }
 
+.differential-revision-actions .subscribe-add {
+  background-image: url(/rsrc/image/icon/subscribe.png);
+}
+
 .differential-revision-actions .revision-edit {
   background-image: url(/rsrc/image/icon/tango/edit.png);
 }

webroot/rsrc/css/core/buttons.css

@@ -143,6 +143,12 @@ button.link {
   padding: 0;
   margin: 0;
   font-size: inherit;
+  border-bottom: none;
+  text-decoration: none;
+  color: #3b5998;
+  -webkit-box-shadow: none;
+  -moz-box-shadow: none;
+  box-shadow: none;
 }
 
 button.link:hover {