-
-
Notifications
You must be signed in to change notification settings - Fork 814
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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 :-)
- Loading branch information
1 parent
0f4c9fa
commit ee49b13
Showing
5 changed files
with
349 additions
and
24 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,280 @@ | ||
| <?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 | | ||
| +--------------------------------------------------------------------+ | ||
| */ | ||
|
|
||
| /** | ||
| * | ||
| * @package CRM | ||
| * @copyright CiviCRM LLC https://civicrm.org/licensing | ||
| * | ||
| * Order class. | ||
| * | ||
| * This class is intended to become the object to manage orders, including via Order.api. | ||
| * | ||
| * As of writing it is in the process of having appropriate functions built up. | ||
| */ | ||
| class CRM_Financial_BAO_Order { | ||
|
|
||
| /** | ||
| * Price set id. | ||
| * | ||
| * @var int | ||
| */ | ||
| protected $priceSetID; | ||
|
|
||
| /** | ||
| * Selected price items in the format we see in forms. | ||
| * | ||
| * ie. | ||
| * [price_3 => 4, price_10 => 7] | ||
| * is equivalent to 'option_value 4 for radio price field 4 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->calculateLineIItems(); | ||
| } | ||
| return $this->lineItems; | ||
| } | ||
|
|
||
| /** | ||
| * @return array | ||
| * @throws \CiviCRM_API3_Exception | ||
| */ | ||
| protected function calculateLineIItems(): 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; | ||
| } | ||
|
|
||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.