Page MenuHomePhabricator

Rollback T88044 (broke rollback-related utilities)
Closed, ResolvedPublic

Tokens
"The World Burns" token, awarded by APerson."Dislike" token, awarded by matej_suchanek."The World Burns" token, awarded by BethNaught."The World Burns" token, awarded by BU_Rob13."The World Burns" token, awarded by Yamaguchi."The World Burns" token, awarded by Xaosflux."The World Burns" token, awarded by Ciencia_Al_Poder.
Assigned To
Authored By
Xaosflux, May 27 2016

Description

Recent changes to the rollback function (T88044: Make rollback use POST instead of GET (use AJAX in GUI)) have led to unwanted page confirmation for administrators and also breaking changes to many tools and scripts

Please roll this back until these issues can be resolved.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
APerson added a subscriber: APerson.
K6ka added a subscriber: K6ka.May 27 2016, 2:06 PM
He7d3r updated the task description. (Show Details)May 27 2016, 2:06 PM
K6ka added a comment.May 27 2016, 2:10 PM

At the very least, couldn't there be an option in Special:Preferences to revert this change?

At the very least, couldn't there be an option in Special:Preferences to revert this change?

I don't really think this is really what the main problem is, namely, that this change (which is about the format of the HTTP packets and how the server responds) is breaking tools. In order to unbreak the tools on a the timescale we need (i.e. soon, we need these anti-vandalism tools) there needs to be at least a temporary rollback of this software change.

This is also preventing cswiki script https://cs.wikipedia.org/wiki/MediaWiki:Gadget-SafeRollback.js to work. This scripts should ask user before rollback to prevent unwanted changes. Please fix it ASAP.

+does anybody knows when this will be fixed?

TheDJ raised the priority of this task from High to Unbreak Now!.May 27 2016, 2:51 PM
TheDJ added a subscriber: TheDJ.

The impact of this seems WAY higher than anyone expected. We can't go into a weekend without ANY of the usual vandal fighting elements online.

I'm gonna try and see if I find someone to rollback rollback

Restricted Application added a subscriber: Luke081515. · View Herald TranscriptMay 27 2016, 2:51 PM
TheDJ updated the task description. (Show Details)May 27 2016, 3:04 PM
Krinkle added a comment.EditedMay 27 2016, 3:08 PM

I've gathered the following issues:

  • Make sure the "summary" parameter works in the AJAX handling. (It already works in the API and it works in the confirmation page, but the in-page AJAX method doesn't look at the summary override).
  • Don't auto-hide (or at least make the duration longer by default). We already don't auto-hide errors, but "Rollback success" notifications auto-hide after 5 seconds. This is the same as we've always done for watch/unwatch and for mark-as-patrolled.
  • Bring back the token parameter in the GET fallback url. This way manually opening in a new tab will still work. We also do this with mark-as-patrolled. When we add this fallback, it should be considered deprecated (just as it is for mask-as-patrolled), and it will be removed in the future. But until we're sure all the main tools have migrated and we've uncovered any other issues, we should keep this functional.

Also:

  • When using it on the history page, the history page is out of data after the revert is complete since there is now an extra edit in the history. Previously, by navigating to a different page (or opening a new tab), it being outdated feels more natural. Now one would need to manually refresh the page to see the edit. The script shouldn't do a refresh since users may want to revert multiple edits.
  • A revert may not always work as expected. Previously, the diff was shown by default below page that said the rollback was successful. This diff tends to cover enough context that no further review is needed. Now, however, users get a small notification bubble with a link to the diff (which requires an additional click to open in a new tab to review). This makes reverting multiple edits tedious (and hard, since the bubbles disappear after a while).

I suggest we'll work on this next week. But for now let's rollback the rollback.

We need to think about how we want to implement this better. While the implementation was in line with our conventions and consistent with ajax interfaces for other page actions - there are things unique to rollback (reviewing the diff afterward, resulting in an edit, history page chaning etc.) that warrant a more elaborate interface. The interface we've used thus far for ajax patrol and ajax watch does not suffice.


From https://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28technical%29#Rollback_function_has_been_changed

I can at least confirm the option to leave a custom edit summary with a rollback as described at Wikipedia:Rollback#Additional_tools still works. —k6ka 🍁 (Talk · Contributions) 02:35, 27 May 2016 (UTC)

https://en.wikipedia.org/wiki/Wikipedia:Rollback#Additional_tools
https://en.wikipedia.org/wiki/Wikipedia:Cleaning_up_vandalism/Tools#Rollback_tools
https://en.wikipedia.org/wiki/User:Mr.Z-man/rollbackSummary.js

The summary parameter is not used in MediaWiki core interfaces by default. But when adding it to the rollback link manually (via JavaScript) it does indeed still work when opening in a new tab (with the confirmation form). However the AJAX handling doesn't look at the "summary" parameter. ApiRollback supports "summary" fine. And the RollbackAction class supports it as well. It's just the AJAX handling. Thanks!

I don't think a revert is needed. [..] stop it from auto-hiding


Krinkle claimed this task.May 27 2016, 3:08 PM

Change 291249 had a related patch set uploaded (by Krinkle):
Revert "RollbackAction: Implement AJAX interface and require POST"

https://gerrit.wikimedia.org/r/291249

TheDJ updated the task description. (Show Details)May 27 2016, 3:14 PM
jrbs renamed this task from Rollback T88044 - Breaking Rollback related utilies to Rollback T88044 - Breaking Rollback-related utilities.May 27 2016, 3:21 PM
jrbs added a subscriber: jrbs.

Hello. I was very surprised to see this change. I can't understand why anyone can think it's a good idea. Rollback and patrolling are not the same. Rollback works on more than one version and sometimes you revert more revisions than intented. Page diff showed this and the problem was fixed. Now you can make more changes that you wanted without even know. Actually, this change makes every patroller to be a potential unintented vandal with rollback superpower. Could you remove this change today, please? Thank you.

Hi,
I disagree with IKhitron. Till today you were passed to a page that just notified if the rollback worked or failed. You wren't shown the actual changes anyhow. So the only diff is that now you're notified this msg without being transferred to another page.
Please do NOT remove this change since it's very useful!

Hi Mr W. I already saw that you loved it. I have no idea why don't you see the diff. Maybe there is a bug in you account, or some script that changes the behaviour. But you are completely wrong. If you love this feature add it to your account as gadget, don't destroy wikipedia just because it's better to you personally.

I agree with IKhitron. I think that it is ok to use this method of rollback *by choice*, and let other users to choose differently.
You can make it even default if you want, but don't force my tools to work as you like.

@HeWikipedianMr.W: maybe you enabled the preference "Don't show diff after performing a rollback"?

Thanks, @Brian_He. I'm not so sure that default is a good idea, because what should we do if some patroller will not use it by default and will erase some article? Fire him? It's not serious.

He7d3r renamed this task from Rollback T88044 - Breaking Rollback-related utilities to Rollback T88044 (broke rollback-related utilities).May 27 2016, 4:08 PM
He7d3r updated the task description. (Show Details)
jayvdb added a subscriber: jayvdb.May 27 2016, 4:10 PM
Danny_B updated the task description. (Show Details)May 27 2016, 4:36 PM

Same issue like objected T135170: Require POST in ?action=purge?

It's quite different. action=purge is not output by MediaWiki by default. And its primarily accessed over URL (either by hand-written urls, template links, gadgets, and Semantic extensions).

On the other hand: Watch, patrol, and rollback are core parts of the MediaWiki interface. As such, MediaWiki has control to attach JavaScript handlers to these links by default and effectively cover 99% of their use (e.g. it's rare for someone to see a confirmation page when adding a page to their watchlist , even though action=watch technically has a confirmation page).

TheDJ updated the task description. (Show Details)May 27 2016, 5:59 PM
Masti added a subscriber: Masti.May 27 2016, 6:00 PM

error also happens when I open diff links from irc.wikimedia.org Recent changes channels. Reverting from such diff also invokes confirmation button

Change 291293 had a related patch set uploaded (by Krinkle):
Revert "RollbackAction: Implement AJAX interface and require POST"

https://gerrit.wikimedia.org/r/291293

Change 291293 merged by jenkins-bot:
Revert "RollbackAction: Implement AJAX interface and require POST"

https://gerrit.wikimedia.org/r/291293

@Krinkle - thank you for the reversion - do you know the timeframe for when this will update production?

@Krinkle - thank you for the reversion - do you know the timeframe for when this will update production?

A few hours at most.

Change 291249 merged by jenkins-bot:
Revert "RollbackAction: Implement AJAX interface and require POST"

https://gerrit.wikimedia.org/r/291249

greg added a subscriber: greg.May 27 2016, 7:49 PM
Krinkle closed this task as Resolved.May 27 2016, 8:02 PM
Teles added a subscriber: Teles.May 27 2016, 9:13 PM
MrStradivarius updated the task description. (Show Details)EditedMay 27 2016, 10:31 PM
MrStradivarius added a subscriber: MrStradivarius.

This was also affecting my ConfirmRollback script. If you have the script installed and click on a rollback link on a page that you have set to "confirm", then the script will pop up a dialog saying "Revert n edits by SomeUser?" After the change, the rollback would occur directly after the initial click on the rollback link, regardless of the dialog.

Xaosflux added a comment.EditedMay 27 2016, 10:42 PM

@Krinkle - I noticed your comment "Removed spurious "return true;" from other method that resulted in a "1" being printed at the end of the page." - well this appears to have been promoted to the release, that "1" is showing - reported on enwiki and simple en

This was also affecting my ConfirmRollback script.

Also, I haven't tested it, but I would assume the same issue occurs for enwiki's default gadget confirmationRollback-mobile.

Change 291376 had a related patch set uploaded (by Legoktm):
RollbackAction: Don't return true, causes '1' to be output

https://gerrit.wikimedia.org/r/291376

Change 291377 had a related patch set uploaded (by Legoktm):
RollbackAction: Don't return true, causes '1' to be output

https://gerrit.wikimedia.org/r/291377

Change 291377 merged by jenkins-bot:
RollbackAction: Don't return true, causes '1' to be output

https://gerrit.wikimedia.org/r/291377

This change also appears to have broken PILT - see User:Philip Trueman/PILT - and it is still broken. PILT is based on AVT but uses AJAX for rollback, and has done for years. I would appreciate a pointer to the documentation for how I need to change the code to get it working again.

RuyP added a subscriber: RuyP.May 28 2016, 3:23 AM
He7d3r updated the task description. (Show Details)May 28 2016, 7:15 PM

Change 291376 merged by jenkins-bot:
RollbackAction: Don't return true, causes '1' to be output

https://gerrit.wikimedia.org/r/291376

@Aklapper Returning them, since it's an issue which requires communication between WMF and community and devs.

Have members of such teams to decide whether to remove them if they will want to, thanks.

@Aklapper Returning them, since it's an issue which requires communication between WMF and community and devs.

Apart from the already existing User-notice tag (for weekly Tech News), the scope of Developer-Advocacy and Community-Relations-Support does not generally cover "communication between WMF and community and devs" as this task is about a regression bug and not about product development.

Have members of such teams to decide whether to remove them if they will want to, thanks.

I'm such a team member.

@Aklapper Returning them, since it's an issue which requires communication between WMF and community and devs.

Apart from the already existing User-notice tag (for weekly Tech News), the scope of Developer-Advocacy and Community-Relations-Support does not generally cover "communication between WMF and community and devs" as this task is about a regression bug and not about product development.

"We facilitate communications from and to the WMF Product department."
This task is about how WMF devs broke something and community is unsatisfied with that...

Have members of such teams to decide whether to remove them if they will want to, thanks.

I'm such a team member.

OK, new structure (again :-/). Valid point then... ;-)

[off-topic]

"We facilitate communications from and to the WMF Product department."
This task is about how WMF devs broke something and community is unsatisfied with that...

@Danny_B: The quote says Product department, which was and is not involved in this task.
(Please bring up potential followup discussion in a better place, as "What does team X do" is unrelated to this task. Thanks!)

Change 291761 had a related patch set uploaded (by Krinkle):
Fix rvtoken=rollback in ApiQueryRevisions

https://gerrit.wikimedia.org/r/291761

This change also appears to have broken PILT - see User:Philip Trueman/PILT - and it is still broken. PILT is based on AVT but uses AJAX for rollback, and has done for years. I would appreciate a pointer to the documentation for how I need to change the code to get it working again.

Thanks. Restored in https://gerrit.wikimedia.org/r/291761. Will be deployed later today.

@Philip_Trueman: PITT/recent2.js uses query prop=revisions&rvtoken=rollback. This triggers the warning "The rvtoken parameter has been deprecated." since it was deprecated (generated docs, mediawiki docs) in 2014 (MediaWiki 1.24). This will continue to work for now, but to avoid breakage or hasty migration in the future, I'd recommend migrating to using the central token module (query meta=tokens&type=.., generated docs, mediawiki docs).

Change 291768 had a related patch set uploaded (by Krinkle):
Fix rvtoken=rollback in ApiQueryRevisions

https://gerrit.wikimedia.org/r/291768

Change 291761 merged by jenkins-bot:
Fix rvtoken=rollback in ApiQueryRevisions

https://gerrit.wikimedia.org/r/291761

Change 291768 merged by jenkins-bot:
Fix rvtoken=rollback in ApiQueryRevisions

https://gerrit.wikimedia.org/r/291768

Mentioned in SAL [2016-05-30T17:27:08Z] <krinkle@tin> Synchronized php-1.28.0-wmf.3/includes/api/ApiQueryRevisions.php: T136375 (duration: 00m 52s)

NQ added a subscriber: NQ.Jun 16 2016, 1:17 PM
Restricted Application added a subscriber: Jay8g. · View Herald TranscriptJan 5 2017, 1:49 AM
Shawn added a subscriber: Shawn.May 6 2017, 4:06 PM