1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-30 02:32:42 +01:00

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
This commit is contained in:
epriestley 2016-02-16 10:11:12 -08:00
parent 0e5cd478c4
commit 376c85a828
6 changed files with 111 additions and 37 deletions

View file

@ -162,19 +162,32 @@ final class PhabricatorProjectProfileController
private function renderWatchAction(PhabricatorProject $project) { private function renderWatchAction(PhabricatorProject $project) {
$viewer = $this->getViewer(); $viewer = $this->getViewer();
$viewer_phid = $viewer->getPHID();
$id = $project->getID(); $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_icon = 'fa-eye';
$watch_text = pht('Watch Project'); $watch_text = pht('Watch Project');
$watch_href = "/project/watch/{$id}/?via=profile"; $watch_href = "/project/watch/{$id}/?via=profile";
$watch_disabled = false;
} else { } else {
$watch_icon = 'fa-eye-slash'; $watch_icon = 'fa-eye-slash';
$watch_text = pht('Unwatch Project'); $watch_text = pht('Unwatch Project');
$watch_href = "/project/unwatch/{$id}/?via=profile"; $watch_href = "/project/unwatch/{$id}/?via=profile";
$watch_disabled = false;
} }
$watch_icon = id(new PHUIIconView()) $watch_icon = id(new PHUIIconView())
@ -185,7 +198,8 @@ final class PhabricatorProjectProfileController
->setWorkflow(true) ->setWorkflow(true)
->setIcon($watch_icon) ->setIcon($watch_icon)
->setText($watch_text) ->setText($watch_text)
->setHref($watch_href); ->setHref($watch_href)
->setDisabled($watch_disabled);
} }
private function buildMilestoneList(PhabricatorProject $project) { private function buildMilestoneList(PhabricatorProject $project) {

View file

@ -25,6 +25,23 @@ final class PhabricatorProjectWatchController
$done_uri = "/project/members/{$id}/"; $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()) { if ($request->isDialogFormPost()) {
$edge_action = null; $edge_action = null;
switch ($action) { switch ($action) {
@ -61,10 +78,14 @@ final class PhabricatorProjectWatchController
switch ($action) { switch ($action) {
case 'watch': case 'watch':
$title = pht('Watch Project?'); $title = pht('Watch Project?');
$body = pht( $body = array();
$body[] = pht(
'Watching a project will let you monitor it closely. You will '. 'Watching a project will let you monitor it closely. You will '.
'receive email and notifications about changes to every object '. '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'); $submit = pht('Watch Project');
break; break;
case 'unwatch': case 'unwatch':
@ -78,12 +99,17 @@ final class PhabricatorProjectWatchController
return new Aphront404Response(); return new Aphront404Response();
} }
return $this->newDialog() $dialog = $this->newDialog()
->setTitle($title) ->setTitle($title)
->addHiddenInput('via', $via) ->addHiddenInput('via', $via)
->appendParagraph($body)
->addCancelButton($done_uri) ->addCancelButton($done_uri)
->addSubmitButton($submit); ->addSubmitButton($submit);
foreach ((array)$body as $paragraph) {
$dialog->appendParagraph($paragraph);
}
return $dialog;
} }
} }

View file

@ -247,7 +247,7 @@ final class PhabricatorProjectQuery
$all_graph = $this->getAllReachableAncestors($projects); $all_graph = $this->getAllReachableAncestors($projects);
if ($this->needAncestorMembers) { if ($this->needAncestorMembers || $this->needWatchers) {
$src_projects = $all_graph; $src_projects = $all_graph;
} else { } else {
$src_projects = $projects; $src_projects = $projects;
@ -255,11 +255,13 @@ final class PhabricatorProjectQuery
$all_sources = array(); $all_sources = array();
foreach ($src_projects as $project) { foreach ($src_projects as $project) {
// For milestones, we need parent members.
if ($project->isMilestone()) { if ($project->isMilestone()) {
$phid = $project->getParentProjectPHID(); $parent_phid = $project->getParentProjectPHID();
} else { $all_sources[$parent_phid] = $parent_phid;
$phid = $project->getPHID();
} }
$phid = $project->getPHID();
$all_sources[$phid] = $phid; $all_sources[$phid] = $phid;
} }
@ -318,7 +320,7 @@ final class PhabricatorProjectQuery
if ($this->needWatchers) { if ($this->needWatchers) {
$watcher_phids = $edge_query->getDestinationPHIDs( $watcher_phids = $edge_query->getDestinationPHIDs(
$source_phids, array($project_phid),
array($watcher_type)); array($watcher_type));
$project->attachWatcherPHIDs($watcher_phids); $project->attachWatcherPHIDs($watcher_phids);
$project->setIsUserWatcher( $project->setIsUserWatcher(

View file

@ -287,6 +287,32 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return $this->assertAttachedKey($this->sparseWatchers, $user_phid); 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) { public function setIsUserWatcher($user_phid, $is_watcher) {
if ($this->sparseWatchers === self::ATTACHABLE) { if ($this->sparseWatchers === self::ATTACHABLE) {
$this->sparseWatchers = array(); $this->sparseWatchers = array();
@ -304,6 +330,21 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return $this->assertAttached($this->watcherPHIDs); 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) { public function attachSlugs(array $slugs) {
$this->slugs = $slugs; $this->slugs = $slugs;
return $this; return $this;

View file

@ -28,24 +28,10 @@ final class PhabricatorSubscriptionsEditController
->withPHIDs(array($phid)) ->withPHIDs(array($phid))
->executeOne(); ->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()) $object = id(new PhabricatorObjectQuery())
->setViewer($viewer) ->setViewer($viewer)
->withPHIDs(array($phid)) ->withPHIDs(array($phid))
->executeOne(); ->executeOne();
}
if (!($object instanceof PhabricatorSubscribableInterface)) { if (!($object instanceof PhabricatorSubscribableInterface)) {
return $this->buildErrorResponse( return $this->buildErrorResponse(

View file

@ -2607,14 +2607,19 @@ abstract class PhabricatorApplicationTransactionEditor
PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); PhabricatorProjectObjectHasProjectEdgeType::EDGECONST);
if ($project_phids) { if ($project_phids) {
$watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST; $projects = id(new PhabricatorProjectQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
->withPHIDs($project_phids)
->needWatchers(true)
->execute();
$query = id(new PhabricatorEdgeQuery()) $watcher_phids = array();
->withSourcePHIDs($project_phids) foreach ($projects as $project) {
->withEdgeTypes(array($watcher_type)); foreach ($project->getAllAncestorWatcherPHIDs() as $phid) {
$query->execute(); $watcher_phids[$phid] = $phid;
}
}
$watcher_phids = $query->getDestinationPHIDs();
if ($watcher_phids) { if ($watcher_phids) {
// We need to do a visibility check for all the watchers, as // We need to do a visibility check for all the watchers, as
// watching a project is not a guarantee that you can see objects // watching a project is not a guarantee that you can see objects