Update Owners auditing rules for multiple reviewers

Summary:
Ref T10939. Fixes T10181. This slightly simplifies, then documents the auditing rules, which haven't been updated for a while. In particular:

  - If an owner authored the change, never audit.
  - Examine all reviewers to determine reviewer audit status, not just the first reviewer.
  - Simplify some of the loading code a bit.

Test Plan:
  - Ran `bin/repository reparse --owners <commit> --force` to trigger this stuff.
  - Verified that the web UI did reasonable things with resulting audits.
  - Read documentation.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10181, T10939

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

src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

@@ -33,108 +33,132 @@ final class PhabricatorRepositoryCommitOwnersWorker
       $repository,
       $commit,
       PhabricatorUser::getOmnipotentUser());
+
     $affected_packages = PhabricatorOwnersPackage::loadAffectedPackages(
       $repository,
       $affected_paths);
 
-    if ($affected_packages) {
+    if (!$affected_packages) {
-      $requests = id(new PhabricatorRepositoryAuditRequest())
+      return;
-        ->loadAllWhere(
-          'commitPHID = %s',
-          $commit->getPHID());
-      $requests = mpull($requests, null, 'getAuditorPHID');
-
-      foreach ($affected_packages as $package) {
-        $request = idx($requests, $package->getPHID());
-        if ($request) {
-          // Don't update request if it exists already.
-          continue;
-        }
-
-        if ($package->isArchived()) {
-          // Don't trigger audits if the package is archived.
-          continue;
-        }
-
-        if ($package->getAuditingEnabled()) {
-          $reasons = $this->checkAuditReasons($commit, $package);
-          if ($reasons) {
-            $audit_status =
-              PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
-          } else {
-            $audit_status =
-              PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
-          }
-        } else {
-          $reasons = array();
-          $audit_status = PhabricatorAuditStatusConstants::NONE;
-        }
-
-        $relationship = new PhabricatorRepositoryAuditRequest();
-        $relationship->setAuditorPHID($package->getPHID());
-        $relationship->setCommitPHID($commit->getPHID());
-        $relationship->setAuditReasons($reasons);
-        $relationship->setAuditStatus($audit_status);
-
-        $relationship->save();
-
-        $requests[$package->getPHID()] = $relationship;
-      }
-
-      $commit->updateAuditStatus($requests);
-      $commit->save();
     }
-  }
-
-  private function checkAuditReasons(
-    PhabricatorRepositoryCommit $commit,
-    PhabricatorOwnersPackage $package) {
 
     $data = id(new PhabricatorRepositoryCommitData())->loadOneWhere(
       'commitID = %d',
       $commit->getID());
+    $commit->attachCommitData($data);
 
-    $reasons = array();
+    $author_phid = $data->getCommitDetail('authorPHID');
-
-    if ($data->getCommitDetail('vsDiff')) {
-      $reasons[] = pht('Changed After Revision Was Accepted');
-    }
-
-    $commit_author_phid = $data->getCommitDetail('authorPHID');
-    if (!$commit_author_phid) {
-      $reasons[] = pht('Commit Author Not Recognized');
-    }
-
     $revision_id = $data->getCommitDetail('differential.revisionID');
-
-    $revision_author_phid = null;
-    $commit_reviewedby_phid = null;
-
     if ($revision_id) {
       $revision = id(new DifferentialRevisionQuery())
         ->setViewer(PhabricatorUser::getOmnipotentUser())
         ->withIDs(array($revision_id))
+        ->needReviewerStatus(true)
         ->executeOne();
-      if ($revision) {
-        $revision_author_phid = $revision->getAuthorPHID();
-        $commit_reviewedby_phid = $data->getCommitDetail('reviewerPHID');
-        if ($revision_author_phid !== $commit_author_phid) {
-          $reasons[] = pht('Author Not Matching with Revision');
-        }
-      } else {
-        $reasons[] = pht('Revision Not Found');
-      }
-
     } else {
-      $reasons[] = pht('No Revision Specified');
+      $revision = null;
     }
 
-    $owners_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
+    $requests = id(new PhabricatorRepositoryAuditRequest())
-      array($package->getID()));
+      ->loadAllWhere(
+        'commitPHID = %s',
+        $commit->getPHID());
+    $requests = mpull($requests, null, 'getAuditorPHID');
 
-    if (!($commit_author_phid && in_array($commit_author_phid, $owners_phids) ||
+
-        $commit_reviewedby_phid && in_array($commit_reviewedby_phid,
+    foreach ($affected_packages as $package) {
-          $owners_phids))) {
+      $request = idx($requests, $package->getPHID());
+      if ($request) {
+        // Don't update request if it exists already.
+        continue;
+      }
+
+      if ($package->isArchived()) {
+        // Don't trigger audits if the package is archived.
+        continue;
+      }
+
+      if ($package->getAuditingEnabled()) {
+        $reasons = $this->checkAuditReasons(
+          $commit,
+          $package,
+          $author_phid,
+          $revision);
+
+        if ($reasons) {
+          $audit_status = PhabricatorAuditStatusConstants::AUDIT_REQUIRED;
+        } else {
+          $audit_status = PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED;
+        }
+      } else {
+        $reasons = array();
+        $audit_status = PhabricatorAuditStatusConstants::NONE;
+      }
+
+      $relationship = new PhabricatorRepositoryAuditRequest();
+      $relationship->setAuditorPHID($package->getPHID());
+      $relationship->setCommitPHID($commit->getPHID());
+      $relationship->setAuditReasons($reasons);
+      $relationship->setAuditStatus($audit_status);
+
+      $relationship->save();
+
+      $requests[$package->getPHID()] = $relationship;
+    }
+
+    $commit->updateAuditStatus($requests);
+    $commit->save();
+  }
+
+  private function checkAuditReasons(
+    PhabricatorRepositoryCommit $commit,
+    PhabricatorOwnersPackage $package,
+    $author_phid,
+    $revision) {
+
+    $owner_phids = PhabricatorOwnersOwner::loadAffiliatedUserPHIDs(
+      array(
+        $package->getID(),
+      ));
+    $owner_phids = array_fuse($owner_phids);
+
+    $reasons = array();
+
+    if (!$author_phid) {
+      $reasons[] = pht('Commit Author Not Recognized');
+    } else if (isset($owner_phids[$author_phid])) {
+      return $reasons;
+    }
+
+    if (!$revision) {
+      $reasons[] = pht('No Revision Specified');
+      return $reasons;
+    }
+
+    $accepted_statuses = array(
+      DifferentialReviewerStatus::STATUS_ACCEPTED,
+      DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER,
+    );
+    $accepted_statuses = array_fuse($accepted_statuses);
+
+    $found_accept = false;
+    foreach ($revision->getReviewerStatus() as $reviewer) {
+      $reviewer_phid = $reviewer->getReviewerPHID();
+
+      // If this reviewer isn't a package owner, just ignore them.
+      if (empty($owner_phids[$reviewer_phid])) {
+        continue;
+      }
+
+      // If this reviewer accepted the revision and owns the package, we're
+      // all clear and do not need to trigger an audit.
+      if (isset($accepted_statuses[$reviewer->getStatus()])) {
+        $found_accept = true;
+        break;
+      }
+    }
+
+    if (!$found_accept) {
       $reasons[] = pht('Owners Not Involved');
     }
 

src/docs/user/userguide/owners.diviner

@@ -98,11 +98,35 @@ The available settings are:
 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.
 
+These rules also 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
 powerful rules by using Herald.
 
 
+Auditing
+========
+
+You can automatically trigger audits on unreviewed code by configuring
+**Auditing**. The available settings are:
+
+  - **Disabled**: Do not trigger audits.
+  - **Enabled**: Trigger audits.
+
+When enabled, audits are triggered for commits which:
+
+  - affect code owned by the package;
+  - were not authored by a package owner; and
+  - were not accepted by a package owner.
+
+Audits do not trigger if the package has been archived.
+
+The intent of this feature is to make it easy to configure simple auditing
+behavior. If you want more powerful auditing behavior, you can use Herald to
+write more sophisticated rules.
+
+
 Files in Multiple Packages
 ==========================