Change Dispute → Details links to Transaction → Details
#7087
Conversation
Jinksi
changed the title
Transaction → DetailsDispute → Details links to Transaction → Details
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: +2 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
…transactions-details
…transactions-details
…transactions-details
| @@ -15,7 +15,7 @@ import { getAdminUrl } from 'wcpay/utils'; | |||
| /** | |||
| * The parent segment is the first part of the URL after the /payments/ path. | |||
| */ | |||
| type ParentSegment = 'deposits' | 'transactions' | 'disputes'; | |||
| type ParentSegment = 'deposits' | 'transactions'; | |||
There was a problem hiding this comment.
We no longer want to support '/payments/disputes/details' as a link destination.
There was a problem hiding this comment.
This component knows quite a bit about what's happening above/around it. What's its job?
There was a problem hiding this comment.
Would it be simpler to just require clients to pass the url in? Then simplify this to just rendering a link with a specific visual look & icon.
There was a problem hiding this comment.
(Not relevant for this PR, just noting so I can move on.)
There was a problem hiding this comment.
Related, DetailsLink exports getDetailsURL anyway, and it's effectively a (typed) wrapper for getAdminUrl. I prefer to use standard APIs throughout over wrapping (which effectively creates a second, forked API). The part I don't understand is whether ParentSegment is a meaningful & useful type.
Jinksi
changed the base branch from
develop
to
add/7289-remove-transaction-dispute-details-feature-flag
…g' into fix/6929-change-dispute-details-links-to-transactions-details
…g' into fix/6929-change-dispute-details-links-to-transactions-details
…transactions-details
changelog/fix-6929-change-dispute-details-links-to-transactions-details
Outdated
Show resolved
Hide resolved
| return add_query_arg( | ||
| [ | ||
| 'page' => 'wc-admin', | ||
| 'path' => '/payments/disputes/details', | ||
| 'id' => $dispute_id, | ||
| 'path' => '/payments/transactions/details', |
There was a problem hiding this comment.
I notice one of the URL from the dispute order note has / in the query string parameter value. Forward slash should be encoded to %2F.
Screen.Recording.2023-10-06.at.01.48.22.mov
To confirm encodeURIComponent('/') in Javascript would return %2F.
I was surprised add_query_arg() does not encode the parameter value but then I read that it expects the parameter value is already encoded.
| 'path' => '/payments/transactions/details', | |
| 'path' => '%2Fpayments%2Ftransactions%2Fdetails', |
or
| 'path' => '/payments/transactions/details', | |
| 'path' => urlencode( '/payments/transactions/details' ), |
There was a problem hiding this comment.
I've tried this and it ends up with a malformed URL with 0.00000 inserted in place of //%2F.
/wp-admin/admin.php?page=wc-admin&path=0.000000payments0.000000transactions0.000000details&id=ch_3Ny06gC15a3WbJ1t14zIF3QB
This URL is being processed by through WC_Payments_Utils::esc_interpolated_html(), so I'll see if there's another way to urlencode.
There was a problem hiding this comment.
Ow wow. Not sure what's going on there. I only tried it on WP Console.
Let's do it in a separate PR.
There was a problem hiding this comment.
I've opened an issue for this: #7399.
| [ 'id' => $dispute_id ], | ||
| admin_url( 'admin.php?page=wc-admin&path=/payments/disputes/details' ) | ||
| [ 'id' => $charge_id ], | ||
| admin_url( 'admin.php?page=wc-admin&path=/payments/transactions/details' ) |
There was a problem hiding this comment.
Either
| admin_url( 'admin.php?page=wc-admin&path=/payments/transactions/details' ) | |
| admin_url( 'admin.php?page=wc-admin&path=%2Fpayments%2Ftransactions%2Fdetails' ) |
Or consistent with compose_dispute_url():
add_query_arg(
[
'page' => 'wc-admin',
'path' => urlencode( '/payments/transactions/details' ),
'id' => $charge_id
],
admin_url( 'admin.php' )
)
There was a problem hiding this comment.
I've opened a followup issue for this: #7399.
|
I also feel like removing disputes here is a good idea so Typescript will scream when it's used. Not sure why it's reverted. |
I can't think of any other places than what you have listed. Your list is more comprehensive that what I initially thought even. Anyway, I tried to search these terms
Found:
Worst case, #7310 will redirect. |
Co-authored-by: Shendy <[email protected]>
That was my first idea, but in discussion above I/we determined that it was out of this PR's scope since there may be more decisions to make with those functions. Removing |
Co-authored-by: Shendy <[email protected]>
…transactions-details
|
Requesting re-review @shendy-a8c as all comments/suggestions are resolved (or issue logged for follow-up). |
Jinksi
deleted the
fix/6929-change-dispute-details-links-to-transactions-details
branch
Fixes #6929
Changes proposed in this Pull Request
The goal of this PR is to update any links to disputes anywhere in the UI. As we are now using the transaction detail page for managing and surfacing disputes, links to "a dispute" should point to the relevant transaction detail URL.
Here's a list of places in the UI that are affected (might not be complete, please explore!):
Respond to dispute for $33 now!mapEventsdispute_needs_response?dispute_updatedwebhook handlerTesting instructions
Review the WooPayments admin UI for places that mention or might link to disputes and ensure they now link to the correct transaction detail screen. Ensure that the transaction screen shows the relevant info about the dispute and the merchant can action, resolve, respond to the dispute.
TBD - list of specific places this PR has fixed
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