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

Commit

Permalink
Fix filter blocks: the data (e.g: max price or stock-status) match th…
Browse files Browse the repository at this point in the history 
…e variation #7245

fix filter blocks: the data (e.g: max price or stock-status) match the variation
  • Loading branch information
@gigitux
gigitux committed Sep 29, 2022
1 parent 5be22b9 commit b8bf17e
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 8 deletions.
6 changes: 5 additions & 1 deletion assets/js/base/context/hooks/collections/use-collection-data.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
*/
import { useState, useEffect, useMemo } from '@wordpress/element';
import { useDebounce } from 'use-debounce';
import { sortBy } from 'lodash';
import { isEmpty, sortBy } from 'lodash';
import { useShallowEqual } from '@woocommerce/base-hooks';
import { getSettingWithCoercion } from '@woocommerce/settings';
import { objectHasProp } from '@woocommerce/types';

/**
Expand Down Expand Up @@ -46,13 +47,15 @@ interface UseCollectionDataProps {
queryPrices?: boolean;
queryStock?: boolean;
queryState: Record< string, unknown >;
productIds?: number[];
}

export const useCollectionData = ( {
queryAttribute,
queryPrices,
queryStock,
queryState,
productIds,
}: UseCollectionDataProps ) => {
let context = useQueryStateContext();
context = `${ context }-collection-data`;
Expand Down Expand Up @@ -145,6 +148,7 @@ export const useCollectionData = ( {
per_page: undefined,
orderby: undefined,
order: undefined,
...( ! isEmpty( productIds ) && { include: productIds } ),
...collectionDataQueryVars,
},
shouldSelect: debouncedShouldSelect,
Expand Down
5 changes: 5 additions & 0 deletions assets/js/blocks/attribute-filter/block.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ const AttributeFilterBlock = ( {
isString
);

const productIds = isEditor
? []
: getSettingWithCoercion( 'product_ids', [], Array.isArray );

const [ hasSetFilterDefaultsFromUrl, setHasSetFilterDefaultsFromUrl ] =
useState( false );

Expand Down Expand Up @@ -157,6 +161,7 @@ const AttributeFilterBlock = ( {
...queryState,
attributes: filterAvailableTerms ? queryState.attributes : null,
},
productIds,
} );

/**
Expand Down
5 changes: 5 additions & 0 deletions assets/js/blocks/price-filter/block.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ const PriceFilterBlock = ( {
isBoolean
);

const productIds = isEditor
? []
: getSettingWithCoercion( 'product_ids', [], Array.isArray );

const [ hasSetFilterDefaultsFromUrl, setHasSetFilterDefaultsFromUrl ] =
useState( false );

Expand All @@ -106,6 +110,7 @@ const PriceFilterBlock = ( {
const { results, isLoading } = useCollectionData( {
queryPrices: true,
queryState,
productIds,
} );

const currency = getCurrencyFromPriceResponse(
Expand Down
5 changes: 5 additions & 0 deletions assets/js/blocks/stock-filter/block.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ const StockStatusFilterBlock = ( {
{}
);

const productIds = isEditor
? []
: getSettingWithCoercion( 'product_ids', [], Array.isArray );

const STOCK_STATUS_OPTIONS = useRef(
getSetting( 'hideOutOfStockItems', false )
? otherStockStatusOptions
Expand Down Expand Up @@ -98,6 +102,7 @@ const StockStatusFilterBlock = ( {
useCollectionData( {
queryStock: true,
queryState,
productIds,
} );

/**
Expand Down
58 changes: 51 additions & 7 deletions src/BlockTypes/ProductQuery.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public function update_query( $pre_render, $parsed_block ) {
// Set this so that our product filters can detect if it's a PHP template.
$this->asset_data_registry->add( 'has_filterable_products', true, true );
$this->asset_data_registry->add( 'is_rendering_php_template', true, true );
$this->asset_data_registry->add( 'product_ids', $this->get_products_ids_by_attributes( $parsed_block ), true );
add_filter(
'query_loop_block_query_vars',
array( $this, 'build_query' ),
Expand Down Expand Up @@ -119,19 +120,62 @@ public function build_query( $query ) {
$queries_by_filters
),
function( $acc, $query ) {
$acc['post__in'] = isset( $query['post__in'] ) ? $this->intersect_arrays_when_not_empty( $acc['post__in'], $query['post__in'] ) : $acc['post__in'];
return $this->merge_queries( $acc, $query );
},
$common_query_values
);
}

/**
* Return the product ids based on the attributes.
*
* @param array $parsed_block The block being rendered.
* @return array
*/
private function get_products_ids_by_attributes( $parsed_block ) {
$queries_by_attributes = $this->get_queries_by_attributes( $parsed_block );

$query = array_reduce(
$queries_by_attributes,
function( $acc, $query ) {
return $this->merge_queries( $acc, $query );
},
array(
'post_type' => 'product',
'post__in' => array(),
'post_status' => 'publish',
'posts_per_page' => -1,
// Ignoring the warning of not using meta queries.
// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query
$acc['meta_query'] = isset( $query['meta_query'] ) ? array_merge( $acc['meta_query'], array( $query['meta_query'] ) ) : $acc['meta_query'];
'meta_query' => array(),
// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query
$acc['tax_query'] = isset( $query['tax_query'] ) ? array_merge( $acc['tax_query'], array( $query['tax_query'] ) ) : $acc['tax_query'];

return $acc;
},
$common_query_values
'tax_query' => array(),
)
);

$products = new \WP_Query( $query );
$post_ids = wp_list_pluck( $products->posts, 'ID' );

return $post_ids;
}

/**
* Merge in the first parameter the keys "post_in", "meta_query" and "tax_query" of the second parameter.
*
* @param array $query1 The first query.
* @param array $query2 The second query.
* @return array
*/
private function merge_queries( $query1, $query2 ) {
$query1['post__in'] = isset( $query2['post__in'] ) ? $this->intersect_arrays_when_not_empty( $query1['post__in'], $query2['post__in'] ) : $query1['post__in'];
// Ignoring the warning of not using meta queries.
// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query
$query1['meta_query'] = isset( $query2['meta_query'] ) ? array_merge( $query1['meta_query'], array( $query2['meta_query'] ) ) : $query1['meta_query'];
// phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_tax_query
$query1['tax_query'] = isset( $query2['tax_query'] ) ? array_merge( $query1['tax_query'], array( $query2['tax_query'] ) ) : $query1['tax_query'];

return $query1;

This comment has been minimized.

Sign in to view

Copy link
@sunyatasattva

sunyatasattva Oct 12, 2022

Contributor

I have a few comments about this function.

  1. Can we make this function accept an unlimited number of arguments? So, either be recursive, or just embed the reduce call within itself?
  2. If not, can we rename the parameters? I'm not a big fan of numbers at the end of parameters name. I'm not sure what I would suggest, but I'd rather have $a and $b, than $query1 and $query2 in these kinds of scenarios, usually.
  3. Can we replace the duplicated phpcs:ignore comment with just one phpcs:disable at the beginning of the function? Or perhaps even at the beginning of the file, given we are going to be using meta_query a bunch here.
  4. Lastly, can we make this function non-destructive and return the new merged query?

This comment has been minimized.

Sign in to view

Copy link
@gigitux

gigitux Oct 13, 2022

Author Contributor
  1. We could do it, but I don't see the advantage. We increase the complexity of this function without any particular reason.
  2. Yeah, I prefer to be always explicit, but I'm okay with changing it.
  3. Yeah, let's do it at the beginning of the file.
  4. Are you suggesting not to mutate $query1? I will do it!

This comment has been minimized.

Sign in to view

Copy link
@sunyatasattva

sunyatasattva Oct 14, 2022

Contributor

Thank you for your reply!

We could do it, but I don't see the advantage. We increase the complexity of this function without any particular reason.

Well, in my opinion, it shouldn't add too much complexity. It'd also be in line with most PHP array related functions (e.g. array_merge or array_intersect both accept any number of args), and basically you could just move your reduce from the other function to here, no?

This comment has been minimized.

Sign in to view

Copy link
@sunyatasattva

sunyatasattva Oct 14, 2022

Contributor

Yeah, I prefer to be always explicit, but I'm okay with changing it.

Yea I admit $a and $b are not the best names either (though for a merge or sort function they are alright, example here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort), but something rubs me the wrong way about numbers at the end of variable names, you know what I mean?

This comment has been minimized.

Sign in to view

Copy link
@gigitux

gigitux Oct 27, 2022

Author Contributor

Well, in my opinion, it shouldn't add too much complexity. It'd also be in line with most PHP array related functions (e.g. array_merge or array_intersect both accept any number of args), and basically you could just move your reduce from the other function to here, no?

Not really. If you remember get_queries_by_attributes and get_queries_by_applied_filters functions returns an object with this shape:

[on_sale] => Array
	 (
	   ....query
	     )
[price_filter] => Array
	 (
	   ....query
	     )

If I use the spread operator ...arrays, in the function merge_queries, the shape of that array will be:

[0] => Array
         (
          [on_sale] => Array
	                          (
	                          ....query
	                          )
)
[1] => Array
       (
          [price_filter] => Array
	                              (
	                               ....query
	                               )
)

So, I should flat the array and, after that, merge the objects. Furthermore, this function merges only post__in, meta_query and tax_query keys.

In the end, if I proceed with your suggestion, the code looks like in this way:

	public function build_query( $query ) {
		$parsed_block = $this->parsed_block;
		if ( ! $this->is_woocommerce_variation( $parsed_block ) ) {
			return $query;
		}

		$common_query_values = array(
			'post_type'      => 'product',
			'post__in'       => array(),
			'post_status'    => 'publish',
			'posts_per_page' => $query['posts_per_page'],
			'orderby'        => $query['orderby'],
			'order'          => $query['order'],
			'offset'         => $query['offset'],
			'meta_query'     => array(),
			'tax_query'      => array(),
		);

		$queries_by_attributes = $this->get_queries_by_attributes( $parsed_block );
		$queries_by_filters    = $this->get_queries_by_applied_filters();
		$query = $this->merge_queries( $queries_by_attributes, $queries_by_filters  );
		return array_merge( $common_query_values, $query );
	}

Especially if that function is used only for this class, I don't see a huge advantage. What do you think?
}


Expand Down

0 comments on commit b8bf17e

Please sign in to comment.