Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
Change
Dispute → Detailslinks toTransaction → Details#7087Change
Dispute → Detailslinks toTransaction → Details#7087Changes from 39 commits
194c9c8869a44906b1983ad5194f000ac7e2208df637ca6bad1b54b3a245b139ae6fe47a619d1011164e41439cf2dec45cc6f10f2266fe7d7cc4546868ab806c40af990834a7c85be9b0d3779328e14a5d68f779030262e571188f171fe63f5bd338520a3790b3e851bebbecd7efd2ab2e47bb846c8c749d7c3d9ea33d51cab1be827ff3bcc73ed058a17ad20a958e5ba300f7ee8df38524d291c7aaf93dcae68fFile filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the follow up question is… is it a bad idea to switch all these to
getAdminUrlnow as part of fixing this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, I made the choice to change one of these links from
getAdminUrl→getDetailsURLsince I assumed it was the preferred option for detail screen URLs.I'm not sure if that's a bad idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all depends on what
parentSegmentmeans, and what value it adds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you prefer and why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To complicate things,
getAdminUrlis a wrapper foraddQueryArgs– should this be used instead of the wrapper function?woocommerce-payments/client/utils/index.js
Line 25 in 2e47bb8
I think we leave this PR very specific to the scope of the PR and change links only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, probably making extra work to switch over from
DetailsLink/getDetailsURL.Here's my take…
getAdminUrlis a useful, easy to understand API for constructing a url anywhere in Woo admin. We should use it whereever it makes sense.getWooPaymentsAdminUrl.getDetailsURLis not an improvement overgetAdminUrl- makes the code less readable.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus points for
getAdminUrl– if you understand how it works (what the params mean), that knowledge transfers toaddQueryArgs(and vice versa). Not so forDetailsLinkandgetDetailsURL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this PR, I'd prefer if the changes were specific to the goal. 😆
From a code-quality perspective, I'd like to understand if having the
getDetailsUrlwith typed parameters helps us in some way before removing it.Thinking about a developer using
getDetailsUrl, it might be a helpful abstraction. They won't have to remember if it istransaction/details,/transactions/details,/payments/transactions/details,%2Ftransactions%2Fdetailsetc. The function remembers all of this and TS yells at me if I use the wrong parent (transactioninstead oftransactions).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point - a typed wrapper of
getAdminUrlorgetWooPaymentsAdminUrlcould add value, though this is orthogonal to my concern thatgetDetailURL( … parentSegment )is an unintuitve, weird API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd find it much easier to read an API like
getWooPaymentsUrl( collection: 'transactions' )getWooPaymentsUrl( collection: 'transactions', item: transactionId )Or more transparently
getWooPaymentsUrl( 'transactions' )getWooPaymentsUrl( 'transactions/details/${ transactionId }' )If you were designing this API, what would it look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this and it ends up with a malformed URL with
0.00000inserted in place of//%2F./wp-admin/admin.php?page=wc-admin&path=0.000000payments0.000000transactions0.000000details&id=ch_3Ny06gC15a3WbJ1t14zIF3QBThis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened an issue for this: #7399.