CRM-20610: Handle payment instrument change on Payment Edit form #10814

monishdeb · 2017-08-03T12:17:27Z

Overview

Handle financial accounts if payment instrument is changed via payment edit form
Modify Financial.create API to call CRM_Core_BAO_FinancialTrxn::create(...)
Added CRM unit test for PaymentEdit

Screencast

CRM-20610: Replace payment details block with editable payment list on 'Edit Contribution' form

monishdeb · 2017-08-03T12:25:01Z

@eileenmcnaughton can you please check now? I have mentioned the changes in PR description.

monishdeb · 2017-08-03T17:50:00Z

Jenkin test this please

eileenmcnaughton · 2017-08-03T23:43:07Z

CRM/Contribute/BAO/Contribution.php

@@ -4186,11 +4186,13 @@ public static function getPaymentInfo($id, $component, $getTrxnInfo = FALSE, $us
            CRM_Core_Action::mask(array(CRM_Core_Permission::EDIT)),
            array(
              'id' => $resultDAO->id,
+              'contri_id' => $contributionId,


Do we have to use this abbreviation - what is so long about the word contribution that we have to abbreviate it into something hard to read?

eileenmcnaughton · 2017-08-04T01:21:51Z

CRM/Core/BAO/FinancialTrxn.php

+    elseif (!empty($params['entity_id']) && !empty($params['entity_table'])) {
+      $entityFinancialTrxnParams['entity_table'] = $params['entity_table'];
+      $entityFinancialTrxnParams['entity_id'] = $params['entity_id'];
+    }


What about changing this function to this -

/** * Create a financial transaction object. * * @param array $params * Values relating to the transaction.. * * @param array $trxnEntityTable * Deprecated: array of entity_id & entity_table. Put * these in params instead. * * @return CRM_Core_BAO_FinancialTrxn */ public static function create($params, $trxnEntityTable = array()) { $trxn = new CRM_Financial_DBO_FinancialTrxn(); $trxn->copyValues($params); if (!CRM_Utils_Rule::currencyCode($trxn->currency)) { $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency; } $trxn->save(); // save to entity_financial_trxn table $entityFinancialTrxnParams = array_merge(array( 'entity_table' => "civicrm_contribution", 'financial_trxn_id' => $trxn->id, 'amount' => $params['total_amount'], ), $trxnEntityTable, $params); if (empty($entityFinancialTrxnParams['entity_id'])) { $entityFinancialTrxnParams['entity_id'] = $params['contribution_id']; } $entityTrxn = new CRM_Financial_DAO_EntityFinancialTrxn(); $entityTrxn->copyValues($entityFinancialTrxnParams); $entityTrxn->save(); return $trxn; }

I could only find two instances where $trxnEntityTable was passed in

BAO_LineItem

$trxnTable = array( 'entity_id' => $newFinancialItem->id, 'entity_table' => 'civicrm_financial_item', ); $reverseTrxn = CRM_Core_BAO_FinancialTrxn::create($updateFinancialItemInfoValues['financialTrxn'], $trxnTable);

&&
FinancialTrxn::createPremiumTrxn (function looks a bit broken to me but that is by the by)

$trxnEntityTable['entity_table'] = 'civicrm_contribution'; $trxnEntityTable['entity_id'] = $params['contributionId']; CRM_Core_BAO_FinancialTrxn::create($financialtrxn, $trxnEntityTable);

eileenmcnaughton · 2017-08-04T01:29:13Z

api/v3/FinancialTrxn.php

@@ -39,7 +39,10 @@
 * @return array
 */
 function civicrm_api3_financial_trxn_create($params) {
-  return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params);
+  if (empty($params['contribution_id']) && (empty($params['entity_table']) && empty($params['entity_id']))) {


I think this be

civicrm_api3_verify_mandatory($params, NULL, array(array('contribution_id', array('entity_table', 'entity_id')));

verify_mandatory can do OR - I Think I got that right for this OR this AND that

Note the big difference between what you did, functionally, and verify_mandatory is that if id is passed in verify_mandatory passes it through. The BAO function doesn't seem to handle passing in id well. I don't think the change proposed makes it worse though.

eileenmcnaughton · 2017-08-04T01:40:04Z

@monishdeb I'm mostly OK with this - I had some suggestions. I can't do a reviewer's Pr due to the temporary rules which make it harder to collaborate so have posted comments on here.

I'm tempted to add a return after $trxn->save() if (!empty($params['id') but I guess that would be a change & it seems to be currently not broken - perhaps it's worth commenting that code at least to point out that it is weird in that scenario

monishdeb · 2017-08-04T11:24:14Z

api/v3/FinancialTrxn.php

@@ -39,7 +39,11 @@
 * @return array
 */
 function civicrm_api3_financial_trxn_create($params) {
-  return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params);
+  if (empty($params['id']) && empty($params['contribution_id']) && empty($params['entity_id'])) {


@eileenmcnaughton actually civicrm_api3_verify_mandatory(...) perform OR operation between keys but here we need AND. I changed it to this.

I think it can do a combo of AND + OR ie

array('x', 'y') is x +y

array (('x', 'y') is x OR y

array (('x', array('y', 'z') ) is x AND y or Z

Or you can use verify_one_mandatory as a clearing way of doing x or y

still it does x OR y check and we need x AND y here :(

monishdeb · 2017-08-04T11:27:11Z

@eileenmcnaughton I have made the additional fixes you mentioned - 50f8ceb. Also added concatenation fix and added unit-test - 650b79b

Can you please check now?

eileenmcnaughton · 2017-08-04T22:02:58Z

CRM/Core/BAO/FinancialTrxn.php

@@ -50,37 +50,38 @@ public function __construct() {
   * @param array $params
   *   (reference ) an assoc array of name/value pairs.
   *
-   * @param string $trxnEntityTable
-   *   Entity_table.
+   * @param array $ids


It's not actually $ids - it's just a deprecated parameter

($ids is generally deprecated on create too but this doesn't hold ids)

On re-reading, it looks like $ids should just be removed as a parameter as you are altering the places that use it

eileenmcnaughton · 2017-08-04T22:03:47Z

CRM/Core/BAO/FinancialTrxn.php

    $trxn = new CRM_Financial_DAO_FinancialTrxn();
    $trxn->copyValues($params);

    if (!CRM_Utils_Rule::currencyCode($trxn->currency)) {
      $trxn->currency = CRM_Core_Config::singleton()->defaultCurrency;
    }

-    $trxn->save();
+    //per http://wiki.civicrm.org/confluence/display/CRM/Database+layer we are moving away from $ids array
+    $financialTrxnID = CRM_Utils_Array::value('FinancialTrxn', $ids, CRM_Utils_Array::value('id', $params));


We aren't passing in 'id' as a field anywhere so we shouldn't start

ie. just CRM_Utils_Array::value('id', $params)) is fine & we don't need to specially set it - copyValues will do that

eileenmcnaughton · 2017-08-04T22:06:04Z

CRM/Price/BAO/LineItem.php

-          );
-          $reverseTrxn = CRM_Core_BAO_FinancialTrxn::create($updateFinancialItemInfoValues['financialTrxn'], $trxnTable);
+          ));
+          $reverseTrxn = CRM_Core_BAO_FinancialTrxn::create($updateFinancialItemInfoValues['financialTrxn']);


There is an issue here - ie. you are passing in $updateFinancialItemInfoValues['financialTrxn'] but it should be $updateFinancialItemInfoValues

eileenmcnaughton · 2017-08-04T22:09:34Z

CRM/Core/BAO/FinancialTrxn.php

      );
-      $trxnEntityTable['entity_table'] = 'civicrm_contribution';


I think it's better practice to explicitly state the table?

eileenmcnaughton · 2017-08-04T22:10:08Z

CRM/Core/BAO/FinancialTrxn.php

    }

+    // save to entity_financial_trxn table


nitpick - whitespace std should use capital letter & full stop

eileenmcnaughton · 2017-08-04T22:12:39Z

CRM/Core/BAO/FinancialTrxn.php

-    $trxn->save();
+    //per http://wiki.civicrm.org/confluence/display/CRM/Database+layer we are moving away from $ids array
+    $financialTrxnID = CRM_Utils_Array::value('FinancialTrxn', $ids, CRM_Utils_Array::value('id', $params));
+    $trxn->id = $financialTrxnID;


done by copyValues

eileenmcnaughton · 2017-08-04T22:13:43Z

tests/phpunit/CiviTest/CiviUnitTestCase.php

@@ -897,7 +897,6 @@ public function callAPISuccessGetValue($entity, $params, $type = NULL) {
  public function callAPISuccessGetSingle($entity, $params, $checkAgainst = NULL) {
    $params += array(
      'version' => $this->_apiversion,
-      'debug' => 1,


because passing this attribute add xdebug properties in the array, which later impacts checkArrayEquals where it only concern about matching the values of the array, the reason why allowing this attribute mismatches with expected array and leads to test failures.

Hmm - it also improves test information available I think - I know the getAndCheck function handles this parameter

@eileenmcnaughton in fact getAndCheck doesn't pass this parameter debug as in here and for API's result array comparison, this extraneous information are avoided while checking:

public function assertAPIArrayComparison($result, $expected, $valuesToExclude = array(), $prefix = '') { $valuesToExclude = array_merge($valuesToExclude, array('debug', 'xdebug', 'sequential')); ...

monishdeb · 2017-08-05T20:46:27Z

@eileenmcnaughton I have updated the PR can you please check now?

monishdeb · 2017-08-07T07:38:48Z

@eileenmcnaughton is it ok to merge?

eileenmcnaughton · 2017-08-09T13:28:13Z

@monishdeb I think we should still not be removing debug & the verify_mandatory can do what we are after. However, those are non functional & I think merging this into the rc should have some priority so I will merge now & try to pick that up against master

CRM-20610: Handle payment instrument change on Payment Edit form

a9dec23

civicrm-builder added the 4.7.24-rc label Aug 3, 2017

monishdeb mentioned this pull request Aug 3, 2017

CRM-20610: Handle payment instrument change on Payment Edit form #10809

Closed

added unit test and other changes

e3a78cb

monishdeb force-pushed the CRM-20610-3-rc branch from 86c5fb5 to e3a78cb Compare August 3, 2017 12:23

eileenmcnaughton reviewed Aug 3, 2017

View reviewed changes

eileenmcnaughton reviewed Aug 4, 2017

View reviewed changes

eileenmcnaughton mentioned this pull request Aug 4, 2017

CRM-20610 expose payment edit form on the contribution page #10776

Merged

monishdeb force-pushed the CRM-20610-3-rc branch 2 times, most recently from a10a382 to b928d3b Compare August 4, 2017 11:20

monishdeb commented Aug 4, 2017

View reviewed changes

additional fixes

50f8ceb

monishdeb force-pushed the CRM-20610-3-rc branch from b928d3b to 50f8ceb Compare August 4, 2017 11:25

concatenate check_number and trxn_id in contribution

650b79b

monishdeb force-pushed the CRM-20610-3-rc branch from 9dc9fd0 to 650b79b Compare August 4, 2017 12:47

eileenmcnaughton reviewed Aug 4, 2017

View reviewed changes

monishdeb force-pushed the CRM-20610-3-rc branch from 271a7e0 to 7e3c9a6 Compare August 5, 2017 20:42

test build failure fix

3137781

monishdeb force-pushed the CRM-20610-3-rc branch from 7e3c9a6 to 3137781 Compare August 5, 2017 20:47

monishdeb mentioned this pull request Aug 9, 2017

CRM-20990 add contributionStatus to online template #10793

Merged

eileenmcnaughton merged commit 743f8e1 into civicrm:4.7.24-rc Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-20610: Handle payment instrument change on Payment Edit form #10814

CRM-20610: Handle payment instrument change on Payment Edit form #10814

monishdeb commented Aug 3, 2017 •

edited by civicrm-builder

Loading

monishdeb commented Aug 3, 2017

monishdeb commented Aug 3, 2017

eileenmcnaughton Aug 3, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton commented Aug 4, 2017

monishdeb Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

monishdeb Aug 5, 2017

monishdeb commented Aug 4, 2017 •

edited

Loading

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

eileenmcnaughton Aug 4, 2017

monishdeb Aug 5, 2017

eileenmcnaughton Aug 5, 2017

monishdeb Aug 7, 2017 •

edited

Loading

monishdeb commented Aug 5, 2017

monishdeb commented Aug 7, 2017

eileenmcnaughton commented Aug 9, 2017

		);
		$trxnEntityTable['entity_table'] = 'civicrm_contribution';

CRM-20610: Handle payment instrument change on Payment Edit form #10814

CRM-20610: Handle payment instrument change on Payment Edit form #10814

Conversation

monishdeb commented Aug 3, 2017 • edited by civicrm-builder Loading

Overview

Screencast

monishdeb commented Aug 3, 2017

monishdeb commented Aug 3, 2017

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 Aug 4, 2017

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 commented Aug 4, 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

monishdeb Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

monishdeb commented Aug 5, 2017

monishdeb commented Aug 7, 2017

eileenmcnaughton commented Aug 9, 2017

monishdeb commented Aug 3, 2017 •

edited by civicrm-builder

Loading

monishdeb commented Aug 4, 2017 •

edited

Loading

monishdeb Aug 7, 2017 •

edited

Loading