From 21de2b1a0c7c16c50b033ea938a4ed43c346de24 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Feb 2014 14:29:17 -0800 Subject: [PATCH] Make Projects a PhabricatorSubscribableInterface, but with restricted defaults Summary: Ref T4379. I want project subscriptions to work like this (yell if this seems whacky, since it makes subscriptions mean somethign a little different for projects than they do for other objects): - You can only subscribe to a project if you're a project member. - When you're added as a member, you're added as a subscriber. - When you're removed as a member, you're removed as a subscriber. - While you're a member, you can optionally unsubscribe. From a UI perspective: - We don't show the subscriber list, since it's going to be some uninteresting subset of the member list. - We don't show CC transactions in history, since they're an uninteresting near-approximation of the membership transactions. - You only see the subscription controls if you're a member. To do this, I've augmented `PhabricatorSubscribableInterface` with two new methods. It would be nice if we were on PHP 5.4+ and could just use traits for this, but we should get data about version usage before we think about this. For now, copy/paste the default implementations into every implementing class. Then, I implemented the interface in `PhabricatorProject` but with alternate defaults. Test Plan: - Used the normal interaction on existing objects. - This has no actual effect on projects, verified no subscription stuff mysteriously appeared. - Hit the new error case by fiddling with the UI. Reviewers: btrahan Reviewed By: btrahan CC: chad, aran Maniphest Tasks: T4379 Differential Revision: https://secure.phabricator.com/D8165 --- src/__phutil_library_map__.php | 1 + .../files/storage/PhabricatorFile.php | 8 ++++ .../configuration/HarbormasterBuildPlan.php | 8 ++++ .../legalpad/storage/LegalpadDocument.php | 16 +++++++- .../storage/PhabricatorFileImageMacro.php | 8 ++++ .../paste/storage/PhabricatorPaste.php | 8 ++++ .../pholio/storage/PholioMock.php | 8 ++++ .../phriction/storage/PhrictionDocument.php | 18 +++++++++ .../ponder/storage/PonderAnswer.php | 7 ++++ .../ponder/storage/PonderQuestion.php | 37 ++++++++++++------ .../PhabricatorProjectProfileController.php | 1 + .../project/storage/PhabricatorProject.php | 21 +++++++++- .../storage/PhabricatorSlowvotePoll.php | 8 ++++ ...PhabricatorSubscriptionsEditController.php | 7 ++++ ...habricatorSubscriptionsUIEventListener.php | 11 ++++++ .../PhabricatorSubscribableInterface.php | 39 +++++++++++++++++++ 16 files changed, 191 insertions(+), 15 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a551e660ce..d6a31cf88c 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4555,6 +4555,7 @@ phutil_register_library_map(array( 0 => 'PhabricatorProjectDAO', 1 => 'PhabricatorFlaggableInterface', 2 => 'PhabricatorPolicyInterface', + 3 => 'PhabricatorSubscribableInterface', ), 'PhabricatorProjectBoardController' => 'PhabricatorProjectController', 'PhabricatorProjectBoardEditController' => 'PhabricatorProjectController', diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index d611f1b062..dc6ad6d1ad 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -933,6 +933,14 @@ final class PhabricatorFile extends PhabricatorFileDAO return ($this->authorPHID == $phid); } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorTokenReceiverInterface )---------------------------------- */ diff --git a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php index e3d505ce57..cffcda72ef 100644 --- a/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php +++ b/src/applications/harbormaster/storage/configuration/HarbormasterBuildPlan.php @@ -64,6 +64,14 @@ final class HarbormasterBuildPlan extends HarbormasterDAO return false; } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/legalpad/storage/LegalpadDocument.php b/src/applications/legalpad/storage/LegalpadDocument.php index 99208d81e1..6a3e4d18f9 100644 --- a/src/applications/legalpad/storage/LegalpadDocument.php +++ b/src/applications/legalpad/storage/LegalpadDocument.php @@ -91,13 +91,25 @@ final class LegalpadDocument extends LegalpadDAO return 'L'.$this->getID(); } -/* -( PhabricatorSubscribableInterface Implementation )-------------------- */ + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + public function isAutomaticallySubscribed($phid) { return ($this->creatorPHID == $phid); } -/* -( PhabricatorPolicyInterface Implementation )-------------------------- */ + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + public function getCapabilities() { return array( diff --git a/src/applications/macro/storage/PhabricatorFileImageMacro.php b/src/applications/macro/storage/PhabricatorFileImageMacro.php index 0bd9e238d8..b8cdbf6a8f 100644 --- a/src/applications/macro/storage/PhabricatorFileImageMacro.php +++ b/src/applications/macro/storage/PhabricatorFileImageMacro.php @@ -79,6 +79,14 @@ final class PhabricatorFileImageMacro extends PhabricatorFileDAO return false; } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/paste/storage/PhabricatorPaste.php b/src/applications/paste/storage/PhabricatorPaste.php index 9e4c8ed265..4c3aee3236 100644 --- a/src/applications/paste/storage/PhabricatorPaste.php +++ b/src/applications/paste/storage/PhabricatorPaste.php @@ -87,6 +87,14 @@ final class PhabricatorPaste extends PhabricatorPasteDAO return ($this->authorPHID == $phid); } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorTokenReceiverInterface )---------------------------------- */ diff --git a/src/applications/pholio/storage/PholioMock.php b/src/applications/pholio/storage/PholioMock.php index 740ac71a9e..3f48f31501 100644 --- a/src/applications/pholio/storage/PholioMock.php +++ b/src/applications/pholio/storage/PholioMock.php @@ -137,6 +137,14 @@ final class PholioMock extends PholioDAO return ($this->authorPHID == $phid); } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/applications/phriction/storage/PhrictionDocument.php b/src/applications/phriction/storage/PhrictionDocument.php index 3e36a26a0b..6cb0658e4f 100644 --- a/src/applications/phriction/storage/PhrictionDocument.php +++ b/src/applications/phriction/storage/PhrictionDocument.php @@ -104,6 +104,10 @@ final class PhrictionDocument extends PhrictionDAO return $parts[1].'/'; } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, @@ -133,12 +137,26 @@ final class PhrictionDocument extends PhrictionDAO return null; } + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + + public function isAutomaticallySubscribed($phid) { return false; } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + + /* -( PhabricatorTokenReceiverInterface )---------------------------------- */ + public function getUsersToNotifyOfTokenGiven() { return PhabricatorSubscribersQuery::loadSubscribersForPHID($this->phid); } diff --git a/src/applications/ponder/storage/PonderAnswer.php b/src/applications/ponder/storage/PonderAnswer.php index 3b6a8b9bd6..f21d22e055 100644 --- a/src/applications/ponder/storage/PonderAnswer.php +++ b/src/applications/ponder/storage/PonderAnswer.php @@ -188,5 +188,12 @@ final class PonderAnswer extends PonderDAO return ($phid == $this->getAuthorPHID()); } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } } diff --git a/src/applications/ponder/storage/PonderQuestion.php b/src/applications/ponder/storage/PonderQuestion.php index 7309afde6a..ed21352ce6 100644 --- a/src/applications/ponder/storage/PonderQuestion.php +++ b/src/applications/ponder/storage/PonderQuestion.php @@ -170,10 +170,6 @@ final class PonderQuestion extends PonderDAO return $this->getPHID(); } - public function isAutomaticallySubscribed($phid) { - return ($phid == $this->getAuthorPHID()); - } - public function save() { if (!$this->getMailKey()) { $this->setMailKey(Filesystem::readRandomCharacters(20)); @@ -181,6 +177,20 @@ final class PonderQuestion extends PonderDAO return parent::save(); } + public function getOriginalTitle() { + // TODO: Make this actually save/return the original title. + return $this->getTitle(); + } + + public function getFullTitle() { + $id = $this->getID(); + $title = $this->getTitle(); + return "Q{$id}: {$title}"; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, @@ -210,15 +220,20 @@ final class PonderQuestion extends PonderDAO 'The user who asked a question can always view and edit it.'); } - public function getOriginalTitle() { - // TODO: Make this actually save/return the original title. - return $this->getTitle(); + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + + + public function isAutomaticallySubscribed($phid) { + return ($phid == $this->getAuthorPHID()); } - public function getFullTitle() { - $id = $this->getID(); - $title = $this->getTitle(); - return "Q{$id}: {$title}"; + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; } diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index cdfe73202a..f643e98c28 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -214,6 +214,7 @@ final class PhabricatorProjectProfileController } $view->addAction($action); + $view->addAction( id(new PhabricatorActionView()) ->setName(pht('View History')) diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index cdd24a15af..0377959401 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -3,7 +3,8 @@ final class PhabricatorProject extends PhabricatorProjectDAO implements PhabricatorFlaggableInterface, - PhabricatorPolicyInterface { + PhabricatorPolicyInterface, + PhabricatorSubscribableInterface { protected $name; protected $status = PhabricatorProjectStatus::STATUS_ACTIVE; @@ -15,7 +16,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO protected $editPolicy; protected $joinPolicy; - private $subprojectsNeedUpdate; private $memberPHIDs = self::ATTACHABLE; private $sparseMembers = self::ATTACHABLE; private $profile = self::ATTACHABLE; @@ -134,4 +134,21 @@ final class PhabricatorProject extends PhabricatorProjectDAO return 'projects/'.$slug; } + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ + + + public function isAutomaticallySubscribed($phid) { + return false; + } + + public function shouldShowSubscribersProperty() { + return false; + } + + public function shouldAllowSubscription($phid) { + return false; + } + + } diff --git a/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php b/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php index c306040b04..0bdb0555ca 100644 --- a/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php +++ b/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php @@ -125,6 +125,14 @@ final class PhabricatorSlowvotePoll extends PhabricatorSlowvoteDAO return ($phid == $this->getAuthorPHID()); } + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + /* -( PhabricatorTokenReceiverInterface )---------------------------------- */ diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php index d46fc37986..5dfd9b01f0 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -56,6 +56,13 @@ final class PhabricatorSubscriptionsEditController $handle->getURI()); } + if (!$object->shouldAllowSubscription($user->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( diff --git a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php index aaf75766b0..0dd106efff 100644 --- a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php +++ b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php @@ -33,6 +33,11 @@ final class PhabricatorSubscriptionsUIEventListener return; } + if (!$object->shouldAllowSubscription($user->getPHID())) { + // This object doesn't allow the viewer to subscribe. + return; + } + if ($object->isAutomaticallySubscribed($user->getPHID())) { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) @@ -95,6 +100,12 @@ final class PhabricatorSubscriptionsUIEventListener // This object isn't subscribable. return; } + + if (!$object->shouldShowSubscribersProperty()) { + // This object doesn't render subscribers in its property list. + return; + } + $subscribers = PhabricatorSubscribersQuery::loadSubscribersForPHID( $object->getPHID()); if ($subscribers) { diff --git a/src/applications/subscriptions/interface/PhabricatorSubscribableInterface.php b/src/applications/subscriptions/interface/PhabricatorSubscribableInterface.php index 8f87941ea5..95275e53bb 100644 --- a/src/applications/subscriptions/interface/PhabricatorSubscribableInterface.php +++ b/src/applications/subscriptions/interface/PhabricatorSubscribableInterface.php @@ -13,4 +13,43 @@ interface PhabricatorSubscribableInterface { */ public function isAutomaticallySubscribed($phid); + + /** + * Return `true` to indicate that "Subscribers:" should be shown when + * rendering property lists for this object, or `false` to omit the property. + * + * @return bool True to show the "Subscribers:" property. + */ + 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 ///////////////////////////////////////////////////// + +/* -( PhabricatorSubscribableInterface )----------------------------------- */ +/* + + public function isAutomaticallySubscribed($phid) { + return false; + } + + public function shouldShowSubscribersProperty() { + return true; + } + + public function shouldAllowSubscription($phid) { + return true; + } + +*/