Performance improvements for disputes reminder task list #9906
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: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Left some suggestions, in line with the P2, while also keeping in mind that this work might end up not to be needed depending on product decision.
Also, we could remove the side effect from the WC_Payments_Task_Disputes constructor and have the render calls try to fetch on demand, now that we have in-memory caching.
| $account_service = WC_Payments::get_account_service(); | ||
| if ( ! $account_service || ! $account_service->is_stripe_account_valid() ) { | ||
| if ( ! $account_service || ! $account_service->is_stripe_account_valid() || $account_service->is_account_under_review() || $account_service->is_account_rejected() ) { |
There was a problem hiding this comment.
is_account_under_review() and is_account_rejected also check is_stripe_connected
There was a problem hiding this comment.
This could also be explained in a comment - i.e. define the preconditions in merchant/human terms :)
There was a problem hiding this comment.
Did a quick review of the code, I haven't tested (feel free to ping me again – I'm happy to test as well if needed).
This is looking good, and has had pretty thorough review from @marcinbot also 🙌
My feedback is all minor, around naming and leaving breadcrumbs about how and why we optimised this. I'm worried that in future someone might refactor things and unintentionally impact performance!
The internal discussion pdjTHR-4rD-p2 should be considered alongside these changes.
@brucealdridge Can you add (or summarise/rewrite) the relevant details from that P2, so the PR is self contained?
Due to the way that the API requests were cached, this only affected a small number of merchants.
What conditions triggered the bug? That's essential info for the PR :)
When I read the final code (or the PR description) I don't see any clues about which specific code is slow (potentially under special-case conditions), and our new design to trigger that code much later. So my general feedback is to …
- Rename any "slow" methods so it's clear that they are expensive. Some are already named
fetch_which is a good clue, but this clue would be useful further up too. For exampleWC_Payments_Task_Disputes::init()=>WC_Payments_Task_Disputes::fetch_relevant_disputes(), I agree with Marcin feedback. - Add comments to slow/expensive methods that document the edge cases/bad performance cases we've seen. E.g. don't try to fetch disputes for rejected (etc) accounts.
Generally make it easy for readers of this code (or the PR) to understand the hard-won performance improvements you've implemented 🙌
| @@ -1336,6 +1336,7 @@ public function add_transactions_notification_badge() { | |||
|
|
|||
| /** | |||
| * Gets the number of disputes which need a response. ie have a 'needs_response' or 'warning_needs_response' status. | |||
| * Used to display a notification badge on the Payments > Disputes menu item. | |||
There was a problem hiding this comment.
Nice to add this breadcrumb. Maybe even could add a "see also" linking to the code that calls this? (if you think that would be helpful)
| * | ||
| * @var array|null | ||
| */ | ||
| private $disputes_needing_response = null; |
There was a problem hiding this comment.
Would be good to clarify this a little more:
- What is the type of the array? i.e. clarify that this is an array of dispute objects, not a count, or array of ids.
- What's the lifetime of this, when is it relevant (and when might it be "stale")? Since it is a
"snapshot", i.e. using a class member as a cache/temporary storage. I imagine this is per-request as is common in Woo/PHP web code, but it's good to be explicit.
| @@ -275,6 +284,10 @@ public function is_complete() { | |||
| * @return bool | |||
| */ | |||
| public function can_view() { | |||
| if ( null === $this->disputes_needing_response ) { | |||
| // init() is called on can_view as this is the first method called in the task lifecycle by WC. | |||
There was a problem hiding this comment.
If we rename init based on what it does – i.e. fetch stuff so we have the data we need – then this comment can be clearer.
These test instructions are happy-path only, but this fix is for various edge cases. Should we be testing the bad performance edge case too? Please add reproduce steps for at least one of the bad-performance cases.
Are these the scenarios that were causing the bug – would it be useful to test with e.g. a rejected WooPayments account (etc)? (Assuming we can fake this with local server) |
|
I approved this, but it looks like we'll need to fix the tests too. Once that's done, feel free to |
| return; | ||
| } | ||
| include_once WCPAY_ABSPATH . 'includes/admin/tasks/class-wc-payments-task-disputes.php'; |
There was a problem hiding this comment.
Thanks for clarifying the details of the problem and how you've optimised it @brucealdridge ! 🚀
I think it's worth adding a comment in this function about why the include and task is deferred, i.e. leave breadcrumbs in the code about the performance problem.
Fixes #9716
Changes proposed in this Pull Request
Performance improvements for the WC Admin Disputes Reminder Task.
Previously this feature could cause some errored accounts (and some non-erroring accounts) to make excessive calls to the Server's Disputes API. Due to the way that the API requests were cached, this only affected a small number of merchants.
We believe this occurred when the Account was Rejected or Suspended, the call to
GET disputeswould error preventing caching.Improvements:
manage_woocommercepermissionAdditional Context
pdjTHR-4rD-p2
Testing instructions
Additional Notes:
DELETE FROM wp_options WHERE option_name='wcpay_active_dispute_cache';wcpay_active_dispute_cacheoption to end in"errored";b:1;}(ie. errored=true)fetchedvalue inwcpay_active_dispute_cachenpm 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