Don't sync invisible products to MC

[nima-karimi]

Changes proposed in this Pull Request:

This PR modifies the ProductHelper::is_sync_ready method so that it also checks the product's visibility. In addition to the previous conditions, it now returns false for a product when:

• woocommerce_hide_out_of_stock_items option is enabled and the product is out of stock
• If it's an invisible variation (i.e. have no price or it's disabled)

The WooCommerce core woocommerce_hide_invisible_variations filter is also used here to check whether the variation should be displayed even if it's invisible.

These conditions were already checked for variations in SyncerHooks class and a few other places where the method accepts a variable product and tries to fetch its "available" variations via the \WC_Product_Variable::get_available_variations method. By adding these conditions to ProductHelper::is_sync_ready we'll also check for the "availability" (or visibility) of all other product types just like we do for variations and make things more consistent.

One major effect of this change is that if the user has enabled the woocommerce_hide_out_of_stock_items we no longer sync the out-of-stock products to Merchant Center and also remove the previously synced products once the product goes out of stock.

However, if the user already has some out-of-stock products and then enables the woocommerce_hide_out_of_stock_items option, we do not remove anything. This should probably be done by watching the WooCommerce events and trigger a refresh job if any settings change. I didn't include this part in the PR because we also don't do this for any other settings changes (e.g. country, language, etc.) and I first wanted to discuss whether we need to.

Detailed test instructions:

1. Create an out-of-stock product
2. Check the Out of stock visibility option under "WooCommerce > Settings > Products > Inventory"
3. Sync the out of stock product via Connect Test page and check that it's NOT synced to MC
4. Uncheck the Out of stock visibility option under "WooCommerce > Settings > Products > Inventory"
5. Sync the out of stock product via Connect Test page and check that it's synced to MC
6. Repeat the above with a variation product
7. Disable a variation by unchecking the "Enabled" option
8. Sync it via Connect Test page and check that it's NOT synced to MC
9. Remove the variation price
10. Sync it via Connect Test page and check that it's NOT synced to MC
11. Add the following filter and then disable a variation

    add_filter( 
        'woocommerce_hide_invisible_variations', 
        function () {
            return false;
        }
    );
    

12. Sync it via Connect Test page and check that it's synced to MC
13. Run phpunit and make sure all the tests pass

Changelog entry

Update - Stop syncing invisible products and variations

[mikkamp]

I see @layoutd was already checking this PR. But on regular products why aren't we using the is_visible function? The only part that might be iffy is the current_user_can edit, although if we are also checking the status to be publish then that should be fine. Otherwise we'll be missing out on the woocommerce_product_is_visible filter.

[nima-karimi]

is_visible function

Yeah, I first wanted to use that one but the issue is that the is_visible method also checks the product's catalog_visibility property and returns false if that is set to search or hidden.

Since we have our own channel_visibility option, I figured these two might conflict. For example when someone wants to hide a product from their shop but sync it to Google. It might be a bit confusing to have both channel visibility and catalog visibility affect the syncing status.

I guess the question is do we want to sync products based on the "catalog visibility" option?

cc: @j111q

[layoutd]

Looks good 👍🏻 . The test instructions all worked as expected and, when I finally got PHP back up and running, the unit tests did, too.

Should be ready to merge, pending the final decision on catalog_visibility.

[j111q]

Since we have our own channel_visibility option, I figured these two might conflict. For example when someone wants to hide a product from their shop but sync it to Google. It might be a bit confusing to have both channel visibility and catalog visibility affect the syncing status.
I guess the question is do we want to sync products based on the "catalog visibility" option?

Please hide it if it's set to Hidden on catalog visibility!

I have not heard a use case where they'd want to hide from their store but show on Google. That would be really confusing for users coming from Google to their store too.

I have heard users mention how they've gone to lengths such as: Setting to Hidden in Catalog, AND turning it into a Draft, AND setting it as out of stock just to make sure it doesn't appear anywhere at all.

[nima-karimi]

Thanks for the review, @layoutd, and for your inputs, @mikkamp, and @j111q 🙏 I've modified the code to also account for catalog visibility when checking if a product is sync-ready (by making use of the \WC_Product::is_visible method). This means that if a product's catalog visibility is set to hidden or search we won't sync it to Google.

@j111q Should we still allow a product's channel visibility to be set to Sync & Show or even display the Channel Visiblity meta box in the case the product's catalog visibility is set to hidden or search? I think this might be confusing.

[j111q]

@j111q Should we still allow a product's channel visibility to be set to Sync & Show or even display the Channel Visiblity meta box in the case the product's catalog visibility is set to hidden or search? I think this might be confusing.

In my mind, ideally we should:

• Automatically set the product's channel visibility for Google to Don't sync and show
• Show-but-disable the select area in the Channel visibility meta box,
• Display a message that informs the user why it's disabled

Screenshot:

Is this possible to do, or is that too complicated? 😅 I do realise it expands the scope of this issue quite a bit.

[nima-karimi]

I've made some changes to disabled the channel visibility select box if the product isn't visible:

Since WooCommerce doesn't provide a JS event when the catalog visibility value is modified (and I didn't want to bind on any inputs since they may change), the select box is disabled on the backend side, which means that if the catalog visibility is modified it would not enable the channel visibility unless the product is saved and the page is reloaded.

Modifying the Channel Visibility value to "Don't Sync & Show" also means that this value is saved to DB once the user saves a product. This means that if the user changes the catalog visibility value back to visible but forgets to change the Channel Visibility value to "Sync & Show" we will still not sync that product.

@layoutd Could you please give the new changes another review/testing?

Testing Instructions:

1. Set a product's catalog visibility to "Hidden" or "Search results only" and save the product
2. The product should not be synced to Google and if it was synced before, it should now be deleted from Merchant Center
3. The Channel Visibility meta box should now be disabled on the edit product page and is similar to the design shared by @j111q
4. The value of the channel visibility select box should be set to "Don't Sync & Show"
5. Modify the catalog visibility value and set it to either "Shop and search results" or "Shop only" and update the product
6. The Channel Visibility value should still be set to "Don't Sync & Show"
7. Change the Channel Visibility value to "Sync & Show" and update the product
8. Make sure the product is synced to Merchant Center

[layoutd]

👍🏻 The latest changes work fine and the catalog visibility is also taken into account when syncing (along with stock quantity and the out of stock visibility setting).

Small nit: I noticed that the "Google sync status" box is still visible sometimes when the GLA channel visibility is forced to Don't Sync and show :

This happens if:

• A product is set to visible in the catalog (Shop and search results, or Shop only), the GLA channel visibility is Sync and show, and the product is saved.
• Then the catalog visibility is set to hidden (Search results only or Hidden) and the product is saved.

The GLA channel visibility box is then disabled correctly, but I believe the actual postmeta value is still sync-and-show, so the "Google sync status - Not synced" message is still displayed.

This isn't the case if the product is manually set to Don't Sync and show first, before the channel visibility is changed to hidden:

I think adding $show_status = false in the is_visible() conditional would resolve it:
https://github.com/woocommerce/google-listings-and-ads/pull/880/files#diff-64e0d8a3cf27a82c2419d94905e0e75e4c317c431cee268733ff969feacf6573R48-R52

[nima-karimi]

Nice catch. Thanks, @layoutd 🙏 This should be fixed by aade5f2

[j111q]

Modifying the Channel Visibility value to "Don't Sync & Show" also means that this value is saved to DB once the user saves a product. This means that if the user changes the catalog visibility value back to visible but forgets to change the Channel Visibility value to "Sync & Show" we will still not sync that product.

Yes, this is fine IMO. Thanks for this @nima-karimi!