diff --git a/src/applications/differential/customfield/DifferentialDraftField.php b/src/applications/differential/customfield/DifferentialDraftField.php index 8147708bca..70f7291148 100644 --- a/src/applications/differential/customfield/DifferentialDraftField.php +++ b/src/applications/differential/customfield/DifferentialDraftField.php @@ -58,7 +58,7 @@ final class DifferentialDraftField ); $blocking_map = array_fuse($blocking_map); - $builds = $revision->loadActiveBuilds($viewer); + $builds = $revision->loadImpactfulBuilds($viewer); $waiting = array(); $blocking = array(); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 9882246608..ccd83892db 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1539,8 +1539,12 @@ final class DifferentialTransactionEditor } if ($object->isDraft() && $auto_undraft) { - $active_builds = $this->hasActiveBuilds($object); - if (!$active_builds) { + $status = $this->loadCompletedBuildableStatus($object); + + $is_passed = ($status === HarbormasterBuildableStatus::STATUS_PASSED); + $is_failed = ($status === HarbormasterBuildableStatus::STATUS_FAILED); + + if ($is_passed) { // When Harbormaster moves a revision out of the draft state, we // attribute the action to the revision author since this is more // natural and more useful. @@ -1572,6 +1576,9 @@ final class DifferentialTransactionEditor // batch of transactions finishes so that Herald can fire on the new // revision state. See T13027 for discussion. $this->queueTransaction($xaction); + } else if ($is_failed) { + // TODO: Change to "Changes Planned + Draft", notify the author (only) + // of the build failure. } } @@ -1604,15 +1611,11 @@ final class DifferentialTransactionEditor return $xactions; } - private function hasActiveBuilds($object) { + private function loadCompletedBuildableStatus( + DifferentialRevision $revision) { $viewer = $this->requireActor(); - - $builds = $object->loadActiveBuilds($viewer); - if (!$builds) { - return false; - } - - return true; + $builds = $revision->loadImpactfulBuilds($viewer); + return $revision->newBuildableStatusForBuilds($builds); } private function requireReviewers(DifferentialRevision $revision) { diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 446d180018..05b82ed089 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -779,23 +779,14 @@ final class DifferentialRevision extends DifferentialDAO // when computing build status. Differential only cares about remote // builds when making publishing and undrafting decisions. - $builds = id(new HarbormasterBuildQuery()) - ->setViewer($viewer) - ->withBuildablePHIDs(array($phid)) - ->withAutobuilds(false) - ->withBuildStatuses( - array( - HarbormasterBuildStatus::STATUS_INACTIVE, - HarbormasterBuildStatus::STATUS_PENDING, - HarbormasterBuildStatus::STATUS_BUILDING, - HarbormasterBuildStatus::STATUS_FAILED, - HarbormasterBuildStatus::STATUS_ABORTED, - HarbormasterBuildStatus::STATUS_ERROR, - HarbormasterBuildStatus::STATUS_PAUSED, - HarbormasterBuildStatus::STATUS_DEADLOCKED, - )) - ->execute(); + $builds = $this->loadImpactfulBuildsForBuildablePHIDs( + $viewer, + array($phid)); + return $this->newBuildableStatusForBuilds($builds); + } + + public function newBuildableStatusForBuilds(array $builds) { // If we have nothing but passing builds, the buildable passes. if (!$builds) { return HarbormasterBuildableStatus::STATUS_PASSED; @@ -803,8 +794,7 @@ final class DifferentialRevision extends DifferentialDAO // If we have any completed, non-passing builds, the buildable fails. foreach ($builds as $build) { - $status = $build->getBuildStatusObject(); - if ($status->isComplete()) { + if ($build->isComplete()) { return HarbormasterBuildableStatus::STATUS_FAILED; } } @@ -813,7 +803,7 @@ final class DifferentialRevision extends DifferentialDAO return null; } - public function loadActiveBuilds(PhabricatorUser $viewer) { + public function loadImpactfulBuilds(PhabricatorUser $viewer) { $diff = $this->getActiveDiff(); // NOTE: We can't use `withContainerPHIDs()` here because the container @@ -827,9 +817,18 @@ final class DifferentialRevision extends DifferentialDAO return array(); } + return $this->loadImpactfulBuildsForBuildablePHIDs( + $viewer, + mpull($buildables, 'getPHID')); + } + + private function loadImpactfulBuildsForBuildablePHIDs( + PhabricatorUser $viewer, + array $phids) { + return id(new HarbormasterBuildQuery()) ->setViewer($viewer) - ->withBuildablePHIDs(mpull($buildables, 'getPHID')) + ->withBuildablePHIDs($phids) ->withAutobuilds(false) ->withBuildStatuses( array(