Select the current new added shipping class #35123

mdperez86 · 2022-10-17T14:33:41Z

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:

Closes #35036.

How to test the changes in this Pull Request:

Go to the Add new product page
Click on Shipping class dropdown field
Select Add new shipping class option from the list
From New shipping class modal form, add a new shipping class name then click Add button.
When the modal closed the new shipping class added should be shown as the selected value of the Shipping class dropdown.

Screen.Recording.2022-10-17.at.2.20.11.PM.mov

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-17T14:56:13Z

Test Results Summary

Commit SHA: c32f0e5

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	188	0	0	2	0	190	0m 43s
E2E Tests	186	0	0	3	0	189	15m 46s

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.

joshuatf

Hey @mdperez86, this looks great and is testing well!

Just left a quick suggestion on how we set the new shipping value.

joshuatf · 2022-10-17T22:49:18Z

plugins/woocommerce-admin/client/products/constants.ts

+// This should never be a real slug value of any existing shipping class
+export const ADD_NEW_SHIPPING_CLASS_OPTION_VALUE =
+	'__ADD_NEW_SHIPPING_CLASS_OPTION__';
+export const UNCATEGORIZED_CATEGORY_SLUG = 'uncategorized';


Thanks for moving these constants into here!

I know you're just moving this slug in here so I won't let it hold up this PR, but I'm wondering if this will function as expected since users can change the uncategorized slug and definitely may depending on locale.

Yes, that's interesting! I'm wondering what is the logic WP is using to assign this category by default to any product if the user never select any category at all. If this is a variable thing then how do they deal with this? If you can enlighten me with that answer will be great for me, otherwise I think I should find the answer by myself.
Anyway this is a super great catch!

Looks like the default category is stored in an option - get_option( 'default_product_cat', 0 ).

We should probably rely on that instead of the slug/name since those could be changed.

This is obviously unrelated to this PR though, so I don't want to block your work here. Would you mind creating a follow-up issue for this?

joshuatf · 2022-10-17T22:55:40Z

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

 							Promise< ProductShippingClass >
 						>( values ).then( ( value ) => {
 							invalidateResolution( 'getProductShippingClasses' );
+							selectShippingClassProps?.onChange( value.slug );


Calling the onChange handler from the Form feels a tiny bit hacky to me and something I'd like to avoid if we can.

Would a call to setValue from the useFormContext work well here?

It's a matter of how it reads. But using setValue forces me to know the field name and duplicate it around the code. But I don't have a special oppinion for any of the two solutions.
I can change it by your suggestion.

But using setValue forces me to know the field name and duplicate it around the code

Ya, that's a great point. I guess my main concern is that the intent of the onChange api is to handle changes coming from an input and not necessarily manual changes.

Luckily this handles both events and direct value changes so it works in the current scenario, but I do worry about breaking changes in the future.

Maybe we can introduce something like setValue from getInputProps that acts as a shortcut if we don't want to hold the name of input in a constant.

joshuatf · 2022-10-17T23:00:17Z

plugins/woocommerce-admin/client/products/sections/test/product-shipping-section.spec.tsx

+	useDispatch: jest.fn(),
+} ) );
+
+describe( 'ProductShippingSection', () => {


Thanks for adding these tests ❤️

Definitively!!!

…duct

joshuatf

Code looks great and is testing well! Thanks, Maikel!

joshuatf · 2022-10-18T15:43:50Z

plugins/woocommerce-admin/client/products/constants.ts

+// This should never be a real slug value of any existing shipping class
+export const ADD_NEW_SHIPPING_CLASS_OPTION_VALUE =
+	'__ADD_NEW_SHIPPING_CLASS_OPTION__';
+export const UNCATEGORIZED_CATEGORY_SLUG = 'uncategorized';


Looks like the default category is stored in an option - get_option( 'default_product_cat', 0 ).

We should probably rely on that instead of the slug/name since those could be changed.

This is obviously unrelated to this PR though, so I don't want to block your work here. Would you mind creating a follow-up issue for this?

github-actions · 2022-10-18T18:41:16Z

Hi @mdperez86, 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

mdperez86 added the focus: product management MVP label Oct 17, 2022

mdperez86 requested a review from a team October 17, 2022 14:33

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

mdperez86 marked this pull request as ready for review October 17, 2022 17:29

mdperez86 self-assigned this Oct 17, 2022

joshuatf reviewed Oct 17, 2022

View reviewed changes

mdperez86 added 3 commits October 18, 2022 10:19

Select the current new added shipping class

f580aa1

Extracting constants into the right file

b72626d

Add unit tests

f6b18fc

mdperez86 force-pushed the add/35036-select-created-class branch from bcc9424 to f6b18fc Compare October 18, 2022 13:20

Use setValue instead onChange to select the shipping class of the pro…

c32f0e5

…duct

mdperez86 requested a review from joshuatf October 18, 2022 13:29

joshuatf approved these changes Oct 18, 2022

View reviewed changes

mdperez86 mentioned this pull request Oct 18, 2022

[Shipping] Use the default category option instead the uncategorized slug #35158

Open

mdperez86 merged commit a0b27a4 into trunk Oct 18, 2022

mdperez86 deleted the add/35036-select-created-class branch October 18, 2022 18:40

github-actions bot added this to the 7.2.0 milestone Oct 18, 2022

github-actions bot mentioned this pull request Dec 15, 2022

Test WooCommerce 7.2 UVLabs/location-picker-at-checkout-for-woocommerce#57

Closed

github-actions bot mentioned this pull request Jan 25, 2023

Test WooCommerce 7.3 UVLabs/location-picker-at-checkout-for-woocommerce#59

Closed

joelclimbsthings mentioned this pull request Mar 2, 2023

Migrating details fields to product-editor package #36945

Merged

8 tasks

Select the current new added shipping class #35123

Select the current new added shipping class #35123

Uh oh!

Conversation

mdperez86 commented Oct 17, 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 17, 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!

joshuatf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mdperez86 commented Oct 17, 2022 •

edited

Loading

github-actions bot commented Oct 17, 2022 •

edited

Loading