From df33c0cd9bfa8c38aa8710a97f94ebb3da660a1a Mon Sep 17 00:00:00 2001 From: Jon Goldberg Date: Tue, 26 Oct 2021 14:27:44 -0400 Subject: [PATCH] don't allow multiple currencies in a batch --- CRM/Batch/BAO/Batch.php | 11 +---- CRM/Batch/BAO/EntityBatch.php | 53 ++++++++++++++++++++++++ CRM/Financial/Page/AJAX.php | 52 ++++++++++++----------- tests/phpunit/api/v3/EntityBatchTest.php | 47 ++++++++++++++++++++- 4 files changed, 128 insertions(+), 35 deletions(-) diff --git a/CRM/Batch/BAO/Batch.php b/CRM/Batch/BAO/Batch.php index 65b441c90711..fbfa95d5f10c 100644 --- 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 index 025a877b8ec3..f4c1e55a7027 100644 --- 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 && $trxn && $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 index 33e891cb2a57..ac2a002c4cdc 100644 --- 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 index d15953c98949..8250199962cd 100644 --- 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']); + } + }