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

Replace WCS&T tax onboarding with Woo Tax onboarding. #1462

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kloon
Copy link

@kloon kloon commented Apr 16, 2024

Changes proposed in this Pull Request:

Replace the WCS&T Tax onboarding with just Woo Tax.

  • Remove the jetpack requirement from the tax onboarding and update wording in step.
  • Added functionality to the addons screen override to handle installing Woo Tax and Woo Shipping for when we start supporting it via the addons screen.
  • Added functionality to suppress any future Woo Shipping and Woo Tax messaging that will get added to WC core.

These changes form part of the work to roll out the new Woo Shipping and Woo Tax to WooExpress sites.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Testing instructions taken from #975

Test Woo Tax

Click on "Yes please" and observe that you're redirected to Homescreen with Tax task completed
Go to WooCommerce > Settings > Tax
Observe that you have Enable automated taxes selected in the Automated taxes dropdown
Go to WooCommerce > Home > Tax task
Click on No thanks, I'll set up manually and observe that you're redirected to tax settings screen
Go to WooCommerce > Home > Tax task
Click on I don't charge sales tax
Observe that you're redirected to Homescreen with Tax task completed

Test manual configuration

Go to WooCommerce > Settings > General
Remove all address fields, set store country to "Malaysia - Johor" and save
Go to WooCommerce > Home > Tax task
Fill in all store address fields in Set store location and click on continue
Click on Configure and observe that you're redirected to tax settings
Go to WooCommerce > Settings > General
Observe that the store address values are saved from the values inserted in the previous form

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?

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.

Add Woo Shipping & Woo Tax install functions to Addons page override for when core introduces this.
Surpress Woo Shipping & Woo Tax messaging from WC core when they get added.
Copy link

github-actions bot commented Apr 16, 2024

Size Change: -55 B (0%)

Total Size: 197 kB

Filename Size Change
./build/index.js 122 kB -55 B (0%)
ℹ️ View Unchanged
Filename Size
./build/53.js 1.05 kB
./build/index.css 883 B
./build/marketing.js 58 kB
./build/payment-gateway-suggestions.css 1.25 kB
./build/payment-gateway-suggestions.js 6.46 kB
./build/plugins.js 3.92 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 805 B

compressed-size-action

@@ -17,7 +17,7 @@ import { TaxChildProps } from '../utils';
export const Card: React.FC< TaxChildProps > = () => {
return (
<PartnerCard
name={ __( 'WooCommerce Tax', 'woocommerce' ) }
name={ __( 'WooC Tax', 'woocommerce' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this one should be Woo Tax ?

@PanosSynetos
Copy link
Contributor

Click on "Yes please"

I'm not sure how to get to this point, so I might leave this for @ilyasfoo or @chihsuan to test this out.

I can code review the PHP part, what do you think @kloon ?

@kloon
Copy link
Author

kloon commented Apr 16, 2024

I'm not sure how to get to this point, so I might leave this for @ilyasfoo or @chihsuan to test this out.

I was also unsure but found you need to upgrade away from a trial site for this to show.

Sk5FbH.png

Also noticing some of the text are not reflecting the updates, might be due to the sync/build process not generating new po/mo files

I can code review the PHP part, what do you think @kloon ?

That would be fine.

Copy link
Contributor

@PanosSynetos PanosSynetos left a comment

Choose a reason for hiding this comment

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

👌 PHP Side of things look good - As I mentioned, I couldn't manual test the process

Minor comments

Make sure that all edited PHP files that have an @version header, it should be

* @version x.x.x

so when you do a release, replace it with the actual version.

You'd also need an entry in readme.txt

See @waclawjacek 's PR

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.

Hey, @kloon! Apologies for the delays in reviewing!

I assume we do not expect the Tax task to install/activate woocommerce-tax plugin? This is because we've removed the install/activate plugins step for bridge (ref), and it could only install plugins listed in wp.org.

I tested and it seems the task is not marked as complete since we've changed the option from wc_connect_taxes_enabled. Otherwise, I think everything else is working as intended

'repo-slug' => 'woocommerce-tax',
);

WC_Install::background_installer( $services_plugin_id, $services_plugin );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're already aware, but just precaution that background_installer only supports wp.org listed plugins, in case we're releasing this prior making the plugin public.

@@ -57,9 +57,9 @@ export const Plugins: React.FC< SetupStepProps > = ( {
nextStep();
}, [ isResolving ] );

const agreementText = pluginsToActivate.includes( 'woocommerce-services' )
const agreementText = pluginsToActivate.includes( 'woocommerce-tax' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the Plugins component isn't used since we're expecting bridge to always have woocommerce-services installed (Removal commit). Let me know if installing plugin in the task is required!

@@ -112,7 +112,7 @@ export const Tax: React.FC< TaxProps > = ( { onComplete, query, task } ) => {
updateAndPersistSettingsForGroup( 'tax', {
tax: {
...taxSettings,
wc_connect_taxes_enabled: 'no',
woo_tax_taxes_enabled: 'no',
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the Tax task uses wc_connect_taxes_enabled option to determine its completion status (ref).

In order to support the new option, we'll need to override the default Tax task class, here's ab example of how we did it for Products task: https://github.com/Automattic/wc-calypso-bridge/pull/963/files.

@chihsuan chihsuan removed their request for review July 31, 2024 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants