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 5 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.


144 changes: 106 additions & 38 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, chevronRightSmall } 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,14 @@ interface Props {
dispute: Dispute;
customer: ChargeBillingDetails | null;
chargeCreated: number;
orderDetails: OrderDetails | null;
}
brucealdridge marked this conversation as resolved.
Show resolved Hide resolved

const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
dispute,
customer,
chargeCreated,
orderDetails,
} ) => {
const { doAccept, isLoading } = useDisputeAccept( dispute );
const [ isModalOpen, setModalOpen ] = useState( false );
Expand All @@ -53,13 +56,32 @@ 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 ( orderDetails?.url ) {
window.location.href = orderDetails?.url;
return;
}

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

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

brucealdridge marked this conversation as resolved.
Show resolved Hide resolved
return (
<div className="transaction-details-dispute-details-wrapper">
<Card>
Expand All @@ -80,7 +102,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
dispute={ dispute }
daysRemaining={ countdownDays }
/>
{ showDisputeStepsAndActions && (
{ showDisputeSteps && (
<DisputeSteps
dispute={ dispute }
customer={ customer }
Expand All @@ -93,7 +115,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 +148,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
'Continue with challenge',
'woocommerce-payments'
)
: __(
'Challenge dispute',
'woocommerce-payments'
) }
: challengeButtonDefaultText }
</Button>
</Link>

Expand All @@ -147,15 +166,25 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
setModalOpen( true );
} }
>
{ __(
'Accept dispute',
'woocommerce-payments'
) }
{ isInquiry( dispute )
? __(
'Issue refund',
'woocommerce-payments'
)
: __(
'Accept dispute',
'woocommerce-payments'
) }
</Button>

{ /** Accept dispute modal */ }
{ isModalOpen && (
<Modal
title="Accept the dispute?"
title={
! isInquiry( dispute )
? 'Accept the dispute?'
: 'Issue a refund?'
}
onRequestClose={ onModalClose }
className="transaction-details-dispute-accept-modal"
>
Expand All @@ -172,35 +201,61 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
<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.',
{ isInquiry( dispute )
? __(
'Issuing a refund will close the inquiry, returning the amount in question back to the cardholder. No additional fees apply.',
'woocommerce-payments'
),
getDisputeFeeFormatted(
dispute,
true
) ?? '-'
),
{
em: <em />,
}
) }
)
: 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'
) }
{ isInquiry( dispute )
? __(
'This action is final and cannot be undone.',
brucealdridge marked this conversation as resolved.
Show resolved Hide resolved
'woocommerce-payments'
)
: __(
'Accepting the dispute is final and cannot be undone.',
'woocommerce-payments'
) }
</FlexItem>
</Flex>
{ isInquiry( dispute ) && (
<Flex justify="start">
<FlexItem className="transaction-details-dispute-accept-modal__icon">
<Icon
icon={ chevronRightSmall }
size={ 24 }
brucealdridge marked this conversation as resolved.
Show resolved Hide resolved
/>
</FlexItem>
<FlexItem>
{ __(
'You will be taken to the order, where you must complete the refund process manually.',
'woocommerce-payments'
) }
</FlexItem>
</Flex>
) }

<Flex
className="transaction-details-dispute-accept-modal__actions"
Expand All @@ -227,19 +282,32 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( {
}
);
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'
) }
{ isInquiry( dispute )
? __(
'View Order to Issue Refund',
brucealdridge marked this conversation as resolved.
Show resolved Hide resolved
'woocommerce-payments'
)
: __(
'Accept dispute',
'woocommerce-payments'
) }
</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 }
orderDetails={ charge.order }
/>
) : (
<DisputeResolutionFooter dispute={ charge.dispute } />
Expand Down
Loading