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

Fix setting of next_sched_contribution_date when doing repeattransaction #17028

Closed
wants to merge 2 commits into from
Closed

Fix setting of next_sched_contribution_date when doing repeattransaction #17028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d4ea66
Fix setting of next_sched_contribution_date when doing repeattransaction
mattwire Apr 8, 2020
b52936e
Add test
mattwire Apr 15, 2020
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
2 changes: 1 addition & 1 deletion CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public static function add(&$params, $ids = []) {
CRM_Contribute_BAO_ContributionRecur::updateOnNewPayment(
(!empty($params['contribution_recur_id']) ? $params['contribution_recur_id'] : $params['prevContribution']->contribution_recur_id),
$contributionStatus,
CRM_Utils_Array::value('receive_date', $params)
$params['receive_date'] ?? NULL
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colemanw This line is just for you..

Copy link
Contributor

Choose a reason for hiding this comment

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

:-)

}

Expand Down
22 changes: 14 additions & 8 deletions CRM/Contribute/BAO/ContributionRecur.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,13 +821,16 @@ public static function getRecurringFields() {
* Payment status - this correlates to the machine name of the contribution status ID ie
* - Completed
* - Failed
* @param string $effectiveDate
* @param string $contributionReceiveDate
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a comment explaining what contributionReceiveDate is here could be very helpful - there's a lot of dicussions about what date means what.

*
* @throws \CiviCRM_API3_Exception
*/
public static function updateOnNewPayment($recurringContributionID, $paymentStatus, $effectiveDate) {
public static function updateOnNewPayment($recurringContributionID, $paymentStatus, $contributionReceiveDate) {
// If contributionReceiveDate matches next_sched_contribution_date we update next_sched_contribution_date by one period
$contributionReceiveDate = $contributionReceiveDate
? date('Y-m-d H:i:s', strtotime($contributionReceiveDate))
: date('Y-m-d') . '00:00:00';
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't an extra space be required here? Will it produce (for example) the string 2020-04-1600:00:00?


$effectiveDate = $effectiveDate ? date('Y-m-d', strtotime($effectiveDate)) : date('Y-m-d');
if (!in_array($paymentStatus, ['Completed', 'Failed'])) {
return;
}
Expand Down Expand Up @@ -856,13 +859,16 @@ public static function updateOnNewPayment($recurringContributionID, $paymentStat

if (!empty($existing['installments']) && self::isComplete($recurringContributionID, $existing['installments'])) {
$params['contribution_status_id'] = 'Completed';
$params['next_sched_contribution_date'] = '';
}
else {
// Only update next sched date if it's empty or 'just now' because payment processors may be managing
// the scheduled date themselves as core did not previously provide any help.
if (empty($existing['next_sched_contribution_date']) || strtotime($existing['next_sched_contribution_date']) ==
strtotime($effectiveDate)) {
$params['next_sched_contribution_date'] = date('Y-m-d', strtotime('+' . $existing['frequency_interval'] . ' ' . $existing['frequency_unit'], strtotime($effectiveDate)));
// Update next_sched_contribution_date in the following circumstances:
// - If it's empty.
// - If it is older than the contribution receive date.
// If we update, set it to the contribution receive date + recur frequency unit * interval
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably note why we don't always do it - ie precautionary in case some other process has already handled it

if (empty($existing['next_sched_contribution_date'])
|| strtotime($existing['next_sched_contribution_date']) <= strtotime($contributionReceiveDate)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned that this will break the Paypal IPN. Unless you're intending to merge both this PR plus my PR?

Let's say you don't merge my Paypal IPN PR and this PR is intended to replace that. This condition will now fail if the following contribution is processed slightly less than the frequency unit. So for example, let's say for a weekly transaction, on week 5 it's processed on 1-Jan-2020 at 10:00:00. If the following week, it's processed at 8-Jan-2020 at 09:59:59, this test will fail and the next scheduled contribution date won't update. At least it will start working again the following week.

Copy link
Contributor

Choose a reason for hiding this comment

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

This stalled on this being too tight - I pulled this & the test into #17744 & loosened it up a bit so it should address @andrew-cormick-dockery's issue. I left the time issues out of scope since I suspect they would lead to slippage over time into new days and they seem more like a 'good idea' than an issue being experienced

$params['next_sched_contribution_date'] = date('Y-m-d H:i:s', strtotime('+' . $existing['frequency_interval'] . ' ' . $existing['frequency_unit'], strtotime($contributionReceiveDate)));
}
}
civicrm_api3('ContributionRecur', 'create', $params);
Expand Down
143 changes: 142 additions & 1 deletion tests/phpunit/api/v3/ContributionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3110,7 +3110,7 @@ public function getScheduledDateData() {
'next_sched_contribution_date' => '2012-02-29',
],
'receive_date' => '2012-02-29 16:00:00',
'expected' => '2012-03-29 00:00:00',
'expected' => '2012-03-29 16:00:00',
];
return $result;
}
Expand Down Expand Up @@ -4207,6 +4207,147 @@ public function testRepeatTransactionMembershipCreatePendingContribution() {
$this->contactDelete($originalContribution['values'][1]['contact_id']);
}

/**
* Test repeatTransaction with installments and next_sched_contribution_date
*
* @dataProvider getRepeatTransactionNextSchedData
*
* @param array $dataSet
*
* @throws \Exception
*/
public function testRepeatTransactionUpdateNextSchedContributionDate($dataSet) {
$paymentProcessorID = $this->paymentProcessorCreate();
// Create the contribution before the recur so it doesn't trigger the update of next_sched_contribution_date
$contribution = $this->callAPISuccess('contribution', 'create', array_merge(
$this->_params,
[
'contribution_status_id' => 'Completed',
'receive_date' => $dataSet['repeat'][0]['receive_date'],
])
);
$contributionRecur = $this->callAPISuccess('contribution_recur', 'create', array_merge([
'contact_id' => $this->_individualId,
'frequency_interval' => '1',
'amount' => '500',
'contribution_status_id' => 'Pending',
'start_date' => '2012-01-01 00:00:00',
'currency' => 'USD',
'frequency_unit' => 'month',
'payment_processor_id' => $paymentProcessorID,
], $dataSet['recur']));
// Link the existing contribution to the recur *after* creating the recur.
// If we just created the contribution now the next_sched_contribution_date would be automatically set
// and we want to test the case when it is empty.
$contribution = $this->callAPISuccess('contribution', 'create', [
'id' => $contribution['id'],
'contribution_recur_id' => $contributionRecur['id'],
]);

$contributionRecur = $this->callAPISuccessGetSingle('ContributionRecur', [
'id' => $contributionRecur['id'],
'return' => ['next_sched_contribution_date', 'contribution_status_id'],
]);
// Check that next_sched_contribution_date is empty
$this->assertEquals('', $contributionRecur['next_sched_contribution_date'] ?? '');

$this->callAPISuccess('Contribution', 'repeattransaction', [
'contribution_status_id' => 'Completed',
'contribution_recur_id' => $contributionRecur['id'],
'receive_date' => $dataSet['repeat'][0]['receive_date'],
]);
$contributionRecur = $this->callAPISuccessGetSingle('ContributionRecur', [
'id' => $contributionRecur['id'],
'return' => ['next_sched_contribution_date', 'contribution_status_id'],
]);
// Check that recur has status "In Progress"
$this->assertEquals(
(string) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_ContributionRecur', 'contribution_status_id', $dataSet['repeat'][0]['expectedRecurStatus']),
$contributionRecur['contribution_status_id']
);
// Check that next_sched_contribution_date has been set to 1 period after the contribution receive date (ie. 1 month)
$this->assertEquals($dataSet['repeat'][0]['expectedNextSched'], $contributionRecur['next_sched_contribution_date']);

// Now call Contribution.repeattransaction again and check that the next_sched_contribution_date has moved forward by 1 period again
$this->callAPISuccess('Contribution', 'repeattransaction', [
'contribution_status_id' => 'Completed',
'contribution_recur_id' => $contributionRecur['id'],
'receive_date' => $dataSet['repeat'][1]['receive_date'],
]);
$contributionRecur = $this->callAPISuccessGetSingle('ContributionRecur', [
'id' => $contributionRecur['id'],
'return' => ['next_sched_contribution_date', 'contribution_status_id'],
]);
// Check that recur has status "In Progress" or "Completed" depending on whether number of installments has been reached
$this->assertEquals(
(string) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_ContributionRecur', 'contribution_status_id', $dataSet['repeat'][1]['expectedRecurStatus']),
$contributionRecur['contribution_status_id']
);
// Check that next_sched_contribution_date has been set to 1 period after the contribution receive date (ie. 1 month)
$this->assertEquals($dataSet['repeat'][1]['expectedNextSched'], $contributionRecur['next_sched_contribution_date'] ?? '');
}

/**
* Get dates for testing.
*
* @return array
*/
public function getRepeatTransactionNextSchedData() {
// Both these tests handle/test the case that next_sched_contribution_date is empty when Contribution.repeattransaction
// is called for the first time. Historically setting it was inconsistent but on new updates it should always be set.
/*
* This tests that calling Contribution.repeattransaction with installments does the following:
* - For the first call to repeattransaction the recur status is In Progress and next_sched_contribution_date is updated
* to match next expected receive_date.
* - Once the 3rd contribution is created contributionRecur status = completed and next_sched_contribution_date = ''.
*/
$result['receive_date_includes_time_with_installments']['2012-01-01-1-month'] = [
'recur' => [
'start_date' => '2012-01-01',
'frequency_interval' => 1,
'installments' => '3',
'frequency_unit' => 'month',
],
'repeat' => [
[
'receive_date' => '2012-02-29 16:00:00',
'expectedNextSched' => '2012-03-29 16:00:00',
'expectedRecurStatus' => 'In Progress',
],
[
'receive_date' => '2012-03-29 16:00:00',
Copy link
Contributor

Choose a reason for hiding this comment

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

How nice that your receive date arrives exactly one month later to the second!

'expectedNextSched' => '',
'expectedRecurStatus' => 'Completed',
],
],
];
/*
* This tests that calling Contribution.repeattransaction with no installments does the following:
* - For the each call to repeattransaction the recur status is In Progress and next_sched_contribution_date is updated
* to match next expected receive_date.
*/
$result['receive_date_includes_time_no_installments']['2012-01-01-1-month'] = [
'recur' => [
'start_date' => '2012-01-01',
'frequency_interval' => 1,
'frequency_unit' => 'month',
],
'repeat' => [
[
'receive_date' => '2012-02-29 16:00:00',
'expectedNextSched' => '2012-03-29 16:00:00',
'expectedRecurStatus' => 'In Progress',
],
[
'receive_date' => '2012-03-29 16:00:00',
'expectedNextSched' => '2012-04-29 16:00:00',
'expectedRecurStatus' => 'In Progress',
],
],
];
return $result;
}

/**
* Test sending a mail via the API.
*/
Expand Down