Store: If TaxJar plugin is active, do not show core settings #19535

allendav · 2017-11-06T20:55:02Z

To test:

Navigate to http://calypso.localhost:3000/store/settings/taxes/{DOMAIN}
Wait for the placeholder to pass
You'll either be placed on a view that tells you you have the TaxJar plugin active, or a view that lets you adjust your tax settings
Navigate to http://calypso.localhost:3000/plugins/manage/{DOMAIN} and deactivate TaxJar if active, or install and activate if not
Return to http://calypso.localhost:3000/store/settings/taxes/{DOMAIN}
Ensure you see whatever view you didn't see before

cc @jeffstieler @justinshreve @kellychoffman @jameskoster

edit: Note: I am going to submit a PR to TaxJar next to have them not hook :allofthethings: unless the user has both checked the enabled setting for their plugin AND entered an API key. That should help WCS easily work around active TaxJar plugins that are not fully configured with an API key.

matticbot · 2017-11-06T20:55:10Z

Test live: https://calypso.live/?branch=fix/17110-store-tax-settings-detect-taxjar

a8ci18n · 2017-11-06T21:02:20Z

client/extensions/woocommerce/app/settings/taxes/taxes-taxjar.js

+						<p>
+							{ translate( "Your store's taxes are being managed by a third-party plugin. " ) }
+							<ExternalLink icon href={ adminUrl } rel="noopener noreferrer">
+								{ translate( 'Plugin settings' ) }


Hi! I've found a possible matching string that has already been translated 21 times:
translate( 'Edit plugin settings' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-11-06T21:02:21Z

client/extensions/woocommerce/app/settings/taxes/taxes-wcs.js

+			if ( onSuccessExtra ) {
+				onSuccessExtra();
+			}
+			return successNotice( translate( 'Settings updated successfully.' ), {


Hi! I've found a possible matching string that has already been translated 24 times:
translate( 'Theme settings updated successfully.' ) ES Score: 13
See 2 additional suggestions in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

jameskoster · 2017-11-09T13:00:41Z

OK, I need a rundown on this cause it feels a little weird how I understand it currently.

The issue is the TaxJar plugin doesn't work properly in Calypso and has been superseded by functionality in Services, right?

So, if the TaxJar plugin is active, you see this;

And if it's not (services is handling taxes) you see this;

Is that right? If so, the flow of heading over to manage plugins, deactivating TaxJar, then coming back to the tax settings screen to learn that my taxes are now being handled by........ TaxJar... would feel very odd xD

Just want to make sure my understanding is correct before giving feedback.

allendav · 2017-11-13T16:57:36Z

@jameskoster

It is a little odd - it is because we used to install the TaxJar plugin for people and then hooked it to allow WooCommerce Services to drive requests through WooCommerce Services' server to the TaxJar server.

Now, WooCommerce Services can do tax calculations without the TaxJar plugin installed (and yet it still uses TaxJar on the backend so we need to give the data privacy notice.)

We still want to allow people to continue to use the TaxJar plugin if they want to (e.g. for filing their taxes electronically) so that's basically why we have these two views (one view with TaxJar plugin active and one view without)

So... all that said, I would like us to consider replacing (for the "without" case)

Sales tax calculations are provided by a third party: TaxJar. By enabling this option, TaxJar will have access to some of your data. Learn more.

with

Sales tax calculations are provided by WooCommerce Services. When this option is enabled, WooCommerce Services will share some of your data with a third party. Learn more.

cc @jeffstieler @pmaiorana @justinshreve

timmyc

Some minor comments above. Verified things rendered as expected when taxjar was installed/enabled vs not. I did encounter some odd behavior editing - which I've tried to capture in a .gif with 🐭 waving:

The Tax Rates block is rendered later than the rest of the page, which I'm guessing has to do with the plugin list loading - but it results in the page jumping around a bit. Not sure if we want a placeholder here to make it feel less choppy. Note - this is happening in production too, so maybe moot.
I uncheck the "charge taxes on shipping costs", save, am told things are all good, but upon refresh the old setting is still shown. Note, this is also happening in production 😆

timmyc · 2017-11-13T17:41:38Z

client/extensions/woocommerce/app/settings/taxes/index.js

 		className: PropTypes.string,
+		siteId: PropTypes.number,
+		sitePluginsLoaded: PropTypes.bool,
+		taxJarPluginActive: PropTypes.bool,


missing isRequestingSitePlugins, and siteSlug

Fixed in 8659592

timmyc · 2017-11-13T17:47:59Z

client/extensions/woocommerce/app/settings/taxes/taxes-taxjar.js

+		} ),
+	};
+
+	onDeactivate = event => {


It appears this isn't used?

Oops - fixed in 8659592

timmyc · 2017-11-13T17:48:35Z

client/extensions/woocommerce/app/settings/taxes/taxes-taxjar.js

+
+	render = () => {
+		const { className, site, translate } = this.props;
+


Do we need to be concerned about site being null here at all?

It should not be null, but defensively changed site to isRequired in 8659592

timmyc · 2017-11-13T17:50:47Z

client/extensions/woocommerce/app/settings/taxes/taxes-wcs.js

+			slug: PropTypes.string,
+			ID: PropTypes.number,
+		} ),
+		className: PropTypes.string,


Missing a few props loaded, pricesIncludeTaxes, shippingIsTaxable, taxesEnabled

Fixed in 8659592

timmyc · 2017-11-13T17:52:23Z

client/extensions/woocommerce/app/settings/taxes/taxes-wcs.js

+	componentDidMount = () => {
+		const { site } = this.props;
+
+		if ( site && site.ID ) {


we could pass in siteId as a prop and simplify some of the logic here and in componentWillReceiveProps

Agreed! Fixed in 054bd7f

timmyc · 2017-11-13T17:56:50Z

client/extensions/woocommerce/app/settings/taxes/taxes-wcs.js

+
+	render = () => {
+		const { className, loaded, site, translate } = this.props;
+


Again any concern or possibility of site being null?

No possibility, but I just made it isRequired in 054bd7f

justinshreve · 2017-11-13T18:17:39Z

Sales tax calculations are provided by WooCommerce Services. When this option is enabled, WooCommerce Services will share some of your data with a third party. Learn more.

I think that text makes sense. It's definitely less confusing.

allendav · 2017-11-13T19:51:09Z

@justinshreve wrote:

Sales tax calculations are provided by WooCommerce Services. When this option is enabled, WooCommerce Services will share some of your data with a third party. Learn more.

I think that text makes sense. It's definitely less confusing.

Fixed in 9d70316 looks like this now @jameskoster

allendav · 2017-11-13T21:36:38Z

@timmyc wrote:

I uncheck the "charge taxes on shipping costs", save, am told things are all good, but upon refresh the old setting is still shown. Note, this is also happening in production

I can't reproduce this with this branch - are you sure the TaxJar plugin is not still active on your site? (It forces these settings' values and can cause this behavior, which is basically why we are doing this PR.)

jameskoster · 2017-11-14T10:15:34Z

Sales tax calculations are provided by WooCommerce Services. When this option is enabled, WooCommerce Services will share some of your data with a third party. Learn more.

I thought we were averse to highlighting 'WooCommerce' anywhere in Store?

I actually think it's the other card which is the problem. "Your store's taxes are being handled by a third-party plugin" just feels too vague. It reminds me of one of those unhelpful Windows error messages from years gone by xD

Couldn't we be more explicit there? Explain that they're running the TaxJar plugin which is no longer required unless they want to do x & y. Then re-assure them that if they deactivate it their store will continue to function just fine.

timmyc · 2017-11-14T16:08:58Z

@allendav still seeing the issue on my site with this branch - another gif from this morning:

TaxJar installed but disabled:

allendav · 2017-11-14T17:14:16Z

@jameskoster wrote:

I thought we were averse to highlighting 'WooCommerce' anywhere in Store?

I think we need too not be afraid to mention WooCommerce Services - after all, WooCommerce Services is a product we want to sell through Store on WPCOM. cc @toddwilkens @jeffstieler

allendav · 2017-11-14T17:30:55Z

@timmyc wrote:

still seeing the issue on my site with this branch - another gif from this morning

@timmyc - a few questions (or grant me access to that site)

Are you able to toggle the setting (and get it to stick after save) if tax calculations are enabled?
In wp-admin on that site ( /wp-admin/admin.php?page=wc-settings&tab=tax ), when tax calculations are enabled, are you able to change the Shipping Tax Class setting from Zero Rate to Standard and have it stick? What appears in calypso after a refresh?
In wp-admin on that site, when tax calculations are enabled, are you able to change the Shipping Tax Class setting from Standard to Zero Rate and have it stick? What appears in calypso after a refresh?
In wp-admin on that site, I presume you still have standard rate and zero rate sub-tabs?

allendav · 2017-11-14T18:02:44Z

@jameskoster wrote:

Couldn't we be more explicit there? Explain that they're running the TaxJar plugin which is no longer required unless they want to do x & y. Then re-assure them that if they deactivate it their store will continue to function just fine.

Good point. OK, let's do this then. If the TaxJar plugin is active on their site, we'll show this

Sound good?

a8ci18n · 2017-11-14T18:11:41Z

client/extensions/woocommerce/app/settings/taxes/taxes-taxjar.js

+								'You can manage settings, including your personal TaxJar API key, in the plugin settings. '
+							) }
+							<ExternalLink icon href={ adminUrl } rel="noopener noreferrer">
+								{ translate( 'Plugin settings' ) }


Hi! I've found a possible matching string that has already been translated 21 times:
translate( 'Edit plugin settings' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

timmyc · 2017-11-14T20:09:31Z

@allendav I just sent you an invite to my test site. Feel free to do whatever you please there... make yourself at home 🏚

jameskoster · 2017-11-15T13:12:52Z

@allendav I think this is an improvement. It's a bit wordy but it's all relevant.

Perhaps to break things up a little we should bullet-list the features the plugin adds. Then switch the last 2 paragraphs.

Your store’s taxes are being managed by the TaxJar - Sales Tax Automation for WooCommerce plugin. This plugin provides optional features such as:

Automated reporting & filing

Multi-nexus support

If you don’t need these features we recommend removing this plugin. Don’t worry, we’ll still handle sales tax calculations for you.

You can manage this plugin’s settings in wp-admin.

allendav · 2017-11-15T19:26:16Z

@jameskoster - with 806731b we now have

a8ci18n · 2017-11-15T19:33:13Z

client/extensions/woocommerce/app/settings/taxes/taxes-taxjar.js

+						<p>
+							{ translate( "You can manage this plugin's settings in wp-admin. " ) }
+							<ExternalLink icon href={ adminUrl } rel="noopener noreferrer">
+								{ translate( 'Plugin settings' ) }


Hi! I've found a possible matching string that has already been translated 21 times:
translate( 'Edit plugin settings' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

allendav · 2017-11-15T20:17:44Z

@timmyc - I tracked down your lack-of-stickyness issue and wrote it up separately here - #19857 - not sure how your site got in that state

jameskoster · 2017-11-16T14:50:00Z

@allendav one final nitpick; let's lose the bottom margin on the final paragraph :)

allendav · 2017-11-16T17:06:54Z

@jameskoster wrote:

@allendav one final nitpick; let's lose the bottom margin on the final paragraph :)

Fixed in 8d4fab1

timmyc

Couple other minor comments, otherwise this is looking good to me code-wise

timmyc · 2017-11-16T17:02:04Z

client/extensions/woocommerce/app/settings/taxes/index.js

 	};

 	componentDidMount = () => {
-		const { site } = this.props;
+		const { isRequestingSitePlugins, siteId, sitePluginsLoaded } = this.props;


This logic is duplicated in componentWillReceiveProps. Perhaps it can be extracted into a maybeFetchPlugins method and called in both lifecycle methods?

Fixed in 4dc1307

timmyc · 2017-11-16T17:05:04Z

client/extensions/woocommerce/app/settings/taxes/index.js


-		if ( site && site.ID ) {
-			this.props.fetchTaxSettings( site.ID );
+		if ( siteId && ! isRequestingSitePlugins && ! sitePluginsLoaded ) {


The only thing that I thought could go awry here is that potentially site plugins could become stale in the context of this component. Since sitePluginsLoaded just checks to see if the plugins array is empty, we could potentially not fetch the latest plugin data here... i.e. I change plugin status outside of calypso.

If you are on the Settings > Taxes page, then go to Plugins and install and activate TaxJar, then return to the Settings > Taxes page, you'll have this problem too. I'll come up with a fix.

It isn't super critical imho, but figured it is worth noting. Could be handled in a subsequent PR.

OK, in 2e6ebd3 I made a change so that if you install/activate TaxJar from Calypso and return to the Taxes Settings view (in the same tab/window), at least we'll update appropriately. It doesn't solve the problem of navigating to wp-admin in a separate tab/window and messing there, but it is a start.

I've written #19928 for the case where they use another window/tab to deactivate/activate the TaxJar plugin.

timmyc · 2017-11-16T17:06:30Z

client/extensions/woocommerce/app/settings/taxes/taxes-rates.js

 		}
 	};

 	componentWillReceiveProps = nextProps => {
 		if ( nextProps.loadedSettingsGeneral ) {
 			if ( ! nextProps.loadedTaxRates ) {
-				this.props.fetchTaxRates( nextProps.site.ID, nextProps.address );
+				this.props.fetchTaxRates( nextProps.siteId, nextProps.address );


could these two if conditions be combined here? if ( nextProps.loadedSettingsGeneral && ! nextProps.loadedTaxRates )

Fixed in 4dc1307

allendav · 2017-11-16T23:05:20Z

@timmyc - still good to go with the last round of changes?

timmyc · 2017-11-16T23:13:07Z

Yep @allendav - still looking good 👍

yansern · 2020-01-23T11:20:34Z

Please ignore my previous mention as it was incorrectly referenced.

allendav self-assigned this Nov 6, 2017

allendav requested review from timmyc and ryelle November 6, 2017 20:59

allendav added Store [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 6, 2017

allendav requested a review from kellychoffman November 6, 2017 20:59

a8ci18n reviewed Nov 6, 2017

View reviewed changes

allendav force-pushed the fix/17110-store-tax-settings-detect-taxjar branch from f56194e to eb317c7 Compare November 7, 2017 18:46

timmyc reviewed Nov 13, 2017

View reviewed changes

Allen Snook added 4 commits November 14, 2017 09:15

Store: If TaxJar plugin is active, do not show core settings

df0e9f4

Store: Update third-party tax notice text

9d1f3d8

No need to pass the entire site as a prop. Just pass the ID and slug.

337513a

Enforced mapped props, remove unused deactivation method

e7c566c

allendav force-pushed the fix/17110-store-tax-settings-detect-taxjar branch from 8659592 to e7c566c Compare November 14, 2017 17:15

Tweak the wording we present to the merchant when TaxJar is active

580b518

a8ci18n reviewed Nov 14, 2017

View reviewed changes

Tweak taxjar active wording a bit more

806731b

a8ci18n reviewed Nov 15, 2017

View reviewed changes

Remove bottom margin on last paragraph of tax jar info

8d4fab1

timmyc approved these changes Nov 16, 2017

View reviewed changes

Allen Snook added 2 commits November 16, 2017 11:04

DRY plugin fetching

4dc1307

Always fetch plugins when the component mounts

2e6ebd3

allendav merged commit f149e84 into master Nov 17, 2017

allendav deleted the fix/17110-store-tax-settings-detect-taxjar branch November 17, 2017 03:10

matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 17, 2017

yansern mentioned this pull request Jan 23, 2020

Gutenboarding: domain picker texts are centered #38834

Closed

This was referenced May 15, 2021

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.10.0 awaisab172/wp-calypso#75

Open

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#76

Open

This was referenced Oct 1, 2023

[Snyk] Security upgrade @wordpress/editor from 9.0.11 to 9.10.0 awaisab172/wp-calypso#158

Open

[Snyk] Fix for 1 vulnerabilities awaisab172/wp-calypso#160

Open

awaisab172 mentioned this pull request Oct 31, 2023

[Snyk] Fix for 2 vulnerabilities awaisab172/wp-calypso#163

Open


		render = () => {
		const { className, site, translate } = this.props;


		render = () => {
		const { className, loaded, site, translate } = this.props;

Store: If TaxJar plugin is active, do not show core settings #19535

Store: If TaxJar plugin is active, do not show core settings #19535

Conversation

allendav commented Nov 6, 2017 • edited Loading

matticbot commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameskoster commented Nov 9, 2017 • edited Loading

allendav commented Nov 13, 2017 • edited Loading

timmyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinshreve commented Nov 13, 2017

allendav commented Nov 13, 2017

allendav commented Nov 13, 2017

jameskoster commented Nov 14, 2017

timmyc commented Nov 14, 2017

allendav commented Nov 14, 2017 • edited Loading

allendav commented Nov 14, 2017 • edited Loading

allendav commented Nov 14, 2017

Choose a reason for hiding this comment

timmyc commented Nov 14, 2017

jameskoster commented Nov 15, 2017 • edited Loading

allendav commented Nov 15, 2017

Choose a reason for hiding this comment

allendav commented Nov 15, 2017

jameskoster commented Nov 16, 2017

allendav commented Nov 16, 2017

timmyc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allendav Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allendav commented Nov 16, 2017

timmyc commented Nov 16, 2017

yansern commented Jan 23, 2020

allendav commented Nov 6, 2017 •

edited

Loading

jameskoster commented Nov 9, 2017 •

edited

Loading

allendav commented Nov 13, 2017 •

edited

Loading

allendav commented Nov 14, 2017 •

edited

Loading

allendav commented Nov 14, 2017 •

edited

Loading

jameskoster commented Nov 15, 2017 •

edited

Loading

allendav Nov 16, 2017 •

edited

Loading