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.

Sign up for GitHub

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

Jump to bottom

Switch to passing payment_processor_id as input param to completeOrder #18528

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Switch to passing payment_processor_id as input param to completeOrder #18528

merged 1 commit into from
Sep 23, 2020

Conversation

mattwire
Copy link
Contributor

Overview

This stops using $objects['paymentProcessor'] and passes the payment_processor_id via $input instead. Further cleanup is required to remove the call to loadRelatedObjects altogether.

Before

loadRelatedObjects required to set $objects['paymentProcessor']

After

loadRelatedObjects not required to set $objects['paymentProcessor']. $input['payment_processor_id'] contains the payment processor ID that is required.

Technical Details

Comments

@eileenmcnaughton replacement for #18518

@civibot
Copy link

civibot bot commented Sep 19, 2020

(Standard links)

@civibot civibot bot added the master label Sep 19, 2020
@mattwire mattwire mentioned this pull request Sep 19, 2020
Closed
@eileenmcnaughton
Copy link
Contributor

Ok I agree with this - I just need to check a bit more carefully.

@seamuslee001
Copy link
Contributor

Test fails look related

CRM_Contact_BAO_QueryTest.testGetSummaryQueryWithFinancialACLDisabled
CRM_Contact_BAO_QueryTest.testGetSummaryQueryWithFinancialACLEnabled
CRM_Contribute_BAO_ContributionRecurTest.testAutoRenewalWhenOneMemberIsDeceased
CRM_Contribute_BAO_ContributionTest.testAnnualWithMultipleLineItems
CRM_Contribute_Form_Task_StatusTest.testUpdatePendingContributionWithSendingEmail
CRM_Contribute_Form_Task_StatusTest.testUpdatePendingContributionWithoutSendingEmail
CRM_Contribute_Form_AdditionalPaymentTest.testAddPaymentUsingCreditCardForPartiallyPaidContribution
CRM_Contribute_Form_AdditionalPaymentTest.testAddPaymentForPartiallyPaidContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMultiplePaymentForPartiallyPaidContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMultiplePaymentForPartiallyPaidContributionWithOneCreditCardPayment
CRM_Contribute_Form_AdditionalPaymentTest.testAddPaymentUsingCreditCardForPendingPayLaterContribution
CRM_Contribute_Form_AdditionalPaymentTest.testAddPaymentForPendingPayLaterContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMembershipStatusAfterCompletingPayLaterContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMultiplePaymentForPendingPayLaterContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMultiplePaymentForPendingPayLaterContributionWithOneCreditCardPayment
CRM_Contribute_Form_ContributionTest.testPartialPaymentWithCreditCard
CRM_Core_BAO_FinancialTrxnTest.testCreateDeferredTrxn
CRM_Event_BAO_AdditionalPaymentTest.testPaymentWithCustomPaymentInstrument
CRM_Event_BAO_AdditionalPaymentTest.testTransactionInfo
CRM_Event_BAO_ChangeFeeSelectionTest.testCRM19273
CRM_Event_BAO_ChangeFeeSelectionTest.testCRM20611
CRM_Event_BAO_ChangeFeeSelectionTest.testCRM17151
CRM_Event_BAO_ChangeFeeSelectionTest.testRefundWithFeeAmount0
CRM_Event_BAO_ChangeFeeSelectionTest.testPartialPaymentEntries
CRM_Event_BAO_ChangeFeeSelectionTest.testRefundPaymentEntries
CRM_Event_Form_Registration_ConfirmTest.testTaxMultipleParticipant
CRM_Event_Form_ParticipantTest.testPaymentAllocationOnMultiLineItemEvent
CRM_Member_BAO_MembershipTest.testMembershipPaymentForSingleContributionMultipleMembership
CRM_Member_Form_MembershipTest.testSubmitPartialPayment with data set #0
CRM_Member_Form_MembershipTest.testSubmitPartialPayment with data set #1
CRM_Member_Form_MembershipTest.testCreatePendingWithMultipleTerms
api_v3_ContributionTest.testCheckTaxAmount with data set #0
api_v3_ContributionTest.testCheckTaxAmount with data set #1
api_v3_ContributionTest.testCompleteTransactionSetStatusToInProgress with data set #0
api_v3_ContributionTest.testCompleteTransactionSetStatusToInProgress with data set #1
api_v3_ContributionTest.testCompleteTransactionSetStatusToInProgress with data set #2
api_v3_ContributionTest.testCompleteTransactionSetStatusToInProgress with data set "receive_date_includes_time"
api_v3_ContributionTest.testCompleteTransaction
api_v3_ContributionTest.testCompleteTransactionEuro
api_v3_ContributionTest.testBillingAddress
api_v3_ContributionTest.testCompleteTransactionFeeAmount
api_v3_ContributionTest.testRepeatTransactionPassedInFinancialType
api_v3_ContributionTest.testRepeatTransactionPassedInFinancialTypeTwoLineItems
api_v3_ContributionTest.testRepeatTransactionUpdatedFinancialType
api_v3_ContributionTest.testRepeatTransactionUpdatedFinancialTypeAndNotEquals
api_v3_ContributionTest.testCompleteTransactionNetAmountOK
api_v3_ContributionTest.testCompleteTransactionWithReceiptDateSet
api_v3_ContributionTest.testCompleteTransactionWithEmailReceiptInput
api_v3_ContributionTest.testCompleteTransactionForRecurring
api_v3_ContributionTest.testCompleteTransactionWithEmailReceiptInputTrue
api_v3_ContributionTest.testCompleteTransactionWithTestTemplate
api_v3_ContributionTest.testCompleteTransactionContributionPageFromAddress
api_v3_ContributionTest.testCompleteTransactionUpdatePledgePayment
api_v3_ContributionTest.testCompleteTransactionWithParticipantRecord
api_v3_ContributionTest.testCompleteTransactionMembershipPriceSet
api_v3_ContributionTest.testCompleteTransactionMembershipPriceSetTwoTerms
api_v3_ContributionTest.testRepeatTransactionMembershipCreatePendingContribution
api_v3_ContributionTest.testPaymentDontChangeReceiveDate
api_v3_ContributionTest.testPaymentVerifyPaymentInstrumentChange
api_v3_MembershipTest.testMultipleMembershipContribution
api_v3_OrderTest.testCreateWithChainedPayment
api_v3_PaymentTest.testMultiplePaymentsForContribution
api_v3_PaymentTest.testPaymentSendContributionReceipt
api_v3_PaymentTest.testPaymentEmailReceiptFullyPaid
api_v3_PaymentTest.testCreatePaymentNoLineItems
api_v3_PaymentTest.testCreatePaymentLineItems
api_v3_PaymentTest.testCreatePaymentPayLater
api_v3_PaymentTest.testCreatePaymentOnFailedContribution
api_v3_PaymentTest.testCreatePaymentPayLaterPartialPayment
api_v3_PaymentTest.testPaymentCreateTrxnIdAndDates
CRM_Contact_BAO_RelationshipTest.testSingleMembershipForTwoRelationships

eileenmcnaughton
eileenmcnaughton reviewed Sep 20, 2020
View reviewed changes
CRM/Core/Payment/PayPalIPN.php
@@ -353,6 +353,8 @@ public function main() {
return;
}

$input['payment_processor_id'] = $paymentProcessorID;

self::$_paymentProcessor = &$objects['paymentProcessor'];
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can go too & we can stop passing $paymentProcessorID into validateData a few lines earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileenmcnaughton I'm less familiar with the IPN code here - do you want to do a separate PR for just those changes you are suggesting and I'll review?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mattwire ok

Copy link
Contributor

Choose a reason for hiding this comment

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

so far these are the prs to do the follow up on the ipn classes that we left out of scope here

#18540
#18600

I don't just want to dive in & pull out the var until the params are a bit more sanitised

@mattwire
Copy link
Contributor Author

All test failures are triggering Failure in api call for Payment create: Undefined index: payment_processor_id

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Sep 21, 2020 •
edited
Loading

@mattwire I think it's an optional parameter so it could be $paymentProcessorId = $input['payment_processor_id'] ?? NULL;

Because it could be cheque or whatever

@eileenmcnaughton
Copy link
Contributor

@mattwire I guess we should check though that where completetransaction is called in core it does pass in payment_processor_id rather than rely on guess work

@mattwire mattwire force-pushed the inputpaymentprocessorid branch from d279dd9 to dc65872 Compare September 23, 2020 09:59
@mattwire
Copy link
Contributor Author

@eileenmcnaughton Tests now passing.

@eileenmcnaughton
Copy link
Contributor

Yep - that should work! I'll put up PRs to further remove from the IPN classes

@eileenmcnaughton eileenmcnaughton merged commit 6777cd9 into civicrm:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eileenmcnaughton eileenmcnaughton eileenmcnaughton left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mattwire @eileenmcnaughton @seamuslee001