From 7aa78908c50c23d8fcd15fb757e87a466627ddba Mon Sep 17 00:00:00 2001 From: eileen Date: Sat, 14 Mar 2020 13:29:28 +1300 Subject: [PATCH] Fix bug where tax_amount is miscalculated on membership renewals In order to replicate 1) enable tax & invoicing 2) configure a financial type with sales tax 3) renew a membership, entering an amount other than the default membership amount. 4) view the membership - tax amount will be the same as it would be if the default amount had been used. Tangental bugs I noticed 1) regression in master on adding financial accounts - I will do a separate PR 2) tax term not assigned to the template - out of scope for now but I believe a pre-hook type structure should exist, assigning this to the template when enabled (as part of moving some code to a core-extension). 3) the form shows the tax amount & does not update it - this is pretty big already so that is also out of scope for now This bug affects both membership & membership renewal pages. This PR only fixes for membership renewal but the goal is to fix for membership too once this is merged. It is a fairly 'bug' fix in that it introduces a new class with new methods to get the information otherwise provided by CRM_Price_BAO_PriceSet::processAmount. processAmount is one of those functions that is hard to read and understand & has been hacked to do various things. Code that calls it is hard to read because of this. Having a cleaner way to operate has been a goal for a while. We have plans to build up the Order api to be much more usable and used. However, we really need to build up sensible helpers before we can address that. This PR adds a class that has a whole lot of functions that are more readable to do what the processAmount otherwise does. The goal is to deprecate processAmount. I started on MembershipRenewal as 1) it has a bug currently and 2) it is pretty simple in that it doesn't actually support pricesets - which is especially helpful when testing :-) --- CRM/Financial/BAO/Order.php | 280 ++++++++++++++++++ CRM/Member/Form.php | 4 +- CRM/Member/Form/MembershipRenewal.php | 40 ++- CRM/Price/BAO/PriceSet.php | 2 +- .../CRM/Member/Form/MembershipRenewalTest.php | 50 +++- .../CRM/Member/Form/MembershipTest.php | 6 +- tests/phpunit/CiviTest/CiviUnitTestCase.php | 18 +- 7 files changed, 361 insertions(+), 39 deletions(-) create mode 100644 CRM/Financial/BAO/Order.php diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php new file mode 100644 index 0000000000..cb4385163f --- /dev/null +++ b/CRM/Financial/BAO/Order.php @@ -0,0 +1,280 @@ + 4, price_10 => 7] + * is equivalent to 'option_value 4 for radio price field 3 and + * a quantity of 7 for text price field 10. + * + * @var array + */ + protected $priceSelection = []; + + /** + * Override for financial type id. + * + * Used when the financial type id is to be overridden for all line items + * (as can happen in backoffice forms) + * + * @var int + */ + protected $overrideFinancialTypeID; + + /** + * Override for the total amount of the order. + * + * When there is a single line item the order total may be overriden. + * + * @var float + */ + protected $overrideTotalAmount; + + /** + * Line items in the order. + * + * @var array + */ + protected $lineItems = []; + + /** + * Metadata for price fields. + * + * @var array + */ + protected $priceFieldMetadata = []; + + /** + * Get Set override for total amount of the order. + * + * @return float|false + */ + public function getOverrideTotalAmount() { + if (count($this->getPriceOptions()) !== 1) { + return FALSE; + } + return $this->overrideTotalAmount ?? FALSE; + } + + /** + * Set override for total amount. + * + * @param float $overrideTotalAmount + */ + public function setOverrideTotalAmount(float $overrideTotalAmount) { + $this->overrideTotalAmount = $overrideTotalAmount; + } + + /** + * Get override for total amount. + * + * @return int| FALSE + */ + public function getOverrideFinancialTypeID() { + if (count($this->getPriceOptions()) !== 1) { + return FALSE; + } + return $this->overrideFinancialTypeID ?? FALSE; + } + + /** + * Set override for financial type ID. + * + * @param int $overrideFinancialTypeID + */ + public function setOverrideFinancialTypeID(int $overrideFinancialTypeID) { + $this->overrideFinancialTypeID = $overrideFinancialTypeID; + } + + /** + * Getter for price set id. + * + * @return int + */ + public function getPriceSetID(): int { + return $this->priceSetID; + } + + /** + * Setter for price set id. + * + * @param int $priceSetID + */ + public function setPriceSetID(int $priceSetID) { + $this->priceSetID = $priceSetID; + } + + /** + * Getter for price selection. + * + * @return array + */ + public function getPriceSelection(): array { + return $this->priceSelection; + } + + /** + * Setter for price selection. + * + * @param array $priceSelection + */ + public function setPriceSelection(array $priceSelection) { + $this->priceSelection = $priceSelection; + } + + /** + * Price options the simplified price fields selections. + * + * ie. the 'price_' is stripped off the key name and the field ID + * is cast to an integer. + * + * @return array + */ + public function getPriceOptions():array { + $priceOptions = []; + foreach ($this->getPriceSelection() as $fieldName => $value) { + $fieldID = substr($fieldName, 6); + $priceOptions[(int) $fieldID] = $value; + } + return $priceOptions; + } + + /** + * Get the metadata for the given field. + * + * @param int $id + * + * @return array + * @throws \CiviCRM_API3_Exception + */ + public function getPriceFieldSpec(int $id) :array { + if (!isset($this->priceFieldMetadata[$id])) { + $this->priceFieldMetadata = CRM_Price_BAO_PriceSet::getCachedPriceSetDetail($this->getPriceSetID())['fields']; + } + return $this->priceFieldMetadata[$id]; + } + + /** + * Set the price field selection from an array of params containing price fields. + * + * This function takes the sort of 'anything & everything' parameters that come in from the + * form layer and filters them before assigning them to the priceSelection property. + * + * @param array $input + */ + public function setPriceSelectionFromUnfilteredInput(array $input) { + foreach ($input as $fieldName => $value) { + if (strpos($fieldName, 'price_') === 0) { + $fieldID = substr($fieldName, 6); + if (is_numeric($fieldID)) { + $this->priceSelection[$fieldName] = $value; + } + } + } + } + + /** + * Get line items. + * + * return array + * + * @throws \CiviCRM_API3_Exception + */ + public function getLineItems():array { + if (empty($this->lineItems)) { + $this->lineItems = $this->calculateLineItems(); + } + return $this->lineItems; + } + + /** + * @return array + * @throws \CiviCRM_API3_Exception + */ + protected function calculateLineItems(): array { + $lineItems = []; + $params = $this->getPriceSelection(); + if ($this->getOverrideTotalAmount() !== FALSE) { + // We need to do this to keep getLine from doing weird stuff but the goal + // is to ditch getLine next round of refactoring + // and make the code more sane. + $params['total_amount'] = $this->getOverrideTotalAmount(); + } + + foreach ($this->getPriceOptions() as $fieldID => $valueID) { + $throwAwayArray = []; + // @todo - still using getLine for now but better to bring it to this class & do a better job. + $lineItems[$valueID] = CRM_Price_BAO_PriceSet::getLine($params, $throwAwayArray, $this->getPriceSetID(), $this->getPriceFieldSpec($fieldID), $fieldID, 0)[1][$valueID]; + } + + $taxRates = CRM_Core_PseudoConstant::getTaxRates(); + foreach ($lineItems as &$lineItem) { + // Set any pre-calculation to zero as we will calculate. + $lineItem['tax_amount'] = 0; + if ($this->getOverrideFinancialTypeID() !== FALSE) { + $lineItem['financial_type_id'] = $this->getOverrideFinancialTypeID(); + } + $taxRate = $taxRates[$lineItem['financial_type_id']] ?? 0; + if ($this->getOverrideTotalAmount() !== FALSE) { + if ($taxRate) { + // Total is tax inclusive. + $lineItem['tax_amount'] = ($taxRate / 100) * $this->getOverrideTotalAmount(); + $lineItem['line_total'] = $lineItem['unit_price'] = $this->getOverrideTotalAmount() - $lineItem['tax_amount']; + } + else { + $lineItem['line_total'] = $lineItem['unit_price'] = $this->getOverrideTotalAmount(); + } + } + elseif ($taxRate) { + $lineItem['tax_amount'] = ($taxRate / 100) * $lineItem['line_total']; + } + } + return $lineItems; + } + + /** + * Get the total tax amount for the order. + * + * @return float + * + * @throws \CiviCRM_API3_Exception + */ + public function getTotalTaxAmount() :float { + $amount = 0.0; + foreach ($this->getLineItems() as $lineItem) { + $amount += $lineItem['tax_amount'] ?? 0.0; + } + return $amount; + } + +} diff --git a/CRM/Member/Form.php b/CRM/Member/Form.php index 1408396daf..f1f0749e07 100644 --- a/CRM/Member/Form.php +++ b/CRM/Member/Form.php @@ -453,9 +453,9 @@ class CRM_Member_Form extends CRM_Contribute_Form_AbstractEditPayment { $priceSetID = CRM_Utils_Array::value('price_set_id', $params); if (!$priceSetID) { $priceSetDetails = self::getPriceSetDetails($params); - return key($priceSetDetails); + return (int) key($priceSetDetails); } - return $priceSetID; + return (int) $priceSetID; } /** diff --git a/CRM/Member/Form/MembershipRenewal.php b/CRM/Member/Form/MembershipRenewal.php index 9b4f1d7491..b79cd2315f 100644 --- a/CRM/Member/Form/MembershipRenewal.php +++ b/CRM/Member/Form/MembershipRenewal.php @@ -108,12 +108,6 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { */ public function setDeleteMessage() {} - /** - * Pre-process form. - * - * @throws \Exception - */ - /** * Set the renewal notification status message. */ @@ -128,6 +122,12 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { CRM_Core_Session::setStatus($statusMsg, ts('Complete'), 'success'); } + /** + * Preprocess form. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + */ public function preProcess() { // This string makes up part of the class names, differentiating them (not sure why) from the membership fields. @@ -597,22 +597,17 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { //create line items $lineItem = []; $this->_params = $this->setPriceSetParameters($this->_params); - CRM_Price_BAO_PriceSet::processAmount($this->_priceSet['fields'], - $this->_params, $lineItem[$this->_priceSetId], $this->_priceSetId - ); - //CRM-11529 for quick config backoffice transactions - //when financial_type_id is passed in form, update the - //line items with the financial type selected in form - if ($submittedFinancialType = CRM_Utils_Array::value('financial_type_id', $this->_params)) { - foreach ($lineItem[$this->_priceSetId] as &$li) { - $li['financial_type_id'] = $submittedFinancialType; - } - } - if (!empty($lineItem)) { - $this->_params['lineItems'] = $lineItem; - $this->_params['processPriceSet'] = TRUE; - } + $order = new CRM_Financial_BAO_Order(); + $order->setPriceSelectionFromUnfilteredInput($this->_params); + $order->setPriceSetID(self::getPriceSetID($this->_params)); + $order->setOverrideTotalAmount($this->_params['total_amount']); + $order->setOverrideFinancialTypeID((int) $this->_params['financial_type_id']); + + $this->_params['lineItems'][$this->_priceSetId] = $order->getLineItems(); + // This is one of those weird & wonderful legacy params we aim to get rid of. + $this->_params['processPriceSet'] = TRUE; + $this->_params['tax_amount'] = $order->getTotalTaxAmount(); //assign contribution contact id to the field expected by recordMembershipContribution if ($this->_contributorContactID != $this->_contactID) { @@ -634,7 +629,8 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { 'contribution_recur_id' => $contributionRecurID, ]); //Remove `tax_amount` if it is not calculated. - if (CRM_Utils_Array::value('tax_amount', $temporaryParams) === 0) { + // ?? WHY - I haven't been able to figure out... + if (CRM_Utils_Array::value('tax_amount', $temporaryParams) === 0.0) { unset($temporaryParams['tax_amount']); } CRM_Member_BAO_Membership::recordMembershipContribution($temporaryParams); diff --git a/CRM/Price/BAO/PriceSet.php b/CRM/Price/BAO/PriceSet.php index f9e24c14d9..561eb6b42f 100644 --- a/CRM/Price/BAO/PriceSet.php +++ b/CRM/Price/BAO/PriceSet.php @@ -1693,7 +1693,7 @@ WHERE ct.id = cp.financial_type_id AND * * @return array */ - protected static function getLine(&$params, &$lineItem, $priceSetID, $field, $id, $totalPrice): array { + public static function getLine(&$params, &$lineItem, $priceSetID, $field, $id, $totalPrice): array { $totalTax = 0; switch ($field['html_type']) { case 'Text': diff --git a/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php b/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php index 7bafb80f67..9f1bd1563d 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php @@ -89,6 +89,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'fixed_period_start_day' => '101', 'fixed_period_rollover_day' => '1231', 'relationship_type_id' => 20, + 'min_fee' => 100, 'financial_type_id' => $this->financialTypeID, ])['id']; @@ -203,6 +204,53 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { ], CRM_Core_Session::singleton()->getStatus()); } + /** + * Test submitting with tax enabled. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + */ + public function testSubmitWithTax() { + $this->enableTaxAndInvoicing(); + $this->relationForFinancialTypeWithFinancialAccount($this->financialTypeID); + $form = $this->getForm(); + + $form->testSubmit([ + 'cid' => $this->_individualId, + 'join_date' => date('Y-m-d'), + 'start_date' => '', + 'end_date' => '', + // This format reflects the 23 being the organisation & the 25 being the type. + 'membership_type_id' => [23, $this->membershipTypeAnnualFixedID], + 'auto_renew' => '0', + 'max_related' => '', + 'num_terms' => '1', + 'source' => '', + 'total_amount' => '50.00', + 'financial_type_id' => $this->financialTypeID, + 'from_email_address' => '"Demonstrators Anonymous" ', + 'receipt_text_signup' => 'Thank you text', + 'payment_processor_id' => $this->_paymentProcessorID, + 'credit_card_number' => '4111111111111111', + 'cvv2' => '123', + 'credit_card_exp_date' => [ + 'M' => '9', + 'Y' => date('Y') + 2, + ], + 'credit_card_type' => 'Visa', + 'billing_first_name' => 'Test', + 'billing_middlename' => 'Last', + 'billing_street_address-5' => '10 Test St', + 'billing_city-5' => 'Test', + 'billing_state_province_id-5' => '1003', + 'billing_postal_code-5' => '90210', + 'billing_country_id-5' => '1228', + ]); + $contribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $this->_individualId, 'is_test' => TRUE, 'return' => ['total_amount', 'tax_amount']]); + $this->assertEquals(50, $contribution['total_amount']); + $this->assertEquals(5, $contribution['tax_amount']); + } + /** * Test the submit function of the membership form. * @@ -456,7 +504,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'return' => ['tax_amount', 'trxn_id'], ]); $this->assertEquals($contribution['trxn_id'], 777); - $this->assertEquals($contribution['tax_amount'], NULL); + $this->assertEquals(NULL, $contribution['tax_amount']); $this->callAPISuccessGetCount('LineItem', [ 'entity_id' => $membership['id'], diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php index 0435fe9029..ec0cee5f8f 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php @@ -1430,10 +1430,6 @@ Expires: ', $financialItems_sum += $financialItem['amount']; } $this->assertEquals($contribution['total_amount'], $financialItems_sum); - - // reset the price options static variable so not leave any dummy data, that might hamper other unit tests - \Civi::$statics['CRM_Price_BAO_PriceField']['priceOptions'] = NULL; - $this->disableTaxAndInvoicing(); } /** @@ -1451,7 +1447,7 @@ Expires: ', $this->createLoggedInUser(); $membershipTypeAnnualRolling = $this->callAPISuccess('membership_type', 'create', [ 'domain_id' => 1, - 'name' => "AnnualRollingNew", + 'name' => 'AnnualRollingNew', 'member_of_contact_id' => 23, 'duration_unit' => "year", 'minimum_fee' => 50, diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 173c06e6e1..7f5614298a 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -3040,18 +3040,20 @@ VALUES /** * Enable Tax and Invoicing + * + * @throws \CRM_Core_Exception */ - protected function disableTaxAndInvoicing($params = []) { + protected function disableTaxAndInvoicing() { + $accounts = $this->callAPISuccess('EntityFinancialAccount', 'get', ['account_relationship' => 'Sales Tax Account is'])['values']; + foreach ($accounts as $account) { + $this->callAPISuccess('EntityFinancialAccount', 'delete', ['id' => $account['id']]); + $this->callAPISuccess('FinancialAccount', 'delete', ['id' => $account['financial_account_id']]); + } + if (!empty(\Civi::$statics['CRM_Core_PseudoConstant']) && isset(\Civi::$statics['CRM_Core_PseudoConstant']['taxRates'])) { unset(\Civi::$statics['CRM_Core_PseudoConstant']['taxRates']); } - // Enable component contribute setting - $contributeSetting = array_merge($params, - [ - 'invoicing' => 0, - ] - ); - return Civi::settings()->set('contribution_invoice_settings', $contributeSetting); + return Civi::settings()->set('invoicing', FALSE); } /** -- 2.25.1