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.

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

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
1 change: 1 addition & 0 deletions .eslintrc.js
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
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"]
5 changes: 5 additions & 0 deletions js/src/constants.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
// An inline script data only available for the admin pages of this extension.
export const glaData = window.glaData;

// An inline script data only available for the UI blocks that are added to the
// product editing page by this extension.
export const glaProductData = window.glaProductData;

export const FREE_LISTINGS_PROGRAM_ID = 0;

// Products report related
Expand Down
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
35 changes: 35 additions & 0 deletions js/src/product-attributes/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,39 @@
/**
* External dependencies
*/
import $ from 'jquery';

/**
* Internal dependencies
*/
import { glaProductData } from '.~/constants';
import './custom-inputs';
import './index.scss';

// 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 =
glaProductData.applicableProductTypes.includes( productType );

$( '.gla_attributes_tab, .gla_meta_box' ).toggle( shouldDisplay );
}
);
33 changes: 22 additions & 11 deletions src/Admin/Admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

use Automattic\WooCommerce\GoogleListingsAndAds\Admin\MetaBox\MetaBoxInterface;
use Automattic\WooCommerce\GoogleListingsAndAds\Ads\AdsService;
use Automattic\WooCommerce\GoogleListingsAndAds\Assets\AdminScriptAsset;
use Automattic\WooCommerce\GoogleListingsAndAds\Assets\AdminScriptWithBuiltDependenciesAsset;
use Automattic\WooCommerce\GoogleListingsAndAds\Assets\AdminStyleAsset;
use Automattic\WooCommerce\GoogleListingsAndAds\Assets\Asset;
Expand All @@ -17,6 +16,7 @@
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\ViewFactory;
use Automattic\WooCommerce\GoogleListingsAndAds\MerchantCenter\MerchantCenterService;
use Automattic\WooCommerce\GoogleListingsAndAds\PluginHelper;
use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductSyncer;
use Automattic\WooCommerce\GoogleListingsAndAds\Value\BuiltScriptDependencyArray;
use Automattic\WooCommerce\GoogleListingsAndAds\View\ViewException;
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsAwareInterface;
Expand Down Expand Up @@ -72,8 +72,6 @@
* Register a service.
*/
public function register(): void {
$this->assets_handler->add_many( $this->get_assets() );

add_action(
'admin_enqueue_scripts',
function() {
Expand All @@ -82,7 +80,10 @@
wp_enqueue_media();
}

$this->assets_handler->enqueue_many( $this->get_assets() );
$assets = $this->get_assets();

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

View check run for this annotation

Codecov / codecov/patch

src/Admin/Admin.php#L83

Added line #L83 was not covered by tests

$this->assets_handler->register_many( $assets );
$this->assets_handler->enqueue_many( $assets );

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

View check run for this annotation

Codecov / codecov/patch

src/Admin/Admin.php#L85-L86

Added lines #L85 - L86 were not covered by tests
}
);

Expand Down Expand Up @@ -135,7 +136,6 @@
'dateFormat' => get_option( 'date_format' ),
'timeFormat' => get_option( 'time_format' ),
'siteLogoUrl' => wp_get_attachment_image_url( get_theme_mod( 'custom_logo' ), 'full' ),

]
);

Expand All @@ -152,13 +152,24 @@
return ( null !== $screen && 'product' === $screen->id );
};

$assets[] = ( new AdminScriptAsset(
'gla-custom-inputs',
'js/build/custom-inputs',
[],
'',
$assets[] = ( new AdminScriptWithBuiltDependenciesAsset(
'gla-product-attributes',
'js/build/product-attributes',
"{$this->get_root_dir()}/js/build/product-attributes.asset.php",
new BuiltScriptDependencyArray(
[
'dependencies' => [],
'version' => (string) filemtime( "{$this->get_root_dir()}/js/build/product-attributes.js" ),
]
),

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

View check run for this annotation

Codecov / codecov/patch

src/Admin/Admin.php#L155-L164

Added lines #L155 - L164 were not covered by tests
$product_condition
) );
) )->add_inline_script(
'glaProductData',
[
'applicableProductTypes' => ProductSyncer::get_supported_product_types(),
]
);

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

View check run for this annotation

Codecov / codecov/patch

src/Admin/Admin.php#L166-L171

Added lines #L166 - L171 were not covered by tests

$assets[] = ( new AdminStyleAsset(
'gla-product-attributes-css',
'js/build/product-attributes',
Expand Down
16 changes: 1 addition & 15 deletions src/Admin/MetaBox/ChannelVisibilityMetaBox.php
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
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
35 changes: 13 additions & 22 deletions src/Assets/AssetsHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
namespace Automattic\WooCommerce\GoogleListingsAndAds\Assets;

use Automattic\WooCommerce\GoogleListingsAndAds\Exception\InvalidAsset;
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Registerable;

/**
* Class AssetsHandler
*
* @package Automattic\WooCommerce\GoogleListingsAndAds\Assets
*/
final class AssetsHandler implements Registerable, AssetsHandlerInterface {
final class AssetsHandler implements AssetsHandlerInterface {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@eason9487 eason9487 Sep 11, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
* Assets known to this asset handler.
Expand All @@ -21,32 +20,24 @@
private $assets = [];

/**
* Add a single asset to the asset handler.
* Register a single asset.
*
* @param Asset $asset Asset to add.
* @param Asset $asset Asset to register.
*/
public function add( Asset $asset ): void {
public function register( Asset $asset ): void {

Check warning on line 27 in src/Assets/AssetsHandler.php

View check run for this annotation

Codecov / codecov/patch

src/Assets/AssetsHandler.php#L27

Added line #L27 was not covered by tests
$this->validate_handle_not_exists( $asset->get_handle() );
$this->assets[ $asset->get_handle() ] = $asset;
$asset->register();

Check warning on line 30 in src/Assets/AssetsHandler.php

View check run for this annotation

Codecov / codecov/patch

src/Assets/AssetsHandler.php#L30

Added line #L30 was not covered by tests
}

/**
* Add multiple assets to the asset handler.
* Register multiple assets.
*
* @param Asset[] $assets Array of assets to add.
* @param Asset[] $assets Array of assets to register.
*/
public function add_many( array $assets ): void {
public function register_many( array $assets ): void {

Check warning on line 38 in src/Assets/AssetsHandler.php

View check run for this annotation

Codecov / codecov/patch

src/Assets/AssetsHandler.php#L38

Added line #L38 was not covered by tests
foreach ( $assets as $asset ) {
$this->add( $asset );
}
}

/**
* Register a service.
*/
public function register(): void {
foreach ( $this->assets as $asset ) {
$asset->register();
$this->register( $asset );

Check warning on line 40 in src/Assets/AssetsHandler.php

View check run for this annotation

Codecov / codecov/patch

src/Assets/AssetsHandler.php#L40

Added line #L40 was not covered by tests
}
}

Expand All @@ -57,8 +48,8 @@
*
* @throws InvalidAsset If the passed-in asset is not valid.
*
* @see AssetsHandlerInterface::add To add assets.
* @see AssetsHandlerInterface::add_many To add multiple assets.
* @see AssetsHandlerInterface::register To register assets.
* @see AssetsHandlerInterface::register_many To register multiple assets.
*/
public function enqueue( Asset $asset ): void {
$this->enqueue_handle( $asset->get_handle() );
Expand All @@ -71,8 +62,8 @@
*
* @throws InvalidAsset If any of the passed-in assets are not valid.
*
* @see AssetsHandlerInterface::add To add assets.
* @see AssetsHandlerInterface::add_many To add multiple assets.
* @see AssetsHandlerInterface::register To register assets.
* @see AssetsHandlerInterface::register_many To register multiple assets.
*/
public function enqueue_many( array $assets ): void {
foreach ( $assets as $asset ) {
Expand Down
20 changes: 10 additions & 10 deletions src/Assets/AssetsHandlerInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@
interface AssetsHandlerInterface {

/**
* Add a single asset to the asset handler.
* Register a single asset.
*
* @param Asset $asset Asset to add.
* @param Asset $asset Asset to register.
*/
public function add( Asset $asset ): void;
public function register( Asset $asset ): void;

/**
* Add multiple assets to the asset handler.
* Register multiple assets.
*
* @param Asset[] $assets Array of assets to add.
* @param Asset[] $assets Array of assets to register.
*/
public function add_many( array $assets ): void;
public function register_many( array $assets ): void;

/**
* Enqueue a single asset.
Expand All @@ -33,8 +33,8 @@ public function add_many( array $assets ): void;
*
* @throws InvalidAsset If the passed-in asset is not valid.
*
* @see AssetsHandlerInterface::add To add assets.
* @see AssetsHandlerInterface::add_many To add multiple assets.
* @see AssetsHandlerInterface::register To register assets.
* @see AssetsHandlerInterface::register_many To register multiple assets.
*/
public function enqueue( Asset $asset ): void;

Expand All @@ -45,8 +45,8 @@ public function enqueue( Asset $asset ): void;
*
* @throws InvalidAsset If any of the passed-in assets are not valid.
*
* @see AssetsHandlerInterface::add To add assets.
* @see AssetsHandlerInterface::add_many To add multiple assets.
* @see AssetsHandlerInterface::register To register assets.
* @see AssetsHandlerInterface::register_many To register multiple assets.
*/
public function enqueue_many( array $assets ): void;

Expand Down
2 changes: 1 addition & 1 deletion src/Google/GlobalSiteTag.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@
}
);

$this->assets_handler->add( $gtag_events );
$this->assets_handler->register( $gtag_events );

Check warning on line 192 in src/Google/GlobalSiteTag.php

View check run for this annotation

Codecov / codecov/patch

src/Google/GlobalSiteTag.php#L192

Added line #L192 was not covered by tests

add_action(
'wp_footer',
Expand Down
3 changes: 0 additions & 3 deletions src/Infrastructure/GoogleListingsAndAdsPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

namespace Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure;

use Automattic\WooCommerce\GoogleListingsAndAds\Assets\AssetsHandlerInterface;
use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\JobInitializer;
use Automattic\WooCommerce\GoogleListingsAndAds\Internal\Requirements\PluginValidator;
use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface;
Expand Down Expand Up @@ -102,8 +101,6 @@ function() {
add_action(
'init',
function() {
$this->container->get( AssetsHandlerInterface::class )->register();

// register the job initializer only if it is available. see JobInitializer::is_needed.
if ( $this->container->has( JobInitializer::class ) ) {
$this->container->get( JobInitializer::class )->register();
Expand Down
Loading