Fix race condition when rendering product attributes tab empty state #38354

mattsherman · 2023-05-18T20:01:45Z

Submission Review Guidelines:

I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have reviewed my code for security best practices.
Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

This PR fixes a race condition that could occur when the first (empty) attribute form was automatically added to the attributes tab of the product (the updated empty state from #38126).

Basically, in an environment such as our e2e tests, where the UI interactions occurred very quickly, the new attribute form would be added using a stale value of the product type, resulting in a missing "Used for variations" checkbox. Note that this is very unlikely to be reproducible manually, as humans just aren't that fast when interacting with the UI.

Flow in code that caused issue:

Product edit screen loads
Ajax call to get new attribute form (empty state) was automatically made, with product type Simple
Product type was switched to Variable by user
Controls were shown/hidden based on updated product type
Ajax call to get new attribute form returned, and used the stale product type Simple to determine if "Used for variations" on the newly added attribute should be shown (thus hiding it)

The implementation has been updated to check the product type immediately before showing/hiding control on the new attribute form.

In addition, a number of refactors have been made to make this area of the code simpler and more maintainable, to lessen the chance of similar issues occurring in the future when further changes to the code are made.

There are no visible UI changes introduced in this PR, outside of the fixing of the visibility of the "Used for variations" checkbox.

How to test the changes in this Pull Request:

As this bug was introduced in #38126, the same test instructions from there should be followed (see below).

In addition, general smoke testing surrounding the following should be done:

Changing product type, and making sure controls/tabs show/hide appropriately

Common instructions

To create a new product, go to Products > Add New
To edit an existing product, go to Products > All Products
To add or remove global attributes, go to Products > Attributes
To save attributes, click Save attributes button on the Attributes tab
To save a product, click Publish (or Update, if an existing product)
To see Tracks events
- Tools > WCA Test Helper > Tools > Enable wc-admin Tracking
- Filter by recordevent in the browser dev console

No global attributes in system

Remove all global attributes from the system
Create a new product
Go to the Attributes tab
Verify that the empty attribute form is shown
Fill out the attribute and save it
Save the product and verify the attributes are correct
Reload the product and verify the attributes are correct

Global attributes in system, adding a new local attribute

Add at least one global attribute to the system, and configure terms for it
Create a new product
Go to the Attributes tab
Verify that the empty attribute form is shown
Fill out the attribute and save it
Save the product and verify the attributes are correct
Reload the product and verify the attributes are correct

Global attributes in system, adding a global attribute

Add at least one global attribute to the system, and configure terms for it
Create a new product
Go to the Attributes tab
Verify that the empty attribute form is shown
Select a global attribute from the Add existing dropdown
Verify the empty attribute form is removed
Verify the same global attribute cannot be selected again from the Add existing dropdown
Select terms for the global attribute and save it
Save the product and verify the attributes are correct
Reload the product and verify the attributes are correct

Editing an existing product with no attributes

Edit one of your existing products no attributes
Verify the empty attribute form is automatically shown when the Attributes tab is shown

Editing an existing product with local attributes

Edit one of your existing products with local attributes (either saved from above or before)
Verify the empty attribute form is not automatically shown when the Attributes tab is shown
Verify the attributes are correct

Editing an existing product with global attributes

Edit one of your existing products with global attributes (either saved from above or before)
Verify the empty attribute form is not automatically shown when the Attributes tab is shown
Verify the attributes are correct

Tracks events

Clicking on Add new should log wcadmin_product_attributes_buttons with action: 'add_new'
Clicking on an existing attribute in Add existing should log wcadmin_product_attributes_buttons with action: 'add_existing'
Clicking on Create value for a global attribute should log wcadmin_product_attributes_add_term

Error handling

Clicking on Add new when offline (or any other server-side error), should show a browser alert message and log the error to the browser console log.

github-actions · 2023-05-18T20:05:31Z

Hi @octaedro,

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 · 2023-05-18T20:36:13Z

Test Results Summary

Commit SHA: 2b58151

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	267	0	0	2	0	269	0m 53s
E2E Tests	194	0	0	10	0	204	16m 58s

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.

mattsherman · 2023-05-18T20:43:26Z

plugins/woocommerce/tests/e2e-pw/tests/merchant/create-variable-product.spec.js

+			// we've seen some flakiness where the "Used for variations" checkbox is not visible,
+			// so let's explicitly wait for it to be visible before proceeding
+			await page
+				.locator( '#product_attributes' )
+				.locator( `input[name="attribute_variation[${ i }]"]` )
+				.waitFor( { timeout: 10000 } );
+


@rodelgc I added this explicit wait for the "Used for variations" checkbox to help assure us that the underlying issue is fixed. That way, if it is missing, the test will fail with a timeout. Or, do you think we either don't need this at all, or should make this an expect?

octaedro · 2023-05-19T15:15:32Z

plugins/woocommerce/client/legacy/js/admin/meta-boxes-product.js


-			attribute_row_indexes();
+	async function add_attribute_to_list( globalAttributeId ) {
+		try {


I think that if we will use exception handling, we should add a catch to this try.

I didn't add a catch because I don't have anything terribly helpful to do at this point, so I figured I'd just let the error bubble up. I did the finally because because I didn't want to totally lock the UI up in this failure case.

I could put up an alert... do you think that is good enough?

I could put up an alert... do you think that is good enough?

An alert mentioning the error sounds good to me 👍

Sounds good. I'll make that change and fix the conflicting test file right after lunch and ping you when ready for another review.

octaedro

Good job @mattsherman! This is testing well here and the code looks good. I just left a small comment.

…dd_custom_attribute_to_list

mattsherman · 2023-05-19T18:12:05Z

@octaedro I added the alert.

You can test by loading the product screen and then using your browser's dev tools Network conditions > Network throttling set to Offline. Click "Add new" and you will trigger the fail case.

I removed the e2e test change I made, as the e2e tests have been restructured and looking at the new structure of them, I don't see a need to add my check back in. If we happen to have future issues, the existing e2e tests should cover things.

Ready for re-review!

octaedro

Thank you @mattsherman for adding the change. The PR LGTM 🚀

github-actions bot added focus: e2e tests Issues related to e2e tests plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 18, 2023

mattsherman changed the title ~~Fix race condition when adding 1st attribute form to the product~~ Fix race condition when rendering product attributes tab empty state May 18, 2023

mattsherman added type: bug The issue is a confirmed bug. focus: product management Related to product creation and editing. labels May 18, 2023

mattsherman self-assigned this May 18, 2023

mattsherman requested a review from a team May 18, 2023 20:05

mattsherman marked this pull request as ready for review May 18, 2023 20:38

mattsherman commented May 18, 2023

View reviewed changes

octaedro requested review from octaedro and removed request for a team May 19, 2023 12:08

octaedro reviewed May 19, 2023

View reviewed changes

mattsherman added 15 commits May 19, 2023 13:51

Remove unused code

567da07

Refactor add_attribute to separate add_global_attribute_to_list and a…

d59488c

…dd_custom_attribute_to_list

Rename size variable

109f007

Rename additional variables

97a22c8

Call add_custom_attribute_to_list directly

cd55a4b

Remove unused event trigger

b94e593

Refactor to move ajax call to a promise

cefbdfa

Change vars to consts

675d65c

Refactor blocking/unblocking

517fb93

Extract show_and_hide_controls function

6da0089

Move changing of tab off of shipping to general for virtual

ff0ac4f

Fix typo

6d28992

Use show_and_hide_controls in add_attribute_to_list

e9777eb

Simply expanding newly added attribute

5267bca

Rename attribute_row_indexes to update_attributes_row_indexes

5a4e948

mattsherman added 4 commits May 19, 2023 13:51

Extract toggle_expansion_of_attribute_list_item

a11bab3

Extract init_select_controls

ac8f778

Changelog

80928e5

Show alert when adding attribute fails

2b58151

mattsherman force-pushed the fix/product-add-attributes-race-condition branch from a759995 to 2b58151 Compare May 19, 2023 17:52

github-actions bot removed the focus: e2e tests Issues related to e2e tests label May 19, 2023

mattsherman requested a review from octaedro May 19, 2023 18:08

octaedro approved these changes May 19, 2023

View reviewed changes

mattsherman merged commit 08d7c24 into trunk May 19, 2023
17 of 18 checks passed

mattsherman deleted the fix/product-add-attributes-race-condition branch May 19, 2023 23:15

github-actions bot added this to the 7.8.0 milestone May 19, 2023

mattsherman mentioned this pull request Jun 20, 2023

Do not show "Adding new attribute failed" error message when loading of product screens is interrupted by page unload #38815

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition when rendering product attributes tab empty state #38354

Fix race condition when rendering product attributes tab empty state #38354

mattsherman commented May 18, 2023 •

edited

github-actions bot commented May 18, 2023 •

edited

github-actions bot commented May 18, 2023 •

edited

mattsherman May 18, 2023

octaedro May 19, 2023 •

edited

mattsherman May 19, 2023

octaedro May 19, 2023 •

edited

mattsherman May 19, 2023

octaedro left a comment

mattsherman commented May 19, 2023

octaedro left a comment

Fix race condition when rendering product attributes tab empty state #38354

Fix race condition when rendering product attributes tab empty state #38354

Conversation

mattsherman commented May 18, 2023 • edited

Submission Review Guidelines:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Common instructions

No global attributes in system

Global attributes in system, adding a new local attribute

Global attributes in system, adding a global attribute

Editing an existing product with no attributes

Editing an existing product with local attributes

Editing an existing product with global attributes

Tracks events

Error handling

github-actions bot commented May 18, 2023 • edited

github-actions bot commented May 18, 2023 • edited

Test Results Summary

mattsherman May 18, 2023

Choose a reason for hiding this comment

octaedro May 19, 2023 • edited

Choose a reason for hiding this comment

mattsherman May 19, 2023

Choose a reason for hiding this comment

octaedro May 19, 2023 • edited

Choose a reason for hiding this comment

mattsherman May 19, 2023

Choose a reason for hiding this comment

octaedro left a comment

Choose a reason for hiding this comment

mattsherman commented May 19, 2023

octaedro left a comment

Choose a reason for hiding this comment

mattsherman commented May 18, 2023 •

edited

github-actions bot commented May 18, 2023 •

edited

github-actions bot commented May 18, 2023 •

edited

octaedro May 19, 2023 •

edited

octaedro May 19, 2023 •

edited