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

diff --git a/includes/libs/objectcache/BagOStuff.php b/includes/libs/objectcache/BagOStuff.php
index bace22b..2fb978d 100644 (file)
--- 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 d59b66b..c052935 100644 (file)
--- 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 274a35d..e092859 100644 (file)
--- 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 280ad90..6249c49 100644 (file)
--- 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 );
        }