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

Make Order api easier to use for default price set #20681

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Make Order api easier to use for default price set #20681

merged 1 commit into from
Jul 5, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 21, 2021 •
edited
Loading

Overview

This changes the order api so that it is not necessary to figure out the details
of the default price set when using it to create memberships.

I was digging into places where line items are not adequately set by the time they reach the inner workings on Contribution.create and they seem to be mostly coming from the unit tests and it's kinda painful to replace them with order.create when we have to calculate so many fields to use it - this reduces the required parameters when relying on the default price set for memberships

Before

image

After

image

Technical Details

Comments

@civibot
Copy link

civibot bot commented Jun 21, 2021

(Standard links)

@civibot civibot bot added the master label Jun 21, 2021
@eileenmcnaughton eileenmcnaughton force-pushed the order_easy branch 6 times, most recently from ee88250 to 2ba1078 Compare June 21, 2021 01:05
This was referenced Jun 21, 2021
Closed
Merged
@eileenmcnaughton
Copy link
Contributor Author

failed tests - looks like some notices

CRM_Contribute_BAO_ContributionTest.testcheckLineItems
CRM_Contribute_BAO_ContributionTest.testAllowUpdateRevenueRecognitionDate
CRM_Contribute_BAO_ContributionTest.testCommaSeparatorAmount
CRM_Contribute_BAO_ContributionTest.testCancelOrderWithPledge
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.testMultiplePaymentForPendingPayLaterContribution
CRM_Contribute_Form_AdditionalPaymentTest.testMultiplePaymentForPendingPayLaterContributionWithOneCreditCardPayment
CRM_Core_BAO_FinancialTrxnTest.testCreateDeferredTrxn
CRM_Core_Payment_AuthorizeNetIPNTest.testIPNPaymentRecurSuccess
CRM_Core_Payment_AuthorizeNetIPNTest.testIPNPaymentRecurSuccessMultiAuthNetProcessor
CRM_Core_Payment_AuthorizeNetIPNTest.testIPNPaymentRecurSuccessSuppliedReceiveDate
CRM_Core_Payment_PayPalIPNTest.testIPNPaymentRecurSuccess
CRM_Core_Payment_PayPalProIPNTest.testIPNPaymentRecurSuccess
CRM_Core_Payment_PayPalProIPNTest.testIPNPaymentCRM13743
CRM_Core_Payment_PayPalProIPNTest.testIPNPaymentExpressNoError
CRM_Core_Payment_PayPalProIPNTest.testIPNPaymentExpressRecurSuccess
api_v3_ContributionTest.testCheckTaxAmount with data set #0
api_v3_ContributionTest.testCheckTaxAmount with data set #1
api_v3_ContributionTest.testRepeatTransactionPassedInFinancialType
api_v3_ContributionTest.testRepeatTransactionUpdatedFinancialType
api_v3_ContributionTest.testRepeatTransactionUpdatedFinancialTypeAndNotEquals
api_v3_ContributionTest.testCompleteTransactionForRecurring
api_v3_ContributionTest.testSendMail
api_v3_OrderTest.testAddOrderForMembership
api_v3_OrderTest.testAddOrderForParticipant
api_v3_OrderTest.testCreateWithChainedPayment
api_v3_OrderTest.testOrderWithMixedTax
api_v3_PaymentTest.testCreatePaymentOnFailedContribution
api_v3_PaymentTest.testCreatePaymentPayLaterPartialPayment
api_v3_PaymentTest.testPaymentWithProcessorWithOddFinancialAccount
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv3"
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv4"

@eileenmcnaughton eileenmcnaughton force-pushed the order_easy branch 2 times, most recently from d074197 to 95d8a7c Compare June 21, 2021 04:00
@eileenmcnaughton
Copy link
Contributor Author

Test Result (10 failures / ±0)
CRM_Contribute_Form_AdditionalPaymentTest.testMembershipStatusAfterCompletingPayLaterContribution
CRM_Member_Form_MembershipTest.testSubmitRecur
CRM_Member_Form_MembershipTest.testSubmitRecurTwoRows
CRM_Member_Form_MembershipTest.testSubmitRecurCompleteInstant
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv3"
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv4"
api_v3_OrderTest.testAddOrderForMembership
api_v3_OrderTest.testAddOrderForParticipant
api_v3_OrderTest.testOrderWithMixedTax
CRM_Contribute_BAO_ContributionTest.testAllowUpdateRevenueRecognitionDate

eileenmcnaughton
eileenmcnaughton commented Jun 21, 2021
View reviewed changes
tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
@@ -820,8 +820,6 @@ public function testcheckLineItems() {
$e->getMessage()
);
}

$this->assertEquals(3, $params['line_items'][0]['line_item'][0]['financial_type_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.

No longer done in the same low-level function (which I intend to remove anyway soon)

@eileenmcnaughton
Copy link
Contributor Author

Test Result (6 failures / -14)
CRM_Contribute_BAO_ContributionTest.testAllowUpdateRevenueRecognitionDate
api_v3_OrderTest.testAddOrderForMembership
api_v3_OrderTest.testAddOrderForParticipant
api_v3_OrderTest.testOrderWithMixedTax
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv3"
Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv4"

@eileenmcnaughton
Copy link
Contributor Author

Test Result (5 failures / +5)api_v3_OrderTest.testAddOrderForParticipantapi_v3_OrderTest.testOrderWithMixedTaxCRM_Contribute_BAO_ContributionTest.testAllowUpdateRevenueRecognitionDateCivi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv3"Civi.Financialacls.LineItemTest.testLineItemApiPermissions with data set "APIv4"

@eileenmcnaughton
Make Order api easier to use for default price set
ca44bb7 
This changes the order api so that it is not necessary to figure out the details
of the default price set when using it to create memberships.
@eileenmcnaughton
Copy link
Contributor Author

@monishdeb this is the one I'm most blocked by

@monishdeb
Copy link
Member

Sorry for the delay.

  • General standards
    • (r-explain) PASS
    • (r-doc) PASS
    • (r-run) PASS (Tested on local, ensured that there is regression/breakage. Tested with the Order.create API on with essential parameters - membership type and amount, verified the financial entries are correct)
  • Developer standards
    • (r-tech) PASS
    • (r-code) PASS (checked with civilint, checked for code breakage)
    • (r-test) PASS

@monishdeb monishdeb merged commit 33dec64 into civicrm:master Jul 5, 2021
@eileenmcnaughton eileenmcnaughton deleted the order_easy branch July 5, 2021 22:25
@eileenmcnaughton
Copy link
Contributor Author

Yay thanks @monishdeb - I really rely on your for these gnarly ones

@kcristiano
Copy link
Member

kcristiano commented Oct 14, 2021 •
edited
Loading

@eileenmcnaughton I am testing 5.43-RC Using Order API I now get

[14-Oct-2021 20:27:56 UTC] PHP Fatal error:  Uncaught TypeError: Return value of CRM_Financial_BAO_Order::getDefaultFinancialTypeID() must be of the type int, null returned in /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/CRM/Financial/BAO/Order.php:185
Stack trace:
#0 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/CRM/Financial/BAO/Order.php(935): CRM_Financial_BAO_Order->getDefaultFinancialTypeID()
#1 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/api/v3/Order.php(106): CRM_Financial_BAO_Order->setLineItem(Array, 0)
#2 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/Civi/API/Provider/MagicFunctionProvider.php(89): civicrm_api3_order_create(Array)
#3 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/Civi/API/Kernel.php(149): Civi\API\Provider\MagicFunctionProvider->invoke(Array)
#4 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/Civi/API/Kernel.php(81): Civi\API\Kernel->runRequest(Array)
#5 /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/api/api.php(132): Civi\API\Kernel->runSafe('Order', in /home/wpcv/public_html/wp-content/plugins/civicrm/civicrm/CRM/Financial/BAO/Order.php on line 185

This should have 'broken' in 5.40 - but I cannot trace what other change could have caused this. Any insight?

@kcristiano
Copy link
Member

I am also testing on other sites just in case this is a single site issue.

@eileenmcnaughton
Copy link
Contributor Author

@kcristiano thanks - please log a gitlab when you figure out more

@kcristiano
Copy link
Member

Just closing the loop - This commit 68a5e5f#diff-0b698309cd442a1ff2983c207f0576b2bdd8d98b2693ae1e0661a417c209de4e Required a change in our API code - I am amazed it did not affect all sites we had upgraded, but not a regression, we just had to pass new params to the order api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @monishdeb @kcristiano