Page MenuHomePhabricator

Upstream API error handling from VisualEditor to MediaWiki, use it in DiscussionTools
Closed, ResolvedPublic

Description

I'd like to upstream API error handling from VisualEditor to MediaWiki, so that we can also use the same code in DiscussionTools, and hopefully others will find it useful as well.

Event Timeline

matmarex created this task.Dec 12 2019, 1:08 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 12 2019, 1:08 AM
matmarex claimed this task.Dec 12 2019, 1:09 AM
matmarex edited projects, added VisualEditor (Current work); removed VisualEditor.

Change 556314 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] mw.Api: Add helper method #getErrorMessage

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

Change 556510 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Use mw.Api#getErrorMessage instead of custom handling

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

Change 556315 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] ReplyWidget: Handle save errors

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

Restricted Application added a project: Platform Engineering. · View Herald TranscriptDec 12 2019, 1:39 AM
Anomie added a subscriber: Anomie.

As this task relates to a particular library for accessing the API rather than the API itself, removing MediaWiki-API.

Anomie removed a subscriber: Anomie.Dec 12 2019, 2:46 PM
matmarex renamed this task from Upstream API error handling from VisualEditor to MediaWiki to Upstream API error handling from VisualEditor to MediaWiki, use it in DiscussionTools.Dec 14 2019, 3:54 PM

Change 556314 merged by jenkins-bot:
[mediawiki/core@master] mw.Api: Add helper method #getErrorMessage

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

Change 556510 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Use mw.Api#getErrorMessage instead of custom handling

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

Change 556315 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] ReplyWidget: Handle save errors

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

ppelberg moved this task from Inbox to High Priority on the Editing QA board.Feb 10 2020, 3:05 AM
Ryasmeen added a subscriber: Ryasmeen.

Please mention what I need to test here and the steps that I need to follow to test that and then move it back to Editing QA board for testing.

Basically the same as T229532#5430000 – repeat for VE and for DiscussionTools. Not necessarily all of those cases, but at least a few. Thanks :)

matmarex moved this task from To Triage to Triaged on the VisualEditor board.Feb 12 2020, 5:21 PM
Ryasmeen added a comment.EditedFeb 12 2020, 11:47 PM

Basically the same as T229532#5430000 – repeat for VE and for DiscussionTools. Not necessarily all of those cases, but at least a few. Thanks :)

@matmarex: okay :) So I repeated the two scenarios I tested in T229532#5430000 for both VE and DiscussionTools.

For VE, here is what I am getting:

  1. Bad token

To reproduce: open VE, then in another browser tab log out and log back in, then attempt saving (note: this is handled transparently by VE, without showing an error message, but you'll see a response with "code":"badtoken" in browser dev tools)

Test Result:

  1. Edit conflict

To reproduce: open VE, then make an edit to the same page as another user (in another browser or private mode), then make changes in VE in the same place on the page and attempt saving.

Test Result:

For DiscussionTools I did not get any such API response, but I did get error message for both scenarios.

Test result for Bad token:

Test result for Edit conflict:

I am not sure if I should look somewhere else for the API responses in case of DiscussionTools.

For DiscussionTools I did not get any such API response, but I did get error message for both scenarios.

You should see then if you look at the "api.php" requests. The ones you looked at were from error logging instrumentation, which is only active for VE.


The test results for DiscussionTools look as expected to me – they are different from VE, because in both of these cases VE has bespoke handling for these problems :/ We have separate tasks for those:

If you want to test something where VE and DT will actually behave the same, triggering an abuse filter is a safe bet. On Beta there is apparently a filter specifically for testing, which should prevent you from adding "is stupid" to any page (https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseFilter/461).

Hey @Ryasmeen this is currently blocked on a response from you.

Hey @Ryasmeen this is currently blocked on a response from you.

Well the week just started, I will look into it soon :)

Ryasmeen added a comment.EditedFeb 18 2020, 8:49 PM

For DiscussionTools I did not get any such API response, but I did get error message for both scenarios.

You should see then if you look at the "api.php" requests. The ones you looked at were from error logging instrumentation, which is only active for VE.


The test results for DiscussionTools look as expected to me – they are different from VE, because in both of these cases VE has bespoke handling for these problems :/ We have separate tasks for those:

If you want to test something where VE and DT will actually behave the same, triggering an abuse filter is a safe bet. On Beta there is apparently a filter specifically for testing, which should prevent you from adding "is stupid" to any page (https://en.wikipedia.beta.wmflabs.org/wiki/Special:AbuseFilter/461).

Alright, thanks @matmarex for pointing out where to look for the API errors in case of DiscussionTools.

Here are the test results and they look expected to me as well.

  1. Bad token

To reproduce: open VE, then in another browser tab log out and log back in, then attempt saving (note: this is handled transparently by VE, without showing an error message, but you'll see a response with "code":"badtoken" in browser dev tools)

Test Result:

  1. Edit conflict

To reproduce: open VE, then make an edit to the same page as another user (in another browser or private mode), then make changes in VE in the same place on the page and attempt saving.

Test Result:

  1. API response handled by extension – As above but these error conditions are caused by extension rather than MediaWiki core, and have custom handling in the extension's code. Also, some extensions return API errors in non-standard format.

AbuseFilter:

One note though @matmarex: This AbuseFilter did not get triggered when I added "is stupid" using VE on my sandbox on Beta. Should it not?

One note though @matmarex: This AbuseFilter did not get triggered when I added "is stupid" using VE on my sandbox on Beta. Should it not?

Oh, I didn't realize it works this way, but this is actually the expected behavior. The rule "confirmed" in user_groups in the filter also matches when your groups include autoconfirmed, so the entire filter never triggers for edits by autoconfirmed users (four days old and have made at least 10 edits). It only triggers for non-logged-in users and new accounts; it won't trigger for your account, contrary to what I wrote earlier.

I think that the report from @thiemowmde at https://www.mediawiki.org/wiki/Topic:Vhgb6y8dqm58yjni is (the edit-conflict portion of) this ticket.

Probably not, I'll reply there.

ppelberg closed this task as Resolved.Feb 29 2020, 1:19 AM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFeb 29 2020, 1:19 AM