Refactor entityParams in Order.Create API so it is easier to understand/modify

[mattwire]

Overview

Partial from #18096 - this is a refactor only.

Before

More compact, less easy to understand/modify code.

After

Less compact, clearer code. Allows for easier changes for individual entities (eg. adding participant statuses per #18096).

Technical Details

Comments

@eileenmcnaughton Could you review/merge? I'll rebase #18096 after

[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 I did the same refactor myself and came out with a slightly different outcome (mostly switching on $item['entity_table'] since we know what the entity is from the table & reducing the IFs on $entityParams). However, the only thing of significance / that couldn't equally well be done in a future round is that I think

break 2;

is wrong - as that would take us out of both the switch AND the line item iteration loop - meaning that a contribution line item would block a later membership item being created. I guess we want to check that scenario is tested before making this change

[eileenmcnaughton]

hmm - it might be OK - actually - perhaps - just looking at the test config we have

   $orderID = $this->callAPISuccess('Order', 'create', [
      'total_amount' => 400,
      'financial_type_id' => 'Member Dues',
      'contact_id' => $this->_contactID,
      'is_test' => 0,
      'payment_instrument_id' => 'Check',
      'receive_date' => '2019-07-25 07:34:23',
      'line_items' => [
        [
          'params' => [
            'contact_id' => $this->ids['contact'][0],
            'membership_type_id' => $this->ids['membership_type'][0],
            'source' => 'Payment',
          ],
          'line_item' => [
            [
              'label' => 'General',
              'qty' => 1,
              'unit_price' => 200,
              'line_total' => 200,
              'financial_type_id' => 1,
              'entity_table' => 'civicrm_membership',
              'price_field_id' => $priceFieldID,
              'price_field_value_id' => $generalPriceFieldValueID,
            ],
          ],
        ],
        [
          'params' => [
            'contact_id' => $this->ids['contact'][1],
            'membership_type_id' => $this->ids['membership_type'][0],
            'source' => 'Payment',
          ],
          'line_item' => [
            [
              'label' => 'General',
              'qty' => 1,
              'unit_price' => 200,
              'line_total' => 200,
              'financial_type_id' => 1,
              'entity_table' => 'civicrm_membership',
              'price_field_id' => $priceFieldID,
              'price_field_value_id' => $generalPriceFieldValueID,
            ],
          ],
        ],
      ],
    ])['id'];

it's unclear whether more than one line could be in line item

I guess what would make it clearer would be something like

if ($item['entity_table'] === 'civicrm_participant') {
  // Do the whole participant create in here - don't try to share code with membership bit.
}

if ($item['entity_table'] === 'civicrm_participant') {
  // Do the whole membership create in here - don't try to share code with participant bit.
}

[eileenmcnaughton]

Hmmm no the second break is definitely out of

 foreach ($params['line_items'] as $lineItems) {

• so it should be moderately easy to tweak the setup in CRMTraits_Financial_OrderTrait to have an order where the first line item is purely a contribution item

[eileenmcnaughton]

Just adding needs test because we want to ensure that case were the line items is an array with a contribution line as the first line is covered

[mattwire]

@eileenmcnaughton This now has a test which should improve the test coverage for the Order API - thanks @jitendrapurohit

[eileenmcnaughton]

@mattwire @jitendrapurohit thanks for the test - it doesn't actually cover the problem in this PR - but I have updated it so that it passes without this PR (locking in correct behaviour) but fails with this PR #18785

As I suspected the in my comments earlier - the break 2; IS causing subsequent line items to be ignored if the first is a contribution - however the test, as it is in this PR ONLY has one line item rather than more than one where the first is contribution only - so it's not testing the impact of that early exit.

[jitendrapurohit]

Thank you for the update @eileenmcnaughton 👍

[mattwire]

@eileenmcnaughton I've updated the PR and tests are now passing

[eileenmcnaughton]

yep that works