Add an "Auditors" field to commit messages which pushes audit requests when present

Summary:
Adds an optional "Auditors" field (like "Reviewers") to commit messages which gives installs a zero-config method for making audit requests.

This field does not appear on templates unless set, and is mostly ignored (but validated and preserved) by Differential.

It is then parsed by the daemons if present, and audit requests are pushed to valid users.

Test Plan: Made an "Auditors" commit and verified it was retained with "arc amend --show". Pushed it and verified the audit was triggered.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, epriestley

Maniphest Tasks: T904, T880

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

src/__phutil_library_map__.php

@@ -185,6 +185,7 @@ phutil_register_library_map(array(
     'DifferentialAffectedPath' => 'applications/differential/storage/affectedpath',
     'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch',
     'DifferentialArcanistProjectFieldSpecification' => 'applications/differential/field/specification/arcanistproject',
+    'DifferentialAuditorsFieldSpecification' => 'applications/differential/field/specification/auditors',
     'DifferentialAuthorFieldSpecification' => 'applications/differential/field/specification/author',
     'DifferentialAuxiliaryField' => 'applications/differential/storage/auxiliaryfield',
     'DifferentialBlameRevisionFieldSpecification' => 'applications/differential/field/specification/blamerev',
@@ -1050,6 +1051,7 @@ phutil_register_library_map(array(
     'DifferentialAffectedPath' => 'DifferentialDAO',
     'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialArcanistProjectFieldSpecification' => 'DifferentialFieldSpecification',
+    'DifferentialAuditorsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialAuthorFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialAuxiliaryField' => 'DifferentialDAO',
     'DifferentialBlameRevisionFieldSpecification' => 'DifferentialFieldSpecification',

src/applications/audit/constants/status/PhabricatorAuditStatusConstants.php

@@ -18,11 +18,12 @@
 
 final class PhabricatorAuditStatusConstants {
 
-  const NONE = '';
+  const NONE                    = '';
-  const AUDIT_NOT_REQUIRED = 'audit-not-required';
+  const AUDIT_NOT_REQUIRED      = 'audit-not-required';
-  const AUDIT_REQUIRED = 'audit-required';
+  const AUDIT_REQUIRED          = 'audit-required';
-  const CONCERNED = 'concerned';
+  const CONCERNED               = 'concerned';
-  const ACCEPTED = 'accepted';
+  const ACCEPTED                = 'accepted';
+  const AUDIT_REQUESTED         = 'requested';
 
   public static function getStatusNameMap() {
     static $map = array(
@@ -31,6 +32,7 @@ final class PhabricatorAuditStatusConstants {
       self::AUDIT_REQUIRED      => 'Audit Required',
       self::CONCERNED           => 'Concern Raised',
       self::ACCEPTED            => 'Accepted',
+      self::AUDIT_REQUESTED     => 'Audit Requested',
     );
 
     return $map;

src/applications/audit/query/audit/PhabricatorAuditQuery.php

@@ -98,6 +98,7 @@ final class PhabricatorAuditQuery {
           array(
             PhabricatorAuditStatusConstants::AUDIT_REQUIRED,
             PhabricatorAuditStatusConstants::CONCERNED,
+            PhabricatorAuditStatusConstants::AUDIT_REQUESTED,
           ));
         break;
       case self::STATUS_ANY:

src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php

@@ -55,6 +55,7 @@ final class DifferentialDefaultFieldSelector
         new DifferentialGitSVNIDFieldSpecification(),
         new DifferentialDateModifiedFieldSpecification(),
         new DifferentialDateCreatedFieldSpecification(),
+        new DifferentialAuditorsFieldSpecification(),
       ));
 
     return $fields;

src/applications/differential/field/selector/default/__init__.php

@@ -9,6 +9,7 @@
 phutil_require_module('phabricator', 'applications/differential/field/selector/base');
 phutil_require_module('phabricator', 'applications/differential/field/specification/applypatch');
 phutil_require_module('phabricator', 'applications/differential/field/specification/arcanistproject');
+phutil_require_module('phabricator', 'applications/differential/field/specification/auditors');
 phutil_require_module('phabricator', 'applications/differential/field/specification/author');
 phutil_require_module('phabricator', 'applications/differential/field/specification/branch');
 phutil_require_module('phabricator', 'applications/differential/field/specification/ccs');

src/applications/differential/field/specification/auditors/DifferentialAuditorsFieldSpecification.php

@@ -0,0 +1,79 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialAuditorsFieldSpecification
+  extends DifferentialFieldSpecification {
+
+  private $auditors = array();
+
+  public function shouldAppearOnCommitMessage() {
+    return true;
+  }
+
+  public function shouldAppearOnCommitMessageTemplate() {
+    return false;
+  }
+
+  public function getCommitMessageKey() {
+    return 'auditorPHIDs';
+  }
+
+  public function setValueFromParsedCommitMessage($value) {
+    $this->auditors = nonempty($value, array());
+    return $this;
+  }
+
+  public function renderLabelForCommitMessage() {
+    return 'Auditors';
+  }
+
+  public function getRequiredHandlePHIDsForCommitMessage() {
+    return $this->auditors;
+  }
+
+  public function renderValueForCommitMessage($is_edit) {
+    if (!$this->auditors) {
+      return null;
+    }
+
+    $names = array();
+    foreach ($this->auditors as $phid) {
+      $names[] = $this->getHandle($phid)->getName();
+    }
+
+    return implode(', ', $names);
+  }
+
+  public function parseValueFromCommitMessage($value) {
+    return $this->parseCommitMessageUserList($value);
+  }
+
+  public function getStorageKey() {
+    return 'phabricator:auditors';
+  }
+
+  public function getValueForStorage() {
+    return json_encode($this->auditors);
+  }
+
+  public function setValueFromStorage($value) {
+    $this->auditors = json_decode($value, true);
+    return $this;
+  }
+
+}

src/applications/differential/field/specification/auditors/__init__.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('DifferentialAuditorsFieldSpecification.php');

src/applications/differential/field/specification/base/DifferentialFieldSpecification.php

@@ -639,10 +639,17 @@ abstract class DifferentialFieldSpecification {
    * Parse and lookup a list of object names, converting them to PHIDs.
    *
    * @param string Raw list of comma-separated object names.
+   * @param bool   True to include mailing lists.
+   * @param bool   True to make a best effort. By default, an exception is
+   *               thrown if any item is invalid.
    * @return list List of corresponding PHIDs.
    * @task load
    */
-  private function parseCommitMessageObjectList($value, $include_mailables) {
+  public static function parseCommitMessageObjectList(
+    $value,
+    $include_mailables,
+    $allow_partial = false) {
+
     $value = array_unique(array_filter(preg_split('/[\s,]+/', $value)));
     if (!$value) {
       return array();
@@ -652,7 +659,6 @@ abstract class DifferentialFieldSpecification {
 
     $users = id(new PhabricatorUser())->loadAllWhere(
       '((username IN (%Ls)) OR (email IN (%Ls)))
-        AND isDisabled = 0
         AND isSystemAgent = 0',
       $value,
       $value);
@@ -691,14 +697,13 @@ abstract class DifferentialFieldSpecification {
       }
     }
 
-    if ($invalid) {
+    if ($invalid && !$allow_partial) {
       $invalid = implode(', ', $invalid);
       $what = $include_mailables
         ? "users and mailing lists"
         : "users";
       throw new DifferentialFieldParseException(
-        "Commit message references disabled or nonexistent {$what}: ".
+        "Commit message references nonexistent {$what}: {$invalid}.");
-        "{$invalid}.");
     }
 
     return array_unique($results);

src/applications/differential/field/specification/reviewers/DifferentialReviewersFieldSpecification.php

@@ -19,7 +19,7 @@
 final class DifferentialReviewersFieldSpecification
   extends DifferentialFieldSpecification {
 
-  private $reviewers;
+  private $reviewers = array();
   private $error;
 
   public function shouldAppearOnRevisionView() {

src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php

@@ -27,6 +27,11 @@ class PhabricatorRepositoryCommitHeraldWorker
       'commitID = %d',
       $commit->getID());
 
+    if (!$data) {
+      // TODO: Permanent failure.
+      return;
+    }
+
     $rules = HeraldRule::loadAllByContentTypeWithFullData(
       HeraldContentTypeConfig::CONTENT_TYPE_COMMIT,
       $commit->getPHID());
@@ -45,6 +50,8 @@ class PhabricatorRepositoryCommitHeraldWorker
       $this->createAudits($commit, $audit_phids, $rules);
     }
 
+    $this->createAuditsFromCommitMessage($commit, $data);
+
     $email_phids = $adapter->getEmailPHIDs();
     if (!$email_phids) {
       return;
@@ -173,8 +180,7 @@ EOBODY;
     array $map,
     array $rules) {
 
-    $table = new PhabricatorRepositoryAuditRequest();
+    $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere(
-    $requests = $table->loadAllWhere(
       'commitPHID = %s',
       $commit->getPHID());
     $requests = mpull($requests, null, 'getAuditorPHID');
@@ -205,4 +211,58 @@ EOBODY;
     $commit->updateAuditStatus($requests);
     $commit->save();
   }
+
+
+  /**
+   * Find audit requests in the "Auditors" field if it is present and trigger
+   * explicit audit requests.
+   */
+  private function createAuditsFromCommitMessage(
+    PhabricatorRepositoryCommit $commit,
+    PhabricatorRepositoryCommitData $data) {
+
+    $message = $data->getCommitMessage();
+
+    $matches = null;
+    if (!preg_match('/^Auditors:\s*(.*)$/im', $message, $matches)) {
+      return;
+    }
+
+    $phids = DifferentialFieldSpecification::parseCommitMessageObjectList(
+      $matches[1],
+      $include_mailables = false,
+      $allow_partial = true);
+
+    if (!$phids) {
+      return;
+    }
+
+    $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere(
+      'commitPHID = %s',
+      $commit->getPHID());
+    $requests = mpull($requests, null, 'getAuditorPHID');
+
+    foreach ($phids as $phid) {
+      if (isset($requests[$phid])) {
+        continue;
+      }
+
+      $request = new PhabricatorRepositoryAuditRequest();
+      $request->setCommitPHID($commit->getPHID());
+      $request->setAuditorPHID($phid);
+      $request->setAuditStatus(
+        PhabricatorAuditStatusConstants::AUDIT_REQUESTED);
+      $request->setAuditReasons(
+        array(
+          'Requested by Author',
+        ));
+      $request->save();
+
+      $requests[$phid] = $request;
+    }
+
+    $commit->updateAuditStatus($requests);
+    $commit->save();
+  }
+
 }

src/applications/repository/worker/herald/__init__.php

@@ -8,6 +8,7 @@
 
 phutil_require_module('phabricator', 'applications/audit/constants/status');
 phutil_require_module('phabricator', 'applications/audit/editor/comment');
+phutil_require_module('phabricator', 'applications/differential/field/specification/base');
 phutil_require_module('phabricator', 'applications/herald/adapter/commit');
 phutil_require_module('phabricator', 'applications/herald/config/contenttype');
 phutil_require_module('phabricator', 'applications/herald/engine/engine');