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 sharing buttons and like buttons when copying a post #14291

Merged
merged 3 commits into from
Jan 9, 2020

Conversation

scottsweb
Copy link
Contributor

Looks like we are explicitly setting sharing_disabled and switch_like_status meta despite the original post relying on the global setting for these to show. It is creating some inconsistencies as per: #14117

Fixes #14117

Changes proposed in this Pull Request:

  • This PR attempts to better handle missing meta values on the post being copied

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Bug fix

Testing instructions:

From the issue:

  • Make sure the Copy Post module in Jetpack is activated, and that the Classic Editor plugin is not active.
  • At WP-Admin ->Posts, make sure you have a post with sharing and Like buttons enabled, and check the buttons are visible in the published post on the front end
  • Use the Copy Post feature to create a new post
  • Before publishing, check the Jetpack sidebar and make sure sharing and Like buttons are enabled
  • Publish the copied post
  • Verify if the sharing and like buttons respect those of the original post and are output on the front end

Note: Even if wp-admin is showing the correct values for these toggles, we have been seeing different results in the theme on the front end, so check both places.

Digging deeper. The meta data before the fix is applied:

Original post:

Screenshot 2020-01-02 at 16 41 57

Copied post:

Screenshot 2020-01-02 at 16 45 16

The meta data after the fix is applied:

Original post:

Screenshot 2020-01-02 at 17 03 41

Copied post:

Screenshot 2020-01-02 at 17 04 50

Ideally switch_like_status would not have been set at all in this scenario - but it does seem to work. I think we might need something better than the isset check.

Proposed changelog entry for your changes:

  • Bug fix: Ensure correct sharing and like settings are copied when posts are duplicated

@scottsweb scottsweb added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Copy a Post labels Jan 2, 2020
@scottsweb scottsweb requested a review from a team January 2, 2020 17:14
@scottsweb scottsweb self-assigned this Jan 2, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello scottsweb! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37227-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Jan 2, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 93eed8c

@kraftbj kraftbj added this to the 8.2 milestone Jan 2, 2020
kraftbj
kraftbj previously requested changes Jan 2, 2020
modules/copy-post.php Outdated Show resolved Hide resolved
@kraftbj kraftbj added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 2, 2020
@matticbot
Copy link
Contributor

scottsweb, Your synced wpcom patch D37227-code has been updated.

@scottsweb
Copy link
Contributor Author

scottsweb commented Jan 3, 2020

I just pushed a fix, thanks for your suggestion @kraftbj - it looks to have worked.

Before:

Screenshot 2020-01-03 at 09 35 49

After:

Screenshot 2020-01-03 at 09 36 23

So I think the original issue can be marked as resolved.

I do however have a lingering concern, when the settings for sharing and likes are set to 'on' for all new posts and pages, I went into a specific post and turned them off. This gave me the following meta:

Screenshot 2020-01-03 at 09 39 53-bugs

It is a shame that sharing_disabled and switch_like_meta are the inverse of each other (both 0 an 1 meaning the option is turned off). It does work as expected though.

When copying in this state though, the meta on the copied post is very different:

Screenshot 2020-01-03 at 09 39 38-bugs

sharing_disabled becomes a serialised value, which also seems like a possible bug.

		$sharing = get_post_meta( $source_post->ID, 'sharing_disabled', false );

Should probably become:

		$sharing = get_post_meta( $source_post->ID, 'sharing_disabled', true );

and then we can perform the same check for sharing '' !== $sharing

Do you see that having any other negative impacts?

@scottsweb scottsweb requested a review from kraftbj January 3, 2020 09:53
@scottsweb scottsweb added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 3, 2020
@jeherve
Copy link
Member

jeherve commented Jan 8, 2020

sharing_disabled becomes a serialised value, which also seems like a possible bug.

It does indeed seem like a bug, and I like your suggestion for a fix.

@matticbot
Copy link
Contributor

scottsweb, Your synced wpcom patch D37227-code has been updated.

@scottsweb
Copy link
Contributor Author

@jeherve fix has been pushed:

Original:

Screenshot 2020-01-08 at 16 17 50

Copy:

Screenshot 2020-01-08 at 16 18 28

Values stay as boolean.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work well in my tests. Should be good to merge.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 9, 2020
@jeherve jeherve dismissed kraftbj’s stale review January 9, 2020 10:51

Changes were made based on feedback.

@scottsweb scottsweb merged commit 69b3bb0 into master Jan 9, 2020
@scottsweb scottsweb deleted the fix/14117-fix-copy-post-meta branch January 9, 2020 17:09
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jan 9, 2020
@scottsweb
Copy link
Contributor Author

r201430-wpcom

jeherve added a commit that referenced this pull request Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Copy a Post Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy Post: Sharing buttons don't appear if post is copied using block editor
5 participants