From fc45398ba99a8383c01894902a8940063a23d4d0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 1 Jul 2012 11:08:59 -0700 Subject: [PATCH] Support basic notification aggregation Summary: This is both only partially complete (supports Maniphest only) and somewhat overcomplicated (includes support for applying similar algorithms to Feed), but provides runtime aggregation of notifications. Test Plan: {F13502} Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D2884 --- src/__phutil_library_map__.php | 4 + .../feed/story/PhabricatorFeedStory.php | 4 + .../story/PhabricatorFeedStoryAggregate.php | 74 ++++++++++++ .../story/PhabricatorFeedStoryManiphest.php | 15 +++ ...PhabricatorFeedStoryManiphestAggregate.php | 86 ++++++++++++++ .../PhabricatorNotificationBuilder.php | 109 +++++++++++++++++- 6 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 src/applications/feed/story/PhabricatorFeedStoryAggregate.php create mode 100644 src/applications/feed/story/PhabricatorFeedStoryManiphestAggregate.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9c46a33268..919899ca78 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -638,11 +638,13 @@ phutil_register_library_map(array( 'PhabricatorFeedPublicStreamController' => 'applications/feed/controller/PhabricatorFeedPublicStreamController.php', 'PhabricatorFeedQuery' => 'applications/feed/PhabricatorFeedQuery.php', 'PhabricatorFeedStory' => 'applications/feed/story/PhabricatorFeedStory.php', + 'PhabricatorFeedStoryAggregate' => 'applications/feed/story/PhabricatorFeedStoryAggregate.php', 'PhabricatorFeedStoryAudit' => 'applications/feed/story/PhabricatorFeedStoryAudit.php', 'PhabricatorFeedStoryCommit' => 'applications/feed/story/PhabricatorFeedStoryCommit.php', 'PhabricatorFeedStoryData' => 'applications/feed/storage/PhabricatorFeedStoryData.php', 'PhabricatorFeedStoryDifferential' => 'applications/feed/story/PhabricatorFeedStoryDifferential.php', 'PhabricatorFeedStoryManiphest' => 'applications/feed/story/PhabricatorFeedStoryManiphest.php', + 'PhabricatorFeedStoryManiphestAggregate' => 'applications/feed/story/PhabricatorFeedStoryManiphestAggregate.php', 'PhabricatorFeedStoryNotification' => 'applications/notification/storage/PhabricatorFeedStoryNotification.php', 'PhabricatorFeedStoryPhriction' => 'applications/feed/story/PhabricatorFeedStoryPhriction.php', 'PhabricatorFeedStoryProject' => 'applications/feed/story/PhabricatorFeedStoryProject.php', @@ -1646,11 +1648,13 @@ phutil_register_library_map(array( 'PhabricatorFeedController' => 'PhabricatorController', 'PhabricatorFeedDAO' => 'PhabricatorLiskDAO', 'PhabricatorFeedPublicStreamController' => 'PhabricatorFeedController', + 'PhabricatorFeedStoryAggregate' => 'PhabricatorFeedStory', 'PhabricatorFeedStoryAudit' => 'PhabricatorFeedStory', 'PhabricatorFeedStoryCommit' => 'PhabricatorFeedStory', 'PhabricatorFeedStoryData' => 'PhabricatorFeedDAO', 'PhabricatorFeedStoryDifferential' => 'PhabricatorFeedStory', 'PhabricatorFeedStoryManiphest' => 'PhabricatorFeedStory', + 'PhabricatorFeedStoryManiphestAggregate' => 'PhabricatorFeedStoryAggregate', 'PhabricatorFeedStoryNotification' => 'PhabricatorFeedDAO', 'PhabricatorFeedStoryPhriction' => 'PhabricatorFeedStory', 'PhabricatorFeedStoryProject' => 'PhabricatorFeedStory', diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php index 672e41eeae..d9ae34d3d1 100644 --- a/src/applications/feed/story/PhabricatorFeedStory.php +++ b/src/applications/feed/story/PhabricatorFeedStory.php @@ -139,4 +139,8 @@ abstract class PhabricatorFeedStory { return $text; } + public function getNotificationAggregations() { + return array(); + } + } diff --git a/src/applications/feed/story/PhabricatorFeedStoryAggregate.php b/src/applications/feed/story/PhabricatorFeedStoryAggregate.php new file mode 100644 index 0000000000..3eaf0a2994 --- /dev/null +++ b/src/applications/feed/story/PhabricatorFeedStoryAggregate.php @@ -0,0 +1,74 @@ +getAggregateStories())->getHasViewed(); + } + + public function getRequiredHandlePHIDs() { + $phids = array(); + foreach ($this->getAggregateStories() as $story) { + $phids[] = $story->getRequiredHandlePHIDs(); + } + return array_mergev($phids); + } + + public function getRequiredObjectPHIDs() { + $phids = array(); + foreach ($this->getAggregateStories() as $story) { + $phids[] = $story->getRequiredObjectPHIDs(); + } + return array_mergev($phids); + } + + protected function getAuthorPHIDs() { + $authors = array(); + foreach ($this->getAggregateStories() as $story) { + $authors[] = $story->getStoryData()->getAuthorPHID(); + } + return array_unique(array_filter($authors)); + } + + protected function getDataValues($key, $default) { + $result = array(); + foreach ($this->getAggregateStories() as $key => $story) { + $result[$key] = $story->getStoryData()->getValue($key, $default); + } + return $result; + } + + final public function setAggregateStories(array $aggregate_stories) { + assert_instances_of($aggregate_stories, 'PhabricatorFeedStory'); + $this->aggregateStories = $aggregate_stories; + return $this; + } + + final public function getAggregateStories() { + return $this->aggregateStories; + } + + final public function getNotificationAggregations() { + throw new Exception( + "You can not get aggregations for an aggregate story."); + } + +} diff --git a/src/applications/feed/story/PhabricatorFeedStoryManiphest.php b/src/applications/feed/story/PhabricatorFeedStoryManiphest.php index 63ff1ef831..7648fbce6c 100644 --- a/src/applications/feed/story/PhabricatorFeedStoryManiphest.php +++ b/src/applications/feed/story/PhabricatorFeedStoryManiphest.php @@ -111,4 +111,19 @@ final class PhabricatorFeedStoryManiphest return $one_line; } + + public function getNotificationAggregations() { + $class = get_class($this); + $phid = $this->getStoryData()->getValue('taskPHID'); + $read = (int)$this->getHasViewed(); + + // Don't aggregate updates separated by more than 2 hours. + $block = (int)($this->getEpoch() / (60 * 60 * 2)); + + return array( + "{$class}:{$phid}:{$read}:{$block}" + => 'PhabricatorFeedStoryManiphestAggregate', + ); + } + } diff --git a/src/applications/feed/story/PhabricatorFeedStoryManiphestAggregate.php b/src/applications/feed/story/PhabricatorFeedStoryManiphestAggregate.php new file mode 100644 index 0000000000..2d81f260fa --- /dev/null +++ b/src/applications/feed/story/PhabricatorFeedStoryManiphestAggregate.php @@ -0,0 +1,86 @@ +getStoryData(); + + $task_link = $this->linkTo($data->getValue('taskPHID')); + + $authors = $this->getAuthorPHIDs(); + + // TODO: These aren't really translatable because linkTo() returns a + // string, not an object with a gender. + + switch (count($authors)) { + case 1: + $author = $this->linkTo(array_shift($authors)); + $title = pht( + '%s made multiple updates to %s', + $author, + $task_link); + break; + case 2: + $author1 = $this->linkTo(array_shift($authors)); + $author2 = $this->linkTo(array_shift($authors)); + $title = pht( + '%s and %s made multiple updates to %s', + $author1, + $author2, + $task_link); + break; + case 3: + $author1 = $this->linkTo(array_shift($authors)); + $author2 = $this->linkTo(array_shift($authors)); + $author3 = $this->linkTo(array_shift($authors)); + $title = pht( + '%s, %s, and %s made multiple updates to %s', + $author1, + $author2, + $author3, + $task_link); + break; + default: + $author1 = $this->linkTo(array_shift($authors)); + $author2 = $this->linkTo(array_shift($authors)); + $others = count($authors); + $title = pht( + '%s, %s, and %d others made multiple updates to %s', + $author1, + $author2, + $others, + $task_link); + break; + } + + $view = new PhabricatorNotificationStoryView(); + $view->setEpoch($this->getEpoch()); + $view->setViewed($this->getHasViewed()); + $view->setTitle($title); + + return $view; + } + +} diff --git a/src/applications/notification/builder/PhabricatorNotificationBuilder.php b/src/applications/notification/builder/PhabricatorNotificationBuilder.php index 22c8c2462c..9f5dc3363e 100644 --- a/src/applications/notification/builder/PhabricatorNotificationBuilder.php +++ b/src/applications/notification/builder/PhabricatorNotificationBuilder.php @@ -27,6 +27,114 @@ final class PhabricatorNotificationBuilder { public function buildView() { $stories = $this->stories; + $stories = mpull($stories, null, 'getChronologicalKey'); + + // Aggregate notifications. Generally, we can aggregate notifications only + // by object, e.g. "a updated T123" and "b updated T123" can become + // "a and b updated T123", but we can't combine "a updated T123" and + // "a updated T234" into "a updated T123 and T234" because there would be + // nowhere sensible for the notification to link to, and no reasonable way + // to unambiguously clear it. + + // Each notification emits keys it can aggregate on. For instance, if this + // story is "a updated T123", it might emit a key like this: + // + // task:phid123:unread => PhabricatorFeedStoryManiphestAggregate + // + // All the unread notifications about the task with PHID "phid123" will + // emit the same key, telling us we can aggregate them into a single + // story of type "PhabricatorFeedStoryManiphestAggregate", which could + // read like "a and b updated T123". + // + // A story might be able to aggregate in multiple ways. Although this is + // unlikely for stories in a notification context, stories in a feed context + // can also aggregate by actor: + // + // task:phid123 => PhabricatorFeedStoryManiphestAggregate + // actor:user123 => PhabricatorFeedStoryActorAggregate + // + // This means the story can either become "a and b updated T123" or + // "a updated T123 and T456". When faced with multiple possibilities, it's + // our job to choose the best aggregation. + // + // For now, we use a simple greedy algorithm and repeatedly select the + // aggregate story which consumes the largest number of individual stories + // until no aggregate story exists that consumes more than one story. + + + // Build up a map of all the possible aggregations. + + $chronokey_map = array(); + $aggregation_map = array(); + $agg_types = array(); + foreach ($stories as $chronokey => $story) { + $chronokey_map[$chronokey] = $story->getNotificationAggregations(); + foreach ($chronokey_map[$chronokey] as $key => $type) { + $agg_types[$key] = $type; + $aggregation_map[$key]['keys'][$chronokey] = true; + } + } + + // Repeatedly select the largest available aggregation until none remain. + + $aggregated_stories = array(); + while ($aggregation_map) { + + // Count the size of each aggregation, removing any which will consume + // fewer than 2 stories. + + foreach ($aggregation_map as $key => $dict) { + $size = count($dict['keys']); + if ($size > 1) { + $aggregation_map[$key]['size'] = $size; + } else { + unset($aggregation_map[$key]); + } + } + + // If we're out of aggregations, break out. + + if (!$aggregation_map) { + break; + } + + // Select the aggregation we're going to make, and remove it from the + // map. + + $aggregation_map = isort($aggregation_map, 'size'); + $agg_info = idx(last($aggregation_map), 'keys'); + $agg_key = last_key($aggregation_map); + unset($aggregation_map[$agg_key]); + + // Select all the stories it aggregates, and remove them from the master + // list of stories and from all other possible aggregations. + + $sub_stories = array(); + foreach ($agg_info as $chronokey => $ignored) { + $sub_stories[$chronokey] = $stories[$chronokey]; + unset($stories[$chronokey]); + foreach ($chronokey_map[$chronokey] as $key => $type) { + unset($aggregation_map[$key]['keys'][$chronokey]); + } + unset($chronokey_map[$chronokey]); + } + + // Build the aggregate story. + + krsort($sub_stories); + $story_class = $agg_types[$agg_key]; + $conv = array(head($sub_stories)->getStoryData()); + + $new_story = newv($story_class, $conv); + $new_story->setAggregateStories($sub_stories); + $aggregated_stories[] = $new_story; + } + + // Combine the aggregate stories back into the list of stories. + + $stories = array_merge($stories, $aggregated_stories); + $stories = mpull($stories, null, 'getChronologicalKey'); + krsort($stories); $handles = array(); $objects = array(); @@ -39,7 +147,6 @@ final class PhabricatorNotificationBuilder { $null_view = new AphrontNullView(); - //TODO ADD NOTIFICATIONS HEADER foreach ($stories as $story) { $story->setHandles($handles); $view = $story->renderNotificationView();