[WIP]CRM-19585, Sales tax fixes #9340

pradpnayak · 2016-10-31T12:21:31Z

CRM-19585: Sales tax issue

omarabuhussein · 2016-11-15T22:36:24Z

hi @pradpnayak ... I've been assigned to review this ticket so hopefully we can get it to be merged to the next release together .

So first thing caught my eye is the lack documentation .. it appear that you have fixed multiple issues with sales tax so can you please describe the issues you fixed in this PR in more details inside the PR description and copy it to the ticket on jira , otherwise no one will now what is going on and why you made these changes in case something break later .

another thing is the lack of unit tests ... I know it probably not required for civicrm-core PRs to have unit tests and actually some parts of civi are very hard or even impossible to test but I think your changes are testable so can you please add some tests .

I will leave the rest of my notes on the code itself during this week .

omarabuhussein · 2016-11-15T22:45:03Z

CRM/Contribute/BAO/Contribution.php

@@ -374,7 +374,8 @@ public static function calculateMissingAmountParams(&$params, $contributionID) {
        $params['fee_amount'] = 0;
      }
    }
-    if (!isset($params['net_amount'])) {
+    // recalculate net amount if tax amount is set


this IF condition will be true if ( tax amount is set OR net amount is not set ) .. but this inline comment only talk about tax amount ... so can you please amend this comment to say that . maybe something like :

// recalculate net amount if it is not set or if tax amount is set

omarabuhussein · 2016-11-15T22:50:19Z

CRM/Contribute/BAO/Contribution.php

@@ -3268,7 +3269,15 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
            self::updateFinancialAccounts($params, 'changeFinancialType');
            /* $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id']; */


since you are working on this file can you remove this commented out line

omarabuhussein · 2016-11-15T23:23:45Z

CRM/Contribute/BAO/Contribution.php

@@ -3268,7 +3269,15 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
            self::updateFinancialAccounts($params, 'changeFinancialType');
            /* $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id']; */
            $params['financial_account_id'] = $newFinancialAccount;
-            $params['total_amount'] = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $trxnParams['total_amount'];
+            $changeFTAmount = $trxnParams['total_amount'];


I think this newly added code can be moved to a new private method called calcluateFTChangeAmount() and rewritten like that , methods should not be big and we should stop adding more lines to them.

list($changeFTAmount, $ignoreChangeAmount) = $this->calcluateFTChangeAmount($params, $trxnParams['total_amount']); private function calcluateFTChangeAmount($params, $totalAmount) { $changeFTAmount = $totalAmount; if (isset($params['prevContribution']->total_amount) || isset($params['tax_amount'])) { $taxAmount = CRM_Utils_Array::value('tax_amount', $params, 0); $changesinTaxAmount = $totalAmount - $params['prevContribution']->total_amount + $params['prevContribution']->tax_amount - $taxAmount; if ($changesinTaxAmount == 0) { $ignoreChangeAmount = TRUE; $changeFTAmount = $totalAmount; } } return array($changeFTAmount, $ignoreChangeAmount); }

also you could probably rewrite it in better way but the key point is to separate the code into more short logical units to improve readability .. also please do not forget to add a doc block for this new private method in case you implemented it .

omarabuhussein · 2016-11-15T23:29:11Z

CRM/Contribute/BAO/Contribution.php

@@ -3447,6 +3456,12 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT
    }
    if ($context == 'changedAmount' || $context == 'changeFinancialType') {
      $itemAmount = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $params['total_amount'] - $params['prevContribution']->total_amount;
+      if (isset($params['prevContribution']->tax_amount)) {


I am not sure about the logic here since I don't have enough context .. but shouldn't this be :

if (isset($params['tax_amount'])) {

@pradpnayak @Edzelopez could you reply to each comment explaining how it is addressed or why you don't think it should be? It appears the same changes have been resubmitted as a new PR but the commented code may still appear, eg https://github.com/civicrm/civicrm-core/pull/9682/files#diff-7a5b0e2d131dabc49178460fc63328c0R3453

Hi @omarabuhussein

The logic here (and below) is intentional. This code here handles the case where a contribution with a taxable financial type is edited to change the financial type to another taxable/non-taxable one. In such a case, previously, the taxes were not subtracted/added to the total amount. The reason why the conditions look reversed is that the taxes usually have reversed mathematical signs. So for example, if the previous total amount was $110 with a contribution of $100 and $10 tax, on changing this to a non-taxable financial type, we would need to subtract the $10. For this reason, we have checked if there was a previous tax applied to a contribution and subtracted it from the new total amount. Similarly, for the next condition, if we have changed the financial type to a new taxable financial type, say amounting to 20% tax, we will need to first subtract the previous tax amount from the contribution to avoid calculating 20% on $110 rather than $100.

HTH,
Edsel

omarabuhussein · 2016-11-15T23:29:41Z

CRM/Contribute/BAO/Contribution.php

+      if (isset($params['prevContribution']->tax_amount)) {
+        $itemAmount -= CRM_Utils_Array::value('tax_amount', $params, 0);
+      }
+      if (isset($params['tax_amount'])) {


same here .. shouldn't this be

if (isset($params['prevContribution']->tax_amount))

so basically flip the if condition with the one above it ... or am I missing something ?

omarabuhussein · 2016-11-15T23:36:39Z

CRM/Contribute/BAO/Contribution.php

@@ -3549,6 +3564,7 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT
      }
    }
    $trxn = CRM_Core_BAO_FinancialTrxn::create($params['trxnParams']);
+    $previousLineItem = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['contribution']->id);


so the variable name is $previousLineItem but you are passing current contribution id .. should you pass $params['prevContribution']->id instead .. so

previousLineItem = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['prevContribution']->id);

in case is what you want is the current contribution line item then I think you should rename the variable name to reflect that .. or maybe I am missing something again ?

$params['contribution']->id and $params['prevContribution']->id hold the same id for contribution.

then I think it would be more clear to use $params['prevContribution']->id

omarabuhussein · 2016-11-15T23:45:56Z

CRM/Contribute/BAO/Contribution.php

            }
          }
          else {
-            $amount = $diff * $fieldValues['line_total'];
+            if ($context == 'changedAmount') {
+              $itemParams['amount'] = $diff * ($fieldValues['line_total'] - $previousLineItem[$fieldValues['id']]['line_total']);


so if this condition evaluated to true .. then the variable $amount will be undefined but it is still used when creating $itemParams array below .. I don't know what are you trying to do but this will cause issues so I think you need to rethink about what you are doing here

also I think this whole logic :

if (!empty($params['is_quick_config'])) { $amount = $itemAmount; if (!$amount) { $amount = $params['total_amount']; if ($context === NULL) { $amount -= CRM_Utils_Array::value('tax_amount', $params, 0); } } } else { if ($context == 'changedAmount') { $itemParams['amount'] = $diff * ($fieldValues['line_total'] - $previousLineItem[$fieldValues['id']]['line_total']); } else { $amount = $diff * $fieldValues['line_total']; } }

could be moved to a new private method that calculate the amount based on passed parameters .. but it depends on what this line $itemParams['amount'] = $diff * ($fieldValues['line'] ... etc is for .

As an aside - this is_quick_config should really not be this deep in the BAO method. Not sure if you can change it here but it's something to always be conscious of. Quick Config was introduced to allow a simplified display but has unfortunately been spaghettified into all sorts of places

omarabuhussein · 2016-11-15T23:54:14Z

CRM/Contribute/BAO/Contribution.php

@@ -3596,13 +3620,25 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT
          $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $amount;
          $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id;

-          if ($fieldValues['tax_amount']) {
+          $taxAmount = $fieldValues['tax_amount'];


what if $fieldValues['tax_amount'] is not exists .. is that possible ... you may need check it first?

omarabuhussein

for now I reviewed these two files :

CRM/Contribute/BAO/Contribution.php
CRM/Contribute/Form/Contribution.php

and left some notes .. I will revisit the rest of files whenever I have some time during this week and continue reviewing them .

omarabuhussein · 2016-11-20T01:32:02Z

CRM/Financial/BAO/FinancialItem.php

+          THEN 0
+          ELSE 1
+        END
+      GROUP BY cfi.id ORDER BY cfi.id DESC LIMIT 1';


since there is "LIMIT 1" and I don't see that you are using any aggreate function in the select colomuns then I see that there is no need for the "GROUP BY" part .

omarabuhussein · 2016-11-20T01:34:56Z

CRM/Financial/BAO/FinancialItem.php

+        INNER JOIN civicrm_financial_trxn cft ON cft.id = ceft.financial_trxn_id
+        INNER JOIN civicrm_financial_account cfa ON cfa.id = cfi.financial_account_id
+      WHERE cfi.entity_table = %2 AND cfi.entity_id = %1 AND 
+        CASE 


is this query complete ? , because I don't see any meaning for this case statement .. whenever any case get satisfied it will end up either as ( AND 1 ) or (AND 0) and either way it has no meaning since you are not comparing or using it in anything .

omarabuhussein · 2016-11-20T01:45:33Z

okay I am done reviewing , since this is still marked as [WIP] and there are many notes I left on it which are still unresolved , So this is not ready for the coming release . cc @eileenmcnaughton

eileenmcnaughton · 2016-11-20T20:04:38Z

Thanks for the review @omarabuhussein

I agree it is absolutely essential to add unit tests when making changes to financial or sales tax issues because they are easy to break & hard to QA and important. The sales tax code is woefully under-tested

JoeMurray · 2016-12-14T15:55:48Z

jenkins please test this

agileware · 2016-12-15T04:54:42Z

We've cloned this to test manually on our dev systems. Aiming to add some unit tests too.

eileenmcnaughton · 2016-12-22T08:26:29Z

CRM/Contribute/BAO/Contribution.php

@@ -374,7 +374,8 @@ public static function calculateMissingAmountParams(&$params, $contributionID) {
        $params['fee_amount'] = 0;
      }
    }
-    if (!isset($params['net_amount'])) {
+    // recalculate net amount if it is not set or if tax amount is set
+    if (!isset($params['net_amount']) || isset($params['tax_amount'])) {


Why do we torture our poor users by asking them to enter net_amount rather than always calculating it from fee amount, tax amount etc - has anyone else felt the love of altering total & then failing to save because net amount fails validation.

Erm that was a tangent

…rectly ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

…ribution amount is changed having tax ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

---------------------------------------- * CRM-19585: https://issues.civicrm.org/jira/browse/CRM-19585

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

…pplicable ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

omarabuhussein · 2016-12-28T14:38:57Z

hey guys .. please ping me in case all issues where sorted out so I can review again

pradpnayak · 2016-12-28T22:17:44Z

closing this in favour of
#9574
#9576
#9588
#9589
#9590

civicrm-builder added the master label Oct 31, 2016

pradpnayak force-pushed the CRM-19585 branch 2 times, most recently from 17443f8 to 857fd8a Compare November 2, 2016 09:21

omarabuhussein reviewed Nov 15, 2016

View reviewed changes

omarabuhussein suggested changes Nov 16, 2016

View reviewed changes

omarabuhussein reviewed Nov 20, 2016

View reviewed changes

pradpnayak force-pushed the CRM-19585 branch from 7796f46 to 5b6416d Compare November 27, 2016 11:05

pradpnayak force-pushed the CRM-19585 branch from 5b6416d to b5059db Compare December 14, 2016 13:35

pradpnayak force-pushed the CRM-19585 branch 3 times, most recently from e1e3c3a to ebf8a32 Compare December 20, 2016 12:49

eileenmcnaughton reviewed Dec 22, 2016

View reviewed changes

pradpnayak added 5 commits December 22, 2016 18:41

CRM-19585, fixed code for storing civicrm_contribution.tax_amount cor…

d91d351

…rectly ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed validation error for change in total amount having tax

417c1df

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed code to get previous financial item details

774b45c

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed code for entries in civicrm_financial_item when cont…

b6b18d9

…ribution amount is changed having tax ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed calculation for default contribution amount price set

a129b74

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

pradpnayak and others added 6 commits December 22, 2016 18:41

CRM-19585, fixed notice errors

42af5d1

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed code for sales tax for change in FT

fcb020c

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, code cleanup, moved set of code into function

f277760

---------------------------------------- * CRM-19585: https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, fixed jenkin error's

5e8a861

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585, added minor changes

943a67e

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585 Changed param for better readability

ed3cbe1

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

pradpnayak force-pushed the CRM-19585 branch from 45f1be3 to ed3cbe1 Compare December 22, 2016 13:12

Edzelopez added 2 commits December 23, 2016 18:16

CRM-19585 Added unit test for change of payment instrument when tax a…

1c314b8

…pplicable ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

CRM-19585 Modified unit test to check changes in financial item

5e965c4

---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585

pradpnayak mentioned this pull request Dec 26, 2016

[ready-for-core-team-review]CRM-19585, created function to calculate financial item amount when co… #9574

Merged

pradpnayak closed this Dec 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]CRM-19585, Sales tax fixes #9340

[WIP]CRM-19585, Sales tax fixes #9340

pradpnayak commented Oct 31, 2016 •

edited by civicrm-builder

Loading

omarabuhussein commented Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

JoeMurray Feb 2, 2017 •

edited

Loading

Edzelopez Feb 3, 2017

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016

pradpnayak Dec 15, 2016

omarabuhussein Dec 22, 2016

omarabuhussein Nov 15, 2016

omarabuhussein Nov 15, 2016 •

edited

Loading

eileenmcnaughton Nov 20, 2016

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein left a comment

omarabuhussein Nov 20, 2016

omarabuhussein Nov 20, 2016 •

edited

Loading

omarabuhussein commented Nov 20, 2016 •

edited

Loading

eileenmcnaughton commented Nov 20, 2016

JoeMurray commented Dec 14, 2016

agileware commented Dec 15, 2016

eileenmcnaughton Dec 22, 2016

omarabuhussein commented Dec 28, 2016

pradpnayak commented Dec 28, 2016

		@@ -3268,7 +3269,15 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
		self::updateFinancialAccounts($params, 'changeFinancialType');
		/* $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id']; */

[WIP]CRM-19585, Sales tax fixes #9340

[WIP]CRM-19585, Sales tax fixes #9340

Conversation

pradpnayak commented Oct 31, 2016 • edited by civicrm-builder Loading

omarabuhussein commented Nov 15, 2016 • edited Loading

omarabuhussein Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarabuhussein Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

omarabuhussein Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

JoeMurray Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarabuhussein Nov 15, 2016 • 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

Choose a reason for hiding this comment

omarabuhussein Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarabuhussein Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

omarabuhussein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarabuhussein Nov 20, 2016 • edited Loading

Choose a reason for hiding this comment

omarabuhussein commented Nov 20, 2016 • edited Loading

eileenmcnaughton commented Nov 20, 2016

JoeMurray commented Dec 14, 2016

agileware commented Dec 15, 2016

Choose a reason for hiding this comment

omarabuhussein commented Dec 28, 2016

pradpnayak commented Dec 28, 2016

pradpnayak commented Oct 31, 2016 •

edited by civicrm-builder

Loading

omarabuhussein commented Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

JoeMurray Feb 2, 2017 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 15, 2016 •

edited

Loading

omarabuhussein Nov 20, 2016 •

edited

Loading

omarabuhussein commented Nov 20, 2016 •

edited

Loading