-
Notifications
You must be signed in to change notification settings - Fork 219
Product Query: implement compatibility with Filter by Rating block #7792
Changes from 1 commit
99eaf04
43a4e94
68a2023
9320961
461af72
17a2b5f
e062f7a
2229b6a
b14a78b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 ), | ||||||
); | ||||||
|
||||||
} | ||||||
|
@@ -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(), | ||||||
); | ||||||
} | ||||||
|
||||||
|
@@ -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 ); | ||||||
$product_visibility_terms = wc_get_product_visibility_term_ids(); | ||||||
|
||||||
if ( empty( $filter_rating_values ) ) { | ||||||
return array(); | ||||||
} | ||||||
Comment on lines
+727
to
+729
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should move the early return condition to after the |
||||||
|
||||||
$parsed_filter_rating_values = explode( ',', $filter_rating_values ); | ||||||
|
||||||
if ( empty( $parsed_filter_rating_values ) || empty( $product_visibility_terms ) ) { | ||||||
return array(); | ||||||
} | ||||||
|
||||||
$rating_query = array_map( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
Suggested change
|
||||||
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, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
'operator' => 'IN', | ||||||
'rating_filter' => true, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add
|
||||||
), | ||||||
), | ||||||
); | ||||||
} | ||||||
|
||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,6 +26,8 @@ import { | |||||||||||||||||||||||||||||||||||
waitForCanvas, | ||||||||||||||||||||||||||||||||||||
} from '../../utils'; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
import { saveOrPublish } from '../../../utils'; | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can import |
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
const block = { | ||||||||||||||||||||||||||||||||||||
name: 'Filter by Rating', | ||||||||||||||||||||||||||||||||||||
slug: 'woocommerce/rating-filter', | ||||||||||||||||||||||||||||||||||||
|
@@ -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', | ||||||||||||||||||||||||||||||||||||
|
@@ -54,7 +57,7 @@ const goToShopPage = () => | |||||||||||||||||||||||||||||||||||
waitUntil: 'networkidle0', | ||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
describe.skip( `${ block.name } Block`, () => { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still skip tests related to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 My idea is to open a PR for doing the same thing for other filters too. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () => { | ||||||||||||||||||||||||||||||||||||
|
@@ -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'; | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
@@ -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 ); | ||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we call |
||||||||||||||||||||||||||||||||||||
await selectBlockByName( block.slug ); | ||||||||||||||||||||||||||||||||||||
await openBlockEditorSettings(); | ||||||||||||||||||||||||||||||||||||
await page.waitForXPath( | ||||||||||||||||||||||||||||||||||||
block.selectors.editor.filterButtonToggle | ||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
const [ filterButtonToggle ] = await page.$x( | ||||||||||||||||||||||||||||||||||||
selectors.editor.filterButtonToggle | ||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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? woocommerce-blocks/tests/e2e/specs/backend/product-query/advanced-filters.ts Lines 149 to 155 in 8c857e2
|
||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||
} ); |
There was a problem hiding this comment.
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
fromget_query_vars_from_filter_blocks()
here so we can centralize the references of filter query variable constants?There was a problem hiding this comment.
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 useget_query_vars_from_filter_blocks
. I'm happy to refactor them!