Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Fix: incorrect default number of products in editor when inheriting query #10303

Merged
merged 8 commits into from
Jul 27, 2023
1 change: 1 addition & 0 deletions assets/js/blocks/product-query/inspector-controls.tsx
Original file line number Diff line number Diff line change
@@ -139,6 +139,7 @@ export const WooInheritToggleControl = (
setQueryAttribute( props, {
...props.defaultWooQueryParams,
inherit,
__woocommerceInherit: inherit,
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK, we should use inherit instead of __woocommerceInherit. The __woocommerceInherit is added only when isCustomInheritGlobalQueryImplementationEnabled is true, but we've decided not to implement the custom inherit query from template. Therefore, this flag will always be false, as seen here. You can also find related discussion on Slack here: p1681812585287949-slack-C02UBB1EPEF.

In short, we shouldn’t use __woocommerceInherit. Additionally, I believe we can remove the isCustomInheritGlobalQueryImplementationEnabled flag and related code since we decided not to implement the "custom inherit query from template." I understand it might be confusing because there is still a partial implementation for the "custom inherit query from template," which is disabled by using the isCustomInheritGlobalQueryImplementationEnabled flag.

Does that make sense?

CC: @gigitux

Copy link
Contributor

Choose a reason for hiding this comment

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

In short, we shouldn’t use __woocommerceInherit. Additionally, I believe we can remove the isCustomInheritGlobalQueryImplementationEnabled flag and related code since we decided not to implement the "custom inherit query from template." I understand it might be confusing because there is still a partial implementation for the "custom inherit query from template," which is disabled by using the isCustomInheritGlobalQueryImplementationEnabled flag.

Since the logic is added on the Products block (that will be deprecated) I agree with you, we can remove that logic!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @imanish003 for the review, I agree this is a bit confusing, so let's discuss more.

  • In 0d8fc18, I fixed the issue you mentioned in the review. I didn't handle well the case adding the block into archive template in the template editor.
  • I use the __woocommerceInherit attribute just for detecting if the current rest query is inherited from template. I used it in the rest_product_query filter here (as a custom rest query argument). The reason for that is I can't find a better way, I'm open to any alternative that we can detect the 'inherit state' in the rest query. I admit this is a workaround than a solution, but given that the Products (Beta) block will be deprecated, this is acceptable IMO.
  • About removing isCustomInheritGlobalQueryImplementationEnabled, let's do it in a separated PR for better history.

Copy link
Contributor

@imanish003 imanish003 Jul 25, 2023

Choose a reason for hiding this comment

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

In 0d8fc18, I fixed the issue you mentioned in the review. I didn't handle well the case adding the block into archive template in the template editor.

Not sure why but it's still showing 9 products in Products beta block 🤔 Are you able to replicate this?

I use the __woocommerceInherit attribute just for detecting if the current rest query is inherited from template. I used it in the rest_product_query filter here (as a custom rest query argument). The reason for that is I can't find a better way, I'm open to any alternative that we can detect the 'inherit state' in the rest query. I admit this is a workaround than a solution, but given that the Products (Beta) block will be deprecated, this is acceptable IMO.

Got it. In that case, maybe we should consider naming this something other than __woocommerceInherit because its original intention was for use in the "custom inherit from query" implementation. Since this fix is not directly related to "custom inherit from query," using __woocommerceInherit for this fix might cause confusion. What do you think?

About removing isCustomInheritGlobalQueryImplementationEnabled, let's do it in a separated PR for better history.

Definitely, Also this doesn't fall into scope of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this fix is not directly related to "custom inherit from query," using __woocommerceInherit for this fix might cause confusion. What do you think?

@imanish003 Because we're removing that "custom inherit feature", and the Products (Beta) block will be deprecated eventually, I don't think that's a big issue. But by the way, any name will work, I will try to find another one to clear any potential confusion.

Not sure why but it's still showing 9 products in Products beta block 🤔 Are you able to replicate this?

Let's pair on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

But by the way, any name will work, I will try to find another one to clear any potential confusion.

Thank you so much for your understanding. Since we're unsure who will be working on removing the "custom inherit query" feature, it's better to avoid any potential confusion if we can. 🙌🏻

// Restore the query object value before inherit was enabled.
...( inherit === false && {
...queryObjectBeforeInheritEnabled,
9 changes: 9 additions & 0 deletions assets/js/blocks/product-template/edit.tsx
Original file line number Diff line number Diff line change
@@ -17,6 +17,8 @@ import { Spinner } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import type { BlockEditProps } from '@wordpress/blocks';
import { ProductCollectionAttributes } from '@woocommerce/blocks/product-collection/types';
import { getSettingWithCoercion } from '@woocommerce/settings';
import { isNumber } from '@woocommerce/types';

const ProductTemplateInnerBlocks = () => {
const innerBlocksProps = useInnerBlocksProps(
@@ -102,6 +104,11 @@ const ProductTemplateEdit = ( {
const [ { page } ] = queryContext;
const [ activeBlockContextId, setActiveBlockContextId ] = useState();
const postType = 'product';
const loopShopPerPage = getSettingWithCoercion(
'loop_shop_per_page',
9,
isNumber
);
const { products, blocks } = useSelect(
( select ) => {
const { getEntityRecords, getTaxonomies } = select( coreStore );
@@ -121,6 +128,7 @@ const ProductTemplateEdit = ( {
slug: templateSlug.replace( 'category-', '' ),
} );
const query: Record< string, unknown > = {
postType,
offset: perPage ? perPage * ( page - 1 ) + offset : 0,
order,
orderby: orderBy,
@@ -171,6 +179,7 @@ const ProductTemplateEdit = ( {
if ( templateCategory ) {
query.categories = templateCategory[ 0 ]?.id;
}
query.per_page = loopShopPerPage;
}
return {
products: getEntityRecords( 'postType', postType, {
16 changes: 16 additions & 0 deletions src/BlockTypes/ProductCollection.php
Original file line number Diff line number Diff line change
@@ -86,6 +86,22 @@ protected function initialize() {
add_filter( 'rest_product_collection_params', array( $this, 'extend_rest_query_allowed_params' ), 10, 1 );
}

/**
* Extra data passed through from server to client for block.
*
* @param array $attributes Any attributes that currently are available from the block.
* Note, this will be empty in the editor context when the block is
* not in the post content on editor load.
*/
protected function enqueue_data( array $attributes = [] ) {
parent::enqueue_data( $attributes );

// The `loop_shop_per_page` filter can be found in WC_Query::product_query().
// phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment
$this->asset_data_registry->add( 'loop_shop_per_page', apply_filters( 'loop_shop_per_page', wc_get_default_products_per_row() * wc_get_default_product_rows_per_page() ), true );
}


/**
* Update the query for the product query block in Editor.
*
7 changes: 7 additions & 0 deletions src/BlockTypes/ProductQuery.php
Original file line number Diff line number Diff line change
@@ -194,6 +194,7 @@ public function update_rest_query( $args, $request ): array {
$orderby = $request->get_param( 'orderby' );
$woo_stock_status = $request->get_param( '__woocommerceStockStatus' );
$on_sale = $request->get_param( '__woocommerceOnSale' ) === 'true';
$is_inherit = $request->get_param( '__woocommerceInherit' ) === 'true';

$on_sale_query = $on_sale ? $this->get_on_sale_products_query() : [];
$orderby_query = $orderby ? $this->get_custom_orderby_query( $orderby ) : [];
@@ -202,6 +203,12 @@ public function update_rest_query( $args, $request ): array {
$visibility_query = is_array( $woo_stock_status ) ? $this->get_product_visibility_query( $stock_query ) : [];
$tax_query = $is_valid_attributes ? $this->merge_tax_queries( $attributes_query, $visibility_query ) : [];

if ( $is_inherit ) {
// The `loop_shop_per_page` filter can be found in WC_Query::product_query().
// phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment, WordPress.WP.PostsPerPage.posts_per_page_posts_per_page
$args['posts_per_page'] = apply_filters( 'loop_shop_per_page', wc_get_default_products_per_row() * wc_get_default_product_rows_per_page() );
}

return array_merge( $args, $on_sale_query, $orderby_query, $stock_query, $tax_query );
}