Last modified: 2012-02-29 01:41:14 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 4553 - Indicate whether edits have already been seen on the watchlist and recent changes
Indicate whether edits have already been seen on the watchlist and recent cha...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Special pages (Other open bugs)
1.6.x
All All
: Low enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks: 2877 536
  Show dependency treegraph
 
Reported: 2006-01-10 15:21 UTC by netocrat
Modified: 2012-02-29 01:41 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Add green U marker to updated watchlist pages in recent changes/watchlist views. (14.56 KB, patch)
2006-01-10 15:27 UTC, netocrat
Details
Patch fixed to quote the title within the SQL insert query to prevent injection. (14.60 KB, patch)
2006-01-10 21:30 UTC, netocrat
Details
The update marker patch with cleanups separated out. (9.66 KB, patch)
2006-01-12 04:54 UTC, netocrat
Details
The code cleanups - apply over previous patch. (5.26 KB, patch)
2006-01-12 04:55 UTC, netocrat
Details
Make global notification logic consistent. (510 bytes, patch)
2006-01-12 04:56 UTC, netocrat
Details
A missing conversion to NULL from the previous zero for wl_notificationtimestamp. (565 bytes, patch)
2006-01-12 04:59 UTC, netocrat
Details
Deals with talk-page-notify missing watchlist entry and $wgEnotifWatchlist, $wgEnotifUserTalk simultaneously true (5.99 KB, patch)
2006-01-12 09:01 UTC, netocrat
Details
Supercedes previous patch which had a minor logic error. (6.06 KB, patch)
2006-01-12 09:26 UTC, netocrat
Details
Minimal-change patch to deal with missing talk page watches; supercedes both previous patches. (3.07 KB, patch)
2006-01-12 13:53 UTC, netocrat
Details

Description netocrat 2006-01-10 15:21:02 UTC
The following patch adds a green 'U' marker for any of the user's watchlist
pages that have been updated since last view, to supplement the 'N' for new
pages and '!' for unpatrolled pages on the watchlist/recent changes special
pages.  It also makes a couple of code clean-ups.
Comment 1 netocrat 2006-01-10 15:27:42 UTC
Created attachment 1282 [details]
Add green U marker to updated watchlist pages in recent changes/watchlist views.

A couple of clean-ups are included - removal of newFromCurRow and
loadFromCurRow functions from RecentChange.php as they are now no longer used
in any core code (nor code of any extensions within the CVS repository), and
update of the SQL of the query whose results were redundantly being routed
through these functions within SpecialWatchlist.php.
Comment 2 Brion Vibber 2006-01-10 20:37:35 UTC
Comment on attachment 1282 [details]
Add green U marker to updated watchlist pages in recent changes/watchlist views.

Do not use this patch; it contains a SQL injection vulnerability on first
inspection.
Comment 3 Brion Vibber 2006-01-10 20:38:11 UTC
Removing patch and needs-review keywords; failed security review. Haven't checked other aspects 
of patch at this time.
Comment 4 netocrat 2006-01-10 21:30:03 UTC
Created attachment 1285 [details]
Patch fixed to quote the title within the SQL insert query to prevent injection.

The clean-up part of the patch can be separated from the new functionality if
that makes it easier to review.
Comment 5 Ævar Arnfjörð Bjarmason 2006-01-11 14:32:56 UTC
(In reply to comment #4)
 > The clean-up part of the patch can be separated from the new functionality if
> that makes it easier to review.

Please do, cleanup patches should be submitted seperately

Comment 6 netocrat 2006-01-12 04:54:15 UTC
Created attachment 1291 [details]
The update marker patch with cleanups separated out.
Comment 7 netocrat 2006-01-12 04:55:03 UTC
Created attachment 1292 [details]
The code cleanups - apply over previous patch.
Comment 8 netocrat 2006-01-12 04:56:57 UTC
Created attachment 1293 [details]
Make global notification logic consistent.

This patch makes the logic of $wgShowUpdateMarker and $wgEnotifWatchlist
consistent with that of UserMailer.php, line 274.
Comment 9 netocrat 2006-01-12 04:59:32 UTC
Created attachment 1294 [details]
A missing conversion to NULL from the previous zero for wl_notificationtimestamp.

Arguably this should be a new bug, but I don't see the point in creating one as
it's related to the previous patches.  Btw, why was the change from 0 to NULL
made in the first place?
Comment 10 Brion Vibber 2006-01-12 06:53:03 UTC
Tom Gries incorrectly used a non-NULL value, assuming that timestamp columns were 
always VARCHAR. For whatever reason he still hadn't fixed this by the time his 
patches got applied to the 1.5 development branch, so it remained broken until I 
fixed up this code recently.

We use proper datetime columns for non-MySQL databases (PostgreSQL and Oracle) and 
will likely fix this on MySQL at some point as well.
Comment 11 netocrat 2006-01-12 08:14:55 UTC
> We use proper datetime columns for non-MySQL databases (PostgreSQL and Oracle)
> will likely fix this on MySQL at some point as well.

OK - presumably (I haven't checked) for the non-MySQL databases, zero can't be
stored into or selected from datetime columns (it can in MySQL).  Is MW policy
for columns to be non-nullable where possible?
Comment 12 netocrat 2006-01-12 09:01:58 UTC
Created attachment 1295 [details]
Deals with talk-page-notify missing watchlist entry and $wgEnotifWatchlist, $wgEnotifUserTalk simultaneously true

Since (at least) the fixes Brion mentions in his comment above, the logic of
the mailer has:
a) assumed that a watchlist entry already exists for users monitoring their own
talk page through the 'enotifusertalkpages' option - this assumption needn't be
true.  This patch removes that assumption by adding the watchlist entry if it
doesn't already exist - the other place that could be patched to fix this is
when saving user preferences, but it seems safer here
b) assumed that either $wgEnotifWatchlist or $wgEnotifUserTalk may be true, but
not both simultaneously.  There's no comment in e.g. DefaultSettings.php that
this restriction holds, and it's possible to deal gracefully with both of them
being true, which this patch attempts to do.

The patch applies on top of the previous patches and doesn't apply cleanly to
CVS code; let me know if a patch not depending on the previous patches would be
useful.
Comment 13 netocrat 2006-01-12 09:26:41 UTC
Created attachment 1298 [details]
Supercedes previous patch which had a minor logic error.
Comment 14 Brion Vibber 2006-01-12 09:35:37 UTC
Are you sure you're looking at current CVS HEAD code? a) and b) both sound like you're 
referring to the code as of about October/November before I changed it.
Comment 15 netocrat 2006-01-12 10:00:14 UTC
I'm looking at revision 1.38, from the sourceforge anonymous-access cvs repository.

The logic (simplified and ignoring error-checking) is:

if ($wgEnotifWatchlist) then
   query for all users bar the editing user
else if ($wgEnotifUserTalk and this is a talk page) then 
   query for the talk-page user only

So if only $wgEnotifWatchlist is true and the user does not have a watch-list
entry, they won't get a notification.

Also if both conditions are true and the user does not have a watch-list entry,
they won't get a notification - which is what lead me to (b), but it's not
really an independent problem so let's drop that one (the patch isn't affected
either way, just the description of why it's useful).
Comment 16 netocrat 2006-01-12 10:03:38 UTC
In the above comment I meant to write "if only $wgEnotifUserTalk is true" rather
than "if only $wgEnotifWatchlist is true".
Comment 17 netocrat 2006-01-12 13:53:55 UTC
Created attachment 1299 [details]
Minimal-change patch to deal with missing talk page watches; supercedes both previous patches.

OK, after some thought, here's a minimal and less intimidating patch than the
first (the second patch was rushed and had a syntax error).  It does the
following:

* when $enotifusertalkpage is true and the user has set the
'enotifusertalkpages' option, make sure a watch exists by calling
$targetUser->addWatch( $title ); - this function results in an sql query using
"insert ignore" so it doesn't matter if there's already a watch
* we want to perform this regardless of whether $wgEnotifWatchlist is also
true, so change the elseif to an if, and to compensate, further in the second
if check for $wgEnotifWatchlist not true before setting $userCondition (a
cosmetic change is to use $enotifusertalkpage as the test condition rather than
the more complicated expression that it represents)
* when testing whether to send an email, for the precedence to be correct there
needs to be parentheses around the or condition (I've also removed redundant
parentheses around the isEmailConfirmed() test)
* we also need to set the notification timestamp when this is a user talk page
so $enotifusertalkpage has been added to the test
Comment 18 netocrat 2006-03-02 08:06:28 UTC
Brion, coming back to this after some time on other things, I find a line that I
missed the first time around in User::setNewtalk():
$this->addWatch( $this->getTalkPage() );
So yes, your changes did largely fix the issue after all, with one exception:
User::setNewtalk() is called within Article::editUpdates(), however the email
notification occurs within the call to RecentChange::notifyEdit() or
RecentChange::notifyNew() in Article::updateArticle() and
Article::insertNewArticle() respectively, both of which are called /prior/ to
Article::editUpdates() within those functions.

So on (at least) the very first occasion, the new talk flag will be set but the
email won't be sent.  This is what my testing showed up and why I reported the
bug, unfortunately I missed that you had dealt with subsequent occasions already.

The patch I submitted on 2006-01-12 13:53 UTC is still valid - it deals with the
problem by moving the precautionary adding of the talk page watch to the email
sending function (it doesn't remove the line I missed which could be removed if
the patch or similar were accepted); there no doubt are other solutions.  Also
as noted the patch depends on previous patches attached to this bug but it would
be simple to remove those dependencies.
Comment 19 Aaron Schulz 2008-05-17 22:26:45 UTC
Can you submit one unified diff? It is hard to tell what is going on there.
Comment 20 netocrat 2008-05-18 09:16:02 UTC
I'm really sorry Aaron, but MediaWiki feels like another world to me - it's been so long since I played with it.  I barely even remember submitting these patches, let alone having the energy and motivation to clarify them for you.  I'm involved in paid work right now, so let alone that I don't have the energy and motivation to chase them up, I can't *afford* to do it either.  I'm afraid that you're either going to have to sort through this on your own, or to dismiss it as unresolvable.  I do recommend that you take the former option, though, because - even though I don't remember the essence of this patch - I don't talk crap and I'm sure that the patch is worthwhile, if it will still apply to the current code.  Just the title of it makes sense to me - it would be great to see through a green "U" icon in your watchlist when a page has changed.  I expect that the code that I submitted achieves that aim, if you could be bothered to analyse it, even though I don't remember the specifics of it a couple of years later.  If not, then I don't imagine that it would be too hard to generate your own diff to achieve that aim.  I expect that you're a bright enough chap.  :-)
Comment 21 Siebrand Mazeland 2009-02-02 11:49:07 UTC
Changed component to "Special pages"
Comment 22 Krinkle 2011-08-31 23:39:49 UTC
Note from the future (from the bug's creation perspective):

On the history-page of a page we can, anno 2011, see which edits were made after the user last looked at it. This is a feature existing atleast since MediaWiki 1.16.

Seems related to this bug, just saying :)
Comment 23 Krinkle 2012-02-29 01:41:14 UTC
Same is in Watchlist as well ("Mark as read").

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links