From 588a4646825236502270b08ff05c53434abbaebc Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 14 Mar 2019 12:50:52 -0700 Subject: [PATCH] Fix WatchedItemStore last-seen stashing logic This should be the "last revision seen" timestamp, which is different than the "first revision not seen" timestamp in the DB. Also make sure that SpecialWatchlist accounts for the stash values. Relatedly, better document the callback usage in BagOStuff::merge(). Change-Id: I98b03a5cd40fec5b4a2633d499ff77079d264e3c --- includes/libs/objectcache/BagOStuff.php | 1 + includes/specials/SpecialWatchlist.php | 12 +++++++-- includes/watcheditem/WatchedItemStore.php | 27 +++++++++++++++---- .../watcheditem/WatchedItemStoreUnitTest.php | 6 ++--- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php index bace22b908..2fb978d6a6 100644 --- a/includes/libs/objectcache/BagOStuff.php +++ b/includes/libs/objectcache/BagOStuff.php @@ -267,6 +267,7 @@ abstract class BagOStuff implements IExpiringStore, LoggerAwareInterface { * (which will be false if not present), and takes the arguments: * (this BagOStuff, cache key, current value, TTL). * The TTL parameter is reference set to $exptime. It can be overriden in the callback. + * If the callback returns false, then the current value will be unchanged (including TTL). * * @param string $key * @param callable $callback Callback method to be executed diff --git a/includes/specials/SpecialWatchlist.php b/includes/specials/SpecialWatchlist.php index d59b66b9ef..c052935eb4 100644 --- a/includes/specials/SpecialWatchlist.php +++ b/includes/specials/SpecialWatchlist.php @@ -37,12 +37,16 @@ class SpecialWatchlist extends ChangesListSpecialPage { protected static $limitPreferenceName = 'wllimit'; protected static $collapsedPreferenceName = 'rcfilters-wl-collapsed'; + /** @var float|int */ private $maxDays; + /** WatchedItemStore */ + private $watchStore; public function __construct( $page = 'Watchlist', $restriction = 'viewmywatchlist' ) { parent::__construct( $page, $restriction ); $this->maxDays = $this->getConfig()->get( 'RCMaxAge' ) / ( 3600 * 24 ); + $this->watchStore = MediaWikiServices::getInstance()->getWatchedItemStore(); } public function doesWrites() { @@ -186,9 +190,13 @@ class SpecialWatchlist extends ChangesListSpecialPage { 'label' => 'rcfilters-filter-watchlistactivity-unseen-label', 'description' => 'rcfilters-filter-watchlistactivity-unseen-description', 'cssClassSuffix' => 'watchedunseen', - 'isRowApplicableCallable' => function ( $ctx, $rc ) { + 'isRowApplicableCallable' => function ( $ctx, RecentChange $rc ) { $changeTs = $rc->getAttribute( 'rc_timestamp' ); - $lastVisitTs = $rc->getAttribute( 'wl_notificationtimestamp' ); + $lastVisitTs = $this->watchStore->getLatestNotificationTimestamp( + $rc->getAttribute( 'wl_notificationtimestamp' ), + $rc->getPerformer(), + $rc->getTitle() + ); return $lastVisitTs !== null && $changeTs >= $lastVisitTs; }, ], diff --git a/includes/watcheditem/WatchedItemStore.php b/includes/watcheditem/WatchedItemStore.php index 274a35dda0..e092859089 100644 --- a/includes/watcheditem/WatchedItemStore.php +++ b/includes/watcheditem/WatchedItemStore.php @@ -966,14 +966,31 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac } } + // Get the timestamp (TS_MW) of this revision to track the latest one seen + $seenTime = call_user_func( + $this->revisionGetTimestampFromIdCallback, + $title, + $oldid ?: $title->getLatestRevID() + ); + // Mark the item as read immediately in lightweight storage $this->stash->merge( $this->getPageSeenTimestampsKey( $user ), - function ( $cache, $key, $current ) use ( $time, $title ) { + function ( $cache, $key, $current ) use ( $title, $seenTime ) { $value = $current ?: new MapCacheLRU( 300 ); - $value->set( $this->getPageSeenKey( $title ), wfTimestamp( TS_MW, $time ) ); - - $this->latestUpdateCache->set( $key, $value, IExpiringStore::TTL_PROC_LONG ); + $subKey = $this->getPageSeenKey( $title ); + + if ( $seenTime > $value->get( $subKey ) ) { + // Revision is newer than the last one seen + $value->set( $subKey, $seenTime ); + $this->latestUpdateCache->set( $key, $value, IExpiringStore::TTL_PROC_LONG ); + } elseif ( $seenTime === false ) { + // Revision does not exist + $value->set( $subKey, wfTimestamp( TS_MW ) ); + $this->latestUpdateCache->set( $key, $value, IExpiringStore::TTL_PROC_LONG ); + } else { + return false; // nothing to update + } return $value; }, @@ -1000,7 +1017,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac /** * @param User $user - * @return MapCacheLRU|null + * @return MapCacheLRU|null The map contains prefixed title keys and TS_MW values */ private function getPageSeenTimestamps( User $user ) { $key = $this->getPageSeenTimestampsKey( $user ); diff --git a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php index 280ad907f3..6249c49961 100644 --- a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php @@ -2388,7 +2388,7 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { $oldid ) ); - $this->assertEquals( 1, $getTimestampCallCounter ); + $this->assertEquals( 2, $getTimestampCallCounter ); ScopedCallback::consume( $scopedOverrideRevision ); } @@ -2530,7 +2530,7 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { $oldid ) ); - $this->assertEquals( 1, $getTimestampCallCounter ); + $this->assertEquals( 2, $getTimestampCallCounter ); ScopedCallback::consume( $scopedOverrideRevision ); } @@ -2609,7 +2609,7 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase { $oldid ) ); - $this->assertEquals( 1, $getTimestampCallCounter ); + $this->assertEquals( 2, $getTimestampCallCounter ); ScopedCallback::consume( $scopedOverrideRevision ); } -- 2.20.1