Use transactions to apply "add auditors" action in Audit

Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.

There are no longer any readers or writers for metadata, so remove the calls for it.

Test Plan: Added auditors from the web UI.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

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

resources/celerity/map.php

@@ -8,7 +8,7 @@
 return array(
   'names' => array(
     'core.pkg.css' => '66ada2ec',
-    'core.pkg.js' => 'c3965034',
+    'core.pkg.js' => '8cd3cd8c',
     'darkconsole.pkg.js' => 'df001cab',
     'differential.pkg.css' => '4a93db37',
     'differential.pkg.js' => '79503aa4',
@@ -463,7 +463,7 @@ return array(
     'rsrc/js/core/behavior-error-log.js' => 'a5d7cf86',
     'rsrc/js/core/behavior-fancy-datepicker.js' => 'a5573bcd',
     'rsrc/js/core/behavior-file-tree.js' => '88236f00',
-    'rsrc/js/core/behavior-form.js' => '3b1557b3',
+    'rsrc/js/core/behavior-form.js' => '5c54cbf3',
     'rsrc/js/core/behavior-gesture.js' => '3ab51e2c',
     'rsrc/js/core/behavior-global-drag-and-drop.js' => '3672899b',
     'rsrc/js/core/behavior-high-security-warning.js' => '8fc1c918',
@@ -551,7 +551,7 @@ return array(
     'javelin-behavior-aphront-basic-tokenizer' => 'b3a4b884',
     'javelin-behavior-aphront-crop' => 'fa0f4fc2',
     'javelin-behavior-aphront-drag-and-drop-textarea' => '92eb531d',
-    'javelin-behavior-aphront-form-disable-on-submit' => '3b1557b3',
+    'javelin-behavior-aphront-form-disable-on-submit' => '5c54cbf3',
     'javelin-behavior-aphront-more' => 'a80d0378',
     'javelin-behavior-audio-source' => '59b251eb',
     'javelin-behavior-audit-preview' => 'd835b03a',
@@ -1069,11 +1069,6 @@ return array(
       'javelin-dom',
       'javelin-magical-init',
     ),
-    '3b1557b3' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-    ),
     '3b3e1664' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1214,6 +1209,11 @@ return array(
       'javelin-behavior',
       'javelin-stratcom',
     ),
+    '5c54cbf3' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+    ),
     '5fefb143' => array(
       'javelin-behavior',
       'javelin-dom',

src/applications/audit/controller/PhabricatorAuditAddCommentController.php

@@ -15,6 +15,7 @@ final class PhabricatorAuditAddCommentController
     $commit = id(new DiffusionCommitQuery())
       ->setViewer($user)
       ->withPHIDs(array($commit_phid))
+      ->needAuditRequests(true)
       ->executeOne();
     if (!$commit) {
       return new Aphront404Response();
@@ -31,10 +32,7 @@ final class PhabricatorAuditAddCommentController
         $auditors = $request->getArr('auditors');
         $comments[] = id(new PhabricatorAuditComment())
           ->setAction(PhabricatorAuditActionConstants::ADD_AUDITORS)
-          ->setMetadata(
+          ->setNewValue(array_fuse($auditors));
-            array(
-              PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors,
-            ));
         break;
       case PhabricatorAuditActionConstants::ADD_CCS:
         $comments[] = id(new PhabricatorAuditComment())

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -47,10 +47,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $audit_phids = self::loadAuditPHIDsForUser($actor);
     $audit_phids = array_fill_keys($audit_phids, true);
 
-    $requests = id(new PhabricatorRepositoryAuditRequest())
+    $requests = $commit->getAudits();
-      ->loadAllWhere(
-        'commitPHID = %s',
-        $commit->getPHID());
 
     // TODO: We should validate the action, currently we allow anyone to, e.g.,
     // close an audit if they muck with form parameters. I'll followup with this
@@ -182,38 +179,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       }
     }
 
-    $auditors = array();
-
-    foreach ($comments as $comment) {
-      $meta = $comment->getMetadata();
-
-      $auditor_phids = idx(
-        $meta,
-        PhabricatorAuditComment::METADATA_ADDED_AUDITORS,
-        array());
-      foreach ($auditor_phids as $phid) {
-        $auditors[] = $phid;
-      }
-    }
-
-    $requests_by_auditor = mpull($requests, null, 'getAuditorPHID');
-    $requests_phids = array_keys($requests_by_auditor);
-
-    $auditors = array_diff($auditors, $requests_phids);
-
-    if ($auditors) {
-      foreach ($auditors as $auditor_phid) {
-        $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED;
-        $requests[] = id (new PhabricatorRepositoryAuditRequest())
-          ->setCommitPHID($commit->getPHID())
-          ->setAuditorPHID($auditor_phid)
-          ->setAuditStatus($audit_requested)
-          ->setAuditReasons(
-            array('Added by '.$actor->getUsername()))
-          ->save();
-      }
-    }
-
     $commit->updateAuditStatus($requests);
     $commit->save();
 

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -86,7 +86,42 @@ final class PhabricatorAuditEditor
       case PhabricatorAuditActionConstants::INLINE:
         return;
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
-        // TODO: For now, these are applied externally.
+        $new = $xaction->getNewValue();
+        if (!is_array($new)) {
+          $new = array();
+        }
+
+        $old = $xaction->getOldValue();
+        if (!is_array($old)) {
+          $old = array();
+        }
+
+        $add = array_diff_key($new, $old);
+
+        $actor = $this->requireActor();
+
+        $requests = $object->getAudits();
+        $requests = mpull($requests, null, 'getAuditorPHID');
+        foreach ($add as $phid) {
+          if (isset($requests[$phid])) {
+            continue;
+          }
+
+          $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED;
+          $requests[] = id (new PhabricatorRepositoryAuditRequest())
+            ->setCommitPHID($object->getPHID())
+            ->setAuditorPHID($phid)
+            ->setAuditStatus($audit_requested)
+            ->setAuditReasons(
+              array(
+                'Added by '.$actor->getUsername(),
+              ))
+            ->save();
+        }
+
+        $object->updateAuditStatus($requests);
+        $object->attachAudits($requests);
+        $object->save();
         return;
     }
 

src/applications/audit/mail/PhabricatorAuditMailReceiver.php

@@ -17,6 +17,7 @@ final class PhabricatorAuditMailReceiver extends PhabricatorObjectMailReceiver {
     return id(new DiffusionCommitQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
+      ->needAuditRequests(true)
       ->executeOne();
   }
 

src/applications/audit/storage/PhabricatorAuditComment.php

@@ -3,8 +3,6 @@
 final class PhabricatorAuditComment
   implements PhabricatorMarkupInterface {
 
-  const METADATA_ADDED_AUDITORS  = 'added-auditors';
-
   const MARKUP_FIELD_BODY        = 'markup:body';
 
   private $proxyComment;
@@ -145,42 +143,6 @@ final class PhabricatorAuditComment
     }
   }
 
-  public function setMetadata(array $metadata) {
-    if (!$this->proxy->getTransactionType()) {
-      throw new Exception(pht('Call setAction() before getMetadata()!'));
-    }
-
-    $type = $this->proxy->getTransactionType();
-    switch ($type) {
-      case PhabricatorAuditActionConstants::ADD_AUDITORS:
-        $raw_phids = idx($metadata, self::METADATA_ADDED_AUDITORS, array());
-        break;
-      default:
-        throw new Exception(pht('No metadata expected!'));
-    }
-
-    $this->proxy->setNewValue(array_fuse($raw_phids));
-
-    return $this;
-  }
-
-  public function getMetadata() {
-    if (!$this->proxy->getTransactionType()) {
-      throw new Exception(pht('Call setAction() before getMetadata()!'));
-    }
-
-    $type = $this->proxy->getTransactionType();
-    $new_value = $this->proxy->getNewValue();
-    switch ($type) {
-      case PhabricatorAuditActionConstants::ADD_AUDITORS:
-        return array(
-          self::METADATA_ADDED_AUDITORS => array_keys($new_value),
-        );
-    }
-
-    return array();
-  }
-
   public function save() {
     throw new Exception(
       pht('This object can no longer be written to directly!'));

src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php

@@ -43,6 +43,7 @@ final class DiffusionCreateCommentConduitAPIMethod
     $commit = id(new DiffusionCommitQuery())
       ->setViewer($request->getUser())
       ->withPHIDs(array($commit_phid))
+      ->needAuditRequests(true)
       ->executeOne();
     if (!$commit) {
       throw new ConduitException('ERR_BAD_COMMIT');