From 700271500a1a3a0630c9467f7bb70414fdbe7d00 Mon Sep 17 00:00:00 2001 From: bsitu Date: Thu, 26 Sep 2013 16:25:46 -0700 Subject: [PATCH] Bundle message should show diff of all included revisions bug: 54391 Change-Id: I6c726d9d36e87fb5092b3c3e205e10ae0de557b4 --- formatters/BasicFormatter.php | 74 +++++++++++++++++++++++----- formatters/NotificationFormatter.php | 19 +++++++ formatters/PageLinkFormatter.php | 2 +- includes/DbEchoBackend.php | 27 ++++------ includes/EchoBackend.php | 5 +- 5 files changed, 95 insertions(+), 32 deletions(-) diff --git a/formatters/BasicFormatter.php b/formatters/BasicFormatter.php index 82d2a79d4..08e34f5ad 100644 --- a/formatters/BasicFormatter.php +++ b/formatters/BasicFormatter.php @@ -4,6 +4,9 @@ * @Todo - Consider having $event/$user as class properties since the formatter is * always tied to these two entities, in this case, we won't have to pass it around * in all the internal method + * @Todo - Instance variable $distributionType has been added, the local distribution + * type variable $type passed along all the protected/private method should be removed + * from all formatters */ class EchoBasicFormatter extends EchoNotificationFormatter { @@ -61,6 +64,14 @@ class EchoBasicFormatter extends EchoNotificationFormatter { 'raw-data-count' => 1 ); + /** + * Max number of raw bundle data to query for each bundle event + */ + protected static $maxRawBundleData = 250; + + /** + * @param array + */ public function __construct( $params ) { parent::__construct( $params ); @@ -141,7 +152,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { * * @param $event EchoEvent that the notification is for. * @param $user User to format the notification for. - * @param $type string The type of notification being distributed (e.g. email, web) + * @deprecated $type */ protected function applyChangeBeforeFormatting( EchoEvent $event, User $user, $type ) { // Use the bundle message if use-bundle is true and there is a bundle message @@ -162,6 +173,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { public function format( $event, $user, $type ) { global $wgExtensionAssetsPath, $wgEchoNotificationIcons; + $this->setDistributionType( $type ); $this->applyChangeBeforeFormatting( $event, $user, $type ); if ( $this->outputFormat === 'email' ) { @@ -245,7 +257,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { * * @param $event EchoEvent * @param $user User - * @param $type string + * @deprecated $type * @return array */ protected function formatEmail( $event, $user, $type ) { @@ -253,7 +265,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { $this->language = $user->getOption( 'language' ); // Email digest - if ( $type === 'emaildigest' ) { + if ( $this->distributionType === 'emaildigest' ) { return $this->formatEmailDigest( $event, $user ); } @@ -522,7 +534,7 @@ class EchoBasicFormatter extends EchoNotificationFormatter { * Get raw bundle data for an event so it can be manipulated * @param $event EchoEvent * @param $user User - * @param $type string Notification distribution type: web/email + * @deprecated $type * @return ResultWrapper|bool */ protected function getRawBundleData( $event, $user, $type ) { @@ -535,11 +547,11 @@ class EchoBasicFormatter extends EchoNotificationFormatter { return false; } - $data = $wgEchoBackend->getRawBundleData( $user, $event->getBundleHash(), $type ); + $data = $wgEchoBackend->getRawBundleData( $user, $event->getBundleHash(), $this->distributionType, 'DESC', self::$maxRawBundleData ); if ( $data ) { $this->bundleData['raw-data-count'] += $data->numRows(); - if ( $type !== 'web' ) { + if ( $this->distributionType !== 'web' ) { $this->bundleData['raw-data-count']--; } } @@ -553,13 +565,17 @@ class EchoBasicFormatter extends EchoNotificationFormatter { * this function to use a differnt group iterator such as title, namespace * @param $event EchoEvent * @param $user User - * @param $type string Notification distribution type + * @deprecated $type */ protected function generateBundleData( $event, $user, $type ) { global $wgEchoMaxNotificationCount; $data = $this->getRawBundleData( $event, $user, $type ); + // Default the last raw data to false, which means there is no + // bundle data other than the base + $this->bundleData['last-raw-data'] = false; + if ( !$data ) { return; } @@ -584,16 +600,19 @@ class EchoBasicFormatter extends EchoNotificationFormatter { $agents[$row->$key] = $row->$key; $count++; } - - if ( $count > $wgEchoMaxNotificationCount + 1 ) { - break; - } + $this->bundleData['last-raw-data'] = $row; } $this->bundleData['agent-other-count'] = $count - 1; if ( $count > 1 ) { $this->bundleData['use-bundle'] = true; } + + // If there is more raw data than we requested, that means we have not + // retrieved the very last raw record, set the key back to null + if ( $data->numRows() >= self::$maxRawBundleData ) { + $this->bundleData['last-raw-data'] = null; + } } /** @@ -702,9 +721,38 @@ class EchoBasicFormatter extends EchoNotificationFormatter { // other parameters $target->setFragment( '#' ); $query = array( - 'oldid' => $eventData['revid'], - 'diff' => 'prev', + 'oldid' => 'prev', + 'diff' => $eventData['revid'], ); + + if ( $event->getBundleHash() ) { + // First try cache data from preivous query + if ( isset( $this->bundleData['last-raw-data'] ) ) { + $stat = $this->bundleData['last-raw-data']; + // Then try to query the storage + } else { + global $wgEchoBackend; + $stat = $wgEchoBackend->getRawBundleData( $user, $event->getBundleHash(), $this->distributionType, 'ASC', 1 ); + if ( $stat ) { + $stat = $stat->current(); + } + } + + if ( $stat ) { + $extra = $stat->event_extra ? unserialize( $stat->event_extra ) : array(); + if ( isset( $extra['revid'] ) ) { + $oldId = $target->getPreviousRevisionID( $extra['revid'] ); + // The diff engine doesn't provide a way to diff against a null revision. + // In this case, just fall back old id to the first revision + if ( !$oldId ) { + $oldId = $extra['revid']; + } + if ( $oldId < $eventData['revid'] ) { + $query['oldid'] = $oldId; + } + } + } + } } break; } diff --git a/formatters/NotificationFormatter.php b/formatters/NotificationFormatter.php index c956f7c98..ecb4fded2 100644 --- a/formatters/NotificationFormatter.php +++ b/formatters/NotificationFormatter.php @@ -14,12 +14,23 @@ abstract class EchoNotificationFormatter { */ protected $validOutputFormats = array( 'text', 'flyout', 'html', 'email', 'htmlemail' ); + /** + * List of valid distribution type + */ + protected $validDistributionType = array( 'web', 'email', 'emaildigest' ); + /** * Current output format, default is 'text' * @var string */ protected $outputFormat = 'text'; + /** + * Distribution type, default is 'web' + * @var string + */ + protected $distributionType = 'web'; + /** * List of parameters for constructing messages * @var array @@ -73,6 +84,14 @@ abstract class EchoNotificationFormatter { $this->outputFormat = $format; } + public function setDistributionType( $type ) { + if ( !in_array( $type, $this->validDistributionType, true ) ) { + throw new Exception( "Invalid distribution type $type" ); + } + + $this->distributionType = $type; + } + /** * Create an EchoNotificationFormatter from the supplied parameters. * @param $parameters array Associative array. diff --git a/formatters/PageLinkFormatter.php b/formatters/PageLinkFormatter.php index da235181e..cdec6290e 100644 --- a/formatters/PageLinkFormatter.php +++ b/formatters/PageLinkFormatter.php @@ -37,7 +37,7 @@ class EchoPageLinkFormatter extends EchoBasicFormatter { * link from Page B and X other pages * @param $event EchoEvent * @param $user User - * @param $type string Notification disytribution type + * @deprecated $type */ protected function generateBundleData( $event, $user, $type ) { global $wgEchoMaxNotificationCount; diff --git a/includes/DbEchoBackend.php b/includes/DbEchoBackend.php index dc6420c87..d97eb854c 100644 --- a/includes/DbEchoBackend.php +++ b/includes/DbEchoBackend.php @@ -92,25 +92,22 @@ class MWDbEchoBackend extends MWEchoBackend { * @param $user User * @param $bundleHash string the bundle hash * @param $type string + * @param $order string 'ASC'/'DESC' + * @param $limit int * @return ResultWrapper|bool */ - public function getRawBundleData( $user, $bundleHash, $type = 'web' ) { + public function getRawBundleData( $user, $bundleHash, $type = 'web', $order = 'DESC', $limit = 250 ) { $dbr = MWEchoDbFactory::getDB( DB_SLAVE ); // We only display 99+ if the number is over 100, we can do limit 250, this should be sufficient - // to return 99 distinct group iterators, avoid select count( distinct ) for the folliwng: + // to return 99 distinct group iterators, avoid select count( distinct ) for the following reason: // 1. it will not scale for large volume data // 2. notification may have random grouping iterator - // 2. agent may be anonymous, can't do distinct over two columens: event_agent_id and event_agent_ip + // 3. agent may be anonymous, can't do distinct over two columns: event_agent_id and event_agent_ip if ( $type == 'web' ) { $res = $dbr->select( array( 'echo_notification', 'echo_event' ), - array( - 'event_agent_id', - 'event_agent_ip', - 'event_extra', - 'event_page_id' - ), + array( 'event_agent_id', 'event_agent_ip', 'event_extra', 'event_page_id' ), array( 'notification_event=event_id', 'notification_user' => $user->getId(), @@ -118,25 +115,20 @@ class MWDbEchoBackend extends MWEchoBackend { 'notification_bundle_display_hash' => $bundleHash ), __METHOD__, - array( 'ORDER BY' => 'notification_timestamp DESC', 'LIMIT' => 250 ) + array( 'ORDER BY' => 'notification_timestamp ' . $order, 'LIMIT' => $limit ) ); // this would be email for now } else { $res = $dbr->select( array( 'echo_email_batch', 'echo_event' ), - array( - 'event_agent_id', - 'event_agent_ip', - 'event_extra', - 'event_page_id' - ), + array( 'event_agent_id', 'event_agent_ip', 'event_extra', 'event_page_id' ), array( 'eeb_event_id=event_id', 'eeb_user_id' => $user->getId(), 'eeb_event_hash' => $bundleHash ), __METHOD__, - array( 'ORDER BY' => 'eeb_event_id DESC', 'LIMIT' => 250 ) + array( 'ORDER BY' => 'eeb_event_id ' . $order, 'LIMIT' => $limit ) ); } @@ -330,4 +322,5 @@ class MWDbEchoBackend extends MWEchoBackend { return $eventIds; } + } diff --git a/includes/EchoBackend.php b/includes/EchoBackend.php index 3030b1a52..2386b60bb 100644 --- a/includes/EchoBackend.php +++ b/includes/EchoBackend.php @@ -68,9 +68,11 @@ abstract class MWEchoBackend { * @param $user User * @param $bundleHash string The hash used to identify a set of bundle-able events * @param $type string 'web'/'email' + * @param $order 'ASC'/'DESC' Sort the result in ascending/descending order + * @param $limit int the number of records to retrieve * @return ResultWrapper|bool */ - abstract public function getRawBundleData( $user, $bundleHash, $type = 'web' ); + abstract public function getRawBundleData( $user, $bundleHash, $type = 'web', $order = 'DESC', $limit = 250 ); /** * Get the last bundle stat - read_timestamp & bundle_display_hash @@ -129,4 +131,5 @@ abstract class MWEchoBackend { * @return array */ abstract public function getUnreadNotifications( $user, $type ); + }