Support "Review Changes" and "Block Changes" settings for Owners package "Auto Review"
Summary:
Ref T10939. Fixes T8887. This enables and implements the "review" and "blocking review" options for packages.
This is a bit copy-pastey from `DifferentialReviewersHeraldAction`, which doesn't feel awesome. I think the right fix is Glorious Infrasturcture, though -- I filed T10967 to track that.
Test Plan:
  - Set package autoreveiw to "Review".
  - Updated, got a reveiwer.
  - Set autoreview to "blocking".
  - Updated, got a blocking reviewer.
{F1311720}
{F1311721}
{F1311722}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8887, T10939
Differential Revision: https://secure.phabricator.com/D15916
			
			
This commit is contained in:
		| @@ -1536,7 +1536,6 @@ final class DifferentialTransactionEditor | |||||||
|  |  | ||||||
|     $xactions = array(); |     $xactions = array(); | ||||||
|     if ($auto_subscribe) { |     if ($auto_subscribe) { | ||||||
|  |  | ||||||
|       $xactions[] = $object->getApplicationTransactionTemplate() |       $xactions[] = $object->getApplicationTransactionTemplate() | ||||||
|         ->setAuthorPHID($owners_phid) |         ->setAuthorPHID($owners_phid) | ||||||
|         ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) |         ->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; |     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( |   protected function buildHeraldAdapter( | ||||||
|     PhabricatorLiskDAO $object, |     PhabricatorLiskDAO $object, | ||||||
|     array $xactions) { |     array $xactions) { | ||||||
|   | |||||||
| @@ -71,13 +71,12 @@ final class PhabricatorOwnersPackage | |||||||
|       self::AUTOREVIEW_SUBSCRIBE => array( |       self::AUTOREVIEW_SUBSCRIBE => array( | ||||||
|         'name' => pht('Subscribe to Changes'), |         'name' => pht('Subscribe to Changes'), | ||||||
|       ), |       ), | ||||||
|       // TODO: Implement these. |       self::AUTOREVIEW_REVIEW => array( | ||||||
|       // self::AUTOREVIEW_REVIEW => array( |         'name' => pht('Review Changes'), | ||||||
|       //   'name' => pht('Review Changes'), |       ), | ||||||
|       // ), |       self::AUTOREVIEW_BLOCK => array( | ||||||
|       // self::AUTOREVIEW_BLOCK => array( |         'name' => pht('Review Changes (Blocking)'), | ||||||
|       //   'name' => pht('Review Changes (Blocking)'), |       ), | ||||||
|       // ), |  | ||||||
|     ); |     ); | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   | |||||||
| @@ -1577,6 +1577,14 @@ abstract class PhabricatorApplicationTransactionEditor | |||||||
|       $type = $xaction->getTransactionType(); |       $type = $xaction->getTransactionType(); | ||||||
|       if (isset($types[$type])) { |       if (isset($types[$type])) { | ||||||
|         foreach ($types[$type] as $other_key) { |         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); |           $merged = $this->mergeTransactions($result[$other_key], $xaction); | ||||||
|           if ($merged) { |           if ($merged) { | ||||||
|             $result[$other_key] = $merged; |             $result[$other_key] = $merged; | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user
	 epriestley
					epriestley