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.7 #9711

Merged
merged 2 commits into from
Feb 3, 2017
Merged

CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.7 #9711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f5911b
Fundamental Fixes for TaxMath Calculations.
KarinG Jan 23, 2017
57d395b
Add PHPUnit Test - testSubmitContributionPageWithPriceSetQuantity.
KarinG Jan 25, 2017
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 @@ -475,7 +475,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);
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.


return $taxAmount;
}
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 @@ -298,7 +298,15 @@ public static function getLineItems($entityId, $entity = 'participant', $isQuick
'tax_amount' => $dao->tax_amount,
'price_set_id' => $dao->price_set_id,
);
$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 all 8 decimals
$lineItems[$dao->id]['tax_rate'] = round($taxRates[$lineItems[$dao->id]['financial_type_id']], 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

civicrm_financial_account.tax_rate allows 8 precision (number of digits after the decimal point), so we should not round this off to 3. If the concern is with tax rate like 10.40300000 or 10.0558000, then i think casting would fix the problem
eg.
$lineItems[$dao->id]['tax_rate'] = (float) $taxRates[$lineItems[$dao->id]['financial_type_id']];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok - let me try that. I have a couple scenarios here I can test that with.

}
else {
// There is no Tax Rate associated with this Financial Type
$lineItems[$dao->id]['tax_rate'] = FALSE;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move line 301-309 to a function calculateTaxRate() ? So that we can reuse the function.

Copy link
Contributor Author

@KarinG KarinG Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not to a function called calculateTaxRate - part of the fundamental issue I addressed was that TaxRate should -never- be calculated [so I removed the existing function calculateTaxRate]. If should simply be retrieved. I can move this into a functional called retrieveTaxRate; That would be cleaner;

$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 @@ -597,26 +605,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 || $lineItemId['qty'] == 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 @@ -1666,11 +1666,12 @@ public static function copyPriceSet($baoName, $id, $newId) {
* @return array
*/
public static function setLineItem($field, $lineItem, $optionValueId, &$totalTax) {
// 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 @@ -78,7 +78,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

@JoeMurray JoeMurray 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.

Let's change {$line.tax_rate|string_format:"%.3f"} to {$line.tax_rate|rtrim:'0'} as per http://stackoverflow.com/questions/26565650/smarty-trim-zeros-from-the-end-of-string-float-value both here and in 4.6?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rtrim:'0' would that not make 10% ->1%?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove string_format and since it would be handled in php at https://github.com/civicrm/civicrm-core/pull/9711/files#r98392553

<td class="right">{$line.tax_amount|crmMoney}</td>
{else}
<td></td>
Expand Down
62 changes: 62 additions & 0 deletions tests/phpunit/api/v3/ContributionPageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1416,4 +1416,66 @@ public function addPriceFields(&$params) {
);
}

/**
* Test Tax Amount is calculated properly when using PriceSet with Field Type = Text/Numeric Quantity
*/
public function testSubmitContributionPageWithPriceSetQuantity() {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks good - I'm glad you found a way to actually test the page rather than just a true unit test as I think it's more reassuring

$this->_priceSetParams['is_quick_config'] = 0;
$this->enableTaxAndInvoicing();
$financialType = $this->createFinancialType();
$financialTypeId = $financialType['id'];
// This function sets the Tax Rate at 10% - it currently has no way to pass Tax Rate into it - so let's work with 10%
$financialAccount = $this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5);
Copy link
Contributor

@pradpnayak pradpnayak Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

relationForFinancialTypeWithFinancialAccount() has only single arg. You will need to change in function to accept second arg.

https://github.com/civicrm/civicrm-core/pull/9643/files#diff-b1bdd8138056a9200ee125bf804dcedeR3677

Copy link
Contributor Author

@KarinG KarinG Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I know - but this 10% will do for this test - that comment in the test makes it clear to anyone looking at the test that the TaxRate is 10% - otherwise the math/logic below is difficult to follow.


$this->setUpContributionPage();
$submitParams = array(
'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']),
'id' => (int) $this->_ids['contribution_page'],
'first_name' => 'J',
'last_name' => 'T',
'email' => 'JT@ohcanada.ca',
'is_pay_later' => TRUE,
);

// Create PriceSet/PriceField
$priceSetID = reset($this->_ids['price_set']);
$priceField = $this->callAPISuccess('price_field', 'create', array(
'price_set_id' => $priceSetID,
'label' => 'Printing Rights',
'html_type' => 'Text',
));
$priceFieldValue = $this->callAPISuccess('price_field_value', 'create', array(
'price_set_id' => $priceSetID,
'price_field_id' => $priceField['id'],
'label' => 'Printing Rights',
'financial_type_id' => $financialTypeId,
'amount' => '16.95',
));
$priceFieldId = $priceField['id'];

// Set quantity for our test
$submitParams['price_' . $priceFieldId] = 180;

// contribution_page submit requires amount and tax_amount - and that's ok we're not testing that - we're testing at the LineItem level
$submitParams['amount'] = 180 * 16.95;
// This is the correct Tax Amount - use it later to compare to what the CiviCRM Core came up with at the LineItem level
$submitParams['tax_amount'] = 180 * 16.95 * 0.10;

$this->callAPISuccess('contribution_page', 'submit', $submitParams);
$contribution = $this->callAPISuccessGetSingle('contribution', array(
'contribution_page_id' => $this->_ids['contribution_page'],
));

// Retrieve the lineItem that belongs to the Printing Rights and check the tax_amount CiviCRM Core calculated for it
$lineItem = $this->callAPISuccess('LineItem', 'get', array(
'contribution_id' => $contribution['id'],
'label' => 'Printing Rights',
));
$lineItemId = $lineItem['id'];
$lineItem_TaxAmount = round($lineItem['values'][$lineItemId]['tax_amount'], 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we round tax_amount? I guess its stored in 2 digit after decimal.

Copy link
Contributor Author

@KarinG KarinG Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes - and some payment processors appear to only process amounts with 2 decimals.


// Compare this to what it should be!
$this->assertEquals($lineItem_TaxAmount, round($submitParams['tax_amount'], 2), 'Wrong Sales Tax Amount is calculated and stored.');
}

}