Convert Paypal Standard IPN payment_date to system's time zone #13439

jgillmanjr · 2019-01-12T05:58:00Z

Overview

Convert payment_date received with the Paypal Standard IPN to the system's time zone

Before

Currently, payment_date is set to (presumably) the local time for Paypal (a Pacific time zone of PST or PDT is part of the timestamp). This leads to the wrong time being written to the database.

After

The timestamp is adjusted to the system's time zone.

Technical Details

Relatively straight forward fix.

Just build a DateTimeZone object for the system's TZ and convert the $receiveDateTime objecting using the setTimezone method.

civicrm-builder · 2019-01-12T05:58:02Z

Can one of the admins verify this patch?

civibot · 2019-01-12T05:58:02Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

seamuslee001 · 2019-01-12T06:31:12Z

Jenkins ok to test

seamuslee001 · 2019-01-12T06:32:17Z

CRM/Core/Payment/PayPalIPN.php

@@ -394,6 +394,8 @@ public function getInput(&$input, &$ids) {
    $paymentDate = $this->retrieve('payment_date', 'String', FALSE);
    if (!empty($paymentDate)) {
      $receiveDateTime = new DateTime($paymentDate);


won't you have to specify that its in Pacific time on creation or is it doing that sensibly already? Also do we need to do this for PayPalPro IPNs as well?

This is from one of the test IPN messages: payment_date=22:08:43 Jan 11, 2019 PST
But yeah, if the time zone wasn't supplied, this wouldn't work.

Pro does look to require it as well, but the code structure for that was... a bit different:

https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/PayPalProIPN.php#L318

// CRM-13737 - am not aware of any reason why payment_date would not be set - this if is a belt & braces $objects['contribution']->receive_date = !empty($input['payment_date']) ? date('YmdHis', strtotime($input['payment_date'])) : $now;

and https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/PayPalProIPN.php#L534:

$input['payment_date'] = $input['receive_date'] = self::retrieve('payment_date', 'String', 'POST', FALSE);

Since I admittedly don't have a setup for "properly" testing things, and don't have access (and have never used) PayPal pro, I didn't feel comfortable taking a completely dark stab - especially with payment_date being referenced in two locations.

See also https://developer.paypal.com/docs/classic/ipn/integration-guide/IPNandPDTVariables/#payment-information-variables

CRM/Core/Payment/PayPalIPN.php

mattwire · 2019-01-14T10:28:35Z

CRM/Core/Payment/PayPalIPN.php

@@ -394,6 +394,8 @@ public function getInput(&$input, &$ids) {
    $paymentDate = $this->retrieve('payment_date', 'String', FALSE);
    if (!empty($paymentDate)) {
      $receiveDateTime = new DateTime($paymentDate);
+      $systemTimeZone = new DateTimeZone(CRM_Core_Config::singleton()->userSystem->getTimeZoneString());


@jgillmanjr Can you add a couple of lines of comments into the code here explaining why we need to do the timezone conversion (so anyone looking at this code in the future can work out what is going on and why). Eg. add a sample payment_date as it comes in from paypal etc.

Comments in 292785c

jgillmanjr · 2019-01-14T19:45:53Z

Aaanddd apparently pulling in the latest from master killed the build...

mattwire · 2019-01-14T19:52:26Z

@jgillmanjr You need to do a git "rebase". Something like git rebase -i upstream/master should do it

Co-Authored-By: jgillmanjr <jason@rrfaae.com>

jgillmanjr · 2019-01-14T20:04:21Z

Welp, hopefully that didn't bork things up too bad

mattwire · 2019-01-15T09:51:41Z

I'm happy with this PR from a technical point of view. @pradpnayak Would you be able to test this?

pradpnayak · 2019-01-15T20:14:17Z

@mattwire this is incase of subsequent payment callback for recurring?

seamuslee001 · 2019-01-15T20:15:14Z

@pradpnayak no this is just so that the transaction date that is recorded in CiviCRM is when it actually happened in the System's local time not in PayPal's time

seamuslee001 · 2019-01-15T20:38:22Z

Australian Greens have released this to our production environment and the dates are looking correct for us now @mattwire @pradpnayak @eileenmcnaughton are you folks satisfied this is fine to merge?

seamuslee001 · 2019-01-15T20:38:31Z

Jenkins re test this please

eileenmcnaughton · 2019-01-15T20:41:13Z

@seamuslee001 yes - you have tested it & @mattwire said "I'm happy with this PR from a technical point of view"

seamuslee001 · 2019-01-16T05:17:35Z

Merging based on Matt and I's review

eileenmcnaughton · 2019-06-02T22:36:22Z

@seamuslee001 @jgillmanjr this caused a regression :-( when accessing the ipn through the old path - see https://lab.civicrm.org/dev/drupal/issues/66

civibot bot added the master label Jan 12, 2019

seamuslee001 reviewed Jan 12, 2019

View reviewed changes

CRM/Core/Payment/PayPalIPN.php Outdated Show resolved Hide resolved

mattwire reviewed Jan 14, 2019

View reviewed changes

jgillmanjr and others added 3 commits January 14, 2019 14:56

Convert Paypal Standard IPN payment_date to system's time zone

f045e2e

Utilize getTimeZoneString() method

e878028

Co-Authored-By: jgillmanjr <jason@rrfaae.com>

Quick comment describing need for IPN TZ adjustment

6800eef

jgillmanjr force-pushed the paypal-standard-ipn-tz-fix branch from 292785c to 6800eef Compare January 14, 2019 19:57

seamuslee001 merged commit 1ce4df2 into civicrm:master Jan 16, 2019

eileenmcnaughton mentioned this pull request Jun 2, 2019

dev/core#973 Fix legacy IPN endpoint for Drupal #14272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Paypal Standard IPN payment_date to system's time zone #13439

Convert Paypal Standard IPN payment_date to system's time zone #13439

jgillmanjr commented Jan 12, 2019

civicrm-builder commented Jan 12, 2019

civibot bot commented Jan 12, 2019

seamuslee001 commented Jan 12, 2019

seamuslee001 Jan 12, 2019

jgillmanjr Jan 12, 2019

mattwire Jan 14, 2019

mattwire Jan 14, 2019

jgillmanjr Jan 14, 2019

jgillmanjr Jan 14, 2019

jgillmanjr commented Jan 14, 2019

mattwire commented Jan 14, 2019

jgillmanjr commented Jan 14, 2019

mattwire commented Jan 15, 2019

pradpnayak commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

eileenmcnaughton commented Jan 15, 2019

seamuslee001 commented Jan 16, 2019

eileenmcnaughton commented Jun 2, 2019

Convert Paypal Standard IPN payment_date to system's time zone #13439

Convert Paypal Standard IPN payment_date to system's time zone #13439

Conversation

jgillmanjr commented Jan 12, 2019

Overview

Before

After

Technical Details

civicrm-builder commented Jan 12, 2019

civibot bot commented Jan 12, 2019

seamuslee001 commented Jan 12, 2019

seamuslee001 Jan 12, 2019

Choose a reason for hiding this comment

jgillmanjr Jan 12, 2019

Choose a reason for hiding this comment

mattwire Jan 14, 2019

Choose a reason for hiding this comment

mattwire Jan 14, 2019

Choose a reason for hiding this comment

jgillmanjr Jan 14, 2019

Choose a reason for hiding this comment

jgillmanjr Jan 14, 2019

Choose a reason for hiding this comment

jgillmanjr commented Jan 14, 2019

mattwire commented Jan 14, 2019

jgillmanjr commented Jan 14, 2019

mattwire commented Jan 15, 2019

pradpnayak commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

seamuslee001 commented Jan 15, 2019

eileenmcnaughton commented Jan 15, 2019

seamuslee001 commented Jan 16, 2019

eileenmcnaughton commented Jun 2, 2019