Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decommision getPartialPaymentTrxn function #13718

Merged
merged 1 commit into from
Mar 1, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Readability fix - remove internal function

Before

getPartialPaymentTrxn called in order to do updates

After

Misleading function removed

Technical Details

This function is only called from one place in the code & also from a test.

It is misleading as it implies a 'get' when it actually does an update
and it groups functionality in a way that doesn't make sense from the calling code pov

In this commit the function is removed and the lines are directly copied to the calling
function, as preparation for cleaning up the calling function

Comments

@monishdeb this is reduced from #13690 - just moving code lines - I realise now there is some more scariness in the handling in that fn - test cover has been really good!

This function is only called from one place in the code & also from a test.

It is misleading as it implies a 'get' when it actually does an update
and it groups functionality in a way that doesn't make sense from the calling code pov

In this commit the function is removed and the lines are directly copied to the calling
function, as preparation for cleaning up the calling function
@civibot
Copy link

civibot bot commented Feb 28, 2019

(Standard links)

@civibot civibot bot added the master label Feb 28, 2019
@monishdeb
Copy link
Member

monishdeb commented Mar 1, 2019

Ok agree with this change. Only occurence of CRM_Core_BAO_FinancialTrxn::getPartialPaymentTrxn is only on the Payment class in entrire code-base and is safely shipped inside Payment.create BAO fn. Existing UT is correctly modified and most importantly (pause)

PASSED

Merging now.

@monishdeb monishdeb merged commit 2169e99 into civicrm:master Mar 1, 2019
@eileenmcnaughton eileenmcnaughton deleted the payment_ref branch March 1, 2019 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants