Skip to content

Commit

Permalink
Fix batch summary to use the api
Browse files Browse the repository at this point in the history
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
  • Loading branch information
eileenmcnaughton committed Jan 25, 2022
1 parent ac90ec6 commit 5a4d6ce
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 39 deletions.
23 changes: 10 additions & 13 deletions CRM/Batch/BAO/Batch.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
58 changes: 36 additions & 22 deletions CRM/Financial/Page/AJAX.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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;
}

}
9 changes: 5 additions & 4 deletions tests/phpunit/CRM/Financial/Page/AjaxBatchSummaryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand Down

0 comments on commit 5a4d6ce

Please sign in to comment.