-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add error specific messages to product save functionality #38307
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
Conversation
|
Hi , @woocommerce/mothra 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: |
Test Results SummaryCommit SHA: 859218d
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. |
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.
This tested well, and code looks good.
Seems a little bit redundant and silly, but I still think this is the best approach. Do we like this language or prefer something else?
I wonder if it would be worth adding a call to action with a link to the SKU field, in this case a link to the Inventory tab. Something like - Update SKU in Inventory tab where inventory tab is a link.
Are there any other common errors we need to account for that should be included in this PR?
Don't think so, the other errors within product abstract are related to invalid catalog, invalid tax status, invalid download. These don't seem relevant to us at the moment. Unless there are some specific post related errors?
I did notice one bug while testing, but also reproducible in trunk, where if you save a product and it fails and update the SKU and save successfully again the route doesn't get updated to the product id ( new issue: #38319 )
I like this idea. @jarekmorawski how does this sound to you? If it looks good (or you have any additional comments), let me know and I'll open an issue around this. |
That sounds great to me. However, shouldn't we surface this error below the SKU field? Then, if it's the only error, we'd put it immediately in focus and highlight the content so the user can update it. This is somehow related to #38194. If the snackbar is the only choice (which I hope isn't the case because we should handle all errors consistently across the form), we could tweak the copy to be a bit more human. The icon is optional.
|
|
@louwie17 This PR needed a rebase. Could you give it another look when you have time? There should not be any additional changes. |
louwie17
left a comment
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.
LGTM 🚀

Submission Review Guidelines:
Changes proposed in this Pull Request:
A couple questions around this PR:
Invalid or duplicated SKU.. Seems a little bit redundant and silly, but I still think this is the best approach. Do we like this language or prefer something else? cc @jarekmorawskiCloses #38215 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
/wp-admin/tools.php?page=woocommerce-admin-test-helperproduct-block-editor/wp-admin/admin.php?page=wc-admin&path=/add-product