From 1aad40b7bf02aa750a01625c86b6fbe56cec8e59 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Mar 2014 10:44:06 -0700 Subject: [PATCH] Allow users to receive email about pushes via Herald Summary: Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries. For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed. Overall, this is basically the same as commit email, but more suitable for some workflows. Test Plan: Wrote some rules, then made a bunch of pushes. Got email like this: {F134929} Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4677 Differential Revision: https://secure.phabricator.com/D8618 --- src/__phutil_library_map__.php | 4 + .../diffusion/data/DiffusionCommitRef.php | 5 + .../engine/DiffusionCommitHookEngine.php | 45 ++++ .../herald/HeraldPreCommitAdapter.php | 18 +- .../herald/HeraldPreCommitContentAdapter.php | 2 +- .../herald/HeraldPreCommitRefAdapter.php | 2 +- .../herald/storage/HeraldRule.php | 2 +- .../PhabricatorRepositoryPushReplyHandler.php | 27 +++ .../PhabricatorRepositoryCommitData.php | 3 + .../storage/PhabricatorRepositoryPushLog.php | 27 ++- .../PhabricatorRepositoryPushMailWorker.php | 225 ++++++++++++++++++ .../PhabricatorBaseEnglishTranslation.php | 14 ++ 12 files changed, 364 insertions(+), 10 deletions(-) create mode 100644 src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php create mode 100644 src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 115dbafb78..b458054da2 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1971,6 +1971,8 @@ phutil_register_library_map(array( 'PhabricatorRepositoryPushLog' => 'applications/repository/storage/PhabricatorRepositoryPushLog.php', 'PhabricatorRepositoryPushLogQuery' => 'applications/repository/query/PhabricatorRepositoryPushLogQuery.php', 'PhabricatorRepositoryPushLogSearchEngine' => 'applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php', + 'PhabricatorRepositoryPushMailWorker' => 'applications/repository/worker/PhabricatorRepositoryPushMailWorker.php', + 'PhabricatorRepositoryPushReplyHandler' => 'applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php', 'PhabricatorRepositoryQuery' => 'applications/repository/query/PhabricatorRepositoryQuery.php', 'PhabricatorRepositoryRefCursor' => 'applications/repository/storage/PhabricatorRepositoryRefCursor.php', 'PhabricatorRepositoryRefCursorQuery' => 'applications/repository/query/PhabricatorRepositoryRefCursorQuery.php', @@ -4818,6 +4820,8 @@ phutil_register_library_map(array( ), 'PhabricatorRepositoryPushLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorRepositoryPushLogSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhabricatorRepositoryPushMailWorker' => 'PhabricatorWorker', + 'PhabricatorRepositoryPushReplyHandler' => 'PhabricatorMailReplyHandler', 'PhabricatorRepositoryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorRepositoryRefCursor' => array( diff --git a/src/applications/diffusion/data/DiffusionCommitRef.php b/src/applications/diffusion/data/DiffusionCommitRef.php index 5088eef00e..4d6b99d8a6 100644 --- a/src/applications/diffusion/data/DiffusionCommitRef.php +++ b/src/applications/diffusion/data/DiffusionCommitRef.php @@ -74,6 +74,11 @@ final class DiffusionCommitRef extends Phobject { return $this->formatUser($this->committerName, $this->committerEmail); } + public function getSummary() { + return PhabricatorRepositoryCommitData::summarizeCommitMessage( + $this->getMessage()); + } + private function formatUser($name, $email) { if (strlen($name) && strlen($email)) { return "{$name} <{$email}>"; diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 98f0d6331c..4d39a71385 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -32,6 +32,7 @@ final class DiffusionCommitHookEngine extends Phobject { private $heraldViewerProjects; private $rejectCode = PhabricatorRepositoryPushLog::REJECT_BROKEN; private $rejectDetails; + private $emailPHIDs = array(); /* -( Config )------------------------------------------------------------- */ @@ -172,6 +173,23 @@ final class DiffusionCommitHookEngine extends Phobject { throw $caught; } + if ($this->emailPHIDs) { + // If Herald rules triggered email to users, queue a worker to send the + // mail. We do this out-of-process so that we block pushes as briefly + // as possible. + + // (We do need to pull some commit info here because the commit objects + // may not exist yet when this worker runs, which could be immediately.) + + PhabricatorWorker::scheduleTask( + 'PhabricatorRepositoryPushMailWorker', + array( + 'eventPHID' => $event->getPHID(), + 'emailPHIDs' => array_values($this->emailPHIDs), + 'info' => $this->loadCommitInfoForWorker($all_updates), + )); + } + return 0; } @@ -282,6 +300,11 @@ final class DiffusionCommitHookEngine extends Phobject { $engine->applyEffects($effects, $adapter, $rules); $xscript = $engine->getTranscript(); + // Store any PHIDs we want to send email to for later. + foreach ($adapter->getEmailPHIDs() as $email_phid) { + $this->emailPHIDs[$email_phid] = $email_phid; + } + if ($blocking_effect === null) { foreach ($effects as $effect) { if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) { @@ -982,6 +1005,7 @@ final class DiffusionCommitHookEngine extends Phobject { return PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer()) ->setPHID($phid) ->setRepositoryPHID($this->getRepository()->getPHID()) + ->attachRepository($this->getRepository()) ->setEpoch(time()); } @@ -1091,5 +1115,26 @@ final class DiffusionCommitHookEngine extends Phobject { } } + private function loadCommitInfoForWorker(array $all_updates) { + $type_commit = PhabricatorRepositoryPushLog::REFTYPE_COMMIT; + + $map = array(); + foreach ($all_updates as $update) { + if ($update->getRefType() != $type_commit) { + continue; + } + $map[$update->getRefNew()] = array(); + } + + foreach ($map as $identifier => $info) { + $ref = $this->loadCommitRefForCommit($identifier); + $map[$identifier] += array( + 'summary' => $ref->getSummary(), + 'branches' => $this->loadBranches($identifier), + ); + } + + return $map; + } } diff --git a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php index 7bb908373c..d78de6be0b 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php @@ -4,6 +4,7 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { private $log; private $hookEngine; + private $emailPHIDs = array(); public function setPushLog(PhabricatorRepositoryPushLog $log) { $this->log = $log; @@ -27,12 +28,16 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { return $this->log; } + public function getEmailPHIDs() { + return array_values($this->emailPHIDs); + } + public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: - return true; case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: + return true; default: return false; } @@ -70,10 +75,12 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: return array( self::ACTION_BLOCK, + self::ACTION_EMAIL, self::ACTION_NOTHING ); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array( + self::ACTION_EMAIL, self::ACTION_NOTHING, ); } @@ -96,6 +103,15 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { true, pht('Did nothing.')); break; + case self::ACTION_EMAIL: + foreach ($effect->getTarget() as $phid) { + $this->emailPHIDs[$phid] = $phid; + } + $result[] = new HeraldApplyTranscript( + $effect, + true, + pht('Added mailable to mail targets.')); + break; case self::ACTION_BLOCK: $result[] = new HeraldApplyTranscript( $effect, diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index 3c9199fc83..5786cecda4 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -18,7 +18,7 @@ final class HeraldPreCommitContentAdapter extends HeraldPreCommitAdapter { public function getAdapterContentDescription() { return pht( "React to commits being pushed to hosted repositories.\n". - "Hook rules can block changes."); + "Hook rules can block changes and send push summary mail."); } public function getFields() { diff --git a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php index be7b42e648..a21eb6b0b0 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php @@ -20,7 +20,7 @@ final class HeraldPreCommitRefAdapter extends HeraldPreCommitAdapter { public function getAdapterContentDescription() { return pht( "React to branches and tags being pushed to hosted repositories.\n". - "Hook rules can block changes."); + "Hook rules can block changes and send push summary mail."); } public function getFieldNameMap() { diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index a911bbc4a0..11e83f1570 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/src/applications/herald/storage/HeraldRule.php @@ -17,7 +17,7 @@ final class HeraldRule extends HeraldDAO protected $isDisabled = 0; protected $triggerObjectPHID; - protected $configVersion = 34; + protected $configVersion = 35; // phids for which this rule has been applied private $ruleApplied = self::ATTACHABLE; diff --git a/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php b/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php new file mode 100644 index 0000000000..09f111868b --- /dev/null +++ b/src/applications/repository/mail/PhabricatorRepositoryPushReplyHandler.php @@ -0,0 +1,27 @@ +getCommitMessage(); + return self::summarizeCommitMessage($message); + } + public static function summarizeCommitMessage($message) { $summary = phutil_split_lines($message, $retain_endings = false); $summary = head($summary); $summary = phutil_utf8_shorten($summary, self::SUMMARY_MAX_LENGTH); diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index dadd581ba7..e027ae106c 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php @@ -45,6 +45,7 @@ final class PhabricatorRepositoryPushLog private $dangerousChangeDescription = self::ATTACHABLE; private $pushEvent = self::ATTACHABLE; + private $repository = self::ATTACHABLE; public static function initializeNewLog(PhabricatorUser $viewer) { return id(new PhabricatorRepositoryPushLog()) @@ -66,10 +67,6 @@ final class PhabricatorRepositoryPushLog PhabricatorRepositoryPHIDTypePushLog::TYPECONST); } - public function getRepository() { - return $this->getPushEvent()->getRepository(); - } - public function attachPushEvent(PhabricatorRepositoryPushEvent $push_event) { $this->pushEvent = $push_event; return $this; @@ -120,6 +117,21 @@ final class PhabricatorRepositoryPushLog return $this->assertAttached($this->dangerousChangeDescription); } + public function attachRepository(PhabricatorRepository $repository) { + // NOTE: Some gymnastics around this because of object construction order + // in the hook engine. Particularly, web build the logs before we build + // their push event. + $this->repository = $repository; + return $this; + } + + public function getRepository() { + if ($this->repository == self::ATTACHABLE) { + return $this->getPushEvent()->getRepository(); + } + return $this->assertAttached($this->repository); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -131,11 +143,14 @@ final class PhabricatorRepositoryPushLog } public function getPolicy($capability) { - return $this->getPushEvent()->getPolicy($capability); + // NOTE: We're passing through the repository rather than the push event + // mostly because we need to do policy checks in Herald before we create + // the event. The two approaches are equivalent in practice. + return $this->getRepository()->getPolicy($capability); } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { - return $this->getPushEvent()->hasAutomaticCapability($capability, $viewer); + return $this->getRepository()->hasAutomaticCapability($capability, $viewer); } public function describeAutomaticCapability($capability) { diff --git a/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php new file mode 100644 index 0000000000..883e547e58 --- /dev/null +++ b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php @@ -0,0 +1,225 @@ +getTaskData(); + + $email_phids = idx($task_data, 'emailPHIDs'); + if (!$email_phids) { + // If we don't have any email targets, don't send any email. + return; + } + + $event_phid = idx($task_data, 'eventPHID'); + $event = id(new PhabricatorRepositoryPushEventQuery()) + ->setViewer($viewer) + ->withPHIDs(array($event_phid)) + ->needLogs(true) + ->executeOne(); + + $repository = $event->getRepository(); + if ($repository->isImporting()) { + // If the repository is still importing, don't send email. + return; + } + + if ($repository->getDetail('herald-disabled')) { + // If publishing is disabled, don't send email. + return; + } + + $logs = $event->getLogs(); + + list($ref_lines, $ref_list) = $this->renderRefs($logs); + list($commit_lines, $subject_line) = $this->renderCommits( + $repository, + $logs, + idx($task_data, 'info', array())); + + $ref_count = count($ref_lines); + $commit_count = count($commit_lines); + + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($event->getPusherPHID())) + ->execute(); + + $pusher_name = $handles[$event->getPusherPHID()]->getName(); + $repo_name = $repository->getMonogram(); + + if ($commit_count) { + $overview = pht( + '%s pushed %d commit(s) to %s.', + $pusher_name, + $commit_count, + $repo_name); + } else { + $overview = pht( + '%s pushed to %s.', + $pusher_name, + $repo_name); + } + + $details_uri = PhabricatorEnv::getProductionURI( + '/diffusion/pushlog/view/'.$event->getID().'/'); + + $body = new PhabricatorMetaMTAMailBody(); + $body->addRawSection($overview); + + $body->addTextSection(pht('DETAILS'), $details_uri); + + if ($commit_lines) { + $body->addTextSection(pht('COMMITS'), implode("\n", $commit_lines)); + } + + if ($ref_lines) { + $body->addTextSection(pht('REFERENCES'), implode("\n", $ref_lines)); + } + + $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); + + $parts = array(); + if ($commit_count) { + $parts[] = pht('%s commit(s)', $commit_count); + } + if ($ref_count) { + $parts[] = implode(', ', $ref_list); + } + $parts = implode(', ', $parts); + + if ($subject_line) { + $subject = pht('(%s) %s', $parts, $subject_line); + } else { + $subject = pht('(%s)', $parts); + } + + $mail = id(new PhabricatorMetaMTAMail()) + ->setRelatedPHID($event->getPHID()) + ->setSubjectPrefix($prefix) + ->setVarySubjectPrefix(pht('[Push]')) + ->setSubject($subject) + ->setFrom($event->getPusherPHID()) + ->setBody($body->render()) + ->setThreadID($event->getPHID(), $is_new = true) + ->addHeader('Thread-Topic', $subject) + ->setIsBulk(true); + + $to_handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs($email_phids) + ->execute(); + + $reply_handler = new PhabricatorRepositoryPushReplyHandler(); + $mails = $reply_handler->multiplexMail( + $mail, + $to_handles, + array()); + + foreach ($mails as $mail) { + $mail->saveAndSend(); + } + } + + public function renderForDisplay(PhabricatorUser $viewer) { + // This data has some sensitive stuff, so don't show it. + return null; + } + + private function renderRefs(array $logs) { + $ref_lines = array(); + $ref_list = array(); + + foreach ($logs as $log) { + $type_name = null; + $type_prefix = null; + switch ($log->getRefType()) { + case PhabricatorRepositoryPushLog::REFTYPE_BRANCH: + $type_name = pht('branch'); + break; + case PhabricatorRepositoryPushLog::REFTYPE_TAG: + $type_name = pht('tag'); + $type_prefix = pht('tag:'); + break; + case PhabricatorRepositoryPushLog::REFTYPE_BOOKMARK: + $type_name = pht('bookmark'); + $type_prefix = pht('bookmark:'); + break; + case PhabricatorRepositoryPushLog::REFTYPE_COMMIT: + default: + break; + } + + if ($type_name === null) { + continue; + } + + $flags = $log->getChangeFlags(); + if ($flags & PhabricatorRepositoryPushLog::CHANGEFLAG_DANGEROUS) { + $action = '!'; + } else if ($flags & PhabricatorRepositoryPushLog::CHANGEFLAG_DELETE) { + $action = '-'; + } else if ($flags & PhabricatorRepositoryPushLog::CHANGEFLAG_REWRITE) { + $action = '~'; + } else if ($flags & PhabricatorRepositoryPushLog::CHANGEFLAG_APPEND) { + $action = ' '; + } else if ($flags & PhabricatorRepositoryPushLog::CHANGEFLAG_ADD) { + $action = '+'; + } else { + $action = '?'; + } + + $old = nonempty($log->getRefOldShort(), pht('')); + $new = nonempty($log->getRefNewShort(), pht('')); + + $name = $log->getRefName(); + + $ref_lines[] = "{$action} {$type_name} {$name} {$old} > {$new}"; + $ref_list[] = $type_prefix.$name; + } + + return array( + $ref_lines, + array_unique($ref_list), + ); + } + + private function renderCommits( + PhabricatorRepository $repository, + array $logs, + array $info) { + + $commit_lines = array(); + $subject_line = null; + foreach ($logs as $log) { + if ($log->getRefType() != PhabricatorRepositoryPushLog::REFTYPE_COMMIT) { + continue; + } + + $commit_info = idx($info, $log->getRefNew(), array()); + + $name = $repository->formatCommitName($log->getRefNew()); + + $branches = null; + if (idx($commit_info, 'branches')) { + $branches = ' ('.implode(', ', $commit_info['branches']).')'; + } + + $summary = null; + if (strlen(idx($commit_info, 'summary'))) { + $summary = ' '.$commit_info['summary']; + } + + $commit_lines[] = "{$name}{$branches}{$summary}"; + if ($subject_line === null) { + $subject_line = "{$name}{$summary}"; + } + } + + return array($commit_lines, $subject_line); + } + +} diff --git a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php index efa978d353..953b13199c 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php @@ -861,6 +861,20 @@ abstract class PhabricatorBaseEnglishTranslation '%s, %d lines', ), + '%s pushed %d commit(s) to %s.' => array( + array( + array( + '%s pushed a commit to %3$s.', + '%s pushed %d commits to %s.', + ), + ), + ), + + '%s commit(s)' => array( + '1 commit', + '%s commits', + ), + ); }