Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-20048 Parse "business" not "receiver_email" from IPN #9858

Merged
merged 1 commit into from
Feb 19, 2017

Conversation

ajdavis
Copy link
Contributor

@ajdavis ajdavis commented Feb 18, 2017

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@ajdavis ajdavis changed the title paypal-ipn-business-name CRM-20048 Parse "business" not "receiver_email" from IPN Feb 18, 2017
@seamuslee001
Copy link
Contributor

@eileenmcnaughton does this make sense to you, I know you have dealt in IPNs a bit

@eileenmcnaughton
Copy link
Contributor

@sunilpawar - what is your degree of confidence on this?

@sunilpawar
Copy link
Contributor

@eileenmcnaughton We had same issue with Paypal email and after applying these changes our problem solved. its more than 6 month no issue reported by customer or detected in CiviCRM log. So i am 100% with this fix.

@eileenmcnaughton eileenmcnaughton merged commit 476b9d4 into civicrm:master Feb 19, 2017
@eileenmcnaughton
Copy link
Contributor

Thanks @ajdavis, @seamuslee001 @sunilpawar - merged

@xurizaemon
Copy link
Member

@eileenmcnaughton I'm not sure the JIRA issue was resolved, my read was that for some (many?) sites business is working and for others receiver_email is correct. I'm wondering if this got merged a bit prematurely? It might be fine, eg if there's confidence that receiver_email will always be the correct value to use here.

@eileenmcnaughton
Copy link
Contributor

I understood from the report by @ajdavis & from @sunilpawar that business is always Ok & receiver_email is sometimes

@eileenmcnaughton
Copy link
Contributor

I guess @sunilpawar it's not 100% clear if you rolled out for all cividesk customers or just one - which would affect my read on your confidence

@xurizaemon
Copy link
Member

xurizaemon commented Feb 21, 2017

We do use business elsewhere, eg here in PayPalImpl.php. So it seems like it ought to be correct. But I'd rather be certain - my comments over on JIRA were before I looked at the code to check my suspicions, and I haven't reviewed transaction logs from PayPal customers myself.

Worth noting that this block of code only gets hit if the IPN URL does not contain processor_id, in which case we look up the processor by processor_type="PayPal_Standard" AND user_name={business or receiver_email}".

It seems we use CRM_Core_Payment::getNotifyURL() for this, in which case ... how are sites NOT getting the right processor_id? Does PayPal ignore the notify_url input we send? Are they old transactions?

It looks like this fix shouldn't even be required if we set notify_url correctly, the email lookup is just a failover.

@ajdavis
Copy link
Contributor Author

ajdavis commented Feb 22, 2017

Thanks, folks. My CiviCRM / WordPress site is receiving IPNs like:

transaction_subject=&\
  payment_date=11:46:34 Feb 09, 2017 PST&\
  txn_type=web_accept&\
  last_name=REDACTED&\
  residence_country=US&\
  item_name=Online Event Registration: February 2017 Zazenkai&\
  payment_gross=25.00&\
  mc_currency=USD&\
  business=info@REDACTED.COM&\
  payment_type=instant&\
  protection_eligibility=Ineligible&\
  verify_sign=ATZo1sTL1YpCT4KW-CzFolXviWIiAUFFSW.wttJfYaUeNDFVJtad0U6u&\
  payer_status=unverified&\
  payer_email=REDACTED&\
  txn_id=8PR304816S824093S&\
  quantity=1&\
  receiver_email=other_email@REDACTED.COM&\
  first_name=REDACTED&\
  invoice=f8bedc4c6070f984c98817cda91347e7&\
  payer_id=783J5MWNJBSY8&\
  receiver_id=WWNZLQH42EBR8&\
  item_number=&\
  payment_status=Completed&\
  payment_fee=0.85&\
  mc_fee=0.85&\
  mc_gross=25.00&\
  custom=&\
  charset=windows-1252&\
  notify_version=3.8&\
  ipn_track_id=b79ad98c923f9

Where "info@REDACTED.COM" is the PayPal account's primary email address and "other_email@REDACTED.COM" is the address I set up for notifications. I see no processor_type.

@kcristiano
Copy link
Member

@xurizaemon from my read of your comments, we've been mapping the 'receiver_email' value from PayPal to 'user_name' in CiviCRM and this patch will change that to mapping the 'business' value from PayPal to 'user_name' in CiviCRM.

This certainly sounds like the proper solution for the use case in https://issues.civicrm.org/jira/browse/CRM-20048 where there was an email change at PayPal. I have not tested this patch when there was _not _ an email change. @nganivet have you deployed this to all CiviDesk clients? If so and it is working, that would make me feel better.

I'd prefer to be cautious here and ensure we are confident in this change before releasing 4.7.17. In @ajdavis 's case, we can apply this patch in the interim as it solves his issue.

@xurizaemon
Copy link
Member

xurizaemon commented Feb 22, 2017

@ajdavis thanks. Can you confirm which CiviCRM version you're using? In 4.6 I don't expect you to get processor_id set, while in 4.7 the processor should be identified via the IPN URL (civicrm/payment/ipn/%processor_id).

That value ought to be carried through to processor_id via the code in CRM_Core_Payment::handlePaymentMethod().

If you're on 4.7 and seeing IPN on a URL that includes such a processor ID either in the path or parameters, AND you're affected by the bug which this PR fixes, my feeling is that a better fix here should be to get processor lookup by passed in ID working.

@xurizaemon
Copy link
Member

xurizaemon commented Feb 22, 2017

@kcristiano my read of the situation (discussion) is that business is "more correct", but that this proposed change doesn't allow for the possibility of breakage for CiviCRM sites currently not affected, and further that we have an existing means to identify the processor in question.

IMO we need to better understand why this is presenting as a problem, and whether the proposed fix is safe for unaffected sites.

@ajdavis
Copy link
Contributor Author

ajdavis commented Feb 22, 2017

Thanks @xurizaemon, we're running CiviCRM 4.6.24, so we don't have the processor_id parameter yet.

@eileenmcnaughton
Copy link
Contributor

OK - more questions for @sunilpawar - ie. did you test this on 4.7 or 4.6 (and have you been running on multiple sites over that time period).

I believe that 4.7 would still be affected by IPNs for old recurring contributions - even if the new ones use the alternate method

@sunilpawar
Copy link
Contributor

Change we did on 4.6 version and multiple site running on same code-base.

@eileenmcnaughton
Copy link
Contributor

Ah - so no-one has tested this on 4.7 - that makes me less comfortable. We need to ask better questions.

However, the 'multiple site running on same code-base' gives me a fair bit of confidence and on 4.6 that line of code would have been hit MORE rather than less often than in 4.7

@nganivet
Copy link
Contributor

nganivet commented Feb 22, 2017

@kcristiano @eileenmcnaughton Yes, this patch been installed on all our customer sites on 7/26/2016 and we have not experienced any issues since. This is the comment that Sunil inserted in the code when making that change:
// receiver_email and business most of the time are same, if differ then use 'business'
// business email stored in payment processor table
// https://developer.paypal.com/docs/classic/ipn/integration-guide/IPNandPDTVariables/

@eileenmcnaughton
Copy link
Contributor

thanks - that feels ok then

monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017
CRM-20048 Parse "business" not "receiver_email" from IPN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants