From b1243e549c6235258a45b577d469f73555a60d06 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 11 Feb 2014 07:45:56 -0800 Subject: [PATCH] Allow unsubscription from projects Summary: Fixes T4379. Several changes: - Migrate all project members into subscribers. - When members are added or removed, subscribe or unsubscribe them. - Show sub/unsub in the UI. - Determine mailable membership of projects by querying subscribers. Test Plan: - As `duck`, joined a project. - Added the project as a reviewer to a revision. - Commented on the revision. - Observed `duck` receive mail. - Unsubscribed as `duck`. - Observed no mail. - Resubscribed as `duck`. - Mail again. - Joined/left project, checked sub/unsub status. - Ran migration, looked at database. Reviewers: btrahan Reviewed By: btrahan CC: aran, asherkin Maniphest Tasks: T4379 Differential Revision: https://secure.phabricator.com/D8189 --- .../20140210.projcfield.4.memmig.sql | 8 ++++++ .../query/PhabricatorMetaMTAMemberQuery.php | 12 ++++---- .../PhabricatorProjectTransactionEditor.php | 28 ++++++++++++++++++- .../project/storage/PhabricatorProject.php | 2 +- 4 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 resources/sql/autopatches/20140210.projcfield.4.memmig.sql diff --git a/resources/sql/autopatches/20140210.projcfield.4.memmig.sql b/resources/sql/autopatches/20140210.projcfield.4.memmig.sql new file mode 100644 index 0000000000..dde525bd29 --- /dev/null +++ b/resources/sql/autopatches/20140210.projcfield.4.memmig.sql @@ -0,0 +1,8 @@ +/* These are here so `grep` will find them if we ever change things: */ + +/* PhabricatorEdgeConfig::TYPE_PROJ_MEMBER = 13 */ +/* PhabricatorEdgeConfig::TYPE_OBJECT_HAS_SUBSCRIBER = 21 */ + +INSERT IGNORE INTO {$NAMESPACE}_project.edge (src, type, dst, dateCreated) + SELECT src, 21, dst, dateCreated FROM {$NAMESPACE}_project.edge + WHERE type = 13; diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php index b0be1ab34e..f270f0089d 100644 --- a/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php @@ -37,24 +37,24 @@ final class PhabricatorMetaMTAMemberQuery extends PhabricatorQuery { foreach ($type_map as $type => $phids) { switch ($type) { case PhabricatorProjectPHIDTypeProject::TYPECONST: - // TODO: For now, project members are always on the "mailing list" - // implied by the project, but we should differentiate members and - // subscribers (i.e., allow you to unsubscribe from mail about - // a project). + // NOTE: We're loading the projects here in order to respect policies. $projects = id(new PhabricatorProjectQuery()) ->setViewer($this->getViewer()) - ->needMembers(true) ->withPHIDs($phids) ->execute(); + $subscribers = id(new PhabricatorSubscribersQuery()) + ->withObjectPHIDs($phids) + ->execute(); + $projects = mpull($projects, null, 'getPHID'); foreach ($phids as $phid) { $project = idx($projects, $phid); if (!$project) { $results[$phid] = array(); } else { - $results[$phid] = $project->getMemberPHIDs(); + $results[$phid] = idx($subscribers, $phid, array()); } } break; diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index 43cc9924a8..4af23f781e 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -118,10 +118,36 @@ final class PhabricatorProjectTransactionEditor case PhabricatorTransactions::TYPE_VIEW_POLICY: case PhabricatorTransactions::TYPE_EDIT_POLICY: case PhabricatorTransactions::TYPE_JOIN_POLICY: - case PhabricatorTransactions::TYPE_EDGE: case PhabricatorProjectTransaction::TYPE_STATUS: case PhabricatorProjectTransaction::TYPE_IMAGE: return; + case PhabricatorTransactions::TYPE_EDGE: + switch ($xaction->getMetadataValue('edge:type')) { + case PhabricatorEdgeConfig::TYPE_PROJ_MEMBER: + // When project members are added or removed, add or remove their + // subscriptions. + $old = $xaction->getOldValue(); + $new = $xaction->getNewValue(); + $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; + } + return; } return parent::applyCustomExternalTransaction($object, $xaction); diff --git a/src/applications/project/storage/PhabricatorProject.php b/src/applications/project/storage/PhabricatorProject.php index 560f2e0797..c0979186cb 100644 --- a/src/applications/project/storage/PhabricatorProject.php +++ b/src/applications/project/storage/PhabricatorProject.php @@ -171,7 +171,7 @@ final class PhabricatorProject extends PhabricatorProjectDAO } public function shouldAllowSubscription($phid) { - return false; + return $this->isUserMember($phid); }