CIVICRM-868: Hiding tax field on edit line item form if no tax collected.

[agilewarealok]

This PR is for Issue #14

Issue

0.00 tax added for financial types with no tax account

Steps to reproduce:

1. Turn on Tax & Invoicing then set up a financial type with no tax account - call this e.g. "No Tax calculation"
2. Create a contribution with a single line item using "No Tax calculation" as the financial type.
3. Check the line items using SQL - tax_amount is NULL - correct
4. Edit the Contribution and change a line item
5. Note that tax_amount is 0.00 on this form.
6. Check the line items using SQL - tax_amount is 0.00 - this causes problems with the invoice template ("% GST" with no number) and also account syncing with Xero, which will receive the full amount as tax exclusive (due to 0.00 as a string not being "empty") and add tax on top for accounts tax instead of calculating the percentage of the given amount which is already tax.

Solution:

Instead of setting default value of tax_amount to 0.00 we set it to NULL, and adding only non-null value fields on edit line item form.

[monishdeb]

@agilewarealok please indicate when this PR is ready for review, as it seems you have pushed a commit now.

I think you have caught that regression and trying to fix it in last commit? The regression was when I change the Financial type to one which got tax, the tax amount was still hidden. I did a simple fix in that direction without your patch, which is:

     $this->_lineitemInfo = civicrm_api3('lineItem', 'getsingle', array('id' => $this->_id));
-    $this->_lineitemInfo['tax_amount'] = CRM_Utils_Array::value('tax_amount', $this->_lineitemInfo, 0.00);
+    $this->_lineitemInfo['tax_amount'] = CRM_Utils_Array::value('tax_amount', $this->_lineitemInfo);

This will expose the tax_amount field but with NULL value set if the FT has no tax rate or will show calculated tax amount if the FT got tax rate:

Point is always show tax_amount field if Sale tax is enabled, irrespective of what FT used, tax amount will be null or not null anyway

[agilewarealok]

@monishdeb
Yes the PR is ready for review after my last commit.

We faced similar issue after fixing it first time, Tax field was hidden for FT with tax rates. Latest fix is as follow,

1. Hide tax field on front-end for FT which does not have tax rate.
2. Show/Hide tax field when we change the FT on edit price set page
3. Set tax_amount as NULL on updating the price set if selected FT does not have tax rate. This prevents adding 0.00 in database.

[jusfreeman]

@monishdeb ping

[monishdeb]

Sorry for delay. @agilewarealok can you please rebase this PR as there are improvements made in tpls. Also this is the final patch I would like to see:

diff --git a/CRM/Lineitemedit/Form/Edit.php b/CRM/Lineitemedit/Form/Edit.php
index 1bff80a..6882bac 100644
--- a/CRM/Lineitemedit/Form/Edit.php
+++ b/CRM/Lineitemedit/Form/Edit.php
@@ -28,6 +28,16 @@ class CRM_Lineitemedit_Form_Edit extends CRM_Core_Form {
     $this->assignFormVariables();
   }
 
+  /**
+   * Check if there is tax value for selected financial type.
+   * @param $financialTypeId
+   * @return bool
+   */
+  private function isTaxEnabledInFinancialType($financialTypeId) {
+    $taxRates = CRM_Core_PseudoConstant::getTaxRates();
+    return (isset($taxRates[$financialTypeId])) ? TRUE : FALSE;
+  }
+
   public function assignFormVariables($params = []) {
     $this->_lineitemInfo = civicrm_api3('lineItem', 'getsingle', array('id' => $this->_id));
     $this->_lineitemInfo['tax_amount'] = CRM_Utils_Array::value('tax_amount', $this->_lineitemInfo, 0.00);
@@ -143,6 +153,10 @@ class CRM_Lineitemedit_Form_Edit extends CRM_Core_Form {
   public function submit($values, $isTest = FALSE) {
     $values['line_total'] = CRM_Utils_Rule::cleanMoney($values['line_total']);
 
+    if (!$this->isTaxEnabledInFinancialType($values['financial_type_id'])) {
+     $values['tax_amount'] = '';
+    }
+
     $balanceAmount = ($values['line_total'] - $this->_lineitemInfo['line_total']);
     $contactId = CRM_Core_DAO::getFieldValue('CRM_Contribute_BAO_Contribution',
       $this->_lineitemInfo['contribution_id'],

Here we should leave the tpl as it is now and instead pass a empty string on submission which will record NULL tax_amount for the line-item record. This will retain the expected behaviour of always showing the tax_amount field.

In my opinion I don't think there is a issue in showing 0 tax_amount in front-end where on submission we are not recording 0 tax amount against that line-item.

[JoeMurray]

If the financial type does not have tax defined it would be ideal from non-profit org perspective to not show $0 tax on at least frontend forms. The reason is that donations aren't taxable and always displaying a $0 tax is off-putting, making it seem like a sale.

[monishdeb]

Thanks @JoeMurray for your feedback.

@agilewarealok thanks for your effort and sorry for putting this PR on hold for a long time as I was not getting enough time to review this PR, also it was a bit dependent on #18 and #20 which I need to merge first. And after that I have rebased and updated this PR.

Merging now

[agilewarealok]

@monishdeb Thanks for merging this PR :)