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.

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

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

Merged
merged 9 commits into from
Nov 17, 2017
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
df0e9f4
Store: If TaxJar plugin is active, do not show core settings
Nov 6, 2017
9d1f3d8
Store: Update third-party tax notice text
Nov 13, 2017
337513a
No need to pass the entire site as a prop. Just pass the ID and slug.
Nov 13, 2017
e7c566c
Enforced mapped props, remove unused deactivation method
Nov 13, 2017
580b518
Tweak the wording we present to the merchant when TaxJar is active
Nov 14, 2017
806731b
Tweak taxjar active wording a bit more
Nov 15, 2017
8d4fab1
Remove bottom margin on last paragraph of tax jar info
Nov 16, 2017
4dc1307
DRY plugin fetching
Nov 16, 2017
2e6ebd3
Always fetch plugins when the component mounts
Nov 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 42 additions & 157 deletions client/extensions/woocommerce/app/settings/taxes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,207 +9,92 @@ import PropTypes from 'prop-types';
import { bindActionCreators } from 'redux';
import classNames from 'classnames';
import { connect } from 'react-redux';
import { find, isEmpty } from 'lodash';
import { localize } from 'i18n-calypso';

/**
* Internal dependencies
*/
import ActionHeader from 'woocommerce/components/action-header';
import {
areSettingsGeneralLoaded,
areTaxCalculationsEnabled,
} from 'woocommerce/state/sites/settings/general/selectors';
import {
areTaxSettingsLoaded,
getPricesIncludeTax,
getShippingIsTaxFree,
} from 'woocommerce/state/sites/settings/tax/selectors';
import ExtendedHeader from 'woocommerce/components/extended-header';
import { updateTaxesEnabledSetting } from 'woocommerce/state/sites/settings/general/actions';
import QuerySettingsGeneral from 'woocommerce/components/query-settings-general';
import { fetchTaxRates } from 'woocommerce/state/sites/meta/taxrates/actions';
import { fetchTaxSettings, updateTaxSettings } from 'woocommerce/state/sites/settings/tax/actions';
import { getLink } from 'woocommerce/lib/nav-utils';
import { fetchPlugins } from 'state/plugins/installed/actions';
import { getPlugins, isRequesting } from 'state/plugins/installed/selectors';
import { getSelectedSiteWithFallback } from 'woocommerce/state/sites/selectors';
import Main from 'components/main';
import TaxSettingsSaveButton from './save-button';
import SettingsNavigation from '../navigation';
import { successNotice, errorNotice } from 'state/notices/actions';
import StoreAddress from 'woocommerce/components/store-address';
import TaxesOptions from './taxes-options';
import TaxesRates from './taxes-rates';
import SettingsTaxesPlaceholder from './taxes-placeholder';
import SettingsTaxesTaxJar from './taxes-taxjar';
import SettingsTaxesWooCommerceServices from './taxes-wcs';

class SettingsTaxes extends Component {
constructor( props ) {
super( props );
this.state = {
isSaving: false,
pricesIncludeTaxes: props.pricesIncludeTaxes,
shippingIsTaxable: props.shippingIsTaxable,
taxesEnabled: props.taxesEnabled,
userBeganEditing: false,
};
}

static propTypes = {
site: PropTypes.shape( {
slug: PropTypes.string,
ID: PropTypes.number,
} ),
className: PropTypes.string,
isRequestingSitePlugins: PropTypes.bool,
siteId: PropTypes.number,
sitePluginsLoaded: PropTypes.bool,
siteSlug: PropTypes.string,
taxJarPluginActive: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

missing isRequestingSitePlugins, and siteSlug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8659592

};

componentDidMount = () => {
const { site } = this.props;
const { isRequestingSitePlugins, siteId, sitePluginsLoaded } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4dc1307


if ( site && site.ID ) {
this.props.fetchTaxSettings( site.ID );
if ( siteId && ! isRequestingSitePlugins && ! sitePluginsLoaded ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@allendav allendav Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.props.fetchPlugins( [ siteId ] );
}
};

componentWillReceiveProps = newProps => {
if ( ! this.state.userBeganEditing ) {
const { site } = this.props;
const newSiteId = ( newProps.site && newProps.site.ID ) || null;
const oldSiteId = ( site && site.ID ) || null;
if ( oldSiteId !== newSiteId ) {
this.props.fetchTaxSettings( newSiteId );
}
const { isRequestingSitePlugins, siteId, sitePluginsLoaded } = newProps;

this.setState( {
pricesIncludeTaxes: newProps.pricesIncludeTaxes,
shippingIsTaxable: newProps.shippingIsTaxable,
taxesEnabled: newProps.taxesEnabled,
} );
if ( siteId && ! isRequestingSitePlugins && ! sitePluginsLoaded ) {
this.props.fetchPlugins( [ siteId ] );
}
};

onEnabledChange = () => {
this.setState( { taxesEnabled: ! this.state.taxesEnabled, userBeganEditing: true } );
};

onCheckboxChange = event => {
const option = event.target.name;
const value = event.target.checked;
this.setState( { [ option ]: value, userBeganEditing: true } );
};

pageHasChanges = () => {
return this.state.userBeganEditing;
};

onSave = ( event, onSuccessExtra ) => {
const { site, translate } = this.props;

event.preventDefault();
this.setState( { isSaving: true } );

const onSuccess = () => {
this.setState( { isSaving: false, userBeganEditing: false } );
if ( onSuccessExtra ) {
onSuccessExtra();
}
return successNotice( translate( 'Settings updated successfully.' ), {
duration: 4000,
displayOnNextPage: true,
} );
};

const onFailure = () => {
this.setState( { isSaving: false } );
return errorNotice(
translate( 'There was a problem saving your changes. Please try again.' )
);
};

// TODO - chain these

this.props.updateTaxesEnabledSetting( site.ID, this.state.taxesEnabled );

this.props.updateTaxSettings(
site.ID,
this.state.pricesIncludeTaxes || false,
! this.state.shippingIsTaxable, // note the inversion
onSuccess,
onFailure
);
};

onAddressChange = address => {
const { site } = this.props;
this.props.fetchTaxRates( site.ID, address, true );
};

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

if ( ! loaded ) {
// TODO placeholder
return <QuerySettingsGeneral siteId={ site.ID } />;
}

const breadcrumbs = [
<a href={ getLink( '/store/settings/:site/', site ) }>{ translate( 'Settings' ) }</a>,
<span>{ translate( 'Taxes' ) }</span>,
];
const renderTaxesByTaxJar = taxJarPluginActive;
const renderTaxesByWCS = sitePluginsLoaded && ! taxJarPluginActive;
const renderPlaceholder = ! renderTaxesByTaxJar && ! renderTaxesByWCS;

return (
<Main className={ classNames( 'settings-taxes', className ) }>
<ActionHeader breadcrumbs={ breadcrumbs }>
<TaxSettingsSaveButton onSave={ this.onSave } />
</ActionHeader>
<SettingsNavigation activeSection="taxes" />
<div className="taxes__nexus">
<ExtendedHeader
label={ translate( 'Store Address' ) }
description={ translate(
'The address of where your business is located for tax purposes.'
) }
/>
<StoreAddress
className="taxes__store-address"
onSetAddress={ this.onAddressChange }
showLabel={ false }
/>
</div>
<TaxesRates
taxesEnabled={ this.state.taxesEnabled }
onEnabledChange={ this.onEnabledChange }
site={ site }
/>
<TaxesOptions
onCheckboxChange={ this.onCheckboxChange }
pricesIncludeTaxes={ this.state.pricesIncludeTaxes }
shippingIsTaxable={ this.state.shippingIsTaxable }
/>
{ renderPlaceholder && <SettingsTaxesPlaceholder siteSlug={ siteSlug } /> }
{ renderTaxesByTaxJar && <SettingsTaxesTaxJar site={ site } /> }
{ renderTaxesByWCS && (
<SettingsTaxesWooCommerceServices siteId={ siteId } siteSlug={ siteSlug } />
) }
</Main>
);
};
}

function mapStateToProps( state ) {
const loaded = areTaxSettingsLoaded( state ) && areSettingsGeneralLoaded( state );
const site = getSelectedSiteWithFallback( state );
const pricesIncludeTaxes = getPricesIncludeTax( state );
const shippingIsTaxable = ! getShippingIsTaxFree( state ); // note the inversion
const taxesEnabled = areTaxCalculationsEnabled( state );
const siteId = site ? site.ID : null;
const siteSlug = site ? site.slug : '';

const isRequestingSitePlugins = site ? isRequesting( state, siteId ) : false;
const sitePlugins = site ? getPlugins( state, [ site.ID ] ) : [];
const sitePluginsLoaded = ! isEmpty( sitePlugins );
const taxJarPluginActive = !! find( sitePlugins, {
slug: 'taxjar-simplified-taxes-for-woocommerce',
active: true,
} );

return {
loaded,
pricesIncludeTaxes,
shippingIsTaxable,
isRequestingSitePlugins,
site,
taxesEnabled,
siteId,
sitePluginsLoaded,
siteSlug,
taxJarPluginActive,
};
}

function mapDispatchToProps( dispatch ) {
return bindActionCreators(
{
fetchTaxRates,
fetchTaxSettings,
updateTaxesEnabledSetting,
updateTaxSettings,
fetchPlugins,
},
dispatch
);
Expand Down
6 changes: 5 additions & 1 deletion client/extensions/woocommerce/app/settings/taxes/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
a.external-link {
margin-left: 8px;
}

a.external-link .gridicons-external {
margin-left: 4px;
}
Expand All @@ -48,3 +48,7 @@
}
}
}

.taxes__placeholder .card {
@include placeholder();
}
40 changes: 40 additions & 0 deletions client/extensions/woocommerce/app/settings/taxes/taxes-placeholder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* External dependencies
*
* @format
*/

import React from 'react';
import PropTypes from 'prop-types';
import { localize } from 'i18n-calypso';

/**
* Internal dependencies
*/
import ActionHeader from 'woocommerce/components/action-header';
import { getLink } from 'woocommerce/lib/nav-utils';
import SettingsNavigation from '../navigation';

const SettingsTaxesPlaceholder = ( { siteSlug, translate } ) => {
const breadcrumbs = [
<a href={ getLink( '/store/settings/:site/', { slug: siteSlug } ) }>
{ translate( 'Settings' ) }
</a>,
<span>{ translate( 'Taxes' ) }</span>,
];

return (
<div>
<ActionHeader breadcrumbs={ breadcrumbs } />
<SettingsNavigation activeSection="taxes" />
<div className="taxes__placeholder card" />
</div>
);
};

SettingsTaxesPlaceholder.propTypes = {
siteSlug: PropTypes.string.isRequired,
translate: PropTypes.func.isRequired,
};

export default localize( SettingsTaxesPlaceholder );
36 changes: 14 additions & 22 deletions client/extensions/woocommerce/app/settings/taxes/taxes-rates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,21 @@ import QuerySettingsGeneral from 'woocommerce/components/query-settings-general'
class TaxesRates extends Component {
static propTypes = {
onEnabledChange: PropTypes.func.isRequired,
site: PropTypes.shape( {
slug: PropTypes.string,
} ),
siteId: PropTypes.number.isRequired,
};

componentDidMount = () => {
const { address, loadedSettingsGeneral, loadedTaxRates, site } = this.props;
const { address, loadedSettingsGeneral, loadedTaxRates, siteId } = this.props;

if ( site && site.ID ) {
if ( loadedSettingsGeneral && ! loadedTaxRates ) {
this.props.fetchTaxRates( site.ID, address );
}
if ( loadedSettingsGeneral && ! loadedTaxRates ) {
this.props.fetchTaxRates( siteId, address );
}
};

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4dc1307

}
}
};
Expand Down Expand Up @@ -263,8 +259,8 @@ class TaxesRates extends Component {
return (
<div className="taxes__taxes-taxjar-notice">
{ translate(
'Sales tax calculations are provided by a third party: TaxJar. By enabling this option, ' +
'TaxJar will have access to some of your data.'
'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.'
) }
<ExternalLink
icon
Expand All @@ -280,7 +276,7 @@ class TaxesRates extends Component {

render = () => {
const {
site,
siteId,
loadedSettingsGeneral,
loadedTaxRates,
onEnabledChange,
Expand All @@ -289,7 +285,7 @@ class TaxesRates extends Component {
} = this.props;

if ( ! loadedSettingsGeneral ) {
return <QuerySettingsGeneral siteId={ site && site.ID } />;
return <QuerySettingsGeneral siteId={ siteId } />;
}

if ( ! loadedTaxRates ) {
Expand Down Expand Up @@ -317,15 +313,11 @@ class TaxesRates extends Component {
}

function mapStateToProps( state, ownProps ) {
let siteId = undefined;
if ( ownProps.site ) {
siteId = ownProps.site.ID;
}
const address = getStoreLocation( state, siteId );
const loadedSettingsGeneral = areSettingsGeneralLoaded( state, siteId );
const areTaxesEnabled = areTaxCalculationsEnabled( state, siteId );
const loadedTaxRates = areTaxRatesLoaded( state, siteId );
const taxRates = getTaxRates( state, siteId );
const address = getStoreLocation( state, ownProps.siteId );
const loadedSettingsGeneral = areSettingsGeneralLoaded( state, ownProps.siteId );
const areTaxesEnabled = areTaxCalculationsEnabled( state, ownProps.siteId );
const loadedTaxRates = areTaxRatesLoaded( state, ownProps.siteId );
const taxRates = getTaxRates( state, ownProps.siteId );

return {
address,
Expand Down
Loading