Page MenuHomePhabricator

Better handle bad token errors (user logs in or out in another tab) in DiscussionTools
Closed, ResolvedPublic

Description

In VisualEditor, when the user is saving their edit, we want to ensure that they understand how their edit will be attributed. Therefore, if the user gets logged out or logs in in another tab, we want to display a message about it before saving.

We should have the same behavior in DiscussionTools.

Event Timeline

Current behavior is:

Screen Shot 2020-02-12 at 3.26.43 PM.png (464×1 px, 139 KB)

Expected behavior:

  • If the token simply expired, we should refresh it and retry without displaying any errors.
  • If the token is invalid because the user logged in/out, we should somehow indicate how their comment will be attributed (and signed).

Change 572374 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] ApiMain: Add support for assert=anon

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

Change 572375 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Use built-in mw.Api 'badtoken' handling, also 'assert'/'assertuser'

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

Change 572376 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Use built-in mw.Api 'badtoken' handling, also 'assert'/'assertuser'

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

These patches implement the first part (retrying without displaying any errors if possible), but not the second (if you logged in or out, you will now get an error message with no obvious way to proceed). I'd like these patches to be reviewed before continuing with the second part.

Change 572374 merged by jenkins-bot:
[mediawiki/core@master] ApiMain: Add support for assert=anon

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

Change 572376 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Use built-in mw.Api 'badtoken' handling, also 'assert'/'assertuser'

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

Although the DiscussionTools part is ready for QA, the cleanup patches in VisualEditor are still waiting for code review: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/572375 https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/572388

After those are merged, I'll work on implementing the second part of this task, that is adding a way to proceed/retry to the current error messages:

These patches implement the first part (retrying without displaying any errors if possible), but not the second (if you logged in or out, you will now get an error message with no obvious way to proceed). I'd like these patches to be reviewed before continuing with the second part.

Change 572375 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Use built-in mw.Api 'badtoken' handling, also 'assert'/'assertuser'

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

I wrote earlier that I wanted to add a way to proceed/retry, instead of only an error message, but on second thought, that doesn't seem to be worth the effort. I propose that we just stop here.

We have the same scenario instrumented in VE. I tried to look at the data and it seems that the user getting logged in or out while editing represents about 0.25% of daily VE errors, I'd expect a similar rate here. @ppelberg Maya or somebody else could probably find you more details using some more sophisticated tools, I just ran a few shell commands to estimate this.

$ pv -c /srv/log/eventlogging/archive/client-side-events.log-20200301.gz | gunzip | grep EditAttemptStep | grep saveFailure | grep userNewUser | wc -l
12.6GiB 0:07:42 [27.8MiB/s] [========================================================================>] 100%
7

$ pv -c /srv/log/eventlogging/archive/client-side-events.log-20200201.gz | gunzip | grep EditAttemptStep | grep saveFailure | grep userNewUser | wc -l
14.3GiB 0:08:39 [28.2MiB/s] [========================================================================>] 100%
10

$ pv -c /srv/log/eventlogging/archive/client-side-events.log-20200301.gz | gunzip | grep EditAttemptStep | grep saveFailure | wc -l
12.6GiB 0:07:42 [27.8MiB/s] [========================================================================>] 100%
3197

$ pv -c /srv/log/eventlogging/archive/client-side-events.log-20200201.gz | gunzip | grep EditAttemptStep | grep saveFailure | wc -l
14.3GiB 0:08:36 [28.3MiB/s] [========================================================================>] 100%
3427

QA steps:

  • Start writing a reply while logged out, then log in in another browser tab, then post the reply
    • Expected: error message
  • Start writing a reply while logged in, then log out in another browser tab, then post the reply
    • Expected: error message
  • Start writing a reply while logged in, then log out and back in in another browser tab, then post the reply
    • Expected: no error message, reply gets posted
    • (note: logging in and out simulates waiting for the token to expire)
Ryasmeen subscribed.

@matmarex: Isn't "Sorry! You are not logged in" more human oriented language than "Assertion that the user is logged in failed."? :P

Screen Shot 2020-03-18 at 12.25.23 PM.png (382×1 px, 78 KB)

Everything else is working as expected though :)

Yes… these messages were originally intended for API users rather than end-users. They can also be used for any action, e.g. watching a page, not just editing. It seemed difficult to come up with a phrasing that works for all these use cases, so I didn't really try, but maybe we can come up with something.

Change 581161 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] Rephrase the "assert..." API error messages for end-users

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

@Ryasmeen How weird does this sound? "You are no longer logged in, so the action could not be completed."

@Ryasmeen How weird does this sound? "You are no longer logged in, so the action could not be completed."

Sounds lot better! :)

Change 581161 merged by jenkins-bot:
[mediawiki/core@master] Rephrase the "assert..." API error messages for end-users

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

Thanks. Don't let me be lazy ;)