Merge pull request #24590 from eileenmcnaughton/cont

Remove another call to deprecated `CRM_Contribute_PseudoConstant::contributionStatus`
civicrm · Nov 29, 2022 · fadcb48 · fadcb48
2 parents f936e84 + c185010
commit fadcb48
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 49 deletions.
diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
@@ -3367,25 +3367,18 @@ public static function isContributionStatusNegative($status_id) {
   /**
    * Check status validation on update of a contribution.
    *
-   * @param array $values
+   * @param array $oldContributionValues
    *   Previous form values before submit.
    *
-   * @param array $fields
+   * @param array $newContributionValues
    *   The input form values.
    *
-   * @param array $errors
-   *   List of errors.
-   *
-   * @return bool
+   * @throws \CRM_Core_Exception
    */
-  public static function checkStatusValidation($values, &$fields, &$errors) {
-    if (CRM_Utils_System::isNull($values) && !empty($fields['id'])) {
-      $values['contribution_status_id'] = CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $fields['id'], 'contribution_status_id');
-      if ($values['contribution_status_id'] == $fields['contribution_status_id']) {
-        return FALSE;
-      }
-    }
-    $contributionStatuses = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name');
+  public static function checkStatusValidation(array $oldContributionValues, array $newContributionValues): void {
+    $newContributionStatus = CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $newContributionValues['contribution_status_id']);
+    $oldContributionStatus = CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $oldContributionValues['contribution_status_id']);
+
     $checkStatus = [
       'Cancelled' => ['Completed', 'Refunded'],
       'Completed' => ['Cancelled', 'Refunded', 'Chargeback'],
@@ -3396,14 +3389,13 @@ public static function checkStatusValidation($values, &$fields, &$errors) {
       'Pending refund' => ['Completed', 'Refunded'],
       'Failed' => ['Pending'],
     ];
+    $validNewStatues = $checkStatus[$oldContributionStatus] ?? [];
 
-    if (!in_array($contributionStatuses[$fields['contribution_status_id']],
-      CRM_Utils_Array::value($contributionStatuses[$values['contribution_status_id']], $checkStatus, []))
-    ) {
-      $errors['contribution_status_id'] = ts("Cannot change contribution status from %1 to %2.", [
-        1 => $contributionStatuses[$values['contribution_status_id']],
-        2 => $contributionStatuses[$fields['contribution_status_id']],
-      ]);
+    if (!in_array($newContributionStatus, $validNewStatues, TRUE)) {
+      throw new CRM_Core_Exception(ts('Cannot change contribution status from %1 to %2.', [
+        1 => $oldContributionStatus,
+        2 => $newContributionStatus,
+      ]));
     }
   }
 

diff --git a/CRM/Contribute/Form/Contribution.php b/CRM/Contribute/Form/Contribution.php
@@ -901,7 +901,12 @@ public static function formRule($fields, $files, $self) {
       && $self->_values['contribution_status_id'] != $fields['contribution_status_id']
       && $self->_values['is_template'] != 1
     ) {
-      CRM_Contribute_BAO_Contribution::checkStatusValidation($self->_values, $fields, $errors);
+      try {
+        CRM_Contribute_BAO_Contribution::checkStatusValidation($self->_values, $fields);
+      }
+      catch (CRM_Core_Exception $e) {
+        $errors['contribution_status_id'] = $e->getMessage();
+      }
     }
     // CRM-16015, add form-rule to restrict change of financial type if using price field of different financial type
     if (($self->_action & CRM_Core_Action::UPDATE)

diff --git a/api/v3/Contribution.php b/api/v3/Contribution.php
@@ -54,13 +54,12 @@ function civicrm_api3_contribution_create($params) {
     }
   }
   if (!empty($params['id']) && !empty($params['contribution_status_id'])) {
-    $error = [];
     //throw error for invalid status change such as setting completed back to pending
     //@todo this sort of validation belongs in the BAO not the API - if it is not an OK
     // action it needs to be blocked there. If it is Ok through a form it needs to be OK through the api
-    CRM_Contribute_BAO_Contribution::checkStatusValidation(NULL, $params, $error);
-    if (array_key_exists('contribution_status_id', $error)) {
-      throw new CRM_Core_Exception($error['contribution_status_id']);
+    $values = ['contribution_status_id' => (int) CRM_Core_DAO::getFieldValue('CRM_Contribute_DAO_Contribution', $params['id'], 'contribution_status_id')];
+    if ($values['contribution_status_id'] !== (int) $params['contribution_status_id']) {
+      CRM_Contribute_BAO_Contribution::checkStatusValidation($values, $params);
     }
   }
   if (!empty($params['id']) && !empty($params['financial_type_id'])) {

diff --git a/tests/phpunit/api/v3/ContributionTest.php b/tests/phpunit/api/v3/ContributionTest.php
@@ -1365,7 +1365,7 @@ public function testCreateUpdateContributionChangeTotal() {
    * Function tests that line items, financial records are updated when pay later contribution is received.
    */
   public function testCreateUpdateContributionPayLater() {
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'receive_date' => '2012-01-01',
       'total_amount' => 100.00,
@@ -1375,9 +1375,9 @@ public function testCreateUpdateContributionPayLater() {
       'is_pay_later' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
 
-    $newParams = array_merge($contribParams, [
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'contribution_status_id' => 1,
     ]);
@@ -1393,17 +1393,17 @@ public function testCreateUpdateContributionPayLater() {
    */
   public function testCreateUpdateContributionPaymentInstrument(): void {
     $instrumentId = $this->_addPaymentInstrument();
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'total_amount' => 100.00,
       'financial_type_id' => $this->_financialTypeId,
       'payment_instrument_id' => 4,
       'contribution_status_id' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
 
-    $newParams = array_merge($contribParams, [
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'payment_instrument_id' => $instrumentId,
     ]);
@@ -1420,17 +1420,17 @@ public function testCreateUpdateContributionPaymentInstrument(): void {
    */
   public function testCreateUpdateNegativeContributionPaymentInstrument() {
     $instrumentId = $this->_addPaymentInstrument();
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'total_amount' => -100.00,
       'financial_type_id' => $this->_financialTypeId,
       'payment_instrument_id' => 4,
       'contribution_status_id' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
 
-    $newParams = array_merge($contribParams, [
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'payment_instrument_id' => $instrumentId,
     ]);
@@ -1686,17 +1686,17 @@ public function testCreateUpdateContributionRefundRefundNullTrxnIDPassedIn() {
   /**
    * Function tests invalid contribution status change.
    */
-  public function testCreateUpdateContributionInValidStatusChange() {
-    $contribParams = [
+  public function testCreateUpdateContributionInValidStatusChange(): void {
+    $contributionParams = [
       'contact_id' => 1,
       'receive_date' => '2012-01-01',
       'total_amount' => 100.00,
       'financial_type_id' => 1,
       'payment_instrument_id' => 1,
       'contribution_status_id' => 1,
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
-    $newParams = array_merge($contribParams, [
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'contribution_status_id' => 2,
     ]);
@@ -1710,7 +1710,7 @@ public function testCreateUpdateContributionInValidStatusChange() {
    * @throws \CRM_Core_Exception
    */
   public function testCreateUpdateContributionCancelPending() {
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'receive_date' => '2012-01-01',
       'total_amount' => 100.00,
@@ -1720,8 +1720,8 @@ public function testCreateUpdateContributionCancelPending() {
       'is_pay_later' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
-    $newParams = array_merge($contribParams, [
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'contribution_status_id' => 3,
       'cancel_date' => '2012-02-02 09:00',
@@ -1741,7 +1741,7 @@ public function testCreateUpdateContributionCancelPending() {
    * @throws \CRM_Core_Exception
    */
   public function testCreateUpdateContributionChangeFinancialType() {
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'receive_date' => '2012-01-01',
       'total_amount' => 100.00,
@@ -1750,8 +1750,8 @@ public function testCreateUpdateContributionChangeFinancialType() {
       'contribution_status_id' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
-    $newParams = array_merge($contribParams, [
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'financial_type_id' => 3,
     ]);
@@ -1766,7 +1766,7 @@ public function testCreateUpdateContributionChangeFinancialType() {
    * @throws \CRM_Core_Exception
    */
   public function testCreateUpdateContributionWithFeeAmountChangeFinancialType() {
-    $contribParams = [
+    $contributionParams = [
       'contact_id' => $this->_individualId,
       'receive_date' => '2012-01-01',
       'total_amount' => 100.00,
@@ -1776,8 +1776,8 @@ public function testCreateUpdateContributionWithFeeAmountChangeFinancialType() {
       'contribution_status_id' => 1,
 
     ];
-    $contribution = $this->callAPISuccess('contribution', 'create', $contribParams);
-    $newParams = array_merge($contribParams, [
+    $contribution = $this->callAPISuccess('contribution', 'create', $contributionParams);
+    $newParams = array_merge($contributionParams, [
       'id' => $contribution['id'],
       'financial_type_id' => 3,
     ]);
@@ -2640,9 +2640,11 @@ public function testRepeattransactionRenewMembershipOldMembership() {
    *
    * @dataProvider contributionStatusProvider
    *
+   * @param array $contributionStatus
+   *
    * @throws \CRM_Core_Exception
    */
-  public function testRepeatTransactionMembershipRenewContributionNotCompleted($contributionStatus): void {
+  public function testRepeatTransactionMembershipRenewContributionNotCompleted(array $contributionStatus): void {
     // Completed status should renew so we don't test that here
     // In Progress status was never actually intended to be available for contributions.
     // Partially paid is not valid.