don't allow multiple currencies in a batch

civicrm · Oct 27, 2021 · 3af7aee · 3af7aee
1 parent 8f0e98f
commit 3af7aee
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 35 deletions.
diff --git a/CRM/Batch/BAO/Batch.php b/CRM/Batch/BAO/Batch.php
@@ -329,16 +329,7 @@ public static function getBatchList(&$params) {
         $values['id']
       );
       // CRM-21205
-      $values['currency'] = CRM_Core_DAO::singleValueQuery("
-        SELECT GROUP_CONCAT(DISTINCT ft.currency)
-        FROM  civicrm_batch batch
-        JOIN civicrm_entity_batch eb
-          ON batch.id = eb.batch_id
-        JOIN civicrm_financial_trxn ft
-          ON eb.entity_id = ft.id
-        WHERE batch.id = %1
-        GROUP BY batch.id
-      ", [1 => [$values['id'], 'Positive']]);
+      $values['currency'] = CRM_Batch_BAO_EntityBatch::getBatchCurrency($values['id']);
       $results[$values['id']] = $values;
     }
 

diff --git a/CRM/Batch/BAO/EntityBatch.php b/CRM/Batch/BAO/EntityBatch.php
@@ -23,6 +23,35 @@ class CRM_Batch_BAO_EntityBatch extends CRM_Batch_DAO_EntityBatch {
    * @return CRM_Batch_DAO_EntityBatch
    */
   public static function create($params) {
+    // Only write the EntityBatch record if the financial trxn and batch match on currency and payment instrument.
+    $batchId = $params['batch_id'] ?? NULL;
+    $entityId = $params['entity_id'] ?? NULL;
+    // Not having a batch ID and entity ID is only acceptable on an update.
+    if (!$batchId) {
+      $existingEntityBatch = \Civi\Api4\EntityBatch::get(FALSE)
+        ->addSelect('id', '=', $params['id'])
+        ->execute()
+        ->first();
+      $batchId = $existingEntityBatch['batch_id'] ?? NULL;
+      $entityId = $existingEntityBatch['entity_id'] ?? NULL;
+    }
+    // There should never be a legitimate case where a record has an ID but no batch ID but SyntaxConformanceTest says otherwise.
+    if ($batchId) {
+      $batchCurrency = self::getBatchCurrency($batchId);
+      $batchPID = (int) CRM_Core_DAO::getFieldValue('CRM_Batch_DAO_Batch', $batchId, 'payment_instrument_id');
+      $trxn = \Civi\Api4\FinancialTrxn::get(FALSE)
+        ->addSelect('currency', 'payment_instrument_id')
+        ->addWhere('id', '=', $entityId)
+        ->execute()
+        ->first();
+      if ($batchCurrency && $batchCurrency !== $trxn['currency']) {
+        throw new \CRM_Core_Exception(ts('You can not add items of two different currencies to a single contribution batch.'));
+      }
+      if ($batchPID && $batchPID !== $trxn['payment_instrument_id']) {
+        $paymentInstrument = CRM_Core_PseudoConstant::getLabel('CRM_Batch_BAO_Batch', 'payment_instrument_id', $batchPID);
+        throw new \CRM_Core_Exception(ts('This batch is configured to include only transactions using %1 payment method. If you want to include other transactions, please edit the batch first and modify the Payment Method.', [1 => $paymentInstrument]));
+      }
+    }
     return self::writeRecord($params);
   }
 
@@ -39,4 +68,28 @@ public static function del($params) {
     return self::deleteRecord($params);
   }
 
+  /**
+   * Get the currency associated with a batch (if any).
+   *
+   * @param int $batchId
+   *
+   */
+  public static function getBatchCurrency($batchId) : ?string {
+    $sql = "SELECT DISTINCT ft.currency
+      FROM  civicrm_batch batch
+      JOIN civicrm_entity_batch eb
+        ON batch.id = eb.batch_id
+      JOIN civicrm_financial_trxn ft
+        ON eb.entity_id = ft.id
+      WHERE batch.id = %1";
+    $dao = CRM_Core_DAO::executeQuery($sql, [1 => [$batchId, 'Positive']]);
+    if ($dao->N === 0) {
+      return NULL;
+    }
+    else {
+      $dao->fetch();
+      return $dao->currency;
+    }
+  }
+
 }
diff --git a/CRM/Financial/Page/AJAX.php b/CRM/Financial/Page/AJAX.php
@@ -170,19 +170,13 @@ public static function assignRemove() {
         switch ($op) {
           case 'assign':
           case 'remove':
-            $recordPID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialTrxn', $recordID, 'payment_instrument_id');
-            $batchPID = CRM_Core_DAO::getFieldValue('CRM_Batch_DAO_Batch', $entityID, 'payment_instrument_id');
-            $paymentInstrument = CRM_Core_PseudoConstant::getLabel('CRM_Batch_BAO_Batch', 'payment_instrument_id', $batchPID);
-            if ($op == 'remove' || ($recordPID == $batchPID && $op == 'assign') || !isset($batchPID)) {
+            if ($op == 'remove' || $op == 'assign') {
               $params = [
                 'entity_id' => $recordID,
                 'entity_table' => 'civicrm_financial_trxn',
                 'batch_id' => $entityID,
               ];
             }
-            else {
-              $response = ['status' => ts("This batch is configured to include only transactions using %1 payment method. If you want to include other transactions, please edit the batch first and modify the Payment Method.", [1 => $paymentInstrument])];
-            }
             break;
 
           case 'close':
@@ -204,7 +198,12 @@ public static function assignRemove() {
         }
 
         if (method_exists($recordBAO, $methods[$op]) & !empty($params)) {
-          $updated = call_user_func_array(array($recordBAO, $methods[$op]), array(&$params));
+          try {
+            $updated = call_user_func_array(array($recordBAO, $methods[$op]), array(&$params));
+          }
+          catch (\CRM_Core_Exception $e) {
+            $errorMessage = $e->getMessage();
+          }
           if ($updated) {
             $redirectStatus = $updated->status_id;
             if ($batchStatus[$updated->status_id] == "Reopened") {
@@ -215,6 +214,9 @@ public static function assignRemove() {
               'status_id' => $redirectStatus,
             ];
           }
+          if ($errorMessage ?? FALSE) {
+            $response = ['status' => $errorMessage];
+          }
         }
       }
     }
@@ -465,33 +467,35 @@ public static function bulkAssignRemove() {
       }
     }
 
-    $batchPID = CRM_Core_DAO::getFieldValue('CRM_Batch_DAO_Batch', $entityID, 'payment_instrument_id');
-    $paymentInstrument = CRM_Core_PseudoConstant::getLabel('CRM_Batch_BAO_Batch', 'payment_instrument_id', $batchPID);
-    foreach ($cIDs as $key => $value) {
-      $recordPID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_FinancialTrxn', $value, 'payment_instrument_id');
-      if ($action == 'Remove' || ($recordPID == $batchPID && $action == 'Assign') || !isset($batchPID)) {
+    foreach ($cIDs as $value) {
+      if ($action == 'Remove' || $action == 'Assign') {
         $params = [
           'entity_id' => $value,
           'entity_table' => 'civicrm_financial_trxn',
           'batch_id' => $entityID,
         ];
-        if ($action == 'Assign') {
-          $updated = CRM_Batch_BAO_EntityBatch::create($params);
-        }
-        else {
-          $delete = \Civi\Api4\EntityBatch::delete(FALSE);
-          foreach ($params as $field => $val) {
-            $delete->addWhere($field, '=', $val);
+        try {
+          if ($action == 'Assign') {
+            CRM_Batch_BAO_EntityBatch::create($params);
+          }
+          else {
+            $delete = \Civi\Api4\EntityBatch::delete(FALSE);
+            foreach ($params as $field => $val) {
+              $delete->addWhere($field, '=', $val);
+            }
+            $delete->execute()->count();
           }
-          $updated = $delete->execute()->count();
+        }
+        catch (\CRM_Core_Exception $e) {
+          $errorMessage = $e->getMessage();
         }
       }
     }
-    if ($updated) {
-      $status = ['status' => 'record-updated-success'];
+    if ($errorMessage ?? FALSE) {
+      $status = ['status' => $errorMessage];
     }
     else {
-      $status = ['status' => ts("This batch is configured to include only transactions using %1 payment method. If you want to include other transactions, please edit the batch first and modify the Payment Method.", [1 => $paymentInstrument])];
+      $status = ['status' => 'record-updated-success'];
     }
     CRM_Utils_JSON::output($status);
   }

diff --git a/tests/phpunit/api/v3/EntityBatchTest.php b/tests/phpunit/api/v3/EntityBatchTest.php
@@ -31,9 +31,16 @@ public function setUp(): void {
 
     $entityParams = ['contact_id' => $this->individualCreate()];
 
+    $contributionId = $this->contributionCreate($entityParams);
+    $financialTrxnId = array_values($this->callAPISuccess('EntityFinancialTrxn', 'get', [
+      'entity_id' => $contributionId,
+      'entity_table' => 'civicrm_contribution',
+      'return' => ['financial_trxn_id'],
+    ])['values'])[0]['financial_trxn_id'];
+
     $this->_entity = 'EntityBatch';
     $this->params = [
-      'entity_id' => $this->contributionCreate($entityParams),
+      'entity_id' => $financialTrxnId,
       'batch_id' => $this->batchCreate(),
       'entity_table' => 'civicrm_financial_trxn',
     ];
@@ -71,4 +78,42 @@ public function testDeleteEntityBatch(): void {
     $this->assertEquals(0, $checkDeleted['count']);
   }
 
+  /**
+   * Ensure that submitting multiple currencies results in an error.
+   * @throws \CRM_Core_Exception
+   */
+  public function testMultipleCurrencies(): void {
+    $params['name'] = $params['title'] = 'MultiCurrencyBatch';
+    $params['status_id'] = 1;
+    $batchId = $this->callAPISuccess('batch', 'create', $params)['id'];
+
+    $contributionId = $this->contributionCreate(['contact_id' => $this->individualCreate()]);
+    $financialTrxnId = array_values($this->callAPISuccess('EntityFinancialTrxn', 'get', [
+      'entity_id' => $contributionId,
+      'entity_table' => 'civicrm_contribution',
+      'return' => ['financial_trxn_id'],
+    ])['values'])[0]['financial_trxn_id'];
+    $firstEntityBatchParams = [
+      'entity_id' => $financialTrxnId,
+      'batch_id' => $batchId,
+      'entity_table' => 'civicrm_financial_trxn',
+    ];
+    $result = $this->callAPISuccess($this->_entity, 'create', $firstEntityBatchParams);
+    $this->assertEquals(1, $result['count']);
+    $secondContributionId = $this->contributionCreate(['contact_id' => $this->individualCreate(), 'currency' => 'CAD']);
+
+    $secondFinancialTrxnId = array_values($this->callAPISuccess('EntityFinancialTrxn', 'get', [
+      'entity_id' => $secondContributionId,
+      'entity_table' => 'civicrm_contribution',
+      'return' => ['financial_trxn_id'],
+    ])['values'])[0]['financial_trxn_id'];
+    $secondEntityBatchParams = [
+      'entity_id' => $secondFinancialTrxnId,
+      'batch_id' => $batchId,
+      'entity_table' => 'civicrm_financial_trxn',
+    ];
+    $result = $this->callAPIFailure($this->_entity, 'create', $secondEntityBatchParams);
+    $this->assertEquals('You can not add items of two different currencies to a single contribution batch.', $result['error_message']);
+  }
+
 }