diff --git a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php --- a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php +++ b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php @@ -15,41 +15,55 @@ $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(); diff --git a/src/applications/audit/controller/PhabricatorAuditPreviewController.php b/src/applications/audit/controller/PhabricatorAuditPreviewController.php --- a/src/applications/audit/controller/PhabricatorAuditPreviewController.php +++ b/src/applications/audit/controller/PhabricatorAuditPreviewController.php @@ -20,57 +20,81 @@ $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)); } } diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/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 @@ 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 @@ 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 @@ $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 @@ $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,32 +205,33 @@ } } - $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); + $auditors = array(); + $ccs = array(); + foreach ($comments as $comment) { + $meta = $comment->getMetadata(); - if ($action == PhabricatorAuditActionConstants::ADD_CCS) { - if ($ccs) { - $metadata[PhabricatorAuditComment::METADATA_ADDED_CCS] = $ccs; - $comment->setMetaData($metadata); - } else { - $comment->setAction(PhabricatorAuditActionConstants::COMMENT); + $auditor_phids = idx( + $meta, + PhabricatorAuditComment::METADATA_ADDED_AUDITORS, + array()); + foreach ($auditor_phids as $phid) { + $auditors[] = $phid; } - } - if ($action == PhabricatorAuditActionConstants::ADD_AUDITORS) { - if ($auditors) { - $metadata[PhabricatorAuditComment::METADATA_ADDED_AUDITORS] - = $auditors; - $comment->setMetaData($metadata); - } else { - $comment->setAction(PhabricatorAuditActionConstants::COMMENT); + $cc_phids = idx( + $meta, + PhabricatorAuditComment::METADATA_ADDED_CCS, + array()); + foreach ($cc_phids as $phid) { + $ccs[] = $phid; } } - $comment->save(); + $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 ($auditors) { foreach ($auditors as $auditor_phid) { @@ -275,7 +262,13 @@ $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 @@ $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()); diff --git a/src/applications/audit/mail/PhabricatorAuditReplyHandler.php b/src/applications/audit/mail/PhabricatorAuditReplyHandler.php --- a/src/applications/audit/mail/PhabricatorAuditReplyHandler.php +++ b/src/applications/audit/mail/PhabricatorAuditReplyHandler.php @@ -45,7 +45,7 @@ $editor->setActor($actor); $editor->setExcludeMailRecipientPHIDs( $this->getExcludeMailRecipientPHIDs()); - $editor->addComment($comment); + $editor->addComments(array($comment)); } } diff --git a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php --- a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php @@ -69,14 +69,23 @@ 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?