jenkins-bot has submitted this change and it was merged.
Change subject: Correctly update wl_notificationtimestamp when viewing old
revisions
......................................................................
Correctly update wl_notificationtimestamp when viewing old revisions
== 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
---
M RELEASE-NOTES-1.23
M docs/hooks.txt
M includes/Article.php
M includes/ImagePage.php
M includes/Skin.php
M includes/User.php
M includes/WatchedItem.php
M includes/Wiki.php
M includes/WikiPage.php
M includes/diff/DifferenceEngine.php
M maintenance/tables.sql
11 files changed, 173 insertions(+), 100 deletions(-)
Approvals:
Brian Wolff: Looks good to me, approved
jenkins-bot: Verified
diff --git a/RELEASE-NOTES-1.23 b/RELEASE-NOTES-1.23
index 372170a..ec7b898 100644
--- a/RELEASE-NOTES-1.23
+++ b/RELEASE-NOTES-1.23
@@ -13,6 +13,10 @@
=== 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 ===
diff --git a/docs/hooks.txt b/docs/hooks.txt
index 5aaf596..2671dd8 100644
--- a/docs/hooks.txt
+++ b/docs/hooks.txt
@@ -2579,6 +2579,7 @@
'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.
diff --git a/includes/Article.php b/includes/Article.php
index 0b18221..928fda0 100644
--- a/includes/Article.php
+++ b/includes/Article.php
@@ -586,7 +586,7 @@
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 @@
$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 @@
$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 );
}
/**
diff --git a/includes/ImagePage.php b/includes/ImagePage.php
index 515f146..cf05ee2 100644
--- a/includes/ImagePage.php
+++ b/includes/ImagePage.php
@@ -128,7 +128,7 @@
$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 @@
# 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
diff --git a/includes/Skin.php b/includes/Skin.php
index 5801806..170e96f 100644
--- a/includes/Skin.php
+++ b/includes/Skin.php
@@ -1379,61 +1379,58 @@
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' )
- );
-
- $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 );
+ } 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' )
+ );
+
+ 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();
diff --git a/includes/User.php b/includes/User.php
index 12912e1..c86b966 100644
--- a/includes/User.php
+++ b/includes/User.php
@@ -3021,8 +3021,9 @@
* 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 @@
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 @@
$force = 'force';
}
- $this->getWatchedItem( $title )->resetNotificationTimestamp(
$force );
+ $this->getWatchedItem( $title )->resetNotificationTimestamp(
$force, $oldid );
}
/**
@@ -3091,14 +3105,12 @@
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().
}
}
diff --git a/includes/WatchedItem.php b/includes/WatchedItem.php
index 1e07e7c..d2fb468 100644
--- a/includes/WatchedItem.php
+++ b/includes/WatchedItem.php
@@ -173,8 +173,9 @@
*
* @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 @@
}
}
+ $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;
}
diff --git a/includes/Wiki.php b/includes/Wiki.php
index edfbba2..403ef11 100644
--- a/includes/Wiki.php
+++ b/includes/Wiki.php
@@ -588,6 +588,7 @@
$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();
diff --git a/includes/WikiPage.php b/includes/WikiPage.php
index 7c3dc93..6d2d80c 100644
--- a/includes/WikiPage.php
+++ b/includes/WikiPage.php
@@ -1134,9 +1134,10 @@
/**
* 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 @@
}
// Update newtalk / watchlist notification status
- $user->clearNotification( $this->mTitle );
+ $user->clearNotification( $this->mTitle, $oldid );
}
/**
diff --git a/includes/diff/DifferenceEngine.php
b/includes/diff/DifferenceEngine.php
index e436f58..ea74164 100644
--- a/includes/diff/DifferenceEngine.php
+++ b/includes/diff/DifferenceEngine.php
@@ -1042,6 +1042,33 @@
}
/**
+ * 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
*/
private function loadRevisionIds() {
@@ -1054,26 +1081,14 @@
$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 ) );
}
/**
diff --git a/maintenance/tables.sql b/maintenance/tables.sql
index de92ef5..61ffcf4 100644
--- a/maintenance/tables.sql
+++ b/maintenance/tables.sql
@@ -1109,8 +1109,9 @@
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*/;
--
To view, visit https://gerrit.wikimedia.org/r/84315
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4144ba1987b8d7a7e8b24f4f067eedac2ae44459
Gerrit-PatchSet: 13
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Bartosz Dziewoński <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Alex Monk <[email protected]>
Gerrit-Reviewer: Bartosz Dziewoński <[email protected]>
Gerrit-Reviewer: Brian Wolff <[email protected]>
Gerrit-Reviewer: Brion VIBBER <[email protected]>
Gerrit-Reviewer: Parent5446 <[email protected]>
Gerrit-Reviewer: Reedy <[email protected]>
Gerrit-Reviewer: Tim Starling <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
