From cbc11a3740ec37c76ad3970218a3ce7e35852ea9 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Thu, 26 May 2022 10:14:33 +1200
Subject: [PATCH] dev/core#3038 Fix handling for payment_instrument_id, cleanup
financial_type_id
This addresses https://lab.civicrm.org/dev/core/-/issues/3038
by getting rid of the non-standardness around payment_instrument_id
and financial_type_id. The two fields are now
marked as importable and we use the
real names (payment_instrument_id) rather than the
'handy-dandy' names (payment_instrument) - which means
we can simplify in a bunch of places. Also since we are
already cleaning up names in the upgrade we add these in there
---
CRM/Contribute/BAO/Contribution.php | 3 ---
CRM/Contribute/DAO/Contribution.php | 4 +++-
CRM/Contribute/Import/Form/MapField.php | 4 ++--
CRM/Contribute/Import/Parser/Contribution.php | 17 ++---------------
CRM/Core/OptionValue.php | 2 ++
CRM/Import/Parser.php | 2 +-
CRM/Upgrade/Incremental/php/FiveFiftyOne.php | 2 ++
.../Import/Parser/ContributionTest.php | 12 ++++++------
xml/schema/Contribute/Contribution.xml | 2 ++
9 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 0250cb4339b..6b1ad984743 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -719,7 +719,6 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution im
$note = CRM_Core_DAO_Note::import();
$tmpFields = CRM_Contribute_DAO_Contribution::import();
unset($tmpFields['option_value']);
- $optionFields = CRM_Core_OptionValue::getFields($mode = 'contribute');
$contactFields = CRM_Contact_BAO_Contact::importableFields($contactType, NULL);
// Using new Dedupe rule.
@@ -758,8 +757,6 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution im
$fields = array_merge($fields, $tmpContactField);
$fields = array_merge($fields, $tmpFields);
$fields = array_merge($fields, $note);
- $fields = array_merge($fields, $optionFields);
- $fields = array_merge($fields, CRM_Financial_DAO_FinancialType::export());
$fields = array_merge($fields, CRM_Core_BAO_CustomField::getFieldsForImport('Contribution'));
self::$_importableFields = $fields;
}
diff --git a/CRM/Contribute/DAO/Contribution.php b/CRM/Contribute/DAO/Contribution.php
index 01911b8f040..0630d3bc7c9 100644
--- a/CRM/Contribute/DAO/Contribution.php
+++ b/CRM/Contribute/DAO/Contribution.php
@@ -6,7 +6,7 @@
*
* Generated from xml/schema/CRM/Contribute/Contribution.xml
* DO NOT EDIT. Generated by CRM_Core_CodeGen
- * (GenCodeChecksum:edd96be2e997a659ceeee0cf823c6f90)
+ * (GenCodeChecksum:0f869aa62eb1a94aedf6009a988cf01d)
*/
/**
@@ -418,6 +418,7 @@ class CRM_Contribute_DAO_Contribution extends CRM_Core_DAO {
'type' => CRM_Utils_Type::T_INT,
'title' => ts('Financial Type ID'),
'description' => ts('FK to Financial Type for (total_amount - non_deductible_amount).'),
+ 'import' => TRUE,
'where' => 'civicrm_contribution.financial_type_id',
'export' => TRUE,
'table_name' => 'civicrm_contribution',
@@ -465,6 +466,7 @@ class CRM_Contribute_DAO_Contribution extends CRM_Core_DAO {
'type' => CRM_Utils_Type::T_INT,
'title' => ts('Payment Method ID'),
'description' => ts('FK to Payment Instrument'),
+ 'import' => TRUE,
'where' => 'civicrm_contribution.payment_instrument_id',
'headerPattern' => '/^payment|(p(ayment\s)?instrument)$/i',
'export' => TRUE,
diff --git a/CRM/Contribute/Import/Form/MapField.php b/CRM/Contribute/Import/Form/MapField.php
index 34567d0874b..f855eaefb73 100644
--- a/CRM/Contribute/Import/Form/MapField.php
+++ b/CRM/Contribute/Import/Form/MapField.php
@@ -38,7 +38,7 @@ class CRM_Contribute_Import_Form_MapField extends CRM_Import_Form_MapField {
$requiredFields = [
$contactORContributionId == 'contribution_id' ? 'contribution_id' : 'contribution_contact_id' => $contactORContributionId == 'contribution_id' ? ts('Contribution ID') : ts('Contact ID'),
'total_amount' => ts('Total Amount'),
- 'financial_type' => ts('Financial Type'),
+ 'financial_type_id' => ts('Financial Type'),
];
foreach ($requiredFields as $field => $title) {
@@ -93,7 +93,7 @@ class CRM_Contribute_Import_Form_MapField extends CRM_Import_Form_MapField {
else {
$this->assign('rowDisplayCount', 2);
}
- $highlightedFields = ['financial_type', 'total_amount'];
+ $highlightedFields = ['financial_type_id', 'total_amount'];
//CRM-2219 removing other required fields since for updation only
//invoice id or trxn id or contribution id is required.
if ($this->_onDuplicate == CRM_Import_Parser::DUPLICATE_UPDATE) {
diff --git a/CRM/Contribute/Import/Parser/Contribution.php b/CRM/Contribute/Import/Parser/Contribution.php
index d97504e9f43..0a7777bd0dc 100644
--- a/CRM/Contribute/Import/Parser/Contribution.php
+++ b/CRM/Contribute/Import/Parser/Contribution.php
@@ -1342,21 +1342,6 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser {
}
break;
- case 'financial_type':
- // @todo add test like testPaymentTypeLabel & remove these lines in favour of 'default' part of switch.
- require_once 'CRM/Contribute/PseudoConstant.php';
- $contriTypes = CRM_Contribute_PseudoConstant::financialType();
- foreach ($contriTypes as $val => $type) {
- if (strtolower($value) == strtolower($type)) {
- $values['financial_type_id'] = $val;
- break;
- }
- }
- if (empty($values['financial_type_id'])) {
- return civicrm_api3_create_error("Financial Type is not valid: $value");
- }
- break;
-
case 'soft_credit':
// import contribution record according to select contact type
// validate contact id and external identifier.
@@ -1537,6 +1522,8 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Import_Parser {
if (isset($fields[$key]) &&
// Yay - just for a surprise we are inconsistent on whether we pass the pseudofield (payment_instrument)
// or the field name (contribution_status_id)
+ // @todo - payment_instrument is goneburger - now payment_instrument_id - how
+ // can we simplify.
(!empty($fields[$key]['is_pseudofield_for']) || !empty($fields[$key]['pseudoconstant']))
) {
$realField = $fields[$key]['is_pseudofield_for'] ?? $key;
diff --git a/CRM/Core/OptionValue.php b/CRM/Core/OptionValue.php
index e1219e982e8..89b566282d2 100644
--- a/CRM/Core/OptionValue.php
+++ b/CRM/Core/OptionValue.php
@@ -289,6 +289,8 @@ class CRM_Core_OptionValue {
$nameTitle = [];
if ($mode == 'contribute') {
+ // @todo - remove this - the only code place that calls
+ // this function in a way that would hit this is commented 'remove this'
// This is part of a move towards standardising option values but we
// should derive them from the fields array so am deprecating it again...
// note that the reason this was needed was that payment_instrument_id was
diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php
index c8651ea4d23..c9140bd6b45 100644
--- a/CRM/Import/Parser.php
+++ b/CRM/Import/Parser.php
@@ -284,7 +284,7 @@ abstract class CRM_Import_Parser {
// Duplicates are being skipped so id matching is not availble.
continue;
}
- $return[$name] = $field['title'];
+ $return[$name] = $field['html']['label'] ?? $field['title'];
}
return $return;
}
diff --git a/CRM/Upgrade/Incremental/php/FiveFiftyOne.php b/CRM/Upgrade/Incremental/php/FiveFiftyOne.php
index 836ff3facae..3bd484774ac 100644
--- a/CRM/Upgrade/Incremental/php/FiveFiftyOne.php
+++ b/CRM/Upgrade/Incremental/php/FiveFiftyOne.php
@@ -59,6 +59,8 @@ class CRM_Upgrade_Incremental_php_FiveFiftyOne extends CRM_Upgrade_Incremental_B
$fieldMap[ts('Soft Credit')] = 'soft_credit';
$fieldMap[ts('Pledge Payment')] = 'pledge_payment';
$fieldMap[ts(ts('Pledge ID'))] = 'pledge_id';
+ $fieldMap[ts(ts('Financial Type'))] = 'financial_type_id';
+ $fieldMap[ts(ts('Payment Method'))] = 'payment_instrument_id';
foreach ($mappings as $mapping) {
if (!empty($fieldMap[$mapping['name']])) {
diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php
index 03ff2e181d5..d3603522252 100644
--- a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php
+++ b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php
@@ -64,7 +64,7 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase {
$contact2Id = $this->individualCreate($contact2Params);
$values = [
'total_amount' => $this->formatMoneyInput(1230.99),
- 'financial_type' => 'Donation',
+ 'financial_type_id' => 'Donation',
'external_identifier' => 'ext-1',
'soft_credit' => 'ext-2',
];
@@ -108,12 +108,12 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase {
$this->addRandomOption();
$contactID = $this->individualCreate();
- $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type' => 'Donation', 'payment_instrument' => 'Check'];
+ $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type_id' => 'Donation', 'payment_instrument_id' => 'Check'];
$this->runImport($values, CRM_Import_Parser::DUPLICATE_UPDATE, NULL);
$contribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $contactID]);
$this->assertEquals('Check', $contribution['payment_instrument']);
- $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type' => 'Donation', 'payment_instrument' => 'not at all random'];
+ $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type_id' => 'Donation', 'payment_instrument_id' => 'not at all random'];
$this->runImport($values, CRM_Import_Parser::DUPLICATE_UPDATE, NULL);
$contribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $contactID, 'payment_instrument_id' => 'random']);
$this->assertEquals('not at all random', $contribution['payment_instrument']);
@@ -124,7 +124,7 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase {
*/
public function testContributionStatusLabel(): void {
$contactID = $this->individualCreate();
- $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type' => 'Donation', 'payment_instrument' => 'Check', 'contribution_status_id' => 'Pending'];
+ $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type_id' => 'Donation', 'payment_instrument_id' => 'Check', 'contribution_status_id' => 'Pending'];
// Note that the expected result should logically be CRM_Import_Parser::valid but writing test to reflect not fix here
$this->runImport($values, CRM_Import_Parser::DUPLICATE_UPDATE, NULL);
$contribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $contactID]);
@@ -172,7 +172,7 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase {
public function testParsedCustomOption(): void {
$contactID = $this->individualCreate();
- $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type' => 'Donation', 'payment_instrument' => 'Check', 'contribution_status_id' => 'Pending'];
+ $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type_id' => 'Donation', 'payment_instrument_id' => 'Check', 'contribution_status_id' => 'Pending'];
// Note that the expected result should logically be CRM_Import_Parser::valid but writing test to reflect not fix here
$this->runImport($values, CRM_Import_Parser::DUPLICATE_UPDATE, NULL);
$contribution = $this->callAPISuccess('Contribution', 'getsingle', ['contact_id' => $contactID]);
@@ -227,7 +227,7 @@ class CRM_Contribute_Import_Parser_ContributionTest extends CiviUnitTestCase {
$this->createCustomGroupWithFieldOfType([], 'checkbox');
$customField = $this->getCustomFieldName('checkbox');
$contactID = $this->individualCreate();
- $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type' => 'Donation', $customField => 'L,V'];
+ $values = ['contribution_contact_id' => $contactID, 'total_amount' => 10, 'financial_type_id' => 'Donation', $customField => 'L,V'];
$this->runImport($values, CRM_Import_Parser::DUPLICATE_SKIP, NULL);
$initialContribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $contactID]);
$this->assertContains('L', $initialContribution[$customField], "Contribution Duplicate Skip Import contains L");
diff --git a/xml/schema/Contribute/Contribution.xml b/xml/schema/Contribute/Contribution.xml
index 6aa5bc4e9a1..c5d470abee1 100644
--- a/xml/schema/Contribute/Contribution.xml
+++ b/xml/schema/Contribute/Contribution.xml
@@ -66,6 +66,7 @@
<labelColumn>name</labelColumn>
</pseudoconstant>
<export>true</export>
+ <import>true</import>
<html>
<type>Select</type>
<label>Financial Type</label>
@@ -108,6 +109,7 @@
<type>int unsigned</type>
<comment>FK to Payment Instrument</comment>
<export>true</export>
+ <import>true</import>
<headerPattern>/^payment|(p(ayment\s)?instrument)$/i</headerPattern>
<pseudoconstant>
<optionGroupName>payment_instrument</optionGroupName>
--
GitLab