From 54f131dc4baad39f54c164ea57135151c4d38542 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 28 Nov 2017 10:42:16 -0800
Subject: [PATCH] Make "first broadcast" rules for Differential drafts more
 general

Summary:
See PHI228. Ref T2543. The current logic gets this slightly wrong: prototypes are off, you create a draft with `--draft`, then promote it with "Request Review". This misses both branches.

Instead, test these conditions a little more broadly. We also need to store broadcast state since `getIsNewObject()` isn't good enough with this workflow.

Test Plan:
  - With prototypes on and autopromotion, got a rich email after builds finished.
  - With prototypes off, got a rich email immediately.
  - With prototypes off and `--draft`, got a rich email after "Request Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18801
---
 .../editor/DifferentialTransactionEditor.php  | 36 ++++++++++++++-----
 .../storage/DifferentialRevision.php          | 10 ++++++
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 7cea29359f..ae652ba0ec 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -10,6 +10,7 @@ final class DifferentialTransactionEditor
   private $hasReviewTransaction = false;
   private $affectedPaths;
   private $firstBroadcast = false;
+  private $wasDraft = false;
 
   public function getEditorApplicationClass() {
     return 'PhabricatorDifferentialApplication';
@@ -166,6 +167,8 @@ final class DifferentialTransactionEditor
       }
     }
 
+    $this->wasDraft = $object->isDraft();
+
     return parent::expandTransactions($object, $xactions);
   }
 
@@ -1581,10 +1584,6 @@ final class DifferentialTransactionEditor
           $this->setActingAsPHID($author_phid);
         }
 
-        // Mark this as the first broadcast we're sending about the revision
-        // so mail can generate specially.
-        $this->firstBroadcast = true;
-
         $xaction = $object->getApplicationTransactionTemplate()
           ->setAuthorPHID($author_phid)
           ->setTransactionType(
@@ -1612,12 +1611,31 @@ final class DifferentialTransactionEditor
 
         $xactions[] = $xaction;
       }
-    } else {
-      // If this revision is being created into some state other than "Draft",
-      // this is the first broadcast and should include sections like "SUMMARY"
-      // and "TEST PLAN".
-      if ($this->getIsNewObject()) {
+    }
+
+    // If the revision is new or was a draft, and is no longer a draft, we
+    // might be sending the first email about it.
+
+    // This might mean it was created directly into a non-draft state, or
+    // it just automatically undrafted after builds finished, or a user
+    // explicitly promoted it out of the draft state with an action like
+    // "Request Review".
+
+    // If we haven't sent any email about it yet, mark this email as the first
+    // email so the mail gets enriched with "SUMMARY" and "TEST PLAN".
+
+    $is_new = $this->getIsNewObject();
+    $was_draft = $this->wasDraft;
+
+    if (!$object->isDraft() && ($was_draft || $is_new)) {
+      if (!$object->getHasBroadcast()) {
+        // Mark this as the first broadcast we're sending about the revision
+        // so mail can generate specially.
         $this->firstBroadcast = true;
+
+        $object
+          ->setHasBroadcast(true)
+          ->save();
       }
     }
 
diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
index bf4bec0abc..73f22c91e4 100644
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -60,6 +60,7 @@ final class DifferentialRevision extends DifferentialDAO
 
   const PROPERTY_CLOSED_FROM_ACCEPTED = 'wasAcceptedBeforeClose';
   const PROPERTY_DRAFT_HOLD = 'draft.hold';
+  const PROPERTY_HAS_BROADCAST = 'draft.broadcast';
 
   public static function initializeNewRevision(PhabricatorUser $actor) {
     $app = id(new PhabricatorApplicationQuery())
@@ -719,6 +720,15 @@ final class DifferentialRevision extends DifferentialDAO
     return $this->setProperty(self::PROPERTY_DRAFT_HOLD, $hold);
   }
 
+  public function getHasBroadcast() {
+    return $this->getProperty(self::PROPERTY_HAS_BROADCAST, false);
+  }
+
+  public function setHasBroadcast($has_broadcast) {
+    return $this->setProperty(self::PROPERTY_HAS_BROADCAST, $has_broadcast);
+  }
+
+
   public function loadActiveBuilds(PhabricatorUser $viewer) {
     $diff = $this->getActiveDiff();