dev/core#88 Make sure financial_type_id is set when contribution is created

[mattwire]

Overview

When a contribution is created via a contribution page (with a confirmation page) the parameter financial_type_id is not set if the contribution is not being created with a recurring contribution.

https://lab.civicrm.org/dev/core/issues/88

Before

financial_type_id is not set when a non-recurring contribution is submitted via a contribution page (with a confirmation page). But it is set when a recurring contribution is being created at the same time.

After

financial_type_id is always set for contribution params when submitted via a contribution page (with a confirmation page).
This makes it more consistent for use in payment processor extensions etc.

[eileenmcnaughton]

@mattwire so we are winding up with contributions with financial_type_id is null? Is that a regression?

@monishdeb ping

[mattwire]

@mattwire so we are winding up with contributions with financial_type_id is null? Is that a regression?

I think it depends on if the payment processor wants to do some extra work to identify what the financial_type_id is meant to be, so we don't necessarily end up with no financial_type. I found this while testing smartdebit with non-recurring payments.

[eileenmcnaughton]

Hmm - can you explain further? Payment processor should set the payment_instrument but that is separate

[monishdeb]

This is a bit strange as $paymentParams is not a parameter nor initialised inside the function yet this is in existence, seems like something to do with past refactoring. Also we should change the variable name to financialTypeId from contributionTypeId as what we all know and in past why that column was renamed :)

so changes here would be:

public function wrangleFinancialTypeID($financialTypeID) {
 if (empty($financialTypeID) && !empty($this->_values['pledge_id'])) {
   $financialTypeID = CRM_Core_DAO::getFieldValue('CRM_Pledge_DAO_Pledge',
         $this->_values['pledge_id'],
         'financial_type_id'
       );
}
 return $financialTypeID;
}

[mattwire]

@monishdeb Thanks for the review. I've updated that function to match what you suggested as that's much cleaner.

[monishdeb]

Jenkins test this please

[monishdeb]

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass Tested on local, works fine, doesn't cause any intended regression
• (r-user) Pass
• (r-tech) Pass

[eileenmcnaughton]

@monishdeb @mattwire I closed the gitlab issue - as a matter of process everyone (submitted, merger, review etc) should feel it's incumbent on them to close the issue since 2/3 will probably forget :-)

[monishdeb]

Thanks @eileenmcnaughton My bad that I forgot to close the ticket :(