Allow overriding participant_status_id in Order API

[mattwire]

Overview

Allow overriding/specifying participant_status_id in Order API.

Before

participant_status_id is hardcoded.

After

participant_status_id can be specified in params to order API. Otherwise it will follow existing logic.

Technical Details

Currently used by #17886 to specify "Pending in cart" as initial participant status.

Comments

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

[eileenmcnaughton]

@mattwire can we restrict this down to 'any pending status'

I have a feeling is would be better to have other params like 'is_approved' and have the status calculated in-function. But we can consider that when we develop the apiv4 Order.api

[mattwire]

@eileenmcnaughton I'm not sure it makes sense to restrict the status. If you don't set it it will be one of two pending states and event cart requires another. However, I can see plenty of valid scenarios for creating participants as part of an order in various other states (eg. "On waitlist" or "Awaiting approval"). Or a scenario where you are adding a set of participant registrations, some free and some paid. The free ones you might set directly to "Registered" while the paid ones would be a pending state.

The majority of participant parameters can currently pass through Order API without modification, it is only this one that was hardcoded.

[eileenmcnaughton]

@mattwire so it looks like should accept 'Pending' or 'Waiting' statuses

With regards to the scenario where there are multiple registrations - so you have a use case or is it hypothetical. I'd rather be stricter about Pending & open it up when we have a use case & a unit test so we can be sure the whole Order->create->payment->create flow works than to find it's hard to support later & have to block it later on (like we did with the top level Pending status for Order).

[eileenmcnaughton]

Although I guess attended makes sense - I'd still rather wait until we have a use case before fully opening up because the whole Pending->completed flow is already fraught & I'd want to have the whole flow under tests before we broaden what we support.

[mattwire]

See https://lab.civicrm.org/dev/financial/-/issues/140

[eileenmcnaughton]

@mattwire I guess the question is really how many tests you want to write as part of extending what we support in this api - we need to think about tests covering the whole flow - ie

Order.create + Payment.create

and checking

1. financial items
2. mailing contents
3. transition of related entities.

I'm comfortable that we have test cover for creating orders with a status of 'Pending from Pay later' and 'Pending from incomplete transaction' across the above. I don't think we have test cover for a mix of statuses and I don't really know what expectation we have for the flow where it is 'pending from waitlist'.

I think I feel like we could treat 'Pending in cart' and 'Pending from approval' as synonyms for 'Pending from Incomplete transaction' and to support those 2 I think the only things that need to be done to the test for that are

1. have a dataprovider so that you are adding one or more new statuses, rather than replacing the previous
2. add a Payment.create to the test & check the status afterwards
3. preferably check mailing contents too

If we add in other statuses / a mix of status types I'd want to make sure we clarify what we expect for those scenarios & look at tests covering the financial items as well as the above.

[mattwire]

@eileenmcnaughton Ok, I've restricted to pending - my main concern is it adds quite a bit of code to do this. We don't have pseudoconstant or API4 support for participantstatustype either and I'm well out of budget for looking at that (this came out of funding for the event cart work).

I suggest you review the first commit first as that is a refactor only.
I've added code to check if it's a pending status and throw an api exception if not.

[eileenmcnaughton]

@mattwire testAddOrderForParticipant relates - I'll take a look on Friday if that is sorted

[mattwire]

Hmm, Test are passing locally

[eileenmcnaughton]

I was gonna look at this today but

CRM_Activity_Form_SearchTest.testSearch
CRM_Batch_Form_EntryTest.testProcessMembership with data set #0
CRM_Batch_Form_EntryTest.testProcessContribution with data set #0
CRM_Contact_BAO_QueryTest.testGetSummaryQueryWithFinancialACLDisabled
CRM_Contact_BAO_QueryTest.testGetSummaryQueryWithFinancialACLEnabled
CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributions
CRM_Contribute_BAO_ContributionTest.testCreateAndGetHonorContact
CRM_Contribute_BAO_ContributionTest.testActivityCreate
CRM_Contribute_BAO_ContributionTest.testReplaceContributionTokens
CRM_Contribute_Form_Task_InvoiceTest.testInvoiceForDueDate

[mattwire]

I'm running PHP7.4 locally so have to use a patch for money_format.. which I accidently pushed up here.

[eileenmcnaughton]

Ok - if this is passing tests tomorrow I'll review it & if I'm able to merge it I'll move on to look at your next 'has-test' PR

[eileenmcnaughton]

Still failing - well I guess it's weekend for you. I'll move on to one of my other queues & check in on this on Friday

[mattwire]

@eileenmcnaughton I've separated the first (refactor) bit into #18306. Please review that first.

Can you run the test in this PR locally and confirm if it passes or fails for you? testAddOrderForParticipant

I cannot get it to fail locally but it's failing every time when run on jenkins. The only difference I have is my PHP version at 7.4

[eileenmcnaughton]

Ok I'll try on Friday

[mattwire]

api_v3_OrderTest::testAddOrderForParticipant
Failure in api call for order create:  Creating a participant via the Order API with a non "pending" status is not supported
#0 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/Civi/API/Provider/MagicFunctionProvider.php(89): civicrm_api3_order_create(Array)
#1 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/Civi/API/Kernel.php(150): Civi\API\Provider\MagicFunctionProvider->invoke(Array)
#2 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/Civi/API/Kernel.php(81): Civi\API\Kernel->runRequest(Array)
#3 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/api/api.php(22): Civi\API\Kernel->runSafe('order', 'create', Array)
#4 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php(292): civicrm_api('order', 'create', Array)
#5 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/Civi/Test/Api3TestTrait.php(169): CiviUnitTestCase->civicrm_api('order', 'create', Array)
#6 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/tests/phpunit/api/v3/OrderTest.php(354): CiviUnitTestCase->callAPISuccess('order', 'create', Array)
#7 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestCase.php(1154): api_v3_OrderTest->testAddOrderForParticipant()
#8 /home/jenkins/bknix-dfl/build/core-18096-87pum/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php(219): PHPUnit\Framework\TestCase->runTest()
#9 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestCase.php(842): CiviUnitTestCase->runTest()
#10 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestResult.php(693): PHPUnit\Framework\TestCase->runBare()
#11 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestCase.php(796): PHPUnit\Framework\TestResult->run(Object(api_v3_OrderTest))
#12 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestSuite.php(746): PHPUnit\Framework\TestCase->run(Object(PHPUnit\Framework\TestResult))
#13 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/Framework/TestSuite.php(746): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#14 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/TextUI/TestRunner.php(652): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Framework\TestResult))
#15 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/TextUI/Command.php(206): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framework\TestSuite), Array, true)
#16 phar:///home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar/phpunit/TextUI/Command.php(162): PHPUnit\TextUI\Command->run(Array, true)
#17 /home/jenkins/bknix-dfl/extern/phpunit7/phpunit7.phar(615): PHPUnit\TextUI\Command::main()
#18 {main}
Failed asserting that a integer is empty.

[jitendrapurohit]

@mattwire The error message seems to be added in the same PR https://github.com/civicrm/civicrm-core/pull/18096/files#diff-844812cc11951c93d83a9779b5802b61R97

Pending from approval status is not enabled by default, so the value returned by CRM_Event_BAO_ParticipantStatusType::getIsValidStatusForClass(...) is NULL as it specifically checks for only active participant statuses. Hence failing on jenkins as it has Pending from approval disabled by default.

I think enabling this status on testAddOrderForParticipant() test before making a call to Order Create API should be able to pass the jenkin test too.

[mattwire]

@eileenmcnaughton Tests now passing @jitendrapurohit Thanks for identifying the problem!

[eileenmcnaughton]

@mattwire OK - I'll take a look once we've finished the completeOrder cleanup (my current definition of 'done' is the related open PRs closed & the weird wranglling for the no longer used variables out of the IPN classes so we don't have to figure that out all over again next time)

[eileenmcnaughton]

test this please

[JoeMurray]

@seamuslee001 could you review the spec and scope and design first, then do a code and run review?

[eileenmcnaughton]

@mattwire I'm happy for you to merge this if you squish the commits first

[mattwire]

Thanks @eileenmcnaughton squashed and flagged merge on pass