From 773b4eaa9ea0999fd86fb9d883b186d867f60a09 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Dec 2018 12:40:24 -0800 Subject: [PATCH] Separate "feed" and "notifications" better, allow stories to appear in notifications only Summary: Depends on D19861. Ref T13222. See PHI996. Fixes T10743. Currently, notifications only work if a story also has a feed rendering. Separate "visible in feed" and "visible in notifications", and make notifications query only notifications and vice versa. Then, set the test notification stories to be visible in notifications only, not feed. This could be refined a bit (there's no way to have the two views render different values today, for example) but since the only actual use case we have right now is test notifications I don't want to go //too// crazy future-proofing it. I could imagine doing some more of this kind of stuff in Conpherence eventually, though, perhaps. Test Plan: Sent myself test notifications, saw them appear on my profile timeline and in the JS popup, and in my notifications menu, but not in feed. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222, T10743 Differential Revision: https://secure.phabricator.com/D19864 --- .../feed/query/PhabricatorFeedQuery.php | 10 ++++- .../feed/story/PhabricatorFeedStory.php | 8 ++++ .../query/PhabricatorNotificationQuery.php | 20 ++++++--- .../PhabricatorUserNotifyTransaction.php | 6 ++- .../PhabricatorNotificationsSettingsPanel.php | 2 +- ...habricatorApplicationTransactionEditor.php | 14 +++++- ...ricatorApplicationTransactionFeedStory.php | 44 ++++++++++++++++--- .../PhabricatorApplicationTransaction.php | 4 ++ .../storage/PhabricatorModularTransaction.php | 11 +++++ .../PhabricatorModularTransactionType.php | 4 ++ 10 files changed, 107 insertions(+), 16 deletions(-) diff --git a/src/applications/feed/query/PhabricatorFeedQuery.php b/src/applications/feed/query/PhabricatorFeedQuery.php index ce472a6026..a35f14da57 100644 --- a/src/applications/feed/query/PhabricatorFeedQuery.php +++ b/src/applications/feed/query/PhabricatorFeedQuery.php @@ -34,7 +34,15 @@ final class PhabricatorFeedQuery } protected function willFilterPage(array $data) { - return PhabricatorFeedStory::loadAllFromRows($data, $this->getViewer()); + $stories = PhabricatorFeedStory::loadAllFromRows($data, $this->getViewer()); + + foreach ($stories as $key => $story) { + if (!$story->isVisibleInFeed()) { + unset($stories[$key]); + } + } + + return $stories; } protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php index c3984573c7..e0c65c7dc2 100644 --- a/src/applications/feed/story/PhabricatorFeedStory.php +++ b/src/applications/feed/story/PhabricatorFeedStory.php @@ -418,6 +418,14 @@ abstract class PhabricatorFeedStory return array(); } + public function isVisibleInFeed() { + return true; + } + + public function isVisibleInNotifications() { + return true; + } + /* -( PhabricatorPolicyInterface Implementation )-------------------------- */ diff --git a/src/applications/notification/query/PhabricatorNotificationQuery.php b/src/applications/notification/query/PhabricatorNotificationQuery.php index b40bdee066..0063c06e45 100644 --- a/src/applications/notification/query/PhabricatorNotificationQuery.php +++ b/src/applications/notification/query/PhabricatorNotificationQuery.php @@ -53,13 +53,13 @@ final class PhabricatorNotificationQuery $data = queryfx_all( $conn, - 'SELECT story.*, notif.hasViewed FROM %T notif - JOIN %T story ON notif.chronologicalKey = story.chronologicalKey + 'SELECT story.*, notif.hasViewed FROM %R notif + JOIN %R story ON notif.chronologicalKey = story.chronologicalKey %Q ORDER BY notif.chronologicalKey DESC %Q', - $notification_table->getTableName(), - $story_table->getTableName(), + $notification_table, + $story_table, $this->buildWhereClause($conn), $this->buildLimitClause($conn)); @@ -93,7 +93,7 @@ final class PhabricatorNotificationQuery (int)!$this->unread); } - if ($this->keys) { + if ($this->keys !== null) { $where[] = qsprintf( $conn, 'notif.chronologicalKey IN (%Ls)', @@ -103,6 +103,16 @@ final class PhabricatorNotificationQuery return $where; } + protected function willFilterPage(array $stories) { + foreach ($stories as $key => $story) { + if (!$story->isVisibleInNotifications()) { + unset($stories[$key]); + } + } + + return $stories; + } + protected function getResultCursor($item) { return $item->getChronologicalKey(); } diff --git a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php index bc5657d22e..4a527f8265 100644 --- a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php @@ -23,10 +23,14 @@ final class PhabricatorUserNotifyTransaction return $this->getNewValue(); } - public function shouldHideForFeed() { + public function shouldHideForNotifications() { return false; } + public function shouldHideForFeed() { + return true; + } + public function shouldHideForMail() { return true; } diff --git a/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php index e75c2b99ee..797bcafcb3 100644 --- a/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php @@ -143,7 +143,7 @@ final class PhabricatorNotificationsSettingsPanel ->setCaption( pht( 'Phabricator can send real-time notifications to your web browser '. - 'or to your desktop. Select where you\'d want to receive these '. + 'or to your desktop. Select where you want to receive these '. 'real-time updates.')) ->initBehavior( 'desktop-notifications-control', diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 9e946a249e..738fcf098b 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3428,7 +3428,19 @@ abstract class PhabricatorApplicationTransactionEditor array $xactions, array $mailed_phids) { - $xactions = mfilter($xactions, 'shouldHideForFeed', true); + // Remove transactions which don't publish feed stories or notifications. + // These never show up anywhere, so we don't need to do anything with them. + foreach ($xactions as $key => $xaction) { + if (!$xaction->shouldHideForFeed()) { + continue; + } + + if (!$xaction->shouldHideForNotifications()) { + continue; + } + + unset($xactions[$key]); + } if (!$xactions) { return; diff --git a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php index 305e38ab09..0e8531a22d 100644 --- a/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php +++ b/src/applications/transactions/feed/PhabricatorApplicationTransactionFeedStory.php @@ -36,12 +36,7 @@ class PhabricatorApplicationTransactionFeedStory // 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); - } + $xactions = $this->getTransactions(); foreach ($xactions as $key => $xaction) { if ($xaction->shouldHideForFeed()) { @@ -52,7 +47,7 @@ class PhabricatorApplicationTransactionFeedStory if ($xactions) { $primary_phid = head($xactions)->getPHID(); } else { - $primary_phid = head($xaction_phids); + $primary_phid = head($this->getValue('transactionPHIDs')); } $this->primaryTransactionPHID = $primary_phid; @@ -61,6 +56,41 @@ class PhabricatorApplicationTransactionFeedStory return $this->primaryTransactionPHID; } + public function isVisibleInNotifications() { + $xactions = $this->getTransactions(); + + foreach ($xactions as $key => $xaction) { + if (!$xaction->shouldHideForNotifications()) { + return true; + } + } + + return false; + } + + public function isVisibleInFeed() { + $xactions = $this->getTransactions(); + + foreach ($xactions as $key => $xaction) { + if (!$xaction->shouldHideForFeed()) { + return true; + } + } + + return false; + } + + private function getTransactions() { + $xaction_phids = $this->getValue('transactionPHIDs'); + + $xactions = array(); + foreach ($xaction_phids as $xaction_phid) { + $xactions[] = $this->getObject($xaction_phid); + } + + return $xactions; + } + public function getPrimaryTransaction() { return $this->getObject($this->getPrimaryTransactionPHID()); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 9e024b43aa..f38c56acb3 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -759,6 +759,10 @@ abstract class PhabricatorApplicationTransaction return $this->shouldHide(); } + public function shouldHideForNotifications() { + return $this->shouldHideForFeed(); + } + public function getTitleForMail() { return id(clone $this)->setRenderingTarget('text')->getTitle(); } diff --git a/src/applications/transactions/storage/PhabricatorModularTransaction.php b/src/applications/transactions/storage/PhabricatorModularTransaction.php index eef4b8e5d9..f6aece2a7f 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransaction.php +++ b/src/applications/transactions/storage/PhabricatorModularTransaction.php @@ -108,6 +108,17 @@ abstract class PhabricatorModularTransaction return parent::shouldHideForMail($xactions); } + final public function shouldHideForNotifications() { + $hide = $this->getTransactionImplementation()->shouldHideForNotifications(); + + // Returning "null" means "use the default behavior". + if ($hide === null) { + return parent::shouldHideForNotifications(); + } + + return $hide; + } + /* 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 ca82af1c66..35dd3ac197 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -59,6 +59,10 @@ abstract class PhabricatorModularTransactionType return false; } + public function shouldHideForNotifications() { + return null; + } + public function getIcon() { return null; }