From be2e79c858d13a8fc84e66e316580304c571cb93 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 9 Mar 2021 00:03:34 +1300 Subject: [PATCH] [REF] Cleanup code to determine financial_type_id Financial type id is really simple on this form - it's either required or it can be determined form the price set. However, the code passes the financial_type_id from array to array, calculating it in more than one place which rather hides the underlying simplicity of it. This retrieves it using a function that does the same from anywhere in the code. Note that if someone tried to add it before this->order is built it would hard fail & kill a bunch of tests. this->order is built at the start of the submit routine --- CRM/Financial/BAO/Order.php | 35 +++++++++++++++- CRM/Member/Form/Membership.php | 40 +++++++++---------- .../CRM/Member/Form/MembershipTest.php | 4 +- 3 files changed, 57 insertions(+), 22 deletions(-) diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php index 66cfdc210467..f1d0c771db37 100644 --- a/CRM/Financial/BAO/Order.php +++ b/CRM/Financial/BAO/Order.php @@ -78,6 +78,13 @@ class CRM_Financial_BAO_Order { */ protected $priceFieldMetadata = []; + /** + * Metadata for price sets. + * + * @var array + */ + protected $priceSetMetadata = []; + /** * Get form object. * @@ -245,7 +252,7 @@ public function getPriceFieldSpec(int $id) :array { */ public function getPriceFieldsMetadata(): array { if (empty($this->priceFieldMetadata)) { - $this->priceFieldMetadata = CRM_Price_BAO_PriceSet::getCachedPriceSetDetail($this->getPriceSetID())['fields']; + $this->getPriceSetMetadata(); if ($this->getForm()) { CRM_Utils_Hook::buildAmount($this->form->getFormContext(), $this->form, $this->priceFieldMetadata); } @@ -253,6 +260,32 @@ public function getPriceFieldsMetadata(): array { return $this->priceFieldMetadata; } + /** + * Get the metadata for the fields in the price set. + * + * @return array + */ + public function getPriceSetMetadata(): array { + if (empty($this->priceSetMetadata)) { + $priceSetMetadata = CRM_Price_BAO_PriceSet::getCachedPriceSetDetail($this->getPriceSetID()); + $this->priceFieldMetadata = $priceSetMetadata['fields']; + unset($priceSetMetadata['fields']); + $this->priceSetMetadata = $priceSetMetadata; + } + return $this->priceSetMetadata; + } + + /** + * Get the financial type id for the order. + * + * This may differ to the line items.... + * + * @return int + */ + public function getFinancialTypeID(): int { + return (int) $this->getOverrideFinancialTypeID() ?: $this->getPriceSetMetadata()['financial_type_id']; + } + /** * Set the price field selection from an array of params containing price fields. * diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php index b2d0bbd377ab..8d094b27b88f 100644 --- a/CRM/Member/Form/Membership.php +++ b/CRM/Member/Form/Membership.php @@ -1014,10 +1014,7 @@ public function submit(): void { $this->_priceSet, $formValues ); - if (empty($formValues['financial_type_id'])) { - $formValues['financial_type_id'] = $this->_priceSet['financial_type_id']; - } - + $formValues['financial_type_id'] = $this->getFinancialTypeID(); $membershipTypeValues = []; foreach ($this->_memTypeSelected as $memType) { $membershipTypeValues[$memType]['membership_type_id'] = $memType; @@ -1154,7 +1151,6 @@ public function submit(): void { if (!empty($formValues['record_contribution'])) { $recordContribution = [ 'total_amount', - 'financial_type_id', 'payment_instrument_id', 'trxn_id', 'contribution_status_id', @@ -1168,6 +1164,7 @@ public function submit(): void { foreach ($recordContribution as $f) { $params[$f] = $formValues[$f] ?? NULL; } + $params['financial_type_id'] = $this->getFinancialTypeID(); if (empty($formValues['source'])) { $params['contribution_source'] = ts('%1 Membership: Offline signup (by %2)', [ @@ -1195,7 +1192,7 @@ public function submit(): void { //insert financial type name in receipt. $formValues['contributionType_name'] = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialType', - $formValues['financial_type_id'] + $this->getFinancialTypeID() ); } @@ -1211,16 +1208,7 @@ public function submit(): void { //CRM-20264 : Store CC type and number (last 4 digit) during backoffice or online payment $params['card_type_id'] = $this->_params['card_type_id'] ?? NULL; $params['pan_truncation'] = $this->_params['pan_truncation'] ?? NULL; - - if (!$isQuickConfig) { - $params['financial_type_id'] = CRM_Core_DAO::getFieldValue('CRM_Price_DAO_PriceSet', - $this->_priceSetId, - 'financial_type_id' - ); - } - else { - $params['financial_type_id'] = $formValues['financial_type_id'] ?? NULL; - } + $params['financial_type_id'] = $this->getFinancialTypeID(); //get the payment processor id as per mode. Try removing in favour of beginPostProcess. $params['payment_processor_id'] = $formValues['payment_processor_id'] = $this->_paymentProcessor['id']; @@ -1231,7 +1219,7 @@ public function submit(): void { $formValues['currencyID'] = $this->getCurrency(); $formValues['description'] = ts("Contribution submitted by a staff person using member's credit card for signup"); $formValues['invoiceID'] = $this->getInvoiceID(); - $formValues['financial_type_id'] = $params['financial_type_id']; + $formValues['financial_type_id'] = $this->getFinancialTypeID(); // at this point we've created a contact and stored its address etc // all the payment processors expect the name and address to be in the @@ -1268,7 +1256,7 @@ public function submit(): void { 'campaign_id' => $paymentParams['campaign_id'] ?? NULL, 'source' => CRM_Utils_Array::value('source', $paymentParams, CRM_Utils_Array::value('description', $paymentParams)), 'payment_instrument_id' => $paymentInstrumentID, - 'financial_type_id' => $params['financial_type_id'], + 'financial_type_id' => $this->getFinancialTypeID(), 'receive_date' => CRM_Utils_Time::date('YmdHis'), 'tax_amount' => $params['tax_amount'] ?? NULL, 'invoice_id' => $this->getInvoiceID(), @@ -1841,7 +1829,7 @@ protected function processContribution( $contactID = $contributionParams['contact_id']; // add these values for the recurringContrib function ,CRM-10188 - $params['financial_type_id'] = $contributionParams['financial_type_id']; + $params['financial_type_id'] = $this->getFinancialTypeID(); $params['is_recur'] = TRUE; $params['payment_instrument_id'] = $contributionParams['payment_instrument_id'] ?? NULL; $recurringContributionID = $this->legacyProcessRecurringContribution($params, $contactID); @@ -1888,7 +1876,7 @@ protected function legacyProcessRecurringContribution(array $params, $contactID) $recurParams['frequency_unit'] = $params['frequency_unit'] ?? NULL; $recurParams['frequency_interval'] = $params['frequency_interval'] ?? NULL; $recurParams['installments'] = $params['installments'] ?? NULL; - $recurParams['financial_type_id'] = $params['financial_type_id']; + $recurParams['financial_type_id'] = $this->getFinancialTypeID(); $recurParams['currency'] = $params['currency'] ?? NULL; $recurParams['payment_instrument_id'] = $params['payment_instrument_id']; @@ -1920,4 +1908,16 @@ protected function isTest(): int { return ($this->_mode === 'test') ? TRUE : FALSE; } + /** + * Get the financial type id relevant to the contribution. + * + * Financial type id is optional when price sets are in use. + * Otherwise they are required for the form to submit. + * + * @return int + */ + protected function getFinancialTypeID(): int { + return (int) $this->getSubmittedValue('financial_type_id') ?: $this->order->getFinancialTypeID(); + } + } diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php index d3591809b496..b329aa64fb34 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php @@ -827,11 +827,13 @@ public function testSubmitRecurTwoRows(): void { 'frequency_interval' => 1, 'frequency_unit' => 'month', 'membership_type_id' => NULL, + // Set financial type id to null to check it is retrieved from the price set. + 'financial_type_id' => NULL, ]; $form->testSubmit(array_merge($this->getBaseSubmitParams(), $priceParams)); $memberships = $this->callAPISuccess('Membership', 'get')['values']; $this->assertCount(2, $memberships); - $this->callAPISuccessGetSingle('Contribution', []); + $this->callAPISuccessGetSingle('Contribution', ['financial_type_id' => 1]); $this->callAPISuccessGetCount('MembershipPayment', [], 2); $lines = $this->callAPISuccess('LineItem', 'get', ['sequential' => 1])['values']; $this->assertCount(2, $lines);