Add "All" and "With Non-Owner Author" options for all Owners Package autoreview rules

Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.

Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.

Maniphest Tasks: T13099, T11664

Differential Revision: https://secure.phabricator.com/D19180

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -982,25 +982,43 @@ final class DifferentialTransactionEditor
       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.
 
-    foreach ($packages as $key => $package) {
-      $package_phid = $package->getPHID();
-      if (isset($authority[$package_phid])) {
-        unset($packages[$key]);
+    $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap();
+    $need_authority = array();
+    foreach ($packages as $package) {
+      $autoreview_setting = $package->getAutoReview();
+
+      $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 @@ final class DifferentialTransactionEditor
 
     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;

src/applications/owners/storage/PhabricatorOwnersPackage.php

@@ -33,8 +33,11 @@ final class PhabricatorOwnersPackage
 
   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 @@ final class PhabricatorOwnersPackage
       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'),
       ),
     );
   }

src/docs/user/userguide/owners.diviner

@@ -84,21 +84,26 @@ You can configure **Auto Review** for packages. When a new code review is
 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.
 
-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 **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.
 
-These rules also do not trigger if the package has been archived.
+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 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