Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show validation errors on step 2 of the onboarding flow when unable to continue #2019

Merged

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Jul 20, 2023

Changes proposed in this Pull Request:

This is a part of implementation for #634 and for 📌 Display form validation errors in #1993.

To show validation errors on step 2 of the onboarding flow when unable to continue, this PR:

Extract the shareable component and adjust the layouts of shipping time components

  • Add a new component ValidationErrors for showing form validation error messages, and replace the errors rendering in AssetGroupCard with it.
  • Move the Card layouts from ShippingTimeSection into ShippingTimeSetup and remove unneeded codes.

Fix and adjust validator js/src/components/free-listings/configure-product-listings/checkErrors.js

  • Fix incorrect keys and rephrase a few messages.
  • Update the validation of tax rate data to check if the store country code is US.

Add the mechanism of showing validation errors to the product listings form

  • Request showing validation errors when clicking the submit of product listings form if any data is invalid.
  • Add a function to render validation errors to the product listings form.
  • Add the rendering of validation errors to:
    • ChooseAudienceSection
    • FlatShippingRatesInputCards
    • EstimatedShippingRatesCard
    • OfferFreeShippingCard
    • MinimumOrderCard
    • ShippingTimeSetup
    • TaxRate

Screenshots:

Kapture.2023-07-20.at.18.19.40.mp4

Detailed test instructions:

Some free listings data are unable to clean via UI. It might need to run the following SQL to clean them from database.

DELETE FROM wp_options WHERE option_name = 'gla_target_audience';
DELETE FROM wp_options WHERE option_name = 'gla_merchant_center';
  1. Go to step 2 of the onboarding flow.
  2. Before clicking the "Continue" button, the UI should not show any validation errors even if there are invalid data.
  3. Check if the "Continue" button is always enabled no matter whether the data are valid or not.
  4. With some invalid data, click the "Continue" button to see if the validation errors are shown in the corresponding places.
  5. Change data to view if the validation errors are shown or hidden accordingly.
  6. Make all data valid and click the "Continue" button to check if it goes to step 3.

Additional details:

💡 The implementation for step 3 will be done by subsequent PR(s)

Changelog entry

@eason9487 eason9487 requested a review from a team July 20, 2023 10:45
@eason9487 eason9487 self-assigned this Jul 20, 2023
@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Jul 20, 2023
@puntope
Copy link
Contributor

puntope commented Jul 20, 2023

DELETE FROM wp_options WHERE option_name = 'gla_target_audience';
DELETE FROM wp_options WHERE option_name = 'gla_merchant_center';

I was able to perform the test without this. Might you explain why is this needed? Does the user (or us programmatically) handle this after the user upgrades with this feature?

@puntope
Copy link
Contributor

puntope commented Jul 20, 2023

Is there any place where I can check the new validation error texts? Or there are no specific requirements?

@puntope
Copy link
Contributor

puntope commented Jul 20, 2023

When I was testing the errors I discover some inconsistency with the Audience Selector and the rest of inputs.

Screen.Recording.2023-07-20.at.17.56.57.mov

As you can see in the video, the inputs are not showing validation errors until the validation error in the Audience selector is solved.

Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

I check the requirements for this feature:

✅ Don’t disable the “Continue” or “Compete” buttons even if the form validation doesn’t pass.

✅ The form validation errors are not displayed until the user clicks the above button.

⚠️ I was unable to find a reference for the texts in the errors.shipping_country_rates and errors. shipping_country_times

⚠️ Some inconsistencies in the error renderization (see comments)

💅 A minor suggestion for the function ValidationErrors()

@eason9487
Copy link
Member Author

eason9487 commented Jul 24, 2023

Hi @puntope, thanks for the review.

This PR is ready for the second round of code reviews. Could you help with it?

DELETE FROM wp_options WHERE option_name = 'gla_target_audience';
DELETE FROM wp_options WHERE option_name = 'gla_merchant_center';

I was able to perform the test without this. Might you explain why is this needed? Does the user (or us programmatically) handle this after the user upgrades with this feature?

The selected Location option value ("Selected countries only" or "All countries") is stored in the gla_target_audience. The selected Shipping rates and Tax rate (required for U.S. only) option values are stored in the gla_merchant_center. These values are unable to be cleared via the UI. Deleting them from DB is a way to test step 2 as in the case of the first time entering the onboarding flow.

image

P.S. Location and Shipping rates will be filled in automatically when empty, so this PR didn't add validation errors for them but it's still a bit better to double-confirm that the automatic file in processing works.

Is there any place where I can check the new validation error texts? Or there are no specific requirements?

No. Except for PMax Assets and phone verification, no previous development or design documents have specified the texts for validation errors.

The main reason for adjusting some texts was to avoid the following situation, which happens to be a line break at the last word.

image

When I was testing the errors I discover some inconsistency with the Audience Selector and the rest of inputs. [...] As you can see in the video, the inputs are not showing validation errors until the validation error in the Audience selector is solved.

This is expected, as each field focuses on different validation errors. When no audience country is selected, the relevant validation errors are already displayed in the most relevant position. It would be a bit redundant to display the same error on both Shipping rates and Shipping times.

However, I noticed an issue from your video: after clearing a value equal to 0 in Shipping rates or Shipping times and moving out the focus, the AppInputNumberControl should be backfilled with 0 again but it didn't.

I tried a couple of different approaches and finally realized that there was only one way to solve the issue first with a relatively small amount of changes, while the rest of the approaches involved too many changes. Please view the fix in a8f9bf9.

Kapture.2023-07-24.at.11.45.50.mp4

@eason9487 eason9487 requested a review from puntope July 24, 2023 03:47
Copy link
Contributor

@puntope puntope left a comment

Choose a reason for hiding this comment

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

✅ LGTM

Thanks for the adjustments, explanation and extra Unit tests.

I left one last 💅 comment in regards to that "syncNeedle" name.

@eason9487 eason9487 merged commit d9bc521 into develop Jul 28, 2023
@eason9487 eason9487 deleted the update/634-onboarding-expose-validation-errors-step-2 branch July 28, 2023 08:32
@martynmjones martynmjones mentioned this pull request Aug 1, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants