From 8463ad2659607886a9cea7d5596ce51a068059c5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Jan 2016 18:56:15 -0800 Subject: [PATCH] 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 --- .../20160119.project.1.silence.sql | 8 ++ src/__phutil_library_map__.php | 5 +- .../query/PhabricatorMetaMTAMemberQuery.php | 38 ++++++-- .../PhabricatorProjectApplication.php | 2 + ...habricatorProjectMembersViewController.php | 80 ++++++++++++++++- .../PhabricatorProjectSilenceController.php | 87 +++++++++++++++++++ .../PhabricatorProjectWatchController.php | 2 - .../PhabricatorProjectSilencedEdgeType.php | 8 ++ .../PhabricatorProjectTransactionEditor.php | 51 +---------- .../engine/PhabricatorProjectEditEngine.php | 1 - .../project/storage/PhabricatorProject.php | 15 ---- .../storage/PhabricatorProjectTransaction.php | 4 - 12 files changed, 225 insertions(+), 76 deletions(-) create mode 100644 resources/sql/autopatches/20160119.project.1.silence.sql create mode 100644 src/applications/project/controller/PhabricatorProjectSilenceController.php create mode 100644 src/applications/project/edge/PhabricatorProjectSilencedEdgeType.php diff --git a/resources/sql/autopatches/20160119.project.1.silence.sql b/resources/sql/autopatches/20160119.project.1.silence.sql new file mode 100644 index 0000000000..3a4dd07fd2 --- /dev/null +++ b/resources/sql/autopatches/20160119.project.1.silence.sql @@ -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; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5fc682cfd0..d606f6c298 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2925,6 +2925,8 @@ phutil_register_library_map(array( 'PhabricatorProjectSchemaSpec' => 'applications/project/storage/PhabricatorProjectSchemaSpec.php', 'PhabricatorProjectSearchEngine' => 'applications/project/query/PhabricatorProjectSearchEngine.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', 'PhabricatorProjectStandardCustomField' => 'applications/project/customfield/PhabricatorProjectStandardCustomField.php', 'PhabricatorProjectStatus' => 'applications/project/constants/PhabricatorProjectStatus.php', @@ -7237,7 +7239,6 @@ phutil_register_library_map(array( 'PhabricatorFlaggableInterface', 'PhabricatorPolicyInterface', 'PhabricatorExtendedPolicyInterface', - 'PhabricatorSubscribableInterface', 'PhabricatorCustomFieldInterface', 'PhabricatorDestructibleInterface', 'PhabricatorFulltextInterface', @@ -7329,6 +7330,8 @@ phutil_register_library_map(array( 'PhabricatorProjectSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorProjectSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorProjectSearchField' => 'PhabricatorSearchTokenizerField', + 'PhabricatorProjectSilenceController' => 'PhabricatorProjectController', + 'PhabricatorProjectSilencedEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectSlug' => 'PhabricatorProjectDAO', 'PhabricatorProjectStandardCustomField' => array( 'PhabricatorProjectCustomField', diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php index f565baf595..e06da5f4e3 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php @@ -42,20 +42,48 @@ final class PhabricatorMetaMTAMemberQuery extends PhabricatorQuery { $projects = id(new PhabricatorProjectQuery()) ->setViewer($this->getViewer()) ->withPHIDs($phids) + ->needMembers(true) + ->needWatchers(true) ->execute(); - $subscribers = id(new PhabricatorSubscribersQuery()) - ->withObjectPHIDs($phids) - ->execute(); + $edge_type = PhabricatorProjectSilencedEdgeType::EDGECONST; + + $edge_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($phids) + ->withEdgeTypes( + array( + $edge_type, + )); + + $edge_query->execute(); $projects = mpull($projects, null, 'getPHID'); foreach ($phids as $phid) { $project = idx($projects, $phid); + if (!$project) { $results[$phid] = array(); - } else { - $results[$phid] = idx($subscribers, $phid, array()); + continue; } + + // 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; default: diff --git a/src/applications/project/application/PhabricatorProjectApplication.php b/src/applications/project/application/PhabricatorProjectApplication.php index c25a1b223f..5f6f7add9f 100644 --- a/src/applications/project/application/PhabricatorProjectApplication.php +++ b/src/applications/project/application/PhabricatorProjectApplication.php @@ -89,6 +89,8 @@ final class PhabricatorProjectApplication extends PhabricatorApplication { 'history/(?P[1-9]\d*)/' => 'PhabricatorProjectHistoryController', '(?Pwatch|unwatch)/(?P[1-9]\d*)/' => 'PhabricatorProjectWatchController', + 'silence/(?P[1-9]\d*)/' + => 'PhabricatorProjectSilenceController', ), '/tag/' => array( '(?P[^/]+)/' => 'PhabricatorProjectViewController', diff --git a/src/applications/project/controller/PhabricatorProjectMembersViewController.php b/src/applications/project/controller/PhabricatorProjectMembersViewController.php index 9189b1ad8d..efe2106a26 100644 --- a/src/applications/project/controller/PhabricatorProjectMembersViewController.php +++ b/src/applications/project/controller/PhabricatorProjectMembersViewController.php @@ -110,6 +110,49 @@ final class PhabricatorProjectMembersViewController $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; } @@ -136,7 +179,9 @@ final class PhabricatorProjectMembersViewController $can_leave = $supports_edit && (!$is_locked || $can_edit); - if (!$project->isUserMember($viewer->getPHID())) { + $viewer_phid = $viewer->getPHID(); + + if (!$project->isUserMember($viewer_phid)) { $view->addAction( id(new PhabricatorActionView()) ->setHref('/project/update/'.$project->getID().'/join/') @@ -170,6 +215,23 @@ final class PhabricatorProjectMembersViewController ->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; $view->addAction( @@ -202,4 +264,20 @@ final class PhabricatorProjectMembersViewController 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]); + } + } diff --git a/src/applications/project/controller/PhabricatorProjectSilenceController.php b/src/applications/project/controller/PhabricatorProjectSilenceController.php new file mode 100644 index 0000000000..6edd9bab39 --- /dev/null +++ b/src/applications/project/controller/PhabricatorProjectSilenceController.php @@ -0,0 +1,87 @@ +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); + } + +} diff --git a/src/applications/project/controller/PhabricatorProjectWatchController.php b/src/applications/project/controller/PhabricatorProjectWatchController.php index d8b818cb63..263c508441 100644 --- a/src/applications/project/controller/PhabricatorProjectWatchController.php +++ b/src/applications/project/controller/PhabricatorProjectWatchController.php @@ -30,11 +30,9 @@ final class PhabricatorProjectWatchController switch ($action) { case 'watch': $edge_action = '+'; - $force_subscribe = true; break; case 'unwatch': $edge_action = '-'; - $force_subscribe = false; break; } diff --git a/src/applications/project/edge/PhabricatorProjectSilencedEdgeType.php b/src/applications/project/edge/PhabricatorProjectSilencedEdgeType.php new file mode 100644 index 0000000000..1fbc1a9ab7 --- /dev/null +++ b/src/applications/project/edge/PhabricatorProjectSilencedEdgeType.php @@ -0,0 +1,8 @@ +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( PhabricatorLiskDAO $object, array $xactions) { @@ -584,6 +539,10 @@ final class PhabricatorProjectTransactionEditor ); } + protected function getMailCc(PhabricatorLiskDAO $object) { + return array(); + } + public function getMailTagsMap() { return array( PhabricatorProjectTransaction::MAILTAG_METADATA => @@ -592,8 +551,6 @@ final class PhabricatorProjectTransactionEditor pht('Project membership changes.'), PhabricatorProjectTransaction::MAILTAG_WATCHERS => pht('Project watcher list changes.'), - PhabricatorProjectTransaction::MAILTAG_SUBSCRIBERS => - pht('Project subscribers change.'), PhabricatorProjectTransaction::MAILTAG_OTHER => pht('Other project activity not listed above occurs.'), ); diff --git a/src/applications/project/engine/PhabricatorProjectEditEngine.php b/src/applications/project/engine/PhabricatorProjectEditEngine.php index 42a2000c86..c9861e96a6 100644 --- a/src/applications/project/engine/PhabricatorProjectEditEngine.php +++ b/src/applications/project/engine/PhabricatorProjectEditEngine.php @@ -148,7 +148,6 @@ final class PhabricatorProjectEditEngine 'icon', 'color', 'slugs', - 'subscriberPHIDs', )); return array( diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index 2ea3a2c131..5f4d511158 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -6,7 +6,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO PhabricatorFlaggableInterface, PhabricatorPolicyInterface, PhabricatorExtendedPolicyInterface, - PhabricatorSubscribableInterface, PhabricatorCustomFieldInterface, PhabricatorDestructibleInterface, PhabricatorFulltextInterface, @@ -179,7 +178,6 @@ final class PhabricatorProject extends PhabricatorProjectDAO return $extended; } - public function isUserMember($user_phid) { if ($this->memberPHIDs !== self::ATTACHABLE) { 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 )------------------------------------ */ diff --git a/src/applications/project/storage/PhabricatorProjectTransaction.php b/src/applications/project/storage/PhabricatorProjectTransaction.php index b16a4e30ca..1186e9c4f7 100644 --- a/src/applications/project/storage/PhabricatorProjectTransaction.php +++ b/src/applications/project/storage/PhabricatorProjectTransaction.php @@ -18,7 +18,6 @@ final class PhabricatorProjectTransaction const MAILTAG_METADATA = 'project-metadata'; const MAILTAG_MEMBERS = 'project-members'; - const MAILTAG_SUBSCRIBERS = 'project-subscribers'; const MAILTAG_WATCHERS = 'project-watchers'; const MAILTAG_OTHER = 'project-other'; @@ -382,9 +381,6 @@ final class PhabricatorProjectTransaction case self::TYPE_COLOR: $tags[] = self::MAILTAG_METADATA; break; - case PhabricatorTransactions::TYPE_SUBSCRIBERS: - $tags[] = self::MAILTAG_SUBSCRIBERS; - break; case PhabricatorTransactions::TYPE_EDGE: $type = $this->getMetadata('edge:type'); $type = head($type);