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

Fixes for PayPal Checkout with drupal webform and other CiviCRM forms #164

Merged
merged 2 commits into from
Aug 1, 2020

Conversation

mattwire
Copy link
Contributor

This fixes two issues with PayPal Checkout implementation.

  1. Does not work with multiple payment processors on the form and PayPal is not the default:
    Fixed by loading variables into billing-block instead of default (html-header).

  2. calculateTotalFee function only exists in specific circumstances on certain CiviCRM forms. If it doesn't exist the current PayPal checkout implementation crashes.
    Fixed by using a copy of implementation from mjwshared/Stripe where we check for various ways of getting the amount from the form. To work with drupal webform requires this PR Add CRM.payment.getTotalAmount() function (fix for PayPal / Stripe) colemanw/webform_civicrm#331

I have been shipping now for a while and developing a CRM.payment library to help with javascript processor implementations. This + the webform PR is the first implementation that would "publicly" use one of the functions - getTotalAmount() and it's designed in such a way that 3rd-party integrations (such as webform) can ship and maintain their own copies of these functions. See https://lab.civicrm.org/extensions/mjwshared/-/blob/0.9/js/crm.payment.js#L160-163 and https://github.com/colemanw/webform_civicrm/pull/331/files#diff-3d9bb68a7f54911dbc5f1017646f40caR137-R142

This PR does not required mjwshared and is written in a way that will work whether or not it finds a CRM.payment library.

@civibot civibot bot added the master label Jul 30, 2020
@eileenmcnaughton
Copy link
Owner

I think I'd rather have a copy of any js within this extension, rather than have it call a non-dependency, if exists. I'd be happy to merge this without those lines as they seem tangental & needing more thought

@mattwire
Copy link
Contributor Author

@eileenmcnaughton What do you think of the webform PR I linked to? That's an example implementation of the CRM.payment.getTotalAmount function and means you (and I) don't need to maintain webform specific code in our payment processors.

@eileenmcnaughton
Copy link
Owner

eileenmcnaughton commented Jul 31, 2020

@mattwire - I think it's a good idea not to have to handle webform in different processors . It says to me that core needs to provide some sort of js api for it & that might just look like moving your class into core & adding documentation.

@mattwire
Copy link
Contributor Author

mattwire commented Aug 1, 2020

might just look like moving your class into core & adding documentation.

Yes :-) It's been evolving and I'm happy that CRM.payment.getTotalAmount() is ready. Per comments on webform PR it's written in a way to work wherever it is defined so that it's not dependent on existing in core, existing in mjwshared etc. and will "just-work". For example if this PR was merged now and we later add CRM.payment.getTotalAmount() to core they will play nicely together.

@eileenmcnaughton
Copy link
Owner

@mattwire since the webform one is merged I'll merge this - I don't have my head fully into this so i'm leaning on your expertise a bit on this one

@eileenmcnaughton eileenmcnaughton merged commit b73b812 into eileenmcnaughton:master Aug 1, 2020
@eileenmcnaughton
Copy link
Owner

@mattwire I've had to revert this based on #165

I'm afraid I actually merged without doing an r-run but now that I am trying I can replicate it too.

@eileenmcnaughton
Copy link
Owner

It could be caching in some way? I just went for a quick revert & re-tag in order to get the immediate issue fixed - if it is caching I'm not sure how we will ensure caches are cleared. Perhaps it requires a whole upgrade fn

@eileenmcnaughton
Copy link
Owner

Oh - I realised I accidentally tested "Does not work with multiple payment processors on the form and PayPal is not the default" - & the commit I didn't revert seems to have fixed that.

mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Aug 10, 2020
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Aug 13, 2020
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Sep 23, 2020
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Dec 21, 2020
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Dec 21, 2020
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Mar 10, 2021
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Feb 13, 2022
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Mar 7, 2022
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Aug 20, 2022
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Oct 14, 2022
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Sep 20, 2023
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Oct 1, 2023
mattwire added a commit to mattwire/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Oct 23, 2023
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

2 participants