CRM-19966 Unfair Double Taxation and net_amounts. #9948

KarinG · 2017-03-08T04:48:18Z

if the total_amount is not in the currently submittedValues -> then it needs to be retrieved from $this->_values - however that total_amount is 'after' sales tax was previously added to it. Hence the subtraction. If we don't subtract it here - you end up being charged sales_tax on top of sales_tax - hence the screenshots in the JIRA issue
net_amount is defined as
"Net value of the contribution (Total Amount minus Fee)."
fee_amount is defined as:
"Processing fee for this transaction (if applicable)."
Neither are related to sales taxes

CRM-19966: Remove unfair double taxation

eileenmcnaughton · 2017-03-08T05:50:24Z

this is the error from jenkins

PHP Fatal error: Call to undefined method CRM_Contribute_BAO_Contribution::calculateNetAmount() in /home/jenkins/buildkit/build/core-9948-6uo0t/sites/all/modules/civicrm/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php on line 1036
RUN:[[./scripts/phpunit --tap --log-junit '/home/jenkins/workspace/CiviCRM-Core-PR/junit/api_v3_AllTests.xml' '--exclude-group' 'ornery' 'api_v3_AllTests']]
TAP version 13

eileenmcnaughton · 2017-03-08T05:53:52Z

@pradpnayak @JoeMurray this fixes a reproducable bug but you should review for other impacts if you have capacity because the code was introduced in response to https://issues.civicrm.org/jira/browse/CRM-19585 - I'm not sure which of the described issues it relates to

JoeMurray · 2017-03-08T11:53:01Z

CRM/Contribute/BAO/Contribution.php

@@ -5371,22 +5371,6 @@ public static function calculateFinancialItemAmount($params, $amountParams, $con
  }

  /**
-   * Calculate net amount.


I agree calculation in this function is incorrect.

Thank you for confirming - I'll strip it out of the phpunit test as well.

OK great, I feel that agreement was a key part of resolving this

JoeMurray · 2017-03-08T11:56:56Z

CRM/Contribute/Form/Contribution.php

@@ -1589,7 +1585,7 @@ protected function submit($submittedValues, $action, $pledgePaymentID) {
    }

    if (!isset($submittedValues['total_amount'])) {
-      $submittedValues['total_amount'] = CRM_Utils_Array::value('total_amount', $this->_values);
+      $submittedValues['total_amount'] = CRM_Utils_Array::value('total_amount', $this->_values) - CRM_Utils_Array::value('tax_amount', $this->_values);


Just raising question: how many significant digits do we have here? Are we changing total amount from two decimals to 3 or more by doing math with tax amount?

I'm not sure but we're in the form/Contribution.php so not worried about too many decimals here. I'll make sure I test with some numbers that did previously introduce rounding issues before I fixed that issue earlier this year.

JoeMurray · 2017-03-08T13:19:49Z

CRM/Contribute/Form/Contribution.php

@@ -377,10 +377,6 @@ public function setDefaultValues() {
      $defaults['fee_amount'] = CRM_Utils_Money::format($defaults['fee_amount'], NULL, '%a');
    }

-    if (isset($defaults['net_amount'])) {


We should be setting this to correct value rather than deliberately introducing this bug to solve a higher priority one. @pradpnayak please provide suggestion here for correct calculation.

Sorry not sure what you mean by 'introducing this bug'? Removing the incorrect calculation here actually fixes net_amount - as it's calculated later on.

JoeMurray · 2017-03-08T13:30:07Z

Pradeep will a) provide a draft for correct calculation of net_amount. He'll also look at changing the form validation that complains about netamount and total amount being different when the total amount displayed on form is back-calculated when taxes have been previously added (yuck- this is not reliable for some amounts and taxes).

KarinG · 2017-03-08T13:41:52Z

We don't need a) as net_amount is already calculated properly;

Please put the form validation bits on separate issue/PR; it's not relevant to CRM 19966

JoeMurray · 2017-03-08T13:59:28Z

Pradeep has found that this fix does not work if a call is made through the API.

KarinG · 2017-03-08T14:37:37Z

@JoeMurray - can you be more specific so I can try and reproduce that? Or is it any call - even simple Create Contribution? And what is not working (net_amount; tax_amount; total_amount)?

KarinG · 2017-03-08T19:24:29Z

I just confirmed with my 4.6 client [major user of sales taxes] that this issue does NOT affect 4.6 - I'll still commit to adding a PHP Unit test to this PR in 4.7;

eileenmcnaughton · 2017-03-08T19:29:53Z

@pradpnayak can you write a unit test for the bit that doesn't work? @KarinG don't let the questions above hold you back from writing a unit test for the part you are trying to fix. Even if the solution needs tweaking the unit test will still ensure the fix is locking in.

(of course not assuming you were going to work on this more today, but I'd hate to see this fantastic momentum lost)

KarinG · 2017-03-08T19:33:03Z

There is no unit test for what doesn't work [and I don't know yet what doesn't work - or if it has ever worked]... let's make that a separate CRM ticket? I strongly feel manage-able bite size - fundamentally correct fixes - are the way to work through an Epic.

agileware · 2017-03-08T20:16:11Z

I just confirmed with my 4.6 client [major user of sales taxes] that this issue does NOT affect 4.6 - I'll still commit to adding a PHP Unit test to this PR in 4.7;

Hey @KarinG, I cannot confirm the same for 4.6. Just pointing this out because I don't want anyone to see your comment and simply assume that all is working in 4.6 too. Because without separate testing and confirmation, it should still be regarded as an issue.

KarinG · 2017-03-08T20:21:44Z

Ok that's odd - what I'm fixing here went into 4.7 only - so it made sense 4.6 wasn't affected - and I just confirmed it with the ED of my project that uses this heavily - we can edit Contributions that are linked the Memberships that have been generated w/ pricesets and neither the tax_amounts not the net_amounts go bananas; and everything on the PDF invoice adds up to.

PS - I'm not saying everything re sales tax is working in 4.6 - but this specific issue appears to be;

agileware · 2017-03-08T20:25:53Z

@KarinG Yes, I get that. But since this is a PR for 4.7. Can we just keep it within that scope. Solving these problems is confusing enough as it is!

KarinG · 2017-03-08T20:28:21Z

Oh I'm all for keeping within scope 😀

agileware · 2017-03-08T20:38:57Z

we can edit Contributions that are linked the Memberships and that have been generated w/ pricesets without and neither the tax_amounts not the net_amounts go bananas

@KarinG and yet, 2 days ago I had to deal with a customer support request where membership with a priceset which was the ~~edited by the user~~ automatically changed status from Pending to Completed when the payment was received, re-calculated a $100 membership fee as:

$100 tax inc.
$10 tax amount for a 10% tax rate.
Net Amount $100
For a revised total of $110

That's just wrong. And here's a few screenshots of this in 4.6.

KarinG · 2017-03-08T20:45:37Z

Hey I thought we were staying within scope :-)!

What you're describing here is a different issue/mechanism. You're changing the status of the Contribution -> that triggers a bunch of other pathways into Financial Accounts... Copy/paste that into a separate JIRA ticket - could be one we can look at next.

agileware · 2017-03-08T20:51:28Z

Agreed, 4.7. Different issue, happy for you to make that call.

For that screenshot. I've amended the description. This problem occurred when CiviCRM automatically changed status from Pending to Completed when the payment was received as part of reconciling with Xero.

agileware · 2017-03-08T20:53:54Z

@KarinG Actually, the problem detailed in the screenshots is https://issues.civicrm.org/jira/browse/CRM-19966 Remove unfair double taxation which is what this PR is for. So I'm not sure why we need to create a new issue for this.

KarinG · 2017-03-08T20:58:22Z

No it's not the same issue -> Your three screenshots in https://issues.civicrm.org/jira/browse/CRM-19966 - all have status = completed & show the Tax creeping up (from $10 -> $11 -> $12.10) -> that's what I was able to reproduce and that's what I fixed here.

agileware · 2017-03-08T21:08:41Z

This is very frustrating.

eileenmcnaughton · 2017-03-08T21:09:14Z

just a general principle, more JIRA tickets is better than less I think. Ie if we can fix something clear cut on a specific version then we should close a JIRA against it.

KarinG · 2017-03-09T01:55:22Z

@Eileen - ran some more tests re: API - if I run a test (doing the exact same update I've been doing via the GUI):

    $result = civicrm_api3('Contribution', 'create', array(
    'sequential' => 1,
    'id' => 101,
    'check_number' => 12345,
  ));

The amounts fixed in this issue (total_amount; tax_amount and net_amount) continue to look good:

{
  "is_error": 0,
  "version": 3,
  "count": 1,
  "id": 101,
  "values": [
      {
          "id": "101",
          "contact_id": "203",
          "financial_type_id": "2",
          "contribution_page_id": "",
          "payment_instrument_id": "4",
          "receive_date": "2017-03-08 04:49:00",
          "non_deductible_amount": "0.00",
          "total_amount": "105.00",
          "fee_amount": "0.00",
          "net_amount": "105.00",
          "trxn_id": "",
          "invoice_id": "",
          "currency": "USD",
          "cancel_date": "",
          "cancel_reason": "",
          "receipt_date": "",
          "thankyou_date": "",
          "source": "My Source field",
          "amount_level": "",
          "contribution_recur_id": "",
          "is_test": "0",
          "is_pay_later": "0",
          "contribution_status_id": "1",
          "address_id": "",
          "check_number": "12345",
          "campaign_id": "",
          "creditnote_id": "",
          "tax_amount": "5.00",
          "revenue_recognition_date": "",
          "contribution_type_id": "2"
      }

KarinG · 2017-03-09T04:24:35Z

tests/phpunit/CRM/Contribute/Form/ContributionTest.php

@@ -851,4 +851,58 @@ public function testSubmitWithOutSaleTax() {
    $this->assertTrue(empty($lineItem['tax_amount']));
  }

+  public function testReSubmitSaleTax() {


This is not working yet!

…ce that function is edited

Resolve immediate issues with test & add checks for mail content (sin…

KarinG · 2017-03-09T16:46:20Z

Thank you for these core fixes - I'll continue to work on the PHP Unit test's assertions tonight.

… - using default price_set.

KarinG · 2017-03-10T03:49:23Z

Ok I fixed one PHP Unit test and another one to simulate Editing a Contribution in the BackOffice [and then checking to ensure net_amount; tax_amount; total_amount are all fine. There is a third test that can and probably should be written. Just not right now...

eileenmcnaughton · 2017-03-10T03:57:09Z

I've been working on this with @KarinG & I agree that

this fixes the issue, which is reproducable
the PR includes code improvement
the PR increases test coverage (including our first test on the test of an invoice I believe!)

merge on pass

eileenmcnaughton · 2017-03-10T05:16:43Z

test this please

totten · 2017-03-10T06:12:14Z

This was flagged merge on pass, and it's passed. Merging.

JoeMurray · 2017-03-10T16:12:34Z

Actually, Pradeep was doing some extensive testing in last 24h and didn't notice @totten's merge. There were several errors still present with the code in this PR, marked by ** in this gist: https://gist.github.com/JoeMurray/be20be2119212eaa991af11c8ba9b951 We could either revert this merge or document them as issues under sales tax epic. I propose the latter.

KarinG · 2017-03-10T18:06:53Z

@JoeMurray - can you ask @pradpnayak to please elaborate on his #8: **8. Updated contribution #7 using contribution form with Sales Tax enabled. @eileenmcnaughton : Pradeep thinks (and I think he is correct) that this is cause by "Change to code at line 1804." That's the refactoring of the lineItem bit - that you did in order to make testing of Submit with ::UPDATE possible. Happy to look at that with you - once we know what scenario contribution #7 exactly is.

KarinG · 2017-03-10T18:12:51Z

I strongly favor we keep this merge as it was never meant to fix all tax issues. It fixes a fundamental one where net_amount had been redefined and where sales_tax were being charge on sales taxes on Edit Contribution. So this PR will eliminate for sure more other Issues - but I do want to make sure that we didn't cause a regression by making it possible to Unit test for this.

eileenmcnaughton · 2017-03-10T20:00:45Z

@JoeMurray I agree with documenting them as issues under the sales tax epic. @pradpnayak's testing looks really good & converting that output into JIRA issues sounds like the next step. One really good thing about this PR is it actually adds unit tests covering sales tax output in invoices, which was previously uncovered by tests, so it's a really good basis for going forwards.

I strongly believe our focus now should be on triage, rather than the code, as it's really hard to review fixes without that step. @agileware has also volunteered some time on this.

We should aim to have an issue open for every remaining symptom (unless they are clearly really just one bug) and to get all the ones that we can't clearly reproduce closed, excluding the epic, which should be for tracking the other issues only

eileenmcnaughton · 2017-03-10T21:53:43Z

I should also note the merged code was possibly different to what @pradpnayak tested as there were some changes to the PR during the QA & test-writing process, so we should double check the bugs against the now-master

JoeMurray · 2017-03-13T13:54:53Z

I agree with the comments over the last few days. I had intended to create separate issues in JIRA for what Pradeep turned up. He'll be doing that today now that we have spoken. We're also going to push the remaining work in CRM-19585 into separate issues under the epic, and create additional issues for each outstanding PR on that issue describing what each does. I think Pradeep is submitting additional tests for the errors documented in his tests.

eileenmcnaughton · 2017-03-13T20:44:34Z

Thanks @JoeMurray

Fixes for CRM-19966 Unfair Double Taxation and net_amounts.

eileenmcnaughton · 2018-07-20T04:28:04Z

Just a few follow on links

The form validation issue was discussed here but it was left out of scope due to the feedback in this discussion. It seemed to be a non issue in 2017 & 2018 but has now become an issue for one site. There are 3 related gitlab issues

https://lab.civicrm.org/dev/core/issues/267
https://lab.civicrm.org/dev/core/issues/265
https://lab.civicrm.org/dev/core/issues/260

Net amount is calculated in the BAO if not set so asking users to enter it does not gain us anything. Currently we require them to calculate it & edit it if they change the fee_amount or total_amount and we validate their data entry, and make them re-do it if they get it wrong. Since the field is pretty hidden it's unintuitive to need to change it and makes for a painful contribution update process. The current behaviour dates back to svn days - however since 2015 the BAO has handled the possibility of it not being set. In addition we have had code issues with the comparison around currency & float comparison issues eg. civicrm#11485 and https://lab.civicrm.org/dev/core/issues/260 ( in the latter case the data saves correctly without net_amount and incorrectly if it is changed to meet the form validation rule. (There was a proposal in Mar 2017 to address that civicrm#9948 (comment) by fixing the calculation but I believe just dropping the field is better). Our unit tests test the form submissions but for some reason net_amoutn was removed from the tests https://github.com/civicrm/civicrm-core/pull/9948/files#diff-40e2e0f106ba620465acf3a9a81f2498L1535 meaning our test coverage is more reliable without it being set.

Fixes for CRM-19966 and net_amounts.

7993b40

civicrm-builder added the master label Mar 8, 2017

JoeMurray reviewed Mar 8, 2017

View reviewed changes

Remove php unit test for function that no longer exists.

dde6827

KarinG added 2 commits March 8, 2017 20:10

PHP Unit test fix: net_amount must equal total_amount minus fee_amount.

0920173

First pass at PHP Unit test for reSubmit (without priceset).

335c41e

KarinG commented Mar 9, 2017

View reviewed changes

Resolve immediate issues with test & add checks for mail content (sin…

f1662cb

…ce that function is edited

Merge pull request #3 from eileenmcnaughton/karing

5c138fc

Resolve immediate issues with test & add checks for mail content (sin…

Added Test to Create Contribution -> then Edit it via BackOffice form…

9308f1c

… - using default price_set.

eileenmcnaughton added the merge on pass label Mar 10, 2017

totten merged commit 360509b into civicrm:master Mar 10, 2017

JoeMurray changed the title ~~Fixes for CRM-19966 Unfair Double Taxation and net_amounts.~~ Fixes for https://issues.civicrm.org/jira/browse/CRM-19966 Unfair Double Taxation and net_amounts. Mar 13, 2017

JoeMurray changed the title ~~Fixes for https://issues.civicrm.org/jira/browse/CRM-19966 Unfair Double Taxation and net_amounts.~~ CRM-19966 Unfair Double Taxation and net_amounts. Mar 13, 2017

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

Merge pull request civicrm#9948 from KarinG/CRM-19966

e15e409

Fixes for CRM-19966 Unfair Double Taxation and net_amounts.

eileenmcnaughton mentioned this pull request Aug 14, 2018

Remove net_amount from the Back office contribution form. #12662

Merged

CRM-19966 Unfair Double Taxation and net_amounts. #9948

CRM-19966 Unfair Double Taxation and net_amounts. #9948

Conversation

KarinG commented Mar 8, 2017 • edited by civicrm-builder Loading

eileenmcnaughton commented Mar 8, 2017

eileenmcnaughton commented Mar 8, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeMurray commented Mar 8, 2017

KarinG commented Mar 8, 2017

JoeMurray commented Mar 8, 2017

KarinG commented Mar 8, 2017 • edited Loading

KarinG commented Mar 8, 2017

eileenmcnaughton commented Mar 8, 2017

KarinG commented Mar 8, 2017 • edited Loading

agileware commented Mar 8, 2017

KarinG commented Mar 8, 2017 • edited Loading

agileware commented Mar 8, 2017

KarinG commented Mar 8, 2017

agileware commented Mar 8, 2017 • edited Loading

KarinG commented Mar 8, 2017

agileware commented Mar 8, 2017

agileware commented Mar 8, 2017

KarinG commented Mar 8, 2017 • edited Loading

agileware commented Mar 8, 2017

eileenmcnaughton commented Mar 8, 2017

KarinG commented Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

KarinG commented Mar 9, 2017

KarinG commented Mar 10, 2017

eileenmcnaughton commented Mar 10, 2017

eileenmcnaughton commented Mar 10, 2017

totten commented Mar 10, 2017

JoeMurray commented Mar 10, 2017 • edited Loading

KarinG commented Mar 10, 2017 • edited Loading

KarinG commented Mar 10, 2017

eileenmcnaughton commented Mar 10, 2017

eileenmcnaughton commented Mar 10, 2017

JoeMurray commented Mar 13, 2017

eileenmcnaughton commented Mar 13, 2017

eileenmcnaughton commented Jul 20, 2018 • edited Loading

KarinG commented Mar 8, 2017 •

edited by civicrm-builder

Loading

KarinG commented Mar 8, 2017 •

edited

Loading

KarinG commented Mar 8, 2017 •

edited

Loading

KarinG commented Mar 8, 2017 •

edited

Loading

agileware commented Mar 8, 2017 •

edited

Loading

KarinG commented Mar 8, 2017 •

edited

Loading

KarinG commented Mar 9, 2017 •

edited

Loading

JoeMurray commented Mar 10, 2017 •

edited

Loading

KarinG commented Mar 10, 2017 •

edited

Loading

eileenmcnaughton commented Jul 20, 2018 •

edited

Loading