Correctly update wl_notificationtimestamp when viewing old revisions
authorBartosz Dziewoński <matma.rex@gmail.com>
Mon, 16 Sep 2013 10:31:40 +0000 (12:31 +0200)
committerBrian Wolff <bawolff+wn@gmail.com>
Sun, 27 Oct 2013 17:47:53 +0000 (17:47 +0000)
== Prelude ==
wl_notificationtimestamp controls sending the user e-mail
notifications about changes to pages, as well as showing the "updated
since last visit" markers on history pages, recent changes and
watchlist.

== The bug ==
Previously, on every view of a page, the notification timestamp was
cleared, regardless of whether the user as actually viewing the latest
revision. When viewing a diff, however, the timestamp was cleared only
if one of the revisions being compared was the latest one of its page.

The same behavior applied to talk page message indicators (which are
actually stored sepately to cater to anonymous users).

This was inconsistent and surprising when one was attempting to, say,
go through the 50 new posts to a discussion page in a peacemeal
fashion.

== The fix ==
If the revision being viewed is the latest (or can't be determined),
the timestamp is cleared as previously, as this is necessary to
reenable e-mail notifications for given user and page.

If the revision isn't the latest, the timestamp is updated to
revision's timestamp plus one second. This uses up to two simple
(selectField) indexed queries per page view, only fired when we
do not already know we're looking at the latest version.

Talk page indicator is updated to point at the next revision after the
one being viewed, or cleared if viewing the latest revision. The
UserClearNewTalkNotification hook gained $oldid as the second argument
(a backwards-compatible change). In Skin, we no longer ignore the
indicator being present if we're viewing the talk page, as it might
still be valid.

== The bonus ==
Comments and formatting was updated in a few places, including
tables.sql and Wiki.php.

The following functions gained a second, optional $oldid parameter
(holy indirection, Batman!):
* WikiPage#doViewUpdates()
* User#clearNotification()
* WatchedItem#resetNotificationTimestamp()

DifferenceEngine gained a public method mapDiffPrevNext() used
to parse the ids from URL parameters like oldid=12345&diff=prev,
factored out of loadRevisionIds(). A bug where the NewDifferenceEngine
hook would not be called in some cases, dating back to its
introduction in r45518, was fixed in the process.

Bug: 41759
Change-Id: I4144ba1987b8d7a7e8b24f4f067eedac2ae44459

RELEASE-NOTES-1.23
docs/hooks.txt
includes/Article.php
includes/ImagePage.php
includes/Skin.php
includes/User.php
includes/WatchedItem.php
includes/Wiki.php
includes/WikiPage.php
includes/diff/DifferenceEngine.php
maintenance/tables.sql

index 372170a..ec7b898 100644 (file)
@@ -13,6 +13,10 @@ production.
 === New features in 1.23 ===
 
 === Bug fixes in 1.23 ===
+* (bug 41759) The "updated since last visit" markers (on history pages, recent
+  changes and watchlist) and the talk page message indicator are now correctly
+  updated when the user is viewing old revisions of pages, instead of always
+  acting as if the latest revision was being viewed.
 
 === API changes in 1.23 ===
 
index 5aaf596..2671dd8 100644 (file)
@@ -2579,6 +2579,7 @@ $user: User (object) whose permission is being checked
 'UserClearNewTalkNotification': Called when clearing the "You have new
 messages!" message, return false to not delete it.
 $user: User (object) that will clear the message
+$oldid: ID of the talk page revision being viewed (0 means the most recent one)
 
 'UserComparePasswords': Called when checking passwords, return false to
 override the default password checks.
index 0b18221..928fda0 100644 (file)
@@ -586,7 +586,7 @@ class Article implements Page {
                                wfDebug( __METHOD__ . ": done file cache\n" );
                                # tell wgOut that output is taken care of
                                $outputPage->disable();
-                               $this->mPage->doViewUpdates( $user );
+                               $this->mPage->doViewUpdates( $user, $oldid );
                                wfProfileOut( __METHOD__ );
 
                                return;
@@ -765,7 +765,7 @@ class Article implements Page {
                $outputPage->setFollowPolicy( $policy['follow'] );
 
                $this->showViewFooter();
-               $this->mPage->doViewUpdates( $user );
+               $this->mPage->doViewUpdates( $user, $oldid );
 
                $outputPage->addModules( 'mediawiki.action.view.postEdit' );
 
@@ -815,10 +815,10 @@ class Article implements Page {
                $this->mRevIdFetched = $de->mNewid;
                $de->showDiffPage( $diffOnly );
 
-               if ( $diff == 0 || $diff == $this->mPage->getLatest() ) {
-                       # Run view updates for current revision only
-                       $this->mPage->doViewUpdates( $user );
-               }
+               // Run view updates for the newer revision being diffed (and shown below the diff if not $diffOnly)
+               list( $old, $new ) = $de->mapDiffPrevNext( $oldid, $diff );
+               // New can be false, convert it to 0 - this conveniently means the latest revision
+               $this->mPage->doViewUpdates( $user, (int)$new );
        }
 
        /**
index 515f146..cf05ee2 100644 (file)
@@ -128,7 +128,7 @@ class ImagePage extends Article {
                                $out->setPageTitle( $this->getTitle()->getPrefixedText() );
                                $out->addHTML( $this->viewRedirect( Title::makeTitle( NS_FILE, $this->mPage->getFile()->getName() ),
                                        /* $appendSubtitle */ true, /* $forceKnown */ true ) );
-                               $this->mPage->doViewUpdates( $this->getContext()->getUser() );
+                               $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() );
                                return;
                        }
                }
@@ -165,7 +165,7 @@ class ImagePage extends Article {
                        # Just need to set the right headers
                        $out->setArticleFlag( true );
                        $out->setPageTitle( $this->getTitle()->getPrefixedText() );
-                       $this->mPage->doViewUpdates( $this->getContext()->getUser() );
+                       $this->mPage->doViewUpdates( $this->getContext()->getUser(), $this->getOldID() );
                }
 
                # Show shared description, if needed
index 5801806..170e96f 100644 (file)
@@ -1379,61 +1379,58 @@ abstract class Skin extends ContextSource {
 
                if ( count( $newtalks ) == 1 && $newtalks[0]['wiki'] === wfWikiID() ) {
                        $uTalkTitle = $user->getTalkPage();
-
-                       if ( !$uTalkTitle->equals( $out->getTitle() ) ) {
-                               $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null;
-                               $nofAuthors = 0;
-                               if ( $lastSeenRev !== null ) {
-                                       $plural = true; // Default if we have a last seen revision: if unknown, use plural
-                                       $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL );
-                                       if ( $latestRev !== null ) {
-                                               // Singular if only 1 unseen revision, plural if several unseen revisions.
-                                               $plural = $latestRev->getParentId() !== $lastSeenRev->getId();
-                                               $nofAuthors = $uTalkTitle->countAuthorsBetween(
-                                                       $lastSeenRev, $latestRev, 10, 'include_new' );
-                                       }
-                               } else {
-                                       // Singular if no revision -> diff link will show latest change only in any case
-                                       $plural = false;
+                       $lastSeenRev = isset( $newtalks[0]['rev'] ) ? $newtalks[0]['rev'] : null;
+                       $nofAuthors = 0;
+                       if ( $lastSeenRev !== null ) {
+                               $plural = true; // Default if we have a last seen revision: if unknown, use plural
+                               $latestRev = Revision::newFromTitle( $uTalkTitle, false, Revision::READ_NORMAL );
+                               if ( $latestRev !== null ) {
+                                       // Singular if only 1 unseen revision, plural if several unseen revisions.
+                                       $plural = $latestRev->getParentId() !== $lastSeenRev->getId();
+                                       $nofAuthors = $uTalkTitle->countAuthorsBetween(
+                                               $lastSeenRev, $latestRev, 10, 'include_new' );
                                }
-                               $plural = $plural ? 2 : 1;
-                               // 2 signifies "more than one revision". We don't know how many, and even if we did,
-                               // the number of revisions or authors is not necessarily the same as the number of
-                               // "messages".
-                               $newMessagesLink = Linker::linkKnown(
-                                       $uTalkTitle,
-                                       $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(),
-                                       array(),
-                                       array( 'redirect' => 'no' )
-                               );
+                       } else {
+                               // Singular if no revision -> diff link will show latest change only in any case
+                               $plural = false;
+                       }
+                       $plural = $plural ? 2 : 1;
+                       // 2 signifies "more than one revision". We don't know how many, and even if we did,
+                       // the number of revisions or authors is not necessarily the same as the number of
+                       // "messages".
+                       $newMessagesLink = Linker::linkKnown(
+                               $uTalkTitle,
+                               $this->msg( 'newmessageslinkplural' )->params( $plural )->escaped(),
+                               array(),
+                               array( 'redirect' => 'no' )
+                       );
 
-                               $newMessagesDiffLink = Linker::linkKnown(
-                                       $uTalkTitle,
-                                       $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(),
-                                       array(),
-                                       $lastSeenRev !== null
-                                               ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' )
-                                               : array( 'diff' => 'cur' )
-                               );
+                       $newMessagesDiffLink = Linker::linkKnown(
+                               $uTalkTitle,
+                               $this->msg( 'newmessagesdifflinkplural' )->params( $plural )->escaped(),
+                               array(),
+                               $lastSeenRev !== null
+                                       ? array( 'oldid' => $lastSeenRev->getId(), 'diff' => 'cur' )
+                                       : array( 'diff' => 'cur' )
+                       );
 
-                               if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) {
-                                       $newMessagesAlert = $this->msg(
-                                               'youhavenewmessagesfromusers',
-                                               $newMessagesLink,
-                                               $newMessagesDiffLink
-                                       )->numParams( $nofAuthors );
-                               } else {
-                                       // $nofAuthors === 11 signifies "11 or more" ("more than 10")
-                                       $newMessagesAlert = $this->msg(
-                                               $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages',
-                                               $newMessagesLink,
-                                               $newMessagesDiffLink
-                                       );
-                               }
-                               $newMessagesAlert = $newMessagesAlert->text();
-                               # Disable Squid cache
-                               $out->setSquidMaxage( 0 );
+                       if ( $nofAuthors >= 1 && $nofAuthors <= 10 ) {
+                               $newMessagesAlert = $this->msg(
+                                       'youhavenewmessagesfromusers',
+                                       $newMessagesLink,
+                                       $newMessagesDiffLink
+                               )->numParams( $nofAuthors );
+                       } else {
+                               // $nofAuthors === 11 signifies "11 or more" ("more than 10")
+                               $newMessagesAlert = $this->msg(
+                                       $nofAuthors > 10 ? 'youhavenewmessagesmanyusers' : 'youhavenewmessages',
+                                       $newMessagesLink,
+                                       $newMessagesDiffLink
+                               );
                        }
+                       $newMessagesAlert = $newMessagesAlert->text();
+                       # Disable Squid cache
+                       $out->setSquidMaxage( 0 );
                } elseif ( count( $newtalks ) ) {
                        $sep = $this->msg( 'newtalkseparator' )->escaped();
                        $msgs = array();
index 12912e1..c86b966 100644 (file)
@@ -3021,8 +3021,9 @@ class User {
         * the next change of the page if it's watched etc.
         * @note If the user doesn't have 'editmywatchlist', this will do nothing.
         * @param $title Title of the article to look at
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function clearNotification( &$title ) {
+       public function clearNotification( &$title, $oldid = 0 ) {
                global $wgUseEnotif, $wgShowUpdatedMarker;
 
                // Do nothing if the database is locked to writes
@@ -3035,12 +3036,25 @@ class User {
                        return;
                }
 
-               if ( $title->getNamespace() == NS_USER_TALK &&
-                       $title->getText() == $this->getName() ) {
-                       if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this ) ) ) {
+               // If we're working on user's talk page, we should update the talk page message indicator
+               if ( $title->getNamespace() == NS_USER_TALK && $title->getText() == $this->getName() ) {
+                       if ( !wfRunHooks( 'UserClearNewTalkNotification', array( &$this, $oldid ) ) ) {
                                return;
                        }
-                       $this->setNewtalk( false );
+
+                       $nextid = $oldid ? $title->getNextRevisionID( $oldid ) : null;
+
+                       if ( !$oldid || !$nextid ) {
+                               // If we're looking at the latest revision, we should definitely clear it
+                               $this->setNewtalk( false );
+                       } else {
+                               // Otherwise we should update its revision, if it's present
+                               if ( $this->getNewtalk() ) {
+                                       // Naturally the other one won't clear by itself
+                                       $this->setNewtalk( false );
+                                       $this->setNewtalk( true, Revision::newFromId( $nextid ) );
+                               }
+                       }
                }
 
                if ( !$wgUseEnotif && !$wgShowUpdatedMarker ) {
@@ -3063,7 +3077,7 @@ class User {
                        $force = 'force';
                }
 
-               $this->getWatchedItem( $title )->resetNotificationTimestamp( $force );
+               $this->getWatchedItem( $title )->resetNotificationTimestamp( $force, $oldid );
        }
 
        /**
@@ -3091,14 +3105,12 @@ class User {
                if ( $id != 0 ) {
                        $dbw = wfGetDB( DB_MASTER );
                        $dbw->update( 'watchlist',
-                               array( /* SET */
-                                       'wl_notificationtimestamp' => null
-                               ), array( /* WHERE */
-                                       'wl_user' => $id
-                               ), __METHOD__
+                               array( /* SET */ 'wl_notificationtimestamp' => null ),
+                               array( /* WHERE */ 'wl_user' => $id ),
+                               __METHOD__
                        );
-               #       We also need to clear here the "you have new message" notification for the own user_talk page
-               #       This is cleared one page view later in Article::viewUpdates();
+                       // We also need to clear here the "you have new message" notification for the own user_talk page;
+                       // it's cleared one page view later in WikiPage::doViewUpdates().
                }
        }
 
index 1e07e7c..d2fb468 100644 (file)
@@ -173,8 +173,9 @@ class WatchedItem {
         *
         * @param $force Whether to force the write query to be executed even if the
         *        page is not watched or the notification timestamp is already NULL.
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function resetNotificationTimestamp( $force = '' ) {
+       public function resetNotificationTimestamp( $force = '', $oldid = 0 ) {
                // Only loggedin user can have a watchlist
                if ( wfReadOnly() || $this->mUser->isAnon() || !$this->isAllowed( 'editmywatchlist' ) ) {
                        return;
@@ -187,10 +188,50 @@ class WatchedItem {
                        }
                }
 
+               $title = $this->getTitle();
+               if ( !$oldid ) {
+                       // No oldid given, assuming latest revision; clear the timestamp.
+                       $notificationTimestamp = null;
+               } elseif ( !$title->getNextRevisionID( $oldid ) ) {
+                       // Oldid given and is the latest revision for this title; clear the timestamp.
+                       $notificationTimestamp = null;
+               } else {
+                       // See if the version marked as read is more recent than the one we're viewing.
+                       // Call load() if it wasn't called before due to $force.
+                       $this->load();
+
+                       if ( $this->timestamp === null ) {
+                               // This can only happen if $force is enabled.
+                               $notificationTimestamp = null;
+                       } else {
+                               // Oldid given and isn't the latest; update the timestamp.
+                               // This will result in no further notification emails being sent!
+                               $dbr = wfGetDB( DB_SLAVE );
+                               $notificationTimestamp = $dbr->selectField(
+                                       'revision', 'rev_timestamp',
+                                       array( 'rev_page' => $title->getArticleID(), 'rev_id' => $oldid )
+                               );
+                               // We need to go one second to the future because of various strict comparisons
+                               // throughout the codebase
+                               $ts = new MWTimestamp( $notificationTimestamp );
+                               $ts->timestamp->add( new DateInterval( 'PT1S' ) );
+                               $notificationTimestamp = $ts->getTimestamp( TS_MW );
+
+                               if ( $notificationTimestamp < $this->timestamp ) {
+                                       if ( $force != 'force' ) {
+                                               return;
+                                       } else {
+                                               // This is a little silly…
+                                               $notificationTimestamp = $this->timestamp;
+                                       }
+                               }
+                       }
+               }
+
                // If the page is watched by the user (or may be watched), update the timestamp on any
                // any matching rows
                $dbw = wfGetDB( DB_MASTER );
-               $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => null ),
+               $dbw->update( 'watchlist', array( 'wl_notificationtimestamp' => $notificationTimestamp ),
                        $this->dbCond(), __METHOD__ );
                $this->timestamp = null;
        }
index edfbba2..403ef11 100644 (file)
@@ -588,6 +588,7 @@ class MediaWiki {
                                                $cache->loadFromFileCache( $this->context );
                                        }
                                        // Do any stats increment/watchlist stuff
+                                       // Assume we're viewing the latest revision (this should always be the case with file cache)
                                        $this->context->getWikiPage()->doViewUpdates( $this->context->getUser() );
                                        // Tell OutputPage that output is taken care of
                                        $this->context->getOutput()->disable();
index 7c3dc93..6d2d80c 100644 (file)
@@ -1134,9 +1134,10 @@ class WikiPage implements Page, IDBAccessObject {
 
        /**
         * Do standard deferred updates after page view
-        * @param $user User The relevant user
+        * @param User $user The relevant user
+        * @param int $oldid The revision id being viewed. If not given or 0, latest revision is assumed.
         */
-       public function doViewUpdates( User $user ) {
+       public function doViewUpdates( User $user, $oldid = 0 ) {
                global $wgDisableCounters;
                if ( wfReadOnly() ) {
                        return;
@@ -1149,7 +1150,7 @@ class WikiPage implements Page, IDBAccessObject {
                }
 
                // Update newtalk / watchlist notification status
-               $user->clearNotification( $this->mTitle );
+               $user->clearNotification( $this->mTitle, $oldid );
        }
 
        /**
index e436f58..ea74164 100644 (file)
@@ -1041,6 +1041,33 @@ class DifferenceEngine extends ContextSource {
                $this->mDiffLang = wfGetLangObj( $lang );
        }
 
+       /**
+        * Maps a revision pair definition as accepted by DifferenceEngine constructor
+        * to a pair of actual integers representing revision ids.
+        *
+        * @param int $old Revision id, e.g. from URL parameter 'oldid'
+        * @param int|string $new Revision id or strings 'next' or 'prev', e.g. from URL parameter 'diff'
+        * @return array Array of two revision ids, older first, later second.
+        *     Zero signifies invalid argument passed.
+        *     false signifies that there is no previous/next revision ($old is the oldest/newest one).
+        */
+       public function mapDiffPrevNext( $old, $new ) {
+               if ( $new === 'prev' ) {
+                       // Show diff between revision $old and the previous one. Get previous one from DB.
+                       $newid = intval( $old );
+                       $oldid = $this->getTitle()->getPreviousRevisionID( $newid );
+               } elseif ( $new === 'next' ) {
+                       // Show diff between revision $old and the next one. Get next one from DB.
+                       $oldid = intval( $old );
+                       $newid = $this->getTitle()->getNextRevisionID( $oldid );
+               } else {
+                       $oldid = intval( $old );
+                       $newid = intval( $new );
+               }
+
+               return array( $oldid, $newid );
+       }
+
        /**
         * Load revision IDs
         */
@@ -1054,26 +1081,14 @@ class DifferenceEngine extends ContextSource {
                $old = $this->mOldid;
                $new = $this->mNewid;
 
-               if ( $new === 'prev' ) {
-                       # Show diff between revision $old and the previous one.
-                       # Get previous one from DB.
-                       $this->mNewid = intval( $old );
-                       $this->mOldid = $this->getTitle()->getPreviousRevisionID( $this->mNewid );
-               } elseif ( $new === 'next' ) {
-                       # Show diff between revision $old and the next one.
-                       # Get next one from DB.
-                       $this->mOldid = intval( $old );
-                       $this->mNewid = $this->getTitle()->getNextRevisionID( $this->mOldid );
-                       if ( $this->mNewid === false ) {
-                               # if no result, NewId points to the newest old revision. The only newer
-                               # revision is cur, which is "0".
-                               $this->mNewid = 0;
-                       }
-               } else {
-                       $this->mOldid = intval( $old );
-                       $this->mNewid = intval( $new );
-                       wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) );
+               list( $this->mOldid, $this->mNewid ) = self::mapDiffPrevNext( $old, $new );
+               if ( $new === 'next' && $this->mNewid === false ) {
+                       # if no result, NewId points to the newest old revision. The only newer
+                       # revision is cur, which is "0".
+                       $this->mNewid = 0;
                }
+
+               wfRunHooks( 'NewDifferenceEngine', array( $this->getTitle(), &$this->mOldid, &$this->mNewid, $old, $new ) );
        }
 
        /**
index de92ef5..61ffcf4 100644 (file)
@@ -1109,8 +1109,9 @@ CREATE TABLE /*_*/watchlist (
   wl_namespace int NOT NULL default 0,
   wl_title varchar(255) binary NOT NULL default '',
 
-  -- Timestamp when user was last sent a notification e-mail;
-  -- cleared when the user visits the page.
+  -- Timestamp used to send notification e-mails and show "updated since last visit" markers on
+  -- history and recent changes / watchlist. Set to NULL when the user visits the latest revision
+  -- of the page, which means that they should be sent an e-mail on the next change.
   wl_notificationtimestamp varbinary(14)
 
 ) /*$wgDBTableOptions*/;