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
Product Collection: Make attributes available in rest_product_query hook #44150
Conversation
This commit introduces the 'includeInQueryContext' attribute to the 'woocommerce/product-collection' block and updates the 'woocommerce/product-template' block to consume this new attribute. Key Changes: 1. `woocommerce/product-collection` Block: - A new attribute 'includeInQueryContext' is added in `block.json`. This attribute is designed to hold a list of attribute names relevant for the query context. - The 'includeInQueryContext' attribute is included in the `providesContext` field to ensure its availability to child blocks. - In `constants.ts`, default values for 'includeInQueryContext' are defined, specifying 'collection' and 'id' as initial attributes. - The `types.ts` file is updated with a comment explaining the purpose of 'includeInQueryContext'. 2. `woocommerce/product-template` Block: - Modified `block.json` to utilize the 'includeInQueryContext' context provided by the parent `woocommerce/product-collection` block. - The `edit.tsx` file is updated to handle the new context. It uses a newly added utility function `useProductCollectionBlockAttributes` from `utils.tsx` to access parent block attributes. - The `utils.tsx` file is introduced, containing the `useProductCollectionBlockAttributes` hook. This hook is responsible for finding the parent 'woocommerce/product-collection' block and returning its attributes. - Within `edit.tsx`, logic is added to create a query context object based on the attributes specified in 'includeInQueryContext', enhancing the block's ability to dynamically adapt to changes.
Test Results SummaryCommit SHA: 10c90f5
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @xristos3490, @joshuatf, @kmanijak, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
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.
Looks fantastic, Manish! Left a few nitpicky comments, but love the overall concept.
plugins/woocommerce-blocks/assets/js/blocks/product-collection/block.json
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/product-template/utils.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/product-template/edit.tsx
Outdated
Show resolved
Hide resolved
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.
Great work @imanish003! 💪 Left couple of suggestions for refactoring but I think that POC proves the point and we can progress in that direction!
plugins/woocommerce-blocks/assets/js/blocks/product-template/utils.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/product-template/edit.tsx
Outdated
Show resolved
Hide resolved
plugins/woocommerce-blocks/assets/js/blocks/product-collection/constants.ts
Outdated
Show resolved
Hide resolved
This commit introduces two significant changes aimed at improving code readability and efficiency. 1. **Renaming of Query Context Attribute:** - The attribute `includeInQueryContext` has been renamed to `queryContextIncludes` across various files, including block JSON configurations and TypeScript definitions. This change makes the attribute's purpose more intuitive, indicating it specifies which attributes to include in the query context. 2. **Optimized Parent Block Detection:** - Replaced the use of `getBlockParents` with `getBlockParentsByBlockName` in utility functions to find the closest Product Collection block. This optimization allows for a more direct and efficient way to identify the relevant parent block by specifying the block name, reducing unnecessary iterations and improving code performance.
Key Changes: - **Introduction of `useProductCollectionQueryContext` Hook:** This new hook takes the `clientId` and `queryContextIncludes` as inputs and returns a query context object. It encapsulates the logic for fetching parent product collection block attributes and constructing the query context accordingly. This abstraction simplifies the edit component's logic, focusing on the essentials and improving code readability. - **Optimization of Parent Block Detection:** The hook uses `getBlockParentsByBlockName` to accurately and efficiently find the closest parent `Product Collection` block, minimizing the overhead previously associated with traversing the block hierarchy.
…to try/query-context-block-params
Hi @kmanijak, |
…to try/query-context-block-params
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.
I left one suggestion but overall - it looks great! You can consider adding some tests to it though.
Also, I realized one test I recently added is flaky and the CI is failing on this PR. I updated the test in this PR: #44404 hopefully it'll get merge soon.
const queryContext: { | ||
[ key: string ]: unknown; | ||
} = { | ||
collection: productCollectionBlockAttributes?.collection, | ||
id: productCollectionBlockAttributes?.id, | ||
}; |
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.
I'm not convinced these attributes should be handled in the hook. I'd lift that part upstream to plugins/woocommerce-blocks/assets/js/blocks/product-template/edit.tsx
so the hook is generic and doesn't care/is not aware of some specific attributes. But I don't have a strong opinion here so feel free to ignore this comment.
Alternatively, I'd extract this mapping to some variable because I'm pretty sure we'll find more attributes like these that we will like to always include in query. In that case it would be easier to extend some standalone object then finding this initialization here and extend it.
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.
Great suggestion! I like the idea of using a constant variable to keep track of the default query context attributes. I've implemented the changes in d4f071d. Could you please have a quick look? 👀
…to try/query-context-block-params
- Introduced `DEFAULT_QUERY_CONTEXT_ATTRIBUTES` in `edit.tsx` to maintain a clear list of default query context attributes. - Modified `ProductTemplateEdit` to automatically include these default attributes in `queryContextIncludes`, ensuring they are always part of the query context without manual initialization. - Simplified `useProductCollectionQueryContext` in `utils.tsx` by removing static initialization of 'collection' and 'id', relying instead on the dynamic addition of necessary attributes from `queryContextIncludes`. This refactor enhances the maintainability and clarity of the code, ensuring a solid foundation for future enhancements and features.
- Added a new test suite 'Query Context in Editor' to validate the correctness of query context parameters when the Product Collection block is used. This suite ensures that: - For the 'Product Catalog', only the ID is sent in the query context, confirming that collection-specific parameters are excluded when not relevant. - For collections such as 'On Sale', the collection name is correctly passed in the query context, validating that the block dynamically adjusts query parameters based on its settings. - Introduced a new utility method `setupAndFetchQueryContextURL` in `product-collection.page.ts`. This method automates the setup of a post with the Product Collection block and fetches the URL with query context parameters, facilitating the validation of query context handling.
…to try/query-context-block-params
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.
There's one issue to fix for existing Product Collection blocks. Otherwise it looks good!
// Add default query context attributes to queryContextIncludes | ||
queryContextIncludes = [ | ||
...new Set( | ||
queryContextIncludes.concat( DEFAULT_QUERY_CONTEXT_ATTRIBUTES ) | ||
), | ||
]; | ||
|
||
const productCollectionQueryContext = useProductCollectionQueryContext( { | ||
clientId, | ||
queryContextIncludes, | ||
} ); | ||
|
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.
I suggest not to modify the props value directly, especially since the queryContextIncludes
may be undefined
for existing Product Collection blocks:
// Add default query context attributes to queryContextIncludes | |
queryContextIncludes = [ | |
...new Set( | |
queryContextIncludes.concat( DEFAULT_QUERY_CONTEXT_ATTRIBUTES ) | |
), | |
]; | |
const productCollectionQueryContext = useProductCollectionQueryContext( { | |
clientId, | |
queryContextIncludes, | |
} ); | |
// Add default query context attributes to queryContextIncludes | |
const allQueryContextIncludes = queryContextIncludes ? [ | |
...new Set( | |
queryContextIncludes.concat( DEFAULT_QUERY_CONTEXT_ATTRIBUTES ) | |
), | |
] : [ DEFAULT_QUERY_CONTEXT_ATTRIBUTES ]; | |
const productCollectionQueryContext = useProductCollectionQueryContext( { | |
clientId, | |
allQueryContextIncludes, | |
} ); | |
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.
Good catch! I have made the changes in 30aec35. Your attention to detail during the PR review is truly appreciated 🙌🏻
- Initializing `queryContextIncludes` with a default empty array directly in the destructuring assignment of the component's props. This approach ensures that `queryContextIncludes` is always an array, simplifying downstream logic. - Creating a new constant `queryContextIncludesWithDefaults` to hold the combination of `queryContextIncludes` and `DEFAULT_QUERY_CONTEXT_ATTRIBUTES`. This step avoids directly mutating the `queryContextIncludes` prop, aligning with best practices for functional purity and making the code easier to understand and debug. - Updating the `useProductCollectionQueryContext` hook call to use `queryContextIncludesWithDefaults`. This ensures that the default query context attributes are consistently included without altering the original prop. These adjustments not only enhance the code's maintainability but also ensure more predictable behavior by avoiding side effects related to parameter mutation.
…to try/query-context-block-params
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.
Thanks for the patience and addressing all the edge cases! LGTM! 🚀
Changes proposed in this Pull Request:
This PR introduces a new attribute,
queryContextIncludes
, to the Product Collection block. This attribute specifies a list of attribute names that should be included in theproductCollectionQueryContext
query param when making REST API query.rest_product_query
hook can use this to modify the WP Query in Editor.Changes include:
queryContextIncludes
attribute toblock.json
of both Product Collection and Product Template blocks. This attribute is designed to hold an array of attribute names to be included in thequeryContext
.DEFAULT_ATTRIBUTES
inconstants.ts
for the Product Collection block to includequeryContextIncludes
with default values.useProductCollectionQueryContext
, inutils.tsx
for the Product Template block. This hook constructs the query context object based on the attribute names provided inqueryContextIncludes
, allowing for dynamic inclusion of attributes in API queries.Closes #44294
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
/wp-json/wp/v2/product
.queryContextIncludes
attribute. For example, if adding thequeryId
, your modified array might look like this:queryContextIncludes: ["collection", "id", "queryId"]
./wp-json/wp/v2/product
REST API request, indicating successful integration. Refer to the example screenshot below:queryContextIncludes
. Because we always need these in query context.Changelog entry
Significance
Type
Message
Add
queryContextIncludes
attribute to Product Collection blockComment