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

Settings - Display erroring bank notice on Settings > Payments > Deposits #9482

Merged
merged 9 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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: 4 additions & 0 deletions changelog/fix-8331-bank-error-notice-settings-deposit
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: add

Show a notice in Payments > Settings > Deposits if there is an error with the bank account.
71 changes: 45 additions & 26 deletions client/settings/deposits/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import { select } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import { Card, SelectControl, ExternalLink } from '@wordpress/components';
import interpolateComponents from '@automattic/interpolate-components';
import { STORE_NAME } from 'wcpay/data/constants';

/**
* Internal dependencies
*/
import { STORE_NAME } from 'wcpay/data/constants';
import { getDepositMonthlyAnchorLabel } from 'wcpay/deposits/utils';
import WCPaySettingsContext from '../wcpay-settings-context';
import CardBody from '../card-body';
Expand All @@ -25,6 +25,8 @@ import {
import './style.scss';
import { recordEvent } from 'tracks';
import InlineNotice from 'components/inline-notice';
import { DepositFailureNotice } from 'components/deposits-overview/deposit-notices';
import { useSelectedCurrencyOverview } from 'wcpay/overview/hooks';

const daysOfWeek = [
{ label: __( 'Monday', 'woocommerce-payments' ), value: 'monday' },
Expand Down Expand Up @@ -208,6 +210,17 @@ const Deposits = () => {
accountStatus: { accountLink },
} = useContext( WCPaySettingsContext );

const { account, overview } = useSelectedCurrencyOverview();
const selectedCurrency =
overview?.currency || wcpaySettings.accountDefaultCurrency;

const hasErroredExternalAccount =
account?.default_external_accounts?.some(
( externalAccount ) =>
externalAccount.currency === selectedCurrency &&
externalAccount.status === 'errored'
) ?? false;
Copy link
Member

@Jinksi Jinksi Sep 26, 2024

Choose a reason for hiding this comment

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

💭 I found that when I used a multi-deposit-currency store, this notice did not display, and there is no way to see to change which currency is selected on the settings page.

What do you think about making this display if any deposit currency's default deposit account is in an errored state?

Suggested change
const { account, overview } = useSelectedCurrencyOverview();
const selectedCurrency =
overview?.currency || wcpaySettings.accountDefaultCurrency;
const hasErroredExternalAccount =
account?.default_external_accounts?.some(
( externalAccount ) =>
externalAccount.currency === selectedCurrency &&
externalAccount.status === 'errored'
) ?? false;
const { overviews } = useAllDepositsOverviews();
const hasErroredExternalAccount =
overviews.account?.default_external_accounts?.some(
( externalAccount ) => externalAccount.status === 'errored'
) ?? false;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about making this display if any deposit currency's default deposit account is in an errored state?

Thanks for testing this with a multi-currency account! what you shared here makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have incorporated the feedback here - e6afbda

Also updated the tests to include multi-currency default accounts scenario - 4e8a2ba

Thanks once again, for the valuable review, Eric 🙌🏼

Copy link
Contributor Author

@nagpai nagpai Sep 26, 2024

Choose a reason for hiding this comment

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

I've tested this with a multi-deposit-currency store with a single errored deposit destination.

I did a round of manual test this time with a similar account, and it seems to work as expected.


return (
<Card className="deposits">
<CardBody>
Expand All @@ -219,31 +232,37 @@ const Deposits = () => {
<h4>
{ __( 'Deposit bank account', 'woocommerce-payments' ) }
</h4>
<p className="deposits__bank-information-help">
{ __(
'Manage and update your deposit account information to receive payments and deposits.',
'woocommerce-payments'
) }{ ' ' }
{ accountLink && (
<ExternalLink
href={ accountLink }
onClick={ () => {
recordEvent(
'wcpay_settings_deposits_manage_in_stripe_click'
);
recordEvent(
'wcpay_account_details_link_clicked',
{ source: 'settings-deposits' }
);
} }
>
{ __(
'Manage in Stripe',
'woocommerce-payments'
) }
</ExternalLink>
) }
</p>
{ hasErroredExternalAccount ? (
<DepositFailureNotice
updateAccountLink={ accountLink }
/>
) : (
<p className="deposits__bank-information-help">
{ __(
'Manage and update your deposit account information to receive payments and deposits.',
'woocommerce-payments'
) }{ ' ' }
{ accountLink && (
<ExternalLink
href={ accountLink }
onClick={ () => {
recordEvent(
'wcpay_settings_deposits_manage_in_stripe_click'
);
recordEvent(
'wcpay_account_details_link_clicked',
{ source: 'settings-deposits' }
);
} }
>
{ __(
'Manage in Stripe',
'woocommerce-payments'
) }
</ExternalLink>
) }
</p>
) }
</div>
</CardBody>
</Card>
Expand Down
89 changes: 75 additions & 14 deletions client/settings/deposits/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,21 @@ import {
useDepositScheduleMonthlyAnchor,
} from 'wcpay/data';

jest.mock( '@wordpress/data' );
import { useSelectedCurrencyOverview } from 'wcpay/overview/hooks';

jest.mock( '@wordpress/data', () => ( {
createRegistryControl: jest.fn(),
dispatch: jest.fn( () => ( {
setIsMatching: jest.fn(),
onLoad: jest.fn(), // Add this line
onHistoryChange: jest.fn(),
} ) ),
registerStore: jest.fn(),
select: jest.fn(),
useDispatch: jest.fn( () => ( { createNotice: jest.fn() } ) ),
withDispatch: jest.fn( () => jest.fn() ),
withSelect: jest.fn( () => jest.fn() ),
} ) );

jest.mock( 'wcpay/data', () => ( {
useAccountStatementDescriptor: jest.fn(),
Expand All @@ -34,6 +48,10 @@ jest.mock( 'wcpay/data', () => ( {
useDepositScheduleMonthlyAnchor: jest.fn(),
} ) );

jest.mock( 'wcpay/overview/hooks', () => ( {
useSelectedCurrencyOverview: jest.fn(),
} ) );

describe( 'Deposits', () => {
const settingsContext = {
accountStatus: { accountLink: '/account-link' },
Expand All @@ -54,6 +72,14 @@ describe( 'Deposits', () => {
account_country: 'US',
} ),
} ) );
useSelectedCurrencyOverview.mockReturnValue( {
account: {
default_external_accounts: [
{ currency: 'USD', status: 'enabled' },
],
},
overview: { currency: 'USD' },
} );
} );

it( 'renders', () => {
Expand Down Expand Up @@ -236,27 +262,62 @@ describe( 'Deposits', () => {
}
} );

it( 'renders the monthly offset select', () => {
useDepositScheduleInterval.mockReturnValue( [ 'monthly', jest.fn() ] );
useDepositScheduleMonthlyAnchor.mockReturnValue( [ 14, jest.fn() ] );
it( 'renders the deposit failure notice if there is an errored bank account', () => {
useSelectedCurrencyOverview.mockReturnValue( {
account: {
default_external_accounts: [
{ currency: 'USD', status: 'errored' },
],
},
overview: { currency: 'USD' },
} );

render(
<WCPaySettingsContext.Provider value={ settingsContext }>
<Deposits />
</WCPaySettingsContext.Provider>
);

const frequencySelect = screen.getByLabelText( /Frequency/ );
expect( frequencySelect ).toHaveValue( 'monthly' );
const depositsMessage = screen.getByText(
/Deposits are currently paused because a recent deposit failed./,
{
ignore: '.a11y-speak-region',
}
);
expect( depositsMessage ).toBeInTheDocument();

const monthlyAnchorSelect = screen.getByLabelText( /Date/ );
expect( monthlyAnchorSelect ).toHaveValue( '14' );
expect(
screen.queryByText(
/Manage and update your deposit account information to receive payments and deposits./,
{
ignore: '.a11y-speak-region',
}
)
).toBeFalsy();
} );

const monthlyAnchors = [ /^1st/i, /^28th/i, /Last day of the month/i ];
for ( const anchor of monthlyAnchors ) {
within( monthlyAnchorSelect ).getByRole( 'option', {
name: anchor,
} );
}
it( 'does not render the deposit failure notice if there is no errored bank account', () => {
render(
<WCPaySettingsContext.Provider value={ settingsContext }>
<Deposits />
</WCPaySettingsContext.Provider>
);

expect(
screen.queryByText(
/Deposits are currently paused because a recent deposit failed./,
{
ignore: '.a11y-speak-region',
}
)
).toBeFalsy();

const depositsMessage = screen.getByText(
/Manage and update your deposit account information to receive payments and deposits./,
{
ignore: '.a11y-speak-region',
}
);
expect( depositsMessage ).toBeInTheDocument();
} );
} );
Loading