From ba833805656e724aea17463618874e75bcd8612f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Dec 2018 11:32:13 -0800 Subject: [PATCH] Update the "Notification Test" workflow to use more modern mechanisms Summary: Depends on D19860. Ref T13222. Ref T10743. See PHI996. Long ago, there were different types of feed stories. Over time, there was less and less need for this, and nowadays basically everything is a "transaction" feed story. Each story renders differently, but they're fundamentally all about transactions. The Notification test controller still uses a custom type of feed story to send notifications. Move away from this, and apply a transaction against the user instead. This has the same ultimate effect, but involves less weird custom code from ages long forgotten. This doesn't fix the actual problem with these things showing up in feed. Currently, stories always use the same rendering for feed and notifications, and there need to be some additional changes to fix this. So no behavioral change yet, just slightly more reasonable code. Test Plan: Clicked the button and got some test notifications, with Aphlict running. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222, T10743 Differential Revision: https://secure.phabricator.com/D19861 --- src/__phutil_library_map__.php | 2 + .../PhabricatorNotificationTestController.php | 51 +++++++++---------- ...abricatorPeopleProfileManageController.php | 6 +++ .../PhabricatorUserTransactionEditor.php | 14 +++++ .../PhabricatorUserDisableTransaction.php | 17 ++----- .../PhabricatorUserNotifyTransaction.php | 34 +++++++++++++ ...habricatorApplicationTransactionEditor.php | 25 +++++++-- .../PhabricatorApplicationTransaction.php | 9 ++++ 8 files changed, 115 insertions(+), 43 deletions(-) create mode 100644 src/applications/people/xaction/PhabricatorUserNotifyTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 184c2a3d7c..e2586354e8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4623,6 +4623,7 @@ phutil_register_library_map(array( 'PhabricatorUserLogView' => 'applications/people/view/PhabricatorUserLogView.php', 'PhabricatorUserMessageCountCacheType' => 'applications/people/cache/PhabricatorUserMessageCountCacheType.php', 'PhabricatorUserNotificationCountCacheType' => 'applications/people/cache/PhabricatorUserNotificationCountCacheType.php', + 'PhabricatorUserNotifyTransaction' => 'applications/people/xaction/PhabricatorUserNotifyTransaction.php', 'PhabricatorUserPHIDResolver' => 'applications/phid/resolver/PhabricatorUserPHIDResolver.php', 'PhabricatorUserPreferences' => 'applications/settings/storage/PhabricatorUserPreferences.php', 'PhabricatorUserPreferencesCacheType' => 'applications/people/cache/PhabricatorUserPreferencesCacheType.php', @@ -10676,6 +10677,7 @@ phutil_register_library_map(array( 'PhabricatorUserLogView' => 'AphrontView', 'PhabricatorUserMessageCountCacheType' => 'PhabricatorUserCacheType', 'PhabricatorUserNotificationCountCacheType' => 'PhabricatorUserCacheType', + 'PhabricatorUserNotifyTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUserPHIDResolver' => 'PhabricatorPHIDResolver', 'PhabricatorUserPreferences' => array( 'PhabricatorUserDAO', diff --git a/src/applications/notification/controller/PhabricatorNotificationTestController.php b/src/applications/notification/controller/PhabricatorNotificationTestController.php index 5c76e53357..f49f9d31a1 100644 --- a/src/applications/notification/controller/PhabricatorNotificationTestController.php +++ b/src/applications/notification/controller/PhabricatorNotificationTestController.php @@ -6,34 +6,31 @@ final class PhabricatorNotificationTestController public function handleRequest(AphrontRequest $request) { $viewer = $request->getViewer(); - $story_type = 'PhabricatorNotificationTestFeedStory'; - $story_data = array( - 'title' => pht( - 'This is a test notification, sent at %s.', - phabricator_datetime(time(), $viewer)), - ); - - $viewer_phid = $viewer->getPHID(); - - // NOTE: Because we don't currently show you your own notifications, make - // sure this comes from a different PHID. - $application_phid = id(new PhabricatorNotificationsApplication()) - ->getPHID(); - - // TODO: When it's easier to get these buttons to render as forms, this - // would be slightly nicer as a more standard isFormPost() check. - if ($request->validateCSRF()) { - id(new PhabricatorFeedStoryPublisher()) - ->setStoryType($story_type) - ->setStoryData($story_data) - ->setStoryTime(time()) - ->setStoryAuthorPHID($application_phid) - ->setRelatedPHIDs(array($viewer_phid)) - ->setPrimaryObjectPHID($viewer_phid) - ->setSubscribedPHIDs(array($viewer_phid)) - ->setNotifyAuthor(true) - ->publish(); + $message_text = pht( + 'This is a test notification, sent at %s.', + phabricator_datetime(time(), $viewer)); + + // NOTE: Currently, the FeedStoryPublisher explicitly filters out + // notifications about your own actions. Send this notification from + // a different actor to get around this. + $application_phid = id(new PhabricatorNotificationsApplication()) + ->getPHID(); + + $xactions = array(); + + $xactions[] = id(new PhabricatorUserTransaction()) + ->setTransactionType( + PhabricatorUserNotifyTransaction::TRANSACTIONTYPE) + ->setNewValue($message_text) + ->setForceNotifyPHIDs(array($viewer->getPHID())); + + $editor = id(new PhabricatorUserTransactionEditor()) + ->setActor($viewer) + ->setActingAsPHID($application_phid) + ->setContentSourceFromRequest($request); + + $editor->applyTransactions($viewer, $xactions); } return id(new AphrontAjaxResponse()); diff --git a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php index 9759a375c7..55f9311ada 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileManageController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileManageController.php @@ -43,6 +43,11 @@ final class PhabricatorPeopleProfileManageController ->setCurtain($curtain) ->addPropertySection(pht('Details'), $properties); + $timeline = $this->buildTransactionTimeline( + $user, + new PhabricatorPeopleTransactionQuery()); + $timeline->setShouldTerminate(true); + return $this->newPage() ->setTitle( array( @@ -54,6 +59,7 @@ final class PhabricatorPeopleProfileManageController ->appendChild( array( $manage, + $timeline, )); } diff --git a/src/applications/people/editor/PhabricatorUserTransactionEditor.php b/src/applications/people/editor/PhabricatorUserTransactionEditor.php index c0dfa941af..929b2e224c 100644 --- a/src/applications/people/editor/PhabricatorUserTransactionEditor.php +++ b/src/applications/people/editor/PhabricatorUserTransactionEditor.php @@ -11,4 +11,18 @@ final class PhabricatorUserTransactionEditor return pht('Users'); } + protected function shouldPublishFeedStory( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + + protected function getMailTo(PhabricatorLiskDAO $object) { + return array(); + } + + protected function getMailCC(PhabricatorLiskDAO $object) { + return array(); + } + } diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php index ab399a28e0..629d538dee 100644 --- a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php @@ -35,19 +35,10 @@ final class PhabricatorUserDisableTransaction } } - public function getTitleForFeed() { - $new = $this->getNewValue(); - if ($new) { - return pht( - '%s disabled %s.', - $this->renderAuthor(), - $this->renderObject()); - } else { - return pht( - '%s enabled %s.', - $this->renderAuthor(), - $this->renderObject()); - } + public function shouldHideForFeed() { + // Don't publish feed stories about disabling users, since this can be + // a sensitive action. + return true; } public function validateTransactions($object, array $xactions) { diff --git a/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php new file mode 100644 index 0000000000..bc5657d22e --- /dev/null +++ b/src/applications/people/xaction/PhabricatorUserNotifyTransaction.php @@ -0,0 +1,34 @@ +renderAuthor()); + } + + public function getTitleForFeed() { + return $this->getNewValue(); + } + + public function shouldHideForFeed() { + return false; + } + + public function shouldHideForMail() { + return true; + } + +} diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 378b96adb4..9e946a249e 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -3378,9 +3378,28 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorLiskDAO $object, array $xactions) { - return array_unique(array_merge( - $this->getMailTo($object), - $this->getMailCC($object))); + // If some transactions are forcing notification delivery, add the forced + // recipients to the notify list. + $force_list = array(); + foreach ($xactions as $xaction) { + $force_phids = $xaction->getForceNotifyPHIDs(); + + if (!$force_phids) { + continue; + } + + foreach ($force_phids as $force_phid) { + $force_list[] = $force_phid; + } + } + + $to_list = $this->getMailTo($object); + $cc_list = $this->getMailCC($object); + + $full_list = array_merge($force_list, $to_list, $cc_list); + $full_list = array_fuse($full_list); + + return array_keys($full_list); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 5fc3385d27..9e024b43aa 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -1662,6 +1662,15 @@ abstract class PhabricatorApplicationTransaction return null; } + public function setForceNotifyPHIDs(array $phids) { + $this->setMetadataValue('notify.force', $phids); + return $this; + } + + public function getForceNotifyPHIDs() { + return $this->getMetadataValue('notify.force', array()); + } + /* -( PhabricatorDestructibleInterface )----------------------------------- */