-
Notifications
You must be signed in to change notification settings - Fork 50
Multiple fixes for PaypalRest #166
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
base: master
Are you sure you want to change the base?
Conversation
@eileenmcnaughton Have updated the comments and this PR with a fix that I think works in all circumstances as well as a core PR to resolve properly in the future. |
a5813ee
to
69e16ba
Compare
4c6ba3d
to
6b69b7c
Compare
Matt, thanks for all the work you've done on this. You wrote 'Note that the existing omnipay implementation only works for a fairly specific contribution page configuration and will need some work (see #166). The "paypal checkout" is a "modern" javascript implementation just like Stripe and comes with all the same form integration issues.' at https://chat.civicrm.org/civicrm/pl/xtu8j71ox3gt3m6q7fex7r3afo
|
@JoeMurray I think in testing I mostly tested pretty standard contribution page submissions - the thing I know to be a problem is multiple participants on event pages (which is kind of a core issue) - I know a few sites are using Paypal Rest - which I believe is now called paypal commerce |
Thanks, @eileenmcnaughton . We'll move forward with implementing this for them. |
@JoeMurray This comment explains one of the issues quite well: https://github.com/eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor/pull/166/files#diff-afb50e3e775ccddec52bffea2f6cfb187cbea9d159380c3db080b1e423bb53b7R77-R80 Fundamentally doing javascript based paymentprocessors in civicrm is hard! I've put 100s of hours into the mjwshared (paymentshared) extension to create a compatibility layer that makes it much easier. The CRM.payment js library is a key part of that and is well commented to explain what each function is doing - https://lab.civicrm.org/extensions/mjwshared/-/blob/1.0/js/crm.payment.js It also allows you to run it manually for debugging on the browser console - eg. |
I originally did some of the work on this PR without introducing the dependency on mjwshared but quickly realised that by far the fastest and safest way was just to use the existing, well-used code that I know works. By using that library you effectively get support "everywhere that stripe works" including drupal webform7, 8, various configurations of contribution pages, event registration pages, frontend and backend payments (though paypal checkout doesn't support backend payments at the moment). |
5489f27
to
b6bfaf5
Compare
b6bfaf5
to
d488480
Compare
8f31154
to
679df0f
Compare
Note that this is not really ready to merge as-is because it introduces a dependency on the MJWShared extension to use the CRM.payment library. As I've solved most of the issues on the javascript layer there it seems silly to re-implement again. The idea would be to get a version of CRM.payment merged into CiviCRM core where we can depend on it directly.
Following changes in #164 and #165 this should resolve both issues. Core PR civicrm/civicrm-core#18141 should allow us to fix this properly one day (I thought it already worked but on looking at Stripe realised I only partially fixed core last time and kept a workaround in Stripe).
The issues that @mwestergaard were almost certainly caused by paypal checkout being default and us trying to addVars into the billing-block region. Because in that situation
billing-block
tries to definevar CRM =
andhtml-header
tries to doCRM.$.extend(CRM ...
which will never work becausehtml-header
always loads first..Log message if CRM.vars.omnipay are not defined instead of trying to load checkout - This should be completely safe and simple to cherry-pick and helps with debugging in situations when things are not loading properly.
Re-work Fixes for PayPal Checkout with drupal webform and other CiviCRM forms #164 so getTotalAmount works in all cases: There seems to be a scope issue in some situations with the omnipay_PaypalRest.js code so that in some cases it didn't find the
getTotalAmount()
function. The simplest solution is to remove the function and get the amount inline (per this commit).Load PaypalRest js via URL and cache code - optional and "independent" of the rest of these fixes - switches the script from inline to loaded via URL and cache code. This means the script is only loaded once and cached in the browser which reduces future load times a little bit. It's not essential to the rest of the PR.
Add fallback to load CRM.vars.omnipay if not loaded via html-header region