CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partia… #10271

eileenmcnaughton · 2017-04-28T01:41:58Z

@monishdeb FYI - calling $this->processBillingAddress(); if the way other forms have been handling all that billing profile code -

It should be called in the main processing routine to support config where billing address is required for pay later, it runs off whether the fields are set

eileenmcnaughton · 2017-04-28T01:42:49Z

NB - I've just opened this to share with you in conjunction with the form tidy up on #10126 - will close later

monishdeb · 2017-04-28T11:33:31Z

@eileenmcnaughton I have updated the PR with desired changes, please have a look

eileenmcnaughton · 2017-04-28T11:42:08Z

CRM/Contribute/Form/AdditionalPayment.php

    }

    $defaults = array();
    $contribution = civicrm_api3('Contribution', 'getsingle', array(
      'return' => array("contribution_status_id"),
      'id' => $this->_contributionId,
    ));
+    // store transaction date time instead of date only
+    $this->_params['trxn_date'] = CRM_Utils_Date::processDate($this->_params['trxn_date'], CRM_Utils_Array::value('trxn_date_time', $this->_params));


I don't think we need this

eileenmcnaughton · 2017-04-28T11:42:25Z

CRM/Contribute/Form/AdditionalPayment.php

    if (!$this->_mode) {
      $js = array('onclick' => "return verify( );");
+
+      $this->add('select', 'payment_instrument_id',


these forms aren't actually showing on the credit card form anyway

Ya but I think that we shouldn't render these quickForm elements needlessly when its not in use.. So I thought why not to move the code here rather exposing it.

eileenmcnaughton · 2017-04-28T11:42:38Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -211,9 +211,10 @@ public function buildQuickForm() {
    }

    CRM_Core_Payment_Form::buildPaymentForm($this, $this->_paymentProcessor, FALSE, TRUE, CRM_Utils_Request::retrieve('payment_instrument_id', 'Integer'));
+    $this->add('select', 'payment_processor_id', ts('Payment Processor'), $this->_processors, NULL);


does this change do anything?

Nope, it's just shifting code to (from L216 to L214) meaningful place

eileenmcnaughton · 2017-04-28T11:43:48Z

CRM/Contribute/Form/AdditionalPayment.php

-      $this->_contactId,
-      NULL, NULL,
-      $ctype
+    $fields = $this->formatParamsForPaymentProcessor($this->_params);


ok - so using the parent function to create the profile contact outside the credit card processing didn't work?

that comment was actually for the next line...

eileenmcnaughton · 2017-04-28T11:44:22Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -502,20 +461,24 @@ public function processCreditCard($submittedValues) {
    $paymentParams['contactID'] = $this->_contactId;
    CRM_Core_Payment_Form::mapParams($this->_bltID, $this->_params, $paymentParams, TRUE);

+    // add some financial type details (as contributionType_name) to the params list


why do we want to assign contributionType_name?

Because it's used in few Payment processor, as per grep

CRM/Core/Payment/Elavon.php:105: * $params['contributionType_name' ]; CRM/Core/Payment/FirstData.php:137: // $requestFields[ '' ] = $params[ 'contributionType_name' ]; CRM/Core/Payment/Realex.php:369: $this->_setParam('varref', $params['contributionType_name']);

hmm - in a deprecated kind of a way - if it wasn't working from this form before & no-one has reported it as a problem I don't think we should add it

ok lemme remove it then

eileenmcnaughton · 2017-04-28T11:46:58Z

CRM/Contribute/Form/AdditionalPayment.php

+    CRM_Contact_BAO_Contact::createProfileContact($params, $fields,
+       $this->_contactId,
+       NULL, NULL,
+       $ctype


$this->processBillingAddress() is the function I would expect to have already done createProfileContact

eileenmcnaughton · 2017-04-28T11:47:36Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -476,6 +432,8 @@ public function processCreditCard($submittedValues) {
      $config->defaultCurrency
    );

+    // take recieve_date as current or from trxn_date
+    $this->_params['receive_date'] = date('Ymd');


is this right? contribution receive date would change?

eileenmcnaughton · 2017-04-28T11:55:19Z

CRM/Contribute/Form/AdditionalPayment.php

-
-    // Get the required fields value only.
-    $params = $submittedValues;
-


I haven't made sense of this part - if you have reviewed it I'm fine with that since I know Pradeep concluded it was cruft

Yup, I have removed it after testing

eileenmcnaughton · 2017-04-28T11:56:33Z

CRM/Contribute/Form/AdditionalPayment.php

-    // Add all the additional payment params we need.
-    $this->_params["state_province-{$this->_bltID}"] = $this->_params["billing_state_province-{$this->_bltID}"] = CRM_Core_PseudoConstant::stateProvinceAbbreviation($this->_params["billing_state_province_id-{$this->_bltID}"]);
-    $this->_params["country-{$this->_bltID}"] = $this->_params["billing_country-{$this->_bltID}"] = CRM_Core_PseudoConstant::countryIsoCode($this->_params["billing_country_id-{$this->_bltID}"]);
-


yep - handled above

eileenmcnaughton · 2017-04-28T12:02:14Z

CRM/Contribute/Form/AdditionalPayment.php

      }
    }

-    if ($result) {
-      $this->_params = array_merge($this->_params, $result);
+    // If payment is not completed then bounce approrptiate status message


isn't it more readable to keep all this in the catch block? I can't see anywhere else where $errorMessage is set.

eileenmcnaughton · 2017-04-28T12:02:46Z

CRM/Contribute/Form/AdditionalPayment.php

+    ) {
+      //set the contribution mode.
+      $urlParams = "action=add&cid={$this->_contactId}&id={$this->_contributionId}&component={$this->_component}&mode={$this->_mode}";
+      if (!$errorMessage) {


this could never be true

eileenmcnaughton · 2017-04-28T12:03:40Z

CRM/Contribute/Form/AdditionalPayment.php

+      if (!$errorMessage) {
+        $errorMessage = ts('Payment from payment processor failed.');
+      }
+      CRM_Core_Session::singleton()->setStatus($errorMessage, ts('Payment Failed'), 'error');


Any thoughts how this compares to using CRM_Core_Error::statusBounce from this line?

eileenmcnaughton · 2017-04-28T12:05:40Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -591,18 +562,25 @@ public function emailReceipt(&$params) {
    // assign payment info here
    $paymentConfig['confirm_email_text'] = CRM_Utils_Array::value('confirm_email_text', $params);
    $this->assign('paymentConfig', $paymentConfig);
+
+    $this->assign('totalAmount', $this->_amtTotal);


what is all this about?

just a code shift as there always be $this->_amtTotal, so I just moved it from the IF-ELSE code below

but I don't see isAmountzero being removed?

actually I am talking about the code removed at L598 and L605

eileenmcnaughton · 2017-04-28T12:10:11Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -611,14 +589,10 @@ public function emailReceipt(&$params) {
    // assign trxn details
    $this->assign('trxn_id', CRM_Utils_Array::value('trxn_id', $params));
    $this->assign('receive_date', CRM_Utils_Array::value('trxn_date', $params));
-    $paymentInstrument = CRM_Contribute_PseudoConstant::paymentInstrument();
-    if (array_key_exists('payment_instrument_id', $params)) {


hmm - so this would be set for credit card too by now?

eileenmcnaughton · 2017-04-28T12:11:26Z

CRM/Contribute/Form/AdditionalPayment.php

-    }
+    $paymentInstruments = CRM_Contribute_PseudoConstant::paymentInstrument();
+    $this->assign('paidBy',
+      CRM_Utils_Array::value($params['payment_instrument_id'], $paymentInstruments)


$this->assign('paidBy', CRM_Core_PseudoConstant::getLabel('CRM_Contribute_BAO_Contribute', 'payment_instrument_id, $params['payment_instrument_id']);

Is the way we are going on these

oops ya missed it

eileenmcnaughton · 2017-04-28T12:11:36Z

templates/CRM/Contribute/Form/AdditionalPayment.tpl

-  <div class="messages status no-popup">
-    <div class="icon inform-icon"></div>&nbsp;{ts}You will not be able to send an automatic email receipt for this payment because there is no email address recorded for this contact. If you want a receipt to be sent when this payment is recorded, click Cancel and then click Edit from the Summary tab to add an email address before recording the payment.{/ts}
-  </div>
+    <div class="messages status no-popup">


eileenmcnaughton · 2017-04-28T12:14:03Z

templates/CRM/Contribute/Form/AdditionalPayment.tpl

@@ -97,6 +97,20 @@
      <td>
        <span id='totalAmount'>{$form.currency.html|crmAddClass:eight}&nbsp;{$form.total_amount.html|crmAddClass:eight}</span>&nbsp; <span class="status">{if $paymentType EQ 'refund'}{ts}Refund Due{/ts}{else}{ts}Balance Owed{/ts}{/if}:&nbsp;{$paymentAmt|crmMoney}</span>
      </td>
+      {if $email and $outBound_option != 2}


this seems fine - didn't Pradeep also rip out a tonne of js that is not required now

eileenmcnaughton · 2017-04-28T12:43:31Z

CRM/Contribute/Form/AdditionalPayment.php

      }
    }

-    if ($result) {
+    if (isset($result)) {


$result = NULL; means it is set - if !empty is probably better here

eileenmcnaughton · 2017-04-28T13:32:22Z

@monishdeb I'm calling it for tonight now - but once you've removed those lines around
isAmountZero & tidied up the commits & pull request name then go ahead & merge -

…lly paid contribution

eileenmcnaughton · 2017-04-29T02:28:16Z

@monishdeb I've taken out those few lines & tidied up the commit message/ squashed the commits & I'm going to merge this. I looked at whether #10126 should be closed but there is a section of cleanup which we have lost - ie. https://github.com/civicrm/civicrm-core/pull/10126/files#diff-bda4a97d68b9f968720ce192d5f960f9L196 & it would be good to get that merged. Also, is the test in that PR merged? I think it may be

civicrm-builder added the master label Apr 28, 2017

monishdeb force-pushed the tidy branch 2 times, most recently from b903358 to 628e12b Compare April 28, 2017 11:29

eileenmcnaughton commented Apr 28, 2017

View reviewed changes

monishdeb force-pushed the tidy branch 2 times, most recently from 94b9f56 to 0a59a75 Compare April 28, 2017 12:39

eileenmcnaughton commented Apr 28, 2017

View reviewed changes

monishdeb force-pushed the tidy branch 2 times, most recently from 315492e to b203609 Compare April 28, 2017 12:46

monishdeb force-pushed the tidy branch from b705927 to 147082b Compare April 28, 2017 14:49

CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partia…

23cb875

…lly paid contribution

eileenmcnaughton force-pushed the tidy branch from 147082b to 23cb875 Compare April 29, 2017 02:24

eileenmcnaughton changed the title ~~[wip] form tidy ups~~ CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partia… Apr 29, 2017

eileenmcnaughton mentioned this pull request Apr 29, 2017

[WIP] CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partially paid contribution #10126

Closed

eileenmcnaughton added the merge on pass label Apr 29, 2017

eileenmcnaughton merged commit c7d488a into civicrm:master Apr 29, 2017

eileenmcnaughton deleted the tidy branch April 29, 2017 03:17


		// Get the required fields value only.
		$params = $submittedValues;

CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partia… #10271

CRM-20392 (IIDA-108), Fixed submit Credit Card transaction for partia… #10271

Conversation

eileenmcnaughton commented Apr 28, 2017

eileenmcnaughton commented Apr 28, 2017

monishdeb commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Apr 28, 2017

eileenmcnaughton commented Apr 29, 2017

monishdeb Apr 28, 2017 •

edited

Loading

monishdeb Apr 28, 2017 •

edited

Loading