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

Data Stores: Refactor partner portal credit card store to createReduxStore() - take 2 #75377

Merged
merged 3 commits into from
Apr 7, 2023
Merged

Data Stores: Refactor partner portal credit card store to createReduxStore() - take 2 #75377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be27883
Revert "Revert "Data Stores: Refactor partner portal credit card stor…
tyxla Apr 6, 2023
e46fe2a
Access store directly, rely on type inference
tyxla Apr 6, 2023
c2fa07a
Remove unnecessary mocks to fix tests
tyxla Apr 6, 2023
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
4 changes: 2 additions & 2 deletions ...nt/jetpack-cloud/sections/partner-portal/credit-card-fields/credit-card-element-field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { CardElement } from '@stripe/react-stripe-js';
import { useSelect } from '@wordpress/data';
import { useI18n } from '@wordpress/react-i18n';
import classnames from 'classnames';
import { creditCardStore } from 'calypso/state/partner-portal/credit-card-form';
import type { StripeElementChangeEvent, StripeElementStyle } from '@stripe/stripe-js';
import type { CreditCardSelectors } from 'calypso/state/partner-portal/types';

export default function CreditCardElementField( {
setIsStripeFullyLoaded,
Expand All @@ -19,7 +19,7 @@ export default function CreditCardElementField( {
const { formStatus } = useFormStatus();
const isDisabled = formStatus !== FormStatus.READY;
const { card: cardError } = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).getCardDataErrors(),
( select ) => select( creditCardStore ).getCardDataErrors(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing special about most of the changed instances - we reference the store through the existing store object instead of its name, which is the recommended way in Gutenberg nowadays.

[]
);

Expand Down
116 changes: 55 additions & 61 deletions ...nt/jetpack-cloud/sections/partner-portal/credit-card-fields/credit-card-submit-button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,41 +1,40 @@
import { Button, FormStatus, useFormStatus } from '@automattic/composite-checkout';
import { useElements, CardElement } from '@stripe/react-stripe-js';
import { useSelect } from '@wordpress/data';
import { useDispatch, useSelect } from '@wordpress/data';
import { useI18n } from '@wordpress/react-i18n';
import debugFactory from 'debug';
import { useMemo } from 'react';
import { creditCardStore } from 'calypso/state/partner-portal/credit-card-form';
import type { StripeConfiguration } from '@automattic/calypso-stripe';
import type { ProcessPayment } from '@automattic/composite-checkout';
import type { StoreState } from '@automattic/wpcom-checkout';
import type { Stripe } from '@stripe/stripe-js';
import type { I18n } from '@wordpress/i18n';
import type { State } from 'calypso/state/partner-portal/credit-card-form/reducer';
import type { CreditCardSelectors } from 'calypso/state/partner-portal/types';

const debug = debugFactory( 'calypso:partner-portal:credit-card' );

export default function CreditCardSubmitButton( {
disabled,
onClick,
store,
stripe,
stripeConfiguration,
activeButtonText,
}: {
disabled?: boolean;
onClick?: ProcessPayment;
store: State;
stripe: Stripe | null;
stripeConfiguration: StripeConfiguration | null;
activeButtonText: string | undefined;
} ) {
const { __ } = useI18n();
const fields: StoreState< string > = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).getFields(),
[]
);
const useAsPrimaryPaymentMethod: boolean = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).useAsPrimaryPaymentMethod(),
const { fields, useAsPrimaryPaymentMethod, errors, incompleteFieldKeys } = useSelect(
( select ) => {
const store = select( creditCardStore );
return {
fields: store.getFields(),
useAsPrimaryPaymentMethod: store.useAsPrimaryPaymentMethod(),
errors: store.getCardDataErrors(),
incompleteFieldKeys: store.getIncompleteFieldKeys(),
};
},
Comment on lines +28 to +37
Copy link
Member Author

Choose a reason for hiding this comment

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

This contains 2 different sets of changes:

  • Combining all creditCardStore selectors into one useSelect() for brevity and predictability.
  • Removing some unnecessary types and relying on type inference instead.

[]
);
const cardholderName = fields.cardholderName;
Expand All @@ -54,30 +53,53 @@ export default function CreditCardSubmitButton( {
return __( 'Please wait…' );
}, [ formStatus, activeButtonText, __ ] );

const { setCardDataError, setFieldValue, setFieldError } = useDispatch( creditCardStore );
Copy link
Member Author

Choose a reason for hiding this comment

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

We're using useDispatch() now to get mapped action creators.


const handleButtonClick = () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

We're inlining this function to be able to use the data from the hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. This is way neater now.

debug( 'validating credit card fields' );
Copy link
Member Author

Choose a reason for hiding this comment

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

We're also inlining the credit card validation function here, that way we can take advantage of the existing store values without having to pass the entire store object. We could keep the function separate, but then we'd still need to pass all the various state data and mapped action creators, which is an extra step of unnecessary complication IMHO.


if ( ! cardholderName?.value.length ) {
// Touch the field so it displays a validation error
setFieldValue( 'cardholderName', '' );
Copy link
Member Author

Choose a reason for hiding this comment

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

See how natural it is to dispatch actions that way.

setFieldError( 'cardholderName', __( 'This field is required' ) );
}
const areThereErrors = Object.keys( errors ).some( ( errorKey ) => errors[ errorKey ] );

if ( incompleteFieldKeys.length > 0 ) {
// Show "this field is required" for each incomplete field
incompleteFieldKeys.map( ( key: string ) =>
setCardDataError( key, __( 'This field is required' ) )
);
}

if ( areThereErrors || ! cardholderName?.value.length || incompleteFieldKeys.length > 0 ) {
// credit card is invalid
return false;
}

debug( 'submitting stripe payment' );

if ( ! onClick ) {
throw new Error(
'Missing onClick prop; CreditCardSubmitButton must be used as a payment button in CheckoutSubmitButton'
);
}

onClick( {
stripe,
name: cardholderName?.value,
stripeConfiguration,
cardElement,
useAsPrimaryPaymentMethod,
} );
return;
};

return (
<Button
className={ ! formSubmitting ? 'button is-primary' : '' }
disabled={ disabled }
onClick={ () => {
if ( isCreditCardFormValid( store, __ ) ) {
debug( 'submitting stripe payment' );

if ( ! onClick ) {
throw new Error(
'Missing onClick prop; CreditCardSubmitButton must be used as a payment button in CheckoutSubmitButton'
);
}

onClick( {
stripe,
name: cardholderName?.value,
stripeConfiguration,
cardElement,
useAsPrimaryPaymentMethod,
} );
return;
}
} }
onClick={ handleButtonClick }
buttonType="primary"
isBusy={ formSubmitting }
fullWidth
Expand All @@ -86,31 +108,3 @@ export default function CreditCardSubmitButton( {
</Button>
);
}

function isCreditCardFormValid( store: State, __: I18n[ '__' ] ) {
debug( 'validating credit card fields' );

const fields = store.selectors.getFields( store.getState() );
const cardholderName = fields.cardholderName;
if ( ! cardholderName?.value.length ) {
// Touch the field so it displays a validation error
store.dispatch( store.actions.setFieldValue( 'cardholderName', '' ) );
store.dispatch(
store.actions.setFieldError( 'cardholderName', __( 'This field is required' ) )
);
}
const errors = store.selectors.getCardDataErrors( store.getState() );
const incompleteFieldKeys = store.selectors.getIncompleteFieldKeys( store.getState() );
const areThereErrors = Object.keys( errors ).some( ( errorKey ) => errors[ errorKey ] );

if ( incompleteFieldKeys.length > 0 ) {
// Show "this field is required" for each incomplete field
incompleteFieldKeys.map( ( key: string ) =>
store.dispatch( store.actions.setCardDataError( key, __( 'This field is required' ) ) )
);
}
if ( areThereErrors || ! cardholderName?.value.length || incompleteFieldKeys.length > 0 ) {
return false;
}
return true;
}
6 changes: 3 additions & 3 deletions client/jetpack-cloud/sections/partner-portal/credit-card-fields/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@ import { useState } from 'react';
import { useDispatch as useReduxDispatch } from 'react-redux';
import { useRecentPaymentMethodsQuery } from 'calypso/jetpack-cloud/sections/partner-portal/hooks';
import { recordTracksEvent } from 'calypso/state/analytics/actions';
import { creditCardStore } from 'calypso/state/partner-portal/credit-card-form';
import CreditCardElementField from './credit-card-element-field';
import CreditCardLoading from './credit-card-loading';
import SetAsPrimaryPaymentMethod from './set-as-primary-payment-method';
import type { StoreState } from '@automattic/wpcom-checkout';
import type { StripeElementChangeEvent, StripeElementStyle } from '@stripe/stripe-js';
import type { CreditCardSelectors } from 'calypso/state/partner-portal/types';
import './style.scss';

export default function CreditCardFields() {
const { __ } = useI18n();
const [ isStripeFullyLoaded, setIsStripeFullyLoaded ] = useState( false );
const fields: StoreState< string > = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).getFields(),
( select ) => select( creditCardStore ).getFields(),
[]
);
const useAsPrimaryPaymentMethod: boolean = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).useAsPrimaryPaymentMethod(),
( select ) => select( creditCardStore ).useAsPrimaryPaymentMethod(),
[]
);
const getField = ( key: string | number ) => fields[ key ] || {};
Expand Down
7 changes: 0 additions & 7 deletions ...tpack-cloud/sections/partner-portal/credit-card-fields/test/credit-card-submit-button.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,16 @@ import { Elements } from '@stripe/react-stripe-js';
import { loadStripe } from '@stripe/stripe-js';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { useSelect } from '@wordpress/data';
import { Provider } from 'react-redux';
import configureStore from 'redux-mock-store';
import * as actions from 'calypso/state/partner-portal/credit-card-form/actions';
import * as selectors from 'calypso/state/partner-portal/credit-card-form/selectors';
import CreditCardSubmitButton from '../credit-card-submit-button';

const mockUseSelector = () => () => null;

jest.mock( '@stripe/stripe-js', () => ( {
loadStripe: () => null,
} ) );

jest.mock( '@wordpress/data' );
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need all those mocks; we can just let @wordpress/data do its job.

jest.mock( 'calypso/state/partner-portal/credit-card-form/selectors', () => {
const items = jest.requireActual( 'calypso/state/partner-portal/credit-card-form/selectors' );
return {
Expand Down Expand Up @@ -71,7 +67,6 @@ describe( '<CreditCardSubmitButton>', () => {
beforeEach( () => {
// Re-mock dependencies
jest.clearAllMocks();
useSelect.mockImplementation( mockUseSelector );
useFormStatus.mockImplementation( () => {
return {
formStatus: 'ready',
Expand All @@ -81,7 +76,6 @@ describe( '<CreditCardSubmitButton>', () => {
} );

afterEach( () => {
useSelect.mockClear();
useFormStatus.mockClear();
} );

Expand All @@ -106,7 +100,6 @@ describe( '<CreditCardSubmitButton>', () => {
const buttonText = 'Save payment method';

const props = {
store: newStore,
stripe,
stripeConfiguration,
disabled: false,
Expand Down
6 changes: 1 addition & 5 deletions ...pack-cloud/sections/partner-portal/payment-methods/hooks/use-create-stored-credit-card.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useMemo } from 'react';
import { createStoredCreditCardMethod } from 'calypso/jetpack-cloud/sections/partner-portal/payment-methods/stored-credit-card-method';
import { createStoredCreditCardPaymentMethodStore } from 'calypso/state/partner-portal/credit-card-form';
import type { StripeConfiguration, StripeLoadingError } from '@automattic/calypso-stripe';
import type { PaymentMethod } from '@automattic/composite-checkout';
import type { Stripe } from '@stripe/stripe-js';
Expand All @@ -20,18 +19,15 @@ export function useCreateStoredCreditCardMethod( {
} ): PaymentMethod | null {
const shouldLoadStripeMethod = ! isStripeLoading && ! stripeLoadingError;

const store = useMemo( () => createStoredCreditCardPaymentMethodStore(), [] );

return useMemo(
() =>
shouldLoadStripeMethod
? createStoredCreditCardMethod( {
store,
stripe,
stripeConfiguration,
activePayButtonText,
} )
: null,
[ shouldLoadStripeMethod, store, stripe, stripeConfiguration, activePayButtonText ]
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer need the store since we reference it directly in the component.

[ shouldLoadStripeMethod, stripe, stripeConfiguration, activePayButtonText ]
);
}
4 changes: 0 additions & 4 deletions client/jetpack-cloud/sections/partner-portal/payment-methods/stored-credit-card-method.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,12 @@ import CreditCardSubmitButton from 'calypso/jetpack-cloud/sections/partner-porta
import type { StripeConfiguration } from '@automattic/calypso-stripe';
import type { PaymentMethod } from '@automattic/composite-checkout';
import type { Stripe } from '@stripe/stripe-js';
import type { State } from 'calypso/state/partner-portal/credit-card-form/reducer';

export function createStoredCreditCardMethod( {
store,
stripe,
stripeConfiguration,
activePayButtonText = undefined,
}: {
store: State;
stripe: Stripe | null;
stripeConfiguration: StripeConfiguration | null;
activePayButtonText?: string | undefined;
Expand All @@ -23,7 +20,6 @@ export function createStoredCreditCardMethod( {
activeContent: <CreditCardFields />,
submitButton: (
<CreditCardSubmitButton
store={ store }
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer need the store since we reference it directly in the component.

stripe={ stripe }
stripeConfiguration={ stripeConfiguration }
activeButtonText={ activePayButtonText }
Expand Down
4 changes: 2 additions & 2 deletions client/jetpack-cloud/sections/partner-portal/primary/payment-method-add/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,11 @@ import { addQueryArgs } from 'calypso/lib/url';
import { recordTracksEvent } from 'calypso/state/analytics/actions';
import { getCurrentUserLocale } from 'calypso/state/current-user/selectors';
import { errorNotice, removeNotice, successNotice } from 'calypso/state/notices/actions';
import { creditCardStore } from 'calypso/state/partner-portal/credit-card-form';
import { doesPartnerRequireAPaymentMethod } from 'calypso/state/partner-portal/partner/selectors';
import { fetchStoredCards } from 'calypso/state/partner-portal/stored-cards/actions';
import getSites from 'calypso/state/selectors/get-sites';
import type { SiteDetails } from '@automattic/data-stores';
import type { CreditCardSelectors } from 'calypso/state/partner-portal/types';

import './style.scss';

Expand All @@ -63,7 +63,7 @@ function PaymentMethodAdd( { selectedSite }: { selectedSite?: SiteDetails | null
[ stripeMethod ]
);
const useAsPrimaryPaymentMethod: boolean = useSelect(
( select ) => ( select( 'credit-card' ) as CreditCardSelectors ).useAsPrimaryPaymentMethod(),
( select ) => select( creditCardStore ).useAsPrimaryPaymentMethod(),
[]
);

Expand Down
16 changes: 7 additions & 9 deletions client/state/partner-portal/credit-card-form/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import { registerStore } from '@wordpress/data';
import { createReduxStore, register } from '@wordpress/data';
import * as actions from 'calypso/state/partner-portal/credit-card-form/actions';
import reducer from 'calypso/state/partner-portal/credit-card-form/reducer';
import * as selectors from 'calypso/state/partner-portal/credit-card-form/selectors';

export function createStoredCreditCardPaymentMethodStore(): Record< string, unknown > {
const store = registerStore( 'credit-card', {
reducer,
actions,
selectors,
} );
export const creditCardStore = createReduxStore( 'credit-card', {
reducer,
actions,
selectors,
} );

return { ...store, actions, selectors };
}
register( creditCardStore );
4 changes: 0 additions & 4 deletions client/state/partner-portal/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ import {
LicenseSortDirection,
LicenseSortField,
} from 'calypso/jetpack-cloud/sections/partner-portal/types';
import * as creditCardSelectors from './credit-card-form/selectors';
import type { SelectFromMap } from '@automattic/data-stores';

export type CreditCardSelectors = SelectFromMap< typeof creditCardSelectors >;

/**
* Utility.
Expand Down