From 64736264a62d825e8311d35bd0cd0151fc7e8dda Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 2 Aug 2014 00:06:45 -0700 Subject: [PATCH] Use standard infrastructure for Audit email generation Summary: Ref T4896. Replace custom stuff with standard stuff. Test Plan: - Sent a bunch of email and it all looked sensible/correct. - Made sure to test inlines, specifically, as they're a bit tricky. Reviewers: joshuaspence, btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4896 Differential Revision: https://secure.phabricator.com/D10112 --- .../editor/PhabricatorAuditCommentEditor.php | 225 +----------------- .../audit/editor/PhabricatorAuditEditor.php | 119 +++++++++ .../storage/PhabricatorAuditTransaction.php | 51 +++- 3 files changed, 173 insertions(+), 222 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 671497a10e..54f005bbe7 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -217,6 +217,8 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $commit->updateAuditStatus($requests); $commit->save(); + $commit->attachAudits($requests); + // Convert old comments into real transactions and apply them with a // normal editor. @@ -240,6 +242,8 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) ->setContentSource($content_source) + ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) + ->setDisableEmail($this->noEmail) ->applyTransactions($commit, $xactions); $feed_dont_publish_phids = array(); @@ -262,14 +266,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { foreach ($comments as $comment) { $this->publishFeedStory($comment, $feed_phids); } - - if (!$this->noEmail) { - $this->sendMail( - $comments, - $other_comments, - $inline_comments, - $requests); - } } @@ -337,217 +333,4 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { ->publish(); } - private function sendMail( - array $comments, - array $other_comments, - array $inline_comments, - array $requests) { - - assert_instances_of($comments, 'PhabricatorAuditComment'); - assert_instances_of($other_comments, 'PhabricatorAuditComment'); - assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface'); - - $commit = $this->commit; - - $data = $commit->loadCommitData(); - $summary = $data->getSummary(); - - $commit_phid = $commit->getPHID(); - $phids = array($commit_phid); - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($phids) - ->execute(); - $handle = $handles[$commit_phid]; - - $name = $handle->getName(); - - $map = array( - PhabricatorAuditActionConstants::CONCERN => 'Raised Concern', - PhabricatorAuditActionConstants::ACCEPT => 'Accepted', - PhabricatorAuditActionConstants::RESIGN => 'Resigned', - PhabricatorAuditActionConstants::CLOSE => 'Closed', - PhabricatorAuditActionConstants::ADD_CCS => 'Added CCs', - PhabricatorAuditActionConstants::ADD_AUDITORS => 'Added Auditors', - ); - if ($comments) { - $any_action = head($comments)->getAction(); - } else { - $any_action = null; - } - $verb = idx($map, $any_action, 'Commented On'); - - $reply_handler = self::newReplyHandlerForCommit($commit); - - $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); - - $repository = id(new PhabricatorRepositoryQuery()) - ->setViewer($this->getActor()) - ->withIDs(array($commit->getRepositoryID())) - ->executeOne(); - $threading = self::getMailThreading($repository, $commit); - list($thread_id, $thread_topic) = $threading; - - $body = $this->renderMailBody( - $comments, - "{$name}: {$summary}", - $handle, - $reply_handler, - $inline_comments); - - $email_to = array(); - $email_cc = array(); - - $email_to[$this->getActor()->getPHID()] = true; - - $author_phid = $data->getCommitDetail('authorPHID'); - if ($author_phid) { - $email_to[$author_phid] = true; - } - - foreach ($other_comments as $other_comment) { - $email_cc[$other_comment->getActorPHID()] = true; - } - - $subscribers = PhabricatorSubscribersQuery::loadSubscribersForPHID( - $commit->getPHID()); - foreach ($subscribers as $subscriber) { - $email_cc[$subscriber] = true; - } - - foreach ($requests as $request) { - switch ($request->getAuditStatus()) { - case PhabricatorAuditStatusConstants::AUDIT_REQUIRED: - $email_cc[$request->getAuditorPHID()] = true; - break; - case PhabricatorAuditStatusConstants::RESIGNED: - unset($email_cc[$request->getAuditorPHID()]); - break; - case PhabricatorAuditStatusConstants::CONCERNED: - case PhabricatorAuditStatusConstants::AUDIT_REQUESTED: - $email_to[$request->getAuditorPHID()] = true; - break; - } - } - - $email_to = array_keys($email_to); - $email_cc = array_keys($email_cc); - - $phids = array_merge($email_to, $email_cc); - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($this->getActor()) - ->withPHIDs($phids) - ->execute(); - - // NOTE: Always set $is_new to false, because the "first" mail in the - // thread is the Herald notification of the commit. - $is_new = false; - - $template = id(new PhabricatorMetaMTAMail()) - ->setSubject("{$name}: {$summary}") - ->setSubjectPrefix($prefix) - ->setVarySubjectPrefix("[{$verb}]") - ->setFrom($this->getActor()->getPHID()) - ->setThreadID($thread_id, $is_new) - ->addHeader('Thread-Topic', $thread_topic) - ->setRelatedPHID($commit->getPHID()) - ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) - ->setIsBulk(true) - ->setBody($body); - - $mails = $reply_handler->multiplexMail( - $template, - array_select_keys($handles, $email_to), - array_select_keys($handles, $email_cc)); - - foreach ($mails as $mail) { - $mail->saveAndSend(); - } - } - - public static function getMailThreading( - PhabricatorRepository $repository, - PhabricatorRepositoryCommit $commit) { - - return array( - 'diffusion-audit-'.$commit->getPHID(), - 'Commit r'.$repository->getCallsign().$commit->getCommitIdentifier(), - ); - } - - public static function newReplyHandlerForCommit($commit) { - $reply_handler = PhabricatorEnv::newObjectFromConfig( - 'metamta.diffusion.reply-handler'); - $reply_handler->setMailReceiver($commit); - return $reply_handler; - } - - private function renderMailBody( - array $comments, - $cname, - PhabricatorObjectHandle $handle, - PhabricatorMailReplyHandler $reply_handler, - array $inline_comments) { - - assert_instances_of($comments, 'PhabricatorAuditComment'); - assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface'); - - $commit = $this->commit; - $actor = $this->getActor(); - $name = $actor->getUsername(); - - $body = new PhabricatorMetaMTAMailBody(); - foreach ($comments as $comment) { - if ($comment->getAction() == PhabricatorAuditActionConstants::INLINE) { - continue; - } - - $verb = PhabricatorAuditActionConstants::getActionPastTenseVerb( - $comment->getAction()); - - $body->addRawSection("{$name} {$verb} commit {$cname}."); - - $content = $comment->getContent(); - if (strlen($content)) { - $body->addRawSection($comment->getContent()); - } - } - - if ($inline_comments) { - $block = array(); - - $path_map = id(new DiffusionPathQuery()) - ->withPathIDs(mpull($inline_comments, 'getPathID')) - ->execute(); - $path_map = ipull($path_map, 'path', 'id'); - - foreach ($inline_comments as $inline) { - $path = idx($path_map, $inline->getPathID()); - if ($path === null) { - continue; - } - - $start = $inline->getLineNumber(); - $len = $inline->getLineLength(); - if ($len) { - $range = $start.'-'.($start + $len); - } else { - $range = $start; - } - - $content = $inline->getContent(); - $block[] = "{$path}:{$range} {$content}"; - } - - $body->addTextSection(pht('INLINE COMMENTS'), implode("\n", $block)); - } - - $body->addTextSection( - pht('COMMIT'), - PhabricatorEnv::getProductionURI($handle->getURI())); - $body->addReplySection($reply_handler->getReplyHandlerInstructions()); - - return $body->render(); - } - } diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index b16864bad2..5aef278637 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -115,4 +115,123 @@ final class PhabricatorAuditEditor return true; } + protected function shouldSendMail( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + + protected function buildReplyHandler(PhabricatorLiskDAO $object) { + $reply_handler = PhabricatorEnv::newObjectFromConfig( + 'metamta.diffusion.reply-handler'); + $reply_handler->setMailReceiver($object); + return $reply_handler; + } + + protected function getMailSubjectPrefix() { + return PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); + } + + protected function getMailThreadID(PhabricatorLiskDAO $object) { + // For backward compatibility, use this legacy thread ID. + return 'diffusion-audit-'.$object->getPHID(); + } + + protected function buildMailTemplate(PhabricatorLiskDAO $object) { + $identifier = $object->getCommitIdentifier(); + $repository = $object->getRepository(); + $monogram = $repository->getMonogram(); + + $summary = $object->getSummary(); + $name = $repository->formatCommitName($identifier); + + $subject = "{$name}: {$summary}"; + $thread_topic = "Commit {$monogram}{$identifier}"; + + return id(new PhabricatorMetaMTAMail()) + ->setSubject($subject) + ->addHeader('Thread-Topic', $thread_topic); + } + + protected function getMailTo(PhabricatorLiskDAO $object) { + $phids = array(); + if ($object->getAuthorPHID()) { + $phids[] = $object->getAuthorPHID(); + } + + $status_resigned = PhabricatorAuditStatusConstants::RESIGNED; + foreach ($object->getAudits() as $audit) { + if ($audit->getAuditStatus() != $status_resigned) { + $phids[] = $audit->getAuditorPHID(); + } + } + + return $phids; + } + + protected function buildMailBody( + PhabricatorLiskDAO $object, + array $xactions) { + + $body = parent::buildMailBody($object, $xactions); + + $type_inline = PhabricatorAuditActionConstants::INLINE; + + $inlines = array(); + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() == $type_inline) { + $inlines[] = $xaction; + } + } + + if ($inlines) { + $body->addTextSection( + pht('INLINE COMMENTS'), + $this->renderInlineCommentsForMail($object, $inlines)); + } + + $monogram = $object->getRepository()->formatCommitName( + $object->getCommitIdentifier()); + + $body->addTextSection( + pht('COMMIT'), + PhabricatorEnv::getProductionURI('/'.$monogram)); + + return $body; + } + + private function renderInlineCommentsForMail( + PhabricatorLiskDAO $object, + array $inline_xactions) { + + $inlines = mpull($inline_xactions, 'getComment'); + + $block = array(); + + $path_map = id(new DiffusionPathQuery()) + ->withPathIDs(mpull($inlines, 'getPathID')) + ->execute(); + $path_map = ipull($path_map, 'path', 'id'); + + foreach ($inlines as $inline) { + $path = idx($path_map, $inline->getPathID()); + if ($path === null) { + continue; + } + + $start = $inline->getLineNumber(); + $len = $inline->getLineLength(); + if ($len) { + $range = $start.'-'.($start + $len); + } else { + $range = $start; + } + + $content = $inline->getContent(); + $block[] = "{$path}:{$range} {$content}"; + } + + return implode("\n", $block); + } + } diff --git a/src/applications/audit/storage/PhabricatorAuditTransaction.php b/src/applications/audit/storage/PhabricatorAuditTransaction.php index 10b1a3ecf0..80b1ef6433 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransaction.php +++ b/src/applications/audit/storage/PhabricatorAuditTransaction.php @@ -42,6 +42,28 @@ final class PhabricatorAuditTransaction return $phids; } + public function getActionName() { + + switch ($this->getTransactionType()) { + case PhabricatorAuditActionConstants::ACTION: + switch ($this->getNewValue()) { + case PhabricatorAuditActionConstants::CONCERN: + return pht('Raised Concern'); + case PhabricatorAuditActionConstants::ACCEPT: + return pht('Accepted'); + case PhabricatorAuditActionConstants::RESIGN: + return pht('Resigned'); + case PhabricatorAuditActionConstants::CLOSE: + return pht('Clsoed'); + } + break; + case PhabricatorAuditActionConstants::ADD_AUDITORS: + return pht('Added Auditors'); + } + + return parent::getActionName(); + } + public function getColor() { $type = $this->getTransactionType(); @@ -63,7 +85,7 @@ final class PhabricatorAuditTransaction $old = $this->getOldValue(); $new = $this->getNewValue(); - $author_handle = $this->getHandle($this->getAuthorPHID())->renderLink(); + $author_handle = $this->renderHandleLink($this->getAuthorPHID()); $type = $this->getTransactionType(); @@ -157,4 +179,31 @@ final class PhabricatorAuditTransaction return parent::getTitle(); } + // TODO: These two mail methods can likely be abstracted by introducing a + // formal concept of "inline comment" transactions. + + public function shouldHideForMail(array $xactions) { + $type_inline = PhabricatorAuditActionConstants::INLINE; + switch ($this->getTransactionType()) { + case $type_inline: + foreach ($xactions as $xaction) { + if ($xaction->getTransactionType() != $type_inline) { + return true; + } + } + return ($this !== head($xactions)); + } + + return parent::shouldHideForMail($xactions); + } + + public function getBodyForMail() { + switch ($this->getTransactionType()) { + case PhabricatorAuditActionConstants::INLINE: + return null; + } + + return parent::getBodyForMail(); + } + }