From e57dbcda336ac7c8e2667fa72abccc31289a8244 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Mar 2018 17:36:14 -0800 Subject: [PATCH] Hide "abraham landed Dxyz irresponsibly" stories from feed Summary: Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance. These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story. Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time. Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly". Maniphest Tasks: T13099, T12787 Differential Revision: https://secure.phabricator.com/D19179 --- ...ferentialRevisionWrongStateTransaction.php | 5 +-- .../storage/ManiphestTransaction.php | 16 --------- .../ManiphestTaskUnblockTransaction.php | 10 ++++++ .../storage/PhrictionTransaction.php | 12 ------- .../PhrictionDocumentMoveAwayTransaction.php | 4 +++ .../PhrictionDocumentMoveToTransaction.php | 4 +++ ...ricatorApplicationTransactionFeedStory.php | 33 ++++++++++++++++++- .../storage/PhabricatorModularTransaction.php | 8 +++++ .../PhabricatorModularTransactionType.php | 4 +++ 9 files changed, 65 insertions(+), 31 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php index e19e54199c..47678e886f 100644 --- a/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionWrongStateTransaction.php @@ -35,7 +35,8 @@ final class DifferentialRevisionWrongStateTransaction $this->renderValue($status->getDisplayName())); } - public function getTitleForFeed() { - return null; + public function shouldHideForFeed() { + return true; } + } diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 6301241050..e2739c1d09 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -40,22 +40,6 @@ final class ManiphestTransaction return parent::shouldGenerateOldValue(); } - public function shouldHideForFeed() { - // NOTE: Modular transactions don't currently support this, and it has - // very few callsites, and it's publish-time rather than display-time. - // This should probably become a supported, display-time behavior. For - // discussion, see T12787. - - // Hide "alice created X, a task blocking Y." from feed because it - // will almost always appear adjacent to "alice created Y". - $is_new = $this->getMetadataValue('blocker.new'); - if ($is_new) { - return true; - } - - return parent::shouldHideForFeed(); - } - public function getRequiredHandlePHIDs() { $phids = parent::getRequiredHandlePHIDs(); diff --git a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php index 1c7a88dec7..8833e62b79 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskUnblockTransaction.php @@ -112,5 +112,15 @@ final class ManiphestTaskUnblockTransaction return 'fa-shield'; } + public function shouldHideForFeed() { + // Hide "alice created X, a task blocking Y." from feed because it + // will almost always appear adjacent to "alice created Y". + $is_new = $this->getMetadataValue('blocker.new'); + if ($is_new) { + return true; + } + + return parent::shouldHideForFeed(); + } } diff --git a/src/applications/phriction/storage/PhrictionTransaction.php b/src/applications/phriction/storage/PhrictionTransaction.php index 2ce9d38a08..860a86be35 100644 --- a/src/applications/phriction/storage/PhrictionTransaction.php +++ b/src/applications/phriction/storage/PhrictionTransaction.php @@ -54,18 +54,6 @@ final class PhrictionTransaction return parent::shouldHideForMail($xactions); } - public function shouldHideForFeed() { - switch ($this->getTransactionType()) { - case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE: - case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE: - return true; - case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE: - return $this->getMetadataValue('stub:create:phid', false); - } - return parent::shouldHideForFeed(); - } - - public function getMailTags() { $tags = array(); switch ($this->getTransactionType()) { diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php index c827f7337d..3cd45f73b7 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveAwayTransaction.php @@ -59,4 +59,8 @@ final class PhrictionDocumentMoveAwayTransaction return 'fa-arrows'; } + public function shouldHideForFeed() { + return true; + } + } diff --git a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php index a95e70ebdb..bdf2c5d8fc 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentMoveToTransaction.php @@ -102,4 +102,8 @@ final class PhrictionDocumentMoveToTransaction return 'fa-arrows'; } + public function shouldHideForFeed() { + return true; + } + } diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php index 6cc18d5181..305e38ab09 100644 --- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php +++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php @@ -6,6 +6,8 @@ class PhabricatorApplicationTransactionFeedStory extends PhabricatorFeedStory { + private $primaryTransactionPHID; + public function getPrimaryObjectPHID() { return $this->getValue('objectPHID'); } @@ -27,7 +29,36 @@ class PhabricatorApplicationTransactionFeedStory } protected function getPrimaryTransactionPHID() { - return head($this->getValue('transactionPHIDs')); + if ($this->primaryTransactionPHID === null) { + // Transactions are filtered and sorted before they're stored, but the + // rendering logic can change between the time an edit occurs and when + // we actually render the story. Recalculate the filtering at display + // time because it's cheap and gets us better results when things change + // by letting the changes apply retroactively. + + $xaction_phids = $this->getValue('transactionPHIDs'); + + $xactions = array(); + foreach ($xaction_phids as $xaction_phid) { + $xactions[] = $this->getObject($xaction_phid); + } + + foreach ($xactions as $key => $xaction) { + if ($xaction->shouldHideForFeed()) { + unset($xactions[$key]); + } + } + + if ($xactions) { + $primary_phid = head($xactions)->getPHID(); + } else { + $primary_phid = head($xaction_phids); + } + + $this->primaryTransactionPHID = $primary_phid; + } + + return $this->primaryTransactionPHID; } public function getPrimaryTransaction() { diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php index 9f3c24b946..fff2b842d1 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransaction.php +++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php @@ -92,6 +92,14 @@ abstract class PhabricatorModularTransaction return parent::shouldHide(); } + final public function shouldHideForFeed() { + if ($this->getTransactionImplementation()->shouldHideForFeed()) { + return true; + } + + return parent::shouldHideForFeed(); + } + /* final */ public function getIcon() { $icon = $this->getTransactionImplementation()->getIcon(); if ($icon !== null) { diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index f2b59c8a2b..d93831affc 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -47,6 +47,10 @@ abstract class PhabricatorModularTransactionType return false; } + public function shouldHideForFeed() { + return false; + } + public function getIcon() { return null; }