Page MenuHomePhabricator

[EPIC] Cleanup Mobile editor error handling
Open, NormalPublic

Description

The logic responsible for parsing errors for the mobile editor lacks tests and is spread across both EditorOverlay.js and EditorGateway.js.This ticket encompasses cleaning up the code for all editing error scenarios so that they can be better managed in the future.

Developer notes (Here be dragons 🐉)

  • The edit-conflict scenario falls through to the fail handler in EditorGateway.js#252 even though it produces a 200 response.
  • That failure is passed to the fail handler in EditorOverlay.js#590 (which expects three parameters, but is only given one 🧐).
  • there are many nested conditionals in the error handling, across both EditorGateway.js and EditorOverlay.js
  • The responsibilities between which errors are handled in EditorGateway.js and EditorOverlay.js are mixed.
  • there are no tests for the edit-conflict scenario.
  • The Deferred's .fails should be switched to Promise compliant .catchs. (in the code as well as in tests).

Acceptance criteria (done)

  • Fix error handling for edit conflicts
  • Any errors are displayed inside the editor itself, above the preview like so:
  • Move all logic relating to error classification inside the error handler for this.gateway.save from EditorOverlay into EditorGateway. EditorOverlay should simply print the error.
  • There is currently an eslint-ignore-line for no-restricted-properties - this should be removed as part of the changes here.
  • Add unit tests for error handling

QA steps (reproduce the following editing error scenarios)

read only error

Open the mobile editor.
While open, ask a developer to enable

$wgReadOnly = 'This wiki is currently being upgraded to a newer software version.';
  • Does the read only message show when you try to save your edit?

edit conflict error

Open the editor in 2 tabs
In the 2nd tab make an edit and save.
In the 1st tab made an edit and save
In the 1st tab an error should show. It should make clear there has been an "edit conflict"

http error

Open the editor
Disable your internet connection
Save.
The error message should be generic.

captcha

Try to create a new page on staging as an anonymous user.
Doing so should trigger the display of a captcha:


It should be possible to type in the captcha and save (note: only if you can decipher it! you may need to try several times...)

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptAug 21 2018, 10:49 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson renamed this task from [Bug] Wikitext editor error handling broken for edit-conflicts to [Bug] Wikitext editor error handling broken for edit-conflicts (and possibly other issues).Aug 21 2018, 6:05 PM
Jdlrobson added a subscriber: Neil_P._Quinn_WMF.

Heads up @Neil_P._Quinn_WMF I'm not sure if the types of error are tracked in the Schema:Edit

Jdrewniak updated the task description. (Show Details)Aug 21 2018, 9:17 PM
Jdlrobson renamed this task from [Bug] Wikitext editor error handling broken for edit-conflicts (and possibly other issues) to [Bug] Wikitext editor error handling broken for edit-conflicts.Aug 21 2018, 9:56 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

I've checked all the different kinds of errors and it looks like only edit conflicts are broken. let's get this estimated!

We tried to estimate this but couldn't agree on the scope of refactoring (margin on 2 to 8 points). There was lots of talk of subtasks so we could probably split this into smaller chunks.

Change 455895 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Show error-conflict message when someone edited given article

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

Jdlrobson renamed this task from [Bug] Wikitext editor error handling broken for edit-conflicts to Cleanup Wikitext editor error handling.Aug 30 2018, 6:41 PM
Jdlrobson updated the task description. (Show Details)

Change 455895 merged by Pmiazga:
[mediawiki/extensions/MobileFrontend@master] Show error-conflict message when someone edited given article

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

Jdlrobson updated the task description. (Show Details)Sep 6 2018, 4:35 PM

The error handling now works thanks to Piotr, but the code still needs love. I'd like us to attempt another estimation. I'm struggling to work how to split this up but if I do hopefully a conversation will tell me how.

Jdlrobson lowered the priority of this task from Normal to Low.

Change 478970 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/MobileFrontend@master] Mobile VE should log saveFailure events

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

Change 478970 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Mobile VE should log saveFailure events

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

Change 488064 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/MobileFrontend@master] EditorOverlay: captcha/abusefilter weren't being shown correctly

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

Change 488064 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] EditorOverlay: captcha/abusefilter weren't being shown correctly

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

Change 488117 had a related patch set uploaded (by Bartosz Dziewoński; owner: DLynch):
[mediawiki/extensions/MobileFrontend@wmf/1.33.0-wmf.14] EditorOverlay: captcha/abusefilter weren't being shown correctly

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

Change 488195 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] saveFailureMessage moved out of EditorOverlayBase

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

Jdlrobson raised the priority of this task from Low to Normal.
Jdlrobson assigned this task to Jdrewniak.

Coming back to this task, I'm having trouble understanding the scope. I've submitted the above patch, but I suspect there's more to this task then doing just that... @Jdrewniak could you revise this task to make "error handler " less ambigious?

Change 488117 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.33.0-wmf.14] EditorOverlay: captcha/abusefilter weren't being shown correctly

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

Stashbot added a subscriber: Stashbot.

Mentioned in SAL (#wikimedia-operations) [2019-02-06T00:33:52Z] <niharika29@deploy1001> Synchronized php-1.33.0-wmf.14/extensions/MobileFrontend/: EditorOverlay: captcha/abusefilter weren't being shown correctly T215101, T202374 (duration: 00m 50s)

It looks like the scope of this task has expanded from "Wikitext editor error handling broken for edit-conflicts" to "everything wrong with edit error-handling on mobile". I'll see if I can track down the work already done in this area and break this up into smaller tasks.

Thanks jan! Feel free to repurpose this an an epic if necessary!

Jdrewniak renamed this task from Cleanup Wikitext editor error handling to [EPIC] Cleanup Wikitext editor error handling.Feb 12 2019, 10:12 AM
Jdrewniak removed Jdrewniak as the assignee of this task.
Jdrewniak added a project: Epic.
Jdrewniak renamed this task from [EPIC] Cleanup Wikitext editor error handling to [EPIC] Cleanup Mobile editor error handling.Feb 12 2019, 10:27 AM
Jdrewniak updated the task description. (Show Details)
Jdrewniak updated the task description. (Show Details)Feb 12 2019, 11:09 AM

a LOT of things have changed since I last looked at the editing codebase, for the better!

  • The editing code has been migrated to webpack T213340!
  • The error parsing logic is now separated into a parseSaveError.js file, no longer mixed in EditorOverlay.js, hurra!
  • Unit tests have been improved.
  • Deferred .fails have been replaced with .rejects.
  • The bugs with conflict errors have been resolved

The only outstanding work I can see here is the stuff that's already in-progress and further increasing the test coverage by writing tests for the edit conflict scenario (maybe other scenario too? I'm not sure how fully covered they are..)

Is this epic useful? Should we simply resort to the inflight tasks? Is an umbrella (epic) task useful?

Change 488195 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] saveFailureMessage moved out of EditorOverlayBase

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