CRM-21656: Backend Membership with Priceset applies taxes twice to line_item #11521
Conversation
|
ping @mlutfy @eileenmcnaughton |
CRM/Price/BAO/PriceSet.php
Outdated
| @@ -430,11 +430,12 @@ public static function getAssoc($withInactive = FALSE, $extendComponentName = FA | |||
| * Appears to have no effect based on reading the code. | |||
| * @param bool $validOnly | |||
| * Should only fields where today's date falls within the valid range be returned? | |||
| * @param bool $resetOptions | |||
There was a problem hiding this comment.
This parameter is introduced for unit test which needs to reload price options when sale tax is enabled, in order to get tax_amount and tax_rate info.
There was a problem hiding this comment.
I think we should switch to using \Civi::$statics inside the CRM_Price_BAO_PriceField::getOptions rather than add this param if it is to support unit tests - we have been doing that elsewhere
| @@ -3774,6 +3777,9 @@ protected function relationForFinancialTypeWithFinancialAccount($financialTypeId | |||
| 'account_relationship' => key(CRM_Core_PseudoConstant::accountOptionValues('account_relationship', NULL, " AND v.name LIKE 'Sales Tax Account is' ")), | |||
| ); | |||
|
|
|||
| // set tax rate (as 10) for provided financial type ID to static variable, later used to fetch tax rates of all financial types | |||
| \Civi::$statics['CRM_Core_PseudoConstant']['taxRates'][$financialTypeId] = 10; | |||
There was a problem hiding this comment.
There is no other way where we can update this static variable (which holds the tax rates of all FT) after setting a Taxable financial account to a financial type.
|
@eileenmcnaughton thanks for the tip :) used |
|
This looks solid to me - lets squash to one patch (to avoid adding & removing the $reset param) and merge once the test passes |
|
Done 👍 |
|
OK - the change itself is minor and logical |
|
Jenkins test this please |
|
I think that might be related - the fail is about ensuring that money is cleaned at the form layer not low down - good to get it fixed |
CRM/Price/BAO/PriceSet.php
Outdated
| if (CRM_Utils_Array::value('field_title', $lineItem[$optionValueId]) == 'Membership Amount') { | ||
| $lineItem[$optionValueId]['line_total'] = $lineItem[$optionValueId]['unit_price'] = CRM_Utils_Rule::cleanMoney($lineItem[$optionValueId]['line_total'] - $lineItem[$optionValueId]['tax_amount']); | ||
| } | ||
| $lineItem[$optionValueId]['line_total'] = $lineItem[$optionValueId]['unit_price'] = CRM_Utils_Rule::cleanMoney($lineItem[$optionValueId]['line_total'] - $lineItem[$optionValueId]['tax_amount']); |
There was a problem hiding this comment.
This is causing weird things in the Contribution Confirm & ThankYou pages, when using pricesets:
- say a membership is 100$ with a 5$ tax,
- then it will show: 95$ membership, 5$ tax, total 100$.
|
@mlutfy I have updated the PR, can you please check now? |
|
@monishdeb yep, works! thanks! I also re-tested some other use-cases, such as a simple membership without a priceset. |
|
test this please |
|
Tests have passed and seems to have been reviewed and tested by @mlutfy so I think this is good to merge now |
Overview
Steps to reproduce:
Enable Taxes, Add a tax account with a 10% tax
Notice how to total amount is OK, but when we view the contribution, the line
item shows a unit_price with the tax_amount included, so then the total_line
ends up adding the taxes again.
Before
After
Technical Details
Also, there is a minor change in contribution backoffice form, where earlier the fn responsible to fetch line-item, got missing parameter $priceSetID.