Prevent financial transactions from being saved with no payment instr… #12502
Conversation
|
(Standard links)
|
| @@ -3400,6 +3400,9 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = | |||
| // change Payment Instrument for a Completed contribution | |||
| // first handle special case when contribution is changed from Pending to Completed status when initial payment | |||
| // instrument is null and now new payment instrument is added along with the payment | |||
| if (!$params['contribution']->payment_instrument_id) { | |||
| $params['contribution']->find(TRUE); | |||
There was a problem hiding this comment.
How about using $params['prevContribution']->payment_instrument_id ?
There was a problem hiding this comment.
@pradpnayak yeah I tossed that up - TBH I was 6 one way half a dozen the other on it - I think in the end I chose this as loading the object could reduce other unpredictabilities - but I could have equally gone with the trivial performance benefit of prevContribution
|
Eileen, I did QA on current state of code. Test case: Before: civicrm_financial_trxn.payment_instrument_id is set to NULL
After: civicrm_financial_trxn.payment_instrument_id is set to |
|
@pradpnayak should that second be AFTER not 2 befores? - Is that an endorsement to merge? |
|
@eileenmcnaughton Sorry it should be after, I have updated my comment. I will do QA one more time with actual db check. Give me some time i will add my results soon. |
|
@pradpnayak related discussion on #12506 too |
|
@eileenmcnaughton i have added my notes on #12506 (comment) |
|
Merging per discussion on #12506 |
Overview
Prevent financial transactions from being saved with no payment instrument
https://lab.civicrm.org/dev/core/issues/264
Before
Create a completed contribution.
Edit contribution by increasing contribution amount and net amount.
This will create a new financial transaction with NO Payment Instrument.
After
Payment instrument saved
Technical Details
This is a very small fix targetted to be included in 5.4 & the 5.3 security drop. I'm not a fan of how we are passing around the contribution in params / handling this but loading when not loaded is
going to prevent the issue
Comments
In the big picture we want to freeze total_amount and leave people with 2 options
https://github.com/JMAConsulting/biz.jmaconsulting.lineitemedit/blob/master/README.md
I think this is not a recent regression but recent changes (the payment edit block) made it more visible.
I also think this fix is safe and we should include it ASAP (