Skip to content

Fix "1 of 2 steps" is shown for the first step in a 3 step tour #34413

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.

Sign up for GitHub

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

Jump to bottom
Merged
chihsuan merged 4 commits into from
Aug 29, 2022
Merged

Fix "1 of 2 steps" is shown for the first step in a 3 step tour #34413

chihsuan merged 4 commits into from
Aug 29, 2022

Conversation

@chihsuan
Copy link
Member

@chihsuan chihsuan commented Aug 22, 2022 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Closes #34194.

This PR fixes the wrong step info of the shipping tour.

Before:

182948809-ee1db5ee-21e4-4f19-9c33-31a58aaa853b

After

Screen Shot 2022-08-22 at 11 06 31

How to test the changes in this Pull Request:

Case 1: The store sells physical products and is located in the US, but JP and WCS are not installed.

  1. Start OBW and choose United States as store country
  2. Choose "Physical products"
  3. Complete the OBW without installing anything from the Business Details tab.
  4. Navigate to WooCommerce -> Settings -> Shipping
  5. "United States (US)" zone should be created with Free shipping method
  6. Observe that the 3 step tour is displayed, with step one showing "Step 1 of 3"
  7. There should be 3 steps in the tour, covering shipping zones, shipping methods and recommended shipping options
  8. The tour should not appear again upon revisit or refresh of the page, if you complete the tour

Case 2: The store sells physical products, has JP and WCS installed and connected, and is located in the US.

  1. Start OBW and choose United States as store country
  2. Choose "Physical products"
  3. Install Jetpack and WooCommerce Shipping from the Business Details tab.
  4. Complete the OBW
  5. Connect and approve Jetpack when prompted.
  6. Navigate to WooCommerce -> Settings -> Shipping
  7. "United States (US)" zone should be created with Free shipping method
  8. Observe that the 3 step tour is displayed, with step one showing "Step 1 of 3"
  9. There should be 3 steps in the tour, with the third one highlighting the WooCommerce Shipping section
  10. The tour should not appear again upon revisit or refresh of the page, if you complete the tour

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm changelog add --filter=<project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@chihsuan chihsuan marked this pull request as ready for review August 22, 2022 03:08
@chihsuan chihsuan self-assigned this Aug 22, 2022
@github-actions github-actions bot added focus: react admin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Aug 22, 2022
@chihsuan chihsuan requested a review from a team August 22, 2022 03:12
@github-actions
Copy link
Contributor

github-actions bot commented Aug 22, 2022 •
edited
Loading

Test Results Summary

Commit SHA: e5abe8b

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests000000NaNm NaNs
E2E Tests186001018715m 3s
 To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

ilyasfoo
ilyasfoo suggested changes Aug 26, 2022
View reviewed changes
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think the fix is unreliable, I've been getting 2 steps even with the delay. It seems on my instance it takes at least 2 seconds to load. On sites with worse performance (eg: 20+ plugins activated), I imagine it would require more delay.

image

I think as an alternative to refactoring, we could also just copy & paste all the relevant resolver resolution status which should be reliable enough

@chihsuan
Copy link
Member Author

chihsuan commented Aug 26, 2022

I think the fix is unreliable, I've been getting 2 steps even with the delay. It seems on my instance it takes at least 2 seconds to load. On sites with worse performance (eg: 20+ plugins activated), I imagine it would require more delay.

Thanks for testing this! 👍

I think as an alternative to refactoring, we could also just copy & paste all the relevant resolver resolution status which should be reliable enough

In this case, I would prefer refactoring because if we copy & paste the logic then we need to ensure when we change one side, we must change another. Otherwise, it would break this again.

@chihsuan chihsuan force-pushed the fix/34194-shipping-tour-step-num branch from 9ad0aaa to 33b284e Compare August 26, 2022 08:51
@chihsuan chihsuan force-pushed the fix/34194-shipping-tour-step-num branch from 4371be5 to 1762b61 Compare August 26, 2022 09:08
@chihsuan
Copy link
Member Author

chihsuan commented Aug 26, 2022

@ilyasfoo I just found out that there is a simple way to fix this. 😃 We can put <shippingTour /> directly in the <ShippingRecommendations /> component so that we can pass a prop to ShippingTour to determine whether to show the step or not. I've updated the PR.

@chihsuan chihsuan requested a review from ilyasfoo August 26, 2022 09:17
ilyasfoo
ilyasfoo approved these changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Awesome, @chihsuan! This tested well, LGTM! 🚢

@chihsuan chihsuan merged commit 0834473 into trunk Aug 29, 2022
@chihsuan chihsuan deleted the fix/34194-shipping-tour-step-num branch August 29, 2022 09:25
@github-actions github-actions bot added this to the 7.0.0 milestone Aug 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2022

Hi @chihsuan, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ilyasfoo ilyasfoo ilyasfoo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chihsuan chihsuan

Labels

plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Testing PR#33688 - "1 of 2 steps" is shown for the first step in a 3 step tour

3 participants

@chihsuan @ilyasfoo