From 1cd3a593784a95c9925bce443d790aaa6c90e996 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 7 Feb 2018 07:08:36 -0800 Subject: [PATCH] When users resign from revisions, stop expanding projects/packages to include them Summary: Depends on D19019. Ref T13053. Fixes T12689. See PHI178. Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable. When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package). `@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber. Test Plan: - Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail. - Resigned as ducker, stopped getting future mail. - Subscribed explicitly, got mail again. - (Plus some `var_dump()` sanity checking in the internals.) Reviewers: amckinley Maniphest Tasks: T13053, T12689 Differential Revision: https://secure.phabricator.com/D19021 --- .../audit/editor/PhabricatorAuditEditor.php | 15 ++++++++-- .../editor/DifferentialTransactionEditor.php | 12 ++++++++ .../storage/DifferentialRevision.php | 10 +++++-- .../PhabricatorMailReplyHandler.php | 30 +++++++++++++++++++ .../PhabricatorRepositoryAuditRequest.php | 9 ++++++ .../storage/PhabricatorRepositoryCommit.php | 3 +- ...habricatorApplicationTransactionEditor.php | 14 +++++++++ 7 files changed, 88 insertions(+), 5 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index c39be75366..d142bd60cd 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -496,7 +496,6 @@ final class PhabricatorAuditEditor $phids[] = $object->getAuthorPHID(); } - $status_resigned = PhabricatorAuditStatusConstants::RESIGNED; foreach ($object->getAudits() as $audit) { if (!$audit->isInteresting()) { // Don't send mail to uninteresting auditors, like packages which @@ -504,7 +503,7 @@ final class PhabricatorAuditEditor continue; } - if ($audit->getAuditStatus() != $status_resigned) { + if (!$audit->isResigned()) { $phids[] = $audit->getAuditorPHID(); } } @@ -514,6 +513,18 @@ final class PhabricatorAuditEditor return $phids; } + protected function newMailUnexpandablePHIDs(PhabricatorLiskDAO $object) { + $phids = array(); + + foreach ($object->getAudits() as $auditor) { + if ($auditor->isResigned()) { + $phids[] = $auditor->getAuditorPHID(); + } + } + + return $phids; + } + protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index f3583438c8..cc29d69fe0 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -644,6 +644,18 @@ final class DifferentialTransactionEditor return $phids; } + protected function newMailUnexpandablePHIDs(PhabricatorLiskDAO $object) { + $phids = array(); + + foreach ($object->getReviewers() as $reviewer) { + if ($reviewer->isResigned()) { + $phids[] = $reviewer->getReviewerPHID(); + } + } + + return $phids; + } + protected function getMailAction( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 938c588857..4591315207 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -820,9 +820,15 @@ final class DifferentialRevision extends DifferentialDAO } foreach ($reviewers as $reviewer) { - if ($reviewer->getReviewerPHID() == $phid) { - return true; + if ($reviewer->getReviewerPHID() !== $phid) { + continue; } + + if ($reviewer->isResigned()) { + continue; + } + + return true; } return false; diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index b0ae2de494..f8dd784e3b 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -6,6 +6,7 @@ abstract class PhabricatorMailReplyHandler extends Phobject { private $applicationEmail; private $actor; private $excludePHIDs = array(); + private $unexpandablePHIDs = array(); final public function setMailReceiver($mail_receiver) { $this->validateMailReceiver($mail_receiver); @@ -45,6 +46,15 @@ abstract class PhabricatorMailReplyHandler extends Phobject { return $this->excludePHIDs; } + public function setUnexpandablePHIDs(array $phids) { + $this->unexpandablePHIDs = $phids; + return $this; + } + + public function getUnexpandablePHIDs() { + return $this->unexpandablePHIDs; + } + abstract public function validateMailReceiver($mail_receiver); abstract public function getPrivateReplyHandlerEmailAddress( PhabricatorUser $user); @@ -297,6 +307,16 @@ abstract class PhabricatorMailReplyHandler extends Phobject { $to_result = array(); $cc_result = array(); + // "Unexpandable" users have disengaged from an object (for example, + // by resigning from a revision). + + // If such a user is still a direct recipient (for example, they're still + // on the Subscribers list) they're fair game, but group targets (like + // projects) will no longer include them when expanded. + + $unexpandable = $this->getUnexpandablePHIDs(); + $unexpandable = array_fuse($unexpandable); + $all_phids = array_merge($to, $cc); if ($all_phids) { $map = id(new PhabricatorMetaMTAMemberQuery()) @@ -305,11 +325,21 @@ abstract class PhabricatorMailReplyHandler extends Phobject { ->execute(); foreach ($to as $phid) { foreach ($map[$phid] as $expanded) { + if ($expanded !== $phid) { + if (isset($unexpandable[$expanded])) { + continue; + } + } $to_result[$expanded] = $expanded; } } foreach ($cc as $phid) { foreach ($map[$phid] as $expanded) { + if ($expanded !== $phid) { + if (isset($unexpandable[$expanded])) { + continue; + } + } $cc_result[$expanded] = $expanded; } } diff --git a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php index ea86594d9e..e05820c825 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php +++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php @@ -72,6 +72,15 @@ final class PhabricatorRepositoryAuditRequest return true; } + public function isResigned() { + switch ($this->getAuditStatus()) { + case PhabricatorAuditStatusConstants::RESIGNED: + return true; + } + + return false; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 31a06dcbd4..1c5998d583 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -657,7 +657,8 @@ final class PhabricatorRepositoryCommit public function isAutomaticallySubscribed($phid) { // TODO: This should also list auditors, but handling that is a bit messy - // right now because we are not guaranteed to have the data. + // right now because we are not guaranteed to have the data. (It should not + // include resigned auditors.) return ($phid == $this->getAuthorPHID()); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 6bb7679fdc..167aa05fed 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -77,6 +77,7 @@ abstract class PhabricatorApplicationTransactionEditor private $oldTo = array(); private $oldCC = array(); private $mailRemovedPHIDs = array(); + private $mailUnexpandablePHIDs = array(); private $transactionQueue = array(); @@ -1204,6 +1205,7 @@ abstract class PhabricatorApplicationTransactionEditor $this->mailShouldSend = true; $this->mailToPHIDs = $this->getMailTo($object); $this->mailCCPHIDs = $this->getMailCC($object); + $this->mailUnexpandablePHIDs = $this->newMailUnexpandablePHIDs($object); // Add any recipients who were previously on the notification list // but were removed by this change. @@ -2562,7 +2564,13 @@ abstract class PhabricatorApplicationTransactionEditor $email_cc = $this->mailCCPHIDs; $email_cc = array_merge($email_cc, $this->heraldEmailPHIDs); + $unexpandable = $this->mailUnexpandablePHIDs; + if (!is_array($unexpandable)) { + $unexpandable = array(); + } + $targets = $this->buildReplyHandler($object) + ->setUnexpandablePHIDs($unexpandable) ->getMailTargets($email_to, $email_cc); // Set this explicitly before we start swapping out the effective actor. @@ -2817,6 +2825,11 @@ abstract class PhabricatorApplicationTransactionEditor } + protected function newMailUnexpandablePHIDs(PhabricatorLiskDAO $object) { + return array(); + } + + /** * @task mail */ @@ -3617,6 +3630,7 @@ abstract class PhabricatorApplicationTransactionEditor 'mailShouldSend', 'mustEncrypt', 'mailStamps', + 'mailUnexpandablePHIDs', ); }