Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.6 #9710

Merged
merged 1 commit into from
Jan 31, 2017
Merged

CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.6 #9710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CRM/Contribute/BAO/Contribution/Utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,8 @@ public static function getFirstLastDetails($contactID) {
*/
public static function calculateTaxAmount($amount, $taxRate) {
$taxAmount = array();
$taxAmount['tax_amount'] = round(($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount), 2);
// There can not be any rounding at this stage - as this is prior to quantity multiplication
$taxAmount['tax_amount'] = ($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount);

return $taxAmount;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally wrong to round here - as at this stage - in this basic Utils function quantity has not yet been taken into account. Rounding here causes incorrect Sales Tax calculations.

Expand Down
32 changes: 9 additions & 23 deletions CRM/Price/BAO/LineItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,15 @@ public static function getLineItems($entityId, $entity = 'participant', $isQuick
'membership_num_terms' => $dao->membership_num_terms,
'tax_amount' => $dao->tax_amount,
);
$lineItems[$dao->id]['tax_rate'] = CRM_Price_BAO_LineItem::calculateTaxRate($lineItems[$dao->id]);
$taxRates = CRM_Core_PseudoConstant::getTaxRates();
if (isset($lineItems[$dao->id]['financial_type_id']) && array_key_exists($lineItems[$dao->id]['financial_type_id'], $taxRates)) {
// We are close to output/display here - so apply some rounding at output/display level - to not show Tax Rate in it's full 8 decimals
$lineItems[$dao->id]['tax_rate'] = round($taxRates[$lineItems[$dao->id]['financial_type_id']], 3);
}
else {
// There is no Tax Rate associated with this Financial Type
$lineItems[$dao->id]['tax_rate'] = FALSE;
}
$lineItems[$dao->id]['subTotal'] = $lineItems[$dao->id]['qty'] * $lineItems[$dao->id]['unit_price'];
if ($lineItems[$dao->id]['tax_amount'] != '') {
$getTaxDetails = TRUE;
Expand Down Expand Up @@ -537,26 +545,4 @@ public static function getLineItemArray(&$params, $entityId = NULL, $entityTable
}
}

/**
* Calculate tax rate in percentage.
*
* @param array $lineItemId
* An assoc array of lineItem.
*
* @return int|void
* tax rate
*/
public static function calculateTaxRate($lineItemId) {
if ($lineItemId['unit_price'] == 0) {
return FALSE;
}
if ($lineItemId['html_type'] == 'Text') {
$tax = round($lineItemId['tax_amount'] / ($lineItemId['unit_price'] * $lineItemId['qty']) * 100, 2);
}
else {
$tax = round(($lineItemId['tax_amount'] / $lineItemId['unit_price']) * 100, 2);
}
return $tax;
}

}
5 changes: 3 additions & 2 deletions CRM/Price/BAO/PriceSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,11 +1536,12 @@ public static function copyPriceSet($baoName, $id, $newId) {
* @return array
*/
public static function setLineItem($field, $lineItem, $optionValueId) {
// Here we round - i.e. after multiplying by quantity
if ($field['html_type'] == 'Text') {
$taxAmount = $field['options'][$optionValueId]['tax_amount'] * $lineItem[$optionValueId]['qty'];
$taxAmount = round($field['options'][$optionValueId]['tax_amount'] * $lineItem[$optionValueId]['qty'], 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can round - we have now multiplied by quantity.

}
else {
$taxAmount = $field['options'][$optionValueId]['tax_amount'];
$taxAmount = round($field['options'][$optionValueId]['tax_amount'], 2);
}
$taxRate = $field['options'][$optionValueId]['tax_rate'];
$lineItem[$optionValueId]['tax_amount'] = $taxAmount;
Expand Down
2 changes: 1 addition & 1 deletion templates/CRM/Price/Page/LineItem.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
{if $getTaxDetails}
<td class="right">{$line.line_total|crmMoney}</td>
{if $line.tax_rate != "" || $line.tax_amount != ""}
<td class="right">{$taxTerm} ({$line.tax_rate|string_format:"%.2f"}%)</td>
<td class="right">{$taxTerm} ({$line.tax_rate|string_format:"%.3f"}%)</td>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some Tax Rates are 3 digits [like e.g. in QEO: 9.975%] - so let's not constrain them to 2 digits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Karen, I like what you are doing above, but I'm unsure why you would want to truncate the display of the tax rate on display here. If the user sets up more digits of accuracy when configuring the tax rate, shouldn't we just display what they entered? Our schema provides 8 digits of accuracy after the decimal. If you want to constrain to 3 digits on display, we should also constrain on input, and likely change the schema to reflect this new reduction in accuracy.

Copy link
Contributor Author

@KarinG KarinG Jan 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not introducing a new reduction in accuracy. In fact I'm increasing the accuracy with which the Tax Rate is displayed on the View Contribution screen - example: from 9.98 to 9.975 for admins who deal with QEO Sales Tax.

If we don't format it here - (neither .2 nor .3) then when orgs configure their financial accounts w/ 5% for tax rate -> it will display as 5.00000000 and for QEO 9.975% -> it will display as 9.97500000;

That's likely why this %.2f has always existed here. I'm just trying to relax that. The actual Tax Math is (w/ this PR) all done with all 8 decimals.

-- Karin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe try putting out all 8 decimals from the schema, then rtrim the trailing zeroes. This will make 10 appear as 10.%. If you would prefer in this case to get rid of the decimal point, then add rtrim on period after the rtrim on 0's. So most complicated would be: {$line.tax_rate|string_format:"%.8f"|rtrim:'0'|rtrim:'.'}
I haven't tested, but I expect this would render 10 as 10% and 9.975 as 9.975%.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added this small/safe/fix to get 3 decimals (like QEO) @mlutfy Tax Rates displayed properly for admins. Please note that the PDF invoice - see above - does display the Tax Rate without any trailing zeros. There probably should be review/check to see how/where what is displayed re: Tax Rates [from input GUI to schema to output on screens etc] - but that's for another PR. These fundamental math issues need fixing now.

<td class="right">{$line.tax_amount|crmMoney}</td>
{else}
<td></td>
Expand Down