diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index 241678487a..b1f51e77e7 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -66,6 +66,12 @@ abstract class DifferentialRevisionActionTransaction return null; } + protected function getRevisionActionMetadata( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return array(); + } + public static function loadAllActions() { return id(new PhutilClassMapQuery()) ->setAncestorClass(__CLASS__) @@ -150,6 +156,11 @@ abstract class DifferentialRevisionActionTransaction $field->setOptions($options); $field->setValue($value); } + + $metadata = $this->getRevisionActionMetadata($revision, $viewer); + foreach ($metadata as $metadata_key => $metadata_value) { + $field->setMetadataValue($metadata_key, $metadata_value); + } } } diff --git a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php index 026b57b55c..32a3ab4a73 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php @@ -6,9 +6,25 @@ final class DifferentialRevisionRequestReviewTransaction const TRANSACTIONTYPE = 'differential.revision.request'; const ACTIONKEY = 'request-review'; + const SOURCE_HARBORMASTER = 'harbormaster'; + const SOURCE_AUTHOR = 'author'; + const SOURCE_VIEWER = 'viewer'; + protected function getRevisionActionLabel( DifferentialRevision $revision, PhabricatorUser $viewer) { + + // See PHI1810. Allow non-authors to "Request Review" on draft revisions + // to promote them out of the draft state. This smoothes over the workflow + // where an author asks for review of an urgent change but has not used + // "Request Review" to skip builds. + + if ($revision->isDraft()) { + if (!$this->isViewerRevisionAuthor($revision, $viewer)) { + return pht('Begin Review Now'); + } + } + return pht('Request Review'); } @@ -16,12 +32,34 @@ final class DifferentialRevisionRequestReviewTransaction DifferentialRevision $revision, PhabricatorUser $viewer) { if ($revision->isDraft()) { - return pht('This revision will be submitted to reviewers for feedback.'); + if (!$this->isViewerRevisionAuthor($revision, $viewer)) { + return pht( + 'This revision will be moved out of the draft state so you can '. + 'review it immediately.'); + } else { + return pht( + 'This revision will be submitted to reviewers for feedback.'); + } } else { return pht('This revision will be returned to reviewers for feedback.'); } } + protected function getRevisionActionMetadata( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + $map = array(); + + if ($revision->isDraft()) { + $action_source = $this->getActorSourceType( + $revision, + $viewer); + $map['promotion.source'] = $action_source; + } + + return $map; + } + protected function getRevisionActionSubmitButtonText( DifferentialRevision $revision, PhabricatorUser $viewer) { @@ -76,29 +114,43 @@ final class DifferentialRevisionRequestReviewTransaction '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)) { - throw new Exception( - pht( - 'You can not request review of this revision because you are not '. - 'the author of the revision.')); - } - } + $this->getActorSourceType($object, $viewer); } public function getTitle() { - return pht( - '%s requested review of this revision.', - $this->renderAuthor()); + $source = $this->getDraftPromotionSource(); + + switch ($source) { + case self::SOURCE_HARBORMASTER: + case self::SOURCE_VIEWER: + case self::SOURCE_AUTHOR: + return pht( + '%s published this revision for review.', + $this->renderAuthor()); + default: + return pht( + '%s requested review of this revision.', + $this->renderAuthor()); + } } public function getTitleForFeed() { - return pht( - '%s requested review of %s.', - $this->renderAuthor(), - $this->renderObject()); + $source = $this->getDraftPromotionSource(); + + switch ($source) { + case self::SOURCE_HARBORMASTER: + case self::SOURCE_VIEWER: + case self::SOURCE_AUTHOR: + return pht( + '%s published %s for review.', + $this->renderAuthor(), + $this->renderObject()); + default: + return pht( + '%s requested review of %s.', + $this->renderAuthor(), + $this->renderObject()); + } } public function getTransactionTypeForConduit($xaction) { @@ -109,4 +161,36 @@ final class DifferentialRevisionRequestReviewTransaction return array(); } + private function getDraftPromotionSource() { + return $this->getMetadataValue('promotion.source'); + } + + private function getActorSourceType( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + + $is_harbormaster = $viewer->isOmnipotent(); + $is_author = $this->isViewerRevisionAuthor($revision, $viewer); + $is_draft = $revision->isDraft(); + + if ($is_harbormaster) { + // When revisions automatically promote out of "Draft" after builds + // finish, the viewer may be acting as the Harbormaster application. + $source = self::SOURCE_HARBORMASTER; + } else if ($is_author) { + $source = self::SOURCE_AUTHOR; + } else if ($is_draft) { + // Non-authors are allowed to "Request Review" on draft revisions, to + // force them into review immediately. + $source = self::SOURCE_VIEWER; + } else { + throw new Exception( + pht( + 'You can not request review of this revision because you are not '. + 'the author of the revision and it is not currently a draft.')); + } + + return $source; + } + }