Write separate comments for every action in Audit

Summary:
Ref T4896. Depends on D10023. Prepares the code for the final migration.

The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.

Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.

Test Plan:
- Used `diffusion.createcomment` to add comments, raise concern, and accept.
- Previewed commenting, adding auditors/ccs, accepting, raising concern.
- Actually performed commenting, adding auditors/ccs, accepting, raising concern.
- Added a user with mentions.
- Added an explicit CC and a mention user.

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T4896

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

src/applications/audit/controller/PhabricatorAuditAddCommentController.php

@@ -15,41 +15,55 @@ final class PhabricatorAuditAddCommentController
     $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
       'phid = %s',
       $commit_phid);
-
     if (!$commit) {
       return new Aphront404Response();
     }
 
     $phids = array($commit_phid);
 
-    $action = $request->getStr('action');
-
-    $comment = id(new PhabricatorAuditComment())
-      ->setAction($action)
-      ->setContent($request->getStr('content'));
+    $comments = array();
 
     // make sure we only add auditors or ccs if the action matches
+    $action = $request->getStr('action');
     switch ($action) {
-      case 'add_auditors':
+      case PhabricatorAuditActionConstants::ADD_AUDITORS:
         $auditors = $request->getArr('auditors');
-        $ccs = array();
+        $comments[] = id(new PhabricatorAuditComment())
+          ->setAction(PhabricatorAuditActionConstants::ADD_AUDITORS)
+          ->setMetadata(
+            array(
+              PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors,
+            ));
         break;
-      case 'add_ccs':
-        $auditors = array();
+      case PhabricatorAuditActionConstants::ADD_CCS:
         $ccs = $request->getArr('ccs');
+        $comments[] = id(new PhabricatorAuditComment())
+          ->setAction(PhabricatorAuditActionConstants::ADD_CCS)
+          ->setMetadata(
+            array(
+              PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs,
+            ));
+        break;
+      case PhabricatorAuditActionConstants::COMMENT:
+        // We'll deal with this below.
         break;
       default:
-        $auditors = array();
-        $ccs = array();
+        $comments[] = id(new PhabricatorAuditComment())
+          ->setAction($action);
         break;
     }
 
+    $content = $request->getStr('content');
+    if (strlen($content)) {
+      $comments[] = id(new PhabricatorAuditComment())
+        ->setAction(PhabricatorAuditActionConstants::COMMENT)
+        ->setContent($content);
+    }
+
     id(new PhabricatorAuditCommentEditor($commit))
       ->setActor($user)
       ->setAttachInlineComments(true)
-      ->addAuditors($auditors)
-      ->addCCs($ccs)
-      ->addComment($comment);
+      ->addComments($comments);
 
     $handles = $this->loadViewerHandles($phids);
     $uri = $handles[$commit_phid]->getURI();

src/applications/audit/controller/PhabricatorAuditPreviewController.php

@@ -20,57 +20,81 @@ final class PhabricatorAuditPreviewController
 
     $action = $request->getStr('action');
 
-    $comment = id(new PhabricatorAuditComment())
-      ->setActorPHID($user->getPHID())
-      ->setTargetPHID($commit->getPHID())
-      ->setAction($action)
-      ->setContent($request->getStr('content'));
-
     $phids = array(
       $user->getPHID(),
       $commit->getPHID(),
     );
 
-    $auditors = $request->getStrList('auditors');
-    if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS && $auditors) {
-      $comment->setMetadata(array(
-        PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors));
-      $phids = array_merge($phids, $auditors);
+    $comments = array();
+
+    if ($action != PhabricatorAuditActionConstants::COMMENT) {
+      $action_comment = id(new PhabricatorAuditComment())
+        ->setActorPHID($user->getPHID())
+        ->setTargetPHID($commit->getPHID())
+        ->setAction($action);
+
+      $auditors = $request->getStrList('auditors');
+      if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS &&
+        $auditors) {
+
+        $action_comment->setMetadata(array(
+          PhabricatorAuditComment::METADATA_ADDED_AUDITORS => $auditors));
+        $phids = array_merge($phids, $auditors);
+      }
+
+      $ccs = $request->getStrList('ccs');
+      if ($action == PhabricatorAuditActionConstants::ADD_CCS && $ccs) {
+        $action_comment->setMetadata(array(
+          PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs));
+        $phids = array_merge($phids, $ccs);
+      }
+
+      $comments[] = $action_comment;
     }
 
-    $ccs = $request->getStrList('ccs');
-    if ($action == PhabricatorAuditActionConstants::ADD_CCS && $ccs) {
-      $comment->setMetadata(array(
-        PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs));
-      $phids = array_merge($phids, $ccs);
+    $content = $request->getStr('content');
+    if (strlen($content)) {
+      $comments[] = id(new PhabricatorAuditComment())
+        ->setActorPHID($user->getPHID())
+        ->setTargetPHID($commit->getPHID())
+        ->setAction(PhabricatorAuditActionConstants::COMMENT)
+        ->setContent($content);
     }
 
     $engine = new PhabricatorMarkupEngine();
     $engine->setViewer($user);
-    $engine->addObject(
-      $comment,
-      PhabricatorAuditComment::MARKUP_FIELD_BODY);
+    foreach ($comments as $comment) {
+      $engine->addObject(
+        $comment,
+        PhabricatorAuditComment::MARKUP_FIELD_BODY);
+    }
     $engine->process();
 
-    $view = id(new DiffusionCommentView())
-      ->setMarkupEngine($engine)
-      ->setUser($user)
-      ->setComment($comment)
-      ->setIsPreview(true);
+    $views = array();
+    foreach ($comments as $comment) {
+      $view = id(new DiffusionCommentView())
+        ->setMarkupEngine($engine)
+        ->setUser($user)
+        ->setComment($comment)
+        ->setIsPreview(true);
 
-    $phids = array_merge($phids, $view->getRequiredHandlePHIDs());
+      $phids = array_merge($phids, $view->getRequiredHandlePHIDs());
+      $views[] = $view;
+    }
 
     $handles = $this->loadViewerHandles($phids);
-    $view->setHandles($handles);
+
+    foreach ($views as $view) {
+      $view->setHandles($handles);
+    }
 
     id(new PhabricatorDraft())
-      ->setAuthorPHID($comment->getActorPHID())
+      ->setAuthorPHID($user->getPHID())
       ->setDraftKey('diffusion-audit-'.$this->id)
-      ->setDraft($comment->getContent())
+      ->setDraft($content)
       ->replaceOrDelete();
 
-    return id(new AphrontAjaxResponse())
-      ->setContent($view->render());
+    return id(new AphrontAjaxResponse())->setContent(hsprintf('%s', $views));
   }
 
 }

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -3,11 +3,7 @@
 final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
 
   private $commit;
-
   private $attachInlineComments;
-  private $auditors = array();
-  private $ccs = array();
-
   private $noEmail;
 
   public function __construct(PhabricatorRepositoryCommit $commit) {
@@ -15,16 +11,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     return $this;
   }
 
-  public function addAuditors(array $auditor_phids) {
-    $this->auditors = array_merge($this->auditors, $auditor_phids);
-    return $this;
-  }
-
-  public function addCCs(array $cc_phids) {
-    $this->ccs = array_merge($this->ccs, $cc_phids);
-    return $this;
-  }
-
   public function setAttachInlineComments($attach_inline_comments) {
     $this->attachInlineComments = $attach_inline_comments;
     return $this;
@@ -35,7 +21,8 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     return $this;
   }
 
-  public function addComment(PhabricatorAuditComment $comment) {
+  public function addComments(array $comments) {
+    assert_instances_of($comments, 'PhabricatorAuditComment');
 
     $commit = $this->commit;
     $actor = $this->getActor();
@@ -51,38 +38,26 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
         $commit->getPHID());
     }
 
-    $comment
-      ->setActorPHID($actor->getPHID())
-      ->setTargetPHID($commit->getPHID())
-      ->save();
+    $content_blocks = array();
+    foreach ($comments as $comment) {
+      $content_blocks[] = $comment->getContent();
+    }
 
-    $content_blocks = array($comment->getContent());
     foreach ($inline_comments as $inline) {
       $content_blocks[] = $inline->getContent();
     }
 
-    $ccs = $this->ccs;
-    $auditors = $this->auditors;
-
-    $metadata = $comment->getMetadata();
-    $metacc = array();
-
     // Find any "@mentions" in the content blocks.
     $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions(
       $this->getActor(),
       $content_blocks);
     if ($mention_ccs) {
-      $metacc = idx(
-        $metadata,
-        PhabricatorAuditComment::METADATA_ADDED_CCS,
-        array());
-      foreach ($mention_ccs as $cc_phid) {
-        $metacc[] = $cc_phid;
-      }
-    }
-
-    if ($metacc) {
-      $ccs = array_merge($ccs, $metacc);
+      $comments[] = id(new PhabricatorAuditComment())
+        ->setAction(PhabricatorAuditActionConstants::ADD_CCS)
+        ->setMetadata(
+          array(
+            PhabricatorAuditComment::METADATA_ADDED_CCS => $mention_ccs,
+          ));
     }
 
     // When an actor submits an audit comment, we update all the audit requests
@@ -101,17 +76,28 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
 
     $action = $comment->getAction();
 
-
     // 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
     // and handle the no-effect cases (e.g., closing and already-closed audit).
 
-
     $actor_is_author = ($actor->getPHID() == $commit->getAuthorPHID());
 
+    // Pick a meaningful action, if we have one.
+    $action = PhabricatorAuditActionConstants::COMMENT;
+    foreach ($comments as $comment) {
+      switch ($comment->getAction()) {
+        case PhabricatorAuditActionConstants::CLOSE:
+        case PhabricatorAuditActionConstants::RESIGN:
+        case PhabricatorAuditActionConstants::ACCEPT:
+        case PhabricatorAuditActionConstants::CONCERN:
+          $action = $comment->getAction();
+          break;
+      }
+    }
+
     if ($action == PhabricatorAuditActionConstants::CLOSE) {
       if (!PhabricatorEnv::getEnvConfig('audit.can-author-close-audit')) {
-          throw new Exception('Cannot Close Audit without enabling'.
+        throw new Exception('Cannot Close Audit without enabling'.
           'audit.can-author-close-audit');
       }
       // "Close" means wipe out all the concerns.
@@ -219,33 +205,34 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       }
     }
 
+    $auditors = array();
+    $ccs = array();
+    foreach ($comments as $comment) {
+      $meta = $comment->getMetadata();
+
+      $auditor_phids = idx(
+        $meta,
+        PhabricatorAuditComment::METADATA_ADDED_AUDITORS,
+        array());
+      foreach ($auditor_phids as $phid) {
+        $auditors[] = $phid;
+      }
+
+      $cc_phids = idx(
+        $meta,
+        PhabricatorAuditComment::METADATA_ADDED_CCS,
+        array());
+      foreach ($cc_phids as $phid) {
+        $ccs[] = $phid;
+      }
+    }
+
     $requests_by_auditor = mpull($requests, null, 'getAuditorPHID');
     $requests_phids = array_keys($requests_by_auditor);
 
     $ccs = array_diff($ccs, $requests_phids);
     $auditors = array_diff($auditors, $requests_phids);
 
-    if ($action == PhabricatorAuditActionConstants::ADD_CCS) {
-      if ($ccs) {
-        $metadata[PhabricatorAuditComment::METADATA_ADDED_CCS] = $ccs;
-        $comment->setMetaData($metadata);
-      } else {
-        $comment->setAction(PhabricatorAuditActionConstants::COMMENT);
-      }
-    }
-
-    if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS) {
-      if ($auditors) {
-        $metadata[PhabricatorAuditComment::METADATA_ADDED_AUDITORS]
-          = $auditors;
-        $comment->setMetaData($metadata);
-      } else {
-        $comment->setAction(PhabricatorAuditActionConstants::COMMENT);
-      }
-    }
-
-    $comment->save();
-
     if ($auditors) {
       foreach ($auditors as $auditor_phid) {
         $audit_requested = PhabricatorAuditStatusConstants::AUDIT_REQUESTED;
@@ -275,7 +262,13 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $commit->updateAuditStatus($requests);
     $commit->save();
 
-    $comments = array($comment);
+    foreach ($comments as $comment) {
+      $comment
+        ->setActorPHID($actor->getPHID())
+        ->setTargetPHID($commit->getPHID())
+        ->save();
+    }
+
     foreach ($inline_comments as $inline) {
       $xaction = id(new PhabricatorAuditComment())
         ->setAction(PhabricatorAuditActionConstants::INLINE)
@@ -307,7 +300,9 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $feed_dont_publish_phids = array_keys($feed_dont_publish_phids);
 
     $feed_phids = array_diff($requests_phids, $feed_dont_publish_phids);
-    $this->publishFeedStory($comment, $feed_phids);
+    foreach ($comments as $comment) {
+      $this->publishFeedStory($comment, $feed_phids);
+    }
 
     id(new PhabricatorSearchIndexer())
       ->queueDocumentForIndexing($commit->getPHID());

src/applications/audit/mail/PhabricatorAuditReplyHandler.php

@@ -45,7 +45,7 @@ final class PhabricatorAuditReplyHandler extends PhabricatorMailReplyHandler {
     $editor->setActor($actor);
     $editor->setExcludeMailRecipientPHIDs(
       $this->getExcludeMailRecipientPHIDs());
-    $editor->addComment($comment);
+    $editor->addComments(array($comment));
   }
 
 }

src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php

@@ -69,14 +69,23 @@ final class DiffusionCreateCommentConduitAPIMethod
       throw new ConduitException('ERR_BAD_ACTION');
     }
 
-    $comment = id(new PhabricatorAuditComment())
-      ->setAction($action)
-      ->setContent($message);
+    $comments = array();
+
+    if ($action != PhabricatorAuditActionConstants::COMMENT) {
+      $comments[] = id(new PhabricatorAuditComment())
+        ->setAction($action);
+    }
+
+    if (strlen($message)) {
+      $comments[] = id(new PhabricatorAuditComment())
+        ->setAction(PhabricatorAuditActionConstants::COMMENT)
+        ->setContent($message);
+    }
 
     id(new PhabricatorAuditCommentEditor($commit))
       ->setActor($request->getUser())
       ->setNoEmail($request->getValue('silent'))
-      ->addComment($comment);
+      ->addComments($comments);
 
     return true;
     // get the full uri of the comment?