UX: better management of workflow in editor (joe is currently editing this post, etc.)

[azaozz]

As the title, scheduled for 3.3.

[azaozz]

The patch is for removing the post lock on page unload.

We may need to delay adding the post lock as unload event is also triggered when the user reloads the page. That may result in race condition where the XHR removing the lock might arrive later than the new request for the page (that adds the lock).

Tested creating a function that adds the post lock and running it from the shutdown action after 2 sec delay. Alternatively we can check the time the lock was added and not remove it if less than few sec. That may need a bit more thinking/testing.

[azaozz]

Adding sleep() to the removal of the post lock (18515-2.patch) seems to be the best solution.

[azaozz]

Closed #6774 as duplicate.

[jayminkapish]

I am attaching locking plugin we are using here at The New York Times.

The code is released under GPLv2 or later.

Attachement has 1 PHP File, 1 JS File, 1 CSS File and 1 PNG File.

How it works:

Plugin activation creates a table to store lock but we can use post meta table as well.

javascript runs every 10 seconds and pings the server to keep the lock on the post and gives the current user lock on the post. There is a configuration setting in the plugin file which is right now set to 1 minute warning if user sits idle without doing anything on the post and after 3 minutes it times out automatically and removes the lock immediately only if user sits idle. It is using jquery ui dialog to popup warnings. If user is continuously working on the post, js keeps extending timer every 10 seconds. Plugin has the cron built in that removes the lock on all posts older than threshold set in the configuration so in case someone closes the browser, cron will remove the lock on the post.

For example, if User 1 is editing a post and User 2 opens the same post (though User 2 will see a small lock icon on list posts screen), User 2 gets a warning in dialog box that User 1 is currently editing a post. If User 2 is an administrator, plugin adds an additional button on the dialog to Unlock the post forcefully this does not trigger autosave or anything on User 1 window but sets it to read only and gives warning that User 2 has taken the lock. We need this was necessary in case someone is still waiting for the coffee at Starbucks and has the lock. User 2 still can open the post in Read Only mode and read. Plugin also adds Get Me Out button in the meta box which helps User 2 to walk out safely.

This plugin is running globally on *.blogs.nytimes.com and we have not heard any problems. Bloggers love it!

We've tested this on 3.2.1.

Future enhancements:

1. Configuration screen to set warning threshold and timeout minutes
2. better way to handle revisions' restore feature. Currently it is kind of hack since we do not have hooks to control restore link on revisions screen. We do not want any other user to restore the post from revision when someone else has the lock on the post and editing it.
3. UI changes so it matches wp-admin UI design
4. add hooks and filters for warning messages etc.

• Jaymin Patel, Blogs Developer @ The New York Times

[SergeyBiryukov]

The ticket should stay opened until a commit is made to the WordPress trunk.

[nacin]

To make jaymin's edit prominent: "The code is released under GPLv2 or later," despite the plugin's current licensing header of "proprietary."

Thanks for the contribution. This looks slick at a glance. Obviously there's a lot that needs to be done here, but we should look at this from the workflow perspective, rather than than code particulars. We'll switch this to postmeta, etc., that's not the worry. Jane will probably want some things that are in dialog boxes to be brought inline, which is also fine.

So let's get some ux and workflow feedback.

[scribu]

Instead of making separate ajax requests, couldn't this be tied into autosave, if it's not already?

[nacin]

Replying to scribu:

Instead of making separate ajax requests, couldn't this be tied into autosave, if it's not already?

Yes, but we may also want a faster timeout than autosave. (Or decrease the default autosave time.) Additionally, this was written as a plugin, so it was probably easier to run its own script at the time.

[nacin]

In [18709]:

Fix issue where post locks were not updating on autosaves. props benbalter, fixes #18642. see #18515 for more on locking. broken in [12991], see #11889.

[benbalter]

One approach to improved post locks

[benbalter]

Took a *very* rough first pass at improved post locking. If a post is locked (e.g., user2 is editing), if user1 attempts to save (either via autosave, by clicking the update button), the post is saved as a "conflicted revision" and the live post is not touched. Any changes to taxonomies, etc. will be ignored.

If user1 is is an editor or greater (and has a "override_edit_lock" cap), he will have the ability to override user2's lock (AJAX) via link in the standard lock notification, in which case user2 will get an HTML5 notification or JS alert, will automatically do an emergency autosave (which will be conflicted), and the post is reloaded with the lock.

Also made times in the revision table relative, and now update live.

Approach is analogous to how autosaves are handled, it appends "-conflicted" to the post_name.

Again, *very* rough (will most likely break under anything other than ideal conditions), but thought I'd sketch out one possible approach.

[nacin]

To summarize this ticket, I've looked extensively at the functionality, and a decent amount of the code, of both contributions here.

The plugin from jayminkapish is pretty nifty. I think it has a relatively limited use case, but it shows how this may be implemented or desired in a real world use case. When stripped down for a core patch, it'd look rather simple. If cleaned up, it'd make a great plugin in its own right.

The patch from benbalter is far more complicated. It introduces a number of new constructs and workflows. The basic workflow is very similar to the work from jayminkapish. Overall, it's a very good direction we should probably take this feature in.

---

I spoke with jane and she suggested that the best fix (and treating the rest of this as a future enhancement, after further review of the approaches outlined here) would be to ensure we clear the lock when the author leaves the page.

This seems pretty simple to do. There are three reasons why the page will be left:

1. On some sort of POST action such as save or publish.
2. On an unrelated GET or POST action, such as an unrelated form, or clicking elsewhere.
3. A refresh of the page.

In cases 2 and 3, we would currently trigger a notice on unload. In cases 1 and 3, we'd want to keep the lock. Keeping the lock in case 1 is easy. Releasing the lock in case 2, but keeping it in case 3, might be difficult.

However, let's assume for a moment that case 3 (a refresh of the page other than case 1) is rare. So rare we probably don't even need to account for it in a special way.

So, let's fire an ajax request on unload (cases 2 or 3). In order to handle case 3, we provide ourselves a 2-second delay. azaozz's patch does this by waiting 5 seconds before forcibly releasing the lock.

Unfortunately, the approach contains a race condition. By sleeping for 4 seconds and checking for five seconds, this assumes that the ajax request didn't take longer than 1 second to process. Otherwise, it's possible that the refreshed page updates the lock and the ajax request then takes it away. Another option could be to, via ajax, updating the _edit_lock to time() + 5. Of course, you're hoping that overlap doesn't occur in some other way.

So, my proposal: If we knew what the current lock is when making the ajax request, then we could see if it's the same lock we ourselves obtained, and if so, kill it. We could avoid the race condition by specifying the $meta_value for delete_post_meta(), since we'll know what lock and user ID we're looking for.

An interesting footnote, I also looked into update_metadata() and found that does not provide the return value of $wpdb->update(), which means if you passed a $prev_value, you don't actually know if update_metadata() was able to actually find the row to update. Since we won't be receiving the result of the ajax request, it wouldn't have been helpful here beyond debugging.

[nacin]

In [18790]:

wp_set_post_lock() only takes one argument. see #18515.

[nacin]

18515.diff​ does what I described in above.

• When a person leaves a post without saving, the lock will terminate five seconds later.

That's it, really. It works. Has a bit of voodoo in the XHR handler to ensure the lock terminates five seconds later, without having the script hang. The alternative is to delete the meta rather than update it, after either waiting five seconds or not. Unfortunately, I found (on Chrome, at least) that you can't fire an async XHR call on the unload event and expect it to go through, so hanging the script isn't really an option.

[azaozz]

Replying to nacin:

... Unfortunately, I found (on Chrome, at least) that you can't fire an async XHR call on the unload event and expect it to go through, so hanging the script isn't really an option.

Yes, also if the user quits the browser the XHR may not be fired.

Looking through 18515.diff there's still a possibility to have collision when the user reloads the edit-post page. The post meta is updated with the XHR and just few milliseconds later wp_set_post_lock() is called as the page is loaded again. The XHR always calls update_meta and loading a post always calls set_post_lock. We need to think of a better way.

Also perhaps we can make the lock expiration faster. No need to wait 4 min (AUTOSAVE_INTERVAL * 2), maybe AUTOSAVE_INTERVAL + 10 would be good.

[nacin]

Replying to azaozz:

Looking through 18515.diff there's still a possibility to have collision when the user reloads the edit-post page. The post meta is updated with the XHR and just few milliseconds later wp_set_post_lock() is called as the page is loaded again. The XHR always calls update_meta and loading a post always calls set_post_lock. We need to think of a better way.

The XHR does not always update the meta, though. It will only update the metadata if the lock it is trying to cancel is still the current lock. It does this atomically, so if the new page has already set a new lock, then the XHR will not. That's the whole crux of this patch. See the last two paragraphs in comment 13.

Also perhaps we can make the lock expiration faster. No need to wait 4 min (AUTOSAVE_INTERVAL * 2), maybe AUTOSAVE_INTERVAL + 10 would be good.

AUTOSAVE_INTERVAL is 60 seconds, so two minutes. I think it's fine for now.

[azaozz]

Replying to nacin:

The most basic workflow:

• the user opens a post for editing, post lock is set from the page load request,
• the user reloads the page, XHR is sent to remove the lock on unload (cancel the current lock) and few milliseconds later a new page load request it sent to load the page again. At this point we have two processes accessing the db: one is running add_post_meta the other update_post_meta for the same post_id and meta key.

[nacin]

Per discussion in IRC, the potential for collision at the DB level is mitigated by the fact that the two locking queries (XHR and pageload) can be run in either order, as the XHR one is bounded by a WHERE for the lock it knows about. Additionally, the XHR is run synchronous to ensure it isn't canceled by the unload event, which bides time. It's no different than how locks are extended both on save and on subsequent pageload.

[ryan]

This seems to cover the bases and is like enough to the cron updates in 3.3 that we will at least be consistent in our locking attempts. :-) Let's commit and give it a nice beta soak.

[nacin]

In [18796]:

Release a user's post lock when the user leaves a post. see #18515.

[nacin]

The only bug report to come out of this was a trailing comma in autosave.js that borked IE.

Closing as fixed.

[azaozz]

In [19258]:

Send the 'wp-remove-post-lock' XHR only when the main window is unloaded (unloading the Thickbox iframe triggers it too), see #18515