Page MenuHomePhabricator
Files F13984954

D12129.id29186.diff
No OneTemporary
Actions

D12129.id29186.diff
View Options

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:

File Metadata

Mime Type
text/plain
Expires
Mon, Oct 21, 5:08 PM (4 w, 1 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6729148
Default Alt Text
D12129.id29186.diff (13 KB)

Event Timeline

Phabricator · Built by Phacility