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

KarinG · 2017-01-23T05:26:51Z

Please see https://issues.civicrm.org/jira/browse/CRM-19908 for full description/rationale.

This PR addresses two fundamentally wrong operations:

do math - do rounding - do math again
calculate tax rate using tax amount that was previously calculated using tax rate

Purpose of these edits - three fold: to make the minimal edits required to:

to make the sales Tax Amounts accurate [by moving premature rounding from the basic Utils function to -after- quantity has been taken into account] - right now they are not - and that's highly critical to organizations that are collecting GST.
to remove/replace the backwards calculation of Tax Rate [do not calculate it based on the Tax Amount that used Tax Rate in the first place]
to round close to output/display - and show three decimals (if needed); one of our provinces in Canada e.g. 9.975% - previously everything was always rounded to two decimals

Note: I've not added a PHP Unit test for 1) to add a test would require edits to CiviCRM Core (contribution.php and confirm.php) itself [right now you can't test line_items w/ quantity within the TEST framework] - that in itself it not without risk. I strongly believe we need to get the fundamentals right first.

This is the BEFORE: note Tax Rate is 5.01% and Tax Amount on that lineItem is $15.30 - this is incorrect:

This is the AFTER: note Tax Rate is 5% and Tax Amount on that lineItem is $15.26 - this is correct:

KarinG · 2017-01-23T05:27:27Z

CRM/Contribute/BAO/Contribution/Utils.php

@@ -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);


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.

KarinG · 2017-01-23T05:27:51Z

CRM/Price/BAO/LineItem.php

-   * @return int|void
-   *   tax rate
-   */
-  public static function calculateTaxRate($lineItemId) {


This function needs to be removed - it is fundamentally wrong to calculate Tax Rate using Tax Amount - which was calculated using Tax Rate.

KarinG · 2017-01-23T05:28:03Z

CRM/Price/BAO/PriceSet.php

    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);


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

KarinG · 2017-01-23T05:28:17Z

templates/CRM/Price/Page/LineItem.tpl

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


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

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?

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

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

KarinG · 2017-01-23T14:42:21Z

I'm not seeing an undefined index in the Drupal dblog when I generate the PDF invoice - but will dig in and figure out why the test says there is. Ok found it - retesting now.

KarinG · 2017-01-23T15:45:49Z

Failure in api call for CustomValue create: DB Error: syntax error
That makes no sense - will ask Jenkins to test again

totten · 2017-01-23T19:55:55Z

Tangential to your problem but maybe relevant to the Jenkins comments: CRM_Contact_BAO_ContactType_ContactTest.testCRM19133 seems to be an ornery test. Any or all of these would be reasonable/proportionate:

Add the annotation @group ornery to testCRM19133 so that it's skipped during PR runs.
Update the notes about ornery tests. We do have CONTIRBUTING.md which links to https://forum.civicrm.org/index.php?topic=36964.0 , but this could be better.
Make a concerted effort to tackle testCRM19133. (Don't expect it to be trivial. Ornery tests are ornery for a reason.)

KarinG · 2017-01-23T21:18:32Z

jenkins test this please

seamuslee001 · 2017-01-24T02:13:08Z

Jenkins re test this please

colemanw · 2017-01-24T19:11:19Z

@civicrm-builder test this please

KarinG · 2017-01-24T20:03:35Z

Sorry but these failures have nothing to do w/ my PR:
Test Result (2 failures / +2)
CRM_Activity_ActionMappingTest::testDefault.testDefault with data set #3
CRM_Contact_BAO_ContactType_ContactTest.testCRM19133

KarinG · 2017-01-24T20:11:31Z

I've posted the how to set up a Sales Tax etc on: the 4.6 PR: #9710 (comment)

seamuslee001 · 2017-01-24T20:45:44Z

Jenkins re test this please

eileenmcnaughton · 2017-01-25T05:39:19Z

@agileware have undertaken to review this next week - in time for 4.6.17

colemanw · 2017-01-25T15:11:40Z

Yay! Thanks @agileware

KarinG · 2017-01-25T16:37:50Z

I've add a PHPUnit Test - and I've debug/tested the test - it will fail if pre-mature rounding is re-introduced

eileenmcnaughton · 2017-01-26T01:48:28Z

tests/phpunit/api/v3/ContributionPageTest.php

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


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

twomice · 2017-01-26T21:20:24Z

For reference, here's a link to my testing results on the 4.6-LTS version of this PR: #9710 (comment)

eileenmcnaughton · 2017-01-27T02:48:06Z

I just want to note than in resolving this we should figure out how it interacts with #9685 #9684 #9683 #9682 #9589 #8884 - which are the other open PRS (in addition to #9710 )

@seamuslee001 @pradpnayak can you determine whether you think this supercedes your PRs or otherwise how your PRs relate.

pradpnayak · 2017-01-30T08:06:30Z

CRM/Price/BAO/LineItem.php

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


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

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;

pradpnayak · 2017-01-30T08:10:43Z

CRM/Price/BAO/LineItem.php

+      $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);


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']];

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

pradpnayak · 2017-01-30T08:15:25Z

tests/phpunit/api/v3/ContributionPageTest.php

+      'label' => 'Printing Rights',
+    ));
+    $lineItemId = $lineItem['id'];
+    $lineItem_TaxAmount = round($lineItem['values'][$lineItemId]['tax_amount'], 2);


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

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

pradpnayak · 2017-01-30T08:20:47Z

tests/phpunit/api/v3/ContributionPageTest.php

+    $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);


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

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.

pradpnayak · 2017-01-30T08:23:24Z

QA'd PR, looks perfect. The calculation for tax_rate should not be rounded to 3 precision as civicrm_financial_account.tax_rate allows precision till 8.

I think we should also add some unit tests to check for tax_rate values.
I had added some test for this at #9643 , but it was failing because of change at https://github.com/civicrm/civicrm-core/pull/9643/files#diff-ecead08fd9d62dd134a679e60a2ba6b4R1954 . I will need to figure out the reason for failure.

KarinG · 2017-01-30T16:18:19Z

Again TaxRate is retrieved - so no need for a basic PHP Unit level test - but perhaps an invoice level test is feasible. Comparing the output on the PDF to the TaxRate that the admin had used to set up his/her Financial Accounts.

agileware · 2017-02-02T01:39:58Z

Just a FYI. We're testing this today manually, using about ~13 test cases

agileware · 2017-02-03T01:16:23Z

Review of CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.7. #9711

The following tests were performed on a CiviCRM 4.7.15 installation before and after CRM-19908 patch is applied.

Test Case: Donation, credit card payment

Set up:

Set up a Donation, Contribution page
Use Amounts, Fixed Contribution Options
Set Financial Account, tax applicable

Method 1:

Use Donation page to submit a test donation
Use credit card number 4444333322221111
Enter donation amount
Submit donation

Result 1:

Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result. Correct tax inclusive amount and tax displayed on the webpage, email receipt and CiviCRM Contribution.
After patch CRM-19908
- Correct result

Method 2:

Locate the Contribution
Click Edit for the Contribution
Do not change any values
Click Save for the Contribution

Result 2:

Verify that Contribution tax inclusive amount and tax does not change after save

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Donation, pending pay later

Set up:

Use previous Donation page
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Donation page to submit a test donation
Choose the "Pay later" option
Enter donation amount
Submit donation

Result 1:

Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result. Correct tax inclusive amount and tax displayed on the webpage, email receipt and CiviCRM Contribution. Before-and-after-submission tax inclusive amount and tax are the same on the contribution page
After patch CRM-19908
- Correct result

Test Case: Event registration, credit card payment

Set up:

Set up an Event
Set Fees, Regular Fees
Set Financial Account, tax applicable

Method 1:

Use Event page to submit a event registration
Use credit card number 4444333322221111
Submit registration

Result 1:

Event registration receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

Locate the Contribution
Click Edit for the Contribution
Do not change any values
Click Save for the Contribution

Result 2:

Verify that Contribution tax inclusive amount and tax does not change after save

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Membership, credit card payment

Set up:

Set up a Membership, Contribution page
Use Amounts, Fixed Contribution Options
Set Financial Account, tax applicable

Method 1

Use Membership page to submit a Membership registration
Use credit card number 4444333322221111
Submit Membership registration

Result 1:

Membership receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

Locate the Contribution
Click Edit for the Contribution
Do not change any values
Click Save for the Contribution

Result 2:

Verify that Contribution tax inclusive amount and tax does not change after save

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Event, pending pay later

Test based on bug report, #23973#note-12 and #23973#note-10

Set up:

Use previous Event page
Set Fees, Regular Fees
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Event page to submit a Event registration
Choose the "Pay later" option
Enter Event amount
Submit Event registration

Result 1:

Event receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result. Correct tax inclusive amount and tax displayed on Event receipt and CiviCRM Contribution.
After patch CRM-19908
- Correct result

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result. Correct tax inclusive amount and tax displayed on Event receipt and CiviCRM Contribution before and after status changed.
After patch CRM-19908
- Correct result

Test Case: Membership, pending pay later

Test based on bug report, #23973#note-12 and #23973#note-10

Set up:

Use previous Membership page
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Membership page to submit a Membership registration
Choose the "Pay later" option
Enter Membership amount
Submit Membership

Result 1:

Membership receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Recording a contribution using credit card

Method:

Locate a contact in CiviCRM
On the Contribution tab
Use the Credit Card Contribution button to create a new contribution
Verify that the financial account has tax associated
Use credit card number 4444333322221111
Submit contribution

Result:

View the print contribution receipt, verify correct tax inclusive amount and tax
Use the email contribution receipt, verify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Event registration credit card payment and price set

Set up:

Set up an Event
Set Amounts, Price Set
Set Financial Account, tax applicable

Method:

Use Event page to submit a event registration
Use credit card number 4444333322221111
Submit registration

Result:

Event registration receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result.
After patch CRM-19908
- Correct result.

Test Case: Membership, credit card payment and price set

Set up:

Set up a Membership, Contribution page
Set Amounts, Price Set
Set Financial Account, tax applicable

Method:

Use Membership page to submit a Membership registration
Use credit card number 4444333322221111
Submit Membership registration

Result:

Membership receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result.
After patch CRM-19908
- Correct result.

Test Case: Donation, credit card payment and price set

Set up:

Set up a Donation, Contribution page
Set Amounts, Price Set
Set Financial Account, tax applicable

Method:

Use Donation page to submit a test donation
Use credit card number 4444333322221111
Enter donation amount
Submit donation

Result:

Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Donation, pending pay later and price set

Set up:

Use previous Donation page
Set Amounts, Price Set
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Donation page to submit a test donation
Choose the "Pay later" option
Enter donation amount
Submit donation

Result 1:

Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Incorrect result. Tax rates are wrong. Each time you click edit and then save, the tax rates increase.
After patch CRM-19908
- Incorrect result. Tax rates are correct but each time you click edit and then save, the tax amount increments.

Test Case: Event, pending pay later and price set

Set up:

Use previous Event page
Set Amounts, Price Set
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Event page to submit a Event registration
Choose the "Pay later" option
Enter Event amount
Submit Event registration

Result 1:

Event receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result
After patch CRM-19908
- Correct result

Test Case: Membership, pending pay later and price set

Set up:

Use previous Membership page
Set Amounts, Price Set
Ensure the "Pay later" contribution option is enabled

Method 1:

Use Membership page to submit a Membership registration
Choose the "Pay later" option
Enter donation amount
Submit donation

Result 1:

Membership receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Correct result.
After patch CRM-19908
- Correct result.

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Incorrect result. Tax rates are wrong. Each time you click edit and then save, the tax rates increase.
After patch CRM-19908
- Incorrect result. Tax rates are correct but each time you click edit and then save, the tax amount increments.

Test Case: Back-end Membership, credit card payment

Set up:

Set up a Membership
Set up Membership, Price Set
Set Financial Account, tax applicable

Method:

Use the Memberships -> New Membership menu
Select a Contact
Assign a Membership
Pay with Credit Card
Submit Membership registration

Result:

Verify tax inclusive amount and tax is correct

Test Case Result:

Before patch CRM-19908
- Incorrect result. Tax rates are wrong. Each time you click edit and then save, the tax rates increase.
After patch CRM-19908
- Incorrect result. Tax rates are correct but each time you click edit and then save, the tax amount increments.

Test Case: Back-end Membership, pending pay later

Set up:

Set up a Membership
Set up Membership, Price Set
Set Financial Account, tax applicable

Method 1:

Use the Memberships -> New Membership menu
Select a Contact
Assign a Membership
Choose the "Pay later" option
Submit Membership registration

Result 1:

Verify tax inclusive amount and tax is correct
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Incorrect result. Tax rates are wrong. Each time you click edit and then save, the tax rates increase.
After patch CRM-19908
- Incorrect result. Tax rates are correct but each time you click edit and then save, the tax amount increments.

Method 2:

In CiviCRM, locate the contribution submitted
Note the tax inclusive amount and tax for the contribution
Change the Payment status from "Pending" to "Completed"
Click Submit to save changes
Note the tax inclusive amount and tax for the contribution after submission
Compare the before and after tax inclusive amount and tax on the contribution page

Result 2:

Compare the before and after tax inclusive amount and tax on the contribution page, they should be the same
Donation receipt should identify correct tax inclusive amount and tax
Contribution in CiviCRM should identify correct tax inclusive amount and tax

Test Case Result:

Before patch CRM-19908
- Incorrect result. Tax rates are wrong. Each time you click edit and then save, the tax rates increase.
After patch CRM-19908
- Incorrect result. Tax rates are correct but each time you click edit and then save, the tax amount increments.

Test Case: Add Membership from search result and override membership amount

Set up:

Set up a Membership
Set up Membership, Price Set
Set Financial Account, tax applicable

Method:

Search for a Contact
From the search results listing
Select the action, "Add Membership"
Change the membership fee (amount)
Submit Membership registration
View the Contribution details for the membership
Use the CiviCRM API to query the Contribution details for the membership

Result:

Verify tax inclusive amount and tax is based on the changed membership fee, not the original membership fee

Test Case Result:

Before patch CRM-19908
- The contribution details shown in both CiviCRM contact and CiviCRM API indicate that tax inclusive amount is based on the changed membership fee, but tax remains unchanged.
After patch CRM-19908
- The contribution details shown in both CiviCRM contact and CiviCRM API indicate that tax inclusive amount is based on the changed membership fee, but tax remains unchanged.

Test Case: Renew Membership from search result and override membership amount

Set up:

Set up a Membership
Set up Membership, Price Set
Set Financial Account, tax applicable

Method:

Search for a Contact
From the search results listing
Select the action, "Renew Membership"
Change the membership fee (amount)
Submit Membership registration
View the Contribution details for the membership
Use the CiviCRM API to query the Contribution details for the membership

Result:

Verify tax inclusive amount and tax is based on the changed membership fee, not the original membership fee

Test Case Result:

Before patch CRM-19908
- The contribution details shown in both CiviCRM contact and CiviCRM API indicate that tax inclusive amount is based on the changed membership fee, but tax remains unchanged.
After patch CRM-19908
- The contribution details shown in both CiviCRM contact and CiviCRM API indicate that tax inclusive amount is based on the changed membership fee, but tax remains unchanged.

eileenmcnaughton · 2017-02-03T01:18:40Z

Thanks for drawing up such a thorough test plan

KarinG · 2017-02-03T01:47:45Z

Wow! Merci! So would it be correct to say that you've found (thus far) is that this PR fixes many cases -but not everything- known to TaxMath and it doesn't appear to break anything (that was previously working), right?

eileenmcnaughton · 2017-02-03T01:49:16Z

The world as known by TaxMath....

eileenmcnaughton · 2017-02-03T01:50:59Z

"Tax rates are correct but each time you click edit and then save, the tax amount increments."

I think you have misinterpreted this as an incorrect result - but from personal experience I can tell you this is exactly how the tax system works.

agileware · 2017-02-03T01:51:51Z

@eileenmcnaughton thanks. It would be great to convert these to unit tests and automate. I think this would be very worthwhile, more auto-test coverage the better.

eileenmcnaughton · 2017-02-03T01:53:01Z

Yes - more tax coverage would be better! Most of these cases have some test coverage - but generally not on the tax calculation - so in some cases it is just adding checks to existing tests

agileware · 2017-02-03T01:53:26Z

@KarinG @eileenmcnaughton we are still finalising the test results and changing this report. So please treat it as draft at this stage. We had to transpose the wiki-text from Redmine (ours) to Github (here) and there were some errors. So still fixing that up.

eileenmcnaughton · 2017-02-03T01:53:27Z

(also note Karin did add some tests in this PR)

agileware · 2017-02-03T01:57:04Z

@KarinG

Wow! Merci! So would it be correct to say that you've found (thus far) is that this PR fixes many cases -but not everything- known to TaxMath and it doesn't appear to break anything (that was previously working), right?

Correct, we haven't come across any new problems at this stage. We're still testing.

agileware · 2017-02-03T05:43:25Z

@eileenmcnaughton we're done and the results of the testing is posted

I think you have misinterpreted this as an incorrect result - but from personal experience I can tell you this is exactly how the tax system works.

I assume this is a poke at tax in general, a joke right? Because clicking the "Save" button and seeing the taxed amount increase dramatically each time is in itself quite a funny experience (as a developer).

eileenmcnaughton · 2017-02-03T09:53:07Z

yes - it was intended as a joke.

So - merge this & create a JIRA ticket for fairer taxation?

KarinG · 2017-02-03T13:23:27Z

If you merge this I'll hunt down the unfair double taxation upon edit/save.

colemanw · 2017-02-03T17:24:21Z

Deal.

KarinG · 2017-02-03T17:28:42Z

Ok I better get on with it then!

KarinG · 2017-02-03T17:41:09Z

OK started: created https://issues.civicrm.org/jira/browse/CRM-19966 - @agileware - could you please go through the open JIRA TaxMath issues and close which ones you believe are now fixed? I'll dig in on the Tax Fairness issue.

agileware · 2017-02-03T19:53:34Z

@KarinG happy too, but our JIRA permissions are insufficient to do that. Would be great if we have permission to link JIRA issues to each other as well. It's a real bugger having to search each time for relateds.

Alternately, if an EPIC could be set up then all the tax related issues could be grouped that way.

KarinG · 2017-02-03T19:57:21Z

Ah ok - just give me or @eileenmcnaughton the JIRA issue numbers that need closing - and we can do that. Perhaps Eileen knows how you'd go about getting additional JIRA permissions.

eileenmcnaughton · 2017-02-04T22:33:23Z

This is really fantastic @KarinG & @agileware - we can go forward now with a lot more confidence in tax in Civi. I've added a couple of JIRA groups to your Agileware account - not sure if it is what required.

@KarinG looks like you have the same JIRA permissions as me - I added some groups at this URL https://issues.civicrm.org/jira/secure/admin/user/UserBrowser.jspa

CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.7

civicrm-builder added the master label Jan 23, 2017

KarinG commented Jan 23, 2017

View reviewed changes

Fundamental Fixes for TaxMath Calculations.

4f5911b

KarinG closed this Jan 24, 2017

KarinG reopened this Jan 24, 2017

Add PHPUnit Test - testSubmitContributionPageWithPriceSetQuantity.

57d395b

eileenmcnaughton reviewed Jan 26, 2017

View reviewed changes

pradpnayak reviewed Jan 30, 2017

View reviewed changes

colemanw merged commit d4a99d6 into civicrm:master Feb 3, 2017

agileware mentioned this pull request Feb 14, 2017

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

Merged

monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017

Merge pull request civicrm#9711 from KarinG/master

5519e0a

CRM-19908 - Fundamental Fixes for TaxMath Calculations 4.7

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

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

Conversation

KarinG commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarinG commented Jan 23, 2017 • edited Loading

KarinG commented Jan 23, 2017

totten commented Jan 23, 2017

KarinG commented Jan 23, 2017

seamuslee001 commented Jan 24, 2017

colemanw commented Jan 24, 2017

KarinG commented Jan 24, 2017

KarinG commented Jan 24, 2017

seamuslee001 commented Jan 24, 2017

eileenmcnaughton commented Jan 25, 2017

colemanw commented Jan 25, 2017

KarinG commented Jan 25, 2017

Choose a reason for hiding this comment

twomice commented Jan 26, 2017

eileenmcnaughton commented Jan 27, 2017

Choose a reason for hiding this comment

KarinG Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarinG Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

pradpnayak Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

KarinG Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

pradpnayak commented Jan 30, 2017 • edited Loading

KarinG commented Jan 30, 2017 • edited Loading

agileware commented Feb 2, 2017

agileware commented Feb 3, 2017 • edited Loading

Test Case: Donation, credit card payment

Test Case Result:

Test Case Result:

Test Case: Donation, pending pay later

Test Case Result:

Test Case Result:

Test Case: Event registration, credit card payment

Test Case Result:

Test Case Result:

Test Case: Membership, credit card payment

Test Case Result:

Test Case Result:

Test Case: Event, pending pay later

Test Case Result:

Test Case Result:

Test Case: Membership, pending pay later

Test Case Result:

Test Case Result:

Test Case: Recording a contribution using credit card

Test Case Result:

Test Case: Event registration credit card payment and price set

Test Case Result:

Test Case: Membership, credit card payment and price set

Test Case Result:

Test Case: Donation, credit card payment and price set

Test Case Result:

Test Case: Donation, pending pay later and price set

Test Case Result:

Test Case Result:

Test Case: Event, pending pay later and price set

Test Case Result:

Test Case Result:

Test Case: Membership, pending pay later and price set

Test Case Result:

Test Case Result:

Test Case: Back-end Membership, credit card payment

Test Case Result:

KarinG commented Jan 23, 2017 •

edited

Loading

JoeMurray Jan 23, 2017 •

edited

Loading

KarinG commented Jan 23, 2017 •

edited

Loading

KarinG Jan 30, 2017 •

edited

Loading

KarinG Jan 30, 2017 •

edited

Loading

pradpnayak Jan 30, 2017 •

edited

Loading

KarinG Jan 30, 2017 •

edited

Loading

pradpnayak commented Jan 30, 2017 •

edited

Loading

KarinG commented Jan 30, 2017 •

edited

Loading

agileware commented Feb 3, 2017 •

edited

Loading

agileware commented Feb 3, 2017 •

edited

Loading