Update Transaction Details breakdown: rename labels and add fee breakdown tooltip when disputed #7118
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. |
in order to filter balance transactions by 'dispute' and get the dispute fee
|
Size Change: +271 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
e.g. text & ClickTooltip
Jinksi
changed the base branch from
develop
to
update/horizontal-list-label-style-uppercase
…te/6974-transaction-details-breakdown-labels-and-fees
Jinksi
changed the title
|
Thanks for catching the issue with multi-currency transactions @shendy-a8c 🙏 I've addressed this in 8c9ef0e and b12f465 to now display the fee in the correct currency (the transaction fee's currency). Tooltip Transaction Fee (kr41.60 in this case): Now matches the Transaction timeline fee total (kr-41.60): 
|
There was a problem hiding this comment.
This PR is working well and implements the design.
I did some brief testing with a couple of disputes, tested with feature flag on and off. Also tested an undisputed transaction and no issues/regressions.
The styling of the transaction breakdown row doesn't feel right to me. The labels and the data use the same styles, which I find hard to read – the data should stand out, the labels are there to "onboard" and orient users to the values they need to find. This could be handled as follow up since this PR adds a useful breakdown tooltip. Keen for input from @nikkivias.
It got me thinking about a few things that we might want to look into (likely as follow up):
- Overall consistency / agreement on this page. Using the same terms/labels and breakdown in the header summary and the timeline.
- Ensure we are using currency labels everywhere. There are some places these are left off and this is confusing. If a store only has 1 currency, no labels are needed, but if multicurrency is set up, every $$$ value should include currency IMO.
NETamount might need a breakdown tooltip too – but maybe there's a bigger confusion that we are trying to address with tooltips. I find it confusing that the deduction amount in the timeline includes the dispute fee but the summary NET / left number doesn't.- Actually that seems like a bug –
NETshould include all fees and deductions.
- Actually that seems like a bug –
- The first summary number is confusing without a label! I understand this is the purchase price after currency conversion.
Example transaction:
Specific suggested changes (happy to log issues for this after discussion):
- Transaction summary
NETshould include dispute fee. In general, that line should add up toNET. - All transaction timeline values should have currency (for multicurrency stores): loan repayment, disputed amount, dispute fee.
Consider:
- First value in summary line (purchase amount in store currency) - is this correct/confusing for disputes?
- Timeline lists all items as
will be added from future depositwill be subtracted from future deposit. Ideas:- Make timeline top-level sentence more descriptive, e.g.
$2.73 USD deducted for loan repayment(was$2.73 will be subtracted from a future deposit.)$33.23 USD deducted due to dispute(was$33.23 USD was deducted from your Jun 13, 2023 deposit.)
- Make timeline top-level sentence more descriptive, e.g.
- Some timeline items link to a real deposit, others like loan repayments seem to just say
Future deposit. I wonder if we need to complicate timeline items with deposit info.
| @@ -177,6 +186,16 @@ const PaymentDetailsSummary: React.FC< PaymentDetailsSummaryProps > = ( { | |||
|
|
|||
| const isFraudOutcomeReview = isOnHoldByFraudTools( charge, paymentIntent ); | |||
|
|
|||
| const disputeFee = | |||
| charge.dispute && | |||
| charge.dispute.status !== 'won' && | |||
There was a problem hiding this comment.
I guess this logic could be moved to an accessor, e.g. getDisputeFeeCharged. What I read from this is that getDisputeFee is confusing. Could rename it getDisputeBalanceTransaction or something more technical/accurate.
| const disputeFee = dispute.balance_transactions.find( | ||
| ( transaction ) => transaction.reporting_category === 'dispute' | ||
| ); | ||
| return disputeFee; |
There was a problem hiding this comment.
Could this find ever return multiple items? Do we expect a single item returned from this function?
There was a problem hiding this comment.
I've added a description to the Dispute TS interface explaining what states the balance_transactions could represent, based on the Stripe docs.
woocommerce-payments/client/types/disputes.d.ts
Lines 105 to 110 in 764566d
I'm adjusting the getDisputeFee function to help make more sense of this.
There was a problem hiding this comment.
I've changed the getDisputeFee logic to clarify some things here.
woocommerce-payments/client/disputes/utils.ts
Lines 75 to 114 in facb162
@haszari do the comments and function names help to clarify the logic here?
| </> | ||
| } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
Suggest adding currency to all currency values for clarity. (We should probably review all recent changes to ensure we're consistently showing currency.)
This tooltip has similar information to the timeline, should we make them more consistent?
For example:
Fee=>Dispute feein timeline$n USD deducted(adddeductedto line item in timeline)- What's the $33.08 headline number in timeline 🤔?
There was a problem hiding this comment.
What's the $33.08 headline number in timeline 🤔?
This I can answer: $18.08 USD disputed amount + $15 USD dispute fee.
Although, the current state of the UI doesn't make this clear. This could be moved to a new issue.
Related to this is the open PR that will change the Transaction Details headline amount to display in the store's currency instead of the customer's currency: #7159
There was a problem hiding this comment.
Sounds good to me – we can iterate on these details over time.
| margin: 0.25rem 1rem 0 0; | ||
| text-transform: uppercase; | ||
| font-weight: 600; | ||
| font-size: 12px; |
There was a problem hiding this comment.
I find it hard to read with the data in the same all caps style as the headings/labels. @nikkivias what do you think about using different style to distinguish the labels from the data?
For example bold small caps for labels, regular weight for data. Mockup:
There was a problem hiding this comment.
From an engineering perspective, this is not a completely straightforward change (these are all rendered as strings, e.g. Deducted: $-18.08 USD, so no separate elements to style).
Therefore, I'll leave this as is for this PR and we can iterate on this in a separate issue after the feedback session.
There was a problem hiding this comment.
Works for me. I also think we need to discuss this idea with design / other stakeholders before proceeding (though we could maybe prototype at appropriate priority).
Jinksi
added
pr: needs author reply
and removed
pr: needs review
pr: needs design review
labels
Jinksi
dismissed
shendy-a8c’s stale review
Issue with transaction fee tooltip currency has been resolved.
and rename getDisputeDeductedBalanceTransaction to clarify intent
Jinksi
deleted the
update/6974-transaction-details-breakdown-labels-and-fees
branch
Fixes #6974
Changes proposed in this Pull Request
This PR updates the Transaction Details screen with the following changes:
Refunded→ DeductedFee→ FeesNote: HorizontalList uppercase labels were updated in #7126
Before: (
develop)After:
Fees tooltip content, multi-currency enabled:
Design:
Additionally, some other code improvements:
reporting_categoryproperty to theBalanceTransactiontype.ClickTooltipbug preventing theclassNameprop from being applied to the tooltip content element.Testing instructions
4000000000000259.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