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-16228, fixed tax calculation as per https://github.com/civicrm/ci… #9643

Closed
wants to merge 3 commits into from
Closed

CRM-16228, fixed tax calculation as per https://github.com/civicrm/ci… #9643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd64e4b
CRM-16228, fixed tax calculation as per https://github.com/civicrm/ci…
pradpnayak Jan 5, 2017
c99cfe1
CRM-16228, rounding off to $taxAmount variable
pradpnayak Jan 5, 2017
9a13c4a
CRM-16228, added test
pradpnayak Jan 5, 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
8 changes: 5 additions & 3 deletions CRM/Contribute/BAO/Contribution/Utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,10 +473,12 @@ public static function getFirstLastDetails($contactID) {
* array of tax amount
*
*/
public static function calculateTaxAmount($amount, $taxRate) {
public static function calculateTaxAmount($amount, $taxRate, $htmlType = NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this an odd place to have $htmlType? A static utility function, which calculates a tax based on an amount and a rate, shouldn't need to know something like $htmlType. I see that it's used to decide whether or not to round the amount, but why does this rounding happen in this function -- or specifically, why does it happen only for certain values of $htmlType? Shouldn't rounding happen somewhere later, just at the time this value actually needs to be rounded (e.g., at display time)?

$taxAmount = array();
$taxAmount['tax_amount'] = round(($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount), 2);

$taxAmount['tax_amount'] = ($taxRate / 100) * CRM_Utils_Rule::cleanMoney($amount);
if ($htmlType != 'Text') {
$taxAmount['tax_amount'] = round($taxAmount['tax_amount'], 2);
}
return $taxAmount;
}

Expand Down
4 changes: 4 additions & 0 deletions CRM/Contribute/Form/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1416,6 +1416,10 @@ public function testSubmit($params, $action, $creditCardMode = NULL) {
$_SERVER['REQUEST_METHOD'] = 'GET';
$this->controller = new CRM_Core_Controller();

if (CRM_Utils_Array::value('price_set_id', $params)) {
$this->_priceSet = current(CRM_Price_BAO_PriceSet::getSetDetail($params['price_set_id']));
}

CRM_Contribute_Form_AdditionalInfo::buildPremium($this);

$this->_fields = array();
Expand Down
4 changes: 2 additions & 2 deletions CRM/Contribute/Form/Contribution/Confirm.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,7 +1914,7 @@ public static function submit($params) {
$form->controller = new CRM_Contribute_Controller_Contribution();
$params['invoiceID'] = md5(uniqid(rand(), TRUE));
$paramsProcessedForForm = $form->_params = self::getFormParams($params['id'], $params);
$form->_amount = $params['amount'];
$form->_amount = CRM_Utils_Array::value('amount', $params, 0);
// hack these in for test support.
$form->_fields['billing_first_name'] = 1;
$form->_fields['billing_last_name'] = 1;
Expand Down Expand Up @@ -1951,7 +1951,7 @@ public static function submit($params) {
}

$priceFields = $priceFields[$priceSetID]['fields'];
CRM_Price_BAO_PriceSet::processAmount($priceFields, $paramsProcessedForForm, $lineItems, 'civicrm_contribution');
CRM_Price_BAO_PriceSet::processAmount($priceFields, $form->_params, $lineItems, 'civicrm_contribution');
$form->_lineItem = array($priceSetID => $lineItems);
$membershipPriceFieldIDs = array();
foreach ((array) $lineItems as $lineItem) {
Expand Down
4 changes: 2 additions & 2 deletions CRM/Price/BAO/PriceField.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ public static function addQuickFormElement(
* @return array
* array of options
*/
public static function getOptions($fieldId, $inactiveNeeded = FALSE, $reset = FALSE, $isDefaultContributionPriceSet = FALSE) {
public static function getOptions($fieldId, $inactiveNeeded = FALSE, $reset = FALSE, $isDefaultContributionPriceSet = FALSE, $htmlType = NULL) {
static $options = array();
if ($reset) {
$options = array();
Expand All @@ -617,7 +617,7 @@ public static function getOptions($fieldId, $inactiveNeeded = FALSE, $reset = FA
foreach ($options[$fieldId] as $priceFieldId => $priceFieldValues) {
if (isset($priceFieldValues['financial_type_id']) && array_key_exists($priceFieldValues['financial_type_id'], $taxRates) && !$isDefaultContributionPriceSet) {
$options[$fieldId][$priceFieldId]['tax_rate'] = $taxRates[$priceFieldValues['financial_type_id']];
$taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceFieldValues['amount'], $options[$fieldId][$priceFieldId]['tax_rate']);
$taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceFieldValues['amount'], $options[$fieldId][$priceFieldId]['tax_rate'], $htmlType);
$options[$fieldId][$priceFieldId]['tax_amount'] = $taxAmount['tax_amount'];
}
}
Expand Down
3 changes: 2 additions & 1 deletion CRM/Price/BAO/PriceSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ public static function getSetDetail($setID, $required = TRUE, $validOnly = FALSE
}
$setTree[$setID]['fields'][$fieldID][$field] = $dao->$field;
}
$setTree[$setID]['fields'][$fieldID]['options'] = CRM_Price_BAO_PriceField::getOptions($fieldID, FALSE, FALSE, $isDefaultContributionPriceSet);
$setTree[$setID]['fields'][$fieldID]['options'] = CRM_Price_BAO_PriceField::getOptions($fieldID, FALSE, FALSE, $isDefaultContributionPriceSet, $dao->html_type);
}

// also get the pre and post help from this price set
Expand Down Expand Up @@ -1672,6 +1672,7 @@ public static function setLineItem($field, $lineItem, $optionValueId, &$totalTax
else {
$taxAmount = $field['options'][$optionValueId]['tax_amount'];
}
$taxAmount = round($taxAmount, 2);
$taxRate = $field['options'][$optionValueId]['tax_rate'];
$lineItem[$optionValueId]['tax_amount'] = $taxAmount;
$lineItem[$optionValueId]['tax_rate'] = $taxRate;
Expand Down
19 changes: 9 additions & 10 deletions api/v3/examples/ContributionPage/Submit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,12 @@
*/
function contribution_page_submit_example() {
$params = array(
'price_3' => '',
'id' => 1,
'amount' => 10,
'amount' => 100,
'billing_first_name' => 'Billy',
'billing_middle_name' => 'Goat',
'billing_last_name' => 'Gruff',
'email' => 'billy@goat.gruff',
'selectMembership' => array(
'0' => 1,
),
'payment_processor_id' => 1,
'credit_card_number' => '4111111111111111',
'credit_card_type' => 'Visa',
Expand All @@ -27,9 +23,11 @@ function contribution_page_submit_example() {
'Y' => 2040,
),
'cvv2' => 123,
'is_recur' => 1,
'frequency_interval' => 1,
'frequency_unit' => 'month',
'pledge_frequency_interval' => 1,
'pledge_frequency_unit' => 'week',
'pledge_installments' => 3,
'is_pledge' => TRUE,
'pledge_block_id' => 2,
);

try{
Expand All @@ -41,7 +39,8 @@ function contribution_page_submit_example() {
$errorCode = $e->getErrorCode();
$errorData = $e->getExtraParams();
return array(
'error' => $errorMessage,
'is_error' => 1,
'error_message' => $errorMessage,
'error_code' => $errorCode,
'error_data' => $errorData,
);
Expand Down Expand Up @@ -70,7 +69,7 @@ function contribution_page_submit_expectedresult() {

/*
* This example has been generated from the API test suite.
* The test that created it is called "testSubmitMembershipPriceSetPaymentPaymentProcessorRecurDelayed"
* The test that created it is called "testSubmitPledgePaymentPaymentProcessorRecurFuturePayment"
* and can be found at:
* https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/api/v3/ContributionPageTest.php
*
Expand Down
78 changes: 78 additions & 0 deletions tests/phpunit/CRM/Contribute/Form/ContributionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,4 +851,82 @@ public function testSubmitWithOutSaleTax() {
$this->assertTrue(empty($lineItem['tax_amount']));
}

/**
* Test form submission CRM-16228.
*/
public function testSubmitContributionPageWithPriceSetHavingTaxCRM16228() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Because unit tests are so important, it's also important that unit tests have very clear names. A name like "testContributionPageWithPriceSetHavingTaxHasCorrectTaxAndTotalAmounts" would be better than "testSubmitContributionPageWithPriceSetHavingTaxCRM16228" because it plainly says what's being tested, without requiring someone to look-up and read through a JIRA ticket. This is an easy fix to improve our ability to keep writing unit tests on financial code.

$this->enableTaxAndInvoicing();
$financialType = $this->createFinancialType();
$this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5);
$priceSetID = $this->createContributionPriceSet($financialType['id']);
$priceFieldId = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '16.85');
$form = new CRM_Contribute_Form_Contribution();
$submitParams = array(
'financial_type_id' => $financialType['id'],
'receive_date' => '04/21/2015',
'receive_date_time' => '11:27PM',
'contact_id' => $this->_individualId,
'payment_instrument_id' => array_search('Check', $this->paymentInstruments),
'contribution_status_id' => 1,
'price_set_id' => $priceSetID,
'price_' . $priceFieldId => 10,
);
$form->testSubmit($submitParams, CRM_Core_Action::ADD);
$contribution = $this->callAPISuccessGetSingle('Contribution',
array(
'contact_id' => $this->_individualId,
'return' => array('tax_amount', 'total_amount'),
)
);
$this->assertEquals($contribution['tax_amount'], '8.43', 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($contribution['total_amount'], '176.93', 'Total amount no matching.');
$lineItem = $this->callAPISuccessGetSingle('LineItem', array(
'contribution_id' => $contribution['id'],
));
$this->assertEquals($lineItem['tax_amount'], '8.43', 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($lineItem['line_total'], '168.50', 'Total amount no matching.');
}

/**
* Test form submission CRM-16228.
*/
public function testSubmitContributionPageWithPriceSetHaving2PriceFieldTaxCRM16228() {
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, it would be very helpful to rename this test to "testContributionPageWithPriceSetHaving2PriceFieldTaxHasCorrectTaxAndTotalAmounts" (because: plainly says what's being tested; doesn't require examining a JIRA ticket to understand). An important and easy fix to improve our ability to keep writing unit tests on financial code.

$this->enableTaxAndInvoicing();
$financialType = $this->createFinancialType();
$this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5);
$priceSetID = $this->createContributionPriceSet($financialType['id']);
$priceFieldId = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '16.85');
$priceFieldId2 = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '13.33');
$form = new CRM_Contribute_Form_Contribution();
$submitParams = array(
'financial_type_id' => $financialType['id'],
'receive_date' => '04/21/2015',
'receive_date_time' => '11:27PM',
'contact_id' => $this->_individualId,
'payment_instrument_id' => array_search('Check', $this->paymentInstruments),
'contribution_status_id' => 1,
'price_set_id' => $priceSetID,
'price_' . $priceFieldId => 10,
'price_' . $priceFieldId2 => 100,
);
$form->testSubmit($submitParams, CRM_Core_Action::ADD);
$contribution = $this->callAPISuccessGetSingle('Contribution',
array(
'contact_id' => $this->_individualId,
'return' => array('tax_amount', 'total_amount'),
)
);
$this->assertEquals($contribution['tax_amount'], '75.08', 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($contribution['total_amount'], '1576.58', 'Total amount no matching.');
$lineItem = $this->callAPISuccess('LineItem', 'get', array(
'contribution_id' => $contribution['id'],
));
$lineTotal = array('1333', '168.50');
$lineTaxAmount = array('66.65', '8.43');
foreach ($lineItem['values'] as $line) {
$this->assertEquals(CRM_Utils_Array::value('tax_amount', $line), array_pop($lineTaxAmount), 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($line['line_total'], array_pop($lineTotal), 'Total amount no matching.');
}
}

}
43 changes: 41 additions & 2 deletions tests/phpunit/CiviTest/CiviUnitTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3674,13 +3674,13 @@ protected function enableTaxAndInvoicing($params = array()) {
*
* @return obj
*/
protected function relationForFinancialTypeWithFinancialAccount($financialTypeId) {
protected function relationForFinancialTypeWithFinancialAccount($financialTypeId, $taxRate = 10) {
$params = array(
'name' => 'Sales tax account ' . substr(sha1(rand()), 0, 4),
'financial_account_type_id' => key(CRM_Core_PseudoConstant::accountOptionValues('financial_account_type', NULL, " AND v.name LIKE 'Liability' ")),
'is_deductible' => 1,
'is_tax' => 1,
'tax_rate' => 10,
'tax_rate' => $taxRate,
'is_active' => 1,
);
$account = CRM_Financial_BAO_FinancialAccount::add($params);
Expand Down Expand Up @@ -3767,4 +3767,43 @@ public function createPriceSetWithPage($entity = NULL, $params = array()) {
$this->_ids['membership_type'] = $membershipTypeID;
}

/**
* Function to add additional price fields to priceset with tax.
* @param int $priceSetID
* @param int $financialTypeId
* @param float $textFieldAmount
*/
public function addTaxTextPriceFields($priceSetID, $financialTypeId, $textFieldAmount) {
$priceField = $this->callAPISuccess('price_field', 'create', array(
'price_set_id' => $priceSetID,
'label' => 'No. of Person(s)' . substr(sha1(rand()), 0, 7),
'html_type' => 'Text',
));
$priceFieldValue = $this->callAPISuccess('price_field_value', 'create', array(
'price_set_id' => $priceSetID,
'price_field_id' => $priceField['id'],
'label' => 'No. of Person(s)' . substr(sha1(rand()), 0, 7),
'financial_type_id' => $financialTypeId,
'amount' => $textFieldAmount,
));
// clear cached values in this function.
CRM_Price_BAO_PriceField::getOptions(NULL, FALSE, TRUE);
return $priceField['id'];
}

/**
* Add Contribution PriceSet.
* @param string $financialType
*/
public function createContributionPriceSet($financialType = 'Donation') {
$params = array(
'is_quick_config' => 0,
'extends' => 'CiviContribute',
'financial_type_id' => $financialType,
'title' => 'Price-set-test' . substr(sha1(rand()), 0, 7),
);
$priceSet = $this->callAPISuccess('price_set', 'create', $params);
return $priceSet['id'];
}

}
82 changes: 82 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,86 @@ public function addPriceFields(&$params) {
);
}

/**
* Test form submission CRM-16228.
*/
public function testSubmitContributionPageWithPriceSetHavingTaxCRM16228() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above comments, it would be very helpful to rename this test to "testContributionPageWithPriceSetHavingTaxHasCorrectTaxAndTotalAmounts" .

$this->_priceSetParams['is_quick_config'] = 0;
$this->enableTaxAndInvoicing();
$financialType = $this->createFinancialType();
$financialAccount = $this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5);
$this->setUpContributionPage();
$submitParams = array(
'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']),
'id' => (int) $this->_ids['contribution_page'],
'first_name' => 'Billy',
'last_name' => 'Gruff',
'email' => 'billy@goat.gruff',
'is_pay_later' => TRUE,
);
$priceSetID = reset($this->_ids['price_set']);
$priceFieldId = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '16.85');
$submitParams['price_' . $priceFieldId] = 100;
$this->callAPISuccess('contribution_page', 'submit', $submitParams);
$contribution = $this->callAPISuccessGetSingle('contribution', array(
'contribution_page_id' => $this->_ids['contribution_page'],
'contribution_status_id' => 2,
'return' => array('total_amount', 'tax_amount'),
));
$this->assertEquals($contribution['tax_amount'], '84.25', 'Wrong Tax amount is calculated and stored.');
// 10 + (100 * 16.85) + (100 * 16.85 * 0.05)
$this->assertEquals($contribution['total_amount'], '1779.25', 'Total amount no matching.');
$lineItem = $this->callAPISuccess('LineItem', 'get', array(
'contribution_id' => $contribution['id'],
));
$lineTotal = array(1685, 10);
$lineTaxAmount = array('84.25', NULL);
foreach ($lineItem['values'] as $line) {
$this->assertEquals(CRM_Utils_Array::value('tax_amount', $line), array_pop($lineTaxAmount), 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($line['line_total'], array_pop($lineTotal), 'Total amount no matching.');
}
}

/**
* Test form submission CRM-16228.
*/
public function testSubmitContributionPageWithPriceSetHaving2PriceFieldTaxCRM16228() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above comments, it would be very helpful to rename this test to "testContributionPageWithPriceSetHaving2PriveFieldTaxHasCorrectTaxAndTotalAmounts" .

$this->_priceSetParams['is_quick_config'] = 0;
$this->enableTaxAndInvoicing();
$financialType = $this->createFinancialType();
$financialAccount = $this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5);
$this->setUpContributionPage();
$submitParams = array(
'price_' . $this->_ids['price_field'][0] => reset($this->_ids['price_field_value']),
'id' => (int) $this->_ids['contribution_page'],
'first_name' => 'Billy',
'last_name' => 'Gruff',
'email' => 'billy@goat.gruff',
'is_pay_later' => TRUE,
);
$priceSetID = reset($this->_ids['price_set']);
$priceFieldId = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '16.85');
$submitParams['price_' . $priceFieldId] = 100;
$priceFieldId = $this->addTaxTextPriceFields($priceSetID, $financialType['id'], '13.33');
$submitParams['price_' . $priceFieldId] = 10;
$this->callAPISuccess('contribution_page', 'submit', $submitParams);
$contribution = $this->callAPISuccessGetSingle('contribution', array(
'contribution_page_id' => $this->_ids['contribution_page'],
'contribution_status_id' => 2,
'return' => array('total_amount', 'tax_amount'),
));
$this->assertEquals($contribution['tax_amount'], '90.92', 'Wrong Tax amount is calculated and stored.');
// 10 + (100 * 16.85) + (100 * 16.85 * 0.05) + (10 * 13.33) + (10 * 13.33 * 0.05)
$this->assertEquals($contribution['total_amount'], '1919.22', 'Total amount no matching.');
$lineItem = $this->callAPISuccess('LineItem', 'get', array(
'contribution_id' => $contribution['id'],
));
$lineTotal = array('133.30', '1685', '10');
$lineTaxAmount = array('6.67', '84.25', NULL);
foreach ($lineItem['values'] as $line) {
$this->assertEquals(CRM_Utils_Array::value('tax_amount', $line), array_pop($lineTaxAmount), 'Wrong Tax amount is calculated and stored.');
$this->assertEquals($line['line_total'], array_pop($lineTotal), 'Total amount no matching.');
}
}

}