-
Notifications
You must be signed in to change notification settings - Fork 72
Add Fees and Net columns to the payout list #11116
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
base: develop
Are you sure you want to change the base?
Add Fees and Net columns to the payout list #11116
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: +698 B (0%) Total Size: 874 kB
ℹ️ View Unchanged
|
granjacours410-source
left a comment
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.
After tampermonkey reseting scripts showing this Line up not East whit so many bugs
mgascam
left a comment
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.
The code changes look good, but I'm not certain this is the right data to surface. Please check my comment for more details, although I haven't dig into the code in detail.
| ), | ||
| }, | ||
| fees: { | ||
| value: formatExportAmount( deposit.fee, deposit.currency ), |
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'm less familiar with deposits, but I'd say that we should surface the total fees paid for every transaction included in the deposit. As it is now, we seem to surface the fee associated with the deposit. In my local (can be wrong) the deposit.fee is always zero but when I click in the row then I see all the transactions, each with their fee. I think those are the fees that we should surface.

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.
@mgascam so, you mean adding up all the transaction fees related to that specific deposit? Is there somewhere we can get that information? Sorry if I'm missing something obvious.
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.
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.
the new approach of introducing useDepositsWithFeeData, which literally loop through all deposits in the current deposit list, and each of which will send a REST request to get the transaction summary.
I think it's not efficient in all terms (user experience and our server load). IMHO, the right approach here is introduce new fields for the deposit list response to map with what provided. But this also means more work should be added into the server to support these new fields.
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.
By using the initial commit in this PR 86c2f2d and this patch in the server By 39915-pb/#diff, the outcome looks good: the first one is instant/manual and the second is automatic:
However, the issue is that it's creating the discrepancy in the view of each one. I think this is something we should also improve, and it's worth having a discussion with designs about this.
| Instant | Automatic |
|---|---|
| No transaction | No fee and percentage |
![]() |
![]() |
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 seems to be an interesting change, because a payout's net amount isn't always defined by the contained transactions:
- For the instant payout, the merchant enters the amount they want from the available volume. The only fee in this case is the instant payout fee.
- Regular payouts do not incur additional fees, but the net amount depends on:
- Transactions included in the payout and related application fees
- Amounts taken with earlier instant payouts
- Additional charges, such as card reader charges.
Therefore, I agree with @htdat that if we start surfacing fees, we need to do it holistically and highlight what "fee" means in each case, linking the WooPayments documentation where needed.


See WOOPMNT-5359
Changes proposed in this Pull Request
This PR add two columns to the payout history list, Fees and Net.
Testing instructions
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