Fix navigate away prompt showing up when we change offer_free_shipping back and forth

[ecgan]

Changes proposed in this Pull Request:

Closes #1318.

In Edit Free Listings, when we change the offer free shipping value back and forth, the navigate away prompt would show up. This is because the offer_free_shipping value is being grouped as settings, and settings is considered changed.

In this PR, we use a new function getSettings here to extract the settings data from the form values object, leaving offer_free_shipping out, so changing the offer free shipping value would not be considered as settings changed.

Screenshots:

📹 Demo video with my voice:

demo-fix-navigate-away-offer-free-shipping.mov

Detailed test instructions:

1. Go to Edit Free Listings.
2. Change the offer free shipping value, and then change it back to the original value.
   • Note: if you change from "yes" to "no", your minimum order amount would have been cleared, and you would have to key in the same minimum order amount again when you change back to "yes".
3. Navigate away by reloading the page. Since all the values are the same and there are no changes, there should be no prompt for you, and you should be able to navigate away.

Changelog entry

[eason9487]

❓
Sorry, I can't see the purpose of this handling. There is already having the processing of un-glue form data. Looks like it could be addressed by omitting offer_free_shipping with that process.

@@ -70,6 +70,7 @@ const SetupFreeListings = ( {
                const {
                        shipping_country_rates: newShippingRates,
                        shipping_country_times: newShippingTimes,
+                       offer_free_shipping, // eslint-disable-line camelcase, no-unused-vars
                        ...newSettings
                } = newVals;

[ecgan]

Yea, I thought about the approach you mentioned. The problem with it is that if someone were to add another field to the Form, it would cause similar issue like #1318. So I use this approach - picking out specific Settings fields - to ensure issue like #1318 won't happen again.

[eason9487]

The problem with it is that if someone were to add another field to the Form, it would cause similar issue like #1318. So I use this approach - picking out specific Settings fields - to ensure issue like #1318 won't happen again.

Unfortunately, this approach might not achieve this purpose. Regardless of the approach, a conceptually similar problem exists and there is no guarantee that it won't happen again.

• If a new field is added that will be saved in the API, it will still be missed in getSettings.
• If a new field is added that will not be saved in the API, it will also be missed when un-glue form data.

Since neither approach can prevent it, considering that the un-glue form data is an existing process and fewer fields need to be filtered out than picked out, would it be better to use the un-glue form data to process it incidentally?

[ecgan]

Note: @eason9487 and I had a related conversation in our internal slack channel, see: p1647313955981019-slack-C01E9LB4X61

---

• If a new field is added that will be saved in the API, it will still be missed in getSettings.

Yes, there will be chances of issues happening. However, because it is more explicit with getSettings, it is easier to notice and fix issues related to it, e.g. "I added a new settings field, something doesn't work, I look at getSettings, it is relatively obvious that I forgot to add it in there."

• If a new field is added that will not be saved in the API, it will also be missed when un-glue form data.

Yes. Issue #1318 is an example of that, and using getSettings would help with that.

considering that the un-glue form data is an existing process and fewer fields need to be filtered out than picked out, would it be better to use the un-glue form data to process it incidentally?

I still think that specifically picking out the wanted fields is better than taking out the unwanted fields and leaving the wanted fields in the rest object, i.e.:

// Approach 1: get the wanted fields via getSettings.
const settings = getSettings( values );

// Approach 2: leave the wanted fields in rest settings object.
const { unwanted1, ...settings } = values;

With Approach 1, if someone were to add a new non-settings form field, settings will still be the same object shape; with Approach 2, settings will contain the new field, which may be accidental and may not be desired, and this is the thing I would like to avoid.

---

I'm feeling like this is the time where we got to "agree to disagree". 😅

[eason9487]

Thanks for the explanation.

---

I'd like to clarify my point was in advocating maintainability and readability when discussed in this PR comment and that internal slack thread.

In terms of maintainability

Both methods have conceptually similar issues. Hope this table could make the point clearer:

Method ╲ New field to be added	Pick out needed (getSettings)	Pick out unneeded (un-glue data in handleChange)
will be saved via API	Must be added as a field to be removed, or the unsaved checking will be incorrect and the settings to be called to API will miss the new field.	No adjustment required
won't be saved via API	No adjustment required	Must be added as a field to be removed, or the unsaved checking will be incorrect and the settings to be called to API will have an invalid field.

No matter which approaches, the new field is always automatically included on one side and adjustment-needed/missed on the other side. In this consideration, there is no difference in higher or lower chance when no one can predict which new field will be added in the future. So I thought there is no difference in the pros and cons between these two methods.

In terms of readability

Although I understood what the problem is that getSettings is trying to solve at this point at the first time review, I didn't see the purpose of adding additional getSettings that is equivalent (at least in my opinion) to the opposite of its problem when there was already the un-glue processing in place. So I started with a ❓ to clarify if I was missing something.

After some discussion, I understood your reason is that getSettings is clearer and more explicit, and has a higher chance of being noticed. With the original getSettings code, I agree that subjectively it had a higher chance of being noticed, but for me it's because the function looks strange and confusing, so it's easy to notice, not because it's an explicit function. In addition, unless the explicit factor is simply and only the visual size of the code block, the glue and un-glue processings are not less explicit in comparison. At least they appear in pairs with their code comments.

Also, in the slack thread, I described additional confusion in reading the original getSettings code, as well as spending some time thinking about questions that seemed relevant but were out of the scope of this PR.

My summary

In summary, readability was my main concern in the original getSettings. And I don't think it's convincing to infer which method has a higher or lower chance based on the developer's individual experience of discovering/encountering this problem in the past. Because if I were to speak from personal experience about the chance of discovering this problem, as a code reviewer role, I had noticed it in almost all of several recent PRs that have relevant changes. But I would not infer which approach has a ~80% chance of being noticed and therefore conclude that which has a higher chance.

I believe we could all agree that describing the purpose for getSettings and how to add a new field can more clearly simplify these contexts and their implied problems (table above).

---

Thanks for the code comments and adjustments!

In this part, it's still based on the readability and maintainability perspectives.

Readability

After further adjustments, the purpose and context are clearer. Thanks! I will comment additionally could be more clearly described in a few sentences.

Maintainability

❔

A new question arises.

The needed settings in initialValues below is the same as the result of getSettings( settings ). Can getSettings also be used to change it to the following?

<Form
    initialValues={ {
        ...getSettings( settings ),
        // ... omitted below
    } }
>

Or is it risky to do so? If it's not appropriate to do so, whether it would be easy to misunderstand that it could be done in future maintenance?

Further maintainability

📜 or ignore this section.

In my opinion, the root cause of these concerns and problems is mixing the UI state with <Form> values. This is exactly why I advocated that <Form> initialValues should be as simple as possible in #1266 (comment).

Borrowing a <Form> value as a state might not be a good idea:

• The state will also appear in the arguments of onChange and onSubmit callbacks. The mix of form values and states in <Form> callbacks could be confusing when reading the code, further reducing readability and increasing complexity when maintaining it.
• The state will trigger <Form>'s onChange callback when calling setValue, which makes callback to be triggered when there is no "real" form values changed. So, additional processing was added:
  

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

  Lines 115 to 117 in 67c3333

  	} else if ( settingsFieldNames.includes( change.name ) ) {
  	onSettingsChange( change, getSettings( values ) );
  	}

Move offer_free_shipping out of initialValues could avoid these side effects and separate the responsibility of data/state handling across a complex form UI.

[ecgan]

Thanks for the detailed explanation, @eason9487 ! 🙂 👍

Here's my opinions on your questions above:

The needed settings in initialValues below is the same as the result of getSettings( settings ). Can getSettings also be used to change it to the following?

Technically yes, it can be used in that way, and devs could have also done it with ...settings like below:

<Form
    initialValues={ {
        ...getSettings( settings ),
        // ... some other fields.
		...settings
    } }
>

However, personally, I don't favor that, for few reasons:

• I prefer to see the long complete written list of fields inside the initialValues props. Whenever I need to recall the field name, I just need to find the form and look at its initialValues. It is a readability point for me.
• I also think it is risky and prone to error, see example below:

<Form
    initialValues={ {
        ...getSettings( settings ),
        // ... some other fields.
		tax_rate: 'abc123' // this tax_rate overwrites tax_rate from getSettings, which may not be desired.
    } }
>

The above problem wouldn't happen when we have the complete list of fields in initialValues props, like how we are having it now in our code base.

If it's not appropriate to do so, whether it would be easy to misunderstand that it could be done in future maintenance?

Unfortunately I don't have an idea to prevent that, other than the code comments. 😅 Also, this problem isn't really specific to getSettings, as devs could have also done it with ...settings in initialValues.

In my opinion, the root cause of these concerns and problems is mixing the UI state with <Form> values.
...
Move offer_free_shipping out of initialValues could avoid these side effects and separate the responsibility of data/state handling across a complex form UI.

Thanks for your opinion! Again, this is where we have different opinions. 😆 🙏

I do think that it makes sense to put offer_free_shipping into the form, for the following reasons:

• It has a related UI with the "yes" / "no" radio options.
• It participates in form validation, and when validation fails (e.g. users did not select "yes" / "no" when it is required), we set errors.offer_free_shipping for it. See PR Shipping rates UI changes: Validation for offer free shipping and minimum order amount #1309.
• Related to the above errors.offer_free_shipping, if / when we want to show validation error messages to the users, we can get the error messages for all fields from the errors object in a coherent manner.
  • If we were to do this with useState, then it would look a bit weird, i.e. for all other fields, we get the error messages from errors object, but for offer_free_shipping specifically, we have to get it using another way.
• When users change the radio options from "yes" to "no", we need to clear the shipping rates' free shipping threshold. There are interdependencies between them, and for me, and it is natural / common / make sense to have interdependencies between form fields, and not so much for form fields and useState.

On the other hand, if we choose to use useState or other mechanism to store offer_free_shipping value instead of using form, I think it would actually be more complicated.

Borrowing a <Form> value as a state might not be a good idea

Thanks for the points. With my above reasons mentioned, I think the points you raised are fine / expected / acceptable "trade-offs". I think it is expected and not an issue, for me. 💅 😄

---

I have one question to ask, to get to know your opinion better and to be sure:

In my opinion, the root cause of these concerns and problems is mixing the UI state with <Form> values.

In your opinion, does it mean that all the Form values should always have a relevant API? So, fields that are purely UI state and not backed by API should not be in Form values? 🤔

I'm asking in that way because we are talking about offer_free_shipping field, which is purely UI side and not backed by API.

Curious to know your thoughts here.

[eason9487]

Thanks for sharing your thought!

---

I do think that it makes sense to put offer_free_shipping into the form, for the following reasons: [...]

I did understand the convenience of having the UI state associated with the form values. We can easily use form's operation APIs that are accessible in several places at development time.

However, I would like to emphasize that it also brings side effects that make state and event management less robust and requires checking for any new control handling overflows or pollutions during ongoing maintenance, and then adding more patches. Even though sometimes we can make those patches seem to be wroten well, sometimes they are workarounds.

Because forms are inherently complex, as applications continue to iteratively improve and become easier for merchants to use, things might get trickier and trickier. Especially this happens with/in forms.

On the other hand, if we choose to use useState or other mechanisms to store offer_free_shipping value instead of using form, I think it would actually be more complicated.

One of the primary influences on the overall robustness of architecture is how data and states are managed. The use of useState or other state management mechanisms has been something that the React ecosystem, and even other frameworks, are constantly working on. It is true that these strategies usually bring complexity, but these complexities bring relatively robust management practices via reducing side effects.

In your opinion, does it mean that all the Form values should always have a relevant API?

No, I don't mean that all form values need to be saved with API.

So, fields that are purely UI state and not backed by API should not be in Form values?

It depends on how a <Form> component is designed, with Woo's one, It has some nice designs in other parts, but I wouldn't recommend doing so if it's possible.

[eason9487]

Tested locally and works well. Left two 💅 comments and a new ❔ concern in the Maintainability section of #1317 (comment). And none of them is a blocker for this PR to address the primary issue. Approved in advance.