Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Facilitate users concurrently editing a snapshot #60

Open
westonruter opened this issue Jul 12, 2016 · 3 comments
Open

Facilitate users concurrently editing a snapshot #60

westonruter opened this issue Jul 12, 2016 · 3 comments
Assignees

Comments

@westonruter
Copy link
Contributor

If two users edit the same snapshot, there is currently the real possibility that users will override each other's changes. There needs to be concurrency support for snapshots. When this happens, the snapshot save should be rejected as a validation failure, with a prompt to resolve the conflict, by accepting their changes or overriding the changes with their own.

We need to keep track of the settings that have been changed since the last snapshot save and send it along with the snapshot save request. See approach for keeping track of settings changed: https://github.com/xwp/wordpress-develop/blob/a501d6c8be242dce7bcff0e14b145953057ff7ba/src/wp-admin/js/customize-controls.js#L3685-L3688

Whenever a value is updated in a snapshot, beyond storing the setting value we can also store the timestamp for when the setting was updated and also the user ID who actually saved it.

Other general notes on concurrency locking in general:

  • Validation is an important way that we can solve concurrency problems.
  • Use heartbeat to announce that a setting has changed, and allow the value to be supplied from the other user
  • Send locking overrides via separate channel than customized data, since modified date hack won't work for other kinds of settings
@lgedeon
Copy link
Contributor

lgedeon commented Jul 22, 2016

@westonruter Instead of locking could we add a history of changes that have happened in the snapshot and keep the latest value? Then we could show that a change has been made and let the content editors keep, restore a different version or combine?

I am thinking of a ui like this:
screen shot 2016-07-21 at 11 43 02 pm

Both links (red and yellow) would pop-up a history (new panel?) with the option to go back to an older version.

The code to capture the history could look like this: (modified from /plugins/customize-snapshots/php/class-customize-snapshot.php)

    public function set( \WP_Customize_Setting $setting, $value, $deprecated = null ) {
        if ( ! is_null( $deprecated ) ) {
            _doing_it_wrong( __METHOD__, 'The $dirty argument has been removed.', '0.4.0' );
            if ( false === $deprecated ) {
                return;
            }
        }

        $history = isset( $this->data[ $setting->id ]['history'] ) ? (array) $this->data[ $setting->id ]['history'] : array();

        if ( ! isset( $this->data[ $setting->id ]['value'] ) || $this->data[ $setting->id ]['value'] !== $value ) {
            $history[] = array(
                'value' => $value,
                'user' => get_current_user_id(),
                'timestamp' => current_time( 'timestamp', true ),
            );
        }

        $this->data[ $setting->id ] = array(
            'value' => $value,
            'sanitized' => false,
            'history' => $history,
        );
    }

@lgedeon lgedeon self-assigned this Jul 22, 2016
@westonruter
Copy link
Contributor Author

@lgedeon yeah, I agree that the user should be able to either accept the other's saved value or override the other's saved value. They should be able to see what the value is. I don't think we need a full history however. We just need to store with each value the modified time and the user who updated it. You can see in the refactor branch that the Customize_Snapshot::set() allows you to pass a mapping of setting ID to an array (including value and any other metadata you like, such as modified and user). When saving a snapshot, the existing snapshot's data, along with the timestamps for each setting, can be examined to see if the timestamp is greater than the current time. If so, the snapshot save should be rejected with an error sent back to the user, along with the conflicted values. This has actually been implemented already in Customize Posts. Try opening two separate browser sessions and change the title of the same post and save in each session. You'll see a conflict notice with an override option. Something like this would be good for snapshots. To communicate the overrides in Customize Posts, we're hacking the post_modified_gmt value. In snapshots, however, we'll need to introduce a new POST var like conflict_overridden_setting_ids which would cause the conflict detection to skip for those settings.

@westonruter
Copy link
Contributor Author

@lgedeon sorry for the confusion regarding “if the timestamp is greater than the current time”. What I meant is if the “setting modified timestamp” in the snapshot is greater than the timestamp for when the user's Customizer session was first started. So when a user opens the Customizer, we should capture the time, and then send this time in every snapshot save request. In the request we can then compare the session start timestamp to determine whether there is a conflict.

Really this functionality could actually be worked into (or at least integrated with) a revisited Customize Concurrency plugin: https://github.com/xwp/wp-customize-concurrency

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants