Last modified: 2012-02-29 01:41:14 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.
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 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.
Removing patch and needs-review keywords; failed security review. Haven't checked other aspects of patch at this time.
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.
(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
Created attachment 1291 [details] The update marker patch with cleanups separated out.
Created attachment 1292 [details] The code cleanups - apply over previous patch.
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.
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?
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.
> 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?
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.
Created attachment 1298 [details] Supercedes previous patch which had a minor logic error.
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.
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).
In the above comment I meant to write "if only $wgEnotifUserTalk is true" rather than "if only $wgEnotifWatchlist is true".
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
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.
Can you submit one unified diff? It is hard to tell what is going on there.
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. :-)
Changed component to "Special pages"
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 :)
Same is in Watchlist as well ("Mark as read").
