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

Hide Tax UI during onboarding & keep in edit campaign #2542

Conversation

dsawardekar
Copy link
Collaborator

@dsawardekar dsawardekar commented Aug 19, 2024

Changes proposed in this Pull Request:

Closes #2490

This PR removes the tax rates UI during the onboarding steps. The UI is preserved for the edit campaign flow.

Note: This PR depends on the work in https://github.com/woocommerce/google-listings-and-ads/pull/2543/files.

Screenshots:

Onboarding:
image

Edit Campaign:
image

Detailed test instructions:

Dependency:
Merge https://github.com/woocommerce/google-listings-and-ads/pull/2543/files into your local branch before testing this feature. Without this you will not be able to continue to Step 3.

  1. Start the onboarding steps from a clean slate.
  2. You should not see any Tax Rate section
  3. You should be able to complete Step 2 and proceed by clicking the Continue button
  4. After onboarding, edit the free Campaign
  5. You should see the Tax Rate UI that was removed during onboarding

Additional details:

Update - Hides tax rate UI during onboarding

Changelog entry

Update - Hides the tax rate setting during onboarding.

@dsawardekar dsawardekar self-assigned this Aug 19, 2024
@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 62.7%. Comparing base (40bbf82) to head (bd39f03).
Report is 133 commits behind head on feature/2458-streamline-onboarding.

Files with missing lines Patch % Lines
.../free-listings/setup-free-listings/form-content.js 0.0% 1 Missing and 1 partial ⚠️
...ponents/free-listings/setup-free-listings/index.js 0.0% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                           @@
##             feature/2458-streamline-onboarding   #2542      +/-   ##
=======================================================================
- Coverage                                  65.2%   62.7%    -2.5%     
=======================================================================
  Files                                       800     319     -481     
  Lines                                     22899    5063   -17836     
  Branches                                   1230    1233       +3     
=======================================================================
- Hits                                      14928    3173   -11755     
+ Misses                                     7803    1716    -6087     
- Partials                                    168     174       +6     
Flag Coverage Δ
js-unit-tests 62.7% <40.0%> (-1.1%) ⬇️
php-unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...listings/configure-product-listings/checkErrors.js 100.0% <100.0%> (ø)
.../src/setup-mc/setup-stepper/saved-setup-stepper.js 87.8% <ø> (ø)
...ponents/free-listings/setup-free-listings/index.js 6.7% <0.0%> (-0.1%) ⬇️
.../free-listings/setup-free-listings/form-content.js 7.7% <0.0%> (-0.6%) ⬇️

... and 489 files with indirect coverage changes

@dsawardekar dsawardekar marked this pull request as ready for review August 20, 2024 05:56
Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

I've left one suggestion inline. Otherwise, the main thing that needs to be addressed is that I see an E2E test that's failing related to the validation logic that happens in the SetupFreeListings component:

const handleValidate = ( values ) => {
const countries = resolveFinalCountries( values );
const { shipping_country_times: shippingTimesData } = values;
return checkErrors(
values,
shippingTimesData,
countries,
storeCountryCode
);
};

The checkErrors() function includes this logic to validate the tax_rate value for 'US' based stores:

/**
* Check tax rate (required for U.S. only).
*/
if (
( storeCountryCode === 'US' || finalCountryCodes.includes( 'US' ) ) &&
! validTaxRateSet.has( values.tax_rate )
) {
errors.tax_rate = __(
'Please specify tax rate option.',
'google-listings-and-ads'
);
}

We're setting this default for tax rates via the API in #2491, so we need to update this so that the validation for tax_rates only runs if hideTaxRates is false.

To verify this locally, set your store location to 'United States' and try completing this form. You will be able to click the "Continue" button, but nothing will happen and no errors will be shown.

js/src/edit-free-campaign/index.js Outdated Show resolved Hide resolved
@joemcgill joemcgill changed the base branch from develop to feature/2458-streamline-onboarding August 22, 2024 14:02
@dsawardekar
Copy link
Collaborator Author

@joemcgill I've addressed the PR feedback, can you take another look? thx

@ankitrox ankitrox self-requested a review August 26, 2024 07:06
Copy link
Collaborator

@ankitrox ankitrox left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Copy link
Collaborator

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

I traced down the E2E test failure that I was seeing since I couldn't reproduce it anymore now that 57ecc5d got merged into this branch. Turns out the default settings get mocked in these tests and needed to be updated in here.

I went ahead and cleaned up a few more test methods that are no longer used after these changes—something to keep an eye going forward when deleting tests.

@ankitguptaindia
Copy link
Member

QA/Test Report-

Testing Environment -

  • WordPress: 6.6.1
  • Theme active on store: Storefront Version: 4.6.0
  • WooCommerce - Version 9.2.3
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 127
  • OS: macOS Sonoma 14.6

Test Results -

  • Positive Test:

    • The tax selection UI does not appear during onboarding but does appear on the edit campaign screen. ✅
    • The tax selection UI appears when the audience location is set to the US. ✅
  • Negative Test: The tax selection UI should not appear when the audience location is set to a country outside the US. ✅

  • Default Selection of Tax UI: "My store uses destination-based tax rates." in the edit campaign screen. ✅

Functional Demo / Screencast -

Recording.809.mp4

Plugin file with this PR related code to test google-listings-and-ads.zip

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Thanks for the change! It works well.

Could you help with removing an unused method?

/**
* Get non-destination-based tax rate radio row.
*
* @return {import('@playwright/test').Locator} Get non-destination-based tax rate radio row.
*/
getNonDestinationBasedTaxRateRadioRow() {
return this.page.getByRole( 'radio', {
name: 'My store does not use destination-based tax rates.',
exact: true,
} );
}

…ithub.com:woocommerce/google-listings-and-ads into update/2490-hide-tax-rate-selection-ui-onboarding
@joemcgill
Copy link
Collaborator

@eason9487 I think this is ready for a second review.

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

Although there are JSDoc and code conflict need to be adjusted, I believe it should not need another round of code reviews after resolving them.

@dsawardekar
Copy link
Collaborator Author

@joemcgill I have updated the jsdoc and fixed the merge conflict. Assigning to you to take another look.

@asvinb
Copy link
Collaborator

asvinb commented Sep 11, 2024

Change LGTM @dsawardekar . As per @eason9487 's comment, I'am reassigning the ticket to @fblascogarma for UAT.

@joemcgill
Copy link
Collaborator

Since this has been approved, I'm merging into the feature branch.

@joemcgill joemcgill merged commit 8eb1fff into feature/2458-streamline-onboarding Sep 16, 2024
8 checks passed
@joemcgill joemcgill deleted the update/2490-hide-tax-rate-selection-ui-onboarding branch September 16, 2024 16:26
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.

Onboarding: Remove Tax Rates Selection UI
6 participants