Opened 14 years ago
Closed 13 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)
Change History (19)
#1
in reply to:
↑ description
@
14 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.
#2
@
14 years ago
This seems to be a regression in 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.
#7
@
14 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.
#8
@
14 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.
#11
@
13 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
#14
follow-up:
↓ 15
@
13 years ago
- Version changed from 3.1.4 to 3.0
Version number is used to track when the bug was initially reported.
#15
in reply to:
↑ 14
@
13 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.
#16
@
13 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.
initial pass, first patch, go easy