From eca7d3fedae56ea4ba83bd6c48286d4b53ab4e02 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 1 Feb 2014 14:35:55 -0800 Subject: [PATCH] Expand aggregate email recipients prior to multiplexing Summary: Ref T4361. Before we figure out which To/CC are addressable, try to expand To/CC. Specifically, the supported expansion right now is project PHIDs expanding to all their members. Because of the way multiplexing works, we have to do this in two places: explicitly in `multiplexMail()`, and when sending mail that wasn't multiplexed. This is messy; eventually we can get rid of it (after ApplicationTransactions are everywhere). This has some rough edges, but should basically give us what we need to make stuff like projects mailable. Particularly, it deals with most issues in D7436: - I got around the resolution/multiplexing issue by resolving aggregate mailables separately from mailable actors. - We get to keep the Project PHID as a To/CC/Reviewer/Whatever until the last second. - Users won't get two emails for being a CC and also a member of a CC'd project. - We can degrade to the list stuff this way if we want, by having the project aggregate yield a single list PHID. Test Plan: Added a comment to a revision with a project reviewer, got mail to all the project's members. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4361 Differential Revision: https://secure.phabricator.com/D8117 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorMetaMTAMemberQuery.php | 69 +++++++++++++++++++ .../PhabricatorMailReplyHandler.php | 39 +++++++++++ .../storage/PhabricatorMetaMTAMail.php | 52 +++++++++++++- 4 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cda38cbf79..2692f975ce 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1648,6 +1648,7 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php', 'PhabricatorMetaMTAMailgunReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php', 'PhabricatorMetaMTAMailingList' => 'applications/mailinglists/storage/PhabricatorMetaMTAMailingList.php', + 'PhabricatorMetaMTAMemberQuery' => 'applications/metamta/query/PhabricatorMetaMTAMemberQuery.php', 'PhabricatorMetaMTAPermanentFailureException' => 'applications/metamta/exception/PhabricatorMetaMTAPermanentFailureException.php', 'PhabricatorMetaMTAReceivedMail' => 'applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php', 'PhabricatorMetaMTAReceivedMailProcessingException' => 'applications/metamta/exception/PhabricatorMetaMTAReceivedMailProcessingException.php', @@ -4325,6 +4326,7 @@ phutil_register_library_map(array( 0 => 'PhabricatorMetaMTADAO', 1 => 'PhabricatorPolicyInterface', ), + 'PhabricatorMetaMTAMemberQuery' => 'PhabricatorQuery', 'PhabricatorMetaMTAPermanentFailureException' => 'Exception', 'PhabricatorMetaMTAReceivedMail' => 'PhabricatorMetaMTADAO', 'PhabricatorMetaMTAReceivedMailProcessingException' => 'Exception', diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php new file mode 100644 index 0000000000..b0be1ab34e --- /dev/null +++ b/src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php @@ -0,0 +1,69 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function execute() { + $phids = array_fuse($this->phids); + $actors = array(); + $type_map = array(); + foreach ($phids as $phid) { + $type_map[phid_get_type($phid)][] = $phid; + } + + // TODO: Generalize this somewhere else. + + $results = array(); + 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). + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->needMembers(true) + ->withPHIDs($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(); + } + } + break; + default: + break; + } + } + + return $results; + } + +} diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index 4278ce2bc6..82adabbf05 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -198,6 +198,15 @@ EOBODY; } } + // TODO: This is pretty messy. We should really be doing all of this + // multiplexing in the task queue, but that requires significant rewriting + // in the general case. ApplicationTransactions can do it fairly easily, + // but other mail sites currently can not, so we need to support this + // junky version until they catch up and we can swap things over. + + $to_handles = $this->expandRecipientHandles($to_handles); + $cc_handles = $this->expandRecipientHandles($cc_handles); + $tos = mpull($to_handles, null, 'getPHID'); $ccs = mpull($cc_handles, null, 'getPHID'); @@ -219,6 +228,8 @@ EOBODY; $body .= $this->getRecipientsSummary($to_handles, $cc_handles); foreach ($recipients as $phid => $recipient) { + + $mail = clone $mail_template; if (isset($to_handles[$phid])) { $mail->addTos(array($phid)); @@ -332,4 +343,32 @@ EOBODY; return rtrim($body); } + private function expandRecipientHandles(array $handles) { + if (!$handles) { + return array(); + } + + $phids = mpull($handles, 'getPHID'); + $map = id(new PhabricatorMetaMTAMemberQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($phids) + ->execute(); + + $results = array(); + foreach ($phids as $phid) { + if (isset($map[$phid])) { + foreach ($map[$phid] as $expanded_phid) { + $results[$expanded_phid] = $expanded_phid; + } + } else { + $results[$phid] = $phid; + } + } + + return id(new PhabricatorHandleQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($results) + ->execute(); + } + } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index c0ce3fe061..98b8f508fa 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -19,6 +19,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { private $excludePHIDs = array(); private $overrideNoSelfMail = false; + private $recipientExpansionMap; public function __construct() { @@ -379,7 +380,8 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->addReplyTo($value, $reply_to_name); break; case 'to': - $to_actors = array_select_keys($deliverable_actors, $value); + $to_phids = $this->expandRecipients($value); + $to_actors = array_select_keys($deliverable_actors, $to_phids); $add_to = array_merge( $add_to, mpull($to_actors, 'getEmailAddress')); @@ -388,7 +390,8 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $add_to = array_merge($add_to, $value); break; case 'cc': - $cc_actors = array_select_keys($deliverable_actors, $value); + $cc_phids = $this->expandRecipients($value); + $cc_actors = array_select_keys($deliverable_actors, $cc_phids); $add_cc = array_merge( $add_cc, mpull($cc_actors, 'getEmailAddress')); @@ -685,9 +688,54 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $this->getToPHIDs(), $this->getCcPHIDs()); + $this->loadRecipientExpansions($actor_phids); + $actor_phids = $this->expandRecipients($actor_phids); + return $this->loadActors($actor_phids); } + private function loadRecipientExpansions(array $phids) { + $expansions = id(new PhabricatorMetaMTAMemberQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($phids) + ->execute(); + + $this->recipientExpansionMap = $expansions; + + return $this; + } + + /** + * Expand a list of recipient PHIDs (possibly including aggregate recipients + * like projects) into a deaggregated list of individual recipient PHIDs. + * For example, this will expand project PHIDs into a list of the project's + * members. + * + * @param list List of recipient PHIDs, possibly including aggregate + * recipients. + * @return list Deaggregated list of mailable recipients. + */ + private function expandRecipients(array $phids) { + if ($this->recipientExpansionMap === null) { + throw new Exception( + pht( + 'Call loadRecipientExpansions() before expandRecipients()!')); + } + + $results = array(); + foreach ($phids as $phid) { + if (!isset($this->recipientExpansionMap[$phid])) { + $results[$phid] = $phid; + } else { + foreach ($this->recipientExpansionMap[$phid] as $recipient_phid) { + $results[$recipient_phid] = $recipient_phid; + } + } + } + + return array_keys($results); + } + private function filterDeliverableActors(array $actors) { assert_instances_of($actors, 'PhabricatorMetaMTAActor'); $deliverable_actors = array();