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

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

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.

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

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 :)

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 :)

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.

Probably not, I'll reply there.