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

Set default tax_rate to destination #2543

Merged
merged 9 commits into from
Aug 22, 2024

Conversation

kt-12
Copy link
Collaborator

@kt-12 kt-12 commented Aug 19, 2024

Changes proposed in this Pull Request:

The Tax Rate (the UI will be removed later; see #2490), shown on the second screen when a US country is selected, must default to destination.

Closes #2491.

Set the default value in the scheme property to destination. After the change, /wp-json/wc/gla/mc/settings does show default tax_rate as destination, in comparison to early `null. This was also reflected in Tax Rate UI.
See the before and after change screenshot of the setting endpoint.

Screenshots:

Before: you can see tax_rate is null
before

After: you can see tax_rate is destination
after

Detailed test instructions:

  1. Move to onboarding second screen.
  2. While on the second page, clear the option npm run wp-env run cli wp option delete gla_merchant_center to ensure it was not previously saved on a previous test.
  3. Reload the 2nd screen, and under location select North American. You should see UI for Tax rate (required for U.S. only) below.
  4. Check if the default option is My store uses destination-based tax rates..
  5. Check db. npm run wp-env run cli wp option get gla_merchant_center. Note to QA:- You need to check this again once the UI removal is complete (Onboarding: Remove Tax Rates Selection UI #2490)

Additional details:

Changelog entry

Tweak - Set the default value of the tax rate to destination-based during onboarding.

@github-actions github-actions bot added the changelog: tweak Small change, that isn't actually very important. label Aug 19, 2024
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.6%. Comparing base (5af0314) to head (d6f5658).
Report is 42 commits behind head on feature/2458-streamline-onboarding.

Additional details and impacted files

Impacted file tree graph

@@                           Coverage Diff                           @@
##             feature/2458-streamline-onboarding   #2543      +/-   ##
=======================================================================
+ Coverage                                  63.8%   65.6%    +1.8%     
- Complexity                                    0    4580    +4580     
=======================================================================
  Files                                       326     475     +149     
  Lines                                      5086   17891   +12805     
  Branches                                   1229       0    -1229     
=======================================================================
+ Hits                                       3247   11737    +8490     
- Misses                                     1672    6154    +4482     
+ Partials                                    167       0     -167     
Flag Coverage Δ
js-unit-tests ?
php-unit-tests 65.6% <100.0%> (?)

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

Files Coverage Δ
.../Controllers/MerchantCenter/SettingsController.php 98.5% <100.0%> (ø)

... and 800 files with indirect coverage changes

@kt-12 kt-12 changed the title Draft: Set default tax_rate to destination Set default tax_rate to destination Aug 19, 2024
@kt-12 kt-12 requested a review from joemcgill August 19, 2024 13:20
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.

This looks like it will work. Thanks for adding a new test class for the MerchantCenter\SettingsController.

@ankitguptaindia
Copy link
Member

QA/Test Report-

Testing Environment -

  • WordPress: 6.6.1
  • Theme: Storefront Version: 4.6.0 / Twenty Twenty-Four Version: 1.2
  • WooCommerce - Version 9.1.4
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 127
  • OS: macOS Sonoma 14.6

Steps to Test- Please follow the steps mentioned in the PR description. If you are using Local software for your testing environment instead of 'wp-env' / Docker-based environment. Use this command to delete the option from DB - wp option delete gla_merchant_center

Test Results -

All related tests are working as expected with this PR.

  • When intentionally saving the other option, i.e., 'My store does not use destination-based tax rates', and deleting the option setting from the database, after a page refresh, it shifts back to the default option as set by this PR. ✅

  • When the merchant uses the second option, i.e., 'My store does not use destination-based tax rates', and moves to the next screen and then back to the previous screen, it keeps the desired option selected. ✅

  • On the fresh onboarding screen, it keeps 'My store uses destination-based tax rates.' selected by default. ✅

  • When the Audience and Store Address are not US-based, this option should not be displayed. ✅

  • When the Store has a non-US-based address and the merchant selects/adds the US in the audience option from the 'Configure product listings' screen, it shows the 'My store uses destination-based tax rates.' option selected by default. ✅

Functional Demo / Screencast -

Recording.798.mp4

Plugin file with this PR-specific build google-listings-and-ads.zip

@eason9487 eason9487 requested review from mikkamp and removed request for eason9487 August 22, 2024 06:48
Copy link
Contributor

@mikkamp mikkamp 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 changes.

I tested fetching the settings and received the expected default:

{
    "shipping_rate": null,
    "shipping_time": null,
    "tax_rate": "destination",
    "website_live": false,
    "checkout_process_secure": false,
    "payment_methods_visible": false,
    "refund_tos_visible": false,
    "contact_info_visible": false
}

I also tested setting some basic values without taxes and still got the expected default set:

{
    "status": "success",
    "message": "Merchant Center Settings successfully updated.",
    "data": {
        "shipping_rate": "automatic",
        "shipping_time": "flat",
        "tax_rate": "destination",
        "website_live": true,
        "checkout_process_secure": true,
        "payment_methods_visible": true,
        "refund_tos_visible": true,
        "contact_info_visible": true
    }
}

The code all looks good to me. Thanks for adding the extra unit test, it runs well.

There is no harm in merging this PR to develop, but that means it will get included in a release next Tuesday. Since it's going to be combined with #2542 I'd suggest to target a feature branch where both these PR's can get merged to.

@joemcgill joemcgill changed the base branch from develop to feature/2458-streamline-onboarding August 22, 2024 13:57
@joemcgill joemcgill merged commit c4bbdf0 into feature/2458-streamline-onboarding Aug 22, 2024
12 checks passed
@joemcgill joemcgill deleted the tweak/default-tax-2491 branch August 22, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: tweak Small change, that isn't actually very important.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Onboarding: Default stores to destination-based tax rates during onboarding
4 participants