Allow any user to watch any project they can see

Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.

Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.

Test Plan:
  - Watched a project I was not a member of.
  - Clicked the feed watch/unwatch button.

{F1064909}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6183, T10054

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

src/applications/badges/storage/PhabricatorBadgesBadge.php

@@ -185,10 +185,6 @@ final class PhabricatorBadgesBadge extends PhabricatorBadgesDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 

src/applications/calendar/storage/PhabricatorCalendarEvent.php

@@ -535,10 +535,6 @@ final class PhabricatorCalendarEvent extends PhabricatorCalendarDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 
 

src/applications/countdown/storage/PhabricatorCountdown.php

@@ -74,10 +74,6 @@ final class PhabricatorCountdown extends PhabricatorCountdownDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
 

src/applications/differential/storage/DifferentialRevision.php

@@ -485,10 +485,6 @@ final class DifferentialRevision extends DifferentialDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorCustomFieldInterface  )------------------------------------ */
 

src/applications/files/storage/PhabricatorFile.php

@@ -1352,10 +1352,6 @@ final class PhabricatorFile extends PhabricatorFileDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 

src/applications/fund/storage/FundInitiative.php

@@ -182,10 +182,6 @@ final class FundInitiative extends FundDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenRecevierInterface  )---------------------------------- */
 

src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php

@@ -129,10 +129,6 @@ final class HarbormasterBuildPlan extends HarbormasterDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

src/applications/herald/storage/HeraldRule.php

@@ -332,10 +332,6 @@ final class HeraldRule extends HeraldDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 

src/applications/legalpad/storage/LegalpadDocument.php

@@ -167,10 +167,6 @@ final class LegalpadDocument extends LegalpadDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

src/applications/macro/storage/PhabricatorFileImageMacro.php

@@ -115,10 +115,6 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenRecevierInterface  )---------------------------------- */
 

src/applications/maniphest/storage/ManiphestTask.php

@@ -227,10 +227,6 @@ final class ManiphestTask extends ManiphestDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  Markup Interface  )--------------------------------------------------- */
 

src/applications/passphrase/storage/PassphraseCredential.php

@@ -161,10 +161,6 @@ final class PassphraseCredential extends PassphraseDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 

src/applications/paste/storage/PhabricatorPaste.php

@@ -159,10 +159,6 @@ final class PhabricatorPaste extends PhabricatorPasteDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 

src/applications/phame/storage/PhameBlog.php

@@ -340,10 +340,6 @@ final class PhameBlog extends PhameDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorConduitResultInterface  )---------------------------------- */
 

src/applications/phame/storage/PhamePost.php

@@ -286,10 +286,6 @@ final class PhamePost extends PhameDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorConduitResultInterface  )---------------------------------- */
 

src/applications/pholio/storage/PholioMock.php

@@ -188,10 +188,6 @@ final class PholioMock extends PholioDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorPolicyInterface Implementation  )-------------------------- */
 

src/applications/phriction/storage/PhrictionDocument.php

@@ -198,10 +198,6 @@ final class PhrictionDocument extends PhrictionDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
 

src/applications/phurl/storage/PhabricatorPhurlURL.php

@@ -173,10 +173,6 @@ final class PhabricatorPhurlURL extends PhabricatorPhurlDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 
 

src/applications/ponder/storage/PonderAnswer.php

@@ -253,10 +253,6 @@ final class PonderAnswer extends PonderDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorDestructibleInterface  )----------------------------------- */
 

src/applications/ponder/storage/PonderQuestion.php

@@ -252,10 +252,6 @@ final class PonderQuestion extends PonderDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 

src/applications/project/controller/PhabricatorProjectMembersViewController.php

@@ -152,22 +152,22 @@ final class PhabricatorProjectMembersViewController
           ->setDisabled(!$can_leave)
           ->setWorkflow(true)
           ->setName(pht('Leave Project')));
+    }
 
-      if (!$project->isUserWatcher($viewer->getPHID())) {
+    if (!$project->isUserWatcher($viewer->getPHID())) {
-        $view->addAction(
+      $view->addAction(
-          id(new PhabricatorActionView())
+        id(new PhabricatorActionView())
-            ->setWorkflow(true)
+          ->setWorkflow(true)
-            ->setHref('/project/watch/'.$project->getID().'/')
+          ->setHref('/project/watch/'.$project->getID().'/')
-            ->setIcon('fa-eye')
+          ->setIcon('fa-eye')
-            ->setName(pht('Watch Project')));
+          ->setName(pht('Watch Project')));
-      } else {
+    } else {
-        $view->addAction(
+      $view->addAction(
-          id(new PhabricatorActionView())
+        id(new PhabricatorActionView())
-            ->setWorkflow(true)
+          ->setWorkflow(true)
-            ->setHref('/project/unwatch/'.$project->getID().'/')
+          ->setHref('/project/unwatch/'.$project->getID().'/')
-            ->setIcon('fa-eye-slash')
+          ->setIcon('fa-eye-slash')
-            ->setName(pht('Unwatch Project')));
+          ->setName(pht('Unwatch Project')));
-      }
     }
 
     $can_add = $can_edit && $supports_edit;

src/applications/project/controller/PhabricatorProjectProfileController.php

@@ -52,6 +52,7 @@ final class PhabricatorProjectProfileController
     $nav = $this->getProfileMenu();
     $nav->selectFilter(PhabricatorProject::PANEL_PROFILE);
 
+    $watch_action = $this->renderWatchAction($project);
 
     $stories = id(new PhabricatorFeedQuery())
       ->setViewer($viewer)
@@ -62,10 +63,15 @@ final class PhabricatorProjectProfileController
       ->setLimit(50)
       ->execute();
 
+
     $feed = $this->renderStories($stories);
 
+    $feed_header = id(new PHUIHeaderView())
+      ->setHeader(pht('Recent Activity'))
+      ->addActionLink($watch_action);
+
     $feed = id(new PHUIObjectBoxView())
-      ->setHeaderText(pht('Recent Activity'))
+      ->setHeader($feed_header)
       ->appendChild($feed);
 
     $columns = id(new AphrontMultiColumnView())
@@ -144,4 +150,33 @@ final class PhabricatorProjectProfileController
     return phutil_tag_div('profile-feed', $view->render());
   }
 
+  private function renderWatchAction(PhabricatorProject $project) {
+    $viewer = $this->getViewer();
+    $viewer_phid = $viewer->getPHID();
+    $id = $project->getID();
+
+    $is_watcher = ($viewer_phid && $project->isUserWatcher($viewer_phid));
+
+    if (!$is_watcher) {
+      $watch_icon = 'fa-eye';
+      $watch_text = pht('Watch Project');
+      $watch_href = "/project/watch/{$id}/?via=profile";
+    } else {
+      $watch_icon = 'fa-eye-slash';
+      $watch_text = pht('Unwatch Project');
+      $watch_href = "/project/unwatch/{$id}/?via=profile";
+    }
+
+    $watch_icon = id(new PHUIIconView())
+      ->setIconFont($watch_icon);
+
+    return id(new PHUIButtonView())
+      ->setTag('a')
+      ->setWorkflow(true)
+      ->setIcon($watch_icon)
+      ->setText($watch_text)
+      ->setHref($watch_href);
+  }
+
+
 }

src/applications/project/controller/PhabricatorProjectWatchController.php

@@ -18,11 +18,11 @@ final class PhabricatorProjectWatchController
       return new Aphront404Response();
     }
 
-    $done_uri = "/project/members/{$id}/";
+    $via = $request->getStr('via');
-
+    if ($via == 'profile') {
-    // You must be a member of a project to watch it.
+      $done_uri = $project->getURI();
-    if (!$project->isUserMember($viewer->getPHID())) {
+    } else {
-      return new Aphront400Response();
+      $done_uri = "/project/members/{$id}/";
     }
 
     if ($request->isDialogFormPost()) {
@@ -38,7 +38,7 @@ final class PhabricatorProjectWatchController
           break;
       }
 
-      $type_member = PhabricatorObjectHasWatcherEdgeType::EDGECONST;
+      $type_watcher = PhabricatorObjectHasWatcherEdgeType::EDGECONST;
       $member_spec = array(
         $edge_action => array($viewer->getPHID() => $viewer->getPHID()),
       );
@@ -46,7 +46,7 @@ final class PhabricatorProjectWatchController
       $xactions = array();
       $xactions[] = id(new PhabricatorProjectTransaction())
         ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-        ->setMetadataValue('edge:type', $type_member)
+        ->setMetadataValue('edge:type', $type_watcher)
         ->setNewValue($member_spec);
 
       $editor = id(new PhabricatorProjectTransactionEditor($project))
@@ -82,6 +82,7 @@ final class PhabricatorProjectWatchController
 
     return $this->newDialog()
       ->setTitle($title)
+      ->addHiddenInput('via', $via)
       ->appendParagraph($body)
       ->addCancelButton($done_uri)
       ->addSubmitButton($submit);

src/applications/project/editor/PhabricatorProjectTransactionEditor.php

@@ -188,21 +188,18 @@ final class PhabricatorProjectTransactionEditor
         switch ($edge_type) {
           case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST:
           case PhabricatorObjectHasWatcherEdgeType::EDGECONST:
+            $edge_const = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST;
+            if ($edge_type != $edge_const) {
+              break;
+            }
+
             $old = $xaction->getOldValue();
             $new = $xaction->getNewValue();
 
-            // When adding members or watchers, we add subscriptions.
+            // When adding members, we add subscriptions. When removing
+            // members, we remove subscriptions.
             $add = array_keys(array_diff_key($new, $old));
-
+            $rem = array_keys(array_diff_key($old, $new));
-            // When removing members, we remove their subscription too.
-            // When unwatching, we leave subscriptions, since it's fine to be
-            // subscribed to a project but not be a member of it.
-            $edge_const = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST;
-            if ($edge_type == $edge_const) {
-              $rem = array_keys(array_diff_key($old, $new));
-            } else {
-              $rem = array();
-            }
 
             // NOTE: The subscribe is "explicit" because there's no implicit
             // unsubscribe, so Join -> Leave -> Join doesn't resubscribe you
@@ -212,27 +209,12 @@ final class PhabricatorProjectTransactionEditor
             // this, which is a fairly weird edge case and pretty arguable both
             // ways.
 
-            // Subscriptions caused by watches should also clearly be explicit,
-            // and that case is unambiguous.
-
             id(new PhabricatorSubscriptionsEditor())
               ->setActor($this->requireActor())
               ->setObject($object)
               ->subscribeExplicit($add)
               ->unsubscribe($rem)
               ->save();
-
-            if ($rem) {
-              // When removing members, also remove any watches on the project.
-              $edge_editor = new PhabricatorEdgeEditor();
-              foreach ($rem as $rem_phid) {
-                $edge_editor->removeEdge(
-                  $object->getPHID(),
-                  PhabricatorObjectHasWatcherEdgeType::EDGECONST,
-                  $rem_phid);
-              }
-              $edge_editor->save();
-            }
             break;
         }
         break;

src/applications/project/storage/PhabricatorProject.php

@@ -548,11 +548,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO
     return false;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return $this->isUserMember($phid) &&
-           !$this->isUserWatcher($phid);
-  }
-
 
 /* -(  PhabricatorCustomFieldInterface  )------------------------------------ */
 

src/applications/project/view/PhabricatorProjectUserListView.php

@@ -79,7 +79,7 @@ abstract class PhabricatorProjectUserListView extends AphrontView {
         ->setHref($handle->getURI())
         ->setImageURI($handle->getImageURI());
 
-      if ($can_edit) {
+      if ($can_edit && !$limit) {
         $remove_uri = $this->getRemoveURI($user_phid);
 
         $item->addAction(
@@ -94,16 +94,32 @@ abstract class PhabricatorProjectUserListView extends AphrontView {
     }
 
     if ($user_phids) {
-      $header = pht(
+      $header_text = pht(
         '%s (%s)',
         $this->getHeaderText(),
         phutil_count($user_phids));
     } else {
-      $header = $this->getHeaderText();
+      $header_text = $this->getHeaderText();
+    }
+
+    $id = $project->getID();
+
+    $header = id(new PHUIHeaderView())
+      ->setHeader($header_text);
+
+    if ($limit) {
+      $header->addActionLink(
+        id(new PHUIButtonView())
+          ->setTag('a')
+          ->setIcon(
+            id(new PHUIIconView())
+              ->setIconFont('fa-list-ul'))
+          ->setText(pht('View All'))
+          ->setHref("/project/members/{$id}/"));
     }
 
     return id(new PHUIObjectBoxView())
-      ->setHeaderText($header)
+      ->setHeader($header)
       ->setObjectList($list);
   }
 

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -443,10 +443,6 @@ final class PhabricatorRepositoryCommit
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

src/applications/slowvote/storage/PhabricatorSlowvotePoll.php

@@ -183,10 +183,6 @@ final class PhabricatorSlowvotePoll extends PhabricatorSlowvoteDAO
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorTokenReceiverInterface  )---------------------------------- */
 

src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php

@@ -61,13 +61,6 @@ final class PhabricatorSubscriptionsEditController
         $handle->getURI());
     }
 
-    if (!$object->shouldAllowSubscription($viewer->getPHID())) {
-      return $this->buildErrorResponse(
-        pht('You Can Not Subscribe'),
-        pht('You can not subscribe to this object.'),
-        $handle->getURI());
-    }
-
     if ($object instanceof PhabricatorApplicationTransactionInterface) {
       if ($is_add) {
         $xaction_value = array(

src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php

@@ -34,11 +34,6 @@ final class PhabricatorSubscriptionsUIEventListener
       return;
     }
 
-    if (!$object->shouldAllowSubscription($user_phid)) {
-      // This object doesn't allow the viewer to subscribe.
-      return;
-    }
-
     if ($user_phid && $object->isAutomaticallySubscribed($user_phid)) {
       $sub_action = id(new PhabricatorActionView())
         ->setWorkflow(true)

src/applications/subscriptions/interface/PhabricatorSubscribableInterface.php

@@ -22,17 +22,6 @@ interface PhabricatorSubscribableInterface {
    */
   public function shouldShowSubscribersProperty();
 
-
-  /**
-   * Return `true` to indicate that the "Subscribe" action should be shown and
-   * enabled when rendering action lists for this object, or `false` to omit
-   * the action.
-   *
-   * @param   phid  Viewing or acting user PHID.
-   * @return  bool  True to allow the user to subscribe.
-   */
-  public function shouldAllowSubscription($phid);
-
 }
 
 // TEMPLATE IMPLEMENTATION /////////////////////////////////////////////////////
@@ -48,8 +37,4 @@ interface PhabricatorSubscribableInterface {
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 */

src/infrastructure/daemon/workers/storage/PhabricatorWorkerBulkJob.php

@@ -222,10 +222,6 @@ final class PhabricatorWorkerBulkJob
     return true;
   }
 
-  public function shouldAllowSubscription($phid) {
-    return true;
-  }
-
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */