diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -496,7 +496,6 @@ $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 @@ continue; } - if ($audit->getAuditStatus() != $status_resigned) { + if (!$audit->isResigned()) { $phids[] = $audit->getAuditorPHID(); } } @@ -514,6 +513,18 @@ 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 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -644,6 +644,18 @@ 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 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -820,9 +820,15 @@ } 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 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -6,6 +6,7 @@ private $applicationEmail; private $actor; private $excludePHIDs = array(); + private $unexpandablePHIDs = array(); final public function setMailReceiver($mail_receiver) { $this->validateMailReceiver($mail_receiver); @@ -45,6 +46,15 @@ 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 @@ $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 @@ ->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 --- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php +++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php @@ -72,6 +72,15 @@ 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 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -657,7 +657,8 @@ 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 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -77,6 +77,7 @@ private $oldTo = array(); private $oldCC = array(); private $mailRemovedPHIDs = array(); + private $mailUnexpandablePHIDs = array(); private $transactionQueue = array(); @@ -1204,6 +1205,7 @@ $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 @@ $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 @@ } + protected function newMailUnexpandablePHIDs(PhabricatorLiskDAO $object) { + return array(); + } + + /** * @task mail */ @@ -3617,6 +3630,7 @@ 'mailShouldSend', 'mustEncrypt', 'mailStamps', + 'mailUnexpandablePHIDs', ); }