diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 126d48970d..ed8c310242 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1536,7 +1536,6 @@ final class DifferentialTransactionEditor $xactions = array(); if ($auto_subscribe) { - $xactions[] = $object->getApplicationTransactionTemplate() ->setAuthorPHID($owners_phid) ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) @@ -1546,11 +1545,97 @@ final class DifferentialTransactionEditor )); } - // TODO: Implement autoreview and autoblock, but these are more invovled. + $specs = array( + array($auto_review, false), + array($auto_block, true), + ); + + foreach ($specs as $spec) { + list($reviewers, $blocking) = $spec; + if (!$reviewers) { + continue; + } + + $phids = mpull($reviewers, 'getPHID'); + $xaction = $this->newAutoReviewTransaction($object, $phids, $blocking); + if ($xaction) { + $xactions[] = $xaction; + } + } return $xactions; } + private function newAutoReviewTransaction( + PhabricatorLiskDAO $object, + array $phids, + $is_blocking) { + + // TODO: This is substantially similar to DifferentialReviewersHeraldAction + // and both are needlessly complex. This logic should live in the normal + // transaction application pipeline. See T10967. + + $reviewers = $object->getReviewerStatus(); + $reviewers = mpull($reviewers, null, 'getReviewerPHID'); + + if ($is_blocking) { + $new_status = DifferentialReviewerStatus::STATUS_BLOCKING; + } else { + $new_status = DifferentialReviewerStatus::STATUS_ADDED; + } + + $new_strength = DifferentialReviewerStatus::getStatusStrength( + $new_status); + + $current = array(); + foreach ($phids as $phid) { + if (!isset($reviewers[$phid])) { + continue; + } + + // If we're applying a stronger status (usually, upgrading a reviewer + // into a blocking reviewer), skip this check so we apply the change. + $old_strength = DifferentialReviewerStatus::getStatusStrength( + $reviewers[$phid]->getStatus()); + if ($old_strength <= $new_strength) { + continue; + } + + $current[] = $phid; + } + + $phids = array_diff($phids, $current); + + if (!$phids) { + return null; + } + + $phids = array_fuse($phids); + + $value = array(); + foreach ($phids as $phid) { + $value[$phid] = array( + 'data' => array( + 'status' => $new_status, + ), + ); + } + + $edgetype_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST; + + $owners_phid = id(new PhabricatorOwnersApplication()) + ->getPHID(); + + return $object->getApplicationTransactionTemplate() + ->setAuthorPHID($owners_phid) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $edgetype_reviewer) + ->setNewValue( + array( + '+' => $value, + )); + } + protected function buildHeraldAdapter( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 3f24db8508..e65e7ecfae 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -71,13 +71,12 @@ final class PhabricatorOwnersPackage self::AUTOREVIEW_SUBSCRIBE => array( 'name' => pht('Subscribe to Changes'), ), - // TODO: Implement these. - // self::AUTOREVIEW_REVIEW => array( - // 'name' => pht('Review Changes'), - // ), - // self::AUTOREVIEW_BLOCK => array( - // 'name' => pht('Review Changes (Blocking)'), - // ), + self::AUTOREVIEW_REVIEW => array( + 'name' => pht('Review Changes'), + ), + self::AUTOREVIEW_BLOCK => array( + 'name' => pht('Review Changes (Blocking)'), + ), ); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index de52515c01..ce458a3407 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1577,6 +1577,14 @@ abstract class PhabricatorApplicationTransactionEditor $type = $xaction->getTransactionType(); if (isset($types[$type])) { foreach ($types[$type] as $other_key) { + $other_xaction = $result[$other_key]; + + // Don't merge transactions with different authors. For example, + // don't merge Herald transactions and owners transactions. + if ($other_xaction->getAuthorPHID() != $xaction->getAuthorPHID()) { + continue; + } + $merged = $this->mergeTransactions($result[$other_key], $xaction); if ($merged) { $result[$other_key] = $merged;