Index: src/__phutil_library_map__.php =================================================================== --- src/__phutil_library_map__.php +++ src/__phutil_library_map__.php @@ -464,7 +464,6 @@ '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 @@ 'DifferentialRevisionUpdateHistoryView' => 'AphrontView', 'DifferentialRevisionViewController' => 'DifferentialController', 'DifferentialSearchIndexer' => 'PhabricatorSearchDocumentIndexer', - 'DifferentialSubscribeController' => 'DifferentialController', 'DifferentialSubscribersField' => 'DifferentialCoreCustomField', 'DifferentialSummaryField' => 'DifferentialCoreCustomField', 'DifferentialSummaryFieldSpecification' => 'DifferentialFreeformFieldSpecification', Index: src/applications/differential/application/PhabricatorApplicationDifferential.php =================================================================== --- src/applications/differential/application/PhabricatorApplicationDifferential.php +++ src/applications/differential/application/PhabricatorApplicationDifferential.php @@ -63,8 +63,6 @@ => 'DifferentialInlineCommentEditController', ), ), - 'subscribe/(?Padd|rem)/(?P[1-9]\d*)/' - => 'DifferentialSubscribeController', 'preview/' => 'PhabricatorMarkupPreviewController', ), ); Index: src/applications/differential/controller/DifferentialRevisionViewController.php =================================================================== --- src/applications/differential/controller/DifferentialRevisionViewController.php +++ src/applications/differential/controller/DifferentialRevisionViewController.php @@ -459,23 +459,17 @@ } private function getRevisionActions(DifferentialRevision $revision) { - $user = $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(); + $viewer = $this->getRequest()->getUser(); $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 @@ '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'); Index: src/applications/differential/controller/DifferentialSubscribeController.php =================================================================== --- src/applications/differential/controller/DifferentialSubscribeController.php +++ /dev/null @@ -1,80 +0,0 @@ -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()); - } -} Index: src/applications/differential/storage/DifferentialRevision.php =================================================================== --- src/applications/differential/storage/DifferentialRevision.php +++ src/applications/differential/storage/DifferentialRevision.php @@ -444,20 +444,41 @@ public function isAutomaticallySubscribed($phid) { - // TODO: Reviewers are also automatically subscribed, but that data may - // not always be loaded, so be conservative for now. All the editing code - // has checks around this already. - return ($phid == $this->getAuthorPHID()); + if ($phid == $this->getAuthorPHID()) { + return true; + } + + // 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 false; + return true; }