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.

Sign up for GitHub

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

Reporting - Match Payment widget numbers to the totals seen in reports #9162

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Reporting - Match Payment widget numbers to the totals seen in reports #9162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c3ec48
Payment widget dates to wp store date
Jul 25, 2024
bebe14e
Changelog
Jul 25, 2024
efd5add
Merge branch 'develop' into fix/server-5934-payment-widget-data-mismatch
haszari Jul 25, 2024
4c6dcae
Remove unwanted use statement
Jul 26, 2024
dbfaffd
Add site timezone date offset to links
Jul 26, 2024
d4d90de
Update tests.
Jul 26, 2024
300bea0
Merge branch 'develop' into fix/server-5934-payment-widget-data-mismatch
Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions changelog/fix-server-5934-payment-widget-data-mismatch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fix
Comment: Make the numbers on the payment activity widget match the totals on the clicked detailed reports screen.


38 changes: 26 additions & 12 deletions client/components/payment-activity/payment-activity-data.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ const PaymentActivityDataComponent: React.FC< Props > = ( {
const disputes = paymentActivityData?.disputes ?? 0;
const refunds = paymentActivityData?.refunds ?? 0;
const currency = paymentActivityData?.currency;
const siteTimeZone = wcSettings.admin.timeZone;
// We need to add a time offset to the date range to ensure the correct dates are passed on to the transactions report via the view report link.

return (
<div className="wcpay-payment-activity-data">
Expand Down Expand Up @@ -114,12 +116,12 @@ const PaymentActivityDataComponent: React.FC< Props > = ( {
path: '/payments/transactions',
filter: 'advanced',
store_currency_is: currency,
'date_between[0]': moment(
paymentActivityData?.date_start
).format( 'YYYY-MM-DD' ),
'date_between[1]': moment(
paymentActivityData?.date_end
).format( 'YYYY-MM-DD' ),
'date_between[0]': moment( paymentActivityData?.date_start )
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
'date_between[1]': moment( paymentActivityData?.date_end )
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
...getSearchParams(
searchTermsForViewReportLink.totalPaymentVolume
),
Expand Down Expand Up @@ -159,10 +161,14 @@ const PaymentActivityDataComponent: React.FC< Props > = ( {
store_currency_is: currency,
'date_between[0]': moment(
paymentActivityData?.date_start
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
'date_between[1]': moment(
paymentActivityData?.date_end
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
...getSearchParams(
searchTermsForViewReportLink.charge
),
Expand All @@ -182,10 +188,14 @@ const PaymentActivityDataComponent: React.FC< Props > = ( {
store_currency_is: currency,
'date_between[0]': moment(
paymentActivityData?.date_start
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
'date_between[1]': moment(
paymentActivityData?.date_end
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
...getSearchParams(
searchTermsForViewReportLink.refunds
),
Expand Down Expand Up @@ -232,10 +242,14 @@ const PaymentActivityDataComponent: React.FC< Props > = ( {
store_currency_is: currency,
'date_between[0]': moment(
paymentActivityData?.date_start
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
'date_between[1]': moment(
paymentActivityData?.date_end
).format( 'YYYY-MM-DD' ),
)
.add( siteTimeZone )
.format( 'YYYY-MM-DD' ),
...getSearchParams(
searchTermsForViewReportLink.dispute
),
Expand Down
10 changes: 10 additions & 0 deletions client/components/payment-activity/test/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ declare const global: {
country: string;
};
};
wcSettings: {
admin: {
timeZone: string;
};
};
};

describe( 'PaymentActivity component', () => {
Expand Down Expand Up @@ -118,6 +123,11 @@ describe( 'PaymentActivity component', () => {
Date.now = jest.fn( () =>
new Date( '2024-04-08T12:33:37.000Z' ).getTime()
);
global.wcSettings = {
admin: {
timeZone: 'UTC',
},
};
} );

afterEach( () => {
Expand Down
1 change: 1 addition & 0 deletions client/globals.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ declare global {
woocommerce_coming_soon: string;
woocommerce_private_link: string;
};
timeZone: string;
};
adminUrl: string;
countries: Record< string, string >;
Expand Down
23 changes: 20 additions & 3 deletions includes/admin/class-wc-rest-payments-reporting-controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

use WCPay\Core\Server\Request\Get_Reporting_Payment_Activity;
use WCPay\Core\Server\Request\Request_Utils;

defined( 'ABSPATH' ) || exit;

Expand Down Expand Up @@ -44,11 +45,27 @@ public function register_routes() {
* @param WP_REST_Request $request The request.
*/
public function get_payment_activity( $request ) {
$wcpay_request = Get_Reporting_Payment_Activity::create();
$wcpay_request->set_date_start( $request->get_param( 'date_start' ) );
$wcpay_request->set_date_end( $request->get_param( 'date_end' ) );
$wcpay_request = Get_Reporting_Payment_Activity::create();
$date_start_in_store_timezone = $this->format_date_to_wp_timezone( $request->get_param( 'date_start' ) );
$date_end_in_store_timezone = $this->format_date_to_wp_timezone( $request->get_param( 'date_end' ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this code is undoing logic in Transactions_List request class:

woocommerce-payments/includes/core/server/request/class-list-transactions.php

Lines 80 to 81 in bebe14e

'date_before' => Request_Utils::format_transaction_date_by_timezone( $request->get_param( 'date_before' ), $user_timezone ),
'date_after' => Request_Utils::format_transaction_date_by_timezone( $request->get_param( 'date_after' ), $user_timezone ),

Should we fix it at the source, i.e. delete that code, rather than reversing it here?

whereas those on the transactions report pages with filter ( used for landing pages ),used an adjustment to match the site settings.

I looked at the other similar request classes and didn't see any doing this timezone adjustment. If Transactions_List is inconsistent then we should fix that. We just need to find out:

  1. If anyone wants/expects special / current behaviour of Transactions_List, i.e. convert to site timezone.
  2. If there is explanation or rationale in GitHub history for this change.

I started digging in git-blame to find out when this conversion was added. It looks like it was long ago, it was there when these pagination controllers were implemented for previous integration/platform project:

So 🤞🏻 maybe it's just an old oversight/bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that fixing the main cause ( option (a) is the ideal solution, compared to adding another workaround. Since we have less time now, it would be good to go with option b for now , and create a separate issue where we check

  • Why was the timezone adjustment added to Transactions_List
  • Downstream implications to other applications / workflows if we remove this adjustment
  • Removing time adjustment proposed in the current PR and any other similar changes done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that we don't have to ship an intermediate patch (aka "b"). Yes there is urgency to wrap up this project, but there is no hard deadline. If we are adding follow up issues, then that undermines this as a way to wrap up the project.

That said I'm not going to stand in the way of a temporary/short term patch if we're confident it's safe. Logging an issue to investigate fix "a" properly is a bottom line for me … but I am concerned it will languish given other pressures.

$wcpay_request->set_date_start( $date_start_in_store_timezone );
$wcpay_request->set_date_end( $date_end_in_store_timezone );
$wcpay_request->set_timezone( $request->get_param( 'timezone' ) );
$wcpay_request->set_currency( $request->get_param( 'currency' ) );
return $wcpay_request->handle_rest_request();
}



/**
* Formats a date string to the WordPress timezone.
*
* @param string $date_string The date string to be formatted.
* @return string The formatted date string in the 'Y-m-d\TH:i:s' format.
*/
private function format_date_to_wp_timezone( $date_string ) {
$date = Request_Utils::format_transaction_date_by_timezone( $date_string, '+00:00' );
$date = new DateTime( $date );
return $date->format( 'Y-m-d\\TH:i:s' );
}
}
Loading