[REF] Fix silly function to do less handling of non-existent scenarios #13563

eileenmcnaughton · 2019-02-11T04:43:01Z

Overview

code cleanup

Before

Function getPaymentInfo calls getPartialPaymentWithType which calls CRM_Contribute_BAO_Contribution::getContributionBalance
Function getPaymentInfo accepts a parameter $returnType and has code handling for it, but no code ever passed this

After

Function getPaymentInfo calls CRM_Contribute_BAO_Contribution::getContributionBalance
parameter $returnType removed from getPartialPaymentWithType

Technical Details

Really I don't think getPartialPaymentWithType has any reason to exist. I kept to this small change however because I think it's pretty easy to review. If anyone wants to tackle fully getting rid of it I'm happy to do that PR or review it

Comments

@mattwire @pradpnayak @monishdeb I'm sure you must have been annoyed by this at some point

testing note
this code has existing test coverage

If you dig down into CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType you find that just calling CRM_Contribute_BAO_Contribution::getContributionBalance is all it actually does here....

civibot · 2019-02-11T04:43:03Z

(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).

monishdeb · 2019-02-11T05:35:05Z

CRM/Core/BAO/FinancialTrxn.php

-        elseif ($paymentVal > 0) {
-          $value['amount_owed'] = $paymentVal;
-        }
-      }


@eileenmcnaughton umm the 'Record Payment' b/o form still uses these amounts:

CRM//Contribute/Form/AdditionalPayment.php:112: if (!empty($paymentInfo['refund_due'])) { CRM//Contribute/Form/AdditionalPayment.php:113: $paymentAmt = $this->_refund = $paymentInfo['refund_due']; CRM//Contribute/Form/AdditionalPayment.php:609: if (!empty($paymentInfo['refund_due'])) { CRM//Contribute/Form/AdditionalPayment.php:610: $this->_refund = $paymentInfo['refund_due'];

to decide if its refund or owed amount.

Won't it cause a regression?

@monishdeb they are calling a different fn

$paymentDetails = CRM_Contribute_BAO_Contribution::getPaymentInfo($this->_id, $this->_component, FALSE, TRUE);

an no I see - it should already be broken since they don't pass 'TRUE' & the default is false

$paymentInfo = CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType($this->_id, $entityType);

Actually, the default is $returnType = TRUE if you look in this line https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/BAO/FinancialTrxn.php#L451

Ug you are right - well I'll drop out that commit then

@monishdeb OK I dropped the param but assume true not false now

eileenmcnaughton · 2019-02-12T19:55:12Z

test this please

eileenmcnaughton · 2019-02-13T04:24:50Z

test this please

eileenmcnaughton · 2019-02-13T20:18:43Z

@monishdeb it passes

eileenmcnaughton · 2019-02-15T18:51:46Z

@monishdeb I think this is OK now?

monishdeb · 2019-02-16T06:17:19Z

CRM/Contribute/BAO/Contribution.php

@@ -4041,7 +4035,8 @@ public static function getPaymentInfo($id, $component, $getTrxnInfo = FALSE, $us
      $total = $total['total_amount'];
    }

-    $paymentBalance = CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType($id, $entity, FALSE, $total);
+    $paymentBalance = CRM_Contribute_BAO_Contribution::getContributionBalance($contributionId, $total);
+


@eileenmcnaughton this will cause an issue with the calculation done below(see L4049, L4057), whereas it is expecting $paymentBalance to be an amount but as per the patch it always returns a array.

@monishdeb - no getContributionBalance returns an amount - when it's called within getPartialPaymentByType they take that amount & create an array for presumably legacy reasons

Oops sorry, my bad didn't saw what function is been called here. Hmm let me do a quick test at UI level, then I'll merge it.

thanks @monishdeb- I think once #13610 is merged & we can clean up the payment form it's going to be easy to remove that getPartialPaymentWithType function altogether.

And now ... all Civi devs repeat after me

"if a function doesn't return the format I want, I will never again just add another parameter to get it to return a different format. I understand a wrapper function is acceptable if I really need to change the format but only if I avoid just butchering something unrelated onto the function" :-)

Hmm rationally, its good in terms of clarity and the definite role each wrapper function has to play rather then bring more parameters to it and to extend its functionality, which is a pain for dev to understand the code (like the export and changeFee fns). Bad in terms of increase in SLOC but thats fine, we have to be more descriptive on how we define a function. It really help us to understand the code or figure out the bug and bring any change without worrying about different scenarios.

SLOC = single line of code? I think drupal has some stuff around that in it's standards (basically goal is readability not brevity)

monishdeb · 2019-02-16T07:32:48Z

Tested refund/ partial/ pending payment for Event's and memberships's payment. Working fine for me (also tried payment API). Merging now

eileenmcnaughton · 2019-02-16T15:12:14Z

thanks @monishdeb

Replace call to silly confusing function with simple function.

abafc4c

If you dig down into CRM_Core_BAO_FinancialTrxn::getPartialPaymentWithType you find that just calling CRM_Contribute_BAO_Contribution::getContributionBalance is all it actually does here....

civibot bot added the master label Feb 11, 2019

monishdeb reviewed Feb 11, 2019

View reviewed changes

eileenmcnaughton force-pushed the fn_fix_payment_by_type branch 2 times, most recently from d23722f to 17d8fac Compare February 11, 2019 21:24

Remove unused variable from getPartialPaymentWithType

2fc731d

eileenmcnaughton force-pushed the fn_fix_payment_by_type branch from 17d8fac to 2fc731d Compare February 12, 2019 02:51

eileenmcnaughton added sig:code maintenance readability testability ok-without-test labels Feb 13, 2019

monishdeb reviewed Feb 16, 2019

View reviewed changes

monishdeb merged commit 4cd3a8e into civicrm:master Feb 16, 2019

eileenmcnaughton deleted the fn_fix_payment_by_type branch February 16, 2019 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REF] Fix silly function to do less handling of non-existent scenarios #13563

[REF] Fix silly function to do less handling of non-existent scenarios #13563

eileenmcnaughton commented Feb 11, 2019 •

edited

Loading

civibot bot commented Feb 11, 2019

monishdeb Feb 11, 2019

eileenmcnaughton Feb 11, 2019

eileenmcnaughton Feb 11, 2019

monishdeb Feb 11, 2019 •

edited

Loading

eileenmcnaughton Feb 11, 2019

eileenmcnaughton Feb 11, 2019

eileenmcnaughton commented Feb 12, 2019

eileenmcnaughton commented Feb 13, 2019

eileenmcnaughton commented Feb 13, 2019

eileenmcnaughton commented Feb 15, 2019

monishdeb Feb 16, 2019

eileenmcnaughton Feb 16, 2019

monishdeb Feb 16, 2019

eileenmcnaughton Feb 16, 2019 •

edited

Loading

monishdeb Feb 16, 2019 •

edited

Loading

eileenmcnaughton Feb 16, 2019

eileenmcnaughton Feb 16, 2019

monishdeb Feb 16, 2019

monishdeb commented Feb 16, 2019

eileenmcnaughton commented Feb 16, 2019

[REF] Fix silly function to do less handling of non-existent scenarios #13563

[REF] Fix silly function to do less handling of non-existent scenarios #13563

Conversation

eileenmcnaughton commented Feb 11, 2019 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 12, 2019

eileenmcnaughton commented Feb 13, 2019

eileenmcnaughton commented Feb 13, 2019

eileenmcnaughton commented Feb 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton Feb 16, 2019 • edited Loading

Choose a reason for hiding this comment

monishdeb Feb 16, 2019 • 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

monishdeb commented Feb 16, 2019

eileenmcnaughton commented Feb 16, 2019

eileenmcnaughton commented Feb 11, 2019 •

edited

Loading

monishdeb Feb 11, 2019 •

edited

Loading

eileenmcnaughton Feb 16, 2019 •

edited

Loading

monishdeb Feb 16, 2019 •

edited

Loading