1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-18 03:20:59 +01:00

Replace subscribe/unsubscribe for projects with explicit mail setting

Summary:
Ref T10054. Ref T6113. Users can currently subscribe to projects, which causes them to receive:

  # mail about project membership changes, description changes, etc; and
  # mail to the project, e.g. when the project is added as a subscriber on a task, or a reviewer on a revision.

Almost no one cares about (1), and after D15061 you can use Herald to get this stuff if you really want it. (It will get progressively more annoying in the future with external membership sources causing automated project membership updates.)

A lot of users are confused about (2) and how it relates to membership, watching, etc, and most users who want (2) don't want (1).

Instead, add an explicit option for this and explain what it does.

This is fairly verbose but I've hidden it on the member/watch screen, which is now the "explain how projects work" screen, I guess.

Test Plan:
{F1064929}

{F1064930}

{F1064931}

  - Disabled/enabled mail for a project.
  - Sent mail to a project with mail disabled, verified I didn't get a copy.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6113, T10054

Differential Revision: https://secure.phabricator.com/D15065
This commit is contained in:
epriestley 2016-01-19 18:56:15 -08:00
parent 5c2e49a812
commit 8463ad2659
12 changed files with 225 additions and 76 deletions

View file

@ -0,0 +1,8 @@
/* PhabricatorObjectHasUnsubscriberEdgeType::EDGECONST = 23 */
/* PhabricatorProjectSilencedEdgeType::EDGECONST = 61 */
/* This is converting existing unsubscribes into disabled mail. */
INSERT IGNORE INTO {$NAMESPACE}_project.edge (src, type, dst, dateCreated)
SELECT src, 61, dst, dateCreated FROM {$NAMESPACE}_project.edge
WHERE type = 23;

View file

@ -2925,6 +2925,8 @@ phutil_register_library_map(array(
'PhabricatorProjectSchemaSpec' => 'applications/project/storage/PhabricatorProjectSchemaSpec.php', 'PhabricatorProjectSchemaSpec' => 'applications/project/storage/PhabricatorProjectSchemaSpec.php',
'PhabricatorProjectSearchEngine' => 'applications/project/query/PhabricatorProjectSearchEngine.php', 'PhabricatorProjectSearchEngine' => 'applications/project/query/PhabricatorProjectSearchEngine.php',
'PhabricatorProjectSearchField' => 'applications/project/searchfield/PhabricatorProjectSearchField.php', 'PhabricatorProjectSearchField' => 'applications/project/searchfield/PhabricatorProjectSearchField.php',
'PhabricatorProjectSilenceController' => 'applications/project/controller/PhabricatorProjectSilenceController.php',
'PhabricatorProjectSilencedEdgeType' => 'applications/project/edge/PhabricatorProjectSilencedEdgeType.php',
'PhabricatorProjectSlug' => 'applications/project/storage/PhabricatorProjectSlug.php', 'PhabricatorProjectSlug' => 'applications/project/storage/PhabricatorProjectSlug.php',
'PhabricatorProjectStandardCustomField' => 'applications/project/customfield/PhabricatorProjectStandardCustomField.php', 'PhabricatorProjectStandardCustomField' => 'applications/project/customfield/PhabricatorProjectStandardCustomField.php',
'PhabricatorProjectStatus' => 'applications/project/constants/PhabricatorProjectStatus.php', 'PhabricatorProjectStatus' => 'applications/project/constants/PhabricatorProjectStatus.php',
@ -7237,7 +7239,6 @@ phutil_register_library_map(array(
'PhabricatorFlaggableInterface', 'PhabricatorFlaggableInterface',
'PhabricatorPolicyInterface', 'PhabricatorPolicyInterface',
'PhabricatorExtendedPolicyInterface', 'PhabricatorExtendedPolicyInterface',
'PhabricatorSubscribableInterface',
'PhabricatorCustomFieldInterface', 'PhabricatorCustomFieldInterface',
'PhabricatorDestructibleInterface', 'PhabricatorDestructibleInterface',
'PhabricatorFulltextInterface', 'PhabricatorFulltextInterface',
@ -7329,6 +7330,8 @@ phutil_register_library_map(array(
'PhabricatorProjectSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorProjectSchemaSpec' => 'PhabricatorConfigSchemaSpec',
'PhabricatorProjectSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectSearchEngine' => 'PhabricatorApplicationSearchEngine',
'PhabricatorProjectSearchField' => 'PhabricatorSearchTokenizerField', 'PhabricatorProjectSearchField' => 'PhabricatorSearchTokenizerField',
'PhabricatorProjectSilenceController' => 'PhabricatorProjectController',
'PhabricatorProjectSilencedEdgeType' => 'PhabricatorEdgeType',
'PhabricatorProjectSlug' => 'PhabricatorProjectDAO', 'PhabricatorProjectSlug' => 'PhabricatorProjectDAO',
'PhabricatorProjectStandardCustomField' => array( 'PhabricatorProjectStandardCustomField' => array(
'PhabricatorProjectCustomField', 'PhabricatorProjectCustomField',

View file

@ -42,20 +42,48 @@ final class PhabricatorMetaMTAMemberQuery extends PhabricatorQuery {
$projects = id(new PhabricatorProjectQuery()) $projects = id(new PhabricatorProjectQuery())
->setViewer($this->getViewer()) ->setViewer($this->getViewer())
->withPHIDs($phids) ->withPHIDs($phids)
->needMembers(true)
->needWatchers(true)
->execute(); ->execute();
$subscribers = id(new PhabricatorSubscribersQuery()) $edge_type = PhabricatorProjectSilencedEdgeType::EDGECONST;
->withObjectPHIDs($phids)
->execute(); $edge_query = id(new PhabricatorEdgeQuery())
->withSourcePHIDs($phids)
->withEdgeTypes(
array(
$edge_type,
));
$edge_query->execute();
$projects = mpull($projects, null, 'getPHID'); $projects = mpull($projects, null, 'getPHID');
foreach ($phids as $phid) { foreach ($phids as $phid) {
$project = idx($projects, $phid); $project = idx($projects, $phid);
if (!$project) { if (!$project) {
$results[$phid] = array(); $results[$phid] = array();
} else { continue;
$results[$phid] = idx($subscribers, $phid, array());
} }
// Recipients are members who haven't silenced the project, plus
// watchers.
$members = $project->getMemberPHIDs();
$members = array_fuse($members);
$watchers = $project->getWatcherPHIDs();
$watchers = array_fuse($watchers);
$silenced = $edge_query->getDestinationPHIDs(
array($phid),
array($edge_type));
$silenced = array_fuse($silenced);
$result_map = array_diff_key($members, $silenced);
$result_map = $result_map + $watchers;
$results[$phid] = array_values($result_map);
} }
break; break;
default: default:

View file

@ -89,6 +89,8 @@ final class PhabricatorProjectApplication extends PhabricatorApplication {
'history/(?P<id>[1-9]\d*)/' => 'PhabricatorProjectHistoryController', 'history/(?P<id>[1-9]\d*)/' => 'PhabricatorProjectHistoryController',
'(?P<action>watch|unwatch)/(?P<id>[1-9]\d*)/' '(?P<action>watch|unwatch)/(?P<id>[1-9]\d*)/'
=> 'PhabricatorProjectWatchController', => 'PhabricatorProjectWatchController',
'silence/(?P<id>[1-9]\d*)/'
=> 'PhabricatorProjectSilenceController',
), ),
'/tag/' => array( '/tag/' => array(
'(?P<slug>[^/]+)/' => 'PhabricatorProjectViewController', '(?P<slug>[^/]+)/' => 'PhabricatorProjectViewController',

View file

@ -110,6 +110,49 @@ final class PhabricatorProjectMembersViewController
$descriptions[PhabricatorPolicyCapability::CAN_JOIN]); $descriptions[PhabricatorPolicyCapability::CAN_JOIN]);
} }
$viewer_phid = $viewer->getPHID();
if ($project->isUserWatcher($viewer_phid)) {
$watch_item = id(new PHUIStatusItemView())
->setIcon('fa-eye green')
->setTarget(phutil_tag('strong', array(), pht('Watching')))
->setNote(
pht(
'You will receive mail about changes made to any related '.
'object.'));
$watch_status = id(new PHUIStatusListView())
->addItem($watch_item);
$view->addProperty(pht('Watching'), $watch_status);
}
if ($project->isUserMember($viewer_phid)) {
$is_silenced = $this->isProjectSilenced($project);
if ($is_silenced) {
$mail_icon = 'fa-envelope-o grey';
$mail_target = pht('Disabled');
$mail_note = pht(
'When mail is sent to project members, you will not receive '.
'a copy.');
} else {
$mail_icon = 'fa-envelope-o green';
$mail_target = pht('Enabled');
$mail_note = pht(
'You will receive mail that is sent to project members.');
}
$mail_item = id(new PHUIStatusItemView())
->setIcon($mail_icon)
->setTarget(phutil_tag('strong', array(), $mail_target))
->setNote($mail_note);
$mail_status = id(new PHUIStatusListView())
->addItem($mail_item);
$view->addProperty(pht('Mail to Members'), $mail_status);
}
return $view; return $view;
} }
@ -136,7 +179,9 @@ final class PhabricatorProjectMembersViewController
$can_leave = $supports_edit && (!$is_locked || $can_edit); $can_leave = $supports_edit && (!$is_locked || $can_edit);
if (!$project->isUserMember($viewer->getPHID())) { $viewer_phid = $viewer->getPHID();
if (!$project->isUserMember($viewer_phid)) {
$view->addAction( $view->addAction(
id(new PhabricatorActionView()) id(new PhabricatorActionView())
->setHref('/project/update/'.$project->getID().'/join/') ->setHref('/project/update/'.$project->getID().'/join/')
@ -170,6 +215,23 @@ final class PhabricatorProjectMembersViewController
->setName(pht('Unwatch Project'))); ->setName(pht('Unwatch Project')));
} }
$can_silence = $project->isUserMember($viewer_phid);
$is_silenced = $this->isProjectSilenced($project);
if ($is_silenced) {
$silence_text = pht('Enable Mail');
} else {
$silence_text = pht('Disable Mail');
}
$view->addAction(
id(new PhabricatorActionView())
->setName($silence_text)
->setIcon('fa-envelope-o')
->setHref("/project/silence/{$id}/")
->setWorkflow(true)
->setDisabled(!$can_silence));
$can_add = $can_edit && $supports_edit; $can_add = $can_edit && $supports_edit;
$view->addAction( $view->addAction(
@ -202,4 +264,20 @@ final class PhabricatorProjectMembersViewController
return $view; return $view;
} }
private function isProjectSilenced(PhabricatorProject $project) {
$viewer = $this->getViewer();
$viewer_phid = $viewer->getPHID();
if (!$viewer_phid) {
return false;
}
$edge_type = PhabricatorProjectSilencedEdgeType::EDGECONST;
$silenced = PhabricatorEdgeQuery::loadDestinationPHIDs(
$project->getPHID(),
$edge_type);
$silenced = array_fuse($silenced);
return isset($silenced[$viewer_phid]);
}
} }

View file

@ -0,0 +1,87 @@
<?php
final class PhabricatorProjectSilenceController
extends PhabricatorProjectController {
public function handleRequest(AphrontRequest $request) {
$viewer = $request->getViewer();
$id = $request->getURIData('id');
$action = $request->getURIData('action');
$project = id(new PhabricatorProjectQuery())
->setViewer($viewer)
->withIDs(array($id))
->needMembers(true)
->executeOne();
if (!$project) {
return new Aphront404Response();
}
$edge_type = PhabricatorProjectSilencedEdgeType::EDGECONST;
$done_uri = "/project/members/{$id}/";
$viewer_phid = $viewer->getPHID();
if (!$project->isUserMember($viewer_phid)) {
return $this->newDialog()
->setTitle(pht('Not a Member'))
->appendParagraph(
pht(
'You are not a project member, so you do not receive mail sent '.
'to members of this project.'))
->addCancelButton($done_uri);
}
$silenced = PhabricatorEdgeQuery::loadDestinationPHIDs(
$project->getPHID(),
$edge_type);
$silenced = array_fuse($silenced);
$is_silenced = isset($silenced[$viewer_phid]);
if ($request->isDialogFormPost()) {
if ($is_silenced) {
$edge_action = '-';
} else {
$edge_action = '+';
}
$xactions = array();
$xactions[] = id(new PhabricatorProjectTransaction())
->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
->setMetadataValue('edge:type', $edge_type)
->setNewValue(
array(
$edge_action => array($viewer_phid => $viewer_phid),
));
$editor = id(new PhabricatorProjectTransactionEditor($project))
->setActor($viewer)
->setContentSourceFromRequest($request)
->setContinueOnNoEffect(true)
->setContinueOnMissingFields(true)
->applyTransactions($project, $xactions);
return id(new AphrontRedirectResponse())->setURI($done_uri);
}
if ($is_silenced) {
$title = pht('Enable Mail');
$body = pht(
'When mail is sent to members of this project, you will receive a '.
'copy.');
$button = pht('Enable Project Mail');
} else {
$title = pht('Disable Mail');
$body = pht(
'When mail is sent to members of this project, you will no longer '.
'receive a copy.');
$button = pht('Disable Project Mail');
}
return $this->newDialog()
->setTitle($title)
->appendParagraph($body)
->addCancelButton($done_uri)
->addSubmitButton($button);
}
}

View file

@ -30,11 +30,9 @@ final class PhabricatorProjectWatchController
switch ($action) { switch ($action) {
case 'watch': case 'watch':
$edge_action = '+'; $edge_action = '+';
$force_subscribe = true;
break; break;
case 'unwatch': case 'unwatch':
$edge_action = '-'; $edge_action = '-';
$force_subscribe = false;
break; break;
} }

View file

@ -0,0 +1,8 @@
<?php
final class PhabricatorProjectSilencedEdgeType
extends PhabricatorEdgeType {
const EDGECONST = 61;
}

View file

@ -178,51 +178,6 @@ final class PhabricatorProjectTransactionEditor
return parent::applyCustomExternalTransaction($object, $xaction); return parent::applyCustomExternalTransaction($object, $xaction);
} }
protected function applyBuiltinExternalTransaction(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
switch ($xaction->getTransactionType()) {
case PhabricatorTransactions::TYPE_EDGE:
$edge_type = $xaction->getMetadataValue('edge:type');
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, 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));
// NOTE: The subscribe is "explicit" because there's no implicit
// unsubscribe, so Join -> Leave -> Join doesn't resubscribe you
// if we use an implicit subscribe, even though you never willfully
// unsubscribed. Not sure if adding implicit unsubscribe (which
// would not write the unsubscribe row) is justified to deal with
// this, which is a fairly weird edge case and pretty arguable both
// ways.
id(new PhabricatorSubscriptionsEditor())
->setActor($this->requireActor())
->setObject($object)
->subscribeExplicit($add)
->unsubscribe($rem)
->save();
break;
}
break;
}
return parent::applyBuiltinExternalTransaction($object, $xaction);
}
protected function validateAllTransactions( protected function validateAllTransactions(
PhabricatorLiskDAO $object, PhabricatorLiskDAO $object,
array $xactions) { array $xactions) {
@ -584,6 +539,10 @@ final class PhabricatorProjectTransactionEditor
); );
} }
protected function getMailCc(PhabricatorLiskDAO $object) {
return array();
}
public function getMailTagsMap() { public function getMailTagsMap() {
return array( return array(
PhabricatorProjectTransaction::MAILTAG_METADATA => PhabricatorProjectTransaction::MAILTAG_METADATA =>
@ -592,8 +551,6 @@ final class PhabricatorProjectTransactionEditor
pht('Project membership changes.'), pht('Project membership changes.'),
PhabricatorProjectTransaction::MAILTAG_WATCHERS => PhabricatorProjectTransaction::MAILTAG_WATCHERS =>
pht('Project watcher list changes.'), pht('Project watcher list changes.'),
PhabricatorProjectTransaction::MAILTAG_SUBSCRIBERS =>
pht('Project subscribers change.'),
PhabricatorProjectTransaction::MAILTAG_OTHER => PhabricatorProjectTransaction::MAILTAG_OTHER =>
pht('Other project activity not listed above occurs.'), pht('Other project activity not listed above occurs.'),
); );

View file

@ -148,7 +148,6 @@ final class PhabricatorProjectEditEngine
'icon', 'icon',
'color', 'color',
'slugs', 'slugs',
'subscriberPHIDs',
)); ));
return array( return array(

View file

@ -6,7 +6,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO
PhabricatorFlaggableInterface, PhabricatorFlaggableInterface,
PhabricatorPolicyInterface, PhabricatorPolicyInterface,
PhabricatorExtendedPolicyInterface, PhabricatorExtendedPolicyInterface,
PhabricatorSubscribableInterface,
PhabricatorCustomFieldInterface, PhabricatorCustomFieldInterface,
PhabricatorDestructibleInterface, PhabricatorDestructibleInterface,
PhabricatorFulltextInterface, PhabricatorFulltextInterface,
@ -179,7 +178,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO
return $extended; return $extended;
} }
public function isUserMember($user_phid) { public function isUserMember($user_phid) {
if ($this->memberPHIDs !== self::ATTACHABLE) { if ($this->memberPHIDs !== self::ATTACHABLE) {
return in_array($user_phid, $this->memberPHIDs); return in_array($user_phid, $this->memberPHIDs);
@ -536,19 +534,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO
} }
/* -( PhabricatorSubscribableInterface )----------------------------------- */
public function isAutomaticallySubscribed($phid) {
return false;
}
public function shouldShowSubscribersProperty() {
return false;
}
/* -( PhabricatorCustomFieldInterface )------------------------------------ */ /* -( PhabricatorCustomFieldInterface )------------------------------------ */

View file

@ -18,7 +18,6 @@ final class PhabricatorProjectTransaction
const MAILTAG_METADATA = 'project-metadata'; const MAILTAG_METADATA = 'project-metadata';
const MAILTAG_MEMBERS = 'project-members'; const MAILTAG_MEMBERS = 'project-members';
const MAILTAG_SUBSCRIBERS = 'project-subscribers';
const MAILTAG_WATCHERS = 'project-watchers'; const MAILTAG_WATCHERS = 'project-watchers';
const MAILTAG_OTHER = 'project-other'; const MAILTAG_OTHER = 'project-other';
@ -382,9 +381,6 @@ final class PhabricatorProjectTransaction
case self::TYPE_COLOR: case self::TYPE_COLOR:
$tags[] = self::MAILTAG_METADATA; $tags[] = self::MAILTAG_METADATA;
break; break;
case PhabricatorTransactions::TYPE_SUBSCRIBERS:
$tags[] = self::MAILTAG_SUBSCRIBERS;
break;
case PhabricatorTransactions::TYPE_EDGE: case PhabricatorTransactions::TYPE_EDGE:
$type = $this->getMetadata('edge:type'); $type = $this->getMetadata('edge:type');
$type = head($type); $type = head($type);