Fix Payment.create to update financial_item.status_id

When I try to switch to the order->create flow for membership forms it turns out we are leaving the financial_item.status_id as 'unpaid' when adding a payment. No one has noticed because this field is kinda unused - but it needs to work to pass tests
civicrm · Jul 27, 2021 · 683840a · 683840a
1 parent 403b032
commit 683840a
Show file tree

Hide file tree

Showing 7 changed files with 114 additions and 30 deletions.
diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
@@ -5015,7 +5015,7 @@ public static function getSalesTaxFinancialAccounts() {
     $dao = CRM_Core_DAO::executeQuery($query, $queryParams);
     $financialAccount = [];
     while ($dao->fetch()) {
-      $financialAccount[$dao->id] = $dao->id;
+      $financialAccount[$dao->id] = (int) $dao->id;
     }
     return $financialAccount;
   }

diff --git a/CRM/Financial/BAO/Order.php b/CRM/Financial/BAO/Order.php
@@ -11,6 +11,7 @@
 
 use Civi\Api4\LineItem;
 use Civi\Api4\PriceField;
+use Civi\Api4\PriceFieldValue;
 use Civi\Api4\PriceSet;
 
 /**
@@ -230,6 +231,13 @@ public function setDefaultFinancialTypeID(?int $defaultFinancialTypeID): void {
    */
   protected $defaultPriceField;
 
+  /**
+   * Cache of the default price field value ID.
+   *
+   * @var array
+   */
+  protected $defaultPriceFieldValueID;
+
   /**
    * Get parameters for the entities bought as part of this order.
    *
@@ -663,6 +671,23 @@ public function getDefaultPriceFieldID():int {
     return $this->defaultPriceField['id'];
   }
 
+  /**
+   * Get the id of the price field to use when just an amount is provided.
+   *
+   * @throws \API_Exception
+   *
+   * @return int
+   */
+  public function getDefaultPriceFieldValueID():int {
+    if (!$this->defaultPriceFieldValueID) {
+      $this->defaultPriceFieldValueID = PriceFieldValue::get(FALSE)
+        ->addWhere('name', '=', 'contribution_amount')
+        ->addWhere('price_field_id.name', '=', 'contribution_amount')
+        ->execute()->first()['id'];
+    }
+    return $this->defaultPriceFieldValueID;
+  }
+
   /**
    * Get line items.
    *
@@ -1089,6 +1114,7 @@ protected function fillDefaultContributionLine(array &$lineItem): void {
     $defaults = [
       'qty' => 1,
       'price_field_id' => $this->getDefaultPriceFieldID(),
+      'price_field_value_id' => $this->getDefaultPriceFieldValueID(),
       'entity_table' => 'civicrm_contribution',
       'unit_price' => $lineItem['line_total'],
       'label' => ts('Contribution Amount'),

diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php
@@ -15,16 +15,20 @@
  * @copyright CiviCRM LLC https://civicrm.org/licensing
  */
 
+use Civi\Api4\FinancialItem;
+use Civi\Api4\LineItem;
+
 /**
  * This class contains payment related functions.
  */
 class CRM_Financial_BAO_Payment {
 
   /**
-   * Function to process additional payment for partial and refund contributions.
+   * Function to process additional payment for partial and refund
+   * contributions.
    *
-   * This function is called via API payment.create function. All forms that add payments
-   * should use this.
+   * This function is called via API payment.create function. All forms that
+   * add payments should use this.
    *
    * @param array $params
    *   - contribution_id
@@ -35,6 +39,7 @@ class CRM_Financial_BAO_Payment {
    *
    * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
+   * @throws \API_Exception
    */
   public static function create(array $params): CRM_Financial_DAO_FinancialTrxn {
     $contribution = civicrm_api3('Contribution', 'getsingle', ['id' => $params['contribution_id']]);
@@ -96,17 +101,37 @@ public static function create(array $params): CRM_Financial_DAO_FinancialTrxn {
       self::reverseAllocationsFromPreviousPayment($params, $trxn->id);
     }
     else {
-      [$ftIds, $taxItems] = CRM_Contribute_BAO_Contribution::getLastFinancialItemIds($params['contribution_id']);
+      $salesTaxFinancialAccount = CRM_Contribute_BAO_Contribution::getSalesTaxFinancialAccounts();
+      $financialItems = LineItem::get(FALSE)
+        ->addSelect('tax_amount', 'price_field_value_id', 'financial_item.id', 'financial_item.status_id:name', 'financial_item.financial_account_id')
+        ->addJoin(
+          'FinancialItem AS financial_item',
+          'INNER',
+          NULL,
+          ['financial_item.entity_table', '=', '"civicrm_line_item"'],
+          ['financial_item.entity_id', '=', 'id']
+        )
+        ->addOrderBy('financial_item.id', 'DESC')
+        ->addWhere('contribution_id', '=', (int) $params['contribution_id'])->execute();
 
       foreach ($lineItems as $key => $value) {
         if ($value['allocation'] === (float) 0) {
           continue;
         }
-
-        if (!empty($ftIds[$value['price_field_value_id']])) {
-          $financialItemID = $ftIds[$value['price_field_value_id']];
+        $financialItemID = NULL;
+        $currentFinancialItemStatus = NULL;
+        foreach ($financialItems as $item) {
+          // We check against price_field_value_id rather than line item
+          // id because that is what the code did previously - but it's
+          // unclear whether this is for good reason or bad coding.
+          if ($item['price_field_value_id'] === (int) $value['price_field_value_id']
+            && !in_array($item['financial_item.financial_account_id'], $salesTaxFinancialAccount, TRUE)
+          ) {
+            $financialItemID = $item['financial_item.id'];
+            $currentFinancialItemStatus = $item['financial_item.status_id:name'];
+          }
         }
-        else {
+        if (!$financialItemID) {
           $financialItemID = self::getNewFinancialItemID($value, $params['trxn_date'], $contribution['contact_id'], $paymentTrxnParams['currency']);
         }
 
@@ -118,20 +143,31 @@ public static function create(array $params): CRM_Financial_DAO_FinancialTrxn {
         ];
 
         civicrm_api3('EntityFinancialTrxn', 'create', $eftParams);
+        if ($currentFinancialItemStatus && 'Paid' !== $currentFinancialItemStatus) {
+          $newStatus = $value['allocation'] < $value['balance'] ? 'Partially paid' : 'Paid';
+          FinancialItem::update(FALSE)
+            ->addValue('status_id:name', $newStatus)
+            ->addWhere('id', '=', $financialItemID)
+            ->execute();
+        }
 
-        if (array_key_exists($value['price_field_value_id'], $taxItems)) {
-          // @todo - this is expected to be broken - it should be fixed to
-          // a) have the getPayableLineItems add the amount to allocate for tax
-          // b) call EntityFinancialTrxn directly - per above.
-          // - see https://github.com/civicrm/civicrm-core/pull/14763
-          $entityParams = [
-            'contribution_total_amount' => $contribution['total_amount'],
-            'trxn_total_amount' => $params['total_amount'],
-            'trxn_id' => $trxn->id,
-            'line_item_amount' => $taxItems[$value['price_field_value_id']]['amount'],
-          ];
-          $eftParams['entity_id'] = $taxItems[$value['price_field_value_id']]['financial_item_id'];
-          CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams);
+        foreach ($financialItems as $item) {
+          if ($item['price_field_value_id'] === (int) $value['price_field_value_id']
+            && in_array($item['financial_item.financial_account_id'], $salesTaxFinancialAccount, TRUE)
+          ) {
+            // @todo - this is expected to be broken - it should be fixed to
+            // a) have the getPayableLineItems add the amount to allocate for tax
+            // b) call EntityFinancialTrxn directly - per above.
+            // - see https://github.com/civicrm/civicrm-core/pull/14763
+            $entityParams = [
+              'contribution_total_amount' => $contribution['total_amount'],
+              'trxn_total_amount' => $params['total_amount'],
+              'trxn_id' => $trxn->id,
+              'line_item_amount' => $item['tax_amount'],
+            ];
+            $eftParams['entity_id'] = $item['financial_item.id'];
+            CRM_Contribute_BAO_Contribution::createProportionalEntry($entityParams, $eftParams);
+          }
         }
       }
     }

diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
@@ -1147,7 +1147,7 @@ public function testgetLastFinancialItemIds() {
    * to the original payment - ie. .0909 of the $110 is 10 & that * 50 is $4.54 (note the rounding seems wrong as it should be
    * saved un-rounded).
    */
-  public function testCreateProportionalFinancialEntriesViaPaymentCreate() {
+  public function testCreateProportionalFinancialEntriesViaPaymentCreate(): void {
     [$contribution, $financialAccount] = $this->createContributionWithTax([], FALSE);
     $params = [
       'total_amount' => 50,
@@ -1164,7 +1164,7 @@ public function testCreateProportionalFinancialEntriesViaPaymentCreate() {
       'financial_trxn_id' => $financialTrxn['id'],
     ];
     $entityFinancialTrxn = $this->callAPISuccess('EntityFinancialTrxn', 'Get', $eftParams);
-    $this->assertEquals($entityFinancialTrxn['count'], 2, 'Invalid count.');
+    $this->assertEquals(2, $entityFinancialTrxn['count'], 'Invalid count.');
     $testAmount = [4.55, 45.45];
     foreach ($entityFinancialTrxn['values'] as $value) {
       $this->assertEquals(array_pop($testAmount), $value['amount'], 'Invalid amount stored in civicrm_entity_financial_trxn.');

diff --git a/tests/phpunit/CRM/Event/Form/ParticipantTest.php b/tests/phpunit/CRM/Event/Form/ParticipantTest.php
@@ -713,7 +713,7 @@ protected function assertPartialPaymentResult($isQuickConfig, CiviMailUtils $mut
       'contact_id' => $this->getContactID(),
       'amount' => 1550.55,
       'currency' => 'USD',
-      'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Unpaid'),
+      'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Financial_BAO_FinancialItem', 'status_id', 'Partially paid'),
       'entity_table' => 'civicrm_line_item',
       'entity_id' => $lineItem['id'],
       'financial_account_id' => 4,

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
@@ -4412,8 +4412,12 @@ protected function setUpRepeatTransaction(array $recurParams, $flag, array $cont
     elseif ($flag === 'single') {
       $params = array_merge($this->_params, ['contribution_recur_id' => $contributionRecur['id']]);
       $params = array_merge($params, $contributionParams);
-      $params['api.Payment.create'] = ['total_amount' => $params['total_amount']];
       $originalContribution = $this->callAPISuccess('Order', 'create', $params);
+      // Note the saved contribution amount will include tax.
+      $this->callAPISuccess('Payment', 'create', [
+        'contribution_id' => $originalContribution['id'],
+        'total_amount' => $originalContribution['values'][$originalContribution['id']]['total_amount'],
+      ]);
     }
     $originalContribution['contribution_recur_id'] = $contributionRecur['id'];
     $originalContribution['payment_processor_id'] = $paymentProcessorID;

diff --git a/tests/phpunit/api/v3/OrderTest.php b/tests/phpunit/api/v3/OrderTest.php
@@ -10,6 +10,7 @@
  */
 
 use Civi\Api4\Contribution;
+use Civi\Api4\FinancialItem;
 
 /**
  *  Test APIv3 civicrm_contribute_* functions
@@ -198,7 +199,7 @@ public function testAddOrder(): void {
   /**
    * Test create order api for membership
    *
-   * @throws \CRM_Core_Exception
+   * @throws \API_Exception
    */
   public function testAddOrderForMembership(): void {
     $membershipType = $this->membershipTypeCreate();
@@ -241,7 +242,7 @@ public function testAddOrderForMembership(): void {
     $params = [
       'contribution_id' => $order['id'],
     ];
-    $order = $this->callAPISuccess('order', 'get', $params);
+    $order = $this->callAPISuccess('Order', 'get', $params);
     $expectedResult = [
       $order['id'] => [
         'total_amount' => 200,
@@ -269,7 +270,7 @@ public function testAddOrderForMembership(): void {
       ],
     ];
     $p['total_amount'] = 300;
-    $order = $this->callAPISuccess('order', 'create', $p);
+    $order = $this->callAPISuccess('Order', 'create', $p);
     $expectedResult = [
       $order['id'] => [
         'total_amount' => 300,
@@ -280,9 +281,26 @@ public function testAddOrderForMembership(): void {
     $paymentMembership = [
       'contribution_id' => $order['id'],
     ];
-    $order = $this->callAPISuccess('order', 'get', $paymentMembership);
+    $order = $this->callAPISuccess('Order', 'get', $paymentMembership);
     $this->checkPaymentResult($order, $expectedResult);
     $this->callAPISuccessGetCount('MembershipPayment', $paymentMembership, 2);
+    $this->callAPISuccess('Payment', 'create', [
+      'contribution_id' => $order['id'],
+      'payment_instrument_id' => 'Check',
+      'total_amount' => 300,
+    ]);
+    foreach (FinancialItem::get(FALSE)
+      ->addJoin(
+        'LineItem AS line_item',
+        'INNER',
+        NULL,
+        ['entity_table', '=', '"civicrm_line_item"'],
+        ['entity_id', '=', 'line_item.id'],
+        ['line_item.contribution_id', '=', $order['id']]
+      )
+      ->addSelect('status_id')->execute() as $item) {
+      $this->assertEquals('Paid', CRM_Core_PseudoConstant::getName('CRM_Financial_BAO_FinancialItem', 'status_id', $item['status_id']));
+    }
     $this->callAPISuccess('Contribution', 'Delete', [
       'id' => $order['id'],
     ]);