Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 6 commits into from
Sep 11, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d376e01
Remove the controls' show/hide CSS classes of the product editing pag…
eason9487 Sep 7, 2023
f325aa1
Control the show or hide of the product data tab and meta box added t…
eason9487 Sep 7, 2023
841cb94
Merge the custom-inputs script into the product-attributes script.
eason9487 Sep 7, 2023
d013eaa
Remove WooCommerceSubscriptions class as it's no longer in use.
eason9487 Sep 8, 2023
d13cdf6
Use the same source of the supported product types as the backend sid…
eason9487 Sep 8, 2023
0468a73
Separate the inline scripts by different variable names to avoid pote…
eason9487 Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ module.exports = {
'@wordpress/stylelint-config',
'@pmmmwh/react-refresh-webpack-plugin',
'react-transition-group',
'jquery',
],
'import/resolver': { webpack: webpackResolver },
},
Expand Down
2 changes: 1 addition & 1 deletion .externalized.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
["@woocommerce/components","@woocommerce/currency","@woocommerce/customer-effort-score","@woocommerce/data","@woocommerce/date","@woocommerce/navigation","@woocommerce/number","@woocommerce/settings","@woocommerce/tracks","@wordpress/api-fetch","@wordpress/components","@wordpress/compose","@wordpress/data","@wordpress/data-controls","@wordpress/date","@wordpress/dom","@wordpress/element","@wordpress/hooks","@wordpress/html-entities","@wordpress/i18n","@wordpress/primitives","@wordpress/url","lodash","react","react-dom"]
["@woocommerce/components","@woocommerce/currency","@woocommerce/customer-effort-score","@woocommerce/data","@woocommerce/date","@woocommerce/navigation","@woocommerce/number","@woocommerce/settings","@woocommerce/tracks","@wordpress/api-fetch","@wordpress/components","@wordpress/compose","@wordpress/data","@wordpress/data-controls","@wordpress/date","@wordpress/dom","@wordpress/element","@wordpress/hooks","@wordpress/html-entities","@wordpress/i18n","@wordpress/primitives","@wordpress/url","jquery","lodash","react","react-dom"]
7 changes: 6 additions & 1 deletion js/src/custom-inputs/index.js → js/src/product-attributes/custom-inputs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
/**
* External dependencies
*/
import $ from 'jquery';

/**
* Internal dependencies
*/
import selectWithText from './select-with-text-input';

window.jQuery( function ( $ ) {
$( function () {
'use strict';

const init = () => {
Expand Down
41 changes: 41 additions & 0 deletions js/src/product-attributes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,45 @@
/**
* External dependencies
*/
import $ from 'jquery';

/**
* Internal dependencies
*/
import './custom-inputs';
import './index.scss';

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

@mikkamp mikkamp Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@eason9487 eason9487 Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion!

Since the original execution order of the related processing is:

  1. AssetsHandler->add_many
  2. AssetsHandler->register
  3. IntegrationInitializer->register and WooCommerceProductBundles->init_product_types
  4. 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.

  1. IntegrationInitializer->register and WooCommerceProductBundles->init_product_types
  2. AssetsHandler->register_many: Combines the former AssetsHandler->add_many and AssetsHandler->register
  3. AssetsHandler->enqueue_many

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


// Originally, this extension relied on a WooCommerce core processing to show or hide
// the product data tab and meta box added to the product editing page.
//
// However, WooCommerce Subscriptions has an additional processing, which overrides
// the associated processed result in WooCommerce core.
//
// Since there is no available way to continue to work around it with `show_if_{productType}`
// or `hide_if_{productType}` CSS classes, a jQuery custom event dispatched by WooCommerce core
// is used to handle show or hide them instead.
//
// See:
// - https://github.com/woocommerce/google-listings-and-ads/issues/2086
//
// Ref:
// - https://github.com/woocommerce/woocommerce/blob/8.0.3/plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js#L204-L243
// - https://github.com/Automattic/woocommerce-subscriptions-core/blob/6.2.0/assets/js/admin/admin.js#L18-L88
// - https://github.com/woocommerce/woocommerce/blob/8.0.3/plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js#L130-L158
$( document ).on(
'woocommerce-product-type-change',
'body',
( e, productType ) => {
const shouldDisplay = applicableProductTypes.has( productType );
$( '.gla_attributes_tab, .gla_meta_box' ).toggle( shouldDisplay );
}
);
0 ...om-inputs/select-with-text-input/index.js → ...ttributes/select-with-text-input/index.js
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions src/Admin/Admin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@
};

$assets[] = ( new AdminScriptAsset(
'gla-custom-inputs',
'js/build/custom-inputs',
'gla-product-attributes',
'js/build/product-attributes',

Check warning on line 157 in src/Admin/Admin.php

View check run for this annotation

Codecov / codecov/patch

src/Admin/Admin.php#L156-L157 

Added lines #L156 - L157 were not covered by tests
[],
'',
$product_condition
Expand Down
16 changes: 1 addition & 15 deletions src/Admin/MetaBox/ChannelVisibilityMetaBox.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,7 @@
* @return array
*/
public function get_classes(): array {
$shown_types = array_map(
function ( string $product_type ) {
return "show_if_{$product_type}";
},
ProductSyncer::get_supported_product_types()
);

$hidden_types = array_map(
function ( string $product_type ) {
return "hide_if_{$product_type}";
},
ProductSyncer::get_hidden_product_types()
);

return array_merge( $shown_types, $hidden_types );
return [ 'gla_meta_box' ];

Check warning on line 106 in src/Admin/MetaBox/ChannelVisibilityMetaBox.php

View check run for this annotation

Codecov / codecov/patch

src/Admin/MetaBox/ChannelVisibilityMetaBox.php#L106 

Added line #L106 was not covered by tests
}

/**
Expand Down
18 changes: 1 addition & 17 deletions src/Admin/Product/Attributes/AttributesTab.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,25 +100,9 @@
* @return array An array with product tabs with the Yoast SEO tab added.
*/
private function add_tab( array $tabs ): array {
$shown_types = array_map(
function ( string $product_type ) {
return "show_if_{$product_type}";
},
$this->get_applicable_product_types()
);

$hidden_types = array_map(
function ( string $product_type ) {
return "hide_if_{$product_type}";
},
ProductSyncer::get_hidden_product_types()
);

$classes = array_merge( [ 'gla' ], $shown_types, $hidden_types );

$tabs['gla_attributes'] = [
'label' => 'Google Listings and Ads',
'class' => join( ' ', $classes ),
'class' => 'gla',

Check warning on line 105 in src/Admin/Product/Attributes/AttributesTab.php

View check run for this annotation

Codecov / codecov/patch

src/Admin/Product/Attributes/AttributesTab.php#L105 

Added line #L105 was not covered by tests
'target' => 'gla_attributes',
];

Expand Down
9 changes: 1 addition & 8 deletions src/Integration/WooCommerceSubscriptions.php
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the downside of removing the file entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,6 @@ public function is_active(): bool {
* @return void
*/
public function init(): void {
add_filter(
'woocommerce_gla_hidden_product_types',
function ( array $product_types ) {
$product_types[] = 'subscription';
$product_types[] = 'variable-subscription';
return $product_types;
}
);
// No required adjustments yet.
}
}
11 changes: 0 additions & 11 deletions src/Product/ProductSyncer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,17 +264,6 @@ public static function get_supported_product_types(): array {
return (array) apply_filters( 'woocommerce_gla_supported_product_types', [ 'simple', 'variable', 'variation' ] );
}

/**
* Return the list of product types we will hide functionality for (default none).
*
* @since 1.2.0
*
* @return array
*/
public static function get_hidden_product_types(): array {
return (array) apply_filters( 'woocommerce_gla_hidden_product_types', [] );
}

/**
* @param BatchInvalidProductEntry[] $invalid_products
*/
Expand Down
5 changes: 0 additions & 5 deletions webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,6 @@ const webpackConfig = {
],
entry: {
index: path.resolve( process.cwd(), 'js/src', 'index.js' ),
'custom-inputs': path.resolve(
process.cwd(),
'js/src/custom-inputs',
'index.js'
),
'product-attributes': path.resolve(
process.cwd(),
'js/src/product-attributes',
Expand Down
Loading