Disputes: Allow merchant to respond to inquiries from transaction detail page #7298
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +1.02 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This tests well @brucealdridge 🙌
I think that there are some suggestions I've left that are worth addressing before merging:
- The
Action is final and cannot be undoneseems incorrect, since the action is viewing an order. I think this line should be removed within this modal for inquiries. - Inconsistencies with button label capitalisation.
Other suggestions are minor/non-blocking.
Tests completed:
- Action buttons appear for inquiries awaiting a response ✅
Submit evidencebutton takes me to the challenge form ✅Issue refundbutton displays a modal ✅Cancelcloses the modal ✅View orderbutton takes me to the correct order ✅- I tested the view order button in the (rare or impossible?) case of missing order URL. Error notice appears ✅
- Subsequently refunding the order results in the inquiry being closed (in test-mode at least) ✅
- Renders the
Continue with challengebutton label for a dispute with staged evidence ✅
Note: I fixed broken tests in 7f24785 / 8a6166c and added tests for warning_needs_response in 24d86f5.
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Outdated
Show resolved
Hide resolved
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Show resolved
Hide resolved
| @@ -93,7 +115,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( { | |||
| /> | |||
|
|
|||
| { /* Dispute Actions */ } | |||
| { showDisputeStepsAndActions && ( | |||
| { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this is quite subjective, so feel free to leave as-is!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if ( isInquiry( dispute ) ) { | ||
| viewOrder(); | ||
| } else { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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_clickis 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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',
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Outdated
Show resolved
Hide resolved
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Outdated
Show resolved
Hide resolved
client/payment-details/dispute-details/dispute-awaiting-response-details.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good @brucealdridge!
Everything I tested before (#7298 (review)) is working well ✅
I've left a comment discussing code readability that shouldn't prevent merging, approving ✅
| @@ -93,7 +115,7 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( { | |||
| /> | |||
|
|
|||
| { /* Dispute Actions */ } | |||
| { showDisputeStepsAndActions && ( | |||
| { | |||
There was a problem hiding this comment.
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.
| if ( isInquiry( dispute ) ) { | ||
| viewOrder(); | ||
| } else { |
There was a problem hiding this comment.
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',
| ); | ||
| }; | ||
|
|
||
| const disputeAcceptAction = getAcceptDisputeProps( dispute ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yeah totally, I don't want any of this to slow us down. We can always maintain, refactor, tidy in future :)
There was a problem hiding this comment.
Reviewing this again as I register tracks events. I really want to refactor this, AcceptDisputeProps is an abomination!
There was a problem hiding this comment.
I started on this refactor in #7339, which may help or may not. I will close it if not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -138,24 +266,21 @@ const DisputeAwaitingResponseDetails: React.FC< Props > = ( { | |||
| disabled={ isLoading } | |||
| onClick={ () => { | |||
| wcpayTracks.recordEvent( | |||
| wcpayTracks.events | |||
| .DISPUTE_ACCEPT_MODAL_VIEW, | |||
| disputeAcceptAction.acceptButtonTracksEvent, | |||
There was a problem hiding this comment.
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.
| </FlexItem> | ||
| </Flex> | ||
|
|
||
| { disputeAcceptAction.modalLines.map( |
There was a problem hiding this comment.
This is really generic and tightly coupled to disputeAcceptAction – to understand this code, requires reading whole file. How can we reduce that load?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I've added some comments – non blocking, I think there are ways we can simplify this code. However I don't fully understand it (need to spend a bit more time to do so), so I might be misguided. Looking at the description, I think perhaps implementing a dedicated component for each modal might help a lot. @brucealdridge – can you clarify the title? I don't fully understand the changes – is this implementing wording changes for inquiries, on existing UI for disputes, and adding a modal flow to guide merchants to refund orders (in response to an inquiry)? If so, I might summmarise that as |
brucealdridge
changed the title
|
I am merging despite failing one test which uses php7.3. The test fails as the latest WC requires php7.4. |
Fixes #7193
Changes proposed in this Pull Request
Add handling of CTA and modal for when a dispute is of type Inquiry.
This PR mostly focuses on minor wording changes.
Inquiries have separate actions that can be undertaken as compared with a normal dispute. They can be challenged but not accepted. With Inquiries the
acceptaction has been replaced with arefundaction.When clicking the primary action
Submit Evidencebutton, the user is taken through the normal flow to submit evidence for the dispute/inquiry. As with disputes, the secondary action buttonIssue refundpops up a modal with more information.The primary action button inside the modal
View Order to Issue Refund. It needs to be very clear to the merchant that they must issue a refund manually.An automatic refund ability may be added later, likely once #7248 is implemented.
Text changes
Challenge Dispute/Accept DisputeSubmit Evidence/Issue RefundAccept the dispute?Issue a refund?Accepting the dispute is final and cannot be undone.
This action is final and cannot be undone.
You will be taken to the order, where you must complete the refund process manually..
Cancel/Accept DisputeCancel/View Order to Issue RefundScreenshots
Testing instructions
update_option( '_wcpay_feature_dispute_on_transaction_page', '1' );in WP Console or by modifying your database tablewp_optionsdirectly.4000000000001976at checkout.Submit Evidence/Issue Refundbuttons.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge