From 7749c5abf3ffb22561c0d759e15ebb4898dc11cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Oct 2012 12:03:11 -0700 Subject: [PATCH] Mark notifications as read in Differential if we also sent an email Summary: See D3789. Same thing for Differential. Test Plan: Created a new revision and made a comment. Verified reviewer got popup notifications but the in-app notifications were delivered already marked as read. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Maniphest Tasks: T1403 Differential Revision: https://secure.phabricator.com/D3790 --- .../editor/DifferentialCommentEditor.php | 13 +- .../editor/DifferentialRevisionEditor.php | 111 +++++++++--------- .../differential/mail/DifferentialMail.php | 15 +++ 3 files changed, 81 insertions(+), 58 deletions(-) diff --git a/src/applications/differential/editor/DifferentialCommentEditor.php b/src/applications/differential/editor/DifferentialCommentEditor.php index 8809d25f70..207edf3cfa 100644 --- a/src/applications/differential/editor/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/DifferentialCommentEditor.php @@ -558,8 +558,9 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $xherald_header = HeraldTranscript::loadXHeraldRulesHeader( $revision->getPHID()); + $mailed_phids = array(); if (!$this->noEmail) { - id(new DifferentialCommentMail( + $mail = id(new DifferentialCommentMail( $revision, $actor_handle, $comment, @@ -575,6 +576,8 @@ final class DifferentialCommentEditor extends PhabricatorEditor { ->setXHeraldRulesHeader($xherald_header) ->setParentMessageID($this->parentMessageID) ->send(); + + $mailed_phids = $mail->getRawMail()->buildRecipientList(); } $event_data = array( @@ -586,12 +589,13 @@ final class DifferentialCommentEditor extends PhabricatorEditor { 'feedback_content' => $comment->getContent(), 'actor_phid' => $actor_phid, ); + + // TODO: Get rid of this id(new PhabricatorTimelineEvent('difx', $event_data)) ->recordEvent(); - // TODO: Move to a daemon? id(new PhabricatorFeedStoryPublisher()) - ->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_DIFFERENTIAL) + ->setStoryType('PhabricatorFeedStoryDifferential') ->setStoryData($event_data) ->setStoryTime(time()) ->setStoryAuthorPHID($actor_phid) @@ -607,9 +611,10 @@ final class DifferentialCommentEditor extends PhabricatorEditor { array($revision->getAuthorPHID()), $revision->getReviewers(), $revision->getCCPHIDs())) + ->setMailRecipientPHIDs($mailed_phids) ->publish(); - // TODO: Move to a daemon? + // TODO: Move to workers PhabricatorSearchDifferentialIndexer::indexRevision($revision); return $comment; diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index 3eeafb2ca9..fc4a4b04e0 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -420,8 +420,62 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { id(new PhabricatorTimelineEvent('difx', $event_data)) ->recordEvent(); + $mailed_phids = array(); + if (!$this->silentUpdate) { + $revision->loadRelationships(); + + if ($add['rev']) { + $message = id(new DifferentialNewDiffMail( + $revision, + $actor_handle, + $changesets)) + ->setIsFirstMailAboutRevision($is_new) + ->setIsFirstMailToRecipients(true) + ->setToPHIDs(array_keys($add['rev'])); + + if ($is_new) { + // The first time we send an email about a revision, put the CCs in + // the "CC:" field of the same "Review Requested" email that reviewers + // get, so you don't get two initial emails if you're on a list that + // is CC'd. + $message->setCCPHIDs(array_keys($add['ccs'])); + } + + $mail[] = $message; + } + + // If we added CCs, we want to send them an email, but only if they were + // not already a reviewer and were not added as one (in these cases, they + // got a "NewDiff" mail, either in the past or just a moment ago). You can + // still get two emails, but only if a revision is updated and you are + // added as a reviewer at the same time a list you are on is added as a + // CC, which is rare and reasonable. + + $implied_ccs = self::getImpliedCCs($revision); + $implied_ccs = array_fill_keys($implied_ccs, true); + $add['ccs'] = array_diff_key($add['ccs'], $implied_ccs); + + if (!$is_new && $add['ccs']) { + $mail[] = id(new DifferentialCCWelcomeMail( + $revision, + $actor_handle, + $changesets)) + ->setIsFirstMailToRecipients(true) + ->setToPHIDs(array_keys($add['ccs'])); + } + + foreach ($mail as $message) { + $message->setHeraldTranscriptURI($xscript_uri); + $message->setXHeraldRulesHeader($xscript_header); + $message->send(); + + $mailed_phids[] = $message->getRawMail()->buildRecipientList(); + } + $mailed_phids = array_mergev($mailed_phids); + } + id(new PhabricatorFeedStoryPublisher()) - ->setStoryType(PhabricatorFeedStoryTypeConstants::STORY_DIFFERENTIAL) + ->setStoryType('PhabricatorFeedStoryDifferential') ->setStoryData($event_data) ->setStoryTime(time()) ->setStoryAuthorPHID($revision->getAuthorPHID()) @@ -436,62 +490,11 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { array($revision->getAuthorPHID()), $revision->getReviewers(), $revision->getCCPHIDs())) + ->setMailRecipientPHIDs($mailed_phids) ->publish(); -// TODO: Move this into a worker task thing. + // TODO: Move this into a worker task thing. PhabricatorSearchDifferentialIndexer::indexRevision($revision); - - if ($this->silentUpdate) { - return; - } - - $revision->loadRelationships(); - - if ($add['rev']) { - $message = id(new DifferentialNewDiffMail( - $revision, - $actor_handle, - $changesets)) - ->setIsFirstMailAboutRevision($is_new) - ->setIsFirstMailToRecipients(true) - ->setToPHIDs(array_keys($add['rev'])); - - if ($is_new) { - // The first time we send an email about a revision, put the CCs in - // the "CC:" field of the same "Review Requested" email that reviewers - // get, so you don't get two initial emails if you're on a list that - // is CC'd. - $message->setCCPHIDs(array_keys($add['ccs'])); - } - - $mail[] = $message; - } - - // If we added CCs, we want to send them an email, but only if they were not - // already a reviewer and were not added as one (in these cases, they got - // a "NewDiff" mail, either in the past or just a moment ago). You can still - // get two emails, but only if a revision is updated and you are added as a - // reviewer at the same time a list you are on is added as a CC, which is - // rare and reasonable. - - $implied_ccs = self::getImpliedCCs($revision); - $implied_ccs = array_fill_keys($implied_ccs, true); - $add['ccs'] = array_diff_key($add['ccs'], $implied_ccs); - - if (!$is_new && $add['ccs']) { - $mail[] = id(new DifferentialCCWelcomeMail( - $revision, - $actor_handle, - $changesets)) - ->setIsFirstMailToRecipients(true) - ->setToPHIDs(array_keys($add['ccs'])); - } - - foreach ($mail as $message) { - $message->setHeraldTranscriptURI($xscript_uri); - $message->setXHeraldRulesHeader($xscript_header); - $message->send(); - } } public static function addCCAndUpdateRevision( diff --git a/src/applications/differential/mail/DifferentialMail.php b/src/applications/differential/mail/DifferentialMail.php index e94867af05..a5cc25c1e2 100644 --- a/src/applications/differential/mail/DifferentialMail.php +++ b/src/applications/differential/mail/DifferentialMail.php @@ -35,6 +35,15 @@ abstract class DifferentialMail { protected $replyHandler; protected $parentMessageID; + private $rawMail; + + public function getRawMail() { + if (!$this->rawMail) { + throw new Exception("Call send() before getRawMail()!"); + } + return $this->rawMail; + } + protected function renderSubject() { $revision = $this->getRevision(); $title = $revision->getTitle(); @@ -187,6 +196,10 @@ abstract class DifferentialMail { $this->prepareBody(); + $this->rawMail = clone $template; + $this->rawMail->addTos($to_phids); + $this->rawMail->addCCs($cc_phids); + $mails = $reply_handler->multiplexMail($template, $to_handles, $cc_handles); $original_translator = PhutilTranslator::getInstance(); @@ -236,6 +249,8 @@ abstract class DifferentialMail { } PhutilTranslator::setInstance($original_translator); + + return $this; } protected function getMailTags() {