Further work on payment.create consolidation - always handle financials from payment.create

[eileenmcnaughton]

Overview

Code logic cleanup when adding payments via the api

Before

Code less logical

After

Code more logicial. In addition this may have fixed a bug where 'to financial account' would be incorrectly set to 'Accounts Receivable' rather than ' Deposit bank account' (for example) when adding a payment to a Partially Paid contribution via the api - it's not clear there are any real-world cases where this would happen

Technical Details

This fixes a 2-track processing in payment.create whereby financial transactions for most payments were being handled by payment.create
but for payments transitioning from 'Pending' to 'Completed' the contribution bao was handling them (via completetransaction).

I've added a new parameter to allow opting out of recording payments in completetransation (is_post_payment_create). I believe
ideally our eventual position is that this would be the case whenever a payment is added. I exposed the parameter to getfields
in the spec for completetransaction but not contribution.create. It kind of is an internal / transitional param but the
eventual logic is that completetransaction would never handle financials whereas the eventual for contribution.create is more
muddy.

This surfaced what I believe is a bug in the getToFinancialAccount function. This is only called when adding payments
so I believe it should never transfer TO accounts receivable (which it was doing). In practical terms this
would only have been hit (prior to this) when adding a payment to a partially paid transaction

Comments

@monishdeb can you review?

I would also note this permits bypassing of BAO processing of financial accounts handling 'on demand' - I don't want to encourage this to be seen as a 'feature' for developers as there is no 'support' for it - ie. no guarantee that it won't change. For this reason I did not expose it as an api option in the spec. I DID add support for it in completetransaction since I think we are somewhat explicitly moving to a flow where that function does not handle the financial transactions.

A cowboy developer could still pass the 'is_post_payment_create' into Contribution::create. I don't think it's our job to ensure people don't interact with the code in unsupported ways - only to not imply any support or expectations that it will not break for them in the future or cause unintended consequences for them

[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]

This caused a test to fail & on review I think it would be always incorrect when adding a payment - the only way to reach this line of code

[JoeMurray]

There is a CiviContribute Component setting that determines if every contribution starts with an Accounts Receivable entry. If a payment is being created at time the contribution is being created then it will be subsequent to this Pay Later initial transaction. I think this line is in error in not checking the setting before deciding what to record first.

[eileenmcnaughton]

@JoeMurray that's a bit cryptic - what do you think the removed lines did? From my testing payments against 'Pending' or the unused 'In Progress' wound up having a from and to account of Accounts Receivable with this line in there

[pradpnayak]

@eileenmcnaughton this line AFAIK decides to_financial_account_id when a contribution is created or updated.

to_financial_account_id should be

1. AR if contribution status is pending (dont recall about In Progress)
2. Else it should grab from payment processor FA if payment is dont through payment processor
3. Else it should grab from payment instrument if payment is not from payment processor

https://wiki.civicrm.org/confluence/pages/viewpage.action?pageId=390955013#CiviAccountsDataFlow-Onlinecompletedcontribution

[eileenmcnaughton]

@pradpnayak it's only called from functions that create payments so scenario 1 should be void - which is what is removed

[pradpnayak]

@eileenmcnaughton than it should be If payment-processor else payment-instrument

[eileenmcnaughton]

@pradpnayak it does an early return if (!empty($params['payment_processor'])) { so it doesn't need the 'else'

[eileenmcnaughton]

test this please

[eileenmcnaughton]

Note that @kcristiano did some testing of payment.create without this patch on #14408 & we determined that the last payment added has problems without this patch - see

#14408 (comment)

[kcristiano]

@eileenmcnaughton I just did a new round with just this PR. Partial Payments record correctly.

mysql sa@localhost:wpcv34civi_39tmn> select e.*,f.id,f.from_financial_account_id,f.to_financial_account_id,f.total_amount,f.is_payment,f.status_id,f.payment_processor_id,f.payment_instrument_id from civicrm_contribution c join
                                      civicrm_entity_financial_trxn e on e.entity_id = c.id  join civicrm_financial_trxn f on f.id = e.financial_trxn_id  where c.id =95;
+-----+----------------------+-----------+-------------------+---------+----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+
| id  | entity_table         | entity_id | financial_trxn_id | amount  | id | from_financial_account_id | to_financial_account_id | total_amount | is_payment | status_id | payment_processor_id | payment_instrument_id |
+-----+----------------------+-----------+-------------------+---------+----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+
| 187 | civicrm_contribution | 95        | 94                | 1500.00 | 94 | <null>                    | 7                       | 1500.00      | 0          | 1         | <null>               | 4                     |
| 188 | civicrm_contribution | 95        | 95                | 250.00  | 95 | 7                         | 6                       | 250.00       | 1          | 1         | <null>               | 4                     |
| 191 | civicrm_contribution | 95        | 96                | 1000.00 | 96 | 7                         | 6                       | 1000.00      | 1          | 1         | <null>               | 4                     |
| 193 | civicrm_contribution | 95        | 97                | 250.00  | 97 | 7                         | 6                       | 250.00       | 1          | 1         | <null>               | 4                     |
+-----+----------------------+-----------+-------------------+---------+----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+

Would be great to get more testing, but this looks good.

[JoeMurray]

Just a note that changing the contribution status requires making financial transactions to record that. I assume that this change is not making it so that no longer happens.

[eileenmcnaughton]

no the change is making it so that the payment.create action records financial transactions regardless of whether the payment completes the contribution or not - it was leaving that to the later Contribution.create only when the payment completed the transaction

[eileenmcnaughton]

@monishdeb are you able to review / merge this?

[eileenmcnaughton]

@pradpnayak I think @monishdeb is overloaded - any chance you can review this? It's the current blocker on cleaning up the Payment.create function & switching the AdditionalPayment form (& other places that create payments) to use the api instead of a 'special sauce'

[eileenmcnaughton]

I've given this has-test as we build up test cover in advance of this

[pradpnayak]

I am not confident about the change in getToFinancialAccount(). As its against the DataFlow. But i will still give it a run today.

[pradpnayak]

Hi @eileenmcnaughton

I did a test on Partial payment, pending refund for Donation, Membership and event payment. All looks good to me. The entries are created with correct records in financial tables.

For change fee selection its broken with and without patch so i didn't do further testing on those use cases and also on Line item editor extension.

This changes look good to me and happy to merge unless @JoeMurray or @monishdeb has any other concerns

[eileenmcnaughton]

Thanks @pradpnayak !!