CRM-21492 - Don't reject recurring contributions whose amount is diff…

[MegaphoneJon]

…erent from the initial amount

Overview

CiviCRM allows you to update the recurring payment amount, and successfully updates Authorize.Net - but IPNs are rejected because the code rejects any payment whose amount doesn't match the original amount.

Before

Submitting an IPN with an amount that doesn't match the initial amount results in no contribution being logged, and the CiviCRM log reports "Subscription amount mismatch".

After

The contribution is logged.

https://issues.civicrm.org/jira/browse/CRM-21492

[seamuslee001]

I think this makes sense to me. I would like to see a unit test if possible @MegaphoneJon any thoughts @eileenmcnaughton @monishdeb @KarinG ?

[KarinG]

I don’t have any sites using recurring w/ authorize.net - so can’t verify this but it does make sense. I don’t think it needs a test; The test would secure that someone does not re-add such code; I don’t see why anyone would approve of that. @MegaphoneJon do you have this in production? I like seeing PRs simmer on actual sites for a bit.

[MegaphoneJon]

I don't, but I will. I have the code running on a test site with a replay of a live IPN (with Eileen's notificationlog extension).

I thought I'd submit this patch before running it in production, because I figured a financial expert would weigh in if there was an obvious problem with this.

Since there isn't - how best to move forward? Shall I close this PR for a month, then reopen it if after a month we have no issues in production? Realistically it'll take a month of running in production to detect errors, because my client reconciles their donations monthly.

[eileenmcnaughton]

I'm prepared to merge this based on a short cool down. We have seen this cause problems in production enough for me to know that this check is unreliable & unhelpful. I'm prepared to let the test go for the reasons Karin suggests.

I think the check is there in case of the potential risk of fraud but we have no evidence of fraud-via-ipn and we do have a known scenario where the check makes no sense

[eileenmcnaughton]

Also @MegaphoneJon you might be interested to know that the Omnipay extension provides some support for a.net - not enough to replace the core version as yet, but if you set up a paralell Omnipay-AuthorizeNet processor it has api actions to retrieve all transactions from Authorize.net and identify ones not in CiviCRM. (it's almost but not quite capable of doing the pre-submit js token replacement that we discussed elsewhere)

[MegaphoneJon]

Ooh, that last point is interesting.

My client's site is on 4.6.33 - but this function appears to have minimal changes since 4.6. They're slated for a 4.7 upgrade, but I want to preface that the production testing will be on 4.6 (but thorough).

[eileenmcnaughton]

@jitendrapurohit you might want to weigh in here - I know Fuzion customers have been hit by this check.

[eileenmcnaughton]

Merging - I think this has been merge-ready for a suitable period of time.

[MegaphoneJon]

@eileenmcnaughton I tried using Omnipay as you described above and figured most of it out, but ran into trouble. I posted a question on Stack Exchange to publicly document what I figured out. It seems like I'm misconfigured, not that the actual error applies. If you're able to drop a hint there, I'd be very appreciative!