add a new page component for redirecting disputes => details: #7310
Conversation
- first cut - registers a new handler component for dispute details - RedirectToTransactionDetails - that handler generates the relevant url Still to do: - get the redirect working - get the user experience working smoothly - currently takes time to generate the URL - we may need an "interstitial" "one moment please" UI
|
I'm in the middle of this but I have no objections to anyone taking over at any time 😁 |
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.37 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
| const { dispute, isLoading } = useDispute( disputeId ); | ||
| const disputeObject = dispute || ( {} as Dispute ); | ||
| const disputeIsAvailable = ! isLoading && dispute && disputeObject.id; | ||
| // Why would dispute.charge ever be a string? |
There was a problem hiding this comment.
Our interfaces for Disputes are not 100% accurate here.:
- When fetching a charge,
charge.dispute.charge: string - When fetching a dispute,
dispute.charge: Charge
Currently, we use Dispute as the interface for both of these response types.
I've had a first crack at solving this in #7311 – unsure if it's the right way to go.
There was a problem hiding this comment.
Thanks for the tip - will take a look!
There was a problem hiding this comment.
Added some comments to the PR. This feels a bit like typescript chicanery at first glance. Hoping there's a simple non-hack solution! If we need to start sketching out hierarchical types that implies maintenance risk IMO (object oriented vibes).
There was a problem hiding this comment.
I've updated my comment, since the type is correct in my view. The part I don't understand – is casting the correct approach an "OR" type like this? I.e. let me know if as Charge is a hack, or if I need a runtime type check.
There was a problem hiding this comment.
I see type-casting as a risk in this scenario since it makes assumptions about the code that may not always be true. E.g. we may in the future change the response type of this endpoint to always return dispute.charge: string and not realise that this has been overridden (as Charge) throughout the codebase.
I think a more robust way to assert that dispute.charge === Charge would be via a type guard function e.g.:
const isCharge = ( charge: Charge | string ): charge is Charge => {
return typeof charge !== 'string';
};
const { charge } = dispute;
if ( isCharge( charge ) ) {
// charge.payment_intent exists!However, since we know that GET disputes/{id} endpoint will return a dispute with dispute.charge: Charge, it would be better to fix this upstream (the Dispute interface) since we have full control here.
I think it's OK to follow this up in a future PR, however.
There was a problem hiding this comment.
Thanks - I look forward to understanding this better in the upstream PR.
This kind of thing feels very heavy compared to classic loose-duck-types JavaScript, so I want to really understand how to use TypeScript to strengthen things in a lean/elegant way.
+ reorg imports (alphabeticish by module)
@nikkivias do you know how we handle things like this?
I'm thinking we need a message like |
|
I will look into how we might handle this @haszari how many seconds is it currently taking to redirect? |
Good question @nikkivias! 2-5 seconds approximately. Here's a video :) Screen.Recording.2023-09-28.at.5.50.59.PM.mov |
This comment was marked as outdated.
This comment was marked as outdated.
- when redirecting, we don't need to render anything at all
…hub.com:Automattic/woocommerce-payments into fix/6891-redirect-dispute-detail-to-transaction
|
FYI @nikkivias there's an updated prototype now using standard I think this is almost shippable, especially since this UI will be very rarely seen (only on first nav to each unique dispute, and we are planning on fixing most links anyway). Let me know what you think. |
- feature off: render (legacy) dispute details page - feature on: render a redirect to transaction details
- This module should be deleted when we go live with this feature - Adding this as a reminder
client/disputes/details/index.tsx
Outdated
| @@ -27,11 +27,19 @@ const DisputeDetails = ( { | |||
| query: { id: disputeId }, | |||
| }: { | |||
| query: { id: string }; | |||
| } ): JSX.Element => { | |||
| } ): JSX.Element | null => { | |||
There was a problem hiding this comment.
I'm (now) in the habit of declaring React components as Element | null, consistent with classic React. As I understand, returning null is totally fine way to not render anything.
Question for front end experts – is this a good approach? Should we have a standard type for this? What does the TypeScript React community do?
There was a problem hiding this comment.
Not a frontend expert but since as of currently, there is a possibility this component will return null, this | null addition is necessary.
However, I have my opinion that checking for feature flag and returning null is not necessary here. Read along =)
There was a problem hiding this comment.
Note that when we use React.FC/React.FunctionComponent, the return type is implicit and we don't have to define it (see TS guidelines doc).
I've switched this to React.FC in 2c4dbfc.
There was a problem hiding this comment.
Aha! Now I'm a fan of React.FC and will use from here on in. Can we encourage that pattern with a linter or tooling?
There was a problem hiding this comment.
Looking at the doc, we might be able to distil this to a principle:
Use React.FC type for React components
| <FlexItem> | ||
| <span>Redirecting to payment details…</span> | ||
| </FlexItem> | ||
| </Flex> |
There was a problem hiding this comment.
Happy days using core components to get a nice, consistent look out of the box!
client/disputes/details/index.tsx
Outdated
| if ( isDisputeOnTransactionPageEnabled ) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Added this so when we're working on #7289 we find this class (search for feature flag) and remember to delete it. 🐘
There was a problem hiding this comment.
To be explicit, I just added one check item in #7289:
- Remove the legacy dispute details page.
In addition, we aren't handling network errors, see screen recording: Dispute.redirect.network.error.movI'm looking into fixing this. PS, this wasn't caught by TS because we're currently casting |
| <div> | ||
| <b>Error retrieving dispute</b> | ||
| </div> | ||
| <div>Please check your network and try again.</div> |
There was a problem hiding this comment.
We could consider giving more accurate error messages, e.g. looking at error.code to determine a suitable messages for resource_missing and fetch_error.
There was a problem hiding this comment.
Not needed IMO, this redirect is a fallback so will hopefully not get much use. If merchant lands here and it doesn't work for any reason, they should be able to get back on track :)
|
I've handled network errors in bec0846, 5e006f8, 6111fac, 6b00887. See screen recording example of using an unknown dispute id below. Dispute.redirect.network.error.handled.movAnother edge case that may or may not require investigating – what will happen if a live-mode dispute email is clicked and the site is in test-mode? |
| @@ -44,7 +49,7 @@ export const useDispute = ( | |||
| const { acceptDispute } = useDispatch( STORE_NAME ); | |||
| const doAccept = () => acceptDispute( id ); | |||
|
|
|||
| return { dispute, isLoading, doAccept }; | |||
| return { dispute, isLoading, error, doAccept }; | |||
There was a problem hiding this comment.
Adding support for handling dispute load errors is possibly overengineering for this use case (fallback redirect), but is useful in general so let's keep it :)
I do wonder how we got to this point in the project without adding this!
There was a problem hiding this comment.
The resolver shows a snackbar on error: “Error retrieving dispute”. I’d guess that this has been a sufficient way to handle errors until now.
There was a problem hiding this comment.
Right, so is there a chance we don't need this extra support? Happy for it to merge, just clarifying the value.
There was a problem hiding this comment.
Yep the value is low at the moment, we’re only checking for a fetch error in this component. Alternatively, we could move the error handling to the component, but I think it’s more predictable to handle dispute fetch errors in a way that is consistent with other wp.data resolvers.
| @@ -68,6 +69,9 @@ describe( 'getDispute resolver', () => { | |||
| expect.any( String ) | |||
| ) | |||
| ); | |||
| expect( generator.next().value ).toEqual( | |||
| updateErrorForDispute( 'dp_mock1', undefined, errorResponse ) | |||
There was a problem hiding this comment.
Curious what regressions tests like this would catch.
There was a problem hiding this comment.
I guess it could help ensure that we’re calling the update function in the case of a refactor but it feels a bit like we’re testing implementation details here.
Note that this isn’t a new test, I added this here to fix a failing test after adding updateErrorForDispute to the getDispute resolver.
There was a problem hiding this comment.
Gotcha, figured there would be a thread pulling you here 😁
| } ) => { | ||
| const { dispute, error, isLoading } = useDispute( disputeId ); | ||
|
|
||
| useEffect( () => { |
There was a problem hiding this comment.
Doh! Is this the fix for my react issue! Obvious in hindsight that I would need to redirect as a side effect. Thanks for sorting this @Jinksi , major oversight on my side.
|
Thank you @Jinksi for getting this PR shipshape. Especially for
I don't think we need to worry about any more edge cases, the only real goal here is that if merchant navs to a dispute, they get redirected to the correct transaction. If there are issues that mean we can't redirect, we have to trust that the merchant will be able to use the UI to get back on track – loading this component with low-risk contingencies is not worth the effort IMO. |
|
Merging! |
haszari
dismissed
shendy-a8c’s stale review
Changes are addressed and has been re-reviewed by @Jinksi
Fixes #6891
This is ready for review though I haven't fully updated the PR description. Reviewer(s) please ping me / review harshly if any info is missing or not clear!This PR changes behaviour with
_wcpay_feature_dispute_on_transaction_pageenabled, and should preserve existing behaviour with flag off or missing.RedirectToTransactionDetailsScreen.Recording.2023-09-29.at.11.47.06.AM.mov
Still to do:
get the redirect workingget the user experience working smoothlyensure works correctly with feature flag_wcpay_feature_dispute_on_transaction_pageon/offTesting instructions
Feature flag:
_wcpay_feature_dispute_on_transaction_pagee.g. set with WP CLI
docker-compose exec wordpress wp --allow-root option set _wcpay_feature_dispute_on_transaction_page 1_wcpay_feature_dispute_on_transaction_pageenabled_wcpay_feature_dispute_on_transaction_pagedisabled or missingnpm 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