From 376c85a828c8af581c4695699f3d02bc5c70ff7e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Feb 2016 10:11:12 -0800 Subject: [PATCH] Make subproject/milestone watch rules work better Summary: Ref T10349. These got sort of half-weirded-up before I separated subscriptions and watching fully. New rules are: - You can watch whatever you want. - Watching a parent watches everything inside it. - If you're watching "Stonework" and go to "Stonework > Masonry", you'll see a "Watching Ancestor" hint to let you know you're already watching a parent or ancestor. Test Plan: - Watched and unwatched "Stonework". - Watched and unwatched "Stonework > Iteration IV". - While watching "Stonework", visited "Iteration IV" and saw "Watching Ancestor" hint. - Created a task tagged "Stonework > Iteration IV". Got notified about it because I watch "Stonework". Reviewers: chad Reviewed By: chad Maniphest Tasks: T10349 Differential Revision: https://secure.phabricator.com/D15280 --- .../PhabricatorProjectProfileController.php | 22 ++++++++-- .../PhabricatorProjectWatchController.php | 34 +++++++++++++-- .../project/query/PhabricatorProjectQuery.php | 12 +++--- .../project/storage/PhabricatorProject.php | 41 +++++++++++++++++++ ...PhabricatorSubscriptionsEditController.php | 22 ++-------- ...habricatorApplicationTransactionEditor.php | 17 +++++--- 6 files changed, 111 insertions(+), 37 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index 0a9bc602d2..81a172ae42 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -162,19 +162,32 @@ final class PhabricatorProjectProfileController private function renderWatchAction(PhabricatorProject $project) { $viewer = $this->getViewer(); - $viewer_phid = $viewer->getPHID(); $id = $project->getID(); - $is_watcher = ($viewer_phid && $project->isUserWatcher($viewer_phid)); + if (!$viewer->isLoggedIn()) { + $is_watcher = false; + $is_ancestor = false; + } else { + $viewer_phid = $viewer->getPHID(); + $is_watcher = $project->isUserWatcher($viewer_phid); + $is_ancestor = $project->isUserAncestorWatcher($viewer_phid); + } - if (!$is_watcher) { + if ($is_ancestor && !$is_watcher) { + $watch_icon = 'fa-eye'; + $watch_text = pht('Watching Ancestor'); + $watch_href = "/project/watch/{$id}/?via=profile"; + $watch_disabled = true; + } else if (!$is_watcher) { $watch_icon = 'fa-eye'; $watch_text = pht('Watch Project'); $watch_href = "/project/watch/{$id}/?via=profile"; + $watch_disabled = false; } else { $watch_icon = 'fa-eye-slash'; $watch_text = pht('Unwatch Project'); $watch_href = "/project/unwatch/{$id}/?via=profile"; + $watch_disabled = false; } $watch_icon = id(new PHUIIconView()) @@ -185,7 +198,8 @@ final class PhabricatorProjectProfileController ->setWorkflow(true) ->setIcon($watch_icon) ->setText($watch_text) - ->setHref($watch_href); + ->setHref($watch_href) + ->setDisabled($watch_disabled); } private function buildMilestoneList(PhabricatorProject $project) { diff --git a/src/applications/project/controller/PhabricatorProjectWatchController.php b/src/applications/project/controller/PhabricatorProjectWatchController.php index 9624a0b730..00117768e1 100644 --- a/src/applications/project/controller/PhabricatorProjectWatchController.php +++ b/src/applications/project/controller/PhabricatorProjectWatchController.php @@ -25,6 +25,23 @@ final class PhabricatorProjectWatchController $done_uri = "/project/members/{$id}/"; } + $is_watcher = $project->isUserWatcher($viewer->getPHID()); + $is_ancestor = $project->isUserAncestorWatcher($viewer->getPHID()); + if ($is_ancestor && !$is_watcher) { + $ancestor_phid = $project->getWatchedAncestorPHID($viewer->getPHID()); + $handles = $viewer->loadHandles(array($ancestor_phid)); + $ancestor_handle = $handles[$ancestor_phid]; + + return $this->newDialog() + ->setTitle(pht('Watching Ancestor')) + ->appendParagraph( + pht( + 'You are already watching %s, an ancestor of this project, and '. + 'are thus watching all of its subprojects.', + $ancestor_handle->renderTag()->render())) + ->addCancelbutton($done_uri); + } + if ($request->isDialogFormPost()) { $edge_action = null; switch ($action) { @@ -61,10 +78,14 @@ final class PhabricatorProjectWatchController switch ($action) { case 'watch': $title = pht('Watch Project?'); - $body = pht( + $body = array(); + $body[] = pht( 'Watching a project will let you monitor it closely. You will '. 'receive email and notifications about changes to every object '. - 'associated with projects you watch.'); + 'tagged with projects you watch.'); + $body[] = pht( + 'Watching a project also watches all subprojects and milestones of '. + 'that project.'); $submit = pht('Watch Project'); break; case 'unwatch': @@ -78,12 +99,17 @@ final class PhabricatorProjectWatchController return new Aphront404Response(); } - return $this->newDialog() + $dialog = $this->newDialog() ->setTitle($title) ->addHiddenInput('via', $via) - ->appendParagraph($body) ->addCancelButton($done_uri) ->addSubmitButton($submit); + + foreach ((array)$body as $paragraph) { + $dialog->appendParagraph($paragraph); + } + + return $dialog; } } diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index b72fa1cec2..9ce5fff77c 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -247,7 +247,7 @@ final class PhabricatorProjectQuery $all_graph = $this->getAllReachableAncestors($projects); - if ($this->needAncestorMembers) { + if ($this->needAncestorMembers || $this->needWatchers) { $src_projects = $all_graph; } else { $src_projects = $projects; @@ -255,11 +255,13 @@ final class PhabricatorProjectQuery $all_sources = array(); foreach ($src_projects as $project) { + // For milestones, we need parent members. if ($project->isMilestone()) { - $phid = $project->getParentProjectPHID(); - } else { - $phid = $project->getPHID(); + $parent_phid = $project->getParentProjectPHID(); + $all_sources[$parent_phid] = $parent_phid; } + + $phid = $project->getPHID(); $all_sources[$phid] = $phid; } @@ -318,7 +320,7 @@ final class PhabricatorProjectQuery if ($this->needWatchers) { $watcher_phids = $edge_query->getDestinationPHIDs( - $source_phids, + array($project_phid), array($watcher_type)); $project->attachWatcherPHIDs($watcher_phids); $project->setIsUserWatcher( diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index 81a5c09dc9..061886f6f0 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -287,6 +287,32 @@ final class PhabricatorProject extends PhabricatorProjectDAO return $this->assertAttachedKey($this->sparseWatchers, $user_phid); } + public function isUserAncestorWatcher($user_phid) { + $is_watcher = $this->isUserWatcher($user_phid); + + if (!$is_watcher) { + $parent = $this->getParentProject(); + if ($parent) { + return $parent->isUserWatcher($user_phid); + } + } + + return $is_watcher; + } + + public function getWatchedAncestorPHID($user_phid) { + if ($this->isUserWatcher($user_phid)) { + return $this->getPHID(); + } + + $parent = $this->getParentProject(); + if ($parent) { + return $parent->getWatchedAncestorPHID($user_phid); + } + + return null; + } + public function setIsUserWatcher($user_phid, $is_watcher) { if ($this->sparseWatchers === self::ATTACHABLE) { $this->sparseWatchers = array(); @@ -304,6 +330,21 @@ final class PhabricatorProject extends PhabricatorProjectDAO return $this->assertAttached($this->watcherPHIDs); } + public function getAllAncestorWatcherPHIDs() { + $parent = $this->getParentProject(); + if ($parent) { + $watchers = $parent->getAllAncestorWatcherPHIDs(); + } else { + $watchers = array(); + } + + foreach ($this->getWatcherPHIDs() as $phid) { + $watchers[$phid] = $phid; + } + + return $watchers; + } + public function attachSlugs(array $slugs) { $this->slugs = $slugs; return $this; diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php index ad089d8f3d..add833c127 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -28,24 +28,10 @@ final class PhabricatorSubscriptionsEditController ->withPHIDs(array($phid)) ->executeOne(); - if (phid_get_type($phid) == PhabricatorProjectProjectPHIDType::TYPECONST) { - // TODO: This is a big hack, but a weak argument for adding some kind - // of "load for role" feature to ObjectQuery, and also not a really great - // argument for adding some kind of "load extra stuff" feature to - // SubscriberInterface. Do this for now and wait for the best way forward - // to become more clear? - - $object = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->withPHIDs(array($phid)) - ->needWatchers(true) - ->executeOne(); - } else { - $object = id(new PhabricatorObjectQuery()) - ->setViewer($viewer) - ->withPHIDs(array($phid)) - ->executeOne(); - } + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($phid)) + ->executeOne(); if (!($object instanceof PhabricatorSubscribableInterface)) { return $this->buildErrorResponse( diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index d6f70cac39..cbfaebb182 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -2607,14 +2607,19 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); if ($project_phids) { - $watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST; + $projects = id(new PhabricatorProjectQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($project_phids) + ->needWatchers(true) + ->execute(); - $query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($project_phids) - ->withEdgeTypes(array($watcher_type)); - $query->execute(); + $watcher_phids = array(); + foreach ($projects as $project) { + foreach ($project->getAllAncestorWatcherPHIDs() as $phid) { + $watcher_phids[$phid] = $phid; + } + } - $watcher_phids = $query->getDestinationPHIDs(); if ($watcher_phids) { // We need to do a visibility check for all the watchers, as // watching a project is not a guarantee that you can see objects