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

Fix snapshot publish issue after settings save #89

Merged
merged 3 commits into from Aug 31, 2016

Conversation

Projects
None yet
4 participants
@PatelUtkarsh
Copy link
Collaborator

commented Aug 31, 2016

No description provided.

@coveralls

This comment has been minimized.

Copy link

commented Aug 31, 2016

Coverage Status

Coverage increased (+0.02%) to 90.968% when pulling ee51770 on bugfix/snapshot-publish-issue into 7e7e048 on develop.

if ( ! empty( $result['errors'] ) ) {
add_filter( 'customize_save_response', function( $response ) use ( $result, $that ) {
$response['snapshot_errors'] = $that->prepare_errors_for_response( $result['errors'] );
return $response;
} );
return false;
return false; // Todo should remove?

This comment has been minimized.

Copy link
@westonruter

westonruter Aug 31, 2016

Contributor

I think the reason for having this return false here is for the sake of unit testing.

@PatelUtkarsh PatelUtkarsh changed the title [WIP] Fix snapshot publish issue after settings save Fix snapshot publish issue after settings save Aug 31, 2016

@coveralls

This comment has been minimized.

Copy link

commented Aug 31, 2016

Coverage Status

Coverage increased (+0.02%) to 90.968% when pulling 2b77775 on bugfix/snapshot-publish-issue into 7e7e048 on develop.

@lgedeon

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

@PatelUtkarsh It looks like this is going to block concurrency conflict checking, but maybe I can find a different way to trigger it in this case.

Actually, @westonruter I am not sure if you want to check for conflicts between what the snapshot is publishing and what has gone live since the snapshot was started. It could be really helpful in some cases but somewhat annoying in other cases.

@lgedeon

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

On second thought, this use case would be better if we used the warning interface that you are using between two different snapshots instead of the block method that I use inside the same snapshot. So we can probably leave this code as is.

[Update: Removed]

@westonruter

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

@lgedeon

It looks like this is going to block concurrency conflict checking, but maybe I can find a different way to trigger it in this case.

Actually, it shouldn't. Note that it's only bypassing the validation when transitioning a snapshot to publish immediately after the customizer settings have been saved. So this is the situation where you're in the customizer, you have a snapshot there, and you hit publish. The validation is going to run as normally for the customizer save process. What we're doing here is bypassing the validation from happening a second time at customize_save_after. So it should still work fine.

@coveralls

This comment has been minimized.

Copy link

commented Aug 31, 2016

Coverage Status

Coverage increased (+0.02%) to 90.968% when pulling c0b4539 on bugfix/snapshot-publish-issue into 7e7e048 on develop.

@westonruter westonruter added this to the 0.6.0 milestone Aug 31, 2016

@westonruter westonruter merged commit 010daf7 into develop Aug 31, 2016

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/snapshot-publish-issue branch Aug 31, 2016

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.