From 5a4d6ce37a6a23d601fa6094b125d81bad9ce60f Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Tue, 25 Jan 2022 20:39:24 +1300 Subject: [PATCH] Fix batch summary to use the api In looking at @colemanw's work to clean up the retrieve function it turned out that in at least one place NULL is being passed to Batch::retrieve Since I dug into that 1 place I fixed it to use v4 api (it is the screen accessed after creating a new accounting batch from contribution->accounting batches->new batch) However, it seems safer to continue to accept NULL --- CRM/Batch/BAO/Batch.php | 23 ++++---- CRM/Financial/Page/AJAX.php | 58 ++++++++++++------- .../Financial/Page/AjaxBatchSummaryTest.php | 9 +-- 3 files changed, 51 insertions(+), 39 deletions(-) diff --git a/CRM/Batch/BAO/Batch.php b/CRM/Batch/BAO/Batch.php index b64826da5bcd..c7f47a3d8956 100644 --- a/CRM/Batch/BAO/Batch.php +++ b/CRM/Batch/BAO/Batch.php @@ -50,24 +50,21 @@ public static function create(&$params) { } /** - * Retrieve the information about the batch. + * Retrieve DB object and copy to defaults array. * * @param array $params - * (reference ) an assoc array of name/value pairs. + * Array of criteria values. * @param array $defaults - * (reference ) an assoc array to hold the flattened values. + * Array to be populated with found values. * - * @return CRM_Batch_BAO_Batch|null - * CRM_Batch_BAO_Batch object on success, null otherwise + * @return self|null + * The DAO object, if found. + * + * @deprecated */ - public static function retrieve(&$params, &$defaults) { - $batch = new CRM_Batch_DAO_Batch(); - $batch->copyValues($params); - if ($batch->find(TRUE)) { - CRM_Core_DAO::storeValues($batch, $defaults); - return $batch; - } - return NULL; + public static function retrieve(array $params, ?array &$defaults = NULL) { + $defaults = $defaults ?? []; + return self::commonRetrieve(self::class, $params, $defaults); } /** diff --git a/CRM/Financial/Page/AJAX.php b/CRM/Financial/Page/AJAX.php index ac2a002c4cdc..1218a53bf4d0 100644 --- a/CRM/Financial/Page/AJAX.php +++ b/CRM/Financial/Page/AJAX.php @@ -15,6 +15,8 @@ * @copyright CiviCRM LLC https://civicrm.org/licensing */ +use Civi\Api4\Batch; + /** * This class contains all the function that are called using AJAX */ @@ -500,39 +502,51 @@ public static function bulkAssignRemove() { CRM_Utils_JSON::output($status); } - public static function getBatchSummary() { - $batchID = CRM_Utils_Type::escape($_REQUEST['batchID'], 'String'); - $params = ['id' => $batchID]; - - $batchSummary = self::makeBatchSummary($batchID, $params); - - CRM_Utils_JSON::output($batchSummary); + /** + * @throws \API_Exception + * @throws \CRM_Core_Exception + */ + public static function getBatchSummary(): void { + CRM_Utils_JSON::output(self::makeBatchSummary(CRM_Utils_Type::escape($_REQUEST['batchID'], 'Integer'))); } /** - * Makes an array of the batch's summary and returns array to parent getBatchSummary() function. + * Get a summary of the batch.. * * @param $batchID - * @param $params * * @return array + * @throws \API_Exception + * @throws \CRM_Core_Exception */ - public static function makeBatchSummary($batchID, $params) { - $batchInfo = CRM_Batch_BAO_Batch::retrieve($params, $value); + public static function makeBatchSummary(int $batchID): array { + // We use permissions false because the old function did that & we + // have not tested to ensure permissions are correct - but ideally + // we would setCheckPermissions = TRUE. + $batchInfo = Batch::get(FALSE) + ->addWhere('id', '=', $batchID) + ->addSelect( + 'description', + 'item_count', + 'total', + 'created_date', + 'created_id.display_name', + 'status_id:label', + 'payment_instrument_id:label' + ) + ->execute()->first(); $batchTotals = CRM_Batch_BAO_Batch::batchTotals([$batchID]); - $batchSummary = [ - 'created_by' => CRM_Contact_BAO_Contact::displayName($batchInfo->created_id), - 'status' => CRM_Core_PseudoConstant::getLabel('CRM_Batch_BAO_Batch', 'status_id', $batchInfo->status_id), - 'description' => $batchInfo->description, - 'payment_instrument' => CRM_Core_PseudoConstant::getLabel('CRM_Batch_BAO_Batch', 'payment_instrument_id', $batchInfo->payment_instrument_id), - 'item_count' => $batchInfo->item_count, + return [ + 'created_by' => $batchInfo['created_id.display_name'], + 'status' => $batchInfo['status_id:label'], + 'description' => $batchInfo['description'], + 'payment_instrument' => $batchInfo['payment_instrument_id:label'], + 'item_count' => $batchInfo['item_count'], 'assigned_item_count' => $batchTotals[$batchID]['item_count'], - 'total' => CRM_Utils_Money::format($batchInfo->total), - 'assigned_total' => CRM_Utils_Money::format($batchTotals[$batchID]['total']), - 'opened_date' => CRM_Utils_Date::customFormat($batchInfo->created_date), + 'total' => Civi::format()->money($batchInfo['total']), + 'assigned_total' => Civi::format()->money($batchTotals[$batchID]['total']), + 'opened_date' => CRM_Utils_Date::customFormat($batchInfo['created_date']), ]; - - return $batchSummary; } } diff --git a/tests/phpunit/CRM/Financial/Page/AjaxBatchSummaryTest.php b/tests/phpunit/CRM/Financial/Page/AjaxBatchSummaryTest.php index 1f32dafb90a0..992db7706c9b 100644 --- a/tests/phpunit/CRM/Financial/Page/AjaxBatchSummaryTest.php +++ b/tests/phpunit/CRM/Financial/Page/AjaxBatchSummaryTest.php @@ -20,13 +20,14 @@ class CRM_Financial_Page_AjaxBatchSummaryTest extends CiviUnitTestCase { * * We want to ensure changing the method of obtaining status and payment_instrument * does not cause any regression. + * + * @throws \API_Exception + * @throws \CRM_Core_Exception */ - public function testMakeBatchSummary() { + public function testMakeBatchSummary(): void { $batch = $this->callAPISuccess('Batch', 'create', ['title' => 'test', 'status_id' => 'Open', 'payment_instrument_id' => 'Cash']); - $batchID = $batch['id']; - $params = ['id' => $batchID]; - $makeBatchSummary = CRM_Financial_Page_AJAX::makeBatchSummary($batchID, $params); + $makeBatchSummary = CRM_Financial_Page_AJAX::makeBatchSummary($batch['id']); $this->assertEquals('Open', $makeBatchSummary['status']); $this->assertEquals('Cash', $makeBatchSummary['payment_instrument']);