WC Home active disputes task: improve wording and reduce due_by threshold to align with Payments Overview task
#6548
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. |
|
As of current state, this PR should be functional with a hard-coded test data. 
Still many TODOs:
|
|
Size Change: +18 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Jinksi
changed the title
due_by threshold to align with Payments Overview task
|
I needed to delete option |
|
Note: I've implemented cache invalidation for the active disputes cache in 167f5f8. |
| @@ -354,7 +354,7 @@ describe( 'getTasks()', () => { | |||
| completed: false, | |||
| level: 1, | |||
| title: 'Respond to a dispute for $10.00 – Last day', | |||
| content: 'Respond today by 11:59 PM', | |||
| content: 'Respond today by 6:59 PM', // shown in local timezone. | |||
There was a problem hiding this comment.
I had a look and couldn't find the local timezone set anywhere. This could cause failures if the timezone is different from the expected (EST ?).
There was a problem hiding this comment.
Nice catch! I've explicitly set this to America/New_York for this test suite, reverting afterAll.
You can check this is working by setting the TZ to e.g. Australia/Brisbane, tests will then fail.
Updated in 02cbd09.
There was a problem hiding this comment.
I realise this will be impacted by daylight savings... fix incoming.
There was a problem hiding this comment.
Set to UTC-5 (EST / non-daylight-savings) in caac016.
There was a problem hiding this comment.
Shouldn't be affected by daylight savings as the dates are consistent.
There was a problem hiding this comment.
Ah, yes, right!
I'm having flashbacks of all of the failing GH checks last time we changed to/from daylight savings. I'll just leave it in, at least it's explicit.
|
|
||
| $this->assertEquals( 'Respond to a dispute for 20,00 € – Last day', $disputes_task->get_title() ); | ||
| // "Respond today by <time> <AM|PM>" | ||
| $this->assertMatchesRegularExpression( '/Respond today by \d{1,2}:\d{2} (AM|PM)/', $disputes_task->get_additional_info() ); |
There was a problem hiding this comment.
Would be nice if there was an easy way to mock the datetime in PHP, without third party libraries.
| if ( ! $dispute['due_by'] ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This shouldn't happen, but worth being defensive here.
| */ | ||
| private function get_disputes_awaiting_response_count() { | ||
| $disputes_status_counts = \WC_Payments::get_database_cache()->get( Database_Cache::DISPUTE_STATUS_COUNTS_KEY ); | ||
| private function get_disputes_needing_response_within_days( $num_days ) { |
There was a problem hiding this comment.
If this task is rendered, this function is called six times, four times with 7 days, two with 1 day. This isn't too much overhead as we will be using the database cache.
But as the code does the same filtering multiple times it might be a good idea to cache the seven and one day results on the class?
| return sprintf( | ||
| /* translators: %s is time, eg: 11:59 PM */ | ||
| __( 'Respond today by %s', 'woocommerce-payments' ), | ||
| $due_by_local_time->format( 'g:i A' ) |
There was a problem hiding this comment.
this should be formatted using the time_format option. Likely wrapped with wc_format_datetime
There was a problem hiding this comment.
Ah, TIL!
I'm thinking that these formats on the Overview task will have to be changes to use the time_format/date_format options as well:
woocommerce-payments/client/overview/task-list/dispute-tasks.ts
Lines 153 to 172 in ff1996b
There was a problem hiding this comment.
Updated in f4bb038. I'll look at incorporating the store's formatting for the Overview task as well.
I couldn't find a single case where the frontend date is formatted using the store's date formatting, so I'll leave it for now.
This will result in inconsistencies across the 2 dispute task items, but it won't impact our goal of highlighting disputes, so happy to leave it.
Co-authored-by: bruce aldridge <[email protected]>
Co-authored-by: bruce aldridge <[email protected]>
Co-authored-by: bruce aldridge <[email protected]>
|
@brucealdridge I think I've addressed all of your suggestions. Thanks for the thorough review, some TIL's in there for me. Marking as ready for re-review 🚀 |
Fixes #6505
Changes proposed in this Pull Request
This PR updates the WC → Home dispute resolution task with the same wording as the Payments → Overview dispute resolution task (see client/overview/task-list/dispute-tasks.ts). The task will now only appear when at least 1 dispute is due within 7 days.
Wc Home Task implemented in this PR:
I've also fixed some bugs with the Payments → Overview task that slipped through:
due_bydatetimes with the current UTC datetime when determining within 1 or 7 days.forto %d of the disputes'.Single/multiple disputes with > 7 days until due
Single dispute with < 7 days until due
Respond to a dispute for $123.00By Jun 30, 2023 – 1 week left to respondSingle dispute with < 24 hours until due
Respond to a dispute for $123.00 – Last dayRespond today by 11:59 PMMultiple disputes with at least one due within 7 days
Respond to N active disputes for a total of $123.00, €123.00Last week to respond to N of the disputesMultiple disputes with at least one due within 24 hours
Respond to N active disputes for a total of $123.00, €123.00Final day to respond to N of the disputesTesting instructions
Mocking disputes
Unfortunately, testing this organically requires waiting 7+ days after creating a disputed transaction.
I was able to mock disputes for both Payments → Overview and WC → Home by insertings the following code to the respective get_* functions and adjusting
due_bydates as necessary.woocommerce-payments/includes/core/server/class-request.php
Line 252 in 7a64f27
woocommerce-payments/includes/wc-payment-api/class-wc-payments-api-client.php
Line 538 in 7a64f27
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