Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a regression whereby payment details are not saved from the AdditionalPayment form #15537

Merged
merged 1 commit into from
Oct 21, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions CRM/Core/BAO/FinancialTrxn.php
Original file line number Diff line number Diff line change
@@ -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);
}
}

}
6 changes: 6 additions & 0 deletions CRM/Core/DAO.php
Original file line number Diff line number Diff line change
@@ -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;
}
43 changes: 28 additions & 15 deletions CRM/Financial/BAO/Payment.php
Original file line number Diff line number Diff line change
@@ -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?
113 changes: 110 additions & 3 deletions api/v3/Payment.php
Original file line number Diff line number Diff line change
@@ -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',
],
],
];
}

3 changes: 3 additions & 0 deletions api/v3/utils.php
Original file line number Diff line number Diff line change
@@ -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';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Is this change required for this PR? As I understand it Payment does not map directly to FinancialTrxn but wraps the financial entities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattwire yes - otherwise the pseudoconstant doesn't work.

Payment is a psuedo-api that represents one specific type of FinancialTrxn - hence the presence of 'is_payment' as a fixed parameter when creating a financial_trxn from the payment api.

Ie it's a subset of the FinancialTrxn entity

$dao = _civicrm_api3_get_DAO($name);
if (!$dao) {
return NULL;
16 changes: 15 additions & 1 deletion tests/phpunit/api/v3/PaymentTest.php
Original file line number Diff line number Diff line change
@@ -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);
3 changes: 3 additions & 0 deletions tests/phpunit/api/v3/SyntaxConformanceTest.php
Original file line number Diff line number Diff line change
@@ -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;
}