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

Disputes: Allow merchant to respond to inquiries from transaction detail page #7298

Merged
merged 13 commits into from
Oct 4, 2023
Merged

Disputes: Allow merchant to respond to inquiries from transaction detail page #7298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0f6c3d7
Add CTA actions for Inquiries
brucealdridge Sep 27, 2023
20889d6
Merge branch 'develop' into add/7193-dispute-cta-for-inquries
brucealdridge Sep 27, 2023
9d5edad
Merge branch 'develop' into add/7193-dispute-cta-for-inquries
haszari Sep 27, 2023
5d45406
Merge branch 'develop' into add/7193-dispute-cta-for-inquries
Jinksi Sep 27, 2023
7f24785
Fix whitespace character JS linter error
Jinksi Sep 27, 2023
8a6166c
Mock @wordpress/data to fix broken tests
Jinksi Sep 28, 2023
24d86f5
Add test for rendering inquiries awaiting response
Jinksi Sep 28, 2023
a6ae0f3
Applying PR feedback, attempting to make code more readable
brucealdridge Sep 29, 2023
e5e2c42
Merge branch 'develop' into add/7193-dispute-cta-for-inquries
Jinksi Oct 2, 2023
4556951
Remove unused icon import
Jinksi Oct 3, 2023
b3c605c
Reorder inqury/dispute return to improve readability
Jinksi Oct 3, 2023
5cb3447
Update values of event names DISPUTE_INQUIRY_REFUND_*
Jinksi Oct 3, 2023
f33745d
Merge branch 'develop' into add/7193-dispute-cta-for-inquries
Jinksi Oct 4, 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
5 changes: 5 additions & 0 deletions changelog/add-7193-dispute-cta-for-inquries
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: dev
Comment: Add CTA for Inquiries, behind a feature flag.


224 changes: 167 additions & 57 deletions client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
import React, { useState } from 'react';
import moment from 'moment';
import { __, sprintf } from '@wordpress/i18n';
import { backup, edit, lock } from '@wordpress/icons';
import { backup, edit, lock, arrowRight } from '@wordpress/icons';
import { useDispatch } from '@wordpress/data';
import { createInterpolateElement } from '@wordpress/element';
import { Link } from '@woocommerce/components';
import {
Expand Down Expand Up @@ -39,12 +40,121 @@ interface Props {
dispute: Dispute;
customer: ChargeBillingDetails | null;
chargeCreated: number;
orderUrl: string | undefined;
}

/**
* The lines of text to display in the modal to confirm acceptance / refunding of the dispute / inquiry.
*/
interface ModalLineItem {
icon: JSX.Element;
description: string | JSX.Element;
}

interface AcceptDisputeProps {
/**
* The label for the button that opens the modal.
*/
acceptButtonLabel: string;
/**
* The event to track when the button that opens the modal is clicked.
*/
acceptButtonTracksEvent: string;
/**
* The title of the modal.
*/
modalTitle: string;
/**
* The lines of text to display in the modal.
*/
modalLines: ModalLineItem[];
/**
* The label for the primary button in the modal to Accept / Refund the dispute / inquiry.
*/
modalButtonLabel: string;
/**
* The event to track when the primary button in the modal is clicked.
*/
modalButtonTracksEvent: string;
}

/**
* Disputes and Inquiries have different text for buttons and the modal.
* They also have different icons and tracks events. This function returns the correct props.
*
* @param dispute
*/
function getAcceptDisputeProps( dispute: Dispute ): AcceptDisputeProps {
if ( isInquiry( dispute ) ) {
return {
acceptButtonLabel: __( 'Issue refund', 'woocommerce-payments' ),
acceptButtonTracksEvent:
wcpayTracks.events.DISPUTE_INQUIRY_REFUND_MODAL_VIEW,
modalTitle: __( 'Issue a refund?', 'woocommerce-payments' ),
modalLines: [
{
icon: <Icon icon={ backup } size={ 24 } />,
description: __(
'Issuing a refund will close the inquiry, returning the amount in question back to the cardholder. No additional fees apply.',
'woocommerce-payments'
),
},
{
icon: <Icon icon={ arrowRight } size={ 24 } />,
description: __(
'You will be taken to the order, where you must complete the refund process manually.',
'woocommerce-payments'
),
},
],
modalButtonLabel: __(
'View order to issue refund',
'woocommerce-payments'
),
modalButtonTracksEvent:
wcpayTracks.events.DISPUTE_INQUIRY_REFUND_CLICK,
};
}

return {
acceptButtonLabel: __( 'Accept dispute', 'woocommerce-payments' ),
acceptButtonTracksEvent: wcpayTracks.events.DISPUTE_ACCEPT_MODAL_VIEW,
modalTitle: __( 'Accept the dispute?', 'woocommerce-payments' ),
modalLines: [
{
icon: <Icon icon={ backup } size={ 24 } />,
description: createInterpolateElement(
sprintf(
/* translators: %s: dispute fee, <em>: emphasis HTML element. */
__(
'Accepting the dispute marks it as <em>Lost</em>. The disputed amount will be returned to the cardholder, with a %s dispute fee deducted from your account.',
'woocommerce-payments'
),
getDisputeFeeFormatted( dispute, true ) ?? '-'
),
{
em: <em />,
}
),
},
{
icon: <Icon icon={ lock } size={ 24 } />,
description: __(
'This action is final and cannot be undone.',
'woocommerce-payments'
),
},
],
modalButtonLabel: __( 'Accept dispute', 'woocommerce-payments' ),
modalButtonTracksEvent: wcpayTracks.events.DISPUTE_ACCEPT_CLICK,
};
}
brucealdridge marked this conversation as resolved.
Show resolved Hide resolved

const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
dispute,
customer,
chargeCreated,
orderUrl,
} ) => {
const { doAccept, isLoading } = useDisputeAccept( dispute );
const [ isModalOpen, setModalOpen ] = useState( false );
Expand All @@ -53,13 +163,34 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
const dueBy = moment.unix( dispute.evidence_details?.due_by ?? 0 );
const countdownDays = Math.floor( dueBy.diff( now, 'days', true ) );
const hasStagedEvidence = dispute.evidence_details?.has_evidence;
const { createErrorNotice } = useDispatch( 'core/notices' );
// This is a temporary restriction and can be removed once steps and actions for inquiries are implemented.
const showDisputeStepsAndActions = ! isInquiry( dispute );
const showDisputeSteps = ! isInquiry( dispute );

const onModalClose = () => {
setModalOpen( false );
};

const viewOrder = () => {
if ( orderUrl ) {
window.location.href = orderUrl;
return;
}

createErrorNotice(
__(
'Unable to view order. Order not found.',
'woocommerce-payments'
)
);
};

const disputeAcceptAction = getAcceptDisputeProps( dispute );
Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting off the key bits of content to another helper function is making this harder to understand IMO. This component is tightly coupled to getAcceptDisputeProps.

I don't fully understand the different conditions needed here (e.g. how many combinations are needed). It might be possible to simplify this code by having a single component with inline conditionals, or various components hard-coded for the common scenarios, and one conditional to choose which to render.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hunch… if we made a component for inquiries and a component for disputes, would that make this more readable and reduce the amount of data we need to pass down as props?

Copy link
Member

@Jinksi Jinksi Oct 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if we made a component for inquiries and a component for disputes

This was kind of the approach I explored in #7339.

I am tackling a similar problem with the inquiry steps to resolve PR (#7292) and I think separate components might help.

I don't think we should risk pushing back the project timeline for this, however. Can we ship this with 6.6 as-is and do a small tidy-up refactor once all of the code is in place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah totally, I don't want any of this to slow us down. We can always maintain, refactor, tidy in future :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing this again as I register tracks events. I really want to refactor this, AcceptDisputeProps is an abomination!

Copy link
Member

Choose a reason for hiding this comment

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

I started on this refactor in #7339, which may help or may not. I will close it if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! @Jinksi Do you recall where you got to / if this was feeling like positive improvement? Great to have work-in-progress to build on, though I'm happy for you to keep moving on it.

Copy link
Member

Choose a reason for hiding this comment

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

I wrote up further in this thread that I was unsure if it was an improvement. I'll have to revisit this with fresh eyes and see.

#7298 (comment)


const challengeButtonDefaultText = isInquiry( dispute )
? __( 'Submit evidence', 'woocommerce-payments' )
: __( 'Challenge dispute', 'woocommerce-payments' );

return (
<div className="transaction-details-dispute-details-wrapper">
<Card>
Expand All @@ -80,7 +211,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
dispute={ dispute }
daysRemaining={ countdownDays }
/>
{ showDisputeStepsAndActions && (
{ showDisputeSteps && (
<DisputeSteps
dispute={ dispute }
customer={ customer }
Expand All @@ -93,7 +224,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
/>

{ /* Dispute Actions */ }
{ showDisputeStepsAndActions && (
{
Copy link
Member

Choose a reason for hiding this comment

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

I find the nested isInquiry ternaries a bit hard to follow. This might benefit from separate components, e.g. DisputeActions & InquiryActions.

This is not a major concern, however.

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'm not sold on splitting the Dispute / Inquiry paths. But agree that it's not easily readable.

I'll have a go at breaking this up into something more manageable.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is quite subjective, so feel free to leave as-is!

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 tried to split it off but it was challenging given the isModalOpen, setModal and, onModalClose that had to be passed on.

I have opted instead for a function that returns the various properties rather than all the ternaries.

Copy link
Member

Choose a reason for hiding this comment

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

This PR works as intended, so I don't think my uncertainties below should hold up this PR being shipped. This is a tricky subject that I struggle to find the correct answers for (see a very similar discussion in #7047 (comment)).

I do think you've improved the readability of this code by the mapping of the labels etc – this more clearly separates what is for inquiries and what is for disputes.

However, I still find this entangled a bit and hard to follow. There are still some isInquiry ternaries outside of the modal, e.g. clicking issue refund/accept will do different things – inquiries probably wants to be a Button href={orderUrl} rather than Button onClick={window.location.orderUrl}.

I've explored another way to write this in #7339, the pseudocode TLDR is:

const ExistingConciseMarkup = () => {
	const actionLabelsEtc = isInquiry ? {
		label: 'Inquiry Label',
	} : {
		label: 'Dispute Label',
	};
	
	// Markup is defined once, but logic is not as easy to follow.
	return (
		<Actions>
			{actionLabelsEtc.label}
		</Actions>
	);
}

// Repeat markup to make it clear what is for inquiries and what is for disputes
const SimpleButRepetitiveMarkup = () => {
	// Markup is defined twice, but logic is straightforward.
	return isInquiry ? (
		<Actions>Inquiry Label</Actions>
	) : (
		<Actions>Dispute Label</Actions>
	)
}

I'm unsure if this is more or less readable as a whole than the existing solution.

<div className="transaction-details-dispute-details-body__actions">
<Link
href={
Expand Down Expand Up @@ -126,10 +257,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
'Continue with challenge',
'woocommerce-payments'
)
: __(
'Challenge dispute',
'woocommerce-payments'
) }
: challengeButtonDefaultText }
</Button>
</Link>

Expand All @@ -138,24 +266,21 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
disabled={ isLoading }
onClick={ () => {
wcpayTracks.recordEvent(
wcpayTracks.events
.DISPUTE_ACCEPT_MODAL_VIEW,
disputeAcceptAction.acceptButtonTracksEvent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Having to parameterise the button names and the tracks events is a signal that the UI is too generic. It's ok to implement a simple component with hard-coded content and single purpose.

{
dispute_status: dispute.status,
}
);
setModalOpen( true );
} }
>
{ __(
'Accept dispute',
'woocommerce-payments'
) }
{ disputeAcceptAction.acceptButtonLabel }
</Button>

{ /** Accept dispute modal */ }
{ isModalOpen && (
<Modal
title="Accept the dispute?"
title={ disputeAcceptAction.modalTitle }
onRequestClose={ onModalClose }
className="transaction-details-dispute-accept-modal"
>
Expand All @@ -167,40 +292,19 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
) }
</strong>
</p>
<Flex justify="start">
<FlexItem className="transaction-details-dispute-accept-modal__icon">
<Icon icon={ backup } size={ 24 } />
</FlexItem>
<FlexItem>
{ createInterpolateElement(
sprintf(
/* translators: %s: dispute fee, <em>: emphasis HTML element. */
__(
'Accepting the dispute marks it as <em>Lost</em>. The disputed amount will be returned to the cardholder, with a %s dispute fee deducted from your account.',
'woocommerce-payments'
),
getDisputeFeeFormatted(
dispute,
true
) ?? '-'
),
{
em: <em />,
}
) }
</FlexItem>
</Flex>
<Flex justify="start">
<FlexItem className="transaction-details-dispute-accept-modal__icon">
<Icon icon={ lock } size={ 24 } />
</FlexItem>
<FlexItem>
{ __(
'Accepting the dispute is final and cannot be undone.',
'woocommerce-payments'
) }
</FlexItem>
</Flex>

{ disputeAcceptAction.modalLines.map(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really generic and tightly coupled to disputeAcceptAction – to understand this code, requires reading whole file. How can we reduce that load?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's like an interface for generically rendering a modal. I don't think we need that capability, we can hard-code each modal we want as a self-contained component.

Copy link
Contributor

Choose a reason for hiding this comment

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

A side effect of this is that the tracks events both fire the same props, even though they are not relevant for both events. The tracks event name and the props should be nearby in the code (i.e. inline in the recordEvent() call) IMO.

( line, key ) => (
<Flex justify="start" key={ key }>
<FlexItem className="transaction-details-dispute-accept-modal__icon">
{ line.icon }
</FlexItem>
<FlexItem>
{ line.description }
</FlexItem>
</Flex>
)
) }

<Flex
className="transaction-details-dispute-accept-modal__actions"
Expand All @@ -219,27 +323,33 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
variant="primary"
onClick={ () => {
wcpayTracks.recordEvent(
wcpayTracks.events
.DISPUTE_ACCEPT_CLICK,
disputeAcceptAction.modalButtonTracksEvent,
{
dispute_status:
dispute.status,
}
);
setModalOpen( false );
doAccept();
/**
* Handle the primary modal action.
* If it's an inquiry, redirect to the order page; otherwise, continue with the default dispute acceptance.
*/
if ( isInquiry( dispute ) ) {
viewOrder();
} else {
Comment on lines +337 to +339
Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider if we need a new tracks event here.

wcpay_dispute_accept_click doesn't seem like it is a suitable event for tracking when the merchant clicks View Order to Issue Refund.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is your concern here for the naming? Or the use of the same event?

I agree the naming is off, maybe something like wcpay_dispute_modal_accept

While it may not be super clear that the button says something different for Inquiries, we can note this and as the dispute status is passed through with this event it is distinguishable

Copy link
Member

@Jinksi Jinksi Sep 28, 2023 •
edited by haszari
Loading

Choose a reason for hiding this comment

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

Reading the "event or props" part of the tracks cheat sheet ( PdjTHR-2FU-p2 ) helped to clarify my thoughts here:

Sometimes you can use properties to distinguish between events in different parts of the UI, or with different parameters. The rule of thumb here is the event – what’s the user intention you’re tracking?

  • If the events track different things, use different events (not props).
  • If it’s the same user intention, with different options, use props for the options.

In this case, I think the user intention that we are tracking is different:

  • Disputes: The user intent is accepting the dispute. wcpay_dispute_accept_click is fine here I think.
  • Inquiries: The user intent is viewing the order page. This should have a distinct event, e.g. wcpay_inquiry_view_order_click

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've added a new event for both. I see them as simple UI, the user clicks button or the user clicks modal button. The actual acceptance or viewing of the order can be tracked separately.

I don't think it hurts at all to keep the tracking for inquiries/disputes separate here.

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I've updated the event names to what I think you intended (feel free to correct me if I'm mistaken). 5cb3447

-DISPUTE_INQUIRY_REFUND_CLICK: 'wcpay_dispute_modal_refund_click',
+DISPUTE_INQUIRY_REFUND_CLICK: 'wcpay_dispute_inquiry_refund_click',
-DISPUTE_INQUIRY_REFUND_MODAL_VIEW: 'wcpay_dispute_modal_refund_click',
+DISPUTE_INQUIRY_REFUND_MODAL_VIEW: 'wcpay_dispute_inquiry_refund_modal_view',

doAccept();
}
} }
>
{ __(
'Accept dispute',
'woocommerce-payments'
) }
{
disputeAcceptAction.modalButtonLabel
}
</Button>
</Flex>
</Modal>
) }
</div>
) }
}
</CardBody>
</Card>
</div>
Expand Down
1 change: 1 addition & 0 deletions client/payment-details/summary/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ const PaymentDetailsSummary: React.FC< PaymentDetailsSummaryProps > = ( {
dispute={ charge.dispute }
customer={ charge.billing_details }
chargeCreated={ charge.created }
orderUrl={ charge.order?.url }
/>
) : (
<DisputeResolutionFooter dispute={ charge.dispute } />
Expand Down
39 changes: 39 additions & 0 deletions client/payment-details/summary/test/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,22 @@ jest.mock( 'wcpay/data', () => ( {
} ) ),
} ) );

jest.mock( '@wordpress/data', () => ( {
createRegistryControl: jest.fn(),
dispatch: jest.fn( () => ( {
setIsMatching: jest.fn(),
onLoad: jest.fn(),
} ) ),
registerStore: jest.fn(),
select: jest.fn(),
useDispatch: jest.fn( () => ( {
createErrorNotice: jest.fn(),
} ) ),
useSelect: jest.fn( () => ( { getNotices: jest.fn() } ) ),
withDispatch: jest.fn( () => jest.fn() ),
withSelect: jest.fn( () => jest.fn() ),
} ) );

const mockUseAuthorization = useAuthorization as jest.MockedFunction<
typeof useAuthorization
>;
Expand Down Expand Up @@ -792,6 +808,29 @@ describe( 'PaymentDetailsSummary', () => {
).toBeNull();
} );

test( 'correctly renders dispute details for "warning_needs_response" inquiry disputes', () => {
const charge = getBaseCharge();
charge.disputed = true;
charge.dispute = getBaseDispute();
charge.dispute.status = 'warning_needs_response';

renderCharge( charge );

// Dispute Notice
screen.getByText(
/The cardholder claims this is an unauthorized transaction/,
{ ignore: '.a11y-speak-region' }
);

// Actions
screen.getByRole( 'button', {
name: /Submit evidence/i,
} );
screen.getByRole( 'button', {
name: /Issue refund/i,
} );
} );

test( 'correctly renders dispute details for "warning_under_review" inquiry disputes', () => {
const charge = getBaseCharge();
charge.disputed = true;
Expand Down
3 changes: 3 additions & 0 deletions client/tracks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ const events = {
DISPUTE_CHALLENGE_CLICK: 'wcpay_dispute_challenge_click',
DISPUTE_ACCEPT_CLICK: 'wcpay_dispute_accept_click',
DISPUTE_ACCEPT_MODAL_VIEW: 'wcpay_dispute_accept_modal_view',
DISPUTE_INQUIRY_REFUND_CLICK: 'wcpay_dispute_inquiry_refund_click',
DISPUTE_INQUIRY_REFUND_MODAL_VIEW:
'wcpay_dispute_inquiry_refund_modal_view',
ORDER_DISPUTE_NOTICE_BUTTON_CLICK:
'wcpay_order_dispute_notice_action_click',
OVERVIEW_BALANCES_CURRENCY_CLICK:
Expand Down
Loading