Merge pull request #15748 from eileenmcnaughton/line_item_belt_braces

Provide precautionary handling for theoretical error scenario.

CRM/Financial/BAO/Payment.php

@@ -105,10 +105,18 @@ public static function create($params) {
       if ($value['allocation'] === (float) 0) {
         continue;
       }
+
+      if (!empty($ftIds[$value['price_field_value_id']])) {
+        $financialItemID = $ftIds[$value['price_field_value_id']];
+      }
+      else {
+        $financialItemID = self::getNewFinancialItemID($value, $params['trxn_date'], $contribution['contact_id'], $paymentTrxnParams['currency']);
+      }
+
       $eftParams = [
         'entity_table' => 'civicrm_financial_item',
         'financial_trxn_id' => $trxn->id,
-        'entity_id' => $ftIds[$value['price_field_value_id']],
+        'entity_id' => $financialItemID,
         'amount' => $value['allocation'],
       ];
 
@@ -413,6 +421,7 @@ private static function updateContributionStatus(int $contributionID, string $st
    * @param $params
    *
    * @return array
+   * @throws \CiviCRM_API3_Exception
    */
   protected static function getPayableLineItems($params): array {
     $lineItems = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['contribution_id']);
@@ -433,6 +442,8 @@ protected static function getPayableLineItems($params): array {
       $ratio = 0;
     }
     foreach ($lineItems as $lineItemID => $lineItem) {
+      // Ideally id would be set deeper but for now just add in here.
+      $lineItems[$lineItemID]['id'] = $lineItemID;
       $lineItems[$lineItemID]['paid'] = self::getAmountOfLineItemPaid($lineItemID);
       $lineItems[$lineItemID]['balance'] = $lineItem['subTotal'] - $lineItems[$lineItemID]['paid'];
 
@@ -503,4 +514,49 @@ protected static function reverseAllocationsFromPreviousPayment($params, $trxnID
     }
   }
 
+  /**
+   * Create a financial items & return the ID.
+   *
+   * Ideally this will never be called.
+   *
+   * However, I hit a scenario in testing where 'something' had  created a pending payment with
+   * no financial items and that would result in a fatal error without handling here. I failed
+   * to replicate & am not investigating via a new test methodology
+   * https://github.com/civicrm/civicrm-core/pull/15706
+   *
+   * After this is in I will do more digging & once I feel confident new instances are not being
+   * created I will add deprecation notices into this function with a view to removing.
+   *
+   * However, I think we want to add it in 5.20 as there is a risk of users experiencing an error
+   * if there is incorrect data & we need time to ensure that what I hit was not a 'thing.
+   * (it might be the demo site data is a bit flawed & that was the issue).
+   *
+   * @param array $lineItem
+   * @param string $trxn_date
+   * @param int $contactID
+   * @param string $currency
+   *
+   * @return int
+   *
+   * @throws \CiviCRM_API3_Exception
+   */
+  protected static function getNewFinancialItemID($lineItem, $trxn_date, $contactID, $currency): int {
+    $financialAccount = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship(
+      $lineItem['financial_type_id'],
+      'Income Account Is'
+    );
+    $itemParams = [
+      'transaction_date' => $trxn_date,
+      'contact_id' => $contactID,
+      'currency' => $currency,
+      'amount' => $lineItem['line_total'],
+      'description' => $lineItem['label'],
+      'status_id' => 'Unpaid',
+      'financial_account_id' => $financialAccount,
+      'entity_table' => 'civicrm_line_item',
+      'entity_id' => $lineItem['id'],
+    ];
+    return (int) civicrm_api3('FinancialItem', 'create', $itemParams)['id'];
+  }
+
 }

tests/phpunit/api/v3/ContributionTest.php

@@ -2146,6 +2146,8 @@ public function testCheckTaxAmount($thousandSeparator) {
 
   /**
    * Test repeat contribution successfully creates line item.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testRepeatTransaction() {
     $originalContribution = $this->setUpRepeatTransaction($recurParams = [], 'single');

tests/phpunit/api/v3/PaymentTest.php

@@ -309,6 +309,29 @@ public function testCreatePaymentPendingOrderNoLineItems() {
     $this->validateAllPayments();
   }
 
+  /**
+   * Test that Payment.create does not fail if the line items are missing.
+   *
+   * In the original spec it was anticipated that financial items would not be created
+   * for pending contributions in some circumstances. We've backed away from this and
+   * I mostly could not find a way to do it through the UI. But I did seem to once &
+   * I want to be sure that if they ARE missing no fatal occurs so this tests
+   * that in an artificial way.
+   *
+   * @throws \CRM_Core_Exception
+   */
+  public function testAddPaymentMissingFinancialItems() {
+    $contribution = $this->callAPISuccess('Contribution', 'create', [
+      'total_amount' => 50,
+      'financial_type_id' => 'Donation',
+      'contact_id' => $this->individualCreate(),
+      'contribution_status_id' => 'Pending',
+    ]);
+    CRM_Core_DAO::executeQuery('DELETE FROM civicrm_financial_item');
+    $this->callAPISuccess('Payment', 'create', ['contribution_id' => $contribution['id'], 'payment_instrument_id' => 'Check', 'total_amount' => 5]);
+    $this->validateAllPayments();
+  }
+
   /**
    * Add participant with contribution
    *