Sync: listen for status changes on Woo product reviews #13079

roccotripaldi · 2019-07-17T19:29:19Z

Fixes an issue where a product review status ( approved / unapproved ) appear incorrectly on Wordpress.com

Changes proposed in this Pull Request:

Makes the list of comment types filterable
WooCommerce sync module adds 'review' to filter

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

See: p99K0U-1mi-p2

Testing instructions:

Run this branch on a connected Woo commerce site
Add a product review to one of your products, and approve it
Check the cache site and ensure the status of the review/comment is approved
Unapprove the review/comment
Check the cache site and ensure the status of the review/comment is unapproved

Proposed changelog entry for your changes:

Fixes an issue where a product review status ( approved / unapproved ) appear incorrectly on Wordpress.com

matticbot · 2019-07-17T19:29:31Z

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

jetpackbot · 2019-07-17T19:31:21Z

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: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against be275db

roccotripaldi · 2019-07-17T20:42:05Z

I'd love to add a unit test or two here, but i cannot figure out how to run Woo tests on my docker setup.

timmyc · 2019-07-18T00:40:54Z

@roccotripaldi I'll give this a test tomorrow, sorry I didn't get to it today.

tyxla · 2019-07-18T08:51:26Z

I'd love to add a unit test or two here, but i cannot figure out how to run Woo tests on my docker setup.

I've added a simple test that makes sure that registering a custom comment type works from the point of view of the Comments sync module.

We could add another one for WooCommerce, but it doesn't seem that the current tests are working. I'll work on a fix for them, and add another test case afterwards.

tyxla

This appears to be working great! I only have a suggestion regarding naming, which I'm just going to address directly, as I'm already touching the branch.

packages/sync/src/modules/Comments.php

tyxla · 2019-07-18T08:06:45Z

packages/sync/src/modules/WooCommerce.php

+	 * return array                The list of comment types with 'review' added.
+	 */
+	public function add_review_comment_types( $comment_types ) {
+		if ( is_array( $comment_types ) ) {


Is this check necessary? I've noticed we don't do these on other filters that work in a similar way.

packages/sync/src/modules/Comments.php

tests/php/sync/test_class.jetpack-sync-comments.php

tyxla · 2019-07-18T09:04:12Z

I've suggested a fix for fixing the WooCommerce tests in #13080. Cherrypicked it here in b376c36

Going to work on adding another test now.

tyxla · 2019-07-18T09:10:49Z

Also, @roccotripaldi in case you were wondering how to run the Woo tests, considering they're fixed (in this branch or in #13080):

Make sure you have the latest WooCommerce git repo checked out in /docker/wp-content/plugins/woocommerce, and WooCommerce is built (composer install, npm install, npm run build in the woo dir does the job).
Navigate to the Jetpack repo root.
Run yarn docker:sh
While in the shell, run cd wp-content/plugins/jetpack
Run JETPACK_TEST_WOOCOMMERCE=1 phpunit --filter WP_Test_Jetpack_Sync_WooCommerce

Also documented in the testing instructions of #13080.

tyxla · 2019-07-18T09:26:13Z

@roccotripaldi @timmyc in 46ca0c6 I've just added a couple of test cases that verify that approving and unapproving a review is being synced as expected.

tyxla · 2019-07-18T09:27:11Z

This is looking good from my perspective 👍 But since I touched the branch quite a bit, let's get someone else from Crew/Poseidon to review it.

jeherve

I like it. This will actually fix #12830 as well, since the Action Scheduler comments use a custom comment type, action_log. Here is an example:

+----------------------+---------------------+
| Field                | Value               |
+----------------------+---------------------+
| comment_ID           | 139                 |
| comment_post_ID      | 702                 |
| comment_author       | ActionScheduler     |
| comment_author_email |                     |
| comment_author_url   |                     |
| comment_author_IP    |                     |
| comment_date         | 2019-07-17 22:53:21 |
| comment_date_gmt     | 2019-07-17 20:53:21 |
| comment_content      | action created      |
| comment_karma        | 0                   |
| comment_approved     | 1                   |
| comment_agent        | ActionScheduler     |
| comment_type         | action_log          |
| comment_parent       | 0                   |
| user_id              | 0                   |
+----------------------+---------------------+

I'll update that other PR to only include the CPT blacklist.

…eduler" This reverts commit f481a2b. No need for this exception once #13079 will be merged.

roccotripaldi · 2019-07-18T15:38:33Z

Rebasing after merging #13080, i am able to run the new unit tests, and they pass!
This still works as described with smoke testing too.

I think it's ready!

gititon

Looks good!

* Sync: sort blacklisted post types alphabetically * Sync: add Action Scheduler to the list of blacklisted post types Fixes #12714 Those events do not need to be synced with WordPress.com. * [not verified] Sync: do not sync comments made via Action Scheduler * Revert "[not verified] Sync: do not sync comments made via Action Scheduler" This reverts commit f481a2b. No need for this exception once #13079 will be merged.

* Add initial changelog / testing list changes for 7.6 * Update stable tag to 7.5.3 * changelog: add #12957 * Changelog: add #12932 * Changelog: add #12867 * Changelog: add #12823 * changelog: add #12969 * changelog: add #13012 * changelog: add #12974 * Changelog: add #13059 * Changelog: add #13079 * Changelog: add #12924 * changelog: add #12954 * Changelog: add #12959 * Changelog: add #12977 * Changelog: add #12830 * Changelog: add #12926 * Changelog: add #12958 * Changelog: add #12999 * Changelog: add #13077 * Changelog: add #13083 * Changelog: add #13087 * Changelog: add #13110 * Changelog: add #13116 * Changelog: add #13117 * Changelog: add #12821 * Changelog: add #13120 * changelog: add #13139 * Changelog: add #13143 * Changelog: add #13147 * Testing list: add section about sync

roccotripaldi added [Type] Bug When a feature is broken and / or not performing as intended [Status] In Progress [Package] Sync labels Jul 17, 2019

roccotripaldi added this to the 7.6 milestone Jul 17, 2019

roccotripaldi requested a review from a team July 17, 2019 19:29

matticbot added the Touches WP.com Files label Jul 17, 2019

matticbot removed the Touches WP.com Files label Jul 17, 2019

roccotripaldi added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jul 17, 2019

roccotripaldi requested review from a team and timmyc July 17, 2019 20:43

tyxla reviewed Jul 18, 2019

View reviewed changes

tyxla mentioned this pull request Jul 18, 2019

Unit Tests: Fix WooCommerce tests bootstrap #13080

Merged

tyxla requested review from a team July 18, 2019 09:27

tyxla assigned roccotripaldi and tyxla Jul 18, 2019

jeherve previously approved these changes Jul 18, 2019

View reviewed changes

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 Jul 18, 2019

jeherve added a commit that referenced this pull request Jul 18, 2019

Revert "[not verified] Sync: do not sync comments made via Action Sch…

9129c5a

…eduler" This reverts commit f481a2b. No need for this exception once #13079 will be merged.

jeherve mentioned this pull request Jul 18, 2019

Sync: add Action Scheduler to the list of blacklisted post types #12830

Merged

This was referenced Jul 18, 2019

Reviews: Marking as unapproved does not update the label woocommerce/woocommerce-ios#612

Closed

Invalidate a Note after un-approving a notification woocommerce/woocommerce-ios#1031

Closed

roccotripaldi and others added 5 commits July 18, 2019 11:32

Ensure we listen for status changes on woo reviews

cae1369

No need to change yarn.lock

1d4b0c9

Add a simple test to assure custom comment types are registered

f40c0fc

Rename "hookable" to "whitelisted"

27565d3

Add test cases for syncing review approval and unapproval

be275db

roccotripaldi dismissed jeherve’s stale review via be275db July 18, 2019 15:32

roccotripaldi force-pushed the fix/woo-commerce-sync-comment-status branch from 46ca0c6 to be275db Compare July 18, 2019 15:32

jeherve approved these changes Jul 18, 2019

View reviewed changes

roccotripaldi merged commit 4924b0f into master Jul 18, 2019

gititon approved these changes Jul 18, 2019

View reviewed changes

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 18, 2019

roccotripaldi deleted the fix/woo-commerce-sync-comment-status branch July 18, 2019 15:58

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Jul 19, 2019

jeherve added a commit that referenced this pull request Jul 19, 2019

Changelog: add #13079

feab5c9

jeherve mentioned this pull request Aug 28, 2019

Activity Log: ActionScheduler actions on the Activity page #13354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync: listen for status changes on Woo product reviews #13079

Sync: listen for status changes on Woo product reviews #13079

roccotripaldi commented Jul 17, 2019 •

edited

Loading

matticbot commented Jul 17, 2019

jetpackbot commented Jul 17, 2019 •

edited

Loading

roccotripaldi commented Jul 17, 2019

timmyc commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla left a comment

tyxla Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

jeherve left a comment

roccotripaldi commented Jul 18, 2019

gititon left a comment

Sync: listen for status changes on Woo product reviews #13079

Sync: listen for status changes on Woo product reviews #13079

Conversation

roccotripaldi commented Jul 17, 2019 • edited Loading

Changes proposed in this Pull Request:

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

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jul 17, 2019

jetpackbot commented Jul 17, 2019 • edited Loading

roccotripaldi commented Jul 17, 2019

timmyc commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla left a comment

Choose a reason for hiding this comment

tyxla Jul 18, 2019

Choose a reason for hiding this comment

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

tyxla commented Jul 18, 2019

jeherve left a comment

Choose a reason for hiding this comment

roccotripaldi commented Jul 18, 2019

gititon left a comment

Choose a reason for hiding this comment

roccotripaldi commented Jul 17, 2019 •

edited

Loading

jetpackbot commented Jul 17, 2019 •

edited

Loading