Add Pricing section #34382

octaedro · 2022-08-18T19:36:24Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

This PR aims to add a Pricing section and the fields List price and Sale price.

In a follow-up PR, we should replace the component ProductSectionLayout with FormSection.

Closes # 37-gh-woocommerce/mothra-private 44-gh-woocommerce/mothra-private .

How to test the changes in this Pull Request:

Go to Products > Add New (MVP) and create a product.
Verify that it's possible to add a List price, and that the price list is correctly formatted after the input loses focus.
Add multiple decimal points and verify that the data is being saved in the DB correctly and that the amount is being shown as it should after leaving the input.
The amount should stay the same after refreshing the page.
Verify that it's possible to add negative numbers.
Go to WooCommerce > Settings > General (URL: /wp-admin/admin.php?page=wc-settings) and change the currency options, verify that the currency reflects the changes that you have made.
Taxes

If tax is included in the price, the helper text reads: Per your store settings, tax is included in the price.
If tax is excluded from the price, the helper text reads: Per your store settings, tax is not included in the price.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully run tests with your changes locally?
Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2022-08-18T20:07:32Z

Test Results Summary

Commit SHA: 77341e2

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	0	0	0	0	0	0	NaNm NaNs
E2E Tests	186	0	0	1	0	187	13m 54s

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.

# Conflicts: # plugins/woocommerce-admin/client/products/add-product-page.tsx # plugins/woocommerce-admin/client/products/layout/product-section-layout.scss # plugins/woocommerce-admin/client/products/sections/product-details-section.tsx

joshuatf

Great work, Fernando! Looking really good.

Just left a few comments/questions.

joshuatf · 2022-08-23T22:32:34Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+	};
+
+	return (
+		<ProductSectionLayout


Could we use the FormSection component now that it's merged?

I like the idea, I added it in the commit 72b843d

joshuatf · 2022-08-23T22:35:12Z

plugins/woocommerce-admin/client/products/product-validation.ts

+
+	if ( values.regular_price && ! /^[0-9.,]+$/.test( values.regular_price ) ) {
+		errors.regular_price = __(
+			'Please enter with one monetary decimal point without thousand separators and currency symbols.',


I guess we're trying to keep this consistent for easier translation, right? I think adding a price may make this slightly more clear.

Suggested change

'Please enter with one monetary decimal point without thousand separators and currency symbols.',

'Please enter a price with one monetary decimal point without thousand separators and currency symbols.',

Although, I would expect a price with thousands separator or currency to work and be sanitized.

Nice catch! Yes, the price is currently sanitized. I added the validation just to be more defensive. It wouldn't hit the validation because I'm sanitizing it in the onChange method. I fixed the text in the commit dd6622e

joshuatf · 2022-08-23T22:35:19Z

plugins/woocommerce-admin/client/products/product-validation.ts

+
+	if ( values.sale_price && ! /^[0-9.,]+$/.test( values.sale_price ) ) {
+		errors.sale_price = __(
+			'Please enter with one monetary decimal point without thousand separators and currency symbols.',


Same comment here.

joshuatf · 2022-08-23T22:36:48Z

plugins/woocommerce-admin/client/products/sections/utils.ts

+		checked,
+		className: classnames( 'woocommerce-add-product__checkbox', className ),
+		onChange: ( isChecked: boolean ) => {
+			recordEvent( `add_product_checkbox_${ name }`, {


Does this only show on the "add prouct page" or will this also show on edit?

Good eyes! I modified this in the commit 99f7048

joshuatf · 2022-08-23T22:38:35Z

plugins/woocommerce-admin/client/products/test/product-form-actions.spec.tsx

+			name: 'My Product',
+			sale_price: 'text',
+		};
+		const productSalePriceWithNotAllowedCharacters: Partial< Product > = {


Love these tests and the clear var naming ❤️

joshuatf · 2022-08-23T22:42:15Z

plugins/woocommerce-admin/client/products/test/product-form-actions.spec.tsx

+			name: 'My Product',
+			sale_price: 'text',
+		};
+		const productSalePriceWithNotAllowedCharacters: Partial< Product > = {


Love these tests and the clear var naming ❤️

joshuatf · 2022-08-23T22:42:22Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+							recordEvent( 'add_product_pricing_help' );
+						} }
+					>
+						How to price your product: expert tips


Think we'll need this localized.

Fixed in the commit 1e35239

joshuatf · 2022-08-23T22:42:54Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+
+			<div className="woocommerce-product-form__custom-label-input">
+				<label htmlFor="sale_price">
+					Sale price&nbsp;


Will need these localized for translation as well. Probably will want to interpolate these to make translation easier.

joshuatf · 2022-08-23T23:53:31Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+			ONLY_ONE_DECIMAL_SEPARATOR.replaceAll( '%s', decimalSeparator ),
+			'g'
+		);
+		const cleanValue = value


If I change my decimal separator to , I'm no longer able to add numbers after the decimal. E.g., 10,59 gets updated to 10,00.

This has been fixed in the commit 55c7c97

joshuatf · 2022-08-23T23:53:56Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+			/>
+			{ ! isTaxSettingsResolving && (
+				<span className="woocommerce-product-form__secondary-text">
+					Per your&nbsp;


Same comment here around localization and interpolation.

octaedro · 2022-08-25T18:31:52Z

Thank you @joshuatf for your review.
I addressed the changes you suggested.
Could you take another look at this PR?

joshuatf

Thanks for changes on this, @octaedro! They look good and are testing well for me.

I left a few optional comments, but one I'd really love to see either here or a follow-up is extracting the sanitizePrice logic to a utils file so we can test it more easily. But I won't block you on this PR if you want to merge and we can follow-up on this.

joshuatf · 2022-08-29T22:36:30Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+			<div className="woocommerce-product-form__custom-label-input">
+				<InputControl
+					label={ __( 'List price', 'woocommerce' ) }
+					placeholder={ __( '10.59', 'woocommerce' ) }


We can always update this later, but I wonder if this is something that shouldn't be translated since store languages don't always match store currencies.

joshuatf · 2022-08-29T23:19:28Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+			'woocommerce'
+		),
+		components: {
+			span: <span className="woocommerce-product-form__optional-input" />,


Would it be worthwhile to include (optional) as its own text here so that it can be translated once? Not sure if this advantageous or not.

Suggested change

span: <span className="woocommerce-product-form__optional-input" />,

span: <span className="woocommerce-product-form__optional-input">{ __( '(optional)', 'woocommerce' ) }</span>,

joshuatf · 2022-08-29T23:24:48Z

plugins/woocommerce-admin/client/products/sections/pricing-section.tsx

+	const context = useContext( CurrencyContext );
+	const { getCurrencyConfig } = context;
+	const { decimalSeparator } = getCurrencyConfig();
+	const priceValidation = ( value: string, name: string ) => {


It would be awesome to extract most of this logic into a separate function so that it's easier to test and this function becomes:

const sanitizedValue = sanitizePrice( value ); setValue( name, sanitizedValue );

octaedro · 2022-08-30T13:26:15Z

Thank you @joshuatf for your review.
I'm going to go ahead and merge this PR and handle the suggestions in a follow-up PR.

github-actions bot added the focus: react admin label Aug 18, 2022

octaedro changed the title ~~Add/30 list price field~~ Add Pricing section Aug 19, 2022

octaedro added 8 commits August 23, 2022 14:30

Add pricing inputs

dd78d1e

# Conflicts: # plugins/woocommerce-admin/client/products/add-product-page.tsx # plugins/woocommerce-admin/client/products/layout/product-section-layout.scss # plugins/woocommerce-admin/client/products/sections/product-details-section.tsx

Add pricing inputs

5e65cd5

Add validation

44de76f

Fix scss

7948760

Moved validations

7aac303

Add tests

752e23a

Add regex validations

7f6147e

Add tax settings

83a46a7

octaedro force-pushed the add/30_list_price_field branch from 6a624ee to 83a46a7 Compare August 23, 2022 17:53

octaedro marked this pull request as ready for review August 23, 2022 17:58

Fix comment

1df9cb3

octaedro requested a review from a team August 23, 2022 18:32

Add product name validation

90f7835

joshuatf reviewed Aug 23, 2022

View reviewed changes

octaedro added 8 commits August 24, 2022 15:10

Fix validation copy

dd6622e

Rename woocommerce-add classes

99f7048

Fix texts i18n

1e35239

Remove image section

c76711b

Fix decimal separator error

55c7c97

Add FormSection component

72b843d

Fix Enriched-label component

b0004a9

Add packages changelog

f578825

github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Aug 25, 2022

octaedro requested a review from joshuatf August 25, 2022 18:33

octaedro added 2 commits August 26, 2022 12:34

Removed not used styles

f3e56b7

Add error handling

77341e2

joshuatf approved these changes Aug 29, 2022

View reviewed changes

octaedro merged commit 3340528 into trunk Aug 30, 2022

octaedro deleted the add/30_list_price_field branch August 30, 2022 13:26

This was referenced Aug 31, 2022

Use label prop on sale price input instead of separate label element #34532

Merged

Products pricing section code cleanup #34560

Merged

Fix product list price help label to properly reflect tax settings #34564

Merged

octaedro mentioned this pull request Sep 7, 2022

Price section - Small refactor and style fix #34558

Merged

8 tasks

mattsherman mentioned this pull request Sep 20, 2022

Fix EnrichedLabel Storybook styles #34736

Merged

8 tasks

roykho mentioned this pull request Sep 21, 2022

Add milestone to PRs based on paths #34767

Closed

	'Please enter with one monetary decimal point without thousand separators and currency symbols.',
	'Please enter a price with one monetary decimal point without thousand separators and currency symbols.',

	span: <span className="woocommerce-product-form__optional-input" />,
	span: <span className="woocommerce-product-form__optional-input">{ __( '(optional)', 'woocommerce' ) }</span>,

Add Pricing section #34382

Add Pricing section #34382

Uh oh!

Conversation

octaedro commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

Uh oh!

github-actions bot commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octaedro commented Aug 25, 2022

Uh oh!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octaedro commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

octaedro commented Aug 18, 2022 •

edited

Loading

github-actions bot commented Aug 18, 2022 •

edited

Loading