Fix tax task showing as not completed after setting up tax #36468
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
focus: react admin
plugin: woocommerce
Contributor
Test Results SummaryCommit SHA: 06a6fcb
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. |
rjchow
approved these changes
Jan 18, 2023
Contributor
rjchow
left a comment
rjchow
left a comment
There was a problem hiding this comment.
Looks good to me other than the comment on the dependency array!
| try { | ||
| await Promise.all( [ | ||
| updateAndPersistSettingsForGroup( 'tax', { | ||
| tax: { |
Contributor
There was a problem hiding this comment.
not introduced by this PR, but I'm surprised there's no bug or lint complaint about taxSettings not being in the useCallback dependency array.
Do you know if it's necessary to add it?
Member
Author
There was a problem hiding this comment.
Good point! I took a quick look, and I think it's okay for new users because there are only default tax settings. But we should better add it to the dependency array to avoid some edge cases. I'll create an issue for it. We may need to look into it deeper.
adrianduffell
pushed a commit
that referenced
this pull request
Jan 20, 2023
* Wait for requests to finish before redirecting * Add changelog
vedanshujain
pushed a commit
that referenced
this pull request
Jan 25, 2023
* Wait for requests to finish before redirecting * Add changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All Submissions:
Changes proposed in this Pull Request:
Closes #35745.
Wait for requests to finish before redirecting to make sure the wc_connect_taxes_enabled option is set synchronously.
Ideally, we should also add a test for it, but it's a bit challenging because it requires a Jetpack connection to complete the flow. I think we can tackle this problem when we start to work on the e2e project.
How to test the changes in this Pull Request:
WooCommerce > Home > Add tax ratesYou can use this release zip file for testing.
Other information:
pnpm --filter=<project> changelog add?FOR PR REVIEWER ONLY:
Screen.Recording.2023-01-17.at.16.18.37.mov