Fix pagination issue with dispute count in task list on overview page

[shendy-a8c]

Fixes #4123.

Changes proposed in this Pull Request

On Payments > Overview page, we display a task to open the awaiting response dispute list under the Things to do list. Before this PR, we're getting a list of disputes and count the ones that need response to get the number in the task. This PR takes a different approach by calling a REST API to get a list of dispute statuses and their counts, solving the problem where awating response disputes got buried in the page 2+.

Testing instructions

0. Make sure to enable Enable account overview task list in WCPay Dev because the task list / 'Things to do' is behind a feature flag.

1. You will need >25 disputed WCPay transactions. Do this by purchasing products multiple times with either of the following cards (tip: save the cards when checking out to reduce repetitive card number form inputs) :

   • 4000000000001976
   • 4000000000000259

2. Checkout the trunk branch.

3. Visit Payments → Overview and observe the disputes task in the Things to do task list. Notice that the # of disputed payments is incorrect. An easy way to check is to compare with the Disputes badge in the admin menu:

   

4. Checkout this PR branch fix/4123-dispute-count and run npm run build:client

5. Visit Payments → Overview and observe the disputes task in the Things to do task list. Notice that the # of disputed payments correct:

   

Notes

• Use nodejs 14 as I find problems running npm run test:js locally with nodejs 16.

---

• Run npm run changelog to add a changelog file, choose patch to 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.
• Covered with tests (or have a good reason not to test in description ☝️)
• Tested on mobile (or does not apply)

Post merge

• Link to testing instructions from release testing doc following these instructions : Add link here / 'QA Testing Not Applicable'
• Add or update critical flows and testing instructions for critical flows, if applicable.
• Add what's changed (description, screenshot, demo videos etc.) to the release announcement post, if applicable.

[haszari]

This looks good @shendy-a8c - we'll need a useDisputeStatusCounts custom hook to expose this data to the UI and get it in the task list - is that in line with your plan? E.g. replace useDisputes in OverviewPage :

woocommerce-payments/client/overview/index.js

Line 38 in a661754

const { disputes, isLoading } = useDisputes( getQuery() );

[shendy-a8c]

is that in line with your plan?

Yes @haszari. Thank you for making sure.

[shendy-a8c]

Thank you @Jinksi for adding testing instructions in this PR's description. Very much appreciated!

[shendy-a8c]

I want to point out that some changes in this PR:

• Fix to the grammar error for sentence disputed payments needs your response
• Linking to disputes list with filter=awaiting_response

Are also fixed in #4414. The plan is to merge #4414 and rebase this PR on to develop afterward. Therefore, I changed the original changelog entry for this PR in 5d3dde6 to reflect that.

Also, I wonder, since the task list is behind a feature flag, customers haven't seen it yet. I have a doubt that we should add any changelog entry but decided to add something. Let me know if we shouldn't include a changelog entry.

[Jinksi]

Use nodejs 14 as I find problems running npm run test:js locally with nodejs 16.

@shendy-a8c I've been testing using node 12.22.6 as per .nvmrc and npm run test:js passes ✅

woocommerce-payments/.nvmrc

Line 1 in 496176b

12.22.6

[shendy-a8c]

I try not to change the behavior of the task list, ie:

• The dispute task list will appear if there is at least 1 dispute, regardless of their status (ref)
• We use the number of disputes needing response to decide the translated string to be singular or plural. However, the number we show in the sentence can be the number of total disputes if there is 0 disputes needing response. I find that is buggy. If the number of disputes needing response is 0 and the total number of disputes is 5, it will say disputed payments need your response. Doesn't sound right.

[shendy-a8c]

I've been testing using node 12.22.6 as per .nvmrc and npm run test:js passes ✅

Ah. Thank you for pointing that out. I should have used the same node version as defined in .nvmrc.

[shendy-a8c]

Actually, I fix what I explained above with 18900e2.

[shendy-a8c]

This PR is ready for a review.

[Jinksi]

• The dispute task list will appear if there is at least 1 dispute, regardless of their status (ref)

@shendy-a8c I feel like this behaviour should change only to show the disputes task if there are disputes awaiting a response. Therefore, the task wouldn't show as completed or be "completable", it would simply not appear.

Merchants with a large number of disputes (of any status) could see an enormous value in the completed task message, e.g. ✅ 17689 disputed payments need your response.

But this might be out of the scope of this PR, what do you think?

[shendy-a8c]

Good point. Do you think we need to loop in product or design person or just call it? Your argument makes sense though. The minor drawback (or maybe not even a drawback) is that we will never have completed dispute-related task. It''d be either showing when incomplete or not showing at all when completed.

[Jinksi]

Do you think we need to loop in product or design person or just call it? Your argument makes sense though.

If you agree, I'm leaning towards calling it. The large number in the completed task could be seen as a bug 🤔

Most other tasks in this list are automatically hidden when completed too, e.g.

woocommerce-payments/client/overview/task-list/tasks.js

Lines 85 to 90 in 18900e2

	content: __(
	'WooCommerce Payments is missing a connected WordPress.com account. ' +
	'Some functionality will be limited without a connected account.',
	'woocommerce-payments'
	),
	completed: false,

To clarify, I think that the disputes task should only display if there are one or more disputes awaiting a response. This is also the approach used in the WC → Home disputes task PR.

[Jinksi]

I've found another bug if we keep the task in a completed state: clicking it directs me to the disputes page with the Needs Response filter active, which is an empty table.

[shendy-a8c]

Sounds good. I have rebased on to develop and made the change so dispute task only appears when there is one to respond.

[Jinksi]

I've stumbled upon this Fatal Error when upgrading a JN site from WCPay 4.4.0 to this branch (zipped via npm run build and uploaded via wp-admin plugin installer). It only appears once. When navigating away the plugin works as expected.

Fatal error: Uncaught Error: Too few arguments to function WC_REST_Payments_Disputes_Controller::__construct(), 1 passed in /srv/users/user514dd149/apps/user514dd149/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php on line 664 and exactly 2 expected
in /srv/users/user514dd149/apps/user514dd149/public/wp-content/plugins/woocommerce-payments/includes/admin/class-wc-rest-payments-disputes-controller.php on line 37

Maybe $database_cache hasn't instantiated yet when this line is called?

woocommerce-payments/includes/class-wc-payments.php

Line 664 in a06d6a1

$disputes_controller = new WC_REST_Payments_Disputes_Controller( self::$api_client, self::$database_cache );

[Jinksi]

@shendy-a8c I have a fix for the Fatal Error mentioned in #4388 (comment).

I opened a parallel branch (fix/4123-dispute-count-2), but if the changes look good to you, I can commit the changes to this branch.

[shendy-a8c]

@Jinksi I took a look at the fatal error. Sorry for messing with your JN site but I can now also reproduce the problem consistently in a separate JN site. I find these conditions need to be met to get the fatal error:

• Onboard WCPay. I find that if store isn't connected, I don't get the fatal error.
• A WCPay version that is active and installed needs to be one that initializes WC_REST_Payments_Disputes_Controller without the 2nd argument.
• A WCPay version that is about to be installed and activated needs to be one that initializes WC_REST_Payments_Disputes_Controller with the 2nd argument.

I don't think the problem is caused by a null $database_cache because $database_cache is initialized before even init_rest_api() is hooked.

I think somehow the old version of class-wc-payments.php is executed in the runtime while the class-wc-rest-payments-disputes-controller.php has been updated in filesystem.

I modified develop branch this way:

try {
    $disputes_controller = new WC_REST_Payments_Disputes_Controller( self::$api_client );
    $disputes_controller->register_routes();
} catch (Throwable $e) {
    ob_start();
    var_dump('init WC_REST_Payments_Disputes_Controller without the 2nd argument');
    debug_print_backtrace();
    file_put_contents(dirname(WCPAY_ABSPATH) . '/debug.log', ob_get_clean() . "\n", FILE_APPEND);
}

Then npm run build and rename the zip file to woocommerce-payments-1.zip.

Then, I modify this branch:

try {
    $disputes_controller = new WC_REST_Payments_Disputes_Controller( self::$api_client, self::$database_cache );
    $disputes_controller->register_routes();
} catch (Throwable $e) {
    ob_start();
    var_dump('init WC_REST_Payments_Disputes_Controller with the 2nd argument');
    debug_print_backtrace();
    file_put_contents(dirname(WCPAY_ABSPATH) . '/debug.log', ob_get_clean() . "\n", FILE_APPEND);
}

npm run build it and rename the zip file to woocommerce-payments-2.zip.

I then install woocommerce-payments-1.zip and upgrade to woocommerce-payments-2.zip.

I see some logging in wp-content/plugins/debug.log from woocommerce-payments-1.zip's code, not from woocommerce-payments-2.zip.

Here's the log:

string(62) "init WC_REST_Payments_Disputes_Controller without the 2nd argument"
#0  WC_Payments::init_rest_api(WP_REST_Server Object (*redacted too long*)) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/class-wp-hook.php:307]
#1  WP_Hook->apply_filters(, Array ([0] => WP_REST_Server Object (*redacted too long*)) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/class-wp-hook.php:331]
#2  WP_Hook->do_action(Array ([0] => WP_REST_Server Object (*redacted too long*)) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/plugin.php:476]
#3  do_action(rest_api_init, WP_REST_Server Object (*redacted too long*)) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/rest-api.php:561]
#4  rest_get_server() called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/rest-api.php:519]
#5  rest_do_request(WP_REST_Request Object (*redacted too long*) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/rest-api.php:2868]
#6  rest_preload_api_request(Array (), /wc-analytics/reports/performance-indicators/allowed)
#7  array_reduce(Array ([0] => /wc-analytics/reports/performance-indicators/allowed,[1] => /wc-analytics/leaderboards/allowed,[2] => /jetpack/v4/connection), rest_preload_api_request) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-content/plugins/woocommerce/src/Internal/Admin/Settings.php:146]
#8  Automattic\WooCommerce\Internal\Admin\Settings->add_component_settings(Array ([orderStatuses] => Array ([pending] => Pending payment,[processing] => Processing,[on-hold] => On hold,[completed] => Completed,[cancelled] => Cancelled,[refunded] => Refunded,[failed] => Failed,[checkout-draft] => Draft),[stockStatuses] => Array ([instock] => In stock,[outofstock] => Out of stock,[onbackorder] => On backorder),[currency] => Array ([code] => USD,[precision] => 2,[symbol] => $,[symbolPosition] => left,[decimalSeparator] => .,[thousandSeparator] => ,,[priceFormat] => %1$s%2$s),[locale] => Array ([siteLocale] => en_US,[userLocale] => en_US,[weekdaysShort] => Array ([0] => Sun,[1] => Mon,[2] => Tue,[3] => Wed,[4] => Thu,[5] => Fri,[6] => Sat)))) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/class-wp-hook.php:307]
#9  WP_Hook->apply_filters(Array (), Array ([0] => Array ())) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/plugin.php:191]
#10 apply_filters(woocommerce_admin_shared_settings, Array ()) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-content/plugins/woocommerce/src/Internal/Admin/WCAdminSharedSettings.php:61]
#11 Automattic\WooCommerce\Internal\Admin\WCAdminSharedSettings->Automattic\WooCommerce\Internal\Admin\{closure}() called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-content/plugins/woocommerce/packages/woocommerce-blocks/src/Assets/AssetDataRegistry.php:258]
#12 Automattic\WooCommerce\Blocks\Assets\AssetDataRegistry->execute_lazy_data() called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-content/plugins/woocommerce/packages/woocommerce-blocks/src/Assets/AssetDataRegistry.php:359]
#13 Automattic\WooCommerce\Blocks\Assets\AssetDataRegistry->enqueue_asset_data() called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/class-wp-hook.php:307]
#14 WP_Hook->apply_filters(, Array ([0] => )) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/class-wp-hook.php:331]
#15 WP_Hook->do_action(Array ([0] => )) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-includes/plugin.php:476]
#16 do_action(admin_print_footer_scripts) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-admin/admin-footer.php:95]
#17 require_once(/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-admin/admin-footer.php) called at [/srv/users/user9e9aa2cc/apps/user9e9aa2cc/public/wp-admin/update.php:182]

[shendy-a8c]

Regarding your fix, it does fix the problem but it's moving away from dependency injection approach.

What do you think if we make the 2nd argument of WC_REST_Payments_Disputes_Controller, the database_cache, optional? This has to be followed with a null check in get_dispute_status_counts() before calling $this->database_cache->get_or_add().

[shendy-a8c]

Looks like similar case happened before when the constructor arguments of classes loaded from class-wc-payments.php change.

This reported problem is caused by this change.

Not sure if it's a problem specific to WCPay.

[shendy-a8c]

I tried to reproduce by creating a simple WordPress plugin and it's not behaving the same. That and the fact it only happens if store is already onboarded, I think this problem is specific to WCPay and not a problem within WP's plugin updater, so I filed a new issue.

[james-allan]

Apologies if this is throwing a large spanner in the works late into the piece but would it be simpler to inject the number of disputes into the JS rather than fetch it via an API request?

For example these changes seem to be working for me locally.

diff --git a/client/overview/index.js b/client/overview/index.js
index 551adb1d..99c5ea6e 100644
--- a/client/overview/index.js
+++ b/client/overview/index.js
@@ -34,6 +34,7 @@ const OverviewPage = () => {
                wpcomReconnectUrl,
                featureFlags: { accountOverviewTaskList },
                needsHttpsSetup,
+               disputesNeedingResponse,
        } = wcpaySettings;
        const { disputes, isLoading } = useDisputes( getQuery() );
        const { isLoading: settingsIsLoading, settings } = useSettings();
@@ -43,7 +44,7 @@ const OverviewPage = () => {
                showUpdateDetailsTask,
                wpcomReconnectUrl,
                needsHttpsSetup,
-               disputes,
+               disputesNeedingResponse,
        } );
        const tasks =
                Array.isArray( tasksUnsorted ) && tasksUnsorted.sort( taskSort );
diff --git a/client/overview/task-list/tasks.js b/client/overview/task-list/tasks.js
index 8d892e46..4bf8f54d 100644
--- a/client/overview/task-list/tasks.js
+++ b/client/overview/task-list/tasks.js
@@ -31,15 +31,15 @@ export const getTasks = ( {
        wpcomReconnectUrl,
        isAccountOverviewTasksEnabled,
        needsHttpsSetup,
-       disputes = [],
+       disputesNeedingResponse,
 } ) => {
        const { status, currentDeadline, pastDue, accountLink } = accountStatus;
        const accountRestrictedSoon = 'restricted_soon' === status;
        const accountDetailsPastDue = 'restricted' === status && pastDue;
        let accountDetailsTaskDescription;

-       const isDisputeTaskVisible = 0 < disputes.length;
-       const disputesToResolve = getDisputesToResolve( disputes );
+       const isDisputeTaskVisible = 0 < disputesNeedingResponse;
+       const disputesToResolve = disputesNeedingResponse;

        if ( accountRestrictedSoon ) {
                accountDetailsTaskDescription = sprintf(
@@ -138,7 +138,7 @@ export const getTasks = ( {
                                        disputesToResolve,
                                        'woocommerce-payments'
                                ),
-                               disputesToResolve ? disputesToResolve : disputes.length
+                               disputesToResolve ? disputesToResolve : disputesNeedingResponse
                        ),
                        additionalInfo: disputesToResolve
                                ? __( 'View and respond', 'woocommerce-payments' )
diff --git a/includes/admin/class-wc-payments-admin.php b/includes/admin/class-wc-payments-admin.php
index 9bd855eb..0833dab3 100644
--- a/includes/admin/class-wc-payments-admin.php
+++ b/includes/admin/class-wc-payments-admin.php
@@ -490,6 +490,7 @@ class WC_Payments_Admin {
                        'currentUserEmail'        => $current_user_email,
                        'currencyData'            => $currency_data,
                        'restUrl'                 => get_rest_url( null, '' ), // rest url to concatenate when merchant use Plain permalinks.
+                       'disputesNeedingResponse' => $this->get_disputes_awaiting_response_count(),
                ];

                wp_localize_script(

[shendy-a8c]

Thank you so much for the fresh approach from totally different angle, @james-allan. It does simplify things a lot. Also, as a bonus, we can avoid the fatal error talked about above! Also, it felt so good avoiding adding bunch of new lines.

One little tiny drawback with this approach is that status counts will be passed on every WCPay menu page loads, not just the Overview page where it's used, which is fine because it's cached data.

Another thing I found out is that wp_localize_script() will convert any value to string here, so wcpaySettings.numDisputesNeedingResponse will be '4' instead of 4 for example. I am tempted to convert that using Number() in the JS (and have to handle if value is NaN, etc) but I thought since there is no strict comparisons of that variable, I won't over-engineer it. Let me know if I need to do all that though.

I rebased onto develop and this PR is ready for another review.

[james-allan]

This works for me.

I:

1. Confirmed the paging issue on develop where more than 25 disputes needing a response results in an incorrect number.
2. Confirmed this branch fixed it.
3. Tested accepting a dispute and it being updated etc.

One other hidden benefit of this count being cached and injected in to the JS, is that the task loads instantly on the page, on develop, the task list was pretty slow to render because of all the querying and filtering going on.

💯

[Jinksi]

💯 🥇 I've tested upgrading from WCPay 4.4.0 and the Fatal Error is no longer a concern.

I've also tested the task when 2, 1 and 0 disputes are awaiting a response, all working as expected.