CRM-16228, fixed tax calculation as per https://github.com/civicrm/ci… #9643
Conversation
|
Will add test soon |
|
The fix should not cause regression for https://issues.civicrm.org/jira/browse/CRM-17848 |
---------------------------------------- * CRM-16228: Tax Math is not correct due to premature rounding https://issues.civicrm.org/jira/browse/CRM-16228
---------------------------------------- * CRM-16228: Tax Math is not correct due to premature rounding https://issues.civicrm.org/jira/browse/CRM-16228
---------------------------------------- * CRM-16228: Tax Math is not correct due to premature rounding https://issues.civicrm.org/jira/browse/CRM-16228
| @@ -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) { | |||
There was a problem hiding this comment.
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)?
| /** | ||
| * Test form submission CRM-16228. | ||
| */ | ||
| public function testSubmitContributionPageWithPriceSetHavingTaxCRM16228() { |
There was a problem hiding this comment.
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.
| /** | ||
| * Test form submission CRM-16228. | ||
| */ | ||
| public function testSubmitContributionPageWithPriceSetHaving2PriceFieldTaxCRM16228() { |
There was a problem hiding this comment.
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.
| /** | ||
| * Test form submission CRM-16228. | ||
| */ | ||
| public function testSubmitContributionPageWithPriceSetHavingTaxCRM16228() { |
There was a problem hiding this comment.
Similar to above comments, it would be very helpful to rename this test to "testContributionPageWithPriceSetHavingTaxHasCorrectTaxAndTotalAmounts" .
| /** | ||
| * Test form submission CRM-16228. | ||
| */ | ||
| public function testSubmitContributionPageWithPriceSetHaving2PriceFieldTaxCRM16228() { |
There was a problem hiding this comment.
Similar to above comments, it would be very helpful to rename this test to "testContributionPageWithPriceSetHaving2PriveFieldTaxHasCorrectTaxAndTotalAmounts" .
|
Please note that there are (Test Result) (10 failures / +9) |
Reference: civicrm#9643
…vicrm-core/pull/9626#issuecomment-270380614
https://issues.civicrm.org/jira/browse/CRM-16228