WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087

eason9487 · 2023-09-07T06:51:09Z

Changes proposed in this Pull Request:

Closes #2086

Remove the controls' show/hide CSS classes of the product editing page from PHP side.
Control the show or hide of the product data tab and meta box added to the product editing page by JS.
Merge the custom-inputs script into the product-attributes script.

Screenshots:

Kapture.2023-09-07.at.14.37.39.mp4

Detailed test instructions:

Install and activate WooCommerce Subscriptions and WooCommerce Product Bundles.
Restart the client build script (npm start) if it's running.
Go to edit a product.

Select each product type to see if the "Google Listings and Ads" tab and "Channel visibility" meta box are visible for supported product types and hidden for unsupported ones.

Supported product types:

google-listings-and-ads/js/src/product-attributes/index.js

Lines 11 to 18 in f325aa1

    
           const applicableProductTypes = new Set( [ 
        
           	// Simple product 
        
           	'simple', 
        
           	// Variable product 
        
           	'variable', 
        
           	// Product bundle from WooCommerce Product Bundles 
        
           	'bundle', 
        
           ] );

Create a product with the "Simple subscription" type and go to its editing page.
After the page is initialized, check if the UIs mentioned in step 3 are hidden.

Changelog entry

Fix - WooCommerce Subscriptions compatibility: Fix the visible issue of the "Google Listings and Ads" tab and "Channel visibility" meta box for some unsupported product types.

…e from PHP side.

…o the product editing page by JS.

eason9487 · 2023-09-07T06:51:54Z

src/Integration/WooCommerceSubscriptions.php

This integration class doesn't actually participate in any runtime processing anymore now, but I'm not sure if it would be better to remove it entirely.

What would be the downside of removing the file entirely?

Originally I guess it might be ok to keep it as a reminder that this extension doesn't support the product types of WooCommerce Subscriptions. After second thoughts, I think it should be fine to remove it because the current way only uses an inclusive list of the supported product types. Other product types will naturally not be supported.

Removed in d013eaa

codecov · 2023-09-07T06:55:29Z

Codecov Report

Merging #2087 (0468a73) into develop (48ac6ce) will increase coverage by 0.1%.
Report is 18 commits behind head on develop.
The diff coverage is 0.0%.

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2087     +/-   ##
===========================================
+ Coverage       58.3%   58.4%   +0.1%     
+ Complexity      4118    4113      -5     
===========================================
  Files            454     453      -1     
  Lines          17683   17651     -32     
===========================================
  Hits           10304   10304             
+ Misses          7379    7347     -32

Flag	Coverage Δ
php-unit-tests	`58.4% <0.0%> (+0.1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/Admin/Admin.php	`0.0% <0.0%> (ø)`
src/Admin/MetaBox/ChannelVisibilityMetaBox.php	`0.0% <0.0%> (ø)`
src/Admin/Product/Attributes/AttributesTab.php	`0.0% <0.0%> (ø)`
src/Assets/AssetsHandler.php	`0.0% <0.0%> (ø)`
src/Google/GlobalSiteTag.php	`0.0% <0.0%> (ø)`
src/Infrastructure/GoogleListingsAndAdsPlugin.php	`0.0% <ø> (ø)`
...ependencyManagement/IntegrationServiceProvider.php	`0.0% <ø> (ø)`
src/Product/ProductSyncer.php	`98.0% <ø> (+1.3%)`	⬆️

martynmjones

Hey @eason9487, thanks for your work on this issue 🙌

Tested the branch locally with Subscriptions and Product Bundles. The GLA tab and channel visibility meta box now only appear if the product type is simple, variable, or bundle. So LGTM ✅

mikkamp · 2023-09-07T14:46:59Z

js/src/product-attributes/index.js

+const applicableProductTypes = new Set( [
+	// Simple product
+	'simple',
+	// Variable product
+	'variable',
+	// Product bundle from WooCommerce Product Bundles
+	'bundle',
+] );


Just have one additional comment about this list. Previously we had the PHP filter woocommerce_gla_supported_product_types which allows an integration such as Bundles to add themselves to the list. We added this filter not only for the internal integrations we support, but also if there is a plugin which wants to add support for syncing their "specific" product type. With this list it's not possible to extend the supported product types. Can we pass this list dynamically to the JS so it's run through the woocommerce_gla_supported_product_types filter?

Thanks for the suggestion!

Since the original execution order of the related processing is:

AssetsHandler->add_many

AssetsHandler->register

IntegrationInitializer->register and WooCommerceProductBundles->init_product_types

AssetsHandler->enqueue_many

On the first call, the inline script glaData is created along with the script file registration. However, the third call has not yet been made, which results in the supported product types not containing the 'bundle'.

Therefore, I adjusted the execution order as follows in d13cdf6, so that the script registration will be called after the IntegrationInitializer->register.

IntegrationInitializer->register and WooCommerceProductBundles->init_product_types

AssetsHandler->register_many: Combines the former AssetsHandler->add_many and AssetsHandler->register

AssetsHandler->enqueue_many

And removed the former calls of AssetsHandler->add_many and AssetsHandler->register.

Address #2087 (comment)

…e on the front-end side. Address #2087 (comment)

eason9487 · 2023-09-08T10:16:25Z

Hi @mikkamp, thanks for the suggestion.

As there are quite a number of changes made for #2087 (comment), could you take another look at this PR?

…e on the front-end side. Address #2087 (comment)

mikkamp

Thanks for all the changes. I've taken some time to look over it and so far it looks good.

But I just wanted to note that previously we would call:

Asset::register right after init hook. Now it's called directly whenever we call AssetHandler::register for the individual Asset. In the case of the collection of admin assets that should be fine because we call register on the hook admin_enqueue_scripts.

But I'd like to test that a little further also on the frontend assets and ensure there aren't any other adverse affects from changing those timings. I'll continue testing on Monday.

mikkamp · 2023-09-08T15:32:22Z

src/Assets/AssetsHandler.php


 /**
 * Class AssetsHandler
 *
 * @package Automattic\WooCommerce\GoogleListingsAndAds\Assets
 */
-final class AssetsHandler implements Registerable, AssetsHandlerInterface {
+final class AssetsHandler implements AssetsHandlerInterface {


I was a bit confused about the renaming of the add > register functions. If I recall correctly that name was previously avoided because it implemnted the Registerable interface. But I guess since you removed that here there is no issue with using the function name register.

If I understood it right, only when a class implements the Service and Registerable interfaces and is tagged with its interfaces via a AbstractServiceProvider method like share_with_tags will be treated as a service and registered in GoogleListingsAndAdsPlugin.

The previous AssetsHandler didn't implement Service and its interfaces were not tagged, so I think it should be fine to remove Registerable and use the register method in this way. So that it also pairs and aligns with the enqueue method in the perspective of wp_register_script and wp_enqueue_script.

Thanks for the clarification. Yes that's correct that it needs to be both a Service and Registerable. The tagging isn't important as it will still fetch the classes even if it's not tagged.

I looked at all the assets further and they all seem to be registered/enqueued at the right time even after these changes. So that all looks good.

mikkamp

Thanks for all the changes. I've tested the assets and I can confirm that they are all loaded correctly with their associated data.

I just left one comment though about the data possibly conflicting. It's not a major blocker as it's not an issue right now. So I'll go ahead and approve this PR.

mikkamp · 2023-09-11T08:48:29Z

src/Admin/Admin.php

 			$product_condition
-		) );
+		) )->add_inline_script( ...$gla_data_inline_script_args );


Based on the conditions we won't ever be on a WC Admin registered page and a (traditional) product edit page, so it's ok to pass the data to two separate scripts. I wonder though if this will always remain the case, like with the new product editing experience. Because in that case we would be setting glaData twice on the page.

If applicableProductTypes is all that's needed then why wouldn't we just pass that separately so we'd never be able to conflict:

Suggested change

) )->add_inline_script( ...$gla_data_inline_script_args );

) )->add_inline_script(

[

'glaProductData',

[

'applicableProductTypes' => ProductSyncer::get_supported_product_types(),

],

]

);

Thanks for the suggestion.

It's uncertain if the new Product Edit page will have the case of setting glaData twice. Indeed, it will be better to avoid possible conflicts in advance. Separated the inline script withe different variable names in 0468a73.

…ntial conflicts in the future. Address #2087 (comment)

eason9487 added 3 commits September 7, 2023 12:03

Remove the controls' show/hide CSS classes of the product editing pag…

d376e01

…e from PHP side.

Control the show or hide of the product data tab and meta box added t…

f325aa1

…o the product editing page by JS.

Merge the custom-inputs script into the product-attributes script.

841cb94

eason9487 requested a review from a team September 7, 2023 06:51

eason9487 self-assigned this Sep 7, 2023

github-actions bot added changelog: fix Took care of something that wasn't working. type: bug The issue is a confirmed bug. labels Sep 7, 2023

eason9487 commented Sep 7, 2023

View reviewed changes

martynmjones approved these changes Sep 7, 2023

View reviewed changes

mikkamp reviewed Sep 7, 2023

View reviewed changes

Remove WooCommerceSubscriptions class as it's no longer in use.

d013eaa

Address #2087 (comment)

eason9487 added a commit that referenced this pull request Sep 8, 2023

Use the same source of the supported product types as the backend sid…

b8ed263

…e on the front-end side. Address #2087 (comment)

eason9487 requested a review from mikkamp September 8, 2023 10:16

eason9487 added a commit that referenced this pull request Sep 8, 2023

Use the same source of the supported product types as the backend sid…

99d1cd1

…e on the front-end side. Address #2087 (comment)

eason9487 force-pushed the fix/2086-wc-subscriptions-compatibility branch from b8ed263 to 99d1cd1 Compare September 8, 2023 10:56

Use the same source of the supported product types as the backend sid…

d13cdf6

…e on the front-end side. Address #2087 (comment)

eason9487 force-pushed the fix/2086-wc-subscriptions-compatibility branch from 99d1cd1 to d13cdf6 Compare September 8, 2023 11:01

mikkamp reviewed Sep 8, 2023

View reviewed changes

mikkamp self-requested a review September 8, 2023 15:54

mikkamp approved these changes Sep 11, 2023

View reviewed changes

Separate the inline scripts by different variable names to avoid pote…

0468a73

…ntial conflicts in the future. Address #2087 (comment)

eason9487 merged commit d251442 into develop Sep 11, 2023
14 of 15 checks passed

eason9487 deleted the fix/2086-wc-subscriptions-compatibility branch September 11, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087

WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087

eason9487 commented Sep 7, 2023 •

edited

Loading

eason9487 Sep 7, 2023

martynmjones Sep 7, 2023

eason9487 Sep 8, 2023

codecov bot commented Sep 7, 2023 •

edited

Loading

martynmjones left a comment

mikkamp Sep 7, 2023 •

edited

Loading

eason9487 Sep 8, 2023 •

edited

Loading

eason9487 commented Sep 8, 2023

mikkamp left a comment

mikkamp Sep 8, 2023

eason9487 Sep 11, 2023 •

edited

Loading

mikkamp Sep 11, 2023

mikkamp left a comment

mikkamp Sep 11, 2023 •

edited

Loading

eason9487 Sep 11, 2023

	const applicableProductTypes = new Set( [
	// Simple product
	'simple',
	// Variable product
	'variable',
	// Product bundle from WooCommerce Product Bundles
	'bundle',
	] );

WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087

WooCommerce Subscriptions compatibility: Fix the visible issue of the product data tab and meta box for some unsupported product types #2087

Conversation

eason9487 commented Sep 7, 2023 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Changelog entry

eason9487 Sep 7, 2023

Choose a reason for hiding this comment

martynmjones Sep 7, 2023

Choose a reason for hiding this comment

eason9487 Sep 8, 2023

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2023 • edited Loading

Codecov Report

martynmjones left a comment

Choose a reason for hiding this comment

mikkamp Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

eason9487 Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

eason9487 commented Sep 8, 2023

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp Sep 8, 2023

Choose a reason for hiding this comment

eason9487 Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

mikkamp Sep 11, 2023

Choose a reason for hiding this comment

mikkamp left a comment

Choose a reason for hiding this comment

mikkamp Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

eason9487 Sep 11, 2023

Choose a reason for hiding this comment

eason9487 commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 7, 2023 •

edited

Loading

mikkamp Sep 7, 2023 •

edited

Loading

eason9487 Sep 8, 2023 •

edited

Loading

eason9487 Sep 11, 2023 •

edited

Loading

mikkamp Sep 11, 2023 •

edited

Loading