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 repeattransaction api to use custom data from the template contribution #17975

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Fix repeattransaction api to use custom data from the template contribution #17975

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/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2636,7 +2636,7 @@ protected static function repeatTransaction(&$contribution, &$input, $contributi

$createContribution = civicrm_api3('Contribution', 'create', $contributionParams);
$contribution->id = $createContribution['id'];
CRM_Contribute_BAO_ContributionRecur::copyCustomValues($contributionParams['contribution_recur_id'], $contribution->id);
$contribution->copyCustomFields($templateContribution['id'], $contribution->id);
self::handleMembershipIDOverride($contribution->id, $input);
return TRUE;
}
Expand Down
2 changes: 2 additions & 0 deletions CRM/Contribute/BAO/ContributionRecur.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,8 @@ public static function sendRecurringStartOrEndNotification($ids, $recur, $isFirs
/**
* Copy custom data of the initial contribution into its recurring contributions.
*
* @deprecated
*
* @param int $recurId
* @param int $targetContributionId
*/
Expand Down
45 changes: 44 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 @@ -9,6 +9,8 @@
+--------------------------------------------------------------------+
*/

use Civi\Api4\Contribution;

/**
* Test APIv3 civicrm_contribute_* functions
*
Expand Down Expand Up @@ -107,7 +109,7 @@ public function setUp() {
*/
public function tearDown() {
$this->quickCleanUpFinancialEntities();
$this->quickCleanup(['civicrm_uf_match']);
$this->quickCleanup(['civicrm_uf_match'], TRUE);
$financialAccounts = $this->callAPISuccess('FinancialAccount', 'get', []);
foreach ($financialAccounts['values'] as $financialAccount) {
if ($financialAccount['name'] === 'Test Tax financial account ' || $financialAccount['name'] === 'Test taxable financial Type') {
Expand Down Expand Up @@ -2326,6 +2328,46 @@ public function testRepeatTransaction() {
], 'online');
}

/**
* Test custom data is copied over from the template transaction.
*
* (Over time various discussions have deemed this to be the most recent one, allowing
* users to alter custom data going forwards. This is implemented for line items already.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs more comments on what it's supposed to be testing. From where do we expect the custom data to be taken?

Looks like

  1. create a contribution on a contribution recur with a custom field value "first" (aside: why is that named setupRepeatTransaction when what it means is create a contrib recur and a contribution, neither of which is a transaction?)
  2. call repeattransaction and test that the new contribution received "first" for the custom datta
  3. update the contribution created in (2) to "second"
  4. call repeattransaction on the original contribution and test that the custom data was taken from the contribution in (2), i.e. is "second" not the original one (1)

Which is confusing to me. I'd expect that calling repeattransaction and passing in a contribution record would take details from that contribution record, not a different one.

Some comments as to what is suppsed to happen (and why) would be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm - Ok I did start from the assumption that what #17454 was trying to do was right & that I just needed to write a test & fix the implementation to get it over the line

I could make a case that the API should require the template contribution to be passed in and just use that. However, we are already calculating the template contribution in the BAO function for line items so I think it makes sense we should do it for facets. I think reversing course doesn't make sense because at least some processors only 'know' the first contribution & are already passing that in

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've updated the comment block

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 OK with the idea that when there's no is_template contribution we use the last contribution when repeating a contribution from a contrib recur. Happy to believe that that's what we agreed on in that case.

  • I'm confused by the idea that when we pass in a reference contribution ID we're mixing its data with the latest one though. That seems like it's only going to confuse things.

  • I think the question "which contribution should be used as a template for a repeat contribution belonging to a recurring contribution" was answered with "we need a contribution that has is_template == 1 and to copy that". Is it because there's no UI for this yet that we're not implementing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@artfulrobot the contribution we are using is the one retrieved by the CRM_Contribute_BAO_ContributionRecur::getTemplateContribution function - which will retrieve an 'actual' template contribution if there is one, but which will get most recent otherwise.

(from Barcelona sprint)

#15456

Copy link
Contributor

@mattwire mattwire Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@artfulrobot In the context of this PR we should ignore "which contribution should be used as a template for a repeat contribution belonging to a recurring contribution". That is an issue but it's covered by #17455.
In this case, we just want to fix the "copy from contribution to new contribution" and assume we have the right contribution to start with!

Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton @mattwire you are both right, sorry. I got confused with original_contribution_id which I mistook for the ID of the contribution to copy.

I see that, correctly, the contrib recur is used to lookup the template (whether that's a template or the latest).

Copy link
Contributor

Choose a reason for hiding this comment

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

As tests serve to document, more comments explaining what's being tested would be helpful to other readers. I'm happy to have a go at adding these in in a later PR.

* @throws \API_Exception
* @throws \CRM_Core_Exception
*/
public function testRepeatTransactionWithCustomData() {
$this->createCustomGroupWithFieldOfType(['extends' => 'Contribution', 'name' => 'Repeat'], 'text');
$originalContribution = $this->setUpRepeatTransaction([], 'single', [$this->getCustomFieldName('text') => 'first']);
$this->callAPISuccess('contribution', 'repeattransaction', [
'contribution_recur_id' => $originalContribution['contribution_recur_id'],
'contribution_status_id' => 'Completed',
'trxn_id' => 'my_trxn',
]);

$contribution = Contribution::get()
->addWhere('trxn_id', '=', 'my_trxn')
->addSelect('Custom_Group.Enter_text_here')
->addSelect('id')
->execute()->first();
$this->assertEquals('first', $contribution['Custom_Group.Enter_text_here']);

Contribution::update()->setValues(['Custom_Group.Enter_text_here' => 'second'])->addWhere('id', '=', $contribution['id'])->execute();

$this->callAPISuccess('contribution', 'repeattransaction', [
'original_contribution_id' => $originalContribution['id'],
'contribution_status_id' => 'Completed',
'trxn_id' => 'number_3',
]);

$contribution = Contribution::get()
->addWhere('trxn_id', '=', 'number_3')
->setSelect(['id', 'Custom_Group.Enter_text_here'])
->execute()->first();
$this->assertEquals('second', $contribution['Custom_Group.Enter_text_here']);
}

/**
* Test repeat contribution successfully creates line items (plural).
*
Expand Down Expand Up @@ -4170,6 +4212,7 @@ protected function setUpRepeatTransaction($recurParams = [], $flag, $contributio
$params = array_merge($params, $contributionParams);
$originalContribution = $this->callAPISuccess('contribution', 'create', $params);
}
$originalContribution['contribution_recur_id'] = $contributionRecur['id'];
$originalContribution['payment_processor_id'] = $paymentProcessorID;
return $originalContribution;
}
Expand Down