[REF] Rename balanceTrxnParams variable to paymentTrxnParams

[eileenmcnaughton]

Overview

Rename a variable to be less misleading

Before

Variable called $balanceTrxnParams

After

Variable called $paymentTrxnParams

Technical Details

This array needs some fixes but I wanted to rename the parameter first as balanceTrxnParams is misleading.

The parameters are for the financial_trxn entry corresponding to the payment

Comments

[civibot]

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

[eileenmcnaughton]

test this please

[mattwire]

@seamuslee001 @eileenmcnaughton This got merged a little fast! In this case I believe balanceTrxnParams is the correct term as it's referencing a specific accounting principle (Stripe also has balance transactions). There are various functions / variables in CiviCRM referencing balanceTrxn(s) specifically and renaming this one creates inconsistency (and confusion with the "Payment" entity in CiviCRM). Can we revert this pending discussion? @JoeMurray @monishdeb

[seamuslee001]

@mattwire i believe the newer variable name is correct as we are in the payment.create function effectively and there isn't anything in this code that is about balance transactions.

[mattwire]

@seamuslee001 But that array of params is specifically being passed to CRM_Core_BAO_FinancialTrxn::create() which takes $balanceTrxnParams as it's parameter. In CRM_Core_BAO_FinancialTrxn there is a function getBalanceTrxnAmt and most references in core to FinancialTrxn use the term balanceTrxn. I think that balanceTrxn (there are two financialTrxns per payment), but if we did decide to change it we should change it everywhere for consistency.

[seamuslee001]

@mattwire we should be evaluating each thing separately. also just because one function has a variable named y doesn't mean everything that calls that function needs to use the same names. In the context of doing a payment create it feels more obvious straight forward calling them the Payment Transaction Params rather than balance trxn params. In the context of doing payment.create what does balanceTrxnParams even mean hence even Eileen getting confused

[mattwire]

@seamuslee001 Understood, but the civicrm_financial_trxn does not represent a payment but a "transfer of balance" (in this case from external via a payment to CiviCRM). The payment itself is a collection of financial entities that are wrapped by the Payment entity. I really need @JoeMurray to weigh in here as he understands this area better than all of us!

[eileenmcnaughton]

@mattwire the params relate to the row being added to civicrm_financial_trxn that represents a payment.

It is not an accounting entry creating a balance transaction - which would happen in other cases where we are debiting one account & crediting another - you can see 'is_payment' is part of the array of values.

The goal on the Payment api is to get to the point where a specific type of financial trxn that clearly maps to the real world concept of a 'Payment' can be interacted with as it's own entity without people having to go deep into the financials & in this case making it clear that was is happening is we are inserting a payment is desirable

[eileenmcnaughton]

Tangentally CRM_Core_BAO_FinancialTrxn::getBalanceTrxnAmt uses 'balance' in a different way again - in that case it refers to the outstanding balance on the order/contribution

$balanceTrxnParams is otherwise used in BAO_Contribution::recordFinancialAccounts - in 2 places. One is a disused deprecated function. The other is kinda unclear. The implication is it is creating a 'balanceTrxn' for the unpaid balance - ie. putting it against accounts receivable. I'm not sure that is actually the case though :-( It doesn't seem like the thing to say we should be copying to me

[JoeMurray]

Sorry I am late replying. Without looking over the code I would just comment on possible historical reasons for that as a name. When partial payments were first added the only supported use case was paying off the balance of the contribution still owed on an event registration. Now we support any payment on any contribution.

[eileenmcnaughton]

Thanks @JoeMurray that makes sense