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

Multiple fixes for PaypalRest #166

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Aug 10, 2020

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 define var CRM = and html-header tries to do CRM.$.extend(CRM ... which will never work because html-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

@mattwire
Copy link
Contributor Author

@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.

@mattwire mattwire changed the title Paypal getTotalAmount (re #164, #165) Multiple fixes for PaypalRest Sep 24, 2020
@JoeMurray
Copy link
Contributor

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

  1. We are thinking of using PayPal Checkout for a tiny org that uses Joomla for a) a simple membership form and b) a priceset donation form with several simple fields. Do you see any problems in this setup given you think this only works for a 'fairly specific contribution page configuration'? Could you maybe put what works and what doesn't into the issue description above?

  2. I think it would help others do reviews and QAs if this was broken up into separate PRs that could be independently reviewed and merged. Indicating dependencies between them would be great. Would you be able to do that? Or minimally if someone tried to do that based on the notes above on something you say is WIP, would you be able to provide feedback? I'm trying to figure out the process for helping this to progress.

@eileenmcnaughton
Copy link
Owner

@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

@JoeMurray
Copy link
Contributor

Thanks, @eileenmcnaughton . We'll move forward with implementing this for them.

@mattwire
Copy link
Contributor Author

@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. CRM.payment.getTotalAmount() which is really helpful for debugging integration issues and troubleshooting. On the one hand I would love to see this in core but on the other hand I'd be worried that it would lead to a "loss of control" over that library which currently, as part of mjwshared, provides a "sliding window" compatibility layer which allows it to work across multiple versions of CiviCRM and provide fixes for issues that are fixed only in later versions. On the other hand I don't think it is sustainable to implement everything multiple times (for each paymentprocessor) as it has been quite a challenge to build a system that works in all situations!

@mattwire
Copy link
Contributor Author

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).

@mattwire mattwire force-pushed the paypal_fixes branch 3 times, most recently from 5489f27 to b6bfaf5 Compare October 14, 2022 15:47
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.

None yet

3 participants