CRM-19585 Sales tax issues

Pradeep Nayak Please provide description with description of each error addressed by PR.

I have fixed #1 to #4.
For #5 i waiting for Joe's reply to email

Email sent Dec 16.

Found additional issue related to tax. Steps to reproduce:

• Create a priceset with taxable financial type eg 10% tax.
• create a price fields with options say $100 and $200.
• Record an offline contribution and select $200. tax will be recorded as $20.
• Click Save. Check the total_amount - $220 (Correct).
• Click on edit link and save the contribution without modifying any fields.
• Check the total_amount - $242 (Incorrect). Seems like the tax_amount gets additionally added to the updated total_amount($220 + $22).
• Now, everytime you edit and save this contribution, the total_amount will be incremented. 242(220+22), 266(240+24), 292(266+26) and so on.

This is a regression as I see this to be working fine in 4.6. Not sure but this seems related https://github.com/civicrm/civicrm-core/pull/8835.

Pradeep, could you fix new issue reported by Jitendra. We saw this very early on iirc, and I thought we had long ago fixed it. Please check if the unmerged PR at https://github.com/civicrm/civicrm-core/pull/9588 is the fix for it and deal with failing test.

Edsel Lopez Pradeep Nayak Could you review the comments on 9340 and see which apply still to 9682 and respond, probably best in comments on 9682, replicating original comment and adding your response? Thx.

I think to get some progress here we need to treat this as an epic or close it & create specific issues.

I looked at the first PR against this issue - but the test failed very early on - with or without the fix

```
{{ /**

• Test for change in FT
  */
  public function testChangePaymentInstrumentTax() { $contactId = $this->individualCreate(); $this->enableTaxAndInvoicing(); $financialType = $this->createFinancialType(); $financialAccount = $this->relationForFinancialTypeWithFinancialAccount($financialType['id']); $params = array( 'contact_id' => $contactId, 'currency' => 'USD', 'financial_type_id' => 1, 'contribution_status_id' => 1, 'payment_instrument_id' => 1, 'total_amount' => 200.00, 'fee_amount' => 0, 'non_deductible_amount' => 0, ); $id = $this->contributionCreate($params); $financialTrxn = $this->callAPISuccess('financial_trxn', 'get', array( 'trxn_id' => 12345, )); $this->assertEquals($financialTrxn['count'], 1, 'Counts do not match.'); $this->assertEquals($financialTrxn['values'][1]['payment_instrument_id'], 1, 'Payment Instrument is not the same.'); $this->assertEquals($financialTrxn['values'][1]['total_amount'], 220.00, 'Amount does not match.'); }

  }}
  ```

So assuming that should work - perhaps the first issue is to get that test added & passing (it might be a test problem not a core problem) - I suspect it is but it feels like the first blocker in https://github.com/civicrm/civicrm-core/pull/9682.

The second blocker being lack of a separate & specific issue

There are still tax issues which have not been solved and grouping these together to raise visibility via an epic (or whatever) would greatly help.

I've converted issue to Epic type.

add test and get passing

passing tests:
https://github.com/civicrm/civicrm-core/pull/9589 - has been approved by a reviewer
https://github.com/civicrm/civicrm-core/pull/9682
https://github.com/civicrm/civicrm-core/pull/9683
https://github.com/civicrm/civicrm-core/pull/9684
https://github.com/civicrm/civicrm-core/pull/9685

Pradeep, could you create separate issues under this epic for each of the PRs, and provide a description of what each PR is doing in the related issue.

Linking another existing tax issue to this epic

Good find on that other issue - looks like it probably needs about 40 minutes more work to get it QA'd & merged & as we have learnt, often merging in this area fixes other issues

I've added a link in to this as it seems likely this fix will impact on & possibly fix other issues. Since it is in QA & passed code-quality review I would priotise it to the top

Just a FYI. Have created CRM-20423 which should be part of this EPIC.

I've closed this because I believe most if not all the issues against it are not closed & we should open new specific tickets where that is not the case