@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
Explicitly condition Differential draft promotion on only "impactful" builds
Summary:
Depends on D19281. This increases consistency between build timeline publishing and revision draft promotion.
There's no real behavioral change here (switching how publishing worked already changed the beahvior) but this sends more callsites down the same code paths.
Since the builds we're looking at include completed builds, change the term "active" to "impactful". This describes the same set of builds, but hopefully describes them more accurately.
Test Plan: Created a local revision, saw it plausibly interact with draft status and promote. There are a lot of moving parts here and some stuff may well have slipped through.
Differential Revision: https://secure.phabricator.com/D19282
+33
-31
+1
-1
src/applications/differential/customfield/DifferentialDraftField.php
+1
-1
src/applications/differential/customfield/DifferentialDraftField.php
+13
-10
src/applications/differential/editor/DifferentialTransactionEditor.php
+13
-10
src/applications/differential/editor/DifferentialTransactionEditor.php
···
1539
1539
}
1540
1540
1541
1541
if ($object->isDraft() && $auto_undraft) {
1542
-
$active_builds = $this->hasActiveBuilds($object);
1543
-
if (!$active_builds) {
1542
+
$status = $this->loadCompletedBuildableStatus($object);
1543
+
1544
+
$is_passed = ($status === HarbormasterBuildableStatus::STATUS_PASSED);
1545
+
$is_failed = ($status === HarbormasterBuildableStatus::STATUS_FAILED);
1546
+
1547
+
if ($is_passed) {
1544
1548
// When Harbormaster moves a revision out of the draft state, we
1545
1549
// attribute the action to the revision author since this is more
1546
1550
// natural and more useful.
···
1572
1576
// batch of transactions finishes so that Herald can fire on the new
1573
1577
// revision state. See T13027 for discussion.
1574
1578
$this->queueTransaction($xaction);
1579
+
} else if ($is_failed) {
1580
+
// TODO: Change to "Changes Planned + Draft", notify the author (only)
1581
+
// of the build failure.
1575
1582
}
1576
1583
}
1577
1584
···
1604
1611
return $xactions;
1605
1612
}
1606
1613
1607
-
private function hasActiveBuilds($object) {
1614
+
private function loadCompletedBuildableStatus(
1615
+
DifferentialRevision $revision) {
1608
1616
$viewer = $this->requireActor();
1609
-
1610
-
$builds = $object->loadActiveBuilds($viewer);
1611
-
if (!$builds) {
1612
-
return false;
1613
-
}
1614
-
1615
-
return true;
1617
+
$builds = $revision->loadImpactfulBuilds($viewer);
1618
+
return $revision->newBuildableStatusForBuilds($builds);
1616
1619
}
1617
1620
1618
1621
private function requireReviewers(DifferentialRevision $revision) {
+19
-20
src/applications/differential/storage/DifferentialRevision.php
+19
-20
src/applications/differential/storage/DifferentialRevision.php
···
779
779
// when computing build status. Differential only cares about remote
780
780
// builds when making publishing and undrafting decisions.
781
781
782
-
$builds = id(new HarbormasterBuildQuery())
783
-
->setViewer($viewer)
784
-
->withBuildablePHIDs(array($phid))
785
-
->withAutobuilds(false)
786
-
->withBuildStatuses(
787
-
array(
788
-
HarbormasterBuildStatus::STATUS_INACTIVE,
789
-
HarbormasterBuildStatus::STATUS_PENDING,
790
-
HarbormasterBuildStatus::STATUS_BUILDING,
791
-
HarbormasterBuildStatus::STATUS_FAILED,
792
-
HarbormasterBuildStatus::STATUS_ABORTED,
793
-
HarbormasterBuildStatus::STATUS_ERROR,
794
-
HarbormasterBuildStatus::STATUS_PAUSED,
795
-
HarbormasterBuildStatus::STATUS_DEADLOCKED,
796
-
))
797
-
->execute();
782
+
$builds = $this->loadImpactfulBuildsForBuildablePHIDs(
783
+
$viewer,
784
+
array($phid));
785
+
786
+
return $this->newBuildableStatusForBuilds($builds);
787
+
}
798
788
789
+
public function newBuildableStatusForBuilds(array $builds) {
799
790
// If we have nothing but passing builds, the buildable passes.
800
791
if (!$builds) {
801
792
return HarbormasterBuildableStatus::STATUS_PASSED;
···
803
794
804
795
// If we have any completed, non-passing builds, the buildable fails.
805
796
foreach ($builds as $build) {
806
-
$status = $build->getBuildStatusObject();
807
-
if ($status->isComplete()) {
797
+
if ($build->isComplete()) {
808
798
return HarbormasterBuildableStatus::STATUS_FAILED;
809
799
}
810
800
}
···
813
803
return null;
814
804
}
815
805
816
-
public function loadActiveBuilds(PhabricatorUser $viewer) {
806
+
public function loadImpactfulBuilds(PhabricatorUser $viewer) {
817
807
$diff = $this->getActiveDiff();
818
808
819
809
// NOTE: We can't use `withContainerPHIDs()` here because the container
···
827
817
return array();
828
818
}
829
819
820
+
return $this->loadImpactfulBuildsForBuildablePHIDs(
821
+
$viewer,
822
+
mpull($buildables, 'getPHID'));
823
+
}
824
+
825
+
private function loadImpactfulBuildsForBuildablePHIDs(
826
+
PhabricatorUser $viewer,
827
+
array $phids) {
828
+
830
829
return id(new HarbormasterBuildQuery())
831
830
->setViewer($viewer)
832
-
->withBuildablePHIDs(mpull($buildables, 'getPHID'))
831
+
->withBuildablePHIDs($phids)
833
832
->withAutobuilds(false)
834
833
->withBuildStatuses(
835
834
array(