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

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 0250cb4339b64cc26fe3d8db1bb99cd8ab164684..6b1ad984743aa1790f9701f9c022d47173de3943 100644 (file)
--- 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 01911b8f0400ed8a50c08674038acad6e8d1a568..0630d3bc7c9fc7042fb0ec4ed217f2312ac26063 100644 (file)
--- 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 34567d0874b4709c0432ff0199d4b8fbe2da27b2..f855eaefb73b9b37c004a9a65bcea4d9a6e25a52 100644 (file)
--- 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 d97504e9f4324752a852242682a0bce6e7a91cd2..0a7777bd0dc069f453b0fba6c17603504d7d28a1 100644 (file)
--- 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 e1219e982e8485ff5aa576b5d0f6ec29f2818b74..89b566282d2acad91073d33e2fe31ba5442385e1 100644 (file)
--- 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 c8651ea4d23a52e68633b70ee2dd5d97cabd17b8..c9140bd6b45d7febf8eda5db94699bbf1bb56bb8 100644 (file)
--- 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 836ff3facae6acf968fd62e3c0fa9744b2905d78..3bd484774ac39cd08e5d5924861608753798f649 100644 (file)
--- 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 03ff2e181d51306d368346c6b17017d7b8ad0da0..d36035222525f2ef87d981389bab9e51ca06cd7f 100644 (file)
--- 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 6aa5bc4e9a187199496d1362dd04a3d822bc6c88..c5d470abee1225e3b14e9a993be19667f27da3d8 100644 (file)
--- 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>