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

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: March 5, 2019.
Scheduled code freeze: February 26, 2019

Generated by 🚫 dangerJS against fa9561c

codebykat, Your synced wpcom patch D24043-code has been updated.

Code looks good to me, nice work @codebykat!

However, I get an incorrect value for the jetpack_likes_enabled field under these circumstances:

• Sharing and Like buttons are enabled in Jetpack > Settings:

• Sharing and Likes are disabled in a specific post (I created this post after enabling the global settings):

When I request the post data via the API, jetpack_likes_enabled is true. I expected to see a false value since the "Show likes" checkbox is unticked. I got the same results using either Gutenberg or the Classic editor.

Note that I have D24043-code applied with the API sandboxed and I run Jetpack locally (with docker + ngrok).

I also wanted to point out that since #10600, we also have WPCOM_REST_API_V2_Field_Controller as a wrapper for register_rest_field

@jeherve is this a non-blocking comment?

is this a non-blocking comment?

Yup, just pointing it out for reference, and for our future selves, as it may be more convenient in some scenarios. :)

I've tested by applying this branch to my local Docker+ngrok setup, and then queried using the developer console (seemed to work well!) to get a single test post (GET wp/v2/sites/:siteId/posts/:postId).

Changing the global settings result in the fields being present or not (eg. the setting being off meant the field would not be in the API result, as opposed to being present and false – I assume this is the desired behaviour?). Changes to individual posts worked as expected for Sharing, but not for Likes; I couldn't get jetpack_likes_enabled: false, it was always true (or not present). This sounds like the quirk you mentioned?

Ah sounds like there might be a bug with Likes -- I wonder if the filter is wrong on Jetpack? Investigating.

codebykat, Your synced wpcom patch D24043-code has been updated.

And yes on Jetpack disabling the global setting will not load the module, so the API key won't be present. We'd only expect to see a value of false when the global setting is enabled but the per-post setting is toggled off.

This is really close @codebykat! I think I'm seeing the same behavior while testing that @mmtr saw in #11298 (comment).

Basically sharing appears to be reflected properly, but when toggling the likes property on a post it's never false for me, unless I modify the likes setting under sharing buttons in Calypso:

codebykat, Your synced wpcom patch D24043-code has been updated.

I FOUND IT and it's REALLY AWFUL.

On Jetpack the switch_likes_setting boolean meta is treated as a TERNARY option. So it is either unset (get_post_meta returns an empty string), true, or false. In the case where likes are disabled globally and the user wants to enable them on a particular post, the meta is set to true. Conversely, if likes are enabled globally and disabled on a particular post, the meta is set to false. If the meta is not set at all, we follow the global setting.

This is actually somewhat great in that it solves the problem of what happens when the global site setting gets changed. On Jetpack, an unset value means "No overrides -- follow the global setting." Whereas a set value allows us to determine what the user originally intended to happen, and proceed accordingly.

On the front-end, the check to display like buttons utilizes empty(), which is kind of genius as it resolves to false in the case of both false and unset.

On WPCOM, where we simply set the meta to true if the user wants to override the global setting, there is a bug when the global setting is changed.

However, it is of course terrible that this boolean currently means different things on different platforms.

I updated this PR with a POC solution that does different logic depending on whether this is a Jetpack or WPCOM environment, but tomorrow I plan to see if I can easily bring the WPCOM behavior in line with the Jetpack behavior and thus be able to simplify things again.

After thinking about this a little, here's the issue on WPCOM that we'll need to either solve or accept that it will break for some users:

Current status on WPCOM:

likes enabled globally,

• a post with likes enabled => no meta
• a post with likes disabled => meta=1

likes disabled globally,

• a post with likes enabled => meta=1
• a post with likes disabled => no meta

THEN: we change the behavior on WPCOM to follow Jetpack. So now:

likes enabled globally,

• a post with likes enabled => no meta => no change 👍
• a post with likes disabled => meta=1 => likes are now enabled erroneously ❌

likes disabled globally,

• a post with likes enabled => meta=1 => likes are enabled, correctly ✅
• a post with likes disabled => no meta => no change 👍

I also need to test more and verify whether this is different on wp-admin and Calypso; I suspect it's an API bug (as the metabox code is shared) and thus may be Calypso-only.

Also noting this patch doesn't apply cleanly on WPCOM because I accidentally reverted the register_meta addition on the WPCOM side. Which doesn't actually matter since this code removes that call anyway, but I'm not sure what the best thing is to do wrt the automated tests.

BUG FOUND AND FIXED. I also added explicit returns and schema after looking at some of the example code for the API.

Thanks @codebykat! However I still see the weird behavior with the POST requests (and only with the POST requests). Code looks good to me, and the GET requests return the correct values so I don't consider this issue a blocker.

Actually, I'm starting to think that it might be a core issue caused by making a POST request without updating the new fields.

Also noting this patch doesn't apply cleanly on WPCOM because I accidentally reverted the register_meta addition on the WPCOM side. Which doesn't actually matter since this code removes that call anyway, but I'm not sure what the best thing is to do wrt the automated tests.

I think you can go ahead and manually update the diff so the changes are synced.

@mmtr Hmm. I tested this from the API console, and my fixes from last night did make the correct values show up from both POST and GET requests. Can you share the specific reproduction steps and whether you have Like buttons enabled or disabled globally? Also, are you seeing this with the sharing value or just likes?

I think you can go ahead and manually update the diff so the changes are synced.

Yeah, I did that, but it's showing up in the "Some checks were not successful" box because it couldn't sync it over. I don't know whether or not that blocks merging.

@mmtr Hmm. I tested this from the API console, and my fixes from last night did make the correct values show up from both POST and GET requests. Can you share the specific reproduction steps and whether you have Like buttons enabled or disabled globally? Also, are you seeing this with the sharing value or just likes?

I get the results while testing on a Jetpack site and updating a post from the UI as you can see on the GIF I shared above:

• Go to Posts > Add new
• Enter some content and publish the post or save it as draft
• Toggle any of the Likes/Shares checkboxes
• Change some content in the post and update it
• Open the browser devtools and go to the Network tab
• Check the response of the POST request to the API that is triggered when we update the post

Hm, what actually updates the values in the db? I can't tell; in fact, I can't get the code in jetpack_post_likes_update_value to execute at all. And I think @mmtr and I are seeing what we're seeing because we're both looking at POST calls to :domain/wp-json/wp/v2/posts/272?_locale=user, which doesn't seem to trigger the update at all, it just does a read of those values via jetpack_post_likes_get_value (likely before the actual update mechanism does its thing).

I tested this from the API console, and my fixes from last night did make the correct values show up from both POST and GET requests.

@kat what is the API call you're making to see the correct results? Is it the same as the POST one above? I wonder if the confusion is that we're all looking at different things 🙃

On a sandboxed WPCOM site, POST wp/v2/sites/sandboxed.wpcom.site/posts/post_ID with the payload jetpack_likes_enabled set to 0 or 1.

Does this happen for y'all on WPCOM sites or just on Jetpack? The logic should be the same 🤔

I think you're right @kwight that the update does call GET to retrieve the new value, but only after running the update code. Is there possibly a missing piece in having it sandboxed on the WPCOM API side as well? I realized I've been testing with the patch applied in both places.

Also can you duplicate the issue with the sharing setting, or is it just Likes?

Ah OK, I think we've been testing two different ways.

@mmtr and I have been changing values in the UI, then hitting Update; this fires the POST call, but the Likes and Shares values are not updated by this call, they're updated after it in the meta-box-loader call which does not go through the API (it's a POST to wp-admin/post.php). This is why the return values we're seeing are out-of-date, and I can't get code in the update part of this PR to fire.

@kat you've been making a direct API call with the fields themselves directly in the body of the POST, meaning they are handled by this PR's jetpack_post_likes_update_value function. Done this way, the return value is correct, as it should be.

TL;DR, this PR is good to go, as far as I'm concerned :)

Does this happen for y'all on WPCOM sites or just on Jetpack?

I've just been testing this PR against a local ngrokked JP site, so I haven't had the API or anything sandboxed. From what I can tell, these calls hit the site directly, and don't have anything to do with public-api.wordpress.com.

As far as the outdated values in the Update API call, I don't consider them a blocker/problem here; if anything, we'd want to follow up with a separate PR that might involve hooks somewhere other than the API.

@jeherve Anything else we need to merge this, beyond one of you to approve? We've forced the wpcom check to pass, because syncing needed manual intervention dotcom-side.

With everyone else approving, let me give it a quick last look.

@codebykat @kwight Please merge this at will when y'all are ready.

Excellent shepherding and bugfixing @codebykat ! 🎉

💖 from the great code reviews here too!