diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -982,25 +982,43 @@ return array(); } - // Remove packages that the revision author is an owner of. If you own - // code, you don't need another owner to review it. - $authority = id(new PhabricatorOwnersPackageQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(mpull($packages, 'getPHID')) - ->withAuthorityPHIDs(array($object->getAuthorPHID())) - ->execute(); - $authority = mpull($authority, null, 'getPHID'); + // Identify the packages with "Non-Owner Author" review rules and remove + // them if the author has authority over the package. + + $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + $need_authority = array(); + foreach ($packages as $package) { + $autoreview_setting = $package->getAutoReview(); - foreach ($packages as $key => $package) { - $package_phid = $package->getPHID(); - if (isset($authority[$package_phid])) { - unset($packages[$key]); + $spec = idx($autoreview_map, $autoreview_setting); + if (!$spec) { continue; } + + if (idx($spec, 'authority')) { + $need_authority[$package->getPHID()] = $package->getPHID(); + } } - if (!$packages) { - return array(); + if ($need_authority) { + $authority = id(new PhabricatorOwnersPackageQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($need_authority) + ->withAuthorityPHIDs(array($object->getAuthorPHID())) + ->execute(); + $authority = mpull($authority, null, 'getPHID'); + + foreach ($packages as $key => $package) { + $package_phid = $package->getPHID(); + if (isset($authority[$package_phid])) { + unset($packages[$key]); + continue; + } + } + + if (!$packages) { + return array(); + } } $auto_subscribe = array(); @@ -1009,15 +1027,18 @@ foreach ($packages as $package) { switch ($package->getAutoReview()) { - case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE: - $auto_subscribe[] = $package; - break; case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW: + case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW_ALWAYS: $auto_review[] = $package; break; case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK: + case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK_ALWAYS: $auto_block[] = $package; break; + case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE: + case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE_ALWAYS: + $auto_subscribe[] = $package; + break; case PhabricatorOwnersPackage::AUTOREVIEW_NONE: default: break; diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -33,8 +33,11 @@ const AUTOREVIEW_NONE = 'none'; const AUTOREVIEW_SUBSCRIBE = 'subscribe'; + const AUTOREVIEW_SUBSCRIBE_ALWAYS = 'subscribe-always'; const AUTOREVIEW_REVIEW = 'review'; + const AUTOREVIEW_REVIEW_ALWAYS = 'review-always'; const AUTOREVIEW_BLOCK = 'block'; + const AUTOREVIEW_BLOCK_ALWAYS = 'block-always'; const DOMINION_STRONG = 'strong'; const DOMINION_WEAK = 'weak'; @@ -74,14 +77,26 @@ self::AUTOREVIEW_NONE => array( 'name' => pht('No Autoreview'), ), - self::AUTOREVIEW_SUBSCRIBE => array( - 'name' => pht('Subscribe to Changes'), - ), self::AUTOREVIEW_REVIEW => array( - 'name' => pht('Review Changes'), + 'name' => pht('Review Changes With Non-Owner Author'), + 'authority' => true, ), self::AUTOREVIEW_BLOCK => array( - 'name' => pht('Review Changes (Blocking)'), + 'name' => pht('Review Changes With Non-Owner Author (Blocking)'), + 'authority' => true, + ), + self::AUTOREVIEW_SUBSCRIBE => array( + 'name' => pht('Subscribe to Changes With Non-Owner Author'), + 'authority' => true, + ), + self::AUTOREVIEW_REVIEW_ALWAYS => array( + 'name' => pht('Review All Changes'), + ), + self::AUTOREVIEW_BLOCK_ALWAYS => array( + 'name' => pht('Review All Changes (Blocking)'), + ), + self::AUTOREVIEW_SUBSCRIBE_ALWAYS => array( + 'name' => pht('Subscribe to All Changes'), ), ); } diff --git a/src/docs/user/userguide/owners.diviner b/src/docs/user/userguide/owners.diviner --- a/src/docs/user/userguide/owners.diviner +++ b/src/docs/user/userguide/owners.diviner @@ -84,21 +84,26 @@ created in Differential which affects code in a package, the package can automatically be added as a subscriber or reviewer. -The available settings are: +The available settings allow you to take these actions: - - **No Autoreview**: This package will not be added to new reviews. - - **Subscribe to Changes**: This package will be added to reviews as a - subscriber. Owners will be notified of changes, but not required to act. - **Review Changes**: This package will be added to reviews as a reviewer. Reviews will appear on the dashboards of package owners. - - **Review Changes (Blocking)** This package will be added to reviews - as a blocking reviewer. A package owner will be required to accept changes + - **Review Changes (Blocking)** This package will be added to reviews as a + blocking reviewer. A package owner will be required to accept changes before they may land. + - **Subscribe to Changes**: This package will be added to reviews as a + subscriber. Owners will be notified of changes, but not required to act. + +If you select the **With Non-Owner Author** option for these actions, the +action will not trigger if the author of the revision is a package owner. This +mode may be helpful if you are using Owners mostly to make sure that someone +who is qualified is involved in each change to a piece of code. -NOTE: These rules **do not trigger** if the change author is a package owner. -They only apply to changes made by users who aren't already owners. +If you select the **All** option for these actions, the action will always +trigger even if the author is a package owner. This mode may be helpful if you +are using Owners mostly to suggest reviewers. -These rules also do not trigger if the package has been archived. +These rules do not trigger if the package has been archived. The intent of this feature is to make it easy to configure simple, reasonable behaviors. If you want more tailored or specific triggers, you can write more