WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#16928 closed defect (bug) (fixed)

Autosave collision detection does not alert user or prevent further overwriting content

Reported by: benbalter Owned by: IAmWilliamWallace
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.0
Component: Autosave Keywords: has-patch ux-feedback
Focuses: Cc:

Description

When editing a post, upon detecting that another user has modified that same post, the autosave function neither alerts the user as to the conflict nor does it prevent the user from overwriting the other user's changes.

To reproduce: Login as a user ("User 1") and edit an existing post. Open a 2nd incognito/private browsing window and login as a different user ("User 2"). Navigate to the same post and hit update as User 2. Although admin-ajax pushes a notice through its response, User 1 has no way of knowing that User 2 now has the file lock.

Believe the problem lies in line 89 of autosave.dev.js.

message = res.responses[0].data;

The error data is being properly passed form admin-ajax, but is not being properly handled on the client side.

The issue is that the error message doesn't end up in the expected location in the response. The attached patch looks in the second location for the message as appropriate. However, a proper fix might instead be in wpAjax, as the intent might be that responses[0].data should be reflecting the error message.

Beyond notice in the ajax message window, as an additional enhancement (for another ticket), at the very least errors should be big, red, and possibly with the <blink> tag, perhaps a modal dialog or other means of temporarily disabling update (and re-enabling autosave) may be prudent as well. A div class="error" should probably be desired behavior, similar to other warnings sometimes shown here.

Attachments (3)

16928.diff (731 bytes) - added by benbalter 3 years ago.
initial pass, first patch, go easy
16928-2.patch (1.5 KB) - added by azaozz 3 years ago.
autosave_16928.diff (772 bytes) - added by IAmWilliamWallace 3 years ago.

Download all attachments as: .zip

Change History (19)

benbalter3 years ago

initial pass, first patch, go easy

comment:1 in reply to: ↑ description nacin3 years ago

  • Keywords has-patch dev-feedback added; needs-patch 2nd-opinion removed

Replying to benbalter:

The issue is that the error message doesn't end up in the expected location in the response. The attached patch looks in the second location for the message as appropriate. However, a proper fix might instead be in wpAjax, as the intent might be that responses[0].data should be reflecting the error message.

That logic makes sense.

A div class="error" should probably be desired behavior, similar to other warnings sometimes shown here.

Also makes sense.

Asked azaozz to take a look.

azaozz3 years ago

comment:2 azaozz3 years ago

This seems to be a regression from 3.0 or even earlier, but since it's very rare it has gone unnoticed for a long time.

16928-2.patch shows a JS alert with the error and also inserts an error div between the title and the editor. Not sure if that's not a bit too much warnings but we need to stop the user from typing. The JS alert would show in TinyMCE full screen mode too.

Version 1, edited 3 years ago by azaozz (previous) (next) (diff)

comment:4 in reply to: ↑ 3 azaozz3 years ago

Replying to nacin:

This might be a report:...

Yes, seems that's the same problem.

comment:5 nacin3 years ago

Had been reported in #14233.

comment:6 azaozz3 years ago

  • Keywords ux-feedback added; dev-feedback removed
  • Version changed from 3.1 to 3.0

comment:7 benbalter3 years ago

I can confirm azaozz's patch resolves the problem.

Would it make sense to put the error before the #post form so that it gains parity with other notices/errors? (above the title box, rather than below.) Alternatively, if non-rich editing also autosaves, I believe it would need to be tied to both #postdivrich and #postdiv.

comment:8 azaozz3 years ago

We could put the warning at the top but from UX point of view it has to be highly visible. That's why it is a double warning in the patch, the JS alert() stops the user from typing and the <div class="error"... keeps reminding them that somebody else has the same post open for editing.

In any case we would need some UX feedback before committing.

comment:9 azaozz3 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [17640]) Fix two rare regressions: alert when the login has expires and when post is edited by another user, fixes #16928

comment:10 nacin3 years ago

  • Milestone changed from Awaiting Review to 3.2

comment:11 IAmWilliamWallace3 years ago

  • Version changed from 3.0 to 3.1.4

I noticed that the "Autosave disabled: admin is currently editing this post." message was being over written by a previous autosave message so the user never saw the autosave error

comment:12 IAmWilliamWallace3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:13 IAmWilliamWallace3 years ago

  • Owner set to IAmWilliamWallace
  • Status changed from reopened to accepted

comment:14 follow-up: SergeyBiryukov3 years ago

  • Version changed from 3.1.4 to 3.0

Version number is used to track when the bug was initially reported.

comment:15 in reply to: ↑ 14 IAmWilliamWallace3 years ago

Replying to SergeyBiryukov:

Version number is used to track when the bug was initially reported.

Thank you, sorry about that.

Well I submitted a patch, not sure where to go from here.

comment:16 nacin3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Since this ticket was closed on a completed milestone, I'm going to re-close this as fixed on 3.2.

IAmWilliamWallace, this looks like a good fix. We'd love to include it. Can you open a new ticket with the explanation, patch, and preferably steps to reproduce? (Autosave bugs are really tough to track down.) We can likely get this fixed in 3.3.

Note: See TracTickets for help on using tickets.