Add withCategories and withAttributes HOCs #935

Aljullu · 2019-09-02T10:53:18Z

This PR:

Creates withCategories and withAttributes HOC.
Makes it so Product Category Control and Product Attribute Control display API errors to the user.
Refactors error handling in all HOCs so JS errors can be handled too.

Screenshots

How to test the changes in this Pull Request:

Create a post with a Products by Category and Products by Attribute blocks.
In src/RestApi/Controllers/ProductCategories.php modify this line with:

-if ( ! $taxonomy || ! \taxonomy_exists( $taxonomy ) ) {
+if( true ) {

In src/RestApi/Controllers/ProductAttributes.php modify this one with:

-if ( ! current_user_can( 'edit_posts' ) ) {
+if ( true ) {

Reload the post and verify errors are displayed.

…trol

assets/js/base/utils/errors.js

nerrad · 2019-09-03T17:12:13Z

assets/js/base/utils/errors.js

-		// let <ApiErrorPlaceholder /> handle the message to display.
-		apiMessage: null,
-	};
+const formatErrorMessage = ( error, messageProperty = 'frontendMessage' ) => {


I think it'd be useful to have a doc block here, especially for describing what messageProperty is a reference to and acceptable values.

Also I'd be wary of getting to context specific with frontendMessage string here. What about userFriendly instead (I'm guessing the idea here is a non-dev type error message?)

frontendMessage was referring to errors produced by the frontend, but not necessarily user-friendly. For example, a JS error.

I completely refactored that part of the code in 2ac40b0, let me know how it looks now.

frontendMessage was referring to errors produced by the frontend, but not necessarily user-friendly. For example, a JS error

That's a good argument for not using frontend though as it's still ambiguous. In continuing with the intended purpose, I would expect it to be something like general (for general JS error) as opposed to api (for error in an api response).

I have some more feedback on the refactor in my review comments to be published soon (including a suggestion to continue iterating on it in a separate pull so we don't block this one).

nerrad · 2019-09-03T17:16:44Z

assets/js/components/api-error-placeholder/error-message.js

@@ -5,11 +5,21 @@ import { __ } from '@wordpress/i18n';
 import PropTypes from 'prop-types';
 import { escapeHTML } from '@wordpress/escape-html';

-const getErrorMessage = ( { apiMessage, message } ) => {
+const getErrorMessage = ( { apiMessage, message, frontendMessage } ) => {


Similar comment here as I left elsewhere. I think frontendMessage is too specific a variable name. I anticipate this may be a type of message left in non frontend contexts as well. Would userFriendlyMessage or informalMessage be a better fit here?

assets/js/components/product-attribute-control/index.js

assets/js/components/product-category-control/index.js

nerrad · 2019-09-03T17:21:20Z

assets/js/components/utils/index.js

+	return apiFetch( {
+		path: addQueryArgs( `${ ENDPOINTS.products }/categories`, { per_page: -1 } ),
+	} );
+};


Not needed in this pull, but with the number of blocks calling product categories, this seems like a good thing to move to a wp.data store so that blocks using categories can share what is retrieved. This will help with limiting the number of queries to the server (same for product attributes and product attribute terms). That's something I can do in a pull?

Sure, sounds like a good idea! 🙂

assets/js/hocs/test/with-attributes.js

Aljullu · 2019-09-04T10:34:42Z

Thanks for the review @nerrad. This is ready for another look.

assets/js/base/utils/errors.js

nerrad · 2019-09-04T13:29:31Z

assets/js/base/utils/errors.js

+	return {
+		message: error.message,
+		type: error.type || 'frontend',
+	};


I like the direction with providing a "type" to the error. However:

defaults vary in the logic. Why?

any caught exception always defaults to frontend as the type. Why?

I'm not sure we've landed on a good api for error handling yet, but it probably should be iterated on in it's own pull.

The rationale behind that was that an API error will be a Response object, so it will have a json method, while errors produced in the frontend will not have that method.

Right, there's a bit of code smell here with the confusing logic. Let's iterate on this in a follow-up (not urgent).

assets/js/base/utils/errors.js

nerrad · 2019-09-04T13:35:51Z

assets/js/base/utils/errors.js

-		// let <ApiErrorPlaceholder /> handle the message to display.
-		apiMessage: null,
-	};
+const formatErrorMessage = ( error, messageProperty = 'frontendMessage' ) => {


frontendMessage was referring to errors produced by the frontend, but not necessarily user-friendly. For example, a JS error

That's a good argument for not using frontend though as it's still ambiguous. In continuing with the intended purpose, I would expect it to be something like general (for general JS error) as opposed to api (for error in an api response).

I have some more feedback on the refactor in my review comments to be published soon (including a suggestion to continue iterating on it in a separate pull so we don't block this one).

nerrad · 2019-09-04T13:37:46Z

assets/js/components/product-attribute-control/index.js

-	}
-
-	onExpandAttribute( item ) {
+const ProductAttributeControl = ( { attributes, error, expandedAttribute, onChange, onOperatorChange, isLoading, operator, selected, termsAreLoading, termsList } ) => {


This is a long line and should be split into multiline arguments here. #

Also applies to other refactors to functional components.

How does it sound installing prettier and setting it as a pre-commit hook in a follow-up and dealing with these issues there? That's what we were using in WooCommerce Admin and seems like the kind of job better suited for a machine than for a human. 😄

nerrad · 2019-09-04T13:41:55Z

assets/js/components/product-attribute-control/index.js

-
-	onExpandAttribute( item ) {
+const ProductAttributeControl = ( { attributes, error, expandedAttribute, onChange, onOperatorChange, isLoading, operator, selected, termsAreLoading, termsList } ) => {
+	const onExpandAttribute = ( item ) => {


This should be declared outside of the component otherwise the <SearchListItem /> will receive a new function on every re-render of this component. One of the gotchas of switching to functional components from a class component. It's okay when you're not passing the function through as a prop but it's being passed through as the value for the onSelect prop here.

Of course, this may be a moot point because there are other places where there will always be a new reference passed through as a prop (see here) for example. So if we're fine with <SearchListItem /> always re-rendering when <ProductAttributeControl /> does, then we can just leave things as is I suppose.

onExpandAttribute relies on the onChange prop, so we would need to re-create the function anyway on every re-render with the new onChange, no?

Oh right! yup.

nerrad

I'm happy with where this pull is at for now. Let's iterate on other feedback in their own pulls and 🚢 this.

nerrad · 2019-09-04T15:23:52Z

assets/js/base/utils/errors.js

+	return {
+		message: error.message,
+		type: error.type || 'frontend',
+	};


Right, there's a bit of code smell here with the confusing logic. Let's iterate on this in a follow-up (not urgent).

Aljullu added 6 commits August 30, 2019 11:48

Add withCategories HOC and show API errors in Product Category Control

17a6f2c

Create withAttributes HOC and display errors in Product Attribute Con…

6543360

…trol

Refactor formatError so it handles JS errors and API errors

b22c529

Rename 'onSelectAttribute' with 'onExpandAttribute'

11d41ca

Add and update tests

fc0ef4f

Fix error in product attributes endpoint

0f886d2

Aljullu added status: in progress type: refactor The issue/PR is related to refactoring. labels Sep 2, 2019

Aljullu added this to the 2.5 milestone Sep 2, 2019

Aljullu self-assigned this Sep 2, 2019

Aljullu added status: needs review and removed status: in progress labels Sep 2, 2019

Aljullu requested review from ehg and a team and removed request for ehg September 2, 2019 11:46

nerrad reviewed Sep 3, 2019

View reviewed changes

Aljullu added 4 commits September 4, 2019 11:45

Refactor ProductCategoryControl into a functional component

d4e2ddd

Refactor ProductAttributeControl into a functional component

31b8cd2

Refactor formatError to use a 'message' and 'type' properties

2ac40b0

Replace enzyme with TestRenderer

107e163

Aljullu requested a review from nerrad September 4, 2019 10:34

nerrad reviewed Sep 4, 2019

View reviewed changes

Fix formatErrors doctype format

5e01342

nerrad reviewed Sep 4, 2019

View reviewed changes

Rename 'frontend' error type to 'general'

02df7c2

nerrad approved these changes Sep 4, 2019

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Sep 4, 2019

Aljullu merged commit e01c341 into master Sep 4, 2019

Aljullu deleted the update/errors-categories-attributes branch September 4, 2019 16:07

This was referenced Sep 5, 2019

Install prettier and reformat files #945

Merged

Rename ApiErrorPlaceholder to ErrorPlaceholder #955

Merged

Aljullu added the skip-changelog PRs that you don't want to appear in the changelog. label Sep 20, 2019

Aljullu mentioned this pull request Oct 4, 2019

Fix ProductAttributeControl not reacting to clicks #1021

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add withCategories and withAttributes HOCs #935

Add withCategories and withAttributes HOCs #935

Aljullu commented Sep 2, 2019

nerrad Sep 3, 2019

Aljullu Sep 4, 2019

nerrad Sep 4, 2019 •

edited

Loading

nerrad Sep 3, 2019

nerrad Sep 3, 2019

Aljullu Sep 4, 2019

Aljullu commented Sep 4, 2019

nerrad Sep 4, 2019

Aljullu Sep 4, 2019

nerrad Sep 4, 2019

nerrad Sep 4, 2019 •

edited

Loading

nerrad Sep 4, 2019

Aljullu Sep 4, 2019

nerrad Sep 4, 2019

Aljullu Sep 4, 2019

nerrad Sep 4, 2019

nerrad left a comment

nerrad Sep 4, 2019

Add withCategories and withAttributes HOCs #935

Add withCategories and withAttributes HOCs #935

Conversation

Aljullu commented Sep 2, 2019

Screenshots

How to test the changes in this Pull Request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aljullu commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad Sep 4, 2019 •

edited

Loading

nerrad Sep 4, 2019 •

edited

Loading