WC 6.9 compatibility: Fix the incorrect form states of shipping settings

[eason9487]

Changes proposed in this Pull Request:

Closes #1747

• Fix the WC 6.9 compatibility problem that the shipping_time of the form state will be incorrect when the selection of shipping rate is 'complex'.

Detailed test instructions:

1. Use a WooCommerce version >= 6.9.0
2. Go to the Edit free listings page
3. Change the shipping options:
   • When the recommended or simple shipping rate is selected, shipping time settings should be displayed.
   • When the complex shipping rate is selected, shipping time settings should be hidden.
4. Save changes and then inspect the gla/mc/settings API request to see if the saved data are correct.
   

   google-listings-and-ads/js/src/components/free-listings/setup-free-listings/index.js

   Lines 126 to 127 in e464c76

   	// When shipping rate is 'manual', shipping time should be 'manual' as well;
   	// When shipping rate is 'automatic' or 'flat', shipping time should be 'flat'.

Additional details:

• This fix is NOT compatible with WC < 6.9 but I think it should be fine since we are going to bump the minimum support WC version to 6.9.
• There is a warning as the following as it did request state updates while rendering. Given the continued use of WC Form component and the need to satisfy business logic, this warning might not be avoided, even though the final form state will still be synchronized.
  

Changelog entry

Fix - WC 6.9 compatibility: Shipping time settings should not appear after selecting the "complex" shipping option.
Fix - WC 6.9 compatibility: The free shipping threshold should be cleared after selecting the "No" free shipping option.
Fix - WC 6.9 compatibility: The selected free shipping option should be reset after setting all shipping rates to 0.

[tomalec]

Tested locally, the UI behaves as expected, however, onSettingsChange callback is called 15 times after a single change. That looks like a big overhead. Is this expected, or could we avoid it?

[jorgemd24]

Thanks, @eason9487, I have tested the code and it is fixing the issue with the shipping_time state 👍

However I have some concerns about the root of this problem, when I was debugging this PR I saw that we have a similar issue with the values.offer_free_shipping state, for example:

• when the rate is 0 and values.offer_free_shipping was previously set to "Y"

The state is not updated correctly, values.offer_free_shipping should be updated to 'undefined'. See video:

Screen.Capture.on.2022-11-08.at.16-51-02.mp4

From what I can see it seems that there is an issue when setValue and getInputProps( 'name' ).onChange are combined and the state is somehow overridden.

google-listings-and-ads/js/src/components/shipping-rate-section/flat-shipping-rates-input-cards.js

Lines 25 to 29 in e464c76

	if ( ! newShippingRates.some( isNonFreeShippingRate ) ) {
	setValue( 'offer_free_shipping', undefined );
	}
	onChange( newShippingRates );

A similar case in the ShippingRateSection prior this PR:

https://github.com/woocommerce/google-listings-and-ads/pull/1750/files#diff-4594bb5f450b25cad952c53f192d5aaa3e33cb73798f578cfa85695c6f4165e2L26-L40

What do you think?

[tomalec]

Give, we have conceptually the components as follows:

SetupFreeListings:
   <Form>
      <ShippingRatesType>
      <FlatShippingRate>
      <FlatShippingTime>
  </Form>

The way I understood the desired data flow here looks like this:

conceptually

• user changes shipping rate type to a newValue
  • SetupFreeListings updates its (direct or indirect) state to have shipping_rate: newValue
  • SetupFreeListings updates its state to have shipping_time: 'manual' if newValue == 'manual', otherwise flat
  • SetupFreeListings (directly, or indirectly via FormContent) shows FlatShippingRate only if newValue == 'flat'
  • SetupFreeListings (directly, or indirectly via FormContent) shows FlatShippingTime only if newValue == 'flat' | 'automatic'
  • SetupFreeListings triggers its onSettingsChanged callback with an updated state

However, the way I read the implementation it's more like this:

implementation

• user changes shipping rate type to a newValue
  1. SetupFreeListings > FormContent updates its state to have shipping_rate: newValue
  2. SetupFreeListings > FormContent shows FlatShippingRate only if newValue == 'flat'
  3. if the change does not result in changing the shippint_time, SetupFreeListings trigger onSettingChanged
  4. if the change results in changing shipping_time, do not trigger callback, but SetupFreeListings updates the state of FormContent
  5. SetupFreeListings > FormContent shows FlatShippingTime only if newValue == 'flat' | 'automatic'
  6. Form notifies SetupFreeListings on the change it just made.
  7. SetupFreeListings triggers the callback.

Steps 3-6 seem to me like unnecessary ping pong. Can we make it simpler by putting state management into a single component?
Is that due to some limitations in the external components we use? If so maybe it's worth rising an issue upstream and linking it in the code, to hopefully, eventually improve that.
I'm afraid, especially considering the comment above that if we keep on patching it may eventually hit us with more bugs.

[tomalec]

Tested locally, reviewed the code, LGTM

[eason9487]

Thanks for the reviews, @tomalec @jorgemd24

---

@tomalec

Tested locally, the UI behaves as expected, however, onSettingsChange callback is called 15 times after a single change. That looks like a big overhead. Is this expected, or could we avoid it?

I was unable to reproduce this. Could you provide the replication steps?

And I will find another time to circle back this #1750 (comment) 🙏

---

@jorgemd24

However I have some concerns about the root of this problem, when I was debugging this PR I saw that we have a similar issue with the values.offer_free_shipping state

Nice finding! I will push a new commit to this PR for fixing that one together.

[eason9487]

Additional fix:

Issue #1753 should be fixed by 415bb91.

Screenshots:

Kapture.2022-11-09.at.20.17.19.mp4

Detailed test instructions:

Please refer to the replication steps in #1753

[eason9487]

One more additional fix:

This processing has the same issue as well. It should be fixed by 2daefb2.

Actual behavior:

The value of Minimum Order Quantity...: is not cleared after selecting the "No" option for free shipping.

Kapture.2022-11-09.at.20.55.31.mp4

Screenshots (Expected behavior):

Kapture.2022-11-09.at.20.54.47.mp4

Detailed test instructions:

1. Go to Edit Free listings
2. Add shipping rates > 0
3. Offer Free Shipping...: 'Yes'
4. Minimum Order Quantity...: any value bigger than 0
5. Offer Free Shipping...: 'No'
6. Offer Free Shipping...: 'Yes' again
7. The value of Minimum Order Quantity...: entered in step 4 should be cleared

[eason9487]

Hi @tomalec, as we discussed during the dev call. To make all relevant issues of WC Form be released sooner, I pushed related commits to this PR. Could you help with the additional round of code reviews?

[tomalec]

🚧 After the last commit
once I change anything on the settings page, it gets blank, and the console throws:

Too many re-renders. React limits the number of renders to prevent an infinite loop

Screencast.from.09.11.2022.21.21.38.webm

[tomalec]

I don't see why 2daefb2 is different than previous ones. Seems we reached the limit of workarounds for colliding versions of React components

[tomalec]

Tested locally, the UI behaves as expected, however, onSettingsChange callback is called 15 times after a single change. That looks like a big overhead. Is this expected, or could we avoid it?

I was unable to reproduce this. Could you provide the replication steps?

Here is how I logged it
https://github.com/woocommerce/google-listings-and-ads/compare/fix/1747-shipping-time-form-state...test/1750-multiple-onSettingsChange-calls?expand=1

Repro steps:

1. Go to /wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&subpath=%2Ffree-listings%2Fedit
2. Change shipping rate from automatic to manual

[eason9487]

@tomalec, thanks for the review!

once I change anything on the settings page, it gets blank [...]

I guess you were testing with WC 7.1? I have to say I was very lucky because I only tested it with WC 6.9 and 7.0. However, I was just aware of the Form in WC 7.1 may have new bugs or breaking changes that caused new incompatibilities.

I couldn't run the code to confirm it right now but the codes below look like lead to trigger onChange for each item in the values regardless whether a value change is did happened, even if we only call setValue for updating a single value.

• https://github.com/woocommerce/woocommerce/blob/7.1.0/packages/js/components/src/form/form.tsx#L209-L211
• https://github.com/woocommerce/woocommerce/blob/7.1.0/packages/js/components/src/form/form.tsx#L182-L197

Probably it needs more hacks since WC 7.1 has been released. 😂

[tomalec]

I guess you were testing with WC 7.1?

Yes, exactly.

I couldn't run the code to confirm it right now but the codes below look like lead to trigger onChange for each item in the values regardless whether a value change is did happened, even if we only call setValue for updating a single value.

Confirmed, that's the case. 🤦

Probably it needs more hacks since WC 7.1 has been released. joy

Our code already consists of mostly workarounds for WC & WP quirks, maybe it's time to finally clean the field.

[eason9487]

@tomalec, thanks for confirming the root cause

🚧 After the last commit once I change anything on the settings page, it gets blank, and the console throws:

Too many re-renders. React limits the number of renders to prevent an infinite loop

Since this issue belongs to the WC 7.1 compatibility issue, I was addressing it in a separate PR #1759. If #1759 is feasible and gets approved, I think this PR could be merged together in order by then.

[tomalec]

Reviewed and tested together with https://github.com/woocommerce/google-listings-and-ads/pull/1759/files# LGTM

[eason9487]

Hi @tomalec, thanks for your patience in waiting for me to get back to the question raised almost two months ago.

Quoted from myself:

And I will find another time to circle back this #1750 (comment) 🙏

---

Topic

Quoted from #1750 (comment):

However, the way I read the implementation it's more like this:

implementation

• user changes shipping rate type to a newValue

  1. SetupFreeListings > FormContent updates its state to have shipping_rate: newValue
  2. SetupFreeListings > FormContent shows FlatShippingRate only if newValue == 'flat'
  3. if the change does not result in changing the shippint_time, SetupFreeListings trigger onSettingChanged
  4. if the change results in changing shipping_time, do not trigger callback, but SetupFreeListings updates the state of FormContent
  5. SetupFreeListings > FormContent shows FlatShippingTime only if newValue == 'flat' | 'automatic'
  6. Form notifies SetupFreeListings on the change it just made.
  7. SetupFreeListings triggers the callback.

Steps 3-6 seem to me like unnecessary ping pong. Can we make it simpler by putting state management into a single component? Is that due to some limitations in the external components we use? If so maybe it's worth rising an issue upstream and linking it in the code, to hopefully, eventually improve that. I'm afraid, especially considering the comment above that if we keep on patching it may eventually hit us with more bugs.

Thanks for bringing up this question. For me, this question has always been considered a code smell, and it has been buried in my mind for many months.

Possible causes

I think the primary cause for this result is the strategy for managing data and states, and the secondary is the limitations imposed by the WC Form component.

• Management strategy of data and states: In GLA frontend codebase, data and state are usually not managed in a hierarchical way, which once made the logic of a field highly coupled, ranging from the backend API, to the frontend data retrieval, wp-data store, and then all the way through the form state to the form's input components.
• Form component: The functionality provided for operating values is very limited. Before WC 7.1, there is only a setValue function and only a flat field can be updated at a time.

Context

Before going further, I would like to mention some context of this case based on the current implementation.

• Currently, the 'automatic' application of shipping time settings is not supported in GLA, so there is a special logic that needs to be handled additionally.
• In the API communication layer, the shipping_time is data.
• In the form layer, shipping_time could be only a state rather than data.
• In the form section layer, shipping_time could be more like a prop for determining the special logic rather than data.
• From the technical design point of view, should the changes of multiple data fields in the custom on*Change event be triggered once combined or multiple times individually?

Possible alternatives

Next, there are several alternatives.

1. Since WC 7.1, the Form has added setValues to support updating multiple fields at once.
2. Let it trigger onSettingsChange two times directly, and debounce the callbacks of onSettingsChange in its parent components or the calls of saveSettings.
3. Adopt a strategy of hierarchical management of data and states across the frontend codebase.

Thoughts on the first two possible alternatives above

1️⃣ Use setValues

In a location similar to this revision, change the handler to an implementation like this:

const { getInputProps, values, setValues } = formProps;

const handleShippingRateOptionChange = ( value ) => {
	switch ( value ) {
		case 'automatic':
		case 'flat':
			setValues( {
				'shipping_rate', value,
				'shipping_time', 'flat',
			} );
			break;

		case 'manual':
			setValues( {
				'shipping_rate', value,
				'shipping_time', value,
			} );
			break;
	}
};

This solution looks intuitive and simple, but relies on the functionality of Form, and the code smells of high coupling still exists, even more than at present. I personally wouldn't recommend this approach given that the entire Onboarding and Free Listings settings were already very complicated.

2️⃣ Debounce the callbacks of onSettingsChange or the calls of saveSettings

It could be something like these removed hooks:

• useAutoSaveSettingsEffect
• useDebouncedCallbackEffect

In earlier versions, several places originally had debouncing handling. I intentionally removed them when developing Free Listings + Paid Ads, because those uses of debouncing were more like workarounds, in my opinion.

• It is not a mandatory solution and could be better handled by controlling the triggering of on*Change See issue #1335 and PR #1616.
• Some issues like the flashing shipping values were indirectly caused by it.
• It also complicates error handling and the reference management of React hook, and reduces flexibility. See #1313 (comment) and #1313 (comment)
• From a certain point of view, it's an unnecessary ping pong as well.

A more suitable use case would be, for example, debouncing the continuous typing on an <input> and then trigger the API request for the proposed results.

I'd say this approach is not ideal as well.

My proposal for the strategy of hierarchical management of data

I have tried to advocate this concept, and some possible problems I mentioned at the time have actually happened. Related discussions can be found here: #1266 (comment) and #1317 (comment). Therefore, I would like to try to advocate the same idea again.

The reasons

• With the handling of data, states, errors, validations and a large variety of events, forms are inherently complex in general, as applications continue to iteratively add more features, improve UI/UX, and become easier for users to use, things might get trickier and trickier for devs.
• Reduce coupling
  • If data obtained from API must be maintained the consistency of its data structure (shape) across the API communication like creation or updating, in wp-data layer, in form layer, and in form input components layer, then a relevant change to a field may involve all of them.
  • It also reduces the reusability of components since they are highly interdependent in the data processing.
• Separation of concerns
  • There are no clear boundaries for what a component is concerned with, e.g. a form input component updates field A while updating another field B that is not directly related to it but used in another component.
  • Clear boundaries can also bring clarity to the test objectives and scope.

Hierarchy

• In the wp-data store, it's in charge of the data bridges between backend APIs and application.
  • Resolvers and Actions focus on how to communicate with APIs and what bi-directional data adaptations are needed.
  • Reducer focuses on how to maintain the store tree.
  • Selectors/Actions is definitely the interfaces for retrieving/mutating the store.
• In the specific implementation of a component like SetupFreeListings based on the Form component
  • Separately manage data and states used in multiple sub-components.
  • The structure of initialValues depends more on the form UI instead of sticking with the data shape got from wp-data.
  • In complex use cases, propagate the data out via custom event callbacks for the component consumer side to handle the wp-data communications rather than communicating with wp-data directly.
  • Managing data correlation internally, such as changing target countries, may also change shipping settings, but the data correlation required by the backend API should be handled in the wp-data layer.
• In the specific implementation of a component for interacting with user inputs such as ShippingRateSection
  • Focuses on how to handle user inputs and callbacks the processed data to its form.
  • Should be agnostic to other irrelevant data as possible. For example, in TargetAudience component, changing shipping rates data should be considered an anti-pattern.
  • Of course, it should try to avoid changing external data via wp-data as well.

Demo

As the actual practice of my thought is easier to demonstrate with code, please refer to add851c for the changes.

In this way,

• Data processing for shipping_rate and shipping_time is restricted to actions.js.
• The confusing ping pong process in SetupFreeListings has been completely eliminated.
• onSettingsChange is still not triggered continuously.
• The form and related components no longer have the shipping_time field they don't need to know or even to mutate.
• Lower coupling, of course.

Obviously, there is one problem that remains unresolved. The special logic of 'automatic' for shipping time is still scattered in FormContent and checkErrors.

However, using the same concept, the logic could be further moved to a util or useSettings, and provided as a prop to FormContent and as an external parameter to checkErrors. That is, we could also give that: Special logic is expected to be handled in [some] layer/place.

Other solutions?

This comment depends mainly on my subjective thoughts and personal experience. I would be happy to learn more if there are other solutions for this case.