Optimize CRM_Core_BAO_FinancialTrxn::getTotalPayment

[monishdeb]

Overview

This patch optimizes the getContributionBalance() function to use CRM_Core_BAO_FinancialTrxn::getTotalPayments to fetch the total paid amount.

Before

getContributionBalance() uses codes already present in CRM_Core_BAO_FinancialTrxn::getTotalPayments

After

1. Generalize CRM_Core_BAO_FinancialTrxn::getTotalPayments consider refund trxns
2. Cleaned up getContributionBalance()

Comments

This is a followup PR of #13151

ping @eileenmcnaughton @jitendrapurohit @seamuslee001

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

[monishdeb]

And I wonder what CRM_Core_BAO_ActionScheduleTest.testRepetitionFrequencyUnit test fail has to do with this patch :/

[seamuslee001]

Jenkins re test this please

[jitendrapurohit]

Refund amount is not properly calculated after this change. Balance amount still equals the refund amount after the payment is refunded. To replicate -

1. Register an event using amount = $1500.
2. Change selection and use a lower amount eg $1000.
3. Record a refund amount of $500 from the payment form.
4. Balance calculated from "getContributionBalance()" is still = "-500". Hence the link to Record Refund still appears for the contribution.

Payment already refunded -

[monishdeb]

@jitendrapurohit thanks for your review. But this PR is meant only to optimize the getTotalPayment() by reusing the code of getTotalPayments() and it is a followup PR of #13151 (see my comment here). Am I right @eileenmcnaughton ?

I am expecting it shouldn't break any scenario nor should fix any issue for partial and refund. After merging this PR can you bring necessary change in #12319 ?

[monishdeb]

Hi @eileenmcnaughton , did you get a chance to review this PR?

[eileenmcnaughton]

@monishdeb I just checked & the issue @jitendrapurohit raises is in fact a regression - here is how the query winds up

SELECT SUM(ft.total_amount) FROM civicrm_financial_trxn ft
      INNER JOIN civicrm_entity_financial_trxn eft ON (eft.financial_trxn_id = ft.id AND eft.entity_table = 'civicrm_contribution')
      WHERE eft.entity_id = 135 AND ft.is_payment = 1 AND ft.status_id IN ('1, 7') 

Note the apostrophes around '1, 7' - that is invalid, I think you need to use string concatenation not CRM_Core_DAO interpolation here - I guess you can validate as commaSeparatedIntegers for good practice - although it's not immediately obvious how someone could get a user value into that field

[colemanw]

@monishdeb I'm marking this WIP until you get the chance to make the suggested changes.

[monishdeb]

Hi @colemanw @eileenmcnaughton sorry for the delay. I have updated this PR, please have a look now.

[eileenmcnaughton]

the test fail is due to the space here - not sure if we should fix the rule to permit but easy to change here for now

[monishdeb]

Jenkins test this please

[monishdeb]

@eileenmcnaughton can you please merge it?

[eileenmcnaughton]

Why have we split out this parameter? There are no places calling it as yet

[monishdeb]

@eileenmcnaughton this is old followup PR of #13151 where we agreed here to have this function modified to include refunded payments rather than only Completed. The need is to get correct contribution balance that earlier considers only Completed payments and is used in the patch at L4178 at Contribution BAO.

[eileenmcnaughton]

I've run through this in a test & am satisified the result is the same with this change - I'll push my test up separately