xwp / wp-customize-snapshots

Customize Snapshots WordPress Plugin
https://wordpress.org/plugins/customize-snapshots/
52 stars 11 forks source link

Facilitate users concurrently editing a snapshot #60

Open westonruter opened 8 years ago

westonruter commented 8 years ago

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:

lgedeon commented 8 years ago

@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,
        );
    }
westonruter commented 8 years ago

@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 commented 8 years ago

@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