[REF] Fully remove contribution object from repeattransaction function

We can now see it is no longer used & remove it

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 00d549ad3a9a62f9ccb034f04506f8b0a344a54f..e8b6a53af2c79aa068d40e3290a3610eb065821e 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -428,7 +428,7 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
    * @return array
    */
   public static function getPaymentProcessorReadyAddressParams($params, $billingLocationTypeID) {
-    list($hasBillingField, $addressParams) = self::getBillingAddressParams($params, $billingLocationTypeID);
+    [$hasBillingField, $addressParams] = self::getBillingAddressParams($params, $billingLocationTypeID);
     foreach ($addressParams as $name => $field) {
       if (substr($name, 0, 8) == 'billing_') {
         $addressParams[substr($name, 9)] = $addressParams[$field];
@@ -2428,16 +2428,21 @@ LEFT JOIN  civicrm_contribution contribution ON ( componentPayment.contribution_
    * 4) The  calling code calls Payment.create which in turn calls CompleteOrder (if completing)
    *   which updates the various entities and sends appropriate emails.
    *
-   * Gaps in the above (@todo)
+   * Gaps in the above (
+   *
+   * @param array $input
+   *
+   * @param array $contributionParams
+   *
+   * @return bool|array
+   * @throws \API_Exception
+   * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
+   * @todo
    *  1) many processors still call repeattransaction with contribution_status_id = Completed
    *  2) repeattransaction code is current munged into completeTransaction code for historical bad coding reasons
    *  3) Repeat transaction duplicates rather than calls Order.create
    *  4) Use of payment.create still limited - completetransaction is more common.
-   *  5) the template transaction is tricky - historically we used the first contribution
-   *    linked to a recurring contribution. More recently that was changed to be the most recent.
-   *    Ideally it would be an actual template - not a contribution used as a template which
-   *    would give more appropriate flexibility. Note line_items have an entity so that table
-   *    could be used for the line item template - the difficulty is the custom fields...
    * 6) the determination of the membership to be linked is tricksy. The prioritised method is
    *   to load the membership(s) referred to via line items in the template transactions. Any other
    *   method is likely to lead to incorrect line items & related entities being created (as the line_item
@@ -2449,55 +2454,27 @@ LEFT JOIN  civicrm_contribution contribution ON ( componentPayment.contribution_
    *   of historical processors since this has been handled 'forever' - specifically for paypal.
    *   albeit by an even nastier mechanism than the current input override.
    *   The count is out on how correct related entities wind up in this case.
-   *
-   * @param CRM_Contribute_BAO_Contribution $contribution
-   * @param array $input
-   * @param array $contributionParams
-   *
-   * @return bool|array
-   * @throws CiviCRM_API3_Exception
    */
-  protected static function repeatTransaction(&$contribution, $input, $contributionParams) {
-    if (!empty($contribution->id)) {
-      return FALSE;
-    }
-
-    // Unclear why this would only be set for repeats.
-    if (!empty($input['amount'])) {
-      $contribution->total_amount = $contributionParams['total_amount'] = $input['amount'];
-    }
+  protected static function repeatTransaction(array $input, array $contributionParams) {
 
-    $recurringContribution = civicrm_api3('ContributionRecur', 'getsingle', [
-      'id' => $contributionParams['contribution_recur_id'],
-    ]);
-    if (!empty($recurringContribution['financial_type_id'])) {
-      // CRM-17718 the campaign id on the contribution recur record should get precedence.
-      $contributionParams['financial_type_id'] = $recurringContribution['financial_type_id'];
-    }
     $templateContribution = CRM_Contribute_BAO_ContributionRecur::getTemplateContribution(
-      $contributionParams['contribution_recur_id'],
-      array_intersect_key($contributionParams, [
-        'total_amount' => TRUE,
-        'financial_type_id' => TRUE,
-      ])
+      (int) $contributionParams['contribution_recur_id'],
+      array_filter([
+        'total_amount' => $input['total_amount'] ?? NULL,
+        'financial_type_id' => $input['financial_type_id'] ?? NULL,
+        'campaign_id' => $input['campaign_id'] ?? NULL,
+        // array_filter with strlen filters out NULL, '' and FALSE but not 0.
+      ], 'strlen')
     );
     $input['line_item'] = $contributionParams['line_item'] = $templateContribution['line_item'];
     $contributionParams['status_id'] = 'Pending';
 
-    foreach (['contact_id', 'financial_type_id', 'currency', 'source', 'amount_level', 'address_id', 'on_behalf', 'source_contact_id', 'tax_amount', 'contribution_page_id'] as $fieldName) {
+    foreach (['contact_id', 'campaign_id', 'financial_type_id', 'currency', 'source', 'amount_level', 'address_id', 'on_behalf', 'source_contact_id', 'tax_amount', 'contribution_page_id', 'total_amount'] as $fieldName) {
       if (isset($templateContribution[$fieldName])) {
         $contributionParams[$fieldName] = $templateContribution[$fieldName];
       }
     }
-    if (!empty($recurringContribution['campaign_id'])) {
-      // CRM-17718 the campaign id on the contribution recur record should get precedence.
-      $contributionParams['campaign_id'] = $recurringContribution['campaign_id'];
-    }
-    if (!isset($contributionParams['campaign_id']) && isset($templateContribution['campaign_id'])) {
-      // Fall back on value from the previous contribution if not passed in as input
-      // or loadable from the recurring contribution.
-      $contributionParams['campaign_id'] = $templateContribution['campaign_id'];
-    }
+
     $contributionParams['source'] = $contributionParams['source'] ?? ts('Recurring contribution');
 
     $createContribution = civicrm_api3('Contribution', 'create', $contributionParams);
@@ -4212,6 +4189,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
    *   transitioning related elements).
    *
    * @return array
+   * @throws \API_Exception
    * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
    */
@@ -4225,6 +4203,8 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
 
     // Unset ids just to make it clear it's not used again.
     unset($ids);
+    $contributionID = !empty($contribution->id) ? (int) $contribution->id : NULL;
+
     // The previous details are used when calculating line items so keep it before any code that 'does something'
     if (!empty($contribution->id)) {
       $input['prevContribution'] = CRM_Contribute_BAO_Contribution::getValues(['id' => $contribution->id]);
@@ -4264,9 +4244,10 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
       $contributionParams['contribution_recur_id'] = $recurringContributionID;
     }
     $changeDate = CRM_Utils_Array::value('trxn_date', $input, date('YmdHis'));
-
-    $contributionResult = self::repeatTransaction($contribution, $input, $contributionParams);
-    $contributionID = (int) ($contribution->id ?? $contributionResult['id']);
+    if (!$contributionID) {
+      $contributionResult = self::repeatTransaction($input, $contributionParams);
+      $contributionID = $contributionResult['id'];
+    }
 
     if ($input['component'] == 'contribute') {
       if ($contributionParams['contribution_status_id'] === $completedContributionStatusID) {
@@ -4287,7 +4268,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
     $contributionParams['id'] = $contributionID;
     $contributionParams['is_post_payment_create'] = $isPostPaymentCreate;
 
-    if (!$contributionResult) {
+    if (empty($contributionResult)) {
       $contributionResult = civicrm_api3('Contribution', 'create', $contributionParams);
     }
 

diff --git a/CRM/Contribute/BAO/ContributionRecur.php b/CRM/Contribute/BAO/ContributionRecur.php
index 82e8c767e6c0af9b459aba87eca1e119ea8c7c10..161cac49bb89622187e2fa3829f6a90f78eff654 100644 (file)
--- a/CRM/Contribute/BAO/ContributionRecur.php
+++ b/CRM/Contribute/BAO/ContributionRecur.php
@@ -9,6 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Api4\Contribution;
+use Civi\Api4\ContributionRecur;
+
 /**
  *
  * @package CRM
@@ -411,29 +414,39 @@ INNER JOIN civicrm_contribution       con ON ( con.id = mp.contribution_id )
    *
    * @return array
    *
-   * @throws \CiviCRM_API3_Exception
-   * @throws \Civi\API\Exception\UnauthorizedException
    * @throws \API_Exception
    */
-  public static function getTemplateContribution($id, $overrides = []) {
-    // use api3 because api4 doesn't handle ContributionRecur yet...
-    $is_test = civicrm_api3('ContributionRecur', 'getvalue', [
-      'return' => "is_test",
-      'id' => $id,
-    ]);
+  public static function getTemplateContribution(int $id, $overrides = []): array {
+    $recurFields = ['is_test', 'financial_type_id', 'total_amount', 'campaign_id'];
+    $recurringContribution = ContributionRecur::get(FALSE)
+      ->addWhere('id', '=', $id)
+      ->setSelect($recurFields)
+      ->execute()
+      ->first();
+    // If financial_type_id or total_amount are set on the
+    // recurring they are overrides, but of lower precedence
+    // than input parameters.
+    // we filter out null, '' and FALSE but not zero - I'm on the fence about zero.
+    $overrides = array_filter(array_merge(
+      // We filter recurringContribution as we only want the fields we asked for
+      // and specifically don't want 'id' added to overrides.
+      array_intersect_key($recurringContribution, array_fill_keys($recurFields, 1)),
+      $overrides
+    ), 'strlen');
+
     // First look for new-style template contribution with is_template=1
-    $templateContributions = \Civi\Api4\Contribution::get(FALSE)
+    $templateContributions = Contribution::get(FALSE)
       ->addWhere('contribution_recur_id', '=', $id)
       ->addWhere('is_template', '=', 1)
-      ->addWhere('is_test', '=', $is_test)
+      ->addWhere('is_test', '=', $recurringContribution['is_test'])
       ->addOrderBy('id', 'DESC')
       ->setLimit(1)
       ->execute();
     if (!$templateContributions->count()) {
       // Fall back to old style template contributions
-      $templateContributions = \Civi\Api4\Contribution::get(FALSE)
+      $templateContributions = Contribution::get(FALSE)
         ->addWhere('contribution_recur_id', '=', $id)
-        ->addWhere('is_test', '=', $is_test)
+        ->addWhere('is_test', '=', $recurringContribution['is_test'])
         ->addOrderBy('id', 'DESC')
         ->setLimit(1)
         ->execute();

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
index e48fd59503395061ade9521def99047dd1f51773..3696a1c2a4bf5c28a8355e557df604151138e2a0 100644 (file)
--- a/tests/phpunit/api/v3/ContributionTest.php
+++ b/tests/phpunit/api/v3/ContributionTest.php
@@ -2338,7 +2338,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
    * @throws \API_Exception
    * @throws \CRM_Core_Exception
    */
-  public function testRepeatTransactionWithCustomData() {
+  public function testRepeatTransactionWithCustomData(): void {
     $this->createCustomGroupWithFieldOfType(['extends' => 'Contribution', 'name' => 'Repeat'], 'text');
     $originalContribution = $this->setUpRepeatTransaction([], 'single', [$this->getCustomFieldName('text') => 'first']);
     $this->callAPISuccess('contribution', 'repeattransaction', [
@@ -2649,9 +2649,12 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
   }
 
   /**
-   * CRM-16397 test appropriate action if total amount has changed for single line items.
+   * CRM-16397 test appropriate action if total amount has changed for single
+   * line items.
+   *
+   * @throws \CRM_Core_Exception
    */
-  public function testRepeatTransactionAlteredAmount() {
+  public function testRepeatTransactionAlteredAmount(): void {
     $paymentProcessorID = $this->paymentProcessorCreate();
     $contributionRecur = $this->callAPISuccess('contribution_recur', 'create', [
       'contact_id' => $this->_individualId,
@@ -2674,7 +2677,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
     $this->callAPISuccess('contribution', 'repeattransaction', [
       'original_contribution_id' => $originalContribution['id'],
       'contribution_status_id' => 'Completed',
-      'trxn_id' => uniqid(),
+      'trxn_id' => 1234,
       'total_amount' => '400',
       'fee_amount' => 50,
     ]);
@@ -2713,8 +2716,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
       'entity_id' => $originalContribution['id'] + 1,
     ]));
 
-    unset($expectedLineItem['id'], $expectedLineItem['entity_id']);
-    unset($lineItem2['values'][0]['id'], $lineItem2['values'][0]['entity_id']);
+    unset($expectedLineItem['id'], $expectedLineItem['entity_id'], $lineItem2['values'][0]['id'], $lineItem2['values'][0]['entity_id']);
     $this->assertEquals($expectedLineItem, $lineItem2['values'][0]);
   }
 
@@ -2934,8 +2936,10 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
    * CRM-17718 campaign stored on contribution recur gets priority.
    *
    * This reflects the fact we permit people to update them.
+   *
+   * @throws \CRM_Core_Exception
    */
-  public function testRepeatTransactionUpdatedCampaign() {
+  public function testRepeatTransactionUpdatedCampaign(): void {
     $paymentProcessorID = $this->paymentProcessorCreate();
     $campaignID = $this->campaignCreate();
     $campaignID2 = $this->campaignCreate();
@@ -2962,10 +2966,10 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
     $this->callAPISuccess('contribution', 'repeattransaction', [
       'original_contribution_id' => $originalContribution['id'],
       'contribution_status_id' => 'Completed',
-      'trxn_id' => uniqid(),
+      'trxn_id' => 789,
     ]);
 
-    $this->callAPISuccessGetSingle('contribution', [
+    $this->callAPISuccessGetSingle('Contribution', [
       'total_amount' => 100,
       'campaign_id' => $campaignID,
     ]);