diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -9,7 +9,8 @@ private $heraldEmailPHIDs = array(); private $affectedFiles; private $rawPatch; - private $expandedDone; + + private $didExpandInlineState; public function addAuditReason($phid, $reason) { if (!isset($this->auditReasonMap[$phid])) { @@ -53,9 +54,9 @@ $types[] = PhabricatorTransactions::TYPE_COMMENT; $types[] = PhabricatorTransactions::TYPE_EDGE; + $types[] = PhabricatorTransactions::TYPE_INLINESTATE; $types[] = PhabricatorAuditTransaction::TYPE_COMMIT; - $types[] = PhabricatorAuditTransaction::TYPE_INLINEDONE; // TODO: These will get modernized eventually, but that can happen one // at a time later on. @@ -104,7 +105,6 @@ case PhabricatorAuditActionConstants::INLINE: case PhabricatorAuditActionConstants::ADD_AUDITORS: case PhabricatorAuditTransaction::TYPE_COMMIT: - case PhabricatorAuditTransaction::TYPE_INLINEDONE: return $xaction->getNewValue(); } @@ -123,7 +123,6 @@ case PhabricatorAuditActionConstants::INLINE: case PhabricatorAuditActionConstants::ADD_AUDITORS: case PhabricatorAuditTransaction::TYPE_COMMIT: - case PhabricatorAuditTransaction::TYPE_INLINEDONE: return; } @@ -147,18 +146,6 @@ $reply->setHasReplies(1)->save(); } return; - case PhabricatorAuditTransaction::TYPE_INLINEDONE: - $table = new PhabricatorAuditTransactionComment(); - $conn_w = $table->establishConnection('w'); - foreach ($xaction->getNewValue() as $phid => $state) { - queryfx( - $conn_w, - 'UPDATE %T SET fixedState = %s WHERE phid = %s', - $table->getTableName(), - $state, - $phid); - } - return; case PhabricatorAuditActionConstants::ADD_AUDITORS: $new = $xaction->getNewValue(); if (!is_array($new)) { @@ -204,6 +191,28 @@ return parent::applyCustomExternalTransaction($object, $xaction); } + protected function applyBuiltinExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_INLINESTATE: + $table = new PhabricatorAuditTransactionComment(); + $conn_w = $table->establishConnection('w'); + foreach ($xaction->getNewValue() as $phid => $state) { + queryfx( + $conn_w, + 'UPDATE %T SET fixedState = %s WHERE phid = %s', + $table->getTableName(), + $state, + $phid); + } + return; + } + + return parent::applyBuiltinExternalTransaction($object, $xaction); + } + protected function applyFinalEffects( PhabricatorLiskDAO $object, array $xactions) { @@ -361,12 +370,11 @@ break; } - if (!$this->expandedDone) { - + if (!$this->didExpandInlineState) { switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_COMMENT: case PhabricatorAuditActionConstants::ACTION: - $this->expandedDone = true; + $this->didExpandInlineState = true; $actor_phid = $this->getActingAsPHID(); $actor_is_author = ($object->getAuthorPHID() == $actor_phid); @@ -374,12 +382,7 @@ break; } - $state_map = array( - PhabricatorInlineCommentInterface::STATE_DRAFT => - PhabricatorInlineCommentInterface::STATE_DONE, - PhabricatorInlineCommentInterface::STATE_UNDRAFT => - PhabricatorInlineCommentInterface::STATE_UNDONE, - ); + $state_map = PhabricatorTransactions::getInlineStateMap(); $inlines = id(new DiffusionDiffInlineCommentQuery()) ->setViewer($this->getActor()) @@ -398,7 +401,7 @@ } $xactions[] = id(new PhabricatorAuditTransaction()) - ->setTransactionType(PhabricatorAuditTransaction::TYPE_INLINEDONE) + ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) ->setIgnoreOnNoEffect(true) ->setOldValue($old_value) ->setNewValue($new_value); diff --git a/src/applications/audit/storage/PhabricatorAuditTransaction.php b/src/applications/audit/storage/PhabricatorAuditTransaction.php --- a/src/applications/audit/storage/PhabricatorAuditTransaction.php +++ b/src/applications/audit/storage/PhabricatorAuditTransaction.php @@ -4,7 +4,6 @@ extends PhabricatorApplicationTransaction { const TYPE_COMMIT = 'audit:commit'; - const TYPE_INLINEDONE = 'audit:inlinedone'; const MAILTAG_ACTION_CONCERN = 'audit-action-concern'; const MAILTAG_ACTION_ACCEPT = 'audit-action-accept'; @@ -183,36 +182,6 @@ } return $title; - case self::TYPE_INLINEDONE: - $done = 0; - $undone = 0; - foreach ($new as $phid => $state) { - if ($state == PhabricatorInlineCommentInterface::STATE_DONE) { - $done++; - } else { - $undone++; - } - } - if ($done && $undone) { - return pht( - '%s marked %s inline comment(s) as done and %s inline comment(s) '. - 'as not done.', - $author_handle, - new PhutilNumber($done), - new PhutilNumber($undone)); - } else if ($done) { - return pht( - '%s marked %s inline comment(s) as done.', - $author_handle, - new PhutilNumber($done)); - } else { - return pht( - '%s marked %s inline comment(s) as not done.', - $author_handle, - new PhutilNumber($undone)); - } - break; - case PhabricatorAuditActionConstants::INLINE: return pht( '%s added inline comments.', @@ -418,39 +387,17 @@ return parent::getBodyForFeed($story); } - - public function shouldGenerateOldValue() { + public function isInlineCommentTransaction() { switch ($this->getTransactionType()) { - case self::TYPE_INLINEDONE: - return false; - } - - return parent::shouldGenerateOldValue(); - } - - - // TODO: These two mail methods can likely be abstracted by introducing a - // formal concept of "inline comment" transactions. - - public function shouldHideForMail(array $xactions) { - $type_inline = PhabricatorAuditActionConstants::INLINE; - switch ($this->getTransactionType()) { - case $type_inline: - foreach ($xactions as $xaction) { - if ($xaction->getTransactionType() != $type_inline) { - return true; - } - } - return ($this !== head($xactions)); + case PhabricatorAuditActionConstants::INLINE: + return true; } - return parent::shouldHideForMail($xactions); + return parent::isInlineCommentTransaction(); } public function getBodyForMail() { switch ($this->getTransactionType()) { - case PhabricatorAuditActionConstants::INLINE: - return null; case self::TYPE_COMMIT: $data = $this->getNewValue(); return $data['description']; diff --git a/src/applications/transactions/constants/PhabricatorTransactions.php b/src/applications/transactions/constants/PhabricatorTransactions.php --- a/src/applications/transactions/constants/PhabricatorTransactions.php +++ b/src/applications/transactions/constants/PhabricatorTransactions.php @@ -11,6 +11,7 @@ const TYPE_CUSTOMFIELD = 'core:customfield'; const TYPE_BUILDABLE = 'harbormaster:buildable'; const TYPE_TOKEN = 'token:give'; + const TYPE_INLINESTATE = 'core:inlinestate'; const COLOR_RED = 'red'; const COLOR_ORANGE = 'orange'; @@ -23,4 +24,14 @@ const COLOR_GREY = 'grey'; const COLOR_BLACK = 'black'; + + public static function getInlineStateMap() { + return array( + PhabricatorInlineCommentInterface::STATE_DRAFT => + PhabricatorInlineCommentInterface::STATE_DONE, + PhabricatorInlineCommentInterface::STATE_UNDRAFT => + PhabricatorInlineCommentInterface::STATE_UNDONE, + ); + } + } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -294,6 +294,7 @@ case PhabricatorTransactions::TYPE_JOIN_POLICY: case PhabricatorTransactions::TYPE_BUILDABLE: case PhabricatorTransactions::TYPE_TOKEN: + case PhabricatorTransactions::TYPE_INLINESTATE: return $xaction->getNewValue(); case PhabricatorTransactions::TYPE_EDGE: return $this->getEdgeTransactionNewValue($xaction); @@ -395,6 +396,8 @@ case PhabricatorTransactions::TYPE_CUSTOMFIELD: $field = $this->getCustomFieldForTransaction($object, $xaction); return $field->applyApplicationTransactionInternalEffects($xaction); + case PhabricatorTransactions::TYPE_INLINESTATE: + return $this->applyBuiltinInternalTransaction($object, $xaction); } return $this->applyCustomInternalTransaction($object, $xaction); @@ -489,6 +492,8 @@ case PhabricatorTransactions::TYPE_CUSTOMFIELD: $field = $this->getCustomFieldForTransaction($object, $xaction); return $field->applyApplicationTransactionExternalEffects($xaction); + case PhabricatorTransactions::TYPE_INLINESTATE: + return $this->applyBuiltinExternalTransaction($object, $xaction); } return $this->applyCustomExternalTransaction($object, $xaction); @@ -512,6 +517,23 @@ "implementation!"); } + // TODO: Write proper documentation for these hooks. These are like the + // "applyCustom" hooks, except that implementation is optional, so you do + // not need to handle all of the builtin transaction types. See T6403. These + // are not completely implemented. + + protected function applyBuiltinInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + return; + } + + protected function applyBuiltinExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + return; + } + /** * Fill in a transaction's common values, like author and content source. */ diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -57,6 +57,7 @@ case PhabricatorTransactions::TYPE_BUILDABLE: case PhabricatorTransactions::TYPE_TOKEN: case PhabricatorTransactions::TYPE_CUSTOMFIELD: + case PhabricatorTransactions::TYPE_INLINESTATE: return false; } return true; @@ -501,6 +502,26 @@ break; } + // If a transaction publishes an inline comment: + // + // - Don't show it if there are other kinds of transactions. The + // rationale here is that application mail will make the presence + // of inline comments obvious enough by including them prominently + // in the body. We could change this in the future if the obviousness + // needs to be increased. + // - If there are only inline transactions, only show the first + // transaction. The rationale is that seeing multiple "added an inline + // comment" transactions is not useful. + + if ($this->isInlineCommentTransaction()) { + foreach ($xactions as $xaction) { + if (!$xaction->isInlineCommentTransaction()) { + return true; + } + } + return ($this !== head($xactions)); + } + return $this->shouldHide(); } @@ -539,10 +560,18 @@ } public function getBodyForMail() { + if ($this->isInlineCommentTransaction()) { + // We don't return inline comment content as mail body content, because + // applications need to contextualize it (by adding line numbers, for + // example) in order for it to make sense. + return null; + } + $comment = $this->getComment(); if ($comment && strlen($comment->getContent())) { return $comment->getContent(); } + return null; } @@ -720,6 +749,36 @@ return null; } + case PhabricatorTransactions::TYPE_INLINESTATE: + $done = 0; + $undone = 0; + foreach ($new as $phid => $state) { + if ($state == PhabricatorInlineCommentInterface::STATE_DONE) { + $done++; + } else { + $undone++; + } + } + if ($done && $undone) { + return pht( + '%s marked %s inline comment(s) as done and %s inline comment(s) '. + 'as not done.', + $this->renderHandleLink($author_phid), + new PhutilNumber($done), + new PhutilNumber($undone)); + } else if ($done) { + return pht( + '%s marked %s inline comment(s) as done.', + $this->renderHandleLink($author_phid), + new PhutilNumber($done)); + } else { + return pht( + '%s marked %s inline comment(s) as not done.', + $this->renderHandleLink($author_phid), + new PhutilNumber($undone)); + } + break; + default: return pht( '%s edited this %s.', @@ -932,6 +991,10 @@ return false; } + public function isInlineCommentTransaction() { + return false; + } + public function getActionName() { switch ($this->getTransactionType()) { case PhabricatorTransactions::TYPE_COMMENT: