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

[REF] minor refactor around retrieving processor id for recur #13643

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 20, 2019

Overview

Minor extraction

Before

uses sql

After

uses api

Technical Details

This is primarily so that we can start calling the latter function in places where the id is only retrieved in order to check it exists -

  • next steps are to deprecate & remove calls like

CRM_Financial_BAO_PaymentProcessor::getProcessorForEntity($recurID, 'recur', 'obj')

and

getPaymentProcessorForRecurringContribution (which duplicates this although I think it uses a better method to load the object.)

Note that I think we should load the manual payment processor if the value is actually empty - that makes it editable which I think makes sense where they have been added other than via normal civi mechanisms - this could include imports or just odd requirements

Comments

@mattwire

preliminary towards https://lab.civicrm.org/dev/core/issues/704

@civibot
Copy link

civibot bot commented Feb 20, 2019

(Standard links)

@civibot civibot bot added the master label Feb 20, 2019
@eileenmcnaughton
Copy link
Contributor Author

CRM_Contact_Page_View_UserDashBoardTest::testDashboardContentContributionsWithInvoicingEnabled
Undefined index: payment_processor_id

relates - will check

$mode
);
if (!$paymentProcessor) {
if (empty($recur->payment_processor_id)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit an enotice in the tests & when I looked here I found that we have just done a dao->fetch so we already have the payment_processor_id - looks to me like the check is for something obsolete anyway - see orphan paymentObject later on... - probably this was to get the url but that's not in the function now

* Payment processor id. If none found return 0 which represents the
* pseudo processor used for pay-later.
*/
public static function getPaymentProcessorID($recurID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire
Copy link
Contributor

Incremental cleanup for payment processor stuff which is good.

@eileenmcnaughton I can't actually find where CRM_Contribute_BAO_ContributionRecur::getPaymentProcessor() is being called?

@eileenmcnaughton
Copy link
Contributor Author

@mattwire looks like I removed the one place in this PR :-) But I think this is my preferred of the 3 variants of the function so we should migrate to this one

@mattwire
Copy link
Contributor

I've tested this locally and it works. It's a preliminary refactor with no visible change. ok to merge

@eileenmcnaughton eileenmcnaughton merged commit baa6938 into civicrm:master Feb 25, 2019
@eileenmcnaughton eileenmcnaughton deleted the recur branch February 25, 2019 20:20
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