Skip to content

Conversation

@joshuatf
Copy link
Contributor

Submission Review Guidelines:

Changes proposed in this Pull Request:

  • Fixes error handling in product saving
  • Adds product specific error messages

A couple questions around this PR:

  • Currently the client-side message matches the server message exactly 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 @jarekmorawski
  • Are there any other common errors we need to account for that should be included in this PR?
Screen Shot 2023-05-15 at 1 25 05 PM

Closes #38215 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Go to /wp-admin/tools.php?page=woocommerce-admin-test-helper
  2. Under Features tab make sure to enable product-block-editor
  3. Then visit /wp-admin/admin.php?page=wc-admin&path=/add-product
  4. Add a sku that has already been used by another product in your store under Inventory -> SKU
  5. Attempt to Preview | Save draft | Save
  6. Make sure that you receive the error message relating to the sku
  7. Update the sku to something unique
  8. Make sure no error is shown

@joshuatf joshuatf requested a review from a team May 15, 2023 20:49
@joshuatf joshuatf self-assigned this May 15, 2023
@github-actions
Copy link
Contributor

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:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented May 15, 2023

Test Results Summary

Commit SHA: 859218d

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests26700202690m 50s
E2E Tests1890010019915m 59s

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.

louwie17
louwie17 previously approved these changes May 16, 2023
Copy link
Contributor

@louwie17 louwie17 left a 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 )

@joshuatf
Copy link
Contributor Author

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.

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.

@jarekmorawski
Copy link

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.

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.

image

@joshuatf
Copy link
Contributor Author

@louwie17 This PR needed a rebase. Could you give it another look when you have time? There should not be any additional changes.

Copy link
Contributor

@louwie17 louwie17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@louwie17 louwie17 merged commit d17ca83 into trunk May 18, 2023
@louwie17 louwie17 deleted the add/38215 branch May 18, 2023 08:28
@github-actions github-actions bot added this to the 7.8.0 milestone May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Product Block Editor: Add additional context to error message when product failed with duplicate sku

4 participants