Free Listings + Paid Ads: Combine the audience and shipping steps #1616

eason9487 · 2022-08-04T10:59:36Z

Changes proposed in this Pull Request:

It's a part of 📌 Adjust the setting sections in the onboarding steps in #1610. Mainly focus on combining the audience and shipping steps for the onboarding flow and the editing free listings page. Most visual and wording changes are not included.

Move the SetupFreeListings component of editing free listings to the shared directory.
Change the shared ChooseAudience to ChooseAudienceSection and merge it into SetupFreeListings with corresponding validations for the editing free listings.
Replace SetupFreeListings with the shared one for the onboarding flow.
Remove unused files and an npm package use-debounce.

It also handles some issues:

There are no useGetRemainingCountryCodes hooks after this PR.
- Refactor useGetRemainingCountryCodes #274
Components of shipping times are changed to use the shared ones. Each group of shipping rates and shipping times components is no longer duplicated.
- Better implementation of Shipping Rates and Shipping Times component for MC Account Edit Settings and Setup MC Step 3 #288
The last shipping values flash issue only happens on the inputs of shipping time during onboarding setup. With the replacement of using shared shipping time components, the form state management was isolated from wp-store.
- Shipping values flash during the onboarding setup #1078
After replacing ChooseAudience and SetupFreeListings with the shared components, the auto-saving mechanism is implemented with on*Change callbacks instead. All auto-saving hooks and debouncing are no longer in need.
- Replace Form-related "auto" effects with Form onChange handler #1335

Screenshots:

📷 Onboarding flow

📷 Edit free listings

Detailed test instructions:

Onboarding flow

Disconnect all accounts or run these SQLs to delete related GLA settings

DELETE FROM wp_options WHERE option_name IN ('gla_mc_setup_completed_at', 'gla_target_audience', 'gla_merchant_center');
DELETE FROM wp_gla_shipping_rates;
DELETE FROM wp_gla_shipping_times;

Start the onboarding setup and check if the combined step for audience and shipping works correctly.
- Auto-saving mechanism for audience, MC settings, shipping rates, and shipping times
- All possible ways to add/edit shipping times
- Switch the location options of audience to see if all countries and selected countries options affect the corresponding shipping sections correctly.
Change the audience countries to see if the removed countries are removed from shipping sections as well.
- 💡 The implementation is not the same as before. Please refer to the originally expected behavior in Setup MC: changing target audience countries in Step 2 after setting up shipping rates and shipping times in Step 3 #232 (comment)
Edit settings to check if the disabled status of the "Continue" button is correct.
Continue or switch between steps to view whether it works as expected.
Refer to the replicate steps in Shipping values flash during the onboarding setup #1078 to check if the value flash problem in shipping time fields is fixed.
- 💡 The problem in shipping rate fields has been fixed in another PR before.

Edit free listings

Go to the Edit Free Listings page.
It should be changed to a one-pager without steps.
Simiar to the tests in the onboarding flow except for the auto-saving mechanism, operate the combined audience and shipping to see if they work well.
Edit some values to see if the unsaved data prompt pops up when navigating away.
Edit the above values back to see if there is no prompt when navigating away.

Additional details:

The auto-fill feature of flat shipping rates in the onboarding flow is removed from the frontend side. Ref: p1659494048432789-slack-C01E9LB4X61
Not sure why but mc/target_audience will respond with an 400 error if saving with empty countries. From my memory, it should be ok before because the UI allows merchants to unselect all. 🤔

Changelog entry

Add - Combine the audience and shipping steps for the onboarding flow and the editing free listings page.
Fix - Shipping time values flash during the onboarding setup.

…ed directory

…t into SetupFreeListings for the editing free listings page

…tings

…flow

…entation

puntope · 2022-08-11T10:25:01Z

...c/components/free-listings/configure-product-listings/__snapshots__/checkErrors.test.js.snap

@@ -1,5 +1,11 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`checkErrors Audience When the audience countries array is empty and the value of audience location option is 'selected', should not pass 1`] = `"Please select at least one country."`;


💅 Out of this PR and opinionated

Maybe to consider migration to unit testing?

Do you mean replacing expect( errors.countries ).toBe( 'Please select at least one country.' ); with expect( errors.countries ).toMatchSnapshot(); ? If so, some related discussions can be found in #893 (comment) and #927 (comment).

puntope · 2022-08-11T12:46:26Z

js/src/setup-mc/setup-stepper/saved-setup-stepper.js

+				shipping_rate: 'automatic',
+				shipping_time: 'flat',


❓ Not a suggestion, just curious. Why we are not adding the default values inside data store DEFAULT_STATE ?

I saw we use null values as checkers for other stuff... probably because of that

The intent here is to trigger API to save the form's default values. It would be easier and more suitable to handle the intent here.

In addition, moving the default values of form layer to wp-data store layer will also bring different abstraction of data/states isolation. Since isResolving and hasFinishedResolution are not enough to cover all statuses of a state in wp-store (see issue #438.), and considering the "statuses" of states in wp-data haven't yet had a consistent definition/convention, for now, the default value in a wp-data state is more like the status that tells the state is not ready to be used. And after receiving the data from API, the state will be changed to that data, which will also change the status of that state to "the status of the state is ready, and the data is the state itself".

IMO, before a consistent definition/convention of the statuses, states, and data in wp-data is created, it's probably hard to say how to set a default value would be a good practice.

puntope · 2022-08-11T13:00:49Z

js/src/setup-mc/setup-stepper/saved-setup-stepper.js

@@ -58,6 +100,15 @@ const SavedSetupStepper = ( { savedStep, onRefetchSavedStep = () => {} } ) => {
 		}
 	};

+	function handleFormChange( errorMessage, newValue ) {
+		this( newValue ).catch( () => createNotice( 'error', errorMessage ) );


💅 it was a bit confusing to understand what this was doing here. I saw after in the code that we bind the action to be called in SetupFreeListings.

However, would be nice to add a JSDOC in this function explaining whats going on.

Added in d8ecc08.

Now is perfect! thanks

js/src/setup-mc/setup-stepper/saved-setup-stepper.js

puntope

I checked the behaviour. Tested locally and I didn't find any error. All the combos worked fine for me.
I checked also the code. Since there is a lot of files deleted and moved around mixed with changes in those files. It was a bit hard to differentiate what was added/changed and what was just a copy paste for the deleted files. However everything looks good for me... left a few comments and minor questions

puntope · 2022-08-11T13:50:33Z

Not sure why but mc/target_audience will respond with an 400 error if saving with empty countries. From my memory, it should be ok before because the UI allows merchants to unselect all. 🤔

I was unable to save with empty value in the audience selector. 🤔 I guess do you mean it shows an error when the autosave runs when the selector is empty?

…mponent. Address #1616 (comment)

eason9487 · 2022-08-12T06:00:35Z

Thanks for the review, @puntope

I checked also the code. Since there is a lot of files deleted and moved around mixed with changes in those files. It was a bit hard to differentiate what was added/changed and what was just a copy paste for the deleted files.

Sorry for the inconvenience, I was trying to separate this as smaller PRs but the second one would only be including file deletions. So I ended up choosing to use deal with it altogether.

There are no deleted file changes that were caused by copy-and-paste. All deleted files were actually deleted (20e758f). And two deleted-and-added files are completely different:

js/src/components/free-listings/choose-audience-section/index.js
js/src/components/free-listings/choose-audience-section/choose-audience-section.scss

I guess do you mean it shows an error when the autosave runs when the selector is empty?

Yes. UI allows the operation to clear the value in the audience selector but then it causes an error. It may confuse merchants. I will check it later while working on this project and open a follow-up issue if needed.

puntope · 2022-08-12T08:36:12Z

Yes. UI allows the operation to clear the value in the audience selector but then it causes an error. It may confuse merchants. I will check it later while working on this project and open a follow-up issue if needed.

I agree

eason9487 added 7 commits August 4, 2022 17:34

Move SetupFreeListings component of editing free listings to the shar…

4881f34

…ed directory

Change the shared ChooseAudience to ChooseAudienceSection and merge i…

93249e6

…t into SetupFreeListings for the editing free listings page

Add validations for merged audience values in the shared SetupFreeLis…

79911f8

…tings

Replace SetupFreeListings with the shared one for the onboarding flow

55e2c09

Catch errors when saving data in the SavedSetupStepper of onboarding …

193ab56

…flow

Remove unused files

20e758f

Remove an unused npm package use-debounce

fdf10dc

eason9487 requested a review from a team August 4, 2022 10:59

eason9487 self-assigned this Aug 4, 2022

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Aug 4, 2022

eason9487 added 2 commits August 5, 2022 14:36

Add the CountryCode type back to SetupFreeListings

159bd2d

Add auto-saving for MC settings to fall back with the original implem…

43a2f76

…entation

puntope reviewed Aug 11, 2022

View reviewed changes

js/src/setup-mc/setup-stepper/saved-setup-stepper.js Show resolved Hide resolved

puntope reviewed Aug 11, 2022

View reviewed changes

Add JSDoc for the handleFormChange function in SavedSetupStepper co…

d8ecc08

…mponent. Address #1616 (comment)

eason9487 requested a review from puntope August 12, 2022 06:00

puntope approved these changes Aug 12, 2022

View reviewed changes

eason9487 merged commit 16244ee into feature/1610-streamlined-onboarding Aug 15, 2022

eason9487 deleted the update/1610-merge-audience-shipping-step-2-3 branch August 15, 2022 03:03

eason9487 mentioned this pull request Aug 15, 2022

Streamlined Free Listings plus Paid Ads for the onboarding flow #1610

Closed

35 tasks

eason9487 mentioned this pull request Sep 14, 2022

Free Listings + Paid Ads: Fix a compatibility issue with the Form component in WC 6.9 #1672

Merged

eason9487 mentioned this pull request Dec 29, 2022

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

Merged

This was referenced Jun 20, 2023

Move entire free campaign stepper into a Form component (setup & edit) #427

Closed

Remove an unused hook useCallbackOnceEffect #1995

Merged

eason9487 mentioned this pull request Jun 29, 2023

Add the built-in WP REST API validation to CountryCodeTrait to avoid errors when clearing all audience countries in the onboarding flow #2003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Free Listings + Paid Ads: Combine the audience and shipping steps #1616

Free Listings + Paid Ads: Combine the audience and shipping steps #1616

Uh oh!

eason9487 commented Aug 4, 2022 •

edited

Loading

Uh oh!

puntope Aug 11, 2022

Uh oh!

eason9487 Aug 12, 2022

Uh oh!

puntope Aug 11, 2022

Uh oh!

puntope Aug 11, 2022

Uh oh!

eason9487 Aug 12, 2022

Uh oh!

puntope Aug 11, 2022

Uh oh!

eason9487 Aug 12, 2022

Uh oh!

puntope Aug 12, 2022

Uh oh!

Uh oh!

puntope left a comment

Uh oh!

puntope commented Aug 11, 2022

Uh oh!

eason9487 commented Aug 12, 2022

Uh oh!

puntope commented Aug 12, 2022

Uh oh!

Uh oh!

		@@ -1,5 +1,11 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`checkErrors Audience When the audience countries array is empty and the value of audience location option is 'selected', should not pass 1`] = `"Please select at least one country."`;

Free Listings + Paid Ads: Combine the audience and shipping steps #1616

Free Listings + Paid Ads: Combine the audience and shipping steps #1616

Uh oh!

Conversation

eason9487 commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Screenshots:

📷 Onboarding flow

📷 Edit free listings

Detailed test instructions:

Onboarding flow

Edit free listings

Additional details:

Changelog entry

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

puntope left a comment

Choose a reason for hiding this comment

Uh oh!

puntope commented Aug 11, 2022

Uh oh!

eason9487 commented Aug 12, 2022

Uh oh!

puntope commented Aug 12, 2022

Uh oh!

Uh oh!

eason9487 commented Aug 4, 2022 •

edited

Loading