Merge pull request #15537 from eileenmcnaughton/pay_check

Fix a regression whereby payment details are not saved from the AdditionalPayment form
civicrm · Oct 21, 2019 · f8fdba9 · f8fdba9
2 parents b19ac15 + a494d7a
commit f8fdba9
Show file tree

Hide file tree

Showing 7 changed files with 184 additions and 19 deletions.
diff --git a/CRM/Core/BAO/FinancialTrxn.php b/CRM/Core/BAO/FinancialTrxn.php
@@ -737,4 +737,23 @@ public static function updateFinancialAccountsOnPaymentInstrumentChange($inputPa
     return TRUE;
   }
 
+  /**
+   * Generate and assign an arbitrary value to a field of a test object.
+   *
+   * Always set is_payment to 1 as this is used for Payment api as  well as FinancialTrxn.
+   *
+   * @param string $fieldName
+   * @param array $fieldDef
+   * @param int $counter
+   *   The globally-unique ID of the test object.
+   */
+  protected function assignTestValue($fieldName, &$fieldDef, $counter) {
+    if ($fieldName === 'is_payment') {
+      $this->is_payment = 1;
+    }
+    else {
+      parent::assignTestValue($fieldName, $fieldDef, $counter);
+    }
+  }
+
 }
diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
@@ -1982,6 +1982,12 @@ public static function createTestObject(
     // Prefer to instantiate BAO's instead of DAO's (when possible)
     // so that assignTestValue()/assignTestFK() can be overloaded.
     $baoName = str_replace('_DAO_', '_BAO_', $daoName);
+    if ($baoName === 'CRM_Financial_BAO_FinancialTrxn') {
+      // OMG OMG OMG this is so incredibly bad. The BAO is insanely named.
+      // @todo create a new class called what the BAO SHOULD be
+      // that extends BAO-crazy-name.... migrate.
+      $baoName = 'CRM_Core_BAO_FinancialTrxn';
+    }
     if (class_exists($baoName)) {
       $daoName = $baoName;
     }

diff --git a/CRM/Financial/BAO/Payment.php b/CRM/Financial/BAO/Payment.php
@@ -59,27 +59,40 @@ public static function create($params) {
 
     $isPaymentCompletesContribution = self::isPaymentCompletesContribution($params['contribution_id'], $params['total_amount']);
 
+    $whiteList = ['check_number', 'payment_processor_id', 'fee_amount', 'total_amount', 'contribution_id', 'net_amount', 'card_type_id', 'pan_truncation', 'trxn_result_code', 'payment_instrument_id', 'trxn_id'];
+    $paymentTrxnParams = array_intersect_key($params, array_fill_keys($whiteList, 1));
+    $paymentTrxnParams['is_payment'] = 1;
+    if (!empty($params['payment_processor'])) {
+      // I can't find evidence this is passed in - I was gonna just remove it but decided to deprecate  as I see getToFinancialAccount
+      // also anticipates it.
+      CRM_Core_Error::deprecatedFunctionWarning('passing payment_processor is deprecated - use payment_processor_id');
+      $paymentTrxnParams['payment_processor_id'] = $params['payment_processor'];
+    }
+    if (isset($paymentTrxnParams['payment_processor_id']) && empty($paymentTrxnParams['payment_processor_id'])) {
+      // Don't pass 0 - ie the Pay Later processor as it is  a pseudo-processor.
+      unset($paymentTrxnParams['payment_processor_id']);
+    }
+    if (empty($paymentTrxnParams['payment_instrument_id'])) {
+      if (!empty($params['payment_processor_id'])) {
+        $paymentTrxnParams['payment_instrument_id'] = civicrm_api3('PaymentProcessor', 'getvalue', ['return' => 'payment_instrument_id', 'id' => $paymentTrxnParams['payment_processor_id']]);
+      }
+      else {
+        // Fall back  on the payment instrument  already  used - should  we  deprecate  this?
+        $paymentTrxnParams['payment_instrument_id'] = $contribution['payment_instrument_id'];
+      }
+    }
+    if (empty($paymentTrxnParams['trxn_id']) && !empty($paymentTrxnParams['contribution_trxn_id'])) {
+      CRM_Core_Error::deprecatedFunctionWarning('contribution_trxn_id is deprecated - use trxn_id');
+      $paymentTrxnParams['trxn_id'] = $paymentTrxnParams['contribution_trxn_id'];
+    }
+
     if ($params['total_amount'] > 0) {
       $paymentTrxnParams['to_financial_account_id'] = CRM_Contribute_BAO_Contribution::getToFinancialAccount($contribution, $params);
       $paymentTrxnParams['from_financial_account_id'] = CRM_Financial_BAO_FinancialAccount::getFinancialAccountForFinancialTypeByRelationship($contribution['financial_type_id'], 'Accounts Receivable Account is');
-      $paymentTrxnParams['total_amount'] = $params['total_amount'];
-      $paymentTrxnParams['contribution_id'] = $params['contribution_id'];
       $paymentTrxnParams['trxn_date'] = CRM_Utils_Array::value('trxn_date', $params, CRM_Utils_Array::value('contribution_receive_date', $params, date('YmdHis')));
-      $paymentTrxnParams['fee_amount'] = CRM_Utils_Array::value('fee_amount', $params);
-      $paymentTrxnParams['net_amount'] = CRM_Utils_Array::value('total_amount', $params);
       $paymentTrxnParams['currency'] = $contribution['currency'];
-      $paymentTrxnParams['trxn_id'] = CRM_Utils_Array::value('contribution_trxn_id', $params, NULL);
       $paymentTrxnParams['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_FinancialTrxn', 'status_id', 'Completed');
-      $paymentTrxnParams['payment_instrument_id'] = CRM_Utils_Array::value('payment_instrument_id', $params, $contribution['payment_instrument_id']);
-      $paymentTrxnParams['check_number'] = CRM_Utils_Array::value('check_number', $params);
-      $paymentTrxnParams['is_payment'] = 1;
-
-      if (!empty($params['payment_processor'])) {
-        // I can't find evidence this is passed in - I was gonna just remove it but decided to deprecate  as I see getToFinancialAccount
-        // also anticipates it.
-        CRM_Core_Error::deprecatedFunctionWarning('passing payment_processor is deprecated - use payment_processor_id');
-        $paymentTrxnParams['payment_processor_id'] = $params['payment_processor'];
-      }
+
       $trxn = CRM_Core_BAO_FinancialTrxn::create($paymentTrxnParams);
 
       // @todo - this is just weird & historical & inconsistent - why 2 tracks?

diff --git a/api/v3/Payment.php b/api/v3/Payment.php
@@ -129,9 +129,12 @@ function civicrm_api3_payment_cancel($params) {
  * @param array $params
  *   Input parameters.
  *
- * @throws API_Exception
  * @return array
  *   Api result array
+ *
+ * @throws \API_Exception
+ * @throws \CRM_Core_Exception
+ * @throws \CiviCRM_API3_Exception
  */
 function civicrm_api3_payment_create($params) {
   // Check if it is an update
@@ -168,9 +171,16 @@ function _civicrm_api3_payment_create_spec(&$params) {
       'type' => CRM_Utils_Type::T_FLOAT,
     ],
     'payment_processor_id' => [
-      'title' => ts('Payment Processor ID'),
+      'name' => 'payment_processor_id',
       'type' => CRM_Utils_Type::T_INT,
-      'description' => ts('Payment processor ID - required for payment processor payments'),
+      'title' => ts('Payment Processor'),
+      'description' => ts('Payment Processor for this payment'),
+      'where' => 'civicrm_financial_trxn.payment_processor_id',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'FKClassName' => 'CRM_Financial_DAO_PaymentProcessor',
     ],
     'id' => [
       'title' => ts('Payment ID'),
@@ -187,6 +197,103 @@ function _civicrm_api3_payment_create_spec(&$params) {
       'type' => CRM_Utils_Type::T_BOOLEAN,
       'api.default' => TRUE,
     ],
+    'payment_instrument_id' => [
+      'name' => 'payment_instrument_id',
+      'type' => CRM_Utils_Type::T_INT,
+      'title' => ts('Payment Method'),
+      'description' => ts('FK to payment_instrument option group values'),
+      'where' => 'civicrm_financial_trxn.payment_instrument_id',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Select',
+      ],
+      'pseudoconstant' => [
+        'optionGroupName' => 'payment_instrument',
+        'optionEditPath' => 'civicrm/admin/options/payment_instrument',
+      ],
+    ],
+    'card_type_id' => [
+      'name' => 'card_type_id',
+      'type' => CRM_Utils_Type::T_INT,
+      'title' => ts('Card Type ID'),
+      'description' => ts('FK to accept_creditcard option group values'),
+      'where' => 'civicrm_financial_trxn.card_type_id',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Select',
+      ],
+      'pseudoconstant' => [
+        'optionGroupName' => 'accept_creditcard',
+        'optionEditPath' => 'civicrm/admin/options/accept_creditcard',
+      ],
+    ],
+    'trxn_result_code' => [
+      'name' => 'trxn_result_code',
+      'type' => CRM_Utils_Type::T_STRING,
+      'title' => ts('Transaction Result Code'),
+      'description' => ts('processor result code'),
+      'maxlength' => 255,
+      'size' => CRM_Utils_Type::HUGE,
+      'where' => 'civicrm_financial_trxn.trxn_result_code',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+    ],
+    'trxn_id' => [
+      'name' => 'trxn_id',
+      'type' => CRM_Utils_Type::T_STRING,
+      'title' => ts('Transaction ID'),
+      'description' => ts('Transaction id supplied by external processor. This may not be unique.'),
+      'maxlength' => 255,
+      'size' => 10,
+      'where' => 'civicrm_financial_trxn.trxn_id',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Text',
+      ],
+    ],
+    'check_number' => [
+      'name' => 'check_number',
+      'type' => CRM_Utils_Type::T_STRING,
+      'title' => ts('Check Number'),
+      'description' => ts('Check number'),
+      'maxlength' => 255,
+      'size' => 6,
+      'where' => 'civicrm_financial_trxn.check_number',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Text',
+      ],
+    ],
+    'pan_truncation' => [
+      'name' => 'pan_truncation',
+      'type' => CRM_Utils_Type::T_STRING,
+      'title' => ts('Pan Truncation'),
+      'description' => ts('Last 4 digits of credit card'),
+      'maxlength' => 4,
+      'size' => 4,
+      'where' => 'civicrm_financial_trxn.pan_truncation',
+      'table_name' => 'civicrm_financial_trxn',
+      'entity' => 'FinancialTrxn',
+      'bao' => 'CRM_Financial_DAO_FinancialTrxn',
+      'localizable' => 0,
+      'html' => [
+        'type' => 'Text',
+      ],
+    ],
   ];
 }
 

diff --git a/api/v3/utils.php b/api/v3/utils.php
@@ -390,6 +390,9 @@ function _civicrm_api3_get_BAO($name) {
     // not the other cache info like search results (which could in fact be in Redis or another cache engine)
     $name = 'PrevNextCache';
   }
+  if ($name === 'Payment') {
+    $name = 'FinancialTrxn';
+  }
   $dao = _civicrm_api3_get_DAO($name);
   if (!$dao) {
     return NULL;

diff --git a/tests/phpunit/api/v3/PaymentTest.php b/tests/phpunit/api/v3/PaymentTest.php
@@ -371,10 +371,13 @@ public function testCreatePaymentNoLineItems() {
 
   /**
    * Function to assert db values
+   *
+   * @throws \CRM_Core_Exception
    */
   public function checkPaymentResult($payment, $expectedResult) {
+    $refreshedPayment = $this->callAPISuccessGetSingle('Payment', ['financial_trxn_id' => $payment['id']]);
     foreach ($expectedResult[$payment['id']] as $key => $value) {
-      $this->assertEquals($payment['values'][$payment['id']][$key], $value, 'mismatch on ' . $key);
+      $this->assertEquals($refreshedPayment[$key], $value, 'mismatch on ' . $key);      $this->assertEquals($refreshedPayment[$key], $value, 'mismatch on ' . $key);
     }
   }
 
@@ -670,6 +673,7 @@ public function testUpdatePayment() {
    */
   public function testCreatePaymentPayLater() {
     $this->createLoggedInUser();
+    $processorID  = $this->paymentProcessorCreate();
     $contributionParams = [
       'total_amount' => 100,
       'currency' => 'USD',
@@ -683,6 +687,11 @@ public function testCreatePaymentPayLater() {
     $params = [
       'contribution_id' => $contribution['id'],
       'total_amount' => 100,
+      'card_type_id' => 'Visa',
+      'pan_truncation' => '1234',
+      'trxn_result_code' => 'Startling success',
+      'payment_instrument_id' => $processorID,
+      'trxn_id' => 1234,
     ];
     $payment = $this->callAPISuccess('Payment', 'create', $params);
     $expectedResult = [
@@ -692,6 +701,11 @@ public function testCreatePaymentPayLater() {
         'total_amount' => 100,
         'status_id' => 1,
         'is_payment' => 1,
+        'card_type_id' => 1,
+        'pan_truncation' => '1234',
+        'trxn_result_code' => 'Startling success',
+        'trxn_id' => 1234,
+        'payment_instrument_id' => 1,
       ],
     ];
     $this->checkPaymentResult($payment, $expectedResult);

diff --git a/tests/phpunit/api/v3/SyntaxConformanceTest.php b/tests/phpunit/api/v3/SyntaxConformanceTest.php
@@ -469,6 +469,7 @@ public static function toBeSkipped_automock($sequential = FALSE) {
       'ReportTemplate',
       'System',
       'Logging',
+      'Payment',
     ];
     if ($sequential === TRUE) {
       return $entitiesWithoutGet;
@@ -561,6 +562,8 @@ public static function toBeSkipped_getlimit() {
       // fails on 5 limit - probably a set up problem
       'Setting',
       //a bit of a pseudoapi - keys by domain
+      'Payment',
+      // pseudoapi - problems with creating required sub  entities.
     ];
     return $entitiesWithout;
   }