Use 'Withdrawal' and 'Deducted' labels when referring to withdrawal deposits, to more accurately communicate the type of transaction that has occurred #9500
Conversation
Jinksi
changed the title
Jinksi
changed the title
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: +147 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Jinksi
changed the title
…atch changes to withdrawal status label
Jinksi
added
needs tests
|
Ready for review – note I'll be adding some additional integration tests to ensure the new withdrawal labels are rendered correctly, as mentioned in the PR description TODO. |
| placeholder="" | ||
| role="combobox" | ||
| type="text" | ||
| value="" | ||
| /> | ||
| <span | ||
| class="screen-reader-text" | ||
| id="search-inline-input-15" | ||
| id="search-inline-input-16" |
There was a problem hiding this comment.
These snapshot changes to the rendered HTML id numbers can be ignored – this occurs because I've added a new test case being rendered, bumping many of these ids by +1.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Manual testThe changes test well on a manual test 🥇
|
| * 'deducted' represents a deposit of the type 'withdrawal' and status 'paid'. | ||
| */ | ||
| export const depositStatusLabels: Record< | ||
| DepositStatus | 'deducted', |
There was a problem hiding this comment.
At the first glance I wondered why deducted is not added within the DepositStatus type. I then realized deducted is not a status coming from Stripe, but what we use for labeling. I read the doc comment later too 💡
|
@rogermattic @haszari now that I've implemented the changes we discussed, could you please take a look at the screenshots above (or the Helix demo site) and share your thoughts on the changes:
There will be some followup CSS alignment issues to correct, I'll do once we confirm we're happy with the naming above – e.g. in the |
Jinksi
added
pr: needs review
pr: needs design review
and removed
pr: needs author reply
labels
There was a problem hiding this comment.
This all looks great! Nice to do this holistically, details matter!
I tested by reviewing the Helix demo site – I haven't reviewed code (assuming this is 👌🏻 at this point).
One area might be worth another look – the payouts overview table.
Would it be possible to bring in the Type column to this table? Otherwise we need to add Settled/Completed (paid) / Settled/Completed (deducted)
This seems redundant to me, so adds clutter (especially since withdrawals are a rare edge case). @Jinksi will the Status show Completed (deducted) for (rare) withdrawals? If so, the Type might not be needed.
Yes, the status in the recent deposits list will show
Update: I've reverted the new
|
|
Just tested it @Jinksi @haszari using the demo site (thanks for this)! Sorry for the suggestion to add the Otherwise everything looks great. I only have one extra question – I was wondering why this label wraps if there seems to be enough space for displaying it all in one line? What's the rule behind it? |
Thanks for catching this @rogermattic 🙏 I'll fix this line-wrapping issue. |
|
It turns out, the deposit details status label was fixed in #8382, which has therefore been fixed once I merged
|
|
Awesome! Thanks @Jinksi ! |
Fixes #7802
Changes proposed in this Pull Request
Paiddeposit status is nowCompleted (paid)Completed (deducted)instead ofPaidWithdrawalinstead ofDepositDeposit transactionsis now simplyTransactions, to reduce some confusion that could be introduced by the termWithdrawal transactions.TODO
Typeto recent deposits list reverted, redundant with theCompleted (deducted)labelPaidandDeductedtoCompleted (paid)andCompleted (deducted)Deposit details screen:
Withdrawal:
Deposit:
Deposit list screen:
Deposit list screen advanced filters:
The option for
paidnow showsCompleted.Payments overview recent deposits list:
Testing instructions
Tip
I've put this PR's changes on the Helix Demo Site, if you have access. If you go back to page 3 of the Payments → Deposits list, you can see a mix of Withdrawal/Deposits.
npm run build4000000000000259trunkwithdrawal deposits will appear as typeDepositwith aPaidstatusWithdrawalwith aDeductedstatus (see screenshot above)Status. ThePaidoption has been re-labelled toPaid / Deducted. Note that we don't support filtering byDeducted`, since it is a display/label only and not a status recognised by the WooPayments Server.Deductedstatus appears next to withdrawals on the Payments → Overview recent deposits list.Withdrawal date,DeductedandWithdrawal transactions(see screenshot above)Deposit date,PaidandDeposit transactionsnpm 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