Sync WooCommerce Data by nabsul · Pull Request #6478 · Automattic/jetpack

nabsul · 2017-02-22T19:05:12Z

The main goal of this PR is to add support for WooCommerce specific data in the Jetpack Sync modules.

Add filters on the whitelist arrays needed to acheive this
Refactor the constants module to avoid race condition on filtered whitelist
Add WooCommerce constants
Add WooCommerce WordPress options
Add Post Metadata for WooCommerce Orders and Products

Changes proposed in this Pull Request:

Add new filter on default constant whitelist
Add new filter on default post metadata whitelist
Add WooCommerce specific whitelist data (constants, options, post metadata)
Refactor sync constants module to correctly apply filtered whitelist

Testing instructions:

Setup a test site with WooCommerce and test products, settings and orders
Install this version of the plugin
Run sync to observe that new data is included

Proposed changelog entry for your changes:

Better support for WooCommerce data sync and backup

georgestephanis

First pass -- I know it's a work in progress but I figured I'd get a couple things flagged early.

georgestephanis · 2017-02-23T20:55:06Z

sync/class.jetpack-sync-defaults.php

Needs documentation for the filter.

georgestephanis · 2017-02-23T20:55:10Z

sync/class.jetpack-sync-defaults.php

Needs documentation for the filter.

georgestephanis · 2017-02-23T20:55:17Z

sync/class.jetpack-sync-defaults.php

Needs documentation for the filter.

georgestephanis · 2017-02-23T20:57:26Z

sync/class.jetpack-sync-defaults.php

Do we know why this method was here, if not in use? I hesitate to take it out of another plugin we release was somehow depending on it.

georgestephanis · 2017-02-23T20:57:53Z

sync/class.jetpack-sync-module-options.php

This comment's not needed here any longer.

georgestephanis · 2017-02-23T20:59:06Z

3rd-party/woocommerce.php

Wouldn't this be simpler to just use array_merge() instead?

https://secure.php.net/manual/en/function.array-merge.php

georgestephanis · 2017-02-23T20:59:37Z

3rd-party/woocommerce.php

This should return the result.

georgestephanis · 2017-02-23T20:59:39Z

3rd-party/woocommerce.php

This should return the result.

georgestephanis · 2017-02-23T20:59:42Z

3rd-party/woocommerce.php

This should return the result.

georgestephanis

A couple random thoughts -- this is shaping up nicely, happy to see it moving forward!

georgestephanis · 2017-02-27T22:30:48Z

sync/class.jetpack-sync-defaults.php

Should we maybe name the filter jetpack_sync_constants or the like? Whitelist doesn't seem hugely relevant -- as that's the nature of having a list we're filtering -- but including sync in the filter could probably help with clarity.

Possibly not, as this is just matching the existing jetpack_options_whitelist naming pattern ... eh, was just a thought.

Yeah, I originally did call those hooks jetpack_sync_*, but then decided to err on the side of consistency :-)

For consistency, I think we should use jetpack_sync_* since that's what we use throughout the rest of the sync code base within Jetpack.

georgestephanis · 2017-02-27T22:30:58Z

sync/class.jetpack-sync-defaults.php

Same naming concern as ^^

Same as above. I would make this jetpack_sync_post_meta_whitelist.

Alright. But just to be clear, we will still have the pre-exsiting filter called jetpack_options_whitelist. Should I go ahead and also create jetpack_sync_options_whitelist ?

georgestephanis · 2017-02-27T22:34:10Z

sync/class.jetpack-sync-module-woocommerce.php

Optional, but it may be nice to add phpdoc function documentation to these functions -- explaining what list can be, etc. If someone wants to just add a single value, maybe we should have a test in add_to_whitelist or something that just casts the values to arrays before the array_merge() ?

I'll make the function name clearer.

Sometimes phpdocs and comments are necessary, but I'm a big fan of "self-documenting" code whenever possible.

georgestephanis · 2017-02-27T22:34:54Z

sync/class.jetpack-sync-module-woocommerce.php

In usage, above seems to use the new values as the first param, but this seems to expect them as the second? It likely doesn't matter hugely, but may be nice to better sort. Or just have the other methods call array_merge() directly -- it's simple enough.

Yeah, a single-lined function rarely makes sense :-P

ebinnion · 2017-02-28T19:34:19Z

The general approach to this seems fine with me. I agree with @georgestephanis that the filters should be jetpack_sync_* to match other places throughout the sync codebase.

As a note, we'll likely need to whitelist this data on the WPCOM side too, but we can do that after this PR is in.

Publicize and subscriptions seem to work fine and full sync seems to work fine. So, after the filters are updated, this LGTM.

enejb · 2017-02-28T21:57:49Z

Running the phpunit test with the WOO flag I get the following test failures

There were 8 failures:

1) WP_Test_Jetpack_Sync_WooCommerce::test_module_is_enabled
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:63

2) WP_Test_Jetpack_Sync_WooCommerce::test_orders_are_synced
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:74

3) WP_Test_Jetpack_Sync_WooCommerce::test_order_status_changes_are_synced
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:90

4) WP_Test_Jetpack_Sync_WooCommerce::test_order_status_payment_complete_is_synced
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'processing'
+'completed'

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:103

5) WP_Test_Jetpack_Sync_WooCommerce::test_created_order_items_are_synced
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:123

6) WP_Test_Jetpack_Sync_WooCommerce::test_updated_order_items_are_synced
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:142

7) WP_Test_Jetpack_Sync_WooCommerce::test_updated_order_item_meta_is_synced
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:160

8) WP_Test_Jetpack_Sync_WooCommerce::test_full_sync_order_items
Failed asserting that false is true.

/vagrant/www/wordpress-develop/src/wp-content/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-woocommerce.php:189

To run the tests do the following
JETPACK_TEST_WOOCOMMERCE=1 phpunit

enejb · 2017-02-28T22:53:47Z

The tests pass now.

nabsul · 2017-03-01T17:36:49Z

Thanks for the help @enejb !

Concerns addressed

- Add WooCommerce constants - Add WooCommerce WordPress options - Add Post Metadata for WooCommerce Orders and Products - Refactor sync constants module to use filters

- Add jetpack_sync_options_whitelist but keep jetpack_options_whitelist for backwards compat

Since woo was initialising things before the default priority. We were not loading woo plugin just yet.

…_Roles() class

…d the fatal error.

enejb

The changes are good to go.

georgestephanis · 2017-03-06T15:32:20Z

@nabsul Just to confirm, this didn't get merged in in time for our code freeze for the March release, so it will be shipping in our April release instead.

@SInCE

* Readme: remove old release and add skeleton for 4.8. * Changelog: add #6572 * Changelog: add #6567 * Changelog: add #6542 * Changelog: add #6527 * Changelog: add #6508 * Changelog: add #6478 * Changelog: add #6477 * Changelog: add #6249 * Update stable version and remove old version from readme. * Changelog: add 4.7.1 to changelog. * Readme: add new contributor. * Sync: update docblock @SInCE version. Related: #6053 * Changelog: add release post. * changelog: add #6053 * Changelog: add #6413 * Changelog: add #6482 * Changelog: add #6584 * Changelog add #6603 * Changelog: add #6606 * Changelog: add #6611 * Changelog: add #6635 * Changelog: add #6639 * Changelog: add #6684 * Changelog: add #6710 * Changelog: add #6711 * Changelog: add #5461 * Testing list: update Settings UI feedback prompt. Props @MichaelArestad * Changelog: add #6789 * Changelog: add #6778 * Changelog: add #6777 * Changelog: add #6775 * Changelog: add #6755 * Changelog: add #6731 * Changelog: add #6721 * Changelog: add #6705 * Changelog: add #6702 * Changelog: add #6671 * Changelog: add #6637 * Changelog: add #6582 * Changelog: add #6566 * Changelog: add #6555 * Changelog: add #6529 * Changelog: add #6344 * Changelog: add #5763 * Changelog: add #5503 * Changelog: update #6637 changelog. @see 40e115c#commitcomment-21523982 * Changelog: add #6699 * Changelog: add #6632 * Changelog: add #6769 * Changelog: add #6707 * Changelog: add #6590

nabsul added the [Status] In Progress label Feb 22, 2017

nabsul self-assigned this Feb 22, 2017

georgestephanis previously requested changes Feb 23, 2017

View reviewed changes

nabsul force-pushed the test/woocommerce-sync branch 2 times, most recently from 50b48f7 to 3fda084 Compare February 27, 2017 18:03

jeherve added [Package] Sync [Team] Poseidon labels Feb 27, 2017

georgestephanis reviewed Feb 27, 2017

View reviewed changes

nabsul force-pushed the test/woocommerce-sync branch from c196ef6 to 9447d80 Compare February 28, 2017 17:39

nabsul added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Feb 28, 2017

nabsul and others added 7 commits March 3, 2017 09:25

- Add filters for options and post metadata whitelists

effc9f4

- Add WooCommerce constants - Add WooCommerce WordPress options - Add Post Metadata for WooCommerce Orders and Products - Refactor sync constants module to use filters

- Prefix new filters with jetpack_sync_*

92c4cf7

- Add jetpack_sync_options_whitelist but keep jetpack_options_whitelist for backwards compat

Fix the broken Woo Tests.

96a2988

Since woo was initialising things before the default priority. We were not loading woo plugin just yet.

Check if we are running WP 4.7 and if that is the case use the new WP…

1da7fc5

…_Roles() class

Check that Woo testing folder exists. Before running the test to avoi…

e4d6812

…d the fatal error.

Fix failing test.

985fc35

Remove version notice was still there.

33fe8db

enejb force-pushed the test/woocommerce-sync branch from 663fdc3 to 33fe8db Compare March 3, 2017 17:26

enejb self-requested a review March 3, 2017 19:26

enejb approved these changes Mar 3, 2017

View reviewed changes

nabsul merged commit e53ab5c into master Mar 3, 2017

matticbot removed the [Status] Needs Review This PR is ready for review. label Mar 3, 2017

nabsul deleted the test/woocommerce-sync branch March 3, 2017 20:15

jeherve added the [Status] Needs Changelog label Mar 6, 2017

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Mar 9, 2017

jeherve added a commit that referenced this pull request Mar 9, 2017

Changelog: add #6478

7de0c3a

jeherve added a commit that referenced this pull request Mar 28, 2017

Changelog: add #6478

636fc40

Conversation

nabsul commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

georgestephanis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgestephanis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebinnion commented Feb 28, 2017

Uh oh!

enejb commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enejb commented Feb 28, 2017

Uh oh!

nabsul commented Mar 1, 2017

Uh oh!

enejb left a comment

Choose a reason for hiding this comment

Uh oh!

georgestephanis commented Mar 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nabsul commented Feb 22, 2017 •

edited

Loading

enejb commented Feb 28, 2017 •

edited

Loading