diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 04ef915b32..301c81a97d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1141,6 +1141,7 @@ phutil_register_library_map(array( 'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/PhabricatorAuditCommitStatusConstants.php', 'PhabricatorAuditController' => 'applications/audit/controller/PhabricatorAuditController.php', 'PhabricatorAuditDAO' => 'applications/audit/storage/PhabricatorAuditDAO.php', + 'PhabricatorAuditEditor' => 'applications/audit/editor/PhabricatorAuditEditor.php', 'PhabricatorAuditInlineComment' => 'applications/audit/storage/PhabricatorAuditInlineComment.php', 'PhabricatorAuditListController' => 'applications/audit/controller/PhabricatorAuditListController.php', 'PhabricatorAuditListView' => 'applications/audit/view/PhabricatorAuditListView.php', @@ -3934,6 +3935,7 @@ phutil_register_library_map(array( 'PhabricatorAuditCommentEditor' => 'PhabricatorEditor', 'PhabricatorAuditController' => 'PhabricatorController', 'PhabricatorAuditDAO' => 'PhabricatorLiskDAO', + 'PhabricatorAuditEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorAuditInlineComment' => 'PhabricatorInlineCommentInterface', 'PhabricatorAuditListController' => 'PhabricatorAuditController', 'PhabricatorAuditListView' => 'AphrontView', diff --git a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php index cd3d67745b..fb0136c814 100644 --- a/src/applications/audit/controller/PhabricatorAuditAddCommentController.php +++ b/src/applications/audit/controller/PhabricatorAuditAddCommentController.php @@ -12,9 +12,10 @@ final class PhabricatorAuditAddCommentController } $commit_phid = $request->getStr('commit'); - $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'phid = %s', - $commit_phid); + $commit = id(new DiffusionCommitQuery()) + ->setViewer($user) + ->withPHIDs(array($commit_phid)) + ->executeOne(); if (!$commit) { return new Aphront404Response(); } @@ -36,12 +37,11 @@ final class PhabricatorAuditAddCommentController )); break; case PhabricatorAuditActionConstants::ADD_CCS: - $ccs = $request->getArr('ccs'); $comments[] = id(new PhabricatorAuditComment()) - ->setAction(PhabricatorAuditActionConstants::ADD_CCS) - ->setMetadata( + ->setAction(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue( array( - PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs, + '+' => $request->getArr('ccs'), )); break; case PhabricatorAuditActionConstants::COMMENT: diff --git a/src/applications/audit/controller/PhabricatorAuditPreviewController.php b/src/applications/audit/controller/PhabricatorAuditPreviewController.php index 8683d14646..f27b55ae34 100644 --- a/src/applications/audit/controller/PhabricatorAuditPreviewController.php +++ b/src/applications/audit/controller/PhabricatorAuditPreviewController.php @@ -37,8 +37,13 @@ final class PhabricatorAuditPreviewController $ccs = $request->getStrList('ccs'); if ($action == PhabricatorAuditActionConstants::ADD_CCS && $ccs) { - $action_xaction->setTransactionType($action); - $action_xaction->setNewValue(array_fuse($ccs)); + $action_xaction->setTransactionType( + PhabricatorTransactions::TYPE_SUBSCRIBERS); + + // NOTE: This doesn't get processed before use, so just provide fake + // values. + $action_xaction->setOldValue(array()); + $action_xaction->setNewValue($ccs); } $xactions[] = $action_xaction; diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 592c2b90f8..356663d592 100644 --- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php @@ -38,28 +38,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $commit->getPHID()); } - $content_blocks = array(); - foreach ($comments as $comment) { - $content_blocks[] = $comment->getContent(); - } - - foreach ($inline_comments as $inline) { - $content_blocks[] = $inline->getContent(); - } - - // Find any "@mentions" in the content blocks. - $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions( - $this->getActor(), - $content_blocks); - if ($mention_ccs) { - $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 // they have authority over to reflect the most recent status. The general // idea here is that if audit has triggered for, e.g., several packages, but @@ -93,8 +71,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { } } - $add_self_cc = false; - if ($action == PhabricatorAuditActionConstants::CLOSE) { if (!PhabricatorEnv::getEnvConfig('audit.can-author-close-audit')) { throw new Exception('Cannot Close Audit without enabling'. @@ -181,7 +157,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { case PhabricatorAuditActionConstants::COMMENT: case PhabricatorAuditActionConstants::ADD_AUDITORS: case PhabricatorAuditActionConstants::ADD_CCS: - $add_self_cc = true; break; case PhabricatorAuditActionConstants::ACCEPT: $new_status = PhabricatorAuditStatusConstants::ACCEPTED; @@ -208,11 +183,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { } $auditors = array(); - $ccs = array(); - - if ($add_self_cc) { - $ccs[] = $actor->getPHID(); - } foreach ($comments as $comment) { $meta = $comment->getMetadata(); @@ -224,20 +194,11 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { 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 ($auditors) { @@ -256,37 +217,30 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $commit->updateAuditStatus($requests); $commit->save(); - if ($ccs) { - id(new PhabricatorSubscriptionsEditor()) - ->setActor($actor) - ->setObject($commit) - ->subscribeExplicit($ccs) - ->save(); + // Convert old comments into real transactions and apply them with a + // normal editor. + + $xactions = array(); + foreach ($comments as $comment) { + $xactions[] = $comment->getTransactionForSave(); + } + + foreach ($inline_comments as $inline) { + $xactions[] = id(new PhabricatorAuditTransaction()) + ->setTransactionType(PhabricatorAuditActionConstants::INLINE) + ->attachComment($inline->getTransactionComment()); } $content_source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_LEGACY, array()); - foreach ($comments as $comment) { - $comment - ->setActorPHID($actor->getPHID()) - ->setTargetPHID($commit->getPHID()) - ->setContentSource($content_source) - ->save(); - } - - foreach ($inline_comments as $inline) { - $xaction = id(new PhabricatorAuditComment()) - ->setProxyComment($inline->getTransactionCommentForSave()) - ->setAction(PhabricatorAuditActionConstants::INLINE) - ->setActorPHID($actor->getPHID()) - ->setTargetPHID($commit->getPHID()) - ->setContentSource($content_source) - ->save(); - - $comments[] = $xaction; - } + $editor = id(new PhabricatorAuditEditor()) + ->setActor($actor) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true) + ->setContentSource($content_source) + ->applyTransactions($commit, $xactions); $feed_dont_publish_phids = array(); foreach ($requests as $request) { @@ -396,8 +350,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { assert_instances_of($other_comments, 'PhabricatorAuditComment'); assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface'); - $any_comment = head($comments); - $commit = $this->commit; $data = $commit->loadCommitData(); @@ -421,7 +373,12 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { PhabricatorAuditActionConstants::ADD_CCS => 'Added CCs', PhabricatorAuditActionConstants::ADD_AUDITORS => 'Added Auditors', ); - $verb = idx($map, $any_comment->getAction(), 'Commented On'); + if ($comments) { + $any_action = head($comments)->getAction(); + } else { + $any_action = null; + } + $verb = idx($map, $any_action, 'Commented On'); $reply_handler = self::newReplyHandlerForCommit($commit); @@ -444,7 +401,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { $email_to = array(); $email_cc = array(); - $email_to[$any_comment->getActorPHID()] = true; + $email_to[$this->getActor()->getPHID()] = true; $author_phid = $data->getCommitDetail('authorPHID'); if ($author_phid) { @@ -493,7 +450,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor { ->setSubject("{$name}: {$summary}") ->setSubjectPrefix($prefix) ->setVarySubjectPrefix("[{$verb}]") - ->setFrom($any_comment->getActorPHID()) + ->setFrom($this->getActor()->getPHID()) ->setThreadID($thread_id, $is_new) ->addHeader('Thread-Topic', $thread_topic) ->setRelatedPHID($commit->getPHID()) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php new file mode 100644 index 0000000000..2e4a29e9b8 --- /dev/null +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -0,0 +1,114 @@ +getTransactionType()) { + case PhabricatorAuditActionConstants::INLINE: + return $xaction->hasComment(); + } + + return parent::transactionHasEffect($object, $xaction); + } + + protected function getCustomTransactionOldValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + switch ($xaction->getTransactionType()) { + case PhabricatorAuditActionConstants::ACTION: + case PhabricatorAuditActionConstants::INLINE: + return null; + case PhabricatorAuditActionConstants::ADD_AUDITORS: + // TODO: For now, just record the added PHIDs. Eventually, turn these + // into real edge transactions, probably? + return array(); + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorAuditActionConstants::ACTION: + case PhabricatorAuditActionConstants::INLINE: + case PhabricatorAuditActionConstants::ADD_AUDITORS: + return $xaction->getNewValue(); + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + case PhabricatorAuditActionConstants::ACTION: + case PhabricatorAuditActionConstants::INLINE: + case PhabricatorAuditActionConstants::ADD_AUDITORS: + return; + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: + case PhabricatorAuditActionConstants::ACTION: + case PhabricatorAuditActionConstants::INLINE: + return; + case PhabricatorAuditActionConstants::ADD_AUDITORS: + // TODO: For now, these are applied externally. + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function sortTransactions(array $xactions) { + $xactions = parent::sortTransactions($xactions); + + $head = array(); + $tail = array(); + + foreach ($xactions as $xaction) { + $type = $xaction->getTransactionType(); + if ($type == PhabricatorAuditActionConstants::INLINE) { + $tail[] = $xaction; + } else { + $head[] = $xaction; + } + } + + return array_values(array_merge($head, $tail)); + } + +} diff --git a/src/applications/audit/storage/PhabricatorAuditComment.php b/src/applications/audit/storage/PhabricatorAuditComment.php index 8f6acc2c2d..0c8dd73629 100644 --- a/src/applications/audit/storage/PhabricatorAuditComment.php +++ b/src/applications/audit/storage/PhabricatorAuditComment.php @@ -4,7 +4,6 @@ final class PhabricatorAuditComment implements PhabricatorMarkupInterface { const METADATA_ADDED_AUDITORS = 'added-auditors'; - const METADATA_ADDED_CCS = 'added-ccs'; const MARKUP_FIELD_BODY = 'markup:body'; @@ -114,6 +113,7 @@ final class PhabricatorAuditComment switch ($action) { case PhabricatorAuditActionConstants::INLINE: case PhabricatorAuditActionConstants::ADD_CCS: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: case PhabricatorAuditActionConstants::ADD_AUDITORS: $this->proxy->setTransactionType($action); break; @@ -137,6 +137,7 @@ final class PhabricatorAuditComment return PhabricatorAuditActionConstants::COMMENT; case PhabricatorAuditActionConstants::INLINE: case PhabricatorAuditActionConstants::ADD_CCS: + case PhabricatorTransactions::TYPE_SUBSCRIBERS: case PhabricatorAuditActionConstants::ADD_AUDITORS: return $type; default: @@ -151,9 +152,6 @@ final class PhabricatorAuditComment $type = $this->proxy->getTransactionType(); switch ($type) { - case PhabricatorAuditActionConstants::ADD_CCS: - $raw_phids = idx($metadata, self::METADATA_ADDED_CCS, array()); - break; case PhabricatorAuditActionConstants::ADD_AUDITORS: $raw_phids = idx($metadata, self::METADATA_ADDED_AUDITORS, array()); break; @@ -161,7 +159,6 @@ final class PhabricatorAuditComment throw new Exception(pht('No metadata expected!')); } - $this->proxy->setOldValue(array()); $this->proxy->setNewValue(array_fuse($raw_phids)); return $this; @@ -175,10 +172,6 @@ final class PhabricatorAuditComment $type = $this->proxy->getTransactionType(); $new_value = $this->proxy->getNewValue(); switch ($type) { - case PhabricatorAuditActionConstants::ADD_CCS: - return array( - self::METADATA_ADDED_CCS => array_keys($new_value), - ); case PhabricatorAuditActionConstants::ADD_AUDITORS: return array( self::METADATA_ADDED_AUDITORS => array_keys($new_value), @@ -189,28 +182,21 @@ final class PhabricatorAuditComment } public function save() { - $this->proxy->openTransaction(); - $this->proxy - ->setViewPolicy('public') - ->setEditPolicy($this->getActorPHID()) - ->save(); + throw new Exception( + pht('This object can no longer be written to directly!')); + } - if (strlen($this->getContent())) { - $this->getProxyComment() - ->setAuthorPHID($this->getActorPHID()) - ->setViewPolicy('public') - ->setEditPolicy($this->getActorPHID()) - ->setCommentVersion(1) - ->setTransactionPHID($this->proxy->getPHID()) - ->save(); + public function getTransactionForSave() { + $xaction = $this->proxy; + if (strlen($this->getContent())) { + $xaction->attachComment($this->getProxyComment()); + } - $this->proxy - ->setCommentVersion(1) - ->setCommentPHID($this->getProxyComment()->getPHID()) - ->save(); - } - $this->proxy->saveTransaction(); + return $xaction; + } + public function setNewValue($value) { + $this->proxy->setNewValue($value); return $this; } diff --git a/src/applications/audit/storage/PhabricatorAuditInlineComment.php b/src/applications/audit/storage/PhabricatorAuditInlineComment.php index 8beee677b7..a5417376fa 100644 --- a/src/applications/audit/storage/PhabricatorAuditInlineComment.php +++ b/src/applications/audit/storage/PhabricatorAuditInlineComment.php @@ -18,6 +18,10 @@ final class PhabricatorAuditInlineComment return $this->proxy->getTransactionPHID(); } + public function getTransactionComment() { + return $this->proxy; + } + public function getTransactionCommentForSave() { $content_source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_LEGACY, diff --git a/src/applications/audit/storage/PhabricatorAuditTransaction.php b/src/applications/audit/storage/PhabricatorAuditTransaction.php index 1d1be92fa6..10b1a3ecf0 100644 --- a/src/applications/audit/storage/PhabricatorAuditTransaction.php +++ b/src/applications/audit/storage/PhabricatorAuditTransaction.php @@ -83,7 +83,9 @@ final class PhabricatorAuditTransaction switch ($type) { case PhabricatorAuditActionConstants::INLINE: - break; + return pht( + '%s added inline comments.', + $author_handle); case PhabricatorAuditActionConstants::ADD_CCS: if ($add && $rem) { return pht( diff --git a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php index adc6b46911..248c64e37b 100644 --- a/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php @@ -40,10 +40,10 @@ final class DiffusionCreateCommentConduitAPIMethod protected function execute(ConduitAPIRequest $request) { $commit_phid = $request->getValue('phid'); - $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere( - 'phid = %s', - $commit_phid); - + $commit = id(new DiffusionCommitQuery()) + ->setViewer($request->getUser()) + ->withPHIDs(array($commit_phid)) + ->executeOne(); if (!$commit) { throw new ConduitException('ERR_BAD_COMMIT'); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php index 7b8cbd07d2..47eb66f5e9 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php @@ -79,7 +79,7 @@ final class PhabricatorApplicationTransactionCommentEditor if ($xaction->getTransactionType() == $type_comment) { if ($comment->getPHID()) { throw new Exception( - 'Transaction comment must not yet have a PHID!'); + 'Transaction comment must not yet have a PHID!'); } }