Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that revisions get created when changesets are updated via explicit saves #111

Merged
merged 12 commits into from Feb 28, 2017

Conversation

Projects
None yet
4 participants
@westonruter
Copy link
Contributor

commented Feb 7, 2017

Found a bug with 0.6.0-rc1 whereby revisions were no longer being made as expected when explicitly clicking the Save Draft button in the UI. The opt-in for creating a revision in core is currently indicated by whether or not a customize_changeset_status param is present. This is not ideal, but since the param is getting stripped by Customize Snapshots if the status is the same as the current status, I added a workaround for this to explicitly indicate the need to create a revision via another channel. There is probably a better solution for this, not at least to allow a save_revision to be passed when calling wp.customize.previewer.save().

See: https://github.com/WordPress/wordpress-develop/blob/8900e2466e3a01c9c228ac31784aa70e8dcf3137/src/wp-includes/class-wp-customize-manager.php#L2279

@westonruter westonruter added this to the 0.6.0 milestone Feb 7, 2017

@westonruter westonruter requested review from PatelUtkarsh and sayedtaqui Feb 7, 2017

@@ -1113,6 +1113,7 @@
isSameStatus = api.state( 'changesetStatus' ).get() === originalOptions.data.customize_changeset_status;
if ( 'customize_save' === originalOptions.data.action && isSameStatus && options.data ) {
options.data = removeParam( options.data, 'customize_changeset_status' );

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 7, 2017

Author Contributor

Why is the customize_changeset_status param getting removed, again? Is this still needed?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Feb 9, 2017

Collaborator

As we are checking customize_snapshots_create_revision customize_changeset_status does not need to be removed now.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 9, 2017

Author Contributor

@sayedtaqui but core will (somewhat unintuitively) create a new revision every time the customize_changeset_status is supplied. Wasn't there a reason for the param to be removed? Did it have something to do with passing the date?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Feb 9, 2017

Collaborator

@westonruter Yes we wanted to auto save the date and title in editbox, but we could not do wp.customize.previewer.save() without status because the default status is publish, and we did not want to create revision each time it auto saves the date/title.

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 9, 2017

Author Contributor

OK, so then we need to still include the logic for removing the status parameter, but then an additional change is needed right? We need to omit the request for a revision when the date and title are modified?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Feb 9, 2017

Collaborator

Oh yes, thats correct.

@coveralls

This comment has been minimized.

Copy link

commented Feb 7, 2017

Coverage Status

Coverage increased (+0.06%) to 76.923% when pulling e5aea6a on bugfix/create-revisions into 7910d6b on develop.

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2017

I think the code was adding status(revision) to request only when we change status from drop down.
@sayedtaqui Can you update such that it pass status when saved explicitly?

@PatelUtkarsh
Copy link
Collaborator

left a comment

This looks good, Unless @sayedtaqui want to change this via using JS only.

@@ -104,6 +104,8 @@ function hooks() {
add_action( 'admin_bar_menu', array( $this, 'remove_all_non_snapshot_admin_bar_links' ), 100000 );
add_action( 'wp_before_admin_bar_render', array( $this, 'print_admin_bar_styles' ) );
add_filter( 'removable_query_args', array( $this, 'filter_removable_query_args' ) );
add_filter( 'wp_save_post_revision_post_has_changed', array( $this, '_filter_revision_post_has_changed' ), 20, 3 );
add_filter( 'save_post_customize_changeset', array( $this, 'create_initial_changeset_revision' ) );

This comment has been minimized.

Copy link
@sayedtaqui

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 9, 2017

Author Contributor

You're right! It should be add_action here.

@PatelUtkarsh

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

I think this is a better approach than doing it via just JS.

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2017

I think checking the param customize_snapshots_create_revision is smarter than removing customize_changeset_status plus this

isSameStatus = api.state( 'changesetStatus' ).get() === originalOptions.data.customize_changeset_status

would return true in case of existing changeset which has draft status so creating revision for the first time makes perfect sense

Looks good 👍

@westonruter westonruter changed the title Ensure that revisions get created when changesets are updated via explicit saves [WIP] Ensure that revisions get created when changesets are updated via explicit saves Feb 10, 2017

@coveralls

This comment has been minimized.

Copy link

commented Feb 10, 2017

Coverage Status

Coverage increased (+0.06%) to 76.923% when pulling 66b6bb0 on bugfix/create-revisions into 7910d6b on develop.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2017

@sayedtaqui I'm noticing something else here that is unexpected. If I open the customizer, change a setting, and then save the changeset as a draft; then if I click the edit icon, and then try supplying a title the changed title won't get saved until I try making another change to settings. In other words, it doesn't seem that the title changes are getting sent on change as expected.

Are you able to tie up the lose ends on this ticket, including sending the flag to indicate a revision should be made, as well as ensuring changes to the title cause a title update to be sent? Also, related is #112 where the date fields are being unexpectedly shown.

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

Sure working on them.

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

try supplying a title the changed title won't get saved until I try making another change to settings. In other words, it doesn't seem that the title changes are getting sent on change as expected.

@westonruter The title would not get saved if the date is not of future, once user updates the date to be of future, it will start auto-saving, but yes it should be happening only in case of future, which I have updated. Now working on the main issue..

@coveralls

This comment has been minimized.

Copy link

commented Feb 13, 2017

Coverage Status

Coverage increased (+0.1%) to 77.005% when pulling 93ad522 on bugfix/create-revisions into 7910d6b on develop.

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator

commented Feb 13, 2017

@westonruter I wasn't able to finish it today, and I am going on a leave till next Tuesday, so may be @PatelUtkarsh can continue on this PR or I can do it when I come back. 👋

@coveralls

This comment has been minimized.

Copy link

commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.824% when pulling 4b69f1f on bugfix/create-revisions into 7910d6b on develop.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

commented Feb 24, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.824% when pulling 4b69f1f on bugfix/create-revisions into 7910d6b on develop.

@coveralls

This comment has been minimized.

Copy link

commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.824% when pulling d3166d4 on bugfix/create-revisions into 7910d6b on develop.

@coveralls

This comment has been minimized.

Copy link

commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.824% when pulling 7913f93 on bugfix/create-revisions into 7910d6b on develop.

@coveralls

This comment has been minimized.

Copy link

commented Feb 25, 2017

Coverage Status

Coverage decreased (-0.04%) to 76.824% when pulling a268785 on bugfix/create-revisions into 7910d6b on develop.

@sayedtaqui sayedtaqui changed the title [WIP] Ensure that revisions get created when changesets are updated via explicit saves Ensure that revisions get created when changesets are updated via explicit saves Feb 25, 2017

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2017

@westonruter Ready for review.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

@sayedtaqui On 4.6 I'm getting an error if I Save a snapshot, then make a change, and then hit Update. A modal pops open with “The snapshot could not be saved”, with the Ajax request returning with a bad_schedule_time error.

@coveralls

This comment has been minimized.

Copy link

commented Feb 28, 2017

Coverage Status

Coverage increased (+0.02%) to 76.878% when pulling 383787c on bugfix/create-revisions into 7910d6b on develop.

@westonruter westonruter merged commit 6438739 into develop Feb 28, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the bugfix/create-revisions branch Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.