dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form #18517

monishdeb · 2020-09-18T12:12:10Z

Overview

This affects iATS Payments ACH/EFT payment processor which renders the payment block based on currency chosen.
Here are the steps to replicate:

Install and enable iATS paymentextension
Add and configure an iATS Payments ACH/EFT payment type payment processor.
Enable USD and CAD currencies
Go to 'Submit Credit Card Contribution' backoffice form
Choose payment processor created at step 2

Issue:

Payment block renders payment block without check template
Switching currency doesn't update the payment block.

Before

After

Technical Details

For more detail discussion on MM - https://chat.civicrm.org/civicrm/pl/ixxrxhxs43r4mfyjpnf6tnwnnc

https://lab.civicrm.org/dev/financial/-/issues/150

ping @KarinG @Edzelopez @eileenmcnaughton @JoeMurray

civibot · 2020-09-18T12:12:13Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

mattwire · 2020-09-18T13:02:38Z

CRM/Contribute/Form/AbstractEditPayment.php

-      $currentCurrency
-    );
-    return $result;
+    return $submittedValues['currency'] ?? $this->_values['currency'] ?? CRM_Core_Config::singleton()->defaultCurrency;


This part looks good - it's just cleanup.

mattwire · 2020-09-18T13:05:14Z

templates/CRM/common/paymentBlock.tpl

@@ -95,7 +95,10 @@

      var payment_instrument_id = $('#payment_instrument_id').val();

-      var dataUrl = "{crmURL p='civicrm/payment/form' h=0 q="formName=`$form.formName`&currency=`$currency`&`$urlPathVar``$isBackOfficePathVar``$profilePathVar``$contributionPageID``$preProfileID`processor_id="}" + type;
+      var currency = '{$currency}';
+      currency = currency == '' ? $('#currency').val() : currency;


This is the only bit I have concerns with - why would the currency not be set when the form loads?

@mattwire actually there are two cases where the currency variable is not set in backend, these are:

When we open the standalone CC form for submitting new live contribution, where we are setting a NULL currency value in here Although the default currency set in the system can be used in here instead of NULL. So I am not sure if we need to do that.

When we are changing the currency beside the amount field https://github.com/civicrm/civicrm-core/pull/18517/files#diff-726c3dcc8c2364a0b9b8f0c0e11bcd34R122 which call the js function, and in this case the code should consider what currency value has been chosen in b/o form and when in reloads the snippet

This is not a issue on edit CC form where currency variable is set.

@monishdeb can we pass currency though the url for case 2?

@eileenmcnaughton yes I am passing the currency value for the 2nd case, if you look at L122 below, I am calling the buildPaymentBlock() fn on currency change and eventually at L99 we are considering the currency value and passing through the url at L144

mattwire · 2020-09-18T13:06:16Z

I've raised a couple of comments for an r-code review. I haven't read through the detail behind this PR so they may have already been answered.

eileenmcnaughton · 2020-09-18T19:34:05Z

test this please

KarinG · 2020-09-18T19:41:33Z

When Jenkins is happy I'll test it.

eileenmcnaughton · 2020-09-18T20:15:30Z

thanks @KarinG

samuelsov · 2020-10-13T22:34:36Z

templates/CRM/common/paymentBlock.tpl

-    $('[name=payment_processor_id]').on('change.paymentBlock', function() {
-        buildPaymentBlock($(this).val());
+    $('[name=payment_processor_id], #currency').on('change.paymentBlock', function() {
+      buildPaymentBlock($('[name=payment_processor_id]').val());


We need to add :checked or it will always take the first option.

Tested on a site with a payment processor and pay later and the pay later option was broken.

Thanks @samuelsov for raising this issue and suggesting the fix. I have updated the PR with this commit b86dc53 as per which we need to consider both the w/o pay-later cases.

mattwire · 2020-10-28T09:47:02Z

@monishdeb Looks like this needs a bit of work?

…n backoffice live CC form

KarinG · 2020-10-30T23:52:47Z

"Submit Credit Card Contribution backoffice form" -> it's not at all obvious that this is where one would go to process an ACH EFT (i.e. a non Credit Card) transaction.

JoeMurray · 2020-10-31T00:01:20Z

We were thinking of a separate PR for changing the name since it really should be Submit Online Contribution or something similar, @KarinG . What do you think?

eileenmcnaughton · 2020-10-31T00:03:50Z

I think you are brave :-) Changing button names is fighting talk. But yes, separate PR or issue don't mix it in with this

KarinG · 2020-10-31T00:04:52Z

I think this PR requires a new button -> "Process ACH EFT"

KarinG · 2020-10-31T00:08:37Z

@adixon - ping!

eileenmcnaughton · 2020-10-31T00:17:43Z

@KarinG no - a SEPARATE PR can cover that....

mlutfy · 2020-11-06T18:45:56Z

templates/CRM/common/paymentBlock.tpl

-        buildPaymentBlock($(this).val());
+    $('[name=payment_processor_id], #currency').on('change.paymentBlock', function() {
+      var payment_processor_id = $('[name=payment_processor_id]:checked').val() == undefined ? $('[name=payment_processor_id]').val() : $('[name=payment_processor_id]:checked').val();
+      buildPaymentBlock(payment_processor_id);


@monishdeb This breaks backend New Contribution forms without a payment processor (ex: Contact Record -> New Contribution, then change the currency).

Adding "if (payment_contribution_id != undefined) { .. }" fixes it, although I haven't tested extensively.

For reference, this form:

mattwire · 2020-11-09T21:23:54Z

Hi @monishdeb This is awaiting changes following @mlutfy testing.

monishdeb · 2020-12-22T18:09:20Z

sorry for delay I have updated the PR.

mlutfy · 2020-12-22T19:38:59Z

Seems good to me, but I will let someone merge since it's not my area of expertise.

Symbiotic has been using this patch in production (with the latest commit) for a few weeks and seems to be working well.

eileenmcnaughton · 2020-12-22T19:45:47Z

I don't have any remaining concerns - @mattwire ?

monishdeb · 2021-07-07T04:12:16Z

ping @eileenmcnaughton @mattwire

civibot bot added the master label Sep 18, 2020

mattwire reviewed Sep 18, 2020

View reviewed changes

KarinG mentioned this pull request Sep 18, 2020

Backoffice contribution credit card form unable to update the billing block iATSPayments/com.iatspayments.civicrm#331

Open

mlutfy changed the title ~~financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form~~ dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form Oct 8, 2020

samuelsov reviewed Oct 13, 2020

View reviewed changes

monishdeb added 2 commits October 30, 2020 17:51

financial#150: civicrm/payment/form url got empty currency argument i…

7aacd50

…n backoffice live CC form

fix use-case where both paylater and payment processor is used

b86dc53

monishdeb force-pushed the financial-issue-150 branch from 4614ad1 to b86dc53 Compare October 30, 2020 12:23

mlutfy reviewed Nov 6, 2020

View reviewed changes

mlutfy added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Nov 6, 2020

QA fix

10495d0

mlutfy removed the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Dec 22, 2020

mattwire merged commit 44f9625 into civicrm:master Jul 7, 2021

monishdeb deleted the financial-issue-150 branch October 4, 2024 05:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form #18517

dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form #18517

monishdeb commented Sep 18, 2020 •

edited by mlutfy

Loading

civibot bot commented Sep 18, 2020

mattwire Sep 18, 2020

mattwire Sep 18, 2020

monishdeb Sep 18, 2020 •

edited

Loading

eileenmcnaughton Sep 18, 2020

monishdeb Oct 30, 2020

mattwire commented Sep 18, 2020

eileenmcnaughton commented Sep 18, 2020

KarinG commented Sep 18, 2020

eileenmcnaughton commented Sep 18, 2020

samuelsov Oct 13, 2020

samuelsov Oct 13, 2020

monishdeb Oct 30, 2020

mattwire commented Oct 28, 2020

KarinG commented Oct 30, 2020

JoeMurray commented Oct 31, 2020

eileenmcnaughton commented Oct 31, 2020

KarinG commented Oct 31, 2020

KarinG commented Oct 31, 2020

eileenmcnaughton commented Oct 31, 2020

mlutfy Nov 6, 2020 •

edited

Loading

mattwire commented Nov 9, 2020

monishdeb commented Dec 22, 2020

mlutfy commented Dec 22, 2020

eileenmcnaughton commented Dec 22, 2020

monishdeb commented Jul 7, 2021

dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form #18517

dev/financial#150: civicrm/payment/form url got empty currency argument in backoffice live CC form #18517

Conversation

monishdeb commented Sep 18, 2020 • edited by mlutfy Loading

Overview

Before

After

Technical Details

civibot bot commented Sep 18, 2020

mattwire Sep 18, 2020

Choose a reason for hiding this comment

mattwire Sep 18, 2020

Choose a reason for hiding this comment

monishdeb Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton Sep 18, 2020

Choose a reason for hiding this comment

monishdeb Oct 30, 2020

Choose a reason for hiding this comment

mattwire commented Sep 18, 2020

eileenmcnaughton commented Sep 18, 2020

KarinG commented Sep 18, 2020

eileenmcnaughton commented Sep 18, 2020

samuelsov Oct 13, 2020

Choose a reason for hiding this comment

samuelsov Oct 13, 2020

Choose a reason for hiding this comment

monishdeb Oct 30, 2020

Choose a reason for hiding this comment

mattwire commented Oct 28, 2020

KarinG commented Oct 30, 2020

JoeMurray commented Oct 31, 2020

eileenmcnaughton commented Oct 31, 2020

KarinG commented Oct 31, 2020

KarinG commented Oct 31, 2020

eileenmcnaughton commented Oct 31, 2020

mlutfy Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

mattwire commented Nov 9, 2020

monishdeb commented Dec 22, 2020

mlutfy commented Dec 22, 2020

eileenmcnaughton commented Dec 22, 2020

monishdeb commented Jul 7, 2021

monishdeb commented Sep 18, 2020 •

edited by mlutfy

Loading

monishdeb Sep 18, 2020 •

edited

Loading

mlutfy Nov 6, 2020 •

edited

Loading