@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
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
+37
-9
+27
-9
src/applications/differential/editor/DifferentialTransactionEditor.php
+27
-9
src/applications/differential/editor/DifferentialTransactionEditor.php
···
10
10
private $hasReviewTransaction = false;
11
11
private $affectedPaths;
12
12
private $firstBroadcast = false;
13
+
private $wasDraft = false;
13
14
14
15
public function getEditorApplicationClass() {
15
16
return 'PhabricatorDifferentialApplication';
···
165
166
break;
166
167
}
167
168
}
169
+
170
+
$this->wasDraft = $object->isDraft();
168
171
169
172
return parent::expandTransactions($object, $xactions);
170
173
}
···
1581
1584
$this->setActingAsPHID($author_phid);
1582
1585
}
1583
1586
1584
-
// Mark this as the first broadcast we're sending about the revision
1585
-
// so mail can generate specially.
1586
-
$this->firstBroadcast = true;
1587
-
1588
1587
$xaction = $object->getApplicationTransactionTemplate()
1589
1588
->setAuthorPHID($author_phid)
1590
1589
->setTransactionType(
···
1612
1611
1613
1612
$xactions[] = $xaction;
1614
1613
}
1615
-
} else {
1616
-
// If this revision is being created into some state other than "Draft",
1617
-
// this is the first broadcast and should include sections like "SUMMARY"
1618
-
// and "TEST PLAN".
1619
-
if ($this->getIsNewObject()) {
1614
+
}
1615
+
1616
+
// If the revision is new or was a draft, and is no longer a draft, we
1617
+
// might be sending the first email about it.
1618
+
1619
+
// This might mean it was created directly into a non-draft state, or
1620
+
// it just automatically undrafted after builds finished, or a user
1621
+
// explicitly promoted it out of the draft state with an action like
1622
+
// "Request Review".
1623
+
1624
+
// If we haven't sent any email about it yet, mark this email as the first
1625
+
// email so the mail gets enriched with "SUMMARY" and "TEST PLAN".
1626
+
1627
+
$is_new = $this->getIsNewObject();
1628
+
$was_draft = $this->wasDraft;
1629
+
1630
+
if (!$object->isDraft() && ($was_draft || $is_new)) {
1631
+
if (!$object->getHasBroadcast()) {
1632
+
// Mark this as the first broadcast we're sending about the revision
1633
+
// so mail can generate specially.
1620
1634
$this->firstBroadcast = true;
1635
+
1636
+
$object
1637
+
->setHasBroadcast(true)
1638
+
->save();
1621
1639
}
1622
1640
}
1623
1641
+10
src/applications/differential/storage/DifferentialRevision.php
+10
src/applications/differential/storage/DifferentialRevision.php
···
60
60
61
61
const PROPERTY_CLOSED_FROM_ACCEPTED = 'wasAcceptedBeforeClose';
62
62
const PROPERTY_DRAFT_HOLD = 'draft.hold';
63
+
const PROPERTY_HAS_BROADCAST = 'draft.broadcast';
63
64
64
65
public static function initializeNewRevision(PhabricatorUser $actor) {
65
66
$app = id(new PhabricatorApplicationQuery())
···
718
719
public function setHoldAsDraft($hold) {
719
720
return $this->setProperty(self::PROPERTY_DRAFT_HOLD, $hold);
720
721
}
722
+
723
+
public function getHasBroadcast() {
724
+
return $this->getProperty(self::PROPERTY_HAS_BROADCAST, false);
725
+
}
726
+
727
+
public function setHasBroadcast($has_broadcast) {
728
+
return $this->setProperty(self::PROPERTY_HAS_BROADCAST, $has_broadcast);
729
+
}
730
+
721
731
722
732
public function loadActiveBuilds(PhabricatorUser $viewer) {
723
733
$diff = $this->getActiveDiff();