-
Notifications
You must be signed in to change notification settings - Fork 19
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
Save autoshare meta from post form before publishing tweet #82
Conversation
Confirmed all looks good on our side. Packaging up for client bugfix and will follow up if they catch anything. |
No reported issues after pushing to client site as hotfix and confirmed Twitter timeline looks good. Thanks @johnwatkins0 . I think we can safely add to next release. |
@johnwatkins0 are there any tests that we can add to cover this to avoid a regression back to this behavior later? |
@jeffpaul Yes, we could definitely increase test coverage. I'll look into adding a unit test for this particular PR later. I'd also like to try #19 to see how well WPAcceptance can cover this sort of thing, but that should probably be a separate task because it will involve setting up acceptance tests and probably adding more than just this one. |
@rickalee @johnwatkins0 did either of you want/need to ship a v1.0.2 for this or hold and include in a larger v1.1.0 later? |
return; | ||
} | ||
|
||
$form_data = sanitize_autoshare_for_twitter_meta_data( | ||
// Using FILTER_DEFAULT here as data is being passed to sanitize function. | ||
filter_input( INPUT_POST, META_PREFIX, FILTER_DEFAULT, FILTER_REQUIRE_ARRAY ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broke this out into a function (see below) for easier testing.
I added some tests and also did some cleanup around meta saving. Sorry about the growing diff, @dinhtungdu . I'm aiming to use my extra time over the next few days to set up WP Acceptance ahead of a 1.1.0 release, but I'll work on that in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked great in my tests! 🎉
Description of the Change
This resolves a bug brought up by @rickalee in #80. Autoshare form data is saved in
save_post
while the tweet action runs ontransition_post_status
.transition_post_status
runs earlier thansave_post
, so the bug happened in the case where:In this case, the enabled status has not been saved before the tweet function runs, so the post is tweeted.
This update causes the form data to be processed before trying to tweet.
Alternate Designs
I'm wanting to refactor large parts of this plugin, but I'm holding for now and will aim to submit issues outlining the specific technical debt I'm seeing.
Benefits
Tweets won't be inadvertently published during the specific scenario outlined above.
Possible Drawbacks
Verification Process
Checklist:
Applicable Issues
Resolves #80
Changelog Entry
Fix a bug that caused posts to be inadvertently tweeted when switching from draft to publish