CRM-20889 : Toggle check_number field on backoffice form as payment form fields

[monishdeb]

• CRM-20889: Toggle check_number field on backoffice form as payment form fields

[monishdeb]

@eileenmcnaughton can you review this patch about introducing check_number as payment field ?

[eileenmcnaughton]

I just gave it a quick skim and I approve it from a code point of view - it is an improvement and 64 is a fine score!

It's still pending actual testing - looks like it affects all back office forms that take payments

[jitendrapurohit]

Tested this changes on back office contribution, membership, pledge and event forms. Found two issues -

1. Contribution -
   a. Pay using credit card or some other instrument but not check. Save.
   b Edit the contribution record. Check number textbox is displayed even-though credit card is selected in payment_instrument_id field.

2. Events -
   a. Add an event registration for a contact using check or any method. Save.
   b. Click Edit form. Payment section is not shown on this page but Check number is displayed as it is.

[monishdeb]

@jitendrapurohit I have fixed #2 but for point #1, as per discussion with Eileen here I am going to replace the payment details section with a list of transactions on backoffice contribution edit form. As per the new spec, the payment fields will be populated on new payment edit form. So, in my opinion, we should skip #1 or else we need to include the patch of current PR as a part of CRM-20610 fix.

@eileenmcnaughton am I right?

[monishdeb]

Jenkin test this please

[eileenmcnaughton]

@monishdeb I think you are saying the payment form should not show up at all on the edit page (which I agree with) - but we can't merge this PR as is I don't think since it is loading with wrong info (from @jitendrapurohit's review )

[monishdeb]

@eileenmcnaughton in order to load correct info that involves loading the values of card_number and card_type_id as default set on respective payment fields (in this case), we need to fetch the latest transaction (i.e. the SELECT ft.* FROM financial_trxn ft WHERE eft.entity_id = <contribution id> AND eft.financial_trxn_id = ft.id ) . Which I did on #10386 but as per the new spec that we discussed earlier on MM I need to revert that change on #10386. So I am not sure if we need to bring that fix again for this PR in order to resolve the #1 , What ya say?

[eileenmcnaughton]

@monishdeb I feel like this is a good code improvement - it removes spaghetti & the UI is more consistent, and can stand alone from other changes.

I tried looking at the issue @jitendrapurohit raised & it can be fixed (at least on contribution form) like this

--- a/CRM/Contribute/Form/AbstractEditPayment.php
+++ b/CRM/Contribute/Form/AbstractEditPayment.php
@@ -516,7 +516,7 @@ WHERE  contribution_id = {$id}
       }
       $this->assignProcessors();
       $this->assignBillingType();
-      CRM_Core_Payment_Form::setPaymentFieldsByProcessor($this, $this->_paymentProcessor, FALSE, TRUE, CRM_Utils_Request::retrieve('payment_instrument_id', 'Integer'));
+      CRM_Core_Payment_Form::setPaymentFieldsByProcessor($this, $this->_paymentProcessor, FALSE, TRUE, CRM_Utils_Request::retrieve('payment_instrument_id', 'Integer', $this));
     }
     catch (CRM_Core_Exception $e) {
       CRM_Core_Error::statusBounce($e->getMessage());
diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php
index c471233..505949d 100644
--- a/CRM/Contribute/Form/Contribution.php
+++ b/CRM/Contribute/Form/Contribution.php
@@ -550,7 +550,8 @@ class CRM_Contribute_Form_Contribution extends CRM_Contribute_Form_AbstractEditP
       $paneNames[ts('Premium Information')] = 'Premium';
     }
 
-    if (CRM_Core_Payment_Form::buildPaymentForm($this, $this->_paymentProcessor, FALSE, TRUE, $this->getDefaultPaymentInstrumentId()) == TRUE) {
+    $this->payment_instrument_id = CRM_Utils_Array::value('payment_instrument_id', $defaults, $this->getDefaultPaymentInstrumentId());
+    if (CRM_Core_Payment_Form::buildPaymentForm($this, $this->_paymentProcessor, FALSE, TRUE, $this->payment_instrument_id) == TRUE) {
       if (!empty($this->_recurPaymentProcessors)) {
         $buildRecurBlock = TRUE;
         if ($this->_ppID) {

[eileenmcnaughton]

(I'm not 100% sure that there isn't a better way that involves less double-setting but I think that is too much scope creep here)

[eileenmcnaughton]

Note the payment details form replacement is currently pending further review by @guanhuan & @jamienovick so we need to make this one work by itself before we can merge

[eileenmcnaughton]

Also - I couldn't reproduce the issue on the event form after applying the patch above.

(note I didn't put the patch as a commit because I want to be able to merge the patch once it is correct & so I can't 'taint' it by adding code to it)

[eileenmcnaughton]

@monishdeb I feel like if you make the changes per my comment above then we can ping @jitendrapurohit to confirm it resolves his issue & merge this?

[monishdeb]

Thanks @eileenmcnaughton for the patch, I have updated the PR with it. After the fix, it correctly shows the payment fields w.r.t chosen payment method. However, it doesn't prefill the fields with respective default values, which I think won't matter much as per new UI changes.

@jitendrapurohit would you like to retest the latest patch, which addresses both the issues you raised. Thanks!

[eileenmcnaughton]

test this please

[jitendrapurohit]

@monishdeb Thanks for the update. Though the input field have been fixed, the header is still shown as "Check Information" for credit card fields. To see this yourself, use the same steps as mentioned in point#1 of #10680 (comment)

[eileenmcnaughton]

Odd - it should be getting the right label from
/**

• Get the name of the payment type.
• @return string
  */
  public function getPaymentTypeLabel() {
  return CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'payment_instrument_id', $this->getPaymentInstrumentID());
  }

[monishdeb]

@eileenmcnaughton that's because we are setting the $paymentInstrumentID in $paymentProcessor['object']['paymentInstrumentID'] and we are NOT calling the getPaymentTypeLabel(...) on that object instance. Here's my fix for that b3453b5

[jitendrapurohit]

All noted issues have been fixed now. Thanks @monishdeb @eileenmcnaughton

[eileenmcnaughton]

Thanks for the review @jitendrapurohit.

I'm going to merge this based on Jitendra's testing. I would note this is a clear code improvement & it is a code simplification we have been inching towards for some time.

@monishdeb + 14 -75 = gold star :-)

Thanks for your work on this.

[monishdeb]

Thanks, @jitendrapurohit @eileenmcnaughton for reviewing it
Yay 😄