From de1a30efc7404dd02f14b6afd98cf5ba3cb701f0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 17 May 2016 11:56:38 -0700
Subject: [PATCH] Improve audit behavior for "uninteresting" auditors

Summary:
Ref T10939. Fixes T10174. We can currently trigger "uninteresting" auditors in two ways:

  - Packages with auditing disabled ("NONE" audits).
  - Packages with auditing enabled, but they don't need an audit (e.g., author is a pacakge owner; "NOT REQUIRED" audits).

These audits aren't interesting (we only write them so we can list "commits in this package" from other UIs) but right now they take up the audit slot. In particular:

  - They show in the UI, but are generally useless/confusing nowadays. The actual table of contents does a better job of just showing "which packages do these paths belong to" now, and shows all packages for each path.
  - They block Herald from adding real auditors.

Change this:

  - Don't show uninteresting auditors.
  - Let Herald upgrade uninteresting auditors into real auditors.

Test Plan:
  - Ran `bin/repository reparse --owners <commit> --force`, and `--herald` to trigger Owners and Herald rules.
  - With a package with auditing disabled, triggered a "None" audit and saw it no longer appear in the UI with the patch applied.
  - With a package with auditing disabled, added a Herald rule to trigger an audit. With the patch, saw it go through and upgrade the audit to "Audit Required".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10174, T10939

Differential Revision: https://secure.phabricator.com/D15940
---
 .../audit/editor/PhabricatorAuditEditor.php   | 19 +++++++++++++++----
 .../controller/DiffusionCommitController.php  |  7 ++++++-
 .../herald/DiffusionAuditorsHeraldAction.php  |  9 +++++++--
 .../PhabricatorRepositoryAuditRequest.php     | 10 ++++++++++
 4 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index 3df9013301..8018e5314a 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -159,7 +159,17 @@ final class PhabricatorAuditEditor
         $requests = mpull($requests, null, 'getAuditorPHID');
         foreach ($add as $phid) {
           if (isset($requests[$phid])) {
-            continue;
+            $request = $requests[$phid];
+
+            // Only update an existing request if the current status is not
+            // an interesting status.
+            if ($request->isInteresting()) {
+              continue;
+            }
+          } else {
+            $request = id(new PhabricatorRepositoryAuditRequest())
+              ->setCommitPHID($object->getPHID())
+              ->setAuditorPHID($phid);
           }
 
           if ($this->getIsHeraldEditor()) {
@@ -170,12 +180,13 @@ final class PhabricatorAuditEditor
             $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED;
             $audit_reason = $this->getAuditReasons($phid);
           }
-          $requests[] = id(new PhabricatorRepositoryAuditRequest())
-            ->setCommitPHID($object->getPHID())
-            ->setAuditorPHID($phid)
+
+          $request
             ->setAuditStatus($audit_requested)
             ->setAuditReasons($audit_reason)
             ->save();
+
+          $requests[$phid] = $request;
         }
 
         $object->attachAudits($requests);
diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php
index e2f0d51f4a..419be17898 100644
--- a/src/applications/diffusion/controller/DiffusionCommitController.php
+++ b/src/applications/diffusion/controller/DiffusionCommitController.php
@@ -455,7 +455,12 @@ final class DiffusionCommitController extends DiffusionController {
     if ($audit_requests) {
       $user_requests = array();
       $other_requests = array();
+
       foreach ($audit_requests as $audit_request) {
+        if (!$audit_request->isInteresting()) {
+          continue;
+        }
+
         if ($audit_request->isUser()) {
           $user_requests[] = $audit_request;
         } else {
@@ -471,7 +476,7 @@ final class DiffusionCommitController extends DiffusionController {
 
       if ($other_requests) {
         $view->addProperty(
-          pht('Project/Package Auditors'),
+          pht('Group Auditors'),
           $this->renderAuditStatusView($other_requests));
       }
     }
diff --git a/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php b/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php
index a7dbdde682..3c34876995 100644
--- a/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php
+++ b/src/applications/diffusion/herald/DiffusionAuditorsHeraldAction.php
@@ -18,8 +18,13 @@ abstract class DiffusionAuditorsHeraldAction
     $object = $adapter->getObject();
 
     $auditors = $object->getAudits();
-    $auditors = mpull($auditors, null, 'getAuditorPHID');
-    $current = array_keys($auditors);
+
+    $current = array();
+    foreach ($auditors as $auditor) {
+      if ($auditor->isInteresting()) {
+        $current[] = $auditor->getAuditorPHID();
+      }
+    }
 
     $allowed_types = array(
       PhabricatorPeopleUserPHIDType::TYPECONST,
diff --git a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php
index d9bae65931..f0a17718aa 100644
--- a/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryAuditRequest.php
@@ -49,6 +49,16 @@ final class PhabricatorRepositoryAuditRequest
     return $this->assertAttached($this->commit);
   }
 
+  public function isInteresting() {
+    switch ($this->getAuditStatus()) {
+      case PhabricatorAuditStatusConstants::NONE:
+      case PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED:
+        return false;
+    }
+
+    return true;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */