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

dev/financial#6 Use template contribution for Contribution.repeattransaction #22487

Merged
merged 1 commit into from
May 23, 2022
Merged

dev/financial#6 Use template contribution for Contribution.repeattransaction #22487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/ContributionRecur.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ public static function ensureTemplateContributionExists(int $id) {
*
* @param int $id
* @param array $overrides
* Parameters that should be overriden. Add unit tests if using parameters other than total_amount & financial_type_id.
* Parameters that should be overridden. Add unit tests if using parameters other than total_amount & financial_type_id.
*
* @return array
*
Expand Down
13 changes: 5 additions & 8 deletions api/v3/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,14 +585,11 @@ function _civicrm_api3_contribution_completetransaction_spec(&$params) {
function civicrm_api3_contribution_repeattransaction($params) {
civicrm_api3_verify_one_mandatory($params, NULL, ['contribution_recur_id', 'original_contribution_id']);
if (empty($params['original_contribution_id'])) {
// CRM-19873 call with test mode.
$params['original_contribution_id'] = civicrm_api3('contribution', 'getvalue', [
'return' => 'id',
'contribution_status_id' => ['IN' => ['Completed']],
'contribution_recur_id' => $params['contribution_recur_id'],
'contribution_test' => CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_ContributionRecur', $params['contribution_recur_id'], 'is_test'),
'options' => ['limit' => 1, 'sort' => 'id DESC'],
]);
$templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution($params['contribution_recur_id']);
if (empty($templateContribution)) {
throw new CiviCRM_API3_Exception('Contribution.repeattransaction failed to get original_contribution_id for recur with ID: ' . $params['contribution_recur_id']);
Copy link
Member

Choose a reason for hiding this comment

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

can you change to API_Exception to make it APIv4 friendly too

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I think within apiv3 this is correct.

API_Exception is a bit of a pain in a way - ideally we could catch 'all CiviCRM extensions' with one catch type but API_Exception doesn't extend any other CiviCRM exception types - I'm guessing that was before it was handed over to CT @colemanw ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@monishdeb This code is within an API3 function so it is unlikely to ever be called by API4? There is quite a bit within the function which would probably be cleaned up before creating a "repeattransaction" for API4 - see eg. #22488 for a start.

}
$params['original_contribution_id'] = $templateContribution['id'];
}
$contribution = new CRM_Contribute_BAO_Contribution();
$contribution->id = $params['original_contribution_id'];
Expand Down
41 changes: 41 additions & 0 deletions tests/phpunit/api/v3/ContributionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2523,6 +2523,47 @@ public function testRepeatTransactionTestRecurId() {
$this->assertEquals($contributionRecur['values'][1]['is_test'], $repeatedContribution['values'][2]['is_test']);
}

/**
* Test repeat contribution accepts recur_id instead of
* original_contribution_id.
*
* @throws \CRM_Core_Exception
*/
public function testRepeatTransactionPreviousContributionRefunded(): void {
$contributionRecur = $this->callAPISuccess('contribution_recur', 'create', [
'contact_id' => $this->_individualId,
'installments' => '12',
'frequency_interval' => '1',
'amount' => '100',
'contribution_status_id' => 1,
'start_date' => '2012-01-01 00:00:00',
'currency' => 'USD',
'frequency_unit' => 'month',
'payment_processor_id' => $this->paymentProcessorID,
]);
$this->callAPISuccess('contribution', 'create', array_merge(
$this->_params,
[
'contribution_recur_id' => $contributionRecur['id'],
'contribution_status_id' => 'Refunded',
]
)
);

$this->callAPISuccess('contribution', 'repeattransaction', [
'contribution_recur_id' => $contributionRecur['id'],
'trxn_id' => 1234,
]);
$contributions = $this->callAPISuccess('contribution', 'get', [
'contribution_recur_id' => $contributionRecur['id'],
'sequential' => 1,
]);
// We should have contribution 0 in "Refunded" status and contribution 1 in "Pending" status
$this->assertEquals(2, $contributions['count']);
$this->assertEquals(7, $contributions['values'][0]['contribution_status_id']);
$this->assertEquals(2, $contributions['values'][1]['contribution_status_id']);
}

Copy link
Member

Choose a reason for hiding this comment

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

@eileenmcnaughton @mattwire I am happy with the logic, but I am not sure moving forward, are we relying on APIv4 to do CRUD operations on new unit tests? if it doesn't matter unless the UT captures the fix/change accurately then I am ok here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@monishdeb I think you are saying 'would this be better if we were using apiv4 instead of 3 in the tests.

I guess we are moving that way - but we are not so far down the track I'm alarmed at adding new tests that use apiv3 - in this case, however, I think v4 would be better on the final get because it would make it easy to check contribution_status_id:name rather than the hard-coded '2' & '7'

Copy link
Member

Choose a reason for hiding this comment

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

yes contribution_status_id:name is the other reason I felt like APIv4 would be appropriate here.

( I wish there's a emoticon for hi5 )

Copy link
Contributor

Choose a reason for hiding this comment

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

five

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton @monishdeb I agree with preferring to use API4 but I'm not sure I understand fully how we would implement it for the tests? We can set $this->_apiversion = 4; but in this case repeattransaction does not exist for API4 so we would have to change api version during the test which feels a bit messy?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire generally I just call the v4 api direct in the tests (unless specifically testing apiv3 or both 3 & 4) - the callApiSuccess helpers are helpful for apiv3 but don't add much to api v4 - so it's really just that last Contribution::get where using apiv4 would make it easy to eliminate the checks on hard-coded status ids (which it the bit that raised the discussion)

/**
* CRM-19945 Tests that Contribute.repeattransaction renews a membership when contribution status=Completed
*
Expand Down