1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-27 01:02:42 +01:00

Execute Herald again after promoting revisions out of the "Draft" state

Summary:
Fixes T13027. Ref T2543. When revisions promote from "Draft" because builds finish or no builds are configured, the status currently switches from "Draft" to "Needs Review" without re-running Herald.

This means that some rules -- notably, "Send me an email" rules -- don't fire as soon as they should.

Instead of applying this promotion in a hacky way inline, queue it and apply it normally in a second edit, after the current group finishes.

Test Plan:
  - Created a revision, reviewed Herald transcripts.
  - Saw three Herald passes:
    - First pass (revision creation) triggered builds and no email.
    - Second pass (builds finished) did not trigger builds (no update) and did not trigger email (revision still a draft).
    - Third pass (after promotion out of 'draft') did not trigger builds (no update) but did trigger email (revision no longer a draft).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13027, T2543

Differential Revision: https://secure.phabricator.com/D18819
This commit is contained in:
epriestley 2017-12-05 06:47:48 -08:00
parent 4f8340c05f
commit 6d3baa92f9
3 changed files with 52 additions and 17 deletions

View file

@ -1588,11 +1588,8 @@ final class DifferentialTransactionEditor
->setAuthorPHID($author_phid) ->setAuthorPHID($author_phid)
->setTransactionType( ->setTransactionType(
DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE) DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE)
->setOldValue(false)
->setNewValue(true); ->setNewValue(true);
$xaction = $this->populateTransaction($object, $xaction);
// If we're creating this revision and immediately moving it out of // If we're creating this revision and immediately moving it out of
// the draft state, mark this as a create transaction so it gets // the draft state, mark this as a create transaction so it gets
// hidden in the timeline and mail, since it isn't interesting: it // hidden in the timeline and mail, since it isn't interesting: it
@ -1601,15 +1598,10 @@ final class DifferentialTransactionEditor
$xaction->setIsCreateTransaction(true); $xaction->setIsCreateTransaction(true);
} }
$object->openTransaction(); // Queue this transaction and apply it separately after the current
$object // batch of transactions finishes so that Herald can fire on the new
->setStatus(DifferentialRevisionStatus::NEEDS_REVIEW) // revision state. See T13027 for discussion.
->save(); $this->queueTransaction($xaction);
$xaction->save();
$object->saveTransaction();
$xactions[] = $xaction;
} }
} }

View file

@ -57,6 +57,9 @@ final class DifferentialRevisionRequestReviewTransaction
'revisions.')); 'revisions.'));
} }
// When revisions automatically promote out of "Draft" after builds finish,
// the viewer may be acting as the Harbormaster application.
if (!$viewer->isOmnipotent()) {
if (!$this->isViewerRevisionAuthor($object, $viewer)) { if (!$this->isViewerRevisionAuthor($object, $viewer)) {
throw new Exception( throw new Exception(
pht( pht(
@ -64,6 +67,7 @@ final class DifferentialRevisionRequestReviewTransaction
'the author of the revision.')); 'the author of the revision.'));
} }
} }
}
public function getTitle() { public function getTitle() {
return pht( return pht(

View file

@ -70,6 +70,8 @@ abstract class PhabricatorApplicationTransactionEditor
private $feedRelatedPHIDs = array(); private $feedRelatedPHIDs = array();
private $modularTypes; private $modularTypes;
private $transactionQueue = array();
const STORAGE_ENCODING_BINARY = 'binary'; const STORAGE_ENCODING_BINARY = 'binary';
/** /**
@ -1174,6 +1176,8 @@ abstract class PhabricatorApplicationTransactionEditor
'priority' => PhabricatorWorker::PRIORITY_ALERTS, 'priority' => PhabricatorWorker::PRIORITY_ALERTS,
)); ));
$this->flushTransactionQueue($object);
return $xactions; return $xactions;
} }
@ -3864,4 +3868,39 @@ abstract class PhabricatorApplicationTransactionEditor
return pht('%s created an object: %s.', $author, $object); return pht('%s created an object: %s.', $author, $object);
} }
/* -( Queue )-------------------------------------------------------------- */
protected function queueTransaction(
PhabricatorApplicationTransaction $xaction) {
$this->transactionQueue[] = $xaction;
return $this;
}
private function flushTransactionQueue($object) {
if (!$this->transactionQueue) {
return;
}
$xactions = $this->transactionQueue;
$this->transactionQueue = array();
$editor = $this->newQueueEditor();
return $editor->applyTransactions($object, $xactions);
}
private function newQueueEditor() {
$editor = id(newv(get_class($this), array()))
->setActor($this->getActor())
->setContentSource($this->getContentSource())
->setContinueOnNoEffect($this->getContinueOnNoEffect())
->setContinueOnMissingFields($this->getContinueOnMissingFields());
if ($this->actingAsPHID !== null) {
$editor->setActingAsPHID($this->actingAsPHID);
}
return $editor;
}
} }