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

Use shared dispute utility function isRefundable for disputed order notice to improve code quality #7965

Merged
merged 14 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
5 changes: 5 additions & 0 deletions changelog/dev-refactor-order-dispute-refundable-util
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: dev
Comment: Not user-facing: refactor's refund eligibility logic for disputed orders


4 changes: 4 additions & 0 deletions changelog/fix-7960-transaction-refund-eligible-disputes-only
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fix

Hide the transaction details refund menu for ineligble disputed transactions
10 changes: 3 additions & 7 deletions client/components/disputed-order-notice/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { getDetailsURL } from 'wcpay/components/details-link';
import {
isAwaitingResponse,
isInquiry,
isRefundable,
isUnderReview,
} from 'wcpay/disputes/utils';
import { useCharge } from 'wcpay/data';
Expand All @@ -30,10 +31,7 @@ const DisputedOrderNoticeHandler = ( { chargeId, onDisableOrderRefund } ) => {
if ( ! charge?.dispute ) {
return;
}
// Refunds are only allowed if the dispute is an inquiry or if it's won.
const isRefundable =
isInquiry( dispute ) || [ 'won' ].includes( dispute.status );
if ( ! isRefundable ) {
if ( ! isRefundable( dispute.status ) ) {
onDisableOrderRefund( dispute.status );
}
}, [ charge, onDisableOrderRefund ] );
Expand All @@ -42,8 +40,6 @@ const DisputedOrderNoticeHandler = ( { chargeId, onDisableOrderRefund } ) => {
if ( ! charge?.dispute ) {
return null;
}
const isRefundable =
isInquiry( dispute ) || [ 'won' ].includes( dispute.status );

// Special case the dispute "under review" notice which is much simpler.
// (And return early.)
Expand All @@ -66,7 +62,7 @@ const DisputedOrderNoticeHandler = ( { chargeId, onDisableOrderRefund } ) => {
// This may be dead code. Leaving in for now as this is consistent with
// the logic before this PR.
// https://github.com/Automattic/woocommerce-payments/pull/7557
if ( dispute.status === 'lost' && ! isRefundable ) {
Copy link
Member Author

@Jinksi Jinksi Jan 3, 2024

Choose a reason for hiding this comment

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

isRefundable is redundant here since isRefundable('lost') will always be false if dispute.status === 'lost'.

if ( dispute.status === 'lost' ) {
return (
<DisputeOrderLockedNotice
message={ __(
Expand Down
5 changes: 5 additions & 0 deletions client/disputes/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ export const isInquiry = ( dispute: Pick< Dispute, 'status' > ): boolean => {
return dispute.status.startsWith( 'warning' );
};

export const isRefundable = ( status: DisputeStatus ): boolean => {
// Refundable dispute statuses are one of `warning_needs_response`, `warning_under_review`, `warning_closed` or `won`.
return isInquiry( { status } ) || 'won' === status;
};

/**
* Returns the dispute fee balance transaction for a dispute if it exists
* and the deduction has not been reversed.
Expand Down
11 changes: 10 additions & 1 deletion client/payment-details/summary/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import DisputeStatusChip from 'components/dispute-status-chip';
import {
getDisputeFeeFormatted,
isAwaitingResponse,
isRefundable,
} from 'wcpay/disputes/utils';
import { useAuthorization } from 'wcpay/data';
import CaptureAuthorizationButton from 'wcpay/components/capture-authorization-button';
Expand Down Expand Up @@ -208,6 +209,14 @@ const PaymentDetailsSummary: React.FC< PaymentDetailsSummaryProps > = ( {
const disputeFee =
charge.dispute && getDisputeFeeFormatted( charge.dispute );

// If this transaction is disputed, check if it is refundable. If not, we should hide the refund menu.
const isDisputeRefundable = charge.dispute
? isRefundable( charge.dispute.status )
: true;

const showRefundMenu =
charge.captured && ! charge.refunded && isDisputeRefundable;

// Use the balance_transaction fee if available. If not (e.g. authorized but not captured), use the application_fee_amount.
const transactionFee = charge.balance_transaction
? {
Expand Down Expand Up @@ -484,7 +493,7 @@ const PaymentDetailsSummary: React.FC< PaymentDetailsSummaryProps > = ( {
</div>
</div>
<div className="payment-details__refund-controls">
{ ! charge?.refunded && charge?.captured && (
{ showRefundMenu && (
<Loadable
isLoading={ isLoading }
placeholder={ moreVertical }
Expand Down
48 changes: 48 additions & 0 deletions client/payment-details/summary/test/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,13 @@ describe( 'PaymentDetailsSummary', () => {
screen.getByRole( 'button', {
name: /Accept dispute/,
} );

// Refund menu is not rendered
expect(
screen.queryByRole( 'button', {
name: /Translation actions/i,
} )
).toBeNull();
} );

test( 'renders the information of a disputed charge when the store/charge currency differ', () => {
Expand Down Expand Up @@ -684,6 +691,11 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is rendered
screen.getByRole( 'button', {
name: /Translation actions/i,
} );
} );

test( 'correctly renders dispute details for "under_review" disputes', () => {
Expand Down Expand Up @@ -712,6 +724,13 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is not rendered
expect(
screen.queryByRole( 'button', {
name: /Translation actions/i,
} )
).toBeNull();
} );

test( 'correctly renders dispute details for "accepted" disputes', () => {
Expand Down Expand Up @@ -744,6 +763,13 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is not rendered
expect(
screen.queryByRole( 'button', {
name: /Translation actions/i,
} )
).toBeNull();
} );

test( 'correctly renders dispute details for "lost" disputes', () => {
Expand Down Expand Up @@ -777,6 +803,13 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is not rendered
expect(
screen.queryByRole( 'button', {
name: /Translation actions/i,
} )
).toBeNull();
} );

test( 'correctly renders dispute details for "warning_needs_response" inquiry disputes', () => {
Expand Down Expand Up @@ -807,6 +840,11 @@ describe( 'PaymentDetailsSummary', () => {
screen.getByRole( 'button', {
name: /Issue refund/i,
} );

// Refund menu is rendered
screen.getByRole( 'button', {
name: /Translation actions/i,
} );
} );

test( 'correctly renders dispute details for "warning_under_review" inquiry disputes', () => {
Expand Down Expand Up @@ -834,6 +872,11 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is rendered
screen.getByRole( 'button', {
name: /Translation actions/i,
} );
} );

test( 'correctly renders dispute details for "warning_closed" inquiry disputes', () => {
Expand Down Expand Up @@ -862,6 +905,11 @@ describe( 'PaymentDetailsSummary', () => {
name: /Accept/i,
} )
).toBeNull();

// Refund menu is rendered
screen.getByRole( 'button', {
name: /Translation actions/i,
} );
} );

describe( 'order missing notice', () => {
Expand Down
Loading