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

Product Query: implement compatibility with Filter by Rating block #7792

Merged
merged 9 commits into from
Dec 7, 2022
41 changes: 40 additions & 1 deletion src/BlockTypes/ProductQuery.php
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ function( $acc, $array ) {
'price_filter_query_args' => array( PriceFilter::MIN_PRICE_QUERY_VAR, PriceFilter::MAX_PRICE_QUERY_VAR ),
'stock_filter_query_args' => array( StockFilter::STOCK_STATUS_QUERY_VAR ),
'attributes_filter_query_args' => $attributes_filter_query_args,
'rating_filter_query_args' => array( RatingFilter::RATING_QUERY_VAR ),
);

}
Expand Down Expand Up @@ -419,6 +420,7 @@ private function get_queries_by_applied_filters() {
'price_filter' => $this->get_filter_by_price_query(),
'attributes_filter' => $this->get_filter_by_attributes_query(),
'stock_status_filter' => $this->get_filter_by_stock_status_query(),
'rating_filter' => $this->get_filter_by_rating_query(),
);
}

Expand Down Expand Up @@ -710,5 +712,42 @@ private function get_global_query( $parsed_block ) {
return $query;
}

}
/**
* Return a query that filters products by rating.
*
* @return array
*/
private function get_filter_by_rating_query() {
$filter_rating_values = get_query_var( RatingFilter::RATING_QUERY_VAR );
Copy link
Member

Choose a reason for hiding this comment

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

Should we use rating_filter_query_args from get_query_vars_from_filter_blocks() here so we can centralize the references of filter query variable constants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong opinion. Other methods (e.g:get_filter_by_price_query) don't use get_query_vars_from_filter_blocks. I'm happy to refactor them!

$product_visibility_terms = wc_get_product_visibility_term_ids();

if ( empty( $filter_rating_values ) ) {
return array();
}
Comment on lines +727 to +729
Copy link
Member

Choose a reason for hiding this comment

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

We should move the early return condition to after the $filter_rating_values declaration so that we can save unnecessary wc_get_product_visibility_term_ids() calls that call get_terms() under the hood.


$parsed_filter_rating_values = explode( ',', $filter_rating_values );

if ( empty( $parsed_filter_rating_values ) || empty( $product_visibility_terms ) ) {
return array();
}

$rating_query = array_map(
Copy link
Member

Choose a reason for hiding this comment

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

I think $rating_terms better explains the value of this variable.

Suggested change
$rating_query = array_map(
$rating_terms = array_map(

function( $rating ) use ( $product_visibility_terms ) {
return $product_visibility_terms[ 'rated-' . $rating ];
},
$parsed_filter_rating_values
);

return array(
'tax_query' => array(
array(
'field' => 'term_taxonomy_id',
'terms' => $rating_query,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'terms' => $rating_query,
'terms' => $rating_terms,

'operator' => 'IN',
'rating_filter' => true,
Copy link
Member

Choose a reason for hiding this comment

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

Should we add taxonomy property here as well, to match with the query used by WC core?

					'taxonomy'      => 'product_visibility',

),
),
);
}

}
4 changes: 3 additions & 1 deletion src/BlockTypes/RatingFilter.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,7 @@ class RatingFilter extends AbstractBlock {
*
* @var string
*/
protected $block_name = 'rating-filter';
protected $block_name = 'rating-filter';
const RATING_QUERY_VAR = 'rating_filter';

}
123 changes: 121 additions & 2 deletions tests/e2e/specs/shopper/filter-products-by-rating.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import {
waitForCanvas,
} from '../../utils';

import { saveOrPublish } from '../../../utils';
Copy link
Member

Choose a reason for hiding this comment

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

We can import saveOrPublish from @woocommerce/blocks-test-utils to avoid multiple levels of relative imports.


const block = {
name: 'Filter by Rating',
slug: 'woocommerce/rating-filter',
Expand All @@ -37,6 +39,7 @@ const block = {
},
frontend: {
productsList: '.wc-block-grid__products > li',
queryProductsList: '.wp-block-post-template > li',
classicProductsList: '.products.columns-3 > li',
fiveStarInput: ".wc-block-rating-filter label[for='5'] input",
submitButton: '.wc-block-components-filter-submit-button',
Expand All @@ -54,7 +57,7 @@ const goToShopPage = () =>
waitUntil: 'networkidle0',
} );

describe.skip( `${ block.name } Block`, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we wanna re-enable this test here? Based on #7744 it sounds like we are waiting on the next release of Gutenberg to do so.

Copy link
Member

@dinhtungdu dinhtungdu Dec 1, 2022

Choose a reason for hiding this comment

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

We still skip tests related to deleteAllTemplates so it doesn't conflict with #7744. However, we should comment there to inform people about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this, and I disabled only the test regarding the PHP Classic Template (the only one that requires deleteAllTemplates function).

My idea is to open a PR for doing the same thing for other filters too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! I also agree with @dinhtungdu 's suggestion of registering this info on the relevant issue for our future reference.

describe( `${ block.name } Block`, () => {
describe( 'with All Products Block', () => {
let link = '';
beforeAll( async () => {
Expand Down Expand Up @@ -91,7 +94,7 @@ describe.skip( `${ block.name } Block`, () => {
} );
} );

describe( 'with PHP classic template', () => {
describe.skip( 'with PHP classic template', () => {
const productCatalogTemplateId =
'woocommerce/woocommerce//archive-product';

Expand Down Expand Up @@ -204,4 +207,120 @@ describe.skip( `${ block.name } Block`, () => {
);
} );
} );

describe( 'with Product Query Block', () => {
let editorPageUrl = '';
let frontedPageUrl = '';

useTheme( 'emptytheme' );
beforeAll( async () => {
await switchUserToAdmin();
await createNewPost( {
postType: 'post',
title: block.name,
} );

await insertBlock( 'Product Query' );
await insertBlock( block.name );
await page.waitForNetworkIdle();
await publishPost();

editorPageUrl = page.url();
frontedPageUrl = await page.evaluate( () =>
wp.data.select( 'core/editor' ).getPermalink()
);
await page.goto( frontedPageUrl );
} );

it( 'should render', async () => {
await expect( page ).toMatchElement( block.class );
} );

it( 'should render products', async () => {
const products = await page.$$(
selectors.frontend.queryProductsList
);

expect( products ).toHaveLength( 5 );
} );
Copy link
Member

Choose a reason for hiding this comment

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

As we have the same E2E tests for Product Query, I don't think we need these tests here, in the Filter by Rating test suite.


it( 'should show only products that match the filter', async () => {
const isRefreshed = jest.fn( () => void 0 );
page.on( 'load', isRefreshed );

await page.waitForSelector( block.class + '.is-loading', {
hidden: true,
} );

expect( isRefreshed ).not.toBeCalled();

await page.waitForSelector( selectors.frontend.fiveStarInput );

await Promise.all( [
page.waitForNavigation(),
page.click( selectors.frontend.fiveStarInput ),
] );

const products = await page.$$(
selectors.frontend.queryProductsList
);
const pageURL = page.url();
const parsedURL = new URL( pageURL );

expect( isRefreshed ).toBeCalledTimes( 1 );
expect( products ).toHaveLength( FIVE_STAR_PRODUCTS_AMOUNT );
Copy link
Member

Choose a reason for hiding this comment

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

This assertion can be false positive, as we have 5 products in total, and they have the same rating. We should improve the test products to have different rating data for some products. I feel that improving the test data is a bit of out scope for this PR, so I think it's better to create an issue for it and continue working there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I created #7853

expect( parsedURL.search ).toEqual(
block.urlSearchParamWhenFilterIsApplied
);
} );

it( 'should refresh the page only if the user click on button', async () => {
await page.goto( editorPageUrl );
await openBlockEditorSettings();
Copy link
Member

Choose a reason for hiding this comment

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

As we call openBlockEditorSettings() below, this isn't necessary to call it here.

await selectBlockByName( block.slug );
await openBlockEditorSettings();
await page.waitForXPath(
block.selectors.editor.filterButtonToggle
);

const [ filterButtonToggle ] = await page.$x(
selectors.editor.filterButtonToggle
);
Copy link
Member

Choose a reason for hiding this comment

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

page.waitForXPath() returns an element, so we can combine these calls into one, something like:

Suggested change
await page.waitForXPath(
block.selectors.editor.filterButtonToggle
);
const [ filterButtonToggle ] = await page.$x(
selectors.editor.filterButtonToggle
);
const filterButtonToggle = await page.waitForXPath(
block.selectors.editor.filterButtonToggle
);

By the way, we introduced some new utilities to interact with inspector settings, can you take a look at them to see if we can use them here?

await setCheckbox(
await getToggleIdByLabel( 'Show only products on sale' )
);
expect( await getPreviewProducts() ).toHaveLength( saleCount );
await unsetCheckbox(
await getToggleIdByLabel( 'Show only products on sale' )
);

await filterButtonToggle.click();
await saveOrPublish();
await page.goto( frontedPageUrl );

const isRefreshed = jest.fn( () => void 0 );
page.on( 'load', isRefreshed );

await page.waitForSelector( block.class + '.is-loading', {
hidden: true,
} );

expect( isRefreshed ).not.toBeCalled();

await page.waitForSelector( selectors.frontend.fiveStarInput );
await page.click( selectors.frontend.fiveStarInput );
await Promise.all( [
page.waitForNavigation( {
waitUntil: 'networkidle0',
} ),
page.click( selectors.frontend.submitButton ),
] );

const pageURL = page.url();
const parsedURL = new URL( pageURL );

await page.waitForSelector( selectors.frontend.queryProductsList );
const products = await page.$$(
selectors.frontend.queryProductsList
);

expect( isRefreshed ).toBeCalledTimes( 1 );
expect( products ).toHaveLength( FIVE_STAR_PRODUCTS_AMOUNT );
expect( parsedURL.search ).toEqual(
block.urlSearchParamWhenFilterIsApplied
);
} );
} );
} );