From 45ae836fb2916b2955e4b45d66a3496ed1222d38 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 18 Jan 2021 08:50:56 +1300 Subject: [PATCH] Fix pledge to not use pass-by-reference We have deprecated this out of BAO classes --- CRM/Pledge/BAO/Pledge.php | 42 +++-------- CRM/Pledge/BAO/PledgePayment.php | 4 +- .../CRM/Pledge/BAO/PledgePaymentTest.php | 19 +++-- tests/phpunit/CRM/Pledge/BAO/PledgeTest.php | 73 ++++++++----------- 4 files changed, 54 insertions(+), 84 deletions(-) diff --git a/CRM/Pledge/BAO/Pledge.php b/CRM/Pledge/BAO/Pledge.php index 98d302a6c166..10436928f9c8 100644 --- a/CRM/Pledge/BAO/Pledge.php +++ b/CRM/Pledge/BAO/Pledge.php @@ -58,16 +58,12 @@ public static function retrieve(&$params, &$defaults) { * @param array $params * Reference array contains the values submitted by the form. * - * - * @return object + * @return CRM_Pledge_DAO_Pledge */ - public static function add(&$params) { - if (!empty($params['id'])) { - CRM_Utils_Hook::pre('edit', 'Pledge', $params['id'], $params); - } - else { - CRM_Utils_Hook::pre('create', 'Pledge', NULL, $params); - } + public static function add(array $params): CRM_Pledge_DAO_Pledge { + + $hook = empty($params['id']) ? 'create' : 'edit'; + CRM_Utils_Hook::pre($hook, 'Pledge', $params['id'] ?? NULL, $params); $pledge = new CRM_Pledge_DAO_Pledge(); @@ -85,12 +81,7 @@ public static function add(&$params) { $result = $pledge->save(); - if (!empty($params['id'])) { - CRM_Utils_Hook::post('edit', 'Pledge', $pledge->id, $pledge); - } - else { - CRM_Utils_Hook::post('create', 'Pledge', $pledge->id, $pledge); - } + CRM_Utils_Hook::post($hook, 'Pledge', $pledge->id, $pledge); return $result; } @@ -121,25 +112,12 @@ public static function &getValues(&$params, &$values, $returnProperties = NULL) * Takes an associative array and creates a pledge object. * * @param array $params - * (reference ) an assoc array of name/value pairs. + * Assoc array of name/value pairs. * - * @return CRM_Pledge_BAO_Pledge + * @return CRM_Pledge_DAO_Pledge + * @throws \CRM_Core_Exception */ - public static function create(&$params) { - // FIXME: a cludgy hack to fix the dates to MySQL format - $dateFields = [ - 'start_date', - 'create_date', - 'acknowledge_date', - 'modified_date', - 'cancel_date', - 'end_date', - ]; - foreach ($dateFields as $df) { - if (isset($params[$df])) { - $params[$df] = CRM_Utils_Date::isoToMysql($params[$df]); - } - } + public static function create($params) { $isRecalculatePledgePayment = self::isPaymentsRequireRecalculation($params); $transaction = new CRM_Core_Transaction(); diff --git a/CRM/Pledge/BAO/PledgePayment.php b/CRM/Pledge/BAO/PledgePayment.php index a728c0b75e63..91930d465e85 100644 --- a/CRM/Pledge/BAO/PledgePayment.php +++ b/CRM/Pledge/BAO/PledgePayment.php @@ -86,7 +86,7 @@ public static function createMultiple(array $params) { $transaction = new CRM_Core_Transaction(); $overdueStatusID = CRM_Core_PseudoConstant::getKey('CRM_Pledge_BAO_PledgePayment', 'status_id', 'Overdue'); $pendingStatusId = CRM_Core_PseudoConstant::getKey('CRM_Pledge_BAO_PledgePayment', 'status_id', 'Pending'); - + $currency = $params['currency'] ?? CRM_Core_Config::singleton()->defaultCurrency; //calculate the scheduled date for every installment $now = date('Ymd') . '000000'; $statues = $prevScheduledDate = []; @@ -110,7 +110,7 @@ public static function createMultiple(array $params) { } if ($params['installment_amount']) { - $params['scheduled_amount'] = $params['installment_amount']; + $params['scheduled_amount'] = round($params['installment_amount'], CRM_Utils_Money::getCurrencyPrecision($currency)); } else { $params['scheduled_amount'] = round(($params['amount'] / $params['installments']), 2); diff --git a/tests/phpunit/CRM/Pledge/BAO/PledgePaymentTest.php b/tests/phpunit/CRM/Pledge/BAO/PledgePaymentTest.php index dd95e96b1d76..811ae2614498 100644 --- a/tests/phpunit/CRM/Pledge/BAO/PledgePaymentTest.php +++ b/tests/phpunit/CRM/Pledge/BAO/PledgePaymentTest.php @@ -422,10 +422,13 @@ public function testCreatePledgePaymentForMultipleInstallments() { * More specifically, in the UI this would be equivalent to creating a $100 * pledge to be paid in 11 installments of $8.33 and one installment of $8.37 * (to compensate the missing $0.04 from round(100/12)*12. - * The API does not allow to do this kind of pledge, because the BAO recalculates - * the 'amount' using original_installment_amount * installment. + * The API does not allow to do this kind of pledge, because the BAO + * recalculates the 'amount' using original_installment_amount * installment. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ - public function testCreatePledgePaymentForMultipleInstallments2() { + public function testCreatePledgePaymentForMultipleInstallments2(): void { $scheduled_date = date('Ymd', mktime(0, 0, 0, date("m"), date("d") + 2, date("y"))); $contact_id = 2; @@ -446,7 +449,7 @@ public function testCreatePledgePaymentForMultipleInstallments2() { 'sequential' => 1, ]; - $pledge = CRM_Pledge_BAO_Pledge::create($params); + $pledge = $this->callAPISuccess('Pledge', 'create', $params); $contributionID = $this->contributionCreate([ 'contact_id' => $contact_id, @@ -462,7 +465,7 @@ public function testCreatePledgePaymentForMultipleInstallments2() { // Fetch the first planned pledge payment/installment $pledgePayments = civicrm_api3('PledgePayment', 'get', [ - 'pledge_id' => $pledge->id, + 'pledge_id' => $pledge['id'], 'sequential' => 1, ]); @@ -492,7 +495,7 @@ public function testCreatePledgePaymentForMultipleInstallments2() { // Fetch the pledge payments again to see if the amounts and statuses // have been updated correctly. $pledgePayments = $this->callAPISuccess('pledge_payment', 'get', [ - 'pledge_id' => $pledge->id, + 'pledge_id' => $pledge['id'], 'sequential' => 1, ]); @@ -511,11 +514,11 @@ public function testCreatePledgePaymentForMultipleInstallments2() { $this->assertEquals(1, $pp['status_id']); } - $this->assertEquals(count($pledgePayments['values']), CRM_Pledge_BAO_Pledge::pledgeHasFinancialTransactions($pledge->id, 2)); + $this->assertEquals(count($pledgePayments['values']), CRM_Pledge_BAO_Pledge::pledgeHasFinancialTransactions($pledge['id'], 2)); // Cleanup civicrm_api3('Pledge', 'delete', [ - 'id' => $pledge->id, + 'id' => $pledge['id'], ]); } diff --git a/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php b/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php index 1111a6a5c915..536f24afadf5 100644 --- a/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php +++ b/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php @@ -20,21 +20,23 @@ class CRM_Pledge_BAO_PledgeTest extends CiviUnitTestCase { /** * Sets up the fixture, for example, opens a network connection. * This method is called before a test is executed. + * + * @throws \CiviCRM_API3_Exception */ protected function setUp() { parent::setUp(); - $this->_contactId = $this->individualCreate(); + $this->ids['Contact'][0] = $this->individualCreate(); $this->_params = [ - 'contact_id' => $this->_contactId, + 'contact_id' => $this->ids['Contact'][0], 'frequency_unit' => 'month', 'original_installment_amount' => 25.00, 'frequency_interval' => 1, 'frequency_day' => 1, 'installments' => 12, 'financial_type_id' => 1, - 'create_date' => '20100513000000', - 'acknowledge_date' => '20100513000000', - 'start_date' => '20100513000000', + 'create_date' => '2010-05-13 00:00:00', + 'acknowledge_date' => '2010-05-13 00:00:00', + 'start_date' => '2010-05-13 00:00:00', 'status_id' => 2, 'currency' => 'USD', 'amount' => 300, @@ -50,65 +52,52 @@ protected function tearDown() { /** * Test for Add/Update Pledge. + * + * @throws \CRM_Core_Exception */ - public function testAdd() { + public function testAdd(): void { //do test for normal add. - $pledge = CRM_Pledge_BAO_Pledge::add($this->_params); - + $pledgeID = $this->callAPISuccess('Pledge', 'create', $this->_params)['id']; + $pledge = new CRM_Pledge_DAO_Pledge(); + $pledge->id = $pledgeID; + $pledge->find(TRUE); + unset($this->_params['status_id']); foreach ($this->_params as $param => $value) { - $this->assertEquals($value, $pledge->$param); + $this->assertEquals($value, $pledge->$param, $param); } } /** * Test Pledge Payment Status with 1 installment * and not passing status id. + * + * @throws \CRM_Core_Exception */ - public function testPledgePaymentStatus() { + public function testPledgePaymentStatus(): void { $scheduledDate = date('Ymd', mktime(0, 0, 0, date("m"), date("d") + 2, date("y"))); $this->_params['installments'] = 1; $this->_params['scheduled_date'] = $scheduledDate; unset($this->_params['status_id']); - $pledge = CRM_Pledge_BAO_Pledge::create($this->_params); - $pledgePayment = CRM_Pledge_BAO_PledgePayment::getPledgePayments($pledge->id); + $pledge = $this->callAPISuccess('Pledge', 'create', $this->_params); + $pledgePayment = CRM_Pledge_BAO_PledgePayment::getPledgePayments($pledge['id']); - $this->assertEquals(count($pledgePayment), 1); + $this->assertCount(1, $pledgePayment); $payment = array_pop($pledgePayment); // Assert that we actually have no pledge Payments - $this->assertEquals(0, CRM_Pledge_BAO_Pledge::pledgeHasFinancialTransactions($pledge->id, array_search('Pending', CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name')))); - $this->assertEquals($payment['status'], 'Pending'); + $this->assertEquals(0, CRM_Pledge_BAO_Pledge::pledgeHasFinancialTransactions($pledge['id'], array_search('Pending', CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name')))); + $this->assertEquals('Pending', $payment['status']); $this->assertEquals($payment['scheduled_date'], date('Y-m-d 00:00:00', strtotime($scheduledDate))); } - /** - * Retrieve a pledge based on a pledge id = 0 - */ - public function testRetrieveZeroPledeID() { - $defaults = []; - $params = ['pledge_id' => 0]; - $pledgeId = CRM_Pledge_BAO_Pledge::retrieve($params, $defaults); - - $this->assertEquals(is_null($pledgeId), 1, "Pledge Id must be greater than 0"); - } - - /** - * Retrieve a payment based on a Null pledge id random string. - */ - public function testRetrieveStringPledgeID() { - $defaults = []; - $params = ['pledge_id' => 'random text']; - $pledgeId = CRM_Pledge_BAO_Pledge::retrieve($params, $defaults); - - $this->assertEquals(is_null($pledgeId), 1, "Pledge Id must be a string"); - } - /** * Test that payment retrieve wrks based on known pledge id. + * + * @throws \CRM_Core_Exception */ - public function testRetrieveKnownPledgeID() { + public function testRetrieveKnownPledgeID(): void { $params = [ - 'contact_id' => $this->_contactId, + 'contact_id' => $this->ids['Contact'][0], 'frequency_unit' => 'month', 'frequency_interval' => 1, 'frequency_day' => 1, @@ -123,14 +112,14 @@ public function testRetrieveKnownPledgeID() { 'amount' => 300, ]; - $pledge = CRM_Pledge_BAO_Pledge::add($params); + $pledge = $this->callAPISuccess('Pledge', 'create', $params); $defaults = []; - $pledgeParams = ['pledge_id' => $pledge->id]; + $pledgeParams = ['pledge_id' => $pledge['id']]; $pledgeId = CRM_Pledge_BAO_Pledge::retrieve($pledgeParams, $defaults); - $this->assertEquals($pledgeId->N, 1, "Pledge was retrieved"); + $this->assertEquals(1, $pledgeId->N, "Pledge was retrieved"); } /**