From fca553f142a23448343747180fc1b59dba2954fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 18 Sep 2017 14:40:36 -0700 Subject: [PATCH] Prepare revision mail for the "Draft" status Summary: Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail. With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft". Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it. Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18627 --- ...ifferentialChangesSinceLastUpdateField.php | 2 +- .../customfield/DifferentialSummaryField.php | 2 +- .../customfield/DifferentialTestPlanField.php | 2 +- .../editor/DifferentialTransactionEditor.php | 69 ++++++++++++++----- .../storage/DifferentialRevision.php | 8 +++ 5 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/applications/differential/customfield/DifferentialChangesSinceLastUpdateField.php b/src/applications/differential/customfield/DifferentialChangesSinceLastUpdateField.php index a7e3654d0d..6fd24fc0d7 100644 --- a/src/applications/differential/customfield/DifferentialChangesSinceLastUpdateField.php +++ b/src/applications/differential/customfield/DifferentialChangesSinceLastUpdateField.php @@ -24,7 +24,7 @@ final class DifferentialChangesSinceLastUpdateField PhabricatorApplicationTransactionEditor $editor, array $xactions) { - if ($editor->getIsNewObject()) { + if ($editor->isFirstBroadcast()) { return; } diff --git a/src/applications/differential/customfield/DifferentialSummaryField.php b/src/applications/differential/customfield/DifferentialSummaryField.php index a11bebca6e..eff2de6f12 100644 --- a/src/applications/differential/customfield/DifferentialSummaryField.php +++ b/src/applications/differential/customfield/DifferentialSummaryField.php @@ -67,7 +67,7 @@ final class DifferentialSummaryField PhabricatorApplicationTransactionEditor $editor, array $xactions) { - if (!$editor->getIsNewObject()) { + if (!$editor->isFirstBroadcast()) { return; } diff --git a/src/applications/differential/customfield/DifferentialTestPlanField.php b/src/applications/differential/customfield/DifferentialTestPlanField.php index c774e81a08..f2fa9cd17f 100644 --- a/src/applications/differential/customfield/DifferentialTestPlanField.php +++ b/src/applications/differential/customfield/DifferentialTestPlanField.php @@ -71,7 +71,7 @@ final class DifferentialTestPlanField PhabricatorApplicationTransactionEditor $editor, array $xactions) { - if (!$editor->getIsNewObject()) { + if (!$editor->isFirstBroadcast()) { return; } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index c6270013aa..989b9c5547 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -26,6 +26,10 @@ final class DifferentialTransactionEditor return pht('%s created %s.', $author, $object); } + public function isFirstBroadcast() { + return $this->getIsNewObject(); + } + public function getDiffUpdateTransaction(array $xactions) { $type_update = DifferentialTransaction::TYPE_UPDATE; @@ -600,24 +604,25 @@ final class DifferentialTransactionEditor return array_values(array_merge($head, $tail)); } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) {} - - return parent::requireCapabilities($object, $xaction); - } - protected function shouldPublishFeedStory( PhabricatorLiskDAO $object, array $xactions) { + + if (!$object->shouldBroadcast()) { + return false; + } + return true; } protected function shouldSendMail( PhabricatorLiskDAO $object, array $xactions) { + + if (!$object->shouldBroadcast()) { + return false; + } + return true; } @@ -633,14 +638,25 @@ final class DifferentialTransactionEditor protected function getMailAction( PhabricatorLiskDAO $object, array $xactions) { - $action = parent::getMailAction($object, $xactions); - $strongest = $this->getStrongestAction($object, $xactions); - switch ($strongest->getTransactionType()) { - case DifferentialTransaction::TYPE_UPDATE: - $count = new PhutilNumber($object->getLineCount()); - $action = pht('%s, %s line(s)', $action, $count); - break; + $show_lines = false; + if ($this->isFirstBroadcast()) { + $action = pht('Request'); + + $show_lines = true; + } else { + $action = parent::getMailAction($object, $xactions); + + $strongest = $this->getStrongestAction($object, $xactions); + $type_update = DifferentialTransaction::TYPE_UPDATE; + if ($strongest->getTransactionType() == $type_update) { + $show_lines = true; + } + } + + if ($show_lines) { + $count = new PhutilNumber($object->getLineCount()); + $action = pht('%s, %s line(s)', $action, $count); } return $action; @@ -679,6 +695,16 @@ final class DifferentialTransactionEditor PhabricatorLiskDAO $object, array $xactions) { + $viewer = $this->requireActor(); + + // If this is the first time we're sending mail about this revision, we + // generate mail for all prior transactions, not just whatever is being + // applied now. This gets the "added reviewers" lines and other relevant + // information into the mail. + if ($this->isFirstBroadcast()) { + $xactions = $this->loadUnbroadcastTransactions($object); + } + $body = new PhabricatorMetaMTAMailBody(); $body->setViewer($this->requireActor()); @@ -1491,4 +1517,15 @@ final class DifferentialTransactionEditor $acting_phid); } + private function loadUnbroadcastTransactions($object) { + $viewer = $this->requireActor(); + + $xactions = id(new DifferentialTransactionQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($object->getPHID())) + ->execute(); + + return array_reverse($xactions); + } + } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 03602b49d9..7bcd178a41 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -694,6 +694,14 @@ final class DifferentialRevision extends DifferentialDAO return $this; } + public function shouldBroadcast() { + if (!$this->isDraft()) { + return true; + } + + return false; + } + /* -( HarbormasterBuildableInterface )------------------------------------- */