Make PhabricatorRepositoryCommmit schema changes for audit

Summary:
  - Add a proper mailKey field to make these things mailable. Backfill all
existing objects.
  - Denormalize authorPHID to the commit object so we can query by it
efficiently in a future diff. We currently use the search engine to drive
"commits by author" but that's not so good for audit, which needs more
constraints.
  - Add an overall audit status field so we can efficiently query "commits that
needs your attention".
  - Add enough code to convince myself that these fields are basically
reasonable and work correctly.

Test Plan:
  - Ran schema upgrades. Checked database state afterward.
  - Ran "reparse.php --owners --herald" to verify worker changes.
  - Looked at a commit, altered aggregate status via audits / reparse.php,
verified it responded correctly.

Reviewers: btrahan, jungejason

Reviewed By: jungejason

CC: aran, epriestley, nh

Maniphest Tasks: T904

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

resources/sql/patches/110.commitaudit.sql

@@ -0,0 +1,11 @@
+ALTER TABLE phabricator_repository.repository_commit
+  ADD mailKey VARCHAR(20) NOT NULL;
+
+ALTER TABLE phabricator_repository.repository_commit
+  ADD authorPHID VARCHAR(64) BINARY;
+
+ALTER TABLE phabricator_repository.repository_commit
+  ADD auditStatus INT UNSIGNED NOT NULL;
+
+ALTER TABLE phabricator_repository.repository_commit
+  ADD KEY (authorPHID, auditStatus, epoch);

resources/sql/patches/111.commitauditmigration.php

@@ -0,0 +1,71 @@
+<?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.
+ */
+
+echo "Updating old commit authors...\n";
+
+$table = new PhabricatorRepositoryCommit();
+$conn = $table->establishConnection('w');
+$data = new PhabricatorRepositoryCommitData();
+$commits = queryfx_all(
+  $conn,
+  'SELECT c.id id, c.authorPHID authorPHID, d.commitDetails details
+    FROM %T c JOIN %T d ON d.commitID = c.id
+    WHERE c.authorPHID IS NULL',
+  $table->getTableName(),
+  $data->getTableName());
+
+foreach ($commits as $commit) {
+  $id = $commit['id'];
+  $details = json_decode($commit['details'], true);
+  $author_phid = idx($details, 'authorPHID');
+  if ($author_phid) {
+    queryfx(
+      $conn,
+      'UPDATE %T SET authorPHID = %s WHERE id = %d',
+      $table->getTableName(),
+      $author_phid,
+      $id);
+    echo "#{$id}\n";
+  }
+}
+
+echo "Done.\n";
+
+
+echo "Updating old commit mailKeys...\n";
+
+$table = new PhabricatorRepositoryCommit();
+$conn = $table->establishConnection('w');
+$commits = queryfx_all(
+  $conn,
+  'SELECT id FROM %T WHERE mailKey = %s',
+  $table->getTableName(),
+  '');
+
+foreach ($commits as $commit) {
+  $id = $commit['id'];
+  queryfx(
+    $conn,
+    'UPDATE %T SET mailKey = %s WHERE id = %d',
+    $table->getTableName(),
+    Filesystem::readRandomCharacters(20),
+    $id);
+  echo "#{$id}\n";
+}
+
+echo "Done.\n";

src/__phutil_library_map__.php

@@ -444,6 +444,7 @@ phutil_register_library_map(array(
     'PhabricatorAuditAddCommentController' => 'applications/audit/controller/addcomment',
     'PhabricatorAuditComment' => 'applications/audit/storage/auditcomment',
     'PhabricatorAuditCommentEditor' => 'applications/audit/editor/comment',
+    'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/commitstatus',
     'PhabricatorAuditController' => 'applications/audit/controller/base',
     'PhabricatorAuditDAO' => 'applications/audit/storage/base',
     'PhabricatorAuditEditController' => 'applications/audit/controller/edit',

src/applications/audit/constants/commitstatus/PhabricatorAuditCommitStatusConstants.php

@@ -0,0 +1,43 @@
+<?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 PhabricatorAuditCommitStatusConstants {
+
+  const NONE                = 0;
+  const NEEDS_AUDIT         = 1;
+  const CONCERN_RAISED      = 2;
+  const PARTIALLY_AUDITED   = 3;
+  const FULLY_AUDITED       = 4;
+
+  public static function getStatusNameMap() {
+    static $map = array(
+      self::NONE                => 'None',
+      self::NEEDS_AUDIT         => 'Audit Required',
+      self::CONCERN_RAISED      => 'Concern Raised',
+      self::PARTIALLY_AUDITED   => 'Partially Audited',
+      self::FULLY_AUDITED       => 'Audited',
+    );
+
+    return $map;
+  }
+
+  public static function getStatusName($code) {
+    return idx(self::getStatusNameMap(), $code, 'Unknown');
+  }
+
+}

src/applications/audit/constants/commitstatus/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorAuditCommitStatusConstants.php');

src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php

@@ -89,8 +89,12 @@ final class PhabricatorAuditCommentEditor {
               : PhabricatorAuditStatusConstants::AUDIT_NOT_REQUIRED)
         ->setAuditReasons(array("Voluntary Participant"))
         ->save();
+      $relationships[] = $relationship;
     }
 
+    $commit->updateAuditStatus($relationships);
+    $commit->save();
+
     $this->publishFeedStory($comment, array_keys($audit_phids));
     PhabricatorSearchCommitIndexer::indexCommit($commit);
     $this->sendMail($comment, $other_comments);

src/applications/diffusion/controller/commit/DiffusionCommitController.php

@@ -268,6 +268,11 @@ class DiffusionCommitController extends DiffusionController {
       $props['Differential Revision'] = $handles[$revision_phid]->renderLink();
     }
 
+    if ($commit->getAuditStatus()) {
+      $props['Audit'] = PhabricatorAuditCommitStatusConstants::getStatusName(
+        $commit->getAuditStatus());
+    }
+
     $request = $this->getDiffusionRequest();
 
     $contains = DiffusionContainsQuery::newFromDiffusionRequest($request);

src/applications/diffusion/controller/commit/__init__.php

@@ -7,6 +7,7 @@
 
 
 phutil_require_module('phabricator', 'applications/audit/constants/action');
+phutil_require_module('phabricator', 'applications/audit/constants/commitstatus');
 phutil_require_module('phabricator', 'applications/audit/editor/comment');
 phutil_require_module('phabricator', 'applications/audit/query/audit');
 phutil_require_module('phabricator', 'applications/audit/storage/auditcomment');

src/applications/repository/storage/commit/PhabricatorRepositoryCommit.php

@@ -22,9 +22,9 @@ class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO {
   protected $phid;
   protected $commitIdentifier;
   protected $epoch;
-
+  protected $mailKey;
-  // TODO: Add this!
+  protected $authorPHID;
-  // protected $mailKey;
+  protected $auditStatus = PhabricatorAuditCommitStatusConstants::NONE;
 
   private $commitData;
 
@@ -61,9 +61,11 @@ class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO {
     return $this->commitData;
   }
 
-  public function getMailKey() {
+  public function save() {
-    // TODO: Fix properly!
+    if (!$this->mailKey) {
-    return $this->phid;
+      $this->mailKey = Filesystem::readRandomCharacters(20);
+    }
+    return parent::save();
   }
 
   public function delete() {
@@ -79,4 +81,46 @@ class PhabricatorRepositoryCommit extends PhabricatorRepositoryDAO {
     return $result;
   }
 
+  /**
+   * Synchronize a commit's overall audit status with the individual audit
+   * triggers.
+   */
+  public function updateAuditStatus(array $rships) {
+    $any_concern = false;
+    $any_accept = false;
+    $any_need = false;
+
+    foreach ($rships as $rship) {
+      switch ($rship->getAuditStatus()) {
+        case PhabricatorAuditStatusConstants::AUDIT_REQUIRED:
+          $any_need = true;
+          break;
+        case PhabricatorAuditStatusConstants::ACCEPTED:
+          $any_accept = true;
+          break;
+        case PhabricatorAuditStatusConstants::CONCERNED:
+          $any_concern = true;
+          break;
+      }
+    }
+
+    if ($any_concern) {
+      $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
+    } else if ($any_accept) {
+      if ($any_need) {
+        $status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED;
+      } else {
+        $status = PhabricatorAuditCommitStatusConstants::FULLY_AUDITED;
+      }
+    } else if ($any_need) {
+      $status = PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT;
+    } else {
+      $status = PhabricatorAuditCommitStatusConstants::NONE;
+    }
+
+    return $this->setAuditStatus($status);
+  }
+
+
+
 }

src/applications/repository/storage/commit/__init__.php

@@ -6,11 +6,14 @@
 
 
 
+phutil_require_module('phabricator', 'applications/audit/constants/commitstatus');
+phutil_require_module('phabricator', 'applications/audit/constants/status');
 phutil_require_module('phabricator', 'applications/phid/constants');
 phutil_require_module('phabricator', 'applications/phid/storage/phid');
 phutil_require_module('phabricator', 'applications/repository/storage/base');
 phutil_require_module('phabricator', 'applications/repository/storage/commitdata');
 
+phutil_require_module('phutil', 'filesystem');
 phutil_require_module('phutil', 'utils');
 
 

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

@@ -151,9 +151,8 @@ EOBODY;
 
     $table = new PhabricatorOwnersPackageCommitRelationship();
     $rships = $table->loadAllWhere(
-      'commitPHID = %s AND packagePHID IN (%Ls)',
+      'commitPHID = %s',
-      $commit->getPHID(),
+      $commit->getPHID());
-      array_keys($map));
     $rships = mpull($rships, null, 'getPackagePHID');
 
     $rules = mpull($rules, null, 'getID');
@@ -179,5 +178,7 @@ EOBODY;
       $rship->save();
     }
 
+    $commit->updateAuditStatus($rships);
+    $commit->save();
   }
 }

src/applications/repository/worker/owner/PhabricatorRepositoryCommitOwnersWorker.php

@@ -30,11 +30,14 @@ class PhabricatorRepositoryCommitOwnersWorker
       $affected_paths);
 
     if ($affected_packages) {
-      foreach ($affected_packages as $package) {
+      $rships = id(new PhabricatorOwnersPackageCommitRelationship())
-        $relationship = id(new PhabricatorOwnersPackageCommitRelationship())
+        ->loadAllWhere(
-          ->loadOneWhere('packagePHID=%s AND commitPHID=%s',
+          'commitPHID = %s',
-          $package->getPHID(),
           $commit->getPHID());
+      $rships = mpull($rships, null, 'getPackagePHID');
+
+      foreach ($affected_packages as $package) {
+        $relationship = idx($rships, $package->getPHID());
 
         // Don't update relationship if it exists already
         if (!$relationship) {
@@ -59,8 +62,13 @@ class PhabricatorRepositoryCommitOwnersWorker
           $relationship->setAuditStatus($audit_status);
 
           $relationship->save();
+
+          $rships[] = $relationship;
         }
       }
+
+      $commit->updateAuditStatus($rships);
+      $commit->save();
     }
 
     if ($this->shouldQueueFollowupTasks()) {
@@ -90,7 +98,11 @@ class PhabricatorRepositoryCommitOwnersWorker
     }
 
     $revision_id = $data->getCommitDetail('differential.revisionID');
+
     $revision_author_phid = null;
+    $commit_reviewedby_phid = null;
+    $commit_author_phid = null;
+
     if ($revision_id) {
       $revision = id(new DifferentialRevision())->load($revision_id);
       if ($revision) {