From 8b3f4fafbdfeb97824046778737a60fa202d626d Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Wed, 19 Jan 2022 17:20:39 +1300 Subject: [PATCH] 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 --- CRM/Contribute/BAO/Contribution.php | 26 ++++++---- CRM/Core/BAO/FinancialTrxn.php | 17 +++++-- .../FinancialTrxnCreationSpecProvider.php | 47 +++++++++++++++++++ tests/phpunit/api/v3/ContributionTest.php | 12 ++--- 4 files changed, 82 insertions(+), 20 deletions(-) create mode 100644 Civi/Api4/Service/Spec/Provider/FinancialTrxnCreationSpecProvider.php diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 2a937f46e2..2d4be53f81 100644 --- 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 27a385df85..b26a125333 100644 --- 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 index 0000000000..eca80431f6 --- /dev/null +++ b/Civi/Api4/Service/Spec/Provider/FinancialTrxnCreationSpecProvider.php @@ -0,0 +1,47 @@ +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 92955eb687..599f2ee4bb 100644 --- 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', -- 2.25.1