From 83644f47cebaa63ef6717d15ed18e6df69e72e3b Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 15 Dec 2017 13:37:28 +1300 Subject: [PATCH] CRM-21562 Fix line item mis-saving when using thousand separator CRM-21562 Additional thousand separator fix on sales tax CRM-21562 additional fixes for money formatting, found by adding deprecated --- CRM/Contribute/BAO/Contribution.php | 15 ++++- CRM/Contribute/BAO/Contribution/Utils.php | 16 ++++- CRM/Contribute/Form/Contribution/Confirm.php | 18 +++--- CRM/Price/BAO/LineItem.php | 9 +-- CRM/Price/Page/Field.php | 2 +- CRM/Price/Page/Option.php | 2 +- CRM/Utils/Rule.php | 10 +++- api/v3/Contribution.php | 3 + .../CRM/Contribute/BAO/ContributionTest.php | 8 ++- .../CRM/Financial/BAO/FinancialItemTest.php | 8 ++- .../CRM/Member/Form/MembershipTest.php | 13 +++-- tests/phpunit/CiviTest/CiviUnitTestCase.php | 33 +++++++++++ tests/phpunit/api/v3/ContributionPageTest.php | 58 +++++++++++++------ tests/phpunit/api/v3/ContributionTest.php | 22 ++++++- .../api/v3/TaxContributionPageTest.php | 55 ++++++++++-------- 15 files changed, 199 insertions(+), 73 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index 79fdeb3ffb..6e485a5922 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -129,6 +129,10 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution { if (!empty($params['skipCleanMoney'])) { unset($moneyFields[0]); } + else { + // @todo put a deprecated here - this should be done in the form layer. + $params['skipCleanMoney'] = FALSE; + } foreach ($moneyFields as $field) { if (isset($params[$field])) { @@ -4205,6 +4209,11 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) public static function checkTaxAmount($params, $isLineItem = FALSE) { $taxRates = CRM_Core_PseudoConstant::getTaxRates(); + // This function should be only called after standardisation (removal of + // thousand separator & using a decimal point for cents separator. + // However, we don't know if that is always true :-( + // There is a deprecation notice tho :-) + $unknownIfMoneyIsClean = empty($params['skipCleanMoney']) && !$isLineItem; // Update contribution. if (!empty($params['id'])) { // CRM-19126 and CRM-19152 If neither total or financial_type_id are set on an update @@ -4251,7 +4260,7 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) empty($params['skipLineItem']) && !$isLineItem ) { $taxRateParams = $taxRates[$params['financial_type_id']]; - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount(CRM_Utils_Array::value('total_amount', $params), $taxRateParams); + $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount(CRM_Utils_Array::value('total_amount', $params), $taxRateParams, $unknownIfMoneyIsClean); $params['tax_amount'] = round($taxAmount['tax_amount'], 2); // Get Line Item on update of contribution @@ -4283,9 +4292,9 @@ WHERE eft.financial_trxn_id IN ({$trxnId}, {$baseTrxnId['financialTrxnId']}) } else { // update line item of contrbution - if (isset($params['financial_type_id']) && array_key_exists($params['financial_type_id'], $taxRates) && $isLineItem) { + if (isset($params['financial_type_id']) && array_key_exists($params['financial_type_id'], $taxRates) && $isLineItem) { $taxRate = $taxRates[$params['financial_type_id']]; - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($params['line_total'], $taxRate); + $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($params['line_total'], $taxRate, $unknownIfMoneyIsClean); $params['tax_amount'] = round($taxAmount['tax_amount'], 2); } } diff --git a/CRM/Contribute/BAO/Contribution/Utils.php b/CRM/Contribute/BAO/Contribution/Utils.php index 3930addfda..ce9caf1ab3 100644 --- a/CRM/Contribute/BAO/Contribution/Utils.php +++ b/CRM/Contribute/BAO/Contribution/Utils.php @@ -114,6 +114,9 @@ class CRM_Contribute_BAO_Contribution_Utils { $contributionParams['payment_instrument_id'] = $paymentParams['payment_instrument_id'] = $form->_paymentProcessor['payment_instrument_id']; } + // @todo this is the wrong place for this - it should be done as close to form submission + // as possible + $paymentParams['amount'] = CRM_Utils_Rule::cleanMoney($paymentParams['amount']); $contribution = CRM_Contribute_Form_Contribution_Confirm::processFormContribution( $form, $paymentParams, @@ -468,15 +471,24 @@ LIMIT 1 * Amount of field. * @param float $taxRate * Tax rate of selected financial account for field. + * @param bool $ugWeDoNotKnowIfItNeedsCleaning_Help + * This should ALWAYS BE FALSE and then be removed. A 'clean' money string uses a standardised format + * such as '1000.99' for one thousand $/Euro/CUR and ninety nine cents/units. + * However, we are in the habit of not necessarily doing that so need to grandfather in + * the new expectation. * * @return array * array of tax amount * */ - public static function calculateTaxAmount($amount, $taxRate) { + public static function calculateTaxAmount($amount, $taxRate, $ugWeDoNotKnowIfItNeedsCleaning_Help = FALSE) { $taxAmount = array(); + if ($ugWeDoNotKnowIfItNeedsCleaning_Help) { + Civi::log()->warning('Deprecated function, make sure money is in usable format before calling this.', array('civi.tag' => 'deprecated')); + $amount = CRM_Utils_Rule::cleanMoney($amount); + } // 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); + $taxAmount['tax_amount'] = ($taxRate / 100) * $amount; return $taxAmount; } diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php index abddc63b47..14355de49d 100644 --- a/CRM/Contribute/Form/Contribution/Confirm.php +++ b/CRM/Contribute/Form/Contribution/Confirm.php @@ -158,7 +158,6 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr * * @param array $params * @param int $financialTypeID - * @param float $nonDeductibleAmount * @param bool $pending * @param array $paymentProcessorOutcome * @param string $receiptDate @@ -167,13 +166,11 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr * @return array */ public static function getContributionParams( - $params, $financialTypeID, $nonDeductibleAmount, $pending, + $params, $financialTypeID, $pending, $paymentProcessorOutcome, $receiptDate, $recurringContributionID) { $contributionParams = array( 'financial_type_id' => $financialTypeID, 'receive_date' => (CRM_Utils_Array::value('receive_date', $params)) ? CRM_Utils_Date::processDate($params['receive_date']) : date('YmdHis'), - 'non_deductible_amount' => $nonDeductibleAmount, - 'total_amount' => $params['amount'], 'tax_amount' => CRM_Utils_Array::value('tax_amount', $params), 'amount_level' => CRM_Utils_Array::value('amount_level', $params), 'invoice_id' => $params['invoiceID'], @@ -202,10 +199,6 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr ); } - // CRM-4038: for non-en_US locales, CRM_Contribute_BAO_Contribution::add() expects localised amounts - $contributionParams['non_deductible_amount'] = trim(CRM_Utils_Money::format($contributionParams['non_deductible_amount'], ' ')); - $contributionParams['total_amount'] = trim(CRM_Utils_Money::format($contributionParams['total_amount'], ' ')); - if ($recurringContributionID) { $contributionParams['contribution_recur_id'] = $recurringContributionID; } @@ -950,7 +943,6 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr $params['is_recur'] = $isRecur; $params['payment_instrument_id'] = CRM_Utils_Array::value('payment_instrument_id', $contributionParams); $recurringContributionID = self::processRecurringContribution($form, $params, $contactID, $financialType); - $nonDeductibleAmount = self::getNonDeductibleAmount($params, $financialType, $online, $form); $now = date('YmdHis'); $receiptDate = CRM_Utils_Array::value('receipt_date', $params); @@ -960,10 +952,15 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr if (isset($params['amount'])) { $contributionParams = array_merge(self::getContributionParams( - $params, $financialType->id, $nonDeductibleAmount, TRUE, + $params, $financialType->id, TRUE, $result, $receiptDate, $recurringContributionID), $contributionParams ); + $contributionParams['non_deductible_amount'] = self::getNonDeductibleAmount($params, $financialType, $online, $form); + $contributionParams['skipCleanMoney'] = TRUE; + // @todo this is the wrong place for this - it should be done as close to form submission + // as possible + $contributionParams['total_amount'] = $params['amount']; $contribution = CRM_Contribute_BAO_Contribution::add($contributionParams); $invoiceSettings = Civi::settings()->get('contribution_invoice_settings'); @@ -1988,6 +1985,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr } $priceFields = $priceFields[$priceSetID]['fields']; + $lineItems = array(); CRM_Price_BAO_PriceSet::processAmount($priceFields, $paramsProcessedForForm, $lineItems, 'civicrm_contribution'); $form->_lineItem = array($priceSetID => $lineItems); $membershipPriceFieldIDs = array(); diff --git a/CRM/Price/BAO/LineItem.php b/CRM/Price/BAO/LineItem.php index 505642d602..1a90a9d3e8 100644 --- a/CRM/Price/BAO/LineItem.php +++ b/CRM/Price/BAO/LineItem.php @@ -341,6 +341,11 @@ WHERE li.contribution_id = %1"; * this is * lineItem array) * @param string $amount_override + * Amount override must be in format 1000.00 - ie no thousand separator & if + * a decimal point is used it should be a decimal + * + * @todo - this parameter is only used for partial payments. It's unclear why a partial + * payment would change the line item price. */ public static function format($fid, $params, $fields, &$values, $amount_override = NULL) { if (empty($params["price_{$fid}"])) { @@ -364,10 +369,6 @@ WHERE li.contribution_id = %1"; foreach ($params["price_{$fid}"] as $oid => $qty) { $price = $amount_override === NULL ? $options[$oid]['amount'] : $amount_override; - // lets clean the price in case it is not yet cleant - // CRM-10974 - $price = CRM_Utils_Rule::cleanMoney($price); - $participantsPerField = CRM_Utils_Array::value('count', $options[$oid], 0); $values[$oid] = array( diff --git a/CRM/Price/Page/Field.php b/CRM/Price/Page/Field.php index 536195af8f..68ebdc7e4a 100644 --- a/CRM/Price/Page/Field.php +++ b/CRM/Price/Page/Field.php @@ -149,7 +149,7 @@ class CRM_Price_Page_Field extends CRM_Core_Page { $getTaxDetails = TRUE; } if (isset($priceField[$priceFieldBAO->id]['tax_rate'])) { - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceField[$priceFieldBAO->id]['price'], $priceField[$priceFieldBAO->id]['tax_rate']); + $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($priceField[$priceFieldBAO->id]['price'], $priceField[$priceFieldBAO->id]['tax_rate'], TRUE); $priceField[$priceFieldBAO->id]['tax_amount'] = $taxAmount['tax_amount']; } } diff --git a/CRM/Price/Page/Option.php b/CRM/Price/Page/Option.php index 0724ebec9b..eab7c0c560 100644 --- a/CRM/Price/Page/Option.php +++ b/CRM/Price/Page/Option.php @@ -158,7 +158,7 @@ class CRM_Price_Page_Option extends CRM_Core_Page { if ($invoicing && isset($customOption[$id]['tax_rate'])) { $getTaxDetails = TRUE; } - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($customOption[$id]['amount'], $customOption[$id]['tax_rate']); + $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($customOption[$id]['amount'], $customOption[$id]['tax_rate'], TRUE); $customOption[$id]['tax_amount'] = $taxAmount['tax_amount']; } if (!empty($values['financial_type_id'])) { diff --git a/CRM/Utils/Rule.php b/CRM/Utils/Rule.php index c9a92fa967..0168e63c78 100644 --- a/CRM/Utils/Rule.php +++ b/CRM/Utils/Rule.php @@ -502,9 +502,15 @@ class CRM_Utils_Rule { } /** - * @param $value + * Strip thousand separator from a money string. + * + * Note that this should be done at the form layer. Once we are processing + * money at the BAO or processor layer we should be working with something that + * is already in a normalised format. + * + * @param string $value * - * @return mixed + * @return string */ public static function cleanMoney($value) { // first remove all white space diff --git a/api/v3/Contribution.php b/api/v3/Contribution.php index 6ed8f2a93e..027725ff58 100644 --- a/api/v3/Contribution.php +++ b/api/v3/Contribution.php @@ -45,6 +45,9 @@ function civicrm_api3_contribution_create(&$params) { $values = array(); _civicrm_api3_custom_format_params($params, $values, 'Contribution'); $params = array_merge($params, $values); + // The BAO should not clean money - it should be done in the form layer & api wrapper + // (although arguably the api should expect pre-cleaned it seems to do some cleaning.) + $params['skipCleanMoney'] = TRUE; if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) { if (empty($params['id'])) { diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php index 586b1bc2d9..6f4352d733 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -1084,8 +1084,14 @@ WHERE eft.entity_id = %1 AND ft.to_financial_account_id <> %2"; /** * Test for function createProportionalEntry(). + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testcreateProportionalEntry() { + public function testCreateProportionalEntry($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); list($contribution, $financialAccount) = $this->createContributionWithTax(); $params = array( 'total_amount' => 55, diff --git a/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php b/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php index 1d65940dc1..a9b56cf00f 100644 --- a/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php +++ b/tests/phpunit/CRM/Financial/BAO/FinancialItemTest.php @@ -311,8 +311,14 @@ class CRM_Financial_BAO_FinancialItemTest extends CiviUnitTestCase { /** * Check method getPreviousFinancialItem() with tax entry. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testGetPreviousFinancialItemHavingTax() { + public function testGetPreviousFinancialItemHavingTax($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $contactId = $this->individualCreate(); $this->enableTaxAndInvoicing(); $this->relationForFinancialTypeWithFinancialAccount(1); diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php index cb461cd3c6..5eb76b859f 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php @@ -610,8 +610,14 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { /** * Test the submit function of the membership form for partial payment. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testSubmitPartialPayment() { + public function testSubmitPartialPayment($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); // Step 1: submit a partial payment for a membership via backoffice $form = $this->getForm(); $form->preProcess(); @@ -629,7 +635,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { // This format reflects the 23 being the organisation & the 25 being the type. 'membership_type_id' => array(23, $this->membershipTypeAnnualFixedID), 'record_contribution' => 1, - 'total_amount' => $partiallyPaidAmount, + 'total_amount' => $this->formatMoneyInput($partiallyPaidAmount), 'receive_date' => date('m/d/Y', time()), 'receive_date_time' => '08:36PM', 'payment_instrument_id' => array_search('Check', $this->paymentInstruments), @@ -655,7 +661,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { $submitParams = array( 'contribution_id' => $contribution['contribution_id'], 'contact_id' => $this->_individualId, - 'total_amount' => $partiallyPaidAmount, + 'total_amount' => $this->formatMoneyInput($partiallyPaidAmount), 'currency' => 'USD', 'financial_type_id' => 2, 'receive_date' => '04/21/2015', @@ -676,7 +682,6 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { 'contact_id' => $this->_individualId, )); $this->assertEquals('Completed', $contribution['contribution_status']); - // $this->assertEquals(50.00, $contribution['total_amount']); // $this->assertEquals(50.00, $contribution['net_amount']); } diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index 324bb4ac2e..d996b0319d 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -2577,6 +2577,8 @@ AND ( TABLE_NAME LIKE 'civicrm_value_%' ) $var = TRUE; CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE); $this->disableTaxAndInvoicing(); + $this->setCurrencySeparators(','); + CRM_Core_PseudoConstant::flush('taxRates'); System::singleton()->flushProcessors(); } @@ -3929,4 +3931,35 @@ AND ( TABLE_NAME LIKE 'civicrm_value_%' ) return $form; } + /** + * Get possible thousand separators. + * + * @return array + */ + public function getThousandSeparators() { + return array(array('.'), array(',')); + } + + /** + * Set the separators for thousands and decimal points. + * + * @param string $thousandSeparator + */ + protected function setCurrencySeparators($thousandSeparator) { + Civi::settings()->set('monetaryThousandSeparator', $thousandSeparator); + Civi::settings() + ->set('monetaryDecimalPoint', ($thousandSeparator === ',' ? '.' : ',')); + } + + /** + * Format money as it would be input. + * + * @param string $amount + * + * @return string + */ + protected function formatMoneyInput($amount) { + return CRM_Utils_Money::format($amount, NULL, '%a'); + } + } diff --git a/tests/phpunit/api/v3/ContributionPageTest.php b/tests/phpunit/api/v3/ContributionPageTest.php index ea7e3428e0..ecaca1a1dd 100644 --- a/tests/phpunit/api/v3/ContributionPageTest.php +++ b/tests/phpunit/api/v3/ContributionPageTest.php @@ -698,8 +698,14 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { * * Ensure a separate payment for the membership vs the contribution, with * correct amounts. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testSubmitMembershipBlockIsSeparatePaymentPaymentProcessorNowChargesCorrectAmounts() { + public function testSubmitMembershipBlockIsSeparatePaymentPaymentProcessorNowChargesCorrectAmounts($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $this->setUpMembershipContributionPage(TRUE); $processor = Civi\Payment\System::singleton()->getById($this->_paymentProcessor['id']); $processor->setDoDirectPaymentResult(array('fee_amount' => .72)); @@ -763,7 +769,6 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { $this->assertCount(2, $distinct_contribution_ids, "Expected exactly 2 log contributions with distinct contributionIDs."); $this->assertTrue($found_contribution_amount, "Expected one log contribution with amount '$contributionPageAmount' (the contribution page amount)"); $this->assertTrue($found_membership_amount, "Expected one log contribution with amount '$this->_membershipBlockAmount' (the membership amount)"); - } /** @@ -1588,8 +1593,14 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { /** * Test form submission with multiple option price set. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testSubmitContributionPageWithPriceSet() { + public function testSubmitContributionPageWithPriceSet($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $this->_priceSetParams['is_quick_config'] = 0; $this->setUpContributionPage(); $submitParams = array( @@ -1608,13 +1619,16 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { 'contribution_page_id' => $this->_ids['contribution_page'], 'contribution_status_id' => 2, )); - $this->callAPISuccessGetCount( - 'LineItem', - array( - 'contribution_id' => $contribution['id'], - ), - 3 - ); + $this->assertEquals(80, $contribution['total_amount']); + $lineItems = $this->callAPISuccess('LineItem', 'get', array( + 'contribution_id' => $contribution['id'], + )); + $this->assertEquals(3, $lineItems['count']); + $totalLineAmount = 0; + foreach ($lineItems['values'] as $lineItem) { + $totalLineAmount = $totalLineAmount + $lineItem['line_total']; + } + $this->assertEquals(80, $totalLineAmount); } /** @@ -1650,14 +1664,20 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { /** * Test Tax Amount is calculated properly when using PriceSet with Field Type = Text/Numeric Quantity + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testSubmitContributionPageWithPriceSetQuantity() { + public function testSubmitContributionPageWithPriceSetQuantity($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $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); + $this->relationForFinancialTypeWithFinancialAccount($financialType['id'], 5); $this->setUpContributionPage(); $submitParams = array( @@ -1689,9 +1709,9 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { $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; + $submitParams['amount'] = $this->formatMoneyInput(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; + $submitParams['tax_amount'] = $this->formatMoneyInput(180 * 16.95 * 0.10); $this->callAPISuccess('contribution_page', 'submit', $submitParams); $contribution = $this->callAPISuccessGetSingle('contribution', array( @@ -1699,15 +1719,15 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { )); // 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( + $lineItem = $this->callAPISuccessGetSingle('LineItem', array( 'contribution_id' => $contribution['id'], 'label' => 'Printing Rights', )); - $lineItemId = $lineItem['id']; - $lineItem_TaxAmount = round($lineItem['values'][$lineItemId]['tax_amount'], 2); - // Compare this to what it should be! - $this->assertEquals($lineItem_TaxAmount, round($submitParams['tax_amount'], 2), 'Wrong Sales Tax Amount is calculated and stored.'); + $lineItem_TaxAmount = round($lineItem['tax_amount'], 2); + + $this->assertEquals($lineItem['line_total'], $contribution['total_amount'], 'Contribution total should match line total'); + $this->assertEquals($lineItem_TaxAmount, round(180 * 16.95 * 0.10, 2), 'Wrong Sales Tax Amount is calculated and stored.'); } public function hook_civicrm_alterPaymentProcessorParams($paymentObj, &$rawParams, &$cookedParams) { diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php index 1c5dd8b2d7..eba61b1828 100644 --- a/tests/phpunit/api/v3/ContributionTest.php +++ b/tests/phpunit/api/v3/ContributionTest.php @@ -122,6 +122,18 @@ class api_v3_ContributionTest extends CiviUnitTestCase { public function tearDown() { $this->quickCleanUpFinancialEntities(); $this->quickCleanup(array('civicrm_uf_match')); + $financialAccounts = $this->callAPISuccess('FinancialAccount', 'get', array()); + foreach ($financialAccounts['values'] as $financialAccount) { + if ($financialAccount['name'] == 'Test Tax financial account ' || $financialAccount['name'] == 'Test taxable financial Type') { + $entityFinancialTypes = $this->callAPISuccess('EntityFinancialAccount', 'get', array( + 'financial_account_id' => $financialAccount['id'], + )); + foreach ($entityFinancialTypes['values'] as $entityFinancialType) { + $this->callAPISuccess('EntityFinancialAccount', 'delete', array('id' => $entityFinancialType['id'])); + } + $this->callAPISuccess('FinancialAccount', 'delete', array('id' => $financialAccount['id'])); + } + } } /** @@ -1929,9 +1941,15 @@ class api_v3_ContributionTest extends CiviUnitTestCase { } /** - * CRM-19126 Add test to verify when complete transaction is called tax amount is not changed + * CRM-19126 Add test to verify when complete transaction is called tax amount is not changed. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testCheckTaxAmount() { + public function testCheckTaxAmount($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $contact = $this->createLoggedInUser(); $financialType = $this->callAPISuccess('financial_type', 'create', array( 'name' => 'Test taxable financial Type', diff --git a/tests/phpunit/api/v3/TaxContributionPageTest.php b/tests/phpunit/api/v3/TaxContributionPageTest.php index 91b784f527..83b1fe2055 100644 --- a/tests/phpunit/api/v3/TaxContributionPageTest.php +++ b/tests/phpunit/api/v3/TaxContributionPageTest.php @@ -167,23 +167,7 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { } public function tearDown() { - $this->quickCleanup(array( - 'civicrm_contribution', - 'civicrm_contribution_soft', - 'civicrm_event', - 'civicrm_contribution_page', - 'civicrm_participant', - 'civicrm_participant_payment', - 'civicrm_line_item', - 'civicrm_financial_trxn', - 'civicrm_financial_item', - 'civicrm_entity_financial_trxn', - 'civicrm_contact', - 'civicrm_membership', - 'civicrm_membership_payment', - 'civicrm_payment_processor', - )); - CRM_Core_PseudoConstant::flush('taxRates'); + $this->quickCleanUpFinancialEntities(); } public function setUpContributionPage() { @@ -224,14 +208,20 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { } /** - * Online and offline contrbution from above created contrbution page. + * Online and offline contrbution from above created contribution page. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators */ - public function testCreateContributionOnline() { + public function testCreateContributionOnline($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $this->setUpContributionPage(); $params = array( 'contact_id' => $this->_individualId, 'receive_date' => '20120511', - 'total_amount' => 100.00, + 'total_amount' => $this->formatMoneyInput(100.00), 'financial_type_id' => $this->financialtypeID, 'contribution_page_id' => $this->_ids['contribution_page'], 'payment_processor' => $this->_ids['paymentProcessID'], @@ -254,7 +244,16 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { $this->_checkFinancialRecords($contribution, 'online'); } - public function testCreateContributionChainedLineItems() { + /** + * Create contribution with chained line items. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators + */ + public function testCreateContributionChainedLineItems($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $this->setUpContributionPage(); $params = array( 'contact_id' => $this->_individualId, @@ -321,12 +320,21 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { $this->_checkFinancialRecords($contribution, 'payLater'); } - public function testCreateContributionPendingOnline() { + /** + * Test online pending contributions. + * + * @param string $thousandSeparator + * punctuation used to refer to thousands. + * + * @dataProvider getThousandSeparators + */ + public function testCreateContributionPendingOnline($thousandSeparator) { + $this->setCurrencySeparators($thousandSeparator); $this->setUpContributionPage(); $params = array( 'contact_id' => $this->_individualId, 'receive_date' => '20120511', - 'total_amount' => 100.00, + 'total_amount' => $this->formatMoneyInput(100.00), 'financial_type_id' => $this->financialtypeID, 'contribution_page_id' => $this->_ids['contribution_page'], 'trxn_id' => 12345, @@ -345,6 +353,7 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { $this->assertEquals($contribution['values'][$contribution['id']]['tax_amount'], 20); $this->assertEquals($contribution['values'][$contribution['id']]['contribution_status_id'], 2); $this->_checkFinancialRecords($contribution, 'pending'); + $this->setCurrencySeparators($thousandSeparator); } /** -- 2.25.1