CRM-21187 Fix handling of currency when updating a contribution to be… #10982

eileenmcnaughton · 2017-09-15T01:04:23Z

Overview

Fix handling of currency when updating a contribution to be completed. Currency value is not retained in the civicrm_financial_trxn table.

This is replicable via the api & potentially other flows. The currency id
of NULL is not accepted in financialTrxn BAO without it replacing with a
default, which is not appropriate on an update.

Before

The completetransaction pathway is used by multiple paths to update contributions when a payment comes in. For JIRA purposes I recreated via the api in a test that does the following

create a pending contribution with currency = EURO
call completetransaction to add an incoming payment
the currency on the financial_trxn record is 'USD' rather than EUR

After

The completetransaction pathway is used by multiple paths to update contributions when a payment comes in. For JIRA purposes I recreated via the api in a test that does the following

create a pending contribution with currency = EUR
call completetransaction to add an incoming payment
the currency on the financial_trxn record is EUR

Technical Details

Ensure no default is applied in the BAO when the id is provided. Pass correct currency in when first creating the trxn.

Comments
@pradpnayak @monishdeb I observed a related issue while looking at this - this subroutine should only entered if $params['refund_trxn_id'] or $params[''card_type_id'] or $params['pan_truncation'] is set. Probably it should be 2 separate ifs (ie. nesting inside $updated doesn't add much value )

        if (!$updated) {
          // Looks like we might have a data correction update.
          // This would be a case where a transaction id has been entered but it is incorrect &
          // the person goes back in & fixes it, as opposed to a new transaction.
          // Currently the UI doesn't support multiple refunds against a single transaction & we are only supporting
          // the data fix scenario.
          // CRM-17751.
          if (isset($params['refund_trxn_id'])) {
            $refundIDs = CRM_Core_BAO_FinancialTrxn::getRefundTransactionIDs($params['id']);
            if (!empty($refundIDs['financialTrxnId']) && $refundIDs['trxn_id'] != $params['refund_trxn_id']) {
              civicrm_api3('FinancialTrxn', 'create', array('id' => $refundIDs['financialTrxnId'], 'trxn_id' => $params['refund_trxn_id']));
            }
          }
          $cardType = CRM_Utils_Array::value('card_type_id', $params);
          $panTruncation = CRM_Utils_Array::value('pan_truncation', $params);
          CRM_Core_BAO_FinancialTrxn::updateCreditCardDetails($params['contribution']->id, $panTruncation, $cardType);
        }
      }

yashodha · 2017-09-15T07:01:56Z

@eileenmcnaughton this works!

nganivet · 2017-09-15T06:36:34Z

tests/phpunit/api/v3/ContributionTest.php

+    $mut->checkMailLog(array(
+      'email:::anthony_anderson@civicrm.org',
+      'is_monetary:::1',
+      'amount:::100.00',


Having a hard-coded value here renders the test brittle, would be preferable to have:
'amount:::' . $this->_params['amount'],
Same goes for the other values (email address might be a bit harder to get to). I realize this is not the style of the rest of the test, but worth mentioning IMO.

Yeah maybe - that part of the test is pretty tangental to what I was trying to test but since similar tests had it I kept it there. Once we drop php 5.3 support I think a good cleanup of the test classes would be in order (with traits)

nganivet · 2017-09-15T06:53:23Z

CRM/Contribute/BAO/Contribution.php

@@ -3539,6 +3539,10 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
          // & this can be removed
          return;
        }
+        if (empty($params['trxnParams']['currency'])) {


Should this block also be added above line 3494? It seems there is a potential for early return there without having the currency code set. The above block has the same basic conditions: $context == 'changedStatus' and $currentContributionStatus == 'Completed'.

I think it's good NOT to set it there. The pass-by-reference is an antipattern here IMHO & we should not add extra params. The reason we add params lower down is specifically to pass the correct params to the FinancialTrxn::create function in the same section

nganivet · 2017-09-15T07:12:53Z

CRM/Contribute/BAO/Contribution.php

@@ -3539,6 +3539,10 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
          // & this can be removed


In general. the updateFinancialAccounts function is difficult to understand as:

there are not enough 'business-level' comments in each of these conditions, ie. above line 3481:
// When a pending or partially paid contribution is completed with another payment instrument
// then there is nothing to do as the older contribution should keep the financial accounts it already has.

and the conditions overlap by their similarity, ie.:

it is not immediately clear what is different between lines 3488 and 3510 and 3530,

or why the structure if the test if not more progressive, ie. outer test on $context only, then test on previousContributionStatus only, and finally test on currentContributionStatus. Concrete example: lines 3528 and 3530 could be deleted since we close an outer if ($context == 'changedStatus') test to re-open the same. Then the structure of tests inside would be clearer.

I agree this is a difficult function to understand. There have been a number of improvements in this area over the course of the year - each one with associated unit tests, but there is still a way to go. I do think more people contributing refactorings here would be helpful.

SImply doing a pull request with comments to go into the code is also a good step

eileenmcnaughton · 2017-09-18T01:12:51Z

@monishdeb @colemanw am keen to trade some review to get this & #10935 which relate to a processor I'm doing for CiviDesk merged. (there are a few other people that could review this but I wouldn't be able to merge it myself if they did under our interim rules)

nganivet · 2017-09-18T03:54:49Z

@monishdeb @colemanw @eileenmcnaughton please tell me if I can help in that trade a do a couple reviews for you.

monishdeb · 2017-09-18T04:01:02Z

CRM/Contribute/BAO/Contribution.php

@@ -3539,6 +3539,10 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
          // & this can be removed
          return;
        }
+        if (empty($params['trxnParams']['currency'])) {
+          // This is an update so original currency if none passed in.
+          $params['trxnParams']['currency'] = isset($params['currency']) ? $params['currency'] : $params['prevContribution']->currency;


Or

$params['trxnParams']['currency'] = CRM_Utils_Array::value('currency', $params, $params['prevContribution']->currency);

@monishdeb I shied away from that because I didn't know whether $params['currency'] might be deliberately set to be empty & what the implications would be on altering it - you think I should be braver?

I don't feel any harm in it, as for update we should implicitly use previous currency value. Otherwise it might pick the default currency, set for the system, which will be inappropriate if it doesn't match $params['prevContribution']->currency. What ya say?

monishdeb · 2017-09-18T19:12:22Z

Patch looks ok to me, except for the minor change I've asked.

… completed. This is replicable via the api & potentially other flows. The currency id of NULL is not accepted in financialTrxn BAO without it replacing with a default, which is not appropriate on an update.

eileenmcnaughton · 2017-09-18T21:43:52Z

@monishdeb I've made the change & also added some todos to highlight the fact that passing $params by reference is an anti-pattern since it's not clear what the added parameters are used for outside the function (my belief is nothing). Hopefully we can work towards changing that in the interests of code predictability

monishdeb · 2017-09-19T06:22:34Z

Works fine. Merging

nganivet reviewed Sep 15, 2017

View reviewed changes

monishdeb reviewed Sep 18, 2017

View reviewed changes

CRM-21187 Fix handling of currency when updating a contribution to be…

8a40179

… completed. This is replicable via the api & potentially other flows. The currency id of NULL is not accepted in financialTrxn BAO without it replacing with a default, which is not appropriate on an update.

eileenmcnaughton force-pushed the trxn_currency branch from 664e4ae to 8a40179 Compare September 18, 2017 21:42

monishdeb merged commit 787baa1 into civicrm:master Sep 19, 2017

eileenmcnaughton deleted the trxn_currency branch September 19, 2017 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21187 Fix handling of currency when updating a contribution to be… #10982

CRM-21187 Fix handling of currency when updating a contribution to be… #10982

eileenmcnaughton commented Sep 15, 2017

yashodha commented Sep 15, 2017

nganivet Sep 15, 2017

eileenmcnaughton Sep 15, 2017

nganivet Sep 15, 2017

eileenmcnaughton Sep 15, 2017

nganivet Sep 15, 2017

eileenmcnaughton Sep 15, 2017

eileenmcnaughton commented Sep 18, 2017

nganivet commented Sep 18, 2017

monishdeb Sep 18, 2017 •

edited

Loading

eileenmcnaughton Sep 18, 2017

monishdeb Sep 18, 2017

monishdeb commented Sep 18, 2017

eileenmcnaughton commented Sep 18, 2017 •

edited

Loading

monishdeb commented Sep 19, 2017

		@@ -3539,6 +3539,10 @@ public static function updateFinancialAccounts(&$params, $context = NULL) {
		// & this can be removed

CRM-21187 Fix handling of currency when updating a contribution to be… #10982

CRM-21187 Fix handling of currency when updating a contribution to be… #10982

Conversation

eileenmcnaughton commented Sep 15, 2017

Overview

Before

After

Technical Details

yashodha commented Sep 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Sep 18, 2017

nganivet commented Sep 18, 2017

monishdeb Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Sep 18, 2017

eileenmcnaughton commented Sep 18, 2017 • edited Loading

monishdeb commented Sep 19, 2017

monishdeb Sep 18, 2017 •

edited

Loading

eileenmcnaughton commented Sep 18, 2017 •

edited

Loading