Skip to content
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

Performance improvements for disputes reminder task list #9906

Merged
merged 8 commits into from
Dec 17, 2024
4 changes: 4 additions & 0 deletions changelog/fix-9716-disputes-api-requests
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: fix

Performance improvements for Disputes Needing Response task shown in WooCommerce admin.
1 change: 1 addition & 0 deletions includes/admin/class-wc-payments-admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

*
* @return int The number of disputes which need a response.
*/
Expand Down
42 changes: 32 additions & 10 deletions includes/admin/tasks/class-wc-payments-task-disputes.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ class WC_Payments_Task_Disputes extends Task {
*/
private $disputes_due_within_1d;

/**
* A memory cache of all disputes needing response.
*
* @var array|null
*/
private $disputes_needing_response = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* WC_Payments_Task_Disputes constructor.
*/
Expand All @@ -57,13 +64,12 @@ public function __construct() {
$this->api_client = \WC_Payments::get_payments_api_client();
$this->database_cache = \WC_Payments::get_database_cache();
parent::__construct();
$this->init();
}

/**
* Initialize the task.
*/
private function init() {
private function fetch_relevant_disputes() {
$this->disputes_due_within_7d = $this->get_disputes_needing_response_within_days( 7 );
$this->disputes_due_within_1d = $this->get_disputes_needing_response_within_days( 1 );
}
Expand All @@ -83,6 +89,9 @@ public function get_id() {
* @return string
*/
public function get_title() {
if ( null === $this->disputes_needing_response ) {
$this->fetch_relevant_disputes();
}
if ( count( (array) $this->disputes_due_within_7d ) === 1 ) {
$dispute = $this->disputes_due_within_7d[0];
$amount = WC_Payments_Utils::interpret_stripe_amount( $dispute['amount'], $dispute['currency'] );
Expand Down Expand Up @@ -275,6 +284,9 @@ public function is_complete() {
* @return bool
*/
public function can_view() {
if ( null === $this->disputes_needing_response ) {
$this->fetch_relevant_disputes();
}
return count( (array) $this->disputes_due_within_7d ) > 0;
}

Expand Down Expand Up @@ -322,15 +334,24 @@ private function get_disputes_needing_response_within_days( $num_days ) {
* @return array|null Array of disputes awaiting a response. Null on failure.
*/
private function get_disputes_needing_response() {
return $this->database_cache->get_or_add(
if ( null !== $this->disputes_needing_response ) {
return $this->disputes_needing_response;
}

brucealdridge marked this conversation as resolved.
Show resolved Hide resolved
$this->disputes_needing_response = $this->database_cache->get_or_add(
Database_Cache::ACTIVE_DISPUTES_KEY,
function () {
$response = $this->api_client->get_disputes(
[
'pagesize' => 50,
'search' => [ 'warning_needs_response', 'needs_response' ],
]
);
try {
$response = $this->api_client->get_disputes(
[
'pagesize' => 50,
'search' => [ 'warning_needs_response', 'needs_response' ],
]
);
} catch ( \Exception $e ) {
// Ensure an array is always returned, even if the API call fails.
return [];
}

$active_disputes = $response['data'] ?? [];

Expand All @@ -347,8 +368,9 @@ function ( $a, $b ) {

return $active_disputes;
},
// We'll consider all array values to be valid as the cache is only invalidated when it is deleted or it expires.
'is_array'
);

return $this->disputes_needing_response;
}
}
7 changes: 5 additions & 2 deletions includes/class-wc-payments-tasks.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ class WC_Payments_Tasks {
* WC_Payments_Admin_Tasks constructor.
*/
public static function init() {
include_once WCPAY_ABSPATH . 'includes/admin/tasks/class-wc-payments-task-disputes.php';
if ( ! current_user_can( 'manage_woocommerce' ) ) {
marcinbot marked this conversation as resolved.
Show resolved Hide resolved
return;
}

add_action( 'init', [ __CLASS__, 'add_task_disputes_need_response' ] );
}
Expand All @@ -31,9 +33,10 @@ public static function init() {
*/
public static function add_task_disputes_need_response() {
$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() ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_account_under_review() and is_account_rejected also check is_stripe_connected

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be explained in a comment - i.e. define the preconditions in merchant/human terms :)

return;
}
include_once WCPAY_ABSPATH . 'includes/admin/tasks/class-wc-payments-task-disputes.php';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 'extended' = 'Things to do next' task list on WooCommerce > Home.
TaskLists::add_task( 'extended', new WC_Payments_Task_Disputes() );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public function set_up() {
$this->_cache = WC_Payments::get_database_cache();
$this->mock_cache = $this->createMock( WCPay\Database_Cache::class );
WC_Payments::set_database_cache( $this->mock_cache );
WC_Payments_Tasks::add_task_disputes_need_response(); // ensure the task is included and initialized.
}

public function tear_down() {
Expand Down
Loading