Add product management SKU #34978

joshuatf · 2022-10-06T14:15:20Z

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:

Adds the SKU field and inventory section to the new product management experience.

Closes #34386 .

How to test the changes in this Pull Request:

Visit the new product page Products -> Add new (MVP)
Type in a product name and blur the input
Note the SKU has been added in slug format to the SKU field
Change the SKU to a bad format including non-alphanumeric characters and spaces
Note the error
Make a very long SKU (greater than 160 characters) and note the error
Save and make sure the SKU persists on refresh
Remove the product name so it's empty
Blur the field and make sure the error appears

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 --filter=<project> run changelog add?

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-10-06T14:36:40Z

Test Results Summary

Commit SHA: 91ef513

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	188	0	0	2	0	190	0m 44s
E2E Tests	124	4	2	8	0	138	13m 46s

⚠️ Warning

Please address the following issues prior to merging this pull request:

FAILED/BROKEN TESTS. There were failed and/or broken API and E2E tests.
INCOMPLETE E2E TEST RUN. We have a total of 189 E2E tests, but only 138 were executed. Note that in CI, E2E tests automatically end when they encounter too many failures.

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.

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

plugins/woocommerce-admin/client/products/sections/product-details-section.tsx

joshuatf · 2022-10-07T16:05:56Z

Thanks for the review, Maikel! This PR is ready for another look when you have time.

mattsherman

Nice work on this so far, @joshuatf !

My comments mainly revolve around validation.

I totally missed that we were proposing changing the validation rules for SKU in the original issue. I don't think we should do so, since it will introduce compatibility issues with products added via other means (existing UI, API). /cc @pmcpinto @jarekmorawski

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

plugins/woocommerce-admin/client/products/sections/product-details-section.tsx

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

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

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

pmcpinto · 2022-10-07T17:20:07Z

I totally missed that we were proposing changing the validation rules for SKU in the original issue. I don't think we should do so, since it will introduce compatibility issues with products added via other means (existing UI, API). /cc @pmcpinto @jarekmorawski

Good point. I agree with @mattsherman assessment. IMO the benefits of this change don't outweigh the associated risks

joshuatf · 2022-10-10T19:31:16Z

@mattsherman @mdperez86 This PR has been updated to remove any validation/sanitization logic around SKUs. I addressed a couple of @mattsherman's feedback requests, though a follow-up to improve form context callbacks may be a nice addition in the future.

mattsherman

Thanks for trying to improve the onBlur handling, @joshuatf! Unfortunately, I believe that your original implementation, while coupling the different UI sections less-than-ideally, functioned better. I left a comment suggesting we either go back to that implementation or change to an implementation where the SKU is updated on-the-fly as the name field changes.

mattsherman · 2022-10-12T15:22:41Z

plugins/woocommerce-admin/client/products/sections/product-inventory-section.tsx

+	};
+
+	const isNameBlurred = ! prevTouchedName && touched.name;
+	useEffect( setSkuIfEmpty, [ isNameBlurred ] );


This does not handle the scenario where the user:

Focuses on empty name field

Blurs to another field (without first entering anything in name field)

Focuses on empty name field

Enters name

Blurs to another field

Perhaps it would be better to go back to our original implementation, which tied to the onBlur of the field itself. I don't love that, since it couples the two UI implementations more than ideal, but it does work better.

Perhaps the more ideal solution needs to be addressed in the future. Maybe we need a way to hook into focus change at the Form level, so we (and extensions) could hook into whether the focus changed from one component to another at the Form level.

Or, if you changed the implementation to always update the SKU on the fly as the name value changed, keystroke by keystroke, as long as it was a new product and the user hadn't manually modified the SKU. That would be pretty slick, as long as it wasn't a performance issue.

This was definitely a hacky solution at best. I agree with moving back to the onBlur. I'll open an issue to introduce a state handler to form context so we can detect various state changes to the form outside of specific components.

I've reverted this in 60da981

mattsherman · 2022-10-12T15:25:21Z

plugins/woocommerce-admin/client/products/sections/product-inventory-section.tsx

+						target="_blank"
+						type="wp-admin"
+						onClick={ () => {
+							recordEvent( 'add_product_inventory_help' );


I don't love clean up issues, but perhaps it would be best to address Tracks naming across the board in a follow up clean up issue, so we can get this functionality merged to unblock further MVP work.

As you mention, you were just following existing naming conventions on the new product experience. And, those existing conventions are not in line with best practices/guidelines. /cc @AnnaMag @pmcpinto

plugins/woocommerce-admin/client/products/sections/product-details-section.tsx

joshuatf · 2022-10-12T16:41:57Z

Thanks for the review @mattsherman! I reverted the to the original onBlur method and this PR is ready for another review.

mattsherman

Nice work on this @joshuatf ! LGTM!

I have tested the latest, and it works as expected, with one ambiguous case. The original acceptance criteria didn't detail this, and I can see reasoning behind either behavior, so I'm going to approve this and we can follow up with a future issue if need be...

Scenario:

User enters in a name (SKU updates automatically)
User clears the SKU
User modifies the name (SKU updates automatically)

It is unclear to me what the "best" behavior is in this scenario... should the SKU stay blank because the user manually cleared it, or should it be auto-filled in, since it was blank? It's easy to clear out the SKU again, so I think that how you have it implemented is the preferred way, but it did not meet my expectations when testing, so I'm raising it for discussion.

/cc @jarekmorawski

mattsherman · 2022-10-12T17:03:49Z

@joshuatf Btw... I see the failing "Run tests against PR / Runs E2E tests", but the failures appear completely unrelated, so I think it is safe to bypass branch protections.

joshuatf · 2022-10-12T17:05:27Z

Thanks for the quick review, @mattsherman!

It's easy to clear out the SKU again, so I think that how you have it implemented is the preferred way

I think so as well. Worst case scenario, are there any detrimental effects to having a SKU filled in? Or reasons a user might want a blank SKU?

@jarekmorawski if you have an opinion on this, please let us know and we'll follow up with another issue.

mattsherman

Changes still look good to me.

github-actions · 2022-10-12T20:11:24Z

Hi @joshuatf, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

Add the release: add testing instructions label

jarekmorawski · 2022-10-13T07:14:27Z

Worst case scenario, are there any detrimental effects to having a SKU filled in?

I don't think so. I'd go further and say that it's only beneficial since both WooCommerce and many inventory management systems that the merchants use treat the SKU as a product UID. It's perfectly fine to have it filled in again when the name changes.

joshuatf requested a review from a team October 6, 2022 14:15

joshuatf self-assigned this Oct 6, 2022

github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Oct 6, 2022

jonathansadowski removed the type: community contribution label Oct 6, 2022

github-actions bot added the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 6, 2022

AnnaMag added section: PM inventory labels Oct 7, 2022

mdperez86 suggested changes Oct 7, 2022

View reviewed changes

plugins/woocommerce-admin/client/products/product-validation.ts Outdated Show resolved Hide resolved

plugins/woocommerce-admin/client/products/sections/product-details-section.tsx Show resolved Hide resolved

mattsherman suggested changes Oct 7, 2022

View reviewed changes

mdperez86 previously approved these changes Oct 7, 2022

View reviewed changes

joshuatf dismissed mdperez86’s stale review via d08c11b October 10, 2022 19:04

joshuatf force-pushed the add/34386 branch from b00dbf9 to d08c11b Compare October 10, 2022 19:04

github-actions bot removed the release: highlight Issues that have a high user impact and need to be discussed/paid attention to. label Oct 10, 2022

mattsherman suggested changes Oct 12, 2022

View reviewed changes

joshuatf force-pushed the add/34386 branch from 91cdfd5 to 60da981 Compare October 12, 2022 16:33

mattsherman previously approved these changes Oct 12, 2022

View reviewed changes

joshuatf added 5 commits October 12, 2022 11:30

Add product inventory section and sku control

82d6047

Add validation and errors

d9e8d17

Add changelog entry

6dc1de0

Add dependencies comment block

858bdeb

Call input props onBlur for product name field

b4799e4

joshuatf added 4 commits October 12, 2022 11:30

Remove slug validation

fc048b2

Watch product name value change in inventory section

f465a99

Fix up usePrevious hook TS

0dac601

Revert watching changes by previous touched status

91ef513

joshuatf dismissed mattsherman’s stale review via 91ef513 October 12, 2022 18:31

joshuatf force-pushed the add/34386 branch from 60da981 to 91ef513 Compare October 12, 2022 18:31

mattsherman approved these changes Oct 12, 2022

View reviewed changes

joshuatf merged commit 0ca3f6d into trunk Oct 12, 2022

joshuatf deleted the add/34386 branch October 12, 2022 20:11

github-actions bot added this to the 7.1.0 milestone Oct 12, 2022

github-actions bot mentioned this pull request Nov 10, 2022

Test WooCommerce 7.1 UVLabs/location-picker-at-checkout-for-woocommerce#55

Closed

Add product management SKU #34978

Add product management SKU #34978

Uh oh!

Conversation

joshuatf commented Oct 6, 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 Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results Summary

⚠️ Warning

Uh oh!

Uh oh!

Uh oh!

joshuatf commented Oct 7, 2022

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmcpinto commented Oct 7, 2022

Uh oh!

joshuatf commented Oct 10, 2022

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

joshuatf Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

mattsherman Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joshuatf commented Oct 12, 2022

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

mattsherman commented Oct 12, 2022

Uh oh!

joshuatf commented Oct 12, 2022

Uh oh!

mattsherman left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 12, 2022

Uh oh!

jarekmorawski commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

joshuatf commented Oct 6, 2022 •

edited

Loading

github-actions bot commented Oct 6, 2022 •

edited

Loading