[CRM-19538] Tax on Memberships is extremely wrong Created: 19/Oct/16  Updated: 25/Apr/17  Resolved: 25/Apr/17

Status: Closed
Project: CiviCRM
Component/s: CiviContribute, CiviMember
Affects Version/s: 4.6.22, 4.7.12
Fix Version/s: 4.7.19

Type: Bug Priority: Critical
Reporter: Agileware Assignee: Pradeep Nayak
Resolution: Fixed/Completed Votes: 1
Labels: None
Remaining Estimate: 0 minutes
Time Spent: 1 hour, 15 minutes
Original Estimate: Not Specified

Attachments: PNG File edit_pending_membership_contribution.png     PNG File Step-1-Contribution-OK-with-Tax-and-Invoicing-Disabled.png     PNG File Step-2-Enable-Tax-and-Invoicing.png     PNG File Step-3-Contribution-NOT-OK-with-Tax-and-Invoicing-Enabled.png     PNG File Step-4-Contribution-Edit-NOT-OK-with-Tax-and-Invoicing-Enabled.png     PNG File Step-5-Contribution-Status-Change-Pending-Completed-NOT-OK-with-Tax-and-Invoicing-Enabled.png    
Issue Links:
Supplementation
supplements CRM-20423 Tax applied repeatedly when editing c... Open - Unverified
supplements CRM-16228 Tax Math is not correct due to premat... Closed
supplements CRM-17417 Tax is reapplied when editing a contr... Closed
supplements CRM-17418 Cancelling a taxable contribution wri... Closed
is supplemented by CRM-19585 Sales tax issues In Progress
is supplemented by CRM-19966 Tax applied repeatedly when 'empty ed... Closed
Versioning Impact: Patch (backwards-compatible bug fixes)
Documentation Required?: None
Funding Source: Needs Funding

 Description   

On the heels of CRM-17417 we've had a couple of clients complaining about tax issues relating to CiviCRM memberships recently.

We've tested issues on 4.7.12 as follows - all tests are using a single tax rate of 10%:

  • Using a contribution page to create an "online" membership seems to work okay - a new membership is created with the correct amount and subsequent offline renewals also work.
  • Adding a membership using the "Add Membership" button and a price set results in an initial contribution that has the tax rate applied twice (e.g. $50 excluding tax becomes $60.50, not $55 as it should); subsequent renewals of this membership have the correct amount. (Appears fixed in master as of Feb 26)
  • Doing the same without a price set appears to work.
  • Adding the membership with a contribution in Pending state results in a Contribution that when edited exhibits multiple incorrect behaviours - on the contribution edit screen the subtotals have double the tax added (as in 20% instead of 10%), however the contribution total still shows the correct amount - attached edit_pending_membership_contribution.png demonstrates. Saving the contribution results in the total having the tax cumulatively re-applied to the post-tax amount (total is 121% of the tax exclusive amount).
  • (Added 2016-12-13): When adding or renewing a membership through the admin interface ("Add Membership" button or "Renew" on a search result), if the amount is changed, the tax amount is applied from the original amount display instead of being calculated from the new amount - although this is not evident in the interface; it will show a contribution with the correct amount, but if you look at the line items via the API you'll see incorrect tax amounts.

Agileware reference 23973



 Comments   
Comment by Seamus Lee [ 19/Oct/16 ]

Just going to tag Joe Murray into this as Joe and his team have been doing work on similar stuff here. Marc Brazeau Also Marc here as Marc has recently been looking into the Add membership and price set stuff.

Comment by Joe Murray [ 19/Oct/16 ]

Jamie Novick does Compucorp want to take a stab at this?

Comment by Yashodha Chaku [ 18/Nov/16 ]

Rough estimate would be 3 - 5 hours.

Comment by Agileware [ 01/Dec/16 ]

Agileware tracking ref 23973

Comment by Yashodha Chaku [ 01/Dec/16 ]

Agileware Do you want core team to start work on this?

Comment by Agileware [ 13/Dec/16 ]

Added the remaining test case, FYI

Comment by Yashodha Chaku [ 05/Jan/17 ]

Agileware Do you want core team to start work on this issue?

Comment by Joe Murray [ 06/Jan/17 ]

This seems to be a reversion of https://issues.civicrm.org/jira/browse/CRM-16228

Comment by Joe Murray [ 06/Jan/17 ]

JMA is working on this in consultation with KarinG under CRM-16228.

Comment by Pradeep Nayak [ 24/Feb/17 ]

Agileware , I was unable to replicate Adding a membership using the "Add Membership" button and a price set results in an initial contribution that has the tax rate applied twice (e.g. $50 excluding tax becomes $60.50, not $55 as it should); subsequent renewals of this membership have the correct amount. on http://dmaster.demo.civicrm.org/

Steps tried to replicate:
Create Financial Account "Sales Tax" with tax rate as 10% and assigned it to Member Dues Financial Type
Create price set with a radio html type and used Student Membership type
On Add Membership form i selected Student Radio Button from the price set option. The amount was calculated correctly as $55.

Am i missing anything?

Comment by Agileware [ 24/Feb/17 ]

Pradeep Nayak you missed a crucial step in your testing, you did not enable: CiviContribute Component Settings: Tax and Invoicing

With the CiviContribute Component Settings: Tax and Invoicing enabled you can see the various problems which are still present in CiviCRM 4.7.17.

Attached are screenshots which highlight the issue and here's some notes for each step:

  1. Step-1-Contribution-OK-with-Tax-and-Invoicing-Disabled - Appears to be working correctly, as you also noted
  2. Step-2-Enable-Tax-and-Invoicing - So let's enable CiviContribute Component Settings: Tax and Invoicing and see what happens next
  3. Step-3-Contribution-NOT-OK-with-Tax-and-Invoicing-Enabled - Wow, the same contribution from the previous screenshot is now different. Why would that be? That's really odd.
    1. Total amount is now $60. Should be $55.
  4. Step-4-Contribution-Edit-NOT-OK-with-Tax-and-Invoicing-Enabled - Editing the same contribution shows the same results. Why?
    1. Total amount is now $60. Should be $55.
  5. Step-5-Contribution-Status-Change-Pending-Completed-NOT-OK-with-Tax-and-Invoicing-Enabled - Payment has been received for this contribution, so let's edit it and change the contribution status from Pending to Completed and save. Looking at the contribution listing, we see a new total and different amounts again. Why?
    1. Total amount is now $60.50. Should be $55.

Let us know if you still cannot reproduce this problem, happy to help out.

Comment by Joe Murray [ 26/Feb/17 ]

Pradeep, could you look at this Tuesday? Thx.

Comment by Agileware [ 26/Feb/17 ]

Re-tested again this morning, I can confirm what Pradeep is seeing - on dmaster, the initial contribution when added from the back-end with a price-set has the correct amount.

Editing the contribution still produces double taxation - this has a separate ticket CRM-19966

Comment by Pradeep Nayak [ 02/Mar/17 ]

Hi Agileware

I had tested above all issues with CiviContribute Component Settings: Tax and Invoicing - Enabled.

I could replicate the error for amount change when a contribution is edited. I think i have covered this fix in my other issue CRM-19585. I will check and post the fix here ASAP.

Comment by Agileware [ 02/Mar/17 ]

Linking related issues.

Comment by Agileware [ 02/Mar/17 ]

> I had tested above all issues with CiviContribute Component Settings: Tax and Invoicing - Enabled.

Good to hear, wasn't detailed in your steps. When I checked dmaster shortly after your posting it Tax and Invoicing was not enabled. I could see the other configuration you had applied.

It is still interesting to note tax is still calculated and included in the total even with the Tax and Invoicing option disabled. As per my previous note and step 1 screenshot. This could be yet another bug.

Without Tax and Invoicing enabled, I would have expected no tax calculations at all to be performed. But hey, these days I expect the unexpected.

Comment by Pradeep Nayak [ 06/Mar/17 ]

Yeah, that true. The code doesn't check for setting value i.e if CiviContribute Component Settings: Tax and Invoicing - Enabled. It applies tax if it has Account relationship 'Sales Tax Account is' defined for Financial type.

Comment by Joe Murray [ 08/Mar/17 ]

Eileen McNaughton KarinG We are proposing to fix this with changes to CRM_Contribute_BAO_Contribution::checkTaxAmount() and a unit test, but no major refactoring. Are you in agreement with that approach?

Comment by Joe Murray [ 08/Mar/17 ]

With respect to Pradeep's comment of 05/Mar/17 the correct approach it seems to me would be to allow edits of any transactions that have sales taxes even when the Component Setting has been turned off for taxes. And when creating new contributions, event regs, memberships when it is disabled taxes should be ignored even if financial accounts for taxes have been set up for relevant financial types. Let's make a separate issue for that.

Comment by KarinG [ 08/Mar/17 ]

Joe Murray - "We are proposing to fix this" -> what is "this" exactly? We must be chipping away at sales tax issues one at a time - le'ts get in #9948 - and then see how that affects the other issues. Else we're going to run into conflicts / waste time.

Comment by Joe Murray [ 08/Mar/17 ]

Yes, there are a variety of tax issues. I think it is best to have CRM issues for them, and to have PRs against those. This issue has been supplemented by two others and supplements 3 closed issues. I'm comfortable if you would prefer to refactor the existing issues. In one case we have changed a bug to an epic and are creating issues in epic for each PR so we can better manage all of the processes. I don't think it is a good idea to only work in PRs.

Comment by Eileen McNaughton [ 09/Mar/17 ]

"We are proposing to fix this with changes to CRM_Contribute_BAO_Contribution::checkTaxAmount() and a unit test, but no major refactoring. Are you in agreement with that approach?"

Yes, that is a good place to fix things. The way the function is called with the $isLineItem param is a bit of a bad code smell, but it is increasingly well tested.

Comment by Joe Murray [ 13/Mar/17 ]

Pradeep Nayak as a normal priority, please put this on your list for core pro bono contributions.

Comment by Eileen McNaughton [ 25/Apr/17 ]

Agileware this is one of those tickets that has become unreadable  because there are multiple issues in it. Can we close this out & get specific issues raised for the remaining issue/s

 

I think there have been some issues fixed but some issues / feature requests are outstanding

Comment by Agileware [ 25/Apr/17 ]

>  Can we close this out & get specific issues raised for the remaining issue/s

Yep, I think this one can be closed now. Happy to track CRM-19585

Comment by Agileware [ 25/Apr/17 ]

Oops, forgot the mention. Eileen McNaughton





[CRM-20172] "Separate Membership Payment" with Memberships enabled and additional contribution causes incorrect authorize.net transactions Created: 23/Feb/17  Updated: 24/Apr/17  Resolved: 24/Apr/17

Status: Closed
Project: CiviCRM
Component/s: CiviContribute, CiviMember
Affects Version/s: 4.7.14, 4.7.16
Fix Version/s: 4.7.20

Type: Bug Priority: Critical
Reporter: Thomas Bacon Assignee: Allen Shaw
Resolution: Fixed/Completed Votes: 0
Labels: PatchSubmitted, Regression, Reproduced
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: PNG File amounts.png     PNG File authorize.net.png     PNG File contact-contribution-record.png     Text File image-2017-04-21-10-24-25-232.png     Text File image-2017-04-21-10-24-54-411.png     Text File image-2017-04-21-10-25-28-088.png     Text File image-2017-04-21-10-27-05-719.png     PNG File memberships.png     PNG File success-page.png    
Issue Links:
Duplicate
is duplicated by CRM-20325 Separate membership payment with Auth... Closed
Versioning Impact: Patch (backwards-compatible bug fixes)
Documentation Required?: None
Funding Source: Needs Funding
Verified?: No

 Description   

Using the following clean/vanilla setup:
1. WordPress 4.7.2
2. CiviCRM 4.7.16
3. PHP7
4. Authorize.net

If you create contribution page with the following options:
1. Contribution Amounts section enabled
2. Memberships enabled
3. Separate Membership Payment

Authorize.net processes two transactions, but BOTH are for the contribution amount. The membership amount is not processed, and the contribution is set to "Pending (Incomplete Transaction)" in CiviCRM.

I've attached screenshots of one complete transaction and configuration for reference.

Let me know if you need any more info!



 Comments   
Comment by Stoob [ 29/Mar/17 ]

I've reproduced this issue on a client site and hope to start work soon. Thomas Bacon are you available for testing, debugging, sponsorship of a fix? there aren't a lot of available resources right now to fix things back at the home base, we may have to hire a contractor. I've got someone in mind.

Comment by Allen Shaw [ 31/Mar/17 ]

Marking as a regression, as I'm unable to reproduce on 4.6.27.

Comment by Allen Shaw [ 02/Apr/17 ]

BTW, I'm unable to repro this in 4.7.13, but can repro in 4.7.14.

Comment by Allen Shaw [ 03/Apr/17 ]

This appears to have been broken by one of the two commits in this PR, but still not sure which: https://github.com/civicrm/civicrm-core/pull/9444/commits

  • Definitely broken in 8e8d287
  • Can't quite tell if it's broken in this way in 13a16f4, because the contribution confirmation page dies with a fatal error, "DB Error: already exists".

 

Comment by Allen Shaw [ 03/Apr/17 ]

[duplicate comment, deleting.]

Comment by Eileen McNaughton [ 11/Apr/17 ]

Is this replicable with the DUMMY processor? Even at the debug level?

Comment by Peter Davis [ 11/Apr/17 ]

just flagging i have 9003 "duplicate transaction" errors with eway on a site i just upgraded to 4.7.18 where the Contrib page has Donations and memberships (not via a price set) with "Separate Membership Payments" checked - in case this is more of a generic issue (r12257)

Comment by Stoob [ 12/Apr/17 ]

Unable to reproduce with a dummy processor.  Please note that the primary problem occurs on the Authorize.net side, where the incorrect amount is transacted on the person's credit card, even though CiviCRM thinks the amount is correct.

Comment by Eileen McNaughton [ 12/Apr/17 ]

Allen Shaw note that there is an alterPaymentProcessorParams hook you could use when writing a test for a.net. You'd probably need to do something colourful with exceptions. It would be easier to test-capture in the Dummy processor if it's about what data hits doDirectPayment

Comment by Jitendra Purohit [ 12/Apr/17 ]

I was having a look at the similar issue we encountered yesterday (mentioned by Pete above) and made a quick patch for it https://gist.github.com/jitendrapurohit/1629eb609a0bf9acc1c02e6a13e84124 Not sure if this is entirely correct but fixes the eway issue for us.

Just a note that made be create the above patch is I've seen some related snippet in Utils.php - processConfirm() where we skip the doPayment() part when skipLineItem is 1 - https://github.com/civicrm/civicrm-core/blob/master/CRM/Contribute/BAO/Contribution/Utils.php#L131-L135. Would be happy to submit a PR if the diff seems fine.

Thanks.

Comment by Allen Shaw [ 12/Apr/17 ]

Peter Davis is there a JIRA ticket for the issue you mention?

Comment by Allen Shaw [ 12/Apr/17 ]

Peter Davis Sorry, one more: what's "r12257"?

Comment by Allen Shaw [ 12/Apr/17 ]

FYI, in the latest master, I'm also observing the error, "1: 11 A duplicate transaction has been submitted.", and still observe that there are two payments in Authorize.net, both in the amount of the non-membership contribution (and no payment for the membership contribution).

Comment by Peter Davis [ 12/Apr/17 ]

Allen - see Jitendra's links to github, not sure if there is JIRA  - suspect not. Apols re r... that is our fuzion code so we can reference back to our own ticket system.

Comment by Allen Shaw [ 14/Apr/17 ]

BTW, it does seem that this will repro with the Dummy Processor, but it's just not so apparent because Dummy doesn't maintain an independent log of transactions the way Authorize.net. As mentioned in the PR, I'm seeing the same problematic behavior (wrong amounts, wrong number of transactions) in the Dummy processor as in the Authorize.net processor. This is a good thing, as automated testing on Authorize.net seems like it would be hard to do.

I'm aiming to write tests for this first, and develop to make test pass.  I wonder if I'm wrong to think this needs a web test, and maybe there's a way to do this with straight phpunit, maybe even with API calls.

Comment by Eileen McNaughton [ 15/Apr/17 ]

don't bother with a webtest - they are rarely run - start from 

 

api_v3_ContributionPageTest:

testSubmitMembershipBlockIsSeparatePaymentPaymentProcessorNow

 

register the hook in the test & throw an exception from the hook if is is 'wrong', or set something in $params - which will get returned to the calling function

alterPaymentProcessorParams

 

eg. 

$this->hookClass->setHook('civicrm_aclWhereClause', array($this, 'hook_civicrm_aclWhereClause'));

from

CRM_ACL_ListTest

Comment by Allen Shaw [ 20/Apr/17 ]

[deleted comment]

Comment by Peter Davis [ 20/Apr/17 ]

it is a JIRA issue - i was writing a comment with lots of screenshots pasted in, but i haven't completed it, so was surprised that any notifications went out!

Comment by Peter Davis [ 20/Apr/17 ]

and have now 'cancelled' that comment so it won't show in this thread at all and other folk might wonder what you are talking about

Comment by Allen Shaw [ 21/Apr/17 ]

I don't know what you're talking about Peter Davis

Comment by Peter Davis [ 21/Apr/17 ]

we could both play at that game - but i won't  - or we will end up with a stream of messages about messages that no longer exist





Store card type and last 4 digits of credit card (CRM-20158)

[CRM-20265] Store CC type and last 4 digits from Membership form Created: 14/Mar/17  Updated: 22/Apr/17

Status: In Quality Assurance
Project: CiviCRM
Component/s: CiviContribute, CiviMember
Affects Version/s: 4.7.16
Fix Version/s: 4.7.18

Type: Sub-task Priority: Major
Reporter: Joe Murray Assignee: Pradeep Nayak
Resolution: Unresolved Votes: 0
Labels: PatchSubmitted
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Versioning Impact: Minor (add functionality in backwards-compatible manner)
Documentation Required?: None
Funding Source: Contributed Code
Verified?: No

 Description   

When purchasing a membership with a credit card, either front office or backoffice, store the cc type and last 4 digits on the civicrm_financial_trxn for the payment.

Please refactor https://github.com/civicrm/civicrm-core/pull/9881 to include the form work to cover this, and rely on the BAO and hopefully js work from CRM-20264.



 Comments   
Comment by Pradeep Nayak [ 26/Mar/17 ]

Submitted PR at https://github.com/civicrm/civicrm-core/pull/10051





Generated at Sat Apr 29 16:25:58 UTC 2017 using JIRA 7.3.3#73014-sha1:d5be8da522213be2ca9ad7b043c51da6e4cc9754.