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

Move tax rate setup to the Settings page #2749

Open
wants to merge 8 commits into
base: feature/shipping-improvements
Choose a base branch
from

Conversation

eason9487
Copy link
Member

Changes proposed in this Pull Request:

Project thread: pcTzPl-2Cf-p2

This PR implements the 📌 Move tax setting to main setting page task of the project.

  • The tax rate setup has been hidden by Streamline onboarding: Conditionally hide the tax setting #2542 in the onboarding flow, and it should remain hidden after moving the setup. Therefore, the tax rate setup is completely removed from the onboarding flow.
  • The tax rate setup is moved from the Edit free listings page to the Settings page.

💡 Update for src/Tracking/README.md will be done by the conjunction PR for the feature branch.

Screenshots:

image

Detailed test instructions:

  1. Set up the country of the store address to the US via WooCommerce Settings or set up the audience countries of this plugin to include the US.
  2. Go to the Edit free listings page
    • Confirm the tax rate setup is not shown
    • Check if the editing function works
  3. Go to the Settings page.
    • Check if the tax rate setup is shown
    • Open the Network tab of DevTool
    • Check if the tax rate setup can be saved
    • Inspect the Network tab to see if the requests are issued in the following order and if they are not in parallel:
      1. wc/gla/mc/settings
      2. wc/gla/mc/settings/sync
    • Change the country of the store address and audience countries to exclude the US
    • Reload the Settings webpage and check if the tax rate setup is not shown
  4. View the E2E testing result: https://github.com/woocommerce/google-listings-and-ads/actions/runs/12593213104

Changelog entry

Update - Move tax rate setup from the Edit free listings page to the Settings page.

@eason9487 eason9487 requested a review from a team January 3, 2025 05:45
@eason9487 eason9487 self-assigned this Jan 3, 2025
@github-actions github-actions bot added the changelog: update Big changes to something that wasn't broken. label Jan 3, 2025
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.8%. Comparing base (6462683) to head (1eda2d7).
Report is 1 commits behind head on feature/shipping-improvements.

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

Impacted file tree graph

@@                       Coverage Diff                       @@
##           feature/shipping-improvements   #2749     +/-   ##
===============================================================
+ Coverage                           63.5%   63.8%   +0.3%     
===============================================================
  Files                                337     334      -3     
  Lines                               5196    5159     -37     
  Branches                            1274    1262     -12     
===============================================================
- Hits                                3300    3290     -10     
+ Misses                              1723    1699     -24     
+ Partials                             173     170      -3     
Flag Coverage Δ
js-unit-tests 63.8% <50.0%> (+0.3%) ⬆️

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%> (ø)
...es/onboarding/setup-stepper/saved-setup-stepper.js 87.2% <ø> (ø)
.../free-listings/setup-free-listings/form-content.js 6.7% <0.0%> (+0.8%) ⬆️
...ponents/free-listings/setup-free-listings/index.js 6.3% <50.0%> (+0.2%) ⬆️

... and 2 files with indirect coverage changes

@puntope
Copy link
Contributor

puntope commented Jan 3, 2025

WDYT about adding a confirmation message as feedback when the Setting save is completed.

Screen.Recording.2025-01-03.at.13.37.01.mov

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.

Thanks @eason9487 for working on this. I didn't found any blocker so far. Just a recommendation about adding a message after saving taxes setting. Not sure if this was already discussed with Design. In any case not a strong blocker for me and can be done also in further PRs

  • Tested the Taxes Section is not shown in Edit Free listings ✅
  • Tested the Edit Free listings still work and can be saved ✅
  • Tested the Taxes Section is not shown in Settings when neither US is the store base address nor a audience target country ✅
  • Tested the Taxes Section is shown in Settings when either US is the store base address or a audience target country ✅
  • Tested you can save the taxes and it calls the API endpoints ✅
  • UI general checks
  • Code & Tests checks
  • Additional Onboarding check

).not.toBeVisible();
} );

test( 'Update the setting', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test( 'Update the setting', async () => {
test( 'Should show the setup when selling to the US and can update the setting', async () => {

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