Improve financial trxn spec to require required fields

It turns out the apiv4 conformance test only passes because it
is bypassing the BAO create method - which requires more parameters

Not this will fail :-( because it messes with the test expectations
- I don't know how to create complex valid test data for it though....

I guess we could create a contribution & pass it as entity_id

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 2a937f46e24f5dc8c28de9e60048d156b633d525..2d4be53f81ab42abf71a5b6b9bba1f8276db4e4e 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -205,7 +205,7 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution im
     $contribution->trxn_result_code = $params['trxn_result_code'] ?? NULL;
     $contribution->payment_processor = $params['payment_processor'] ?? NULL;
 
-    //add Account details
+    // Loading contribution used to be required for recordFinancialAccounts.
     $params['contribution'] = $contribution;
     if (empty($params['is_post_payment_create'])) {
       // If this is being called from the Payment.create api/ BAO then that Entity
@@ -215,7 +215,7 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution im
       // Note that leveraging this parameter for any other code flow is not supported and
       // is likely to break in future and / or cause serious problems in your data.
       // https://github.com/civicrm/civicrm-core/pull/14673
-      self::recordFinancialAccounts($params);
+      self::recordFinancialAccounts($params, $contribution);
     }
 
     if (self::isUpdateToRecurringContribution($params)) {
@@ -3073,11 +3073,11 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
    *
    * @param array $params
    *   Contribution object, line item array and params for trxn.
-   *
+   * @param \CRM_Contribute_DAO_Contribution $contribution
    *
    * @return null|\CRM_Core_BAO_FinancialTrxn
    */
-  public static function recordFinancialAccounts(&$params) {
+  public static function recordFinancialAccounts(&$params, CRM_Contribute_DAO_Contribution $contribution) {
     $skipRecords = $return = FALSE;
     $isUpdate = !empty($params['prevContribution']);
 
@@ -3097,7 +3097,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
       $entityTable = 'civicrm_membership';
     }
     else {
-      $entityId = $params['contribution']->id;
+      $entityId = $contribution->id;
       $entityTable = 'civicrm_contribution';
     }
 
@@ -3154,16 +3154,24 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
       if (!isset($totalAmount) && !empty($params['prevContribution'])) {
         $totalAmount = $params['total_amount'] = $params['prevContribution']->total_amount;
       }
+      if (empty($contribution->currency)) {
+        $contribution->find(TRUE);
+      }
       //build financial transaction params
       $trxnParams = [
-        'contribution_id' => $params['contribution']->id,
+        'contribution_id' => $contribution->id,
         'to_financial_account_id' => $params['to_financial_account_id'],
-        'trxn_date' => !empty($params['contribution']->receive_date) ? $params['contribution']->receive_date : date('YmdHis'),
+        // If receive_date is not deliberately passed in we assume 'now'.
+        // test testCompleteTransactionWithReceiptDateSet ensures we don't
+        // default to loading the stored contribution receive_date.
+        // Note that as we deprecate completetransaction in favour
+        // of Payment.create handling of trxn_date will tighten up.
+        'trxn_date' => $params['receive_date'] ?? date('YmdHis'),
         'total_amount' => $totalAmount,
         'fee_amount' => $params['fee_amount'] ?? NULL,
         'net_amount' => CRM_Utils_Array::value('net_amount', $params, $totalAmount),
-        'currency' => $params['contribution']->currency,
-        'trxn_id' => $params['contribution']->trxn_id,
+        'currency' => $contribution->currency,
+        'trxn_id' => $contribution->trxn_id,
         // @todo - this is getting the status id from the contribution - that is BAD - ie the contribution could be partially
         // paid but each payment is completed. The work around is to pass in the status_id in the trxn_params but
         // this should really default to completed (after discussion).

diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php
index 27a385df856bfa970444be772f7885e50561b0be..b26a1253339fd2b89f16f457ab7b960cb080416b 100644 (file)
--- a/CRM/Core/BAO/FinancialTrxn.php
+++ b/CRM/Core/BAO/FinancialTrxn.php
@@ -24,7 +24,18 @@ class CRM_Core_BAO_FinancialTrxn extends CRM_Financial_DAO_FinancialTrxn {
    *
    * @return CRM_Financial_DAO_FinancialTrxn
    */
-  public static function create($params) {
+  public static function create(array $params): CRM_Financial_DAO_FinancialTrxn {
+    if (empty($params['id'])) {
+      $params = array_merge([
+        'currency' => CRM_Core_Config::singleton()->defaultCurrency,
+        'status_id' => CRM_Core_Pseudoconstant::getKey(__CLASS__, 'status_id', 'Paid'),
+      ], $params);
+      if (!CRM_Utils_Rule::currencyCode($params['currency'])) {
+        CRM_Core_Error::deprecatedWarning('invalid currency data for financial transactions is deprecated');
+        $params['currency'] = CRM_Core_Config::singleton()->defaultCurrency;
+      }
+    }
+
     $trxn = new CRM_Financial_DAO_FinancialTrxn();
     $trxn->copyValues($params);
 
@@ -36,10 +47,6 @@ class CRM_Core_BAO_FinancialTrxn extends CRM_Financial_DAO_FinancialTrxn {
       $trxn->net_amount = $params['total_amount'] - $params['fee_amount'];
     }
 
-    if (empty($params['id']) && !CRM_Utils_Rule::currencyCode($trxn->currency)) {
-      $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency;
-    }
-
     $trxn->save();
 
     if (!empty($params['id'])) {

diff --git a/Civi/Api4/Service/Spec/Provider/FinancialTrxnCreationSpecProvider.php b/Civi/Api4/Service/Spec/Provider/FinancialTrxnCreationSpecProvider.php
new file mode 100644 (file)
index 0000000..eca8043
--- /dev/null
+++ b/Civi/Api4/Service/Spec/Provider/FinancialTrxnCreationSpecProvider.php
@@ -0,0 +1,47 @@
+<?php
+
+/*
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC. All rights reserved.                        |
+ |                                                                    |
+ | This work is published under the GNU AGPLv3 license with some      |
+ | permitted exceptions and without any warranty. For full license    |
+ | and copyright information, see https://civicrm.org/licensing       |
+ +--------------------------------------------------------------------+
+ */
+
+namespace Civi\Api4\Service\Spec\Provider;
+
+use Civi\Api4\Service\Spec\FieldSpec;
+use Civi\Api4\Service\Spec\RequestSpec;
+
+class FinancialTrxnCreationSpecProvider implements Generic\SpecProviderInterface {
+
+  /**
+   * Modify the api spec.
+   *
+   * @param \Civi\Api4\Service\Spec\RequestSpec $spec
+   */
+  public function modifySpec(RequestSpec $spec): void {
+    $spec->getFieldByName('to_financial_account_id')->setRequired(TRUE);
+    $field = new FieldSpec('entity_id', 'FinancialTrxn', 'Integer');
+    $field->setRequired(TRUE);
+    $field->setTitle(ts('Related entity (eg. contribution) id'));
+    $spec->addFieldSpec($field);
+    $spec->getFieldByName('status_id')->setDefaultValue(1);
+    $spec->getFieldByName('total_amount')->setRequired(TRUE);
+  }
+
+  /**
+   * Specify the entity & action it applies to.
+   *
+   * @param string $entity
+   * @param string $action
+   *
+   * @return bool
+   */
+  public function applies($entity, $action): bool {
+    return $entity === 'FinancialTrxn' && $action === 'create';
+  }
+
+}

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
index 92955eb687379fe4037d499a4c54980af4acd065..599f2ee4bb69a472e6ad3cd9cb999fd0f623f6c7 100644 (file)
--- a/tests/phpunit/api/v3/ContributionTest.php
+++ b/tests/phpunit/api/v3/ContributionTest.php
@@ -1390,7 +1390,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
   /**
    * Function tests that financial records are updated when Payment Instrument is changed.
    */
-  public function testCreateUpdateContributionPaymentInstrument() {
+  public function testCreateUpdateContributionPaymentInstrument(): void {
     $instrumentId = $this->_addPaymentInstrument();
     $contribParams = [
       'contact_id' => $this->_individualId,
@@ -1406,12 +1406,12 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
       'id' => $contribution['id'],
       'payment_instrument_id' => $instrumentId,
     ]);
-    $contribution = $this->callAPISuccess('contribution', 'create', $newParams);
+    $contribution = $this->callAPISuccess('Contribution', 'create', $newParams);
     $this->assertAPISuccess($contribution);
     $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId);
 
     // cleanup - delete created payment instrument
-    $this->_deletedAddedPaymentInstrument();
+    $this->deletedAddedPaymentInstrument();
   }
 
   /**
@@ -1438,7 +1438,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
     $this->checkFinancialTrxnPaymentInstrumentChange($contribution['id'], 4, $instrumentId, -100);
 
     // cleanup - delete created payment instrument
-    $this->_deletedAddedPaymentInstrument();
+    $this->deletedAddedPaymentInstrument();
   }
 
   /**
@@ -3064,7 +3064,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
   /**
    * CRM-14151 - Test completing a transaction via the API.
    */
-  public function testCompleteTransactionWithReceiptDateSet() {
+  public function testCompleteTransactionWithReceiptDateSet(): void {
     $this->swapMessageTemplateForTestTemplate();
     $mut = new CiviMailUtils($this, TRUE);
     $this->createLoggedInUser();
@@ -4196,7 +4196,7 @@ class api_v3_ContributionTest extends CiviUnitTestCase {
     return $optionValue['values'][$optionValue['id']]['value'];
   }
 
-  public function _deletedAddedPaymentInstrument() {
+  public function deletedAddedPaymentInstrument() {
     $result = $this->callAPISuccess('OptionValue', 'get', [
       'option_group_id' => 'payment_instrument',
       'name' => 'Test Card',