From 14b9e069607c6cbcbbaf21a209f647ba422b8e04 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 1 Aug 2019 10:04:07 +1200 Subject: [PATCH] Fix mishandling of renamed membership status labels on membership import --- CRM/Contribute/Import/Parser/Contribution.php | 13 +-- CRM/Core/PseudoConstant.php | 6 +- CRM/Import/Parser.php | 25 ++++++ CRM/Member/Import/Parser/Membership.php | 57 ++++-------- CRM/Member/PseudoConstant.php | 3 + .../Member/Import/Parser/MembershipTest.php | 89 ++++++++++++++++--- 6 files changed, 124 insertions(+), 69 deletions(-) diff --git a/CRM/Contribute/Import/Parser/Contribution.php b/CRM/Contribute/Import/Parser/Contribution.php index b6c801431f..82eb9ef421 100644 --- a/CRM/Contribute/Import/Parser/Contribution.php +++ b/CRM/Contribute/Import/Parser/Contribution.php @@ -1011,18 +1011,7 @@ class CRM_Contribute_Import_Parser_Contribution extends CRM_Contribute_Import_Pa ) { $realField = $fields[$key]['is_pseudofield_for'] ?? $key; $realFieldSpec = $fields[$realField]; - /* @var \CRM_Core_DAO $bao */ - $bao = $realFieldSpec['bao']; - // Get names & labels - we will try to match name first but if not available then see if - // we have a label that can be converted to a name. - // For historical reasons use validate as context - ie disabled name matches ARE permitted per prior to change. - $nameOptions = $bao::buildOptions($realField, 'validate'); - if (!isset($nameOptions[$value])) { - $labelOptions = array_flip($bao::buildOptions($realField, 'match')); - if (isset($labelOptions[$params[$key]])) { - $values[$key] = $labelOptions[$params[$key]]; - } - } + $values[$key] = $this->parsePseudoConstantField($value, $realFieldSpec); } break; } diff --git a/CRM/Core/PseudoConstant.php b/CRM/Core/PseudoConstant.php index df29c7f3f5..96a6a86420 100644 --- a/CRM/Core/PseudoConstant.php +++ b/CRM/Core/PseudoConstant.php @@ -300,8 +300,8 @@ class CRM_Core_PseudoConstant { $cacheKey = $daoName . $fieldName . serialize($params); // Retrieve cached options - if (isset(self::$cache[$cacheKey]) && empty($params['fresh'])) { - $output = self::$cache[$cacheKey]; + if (isset(\Civi::$statics[__CLASS__][$cacheKey]) && empty($params['fresh'])) { + $output = \Civi::$statics[__CLASS__][$cacheKey]; } else { $daoName = CRM_Core_DAO_AllCoreTables::getClassForTable($pseudoconstant['table']); @@ -386,7 +386,7 @@ class CRM_Core_PseudoConstant { } } CRM_Utils_Hook::fieldOptions($entity, $fieldName, $output, $params); - self::$cache[$cacheKey] = $output; + \Civi::$statics[__CLASS__][$cacheKey] = $output; } return $flip ? array_flip($output) : $output; } diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index cf01fdd054..6fab5498b9 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -520,4 +520,29 @@ abstract class CRM_Import_Parser { return $error; } + /** + * Parse a field which could be represented by a label or name value rather than the DB value. + * + * We will try to match name first but if not available then see if we have a label that can be converted to a name. + * + * @param string|int|null $submittedValue + * @param array $fieldSpec + * Metadata for the field + * + * @return mixed + */ + protected function parsePseudoConstantField($submittedValue, $fieldSpec) { + /* @var \CRM_Core_DAO $bao */ + $bao = $fieldSpec['bao']; + // For historical reasons use validate as context - ie disabled name matches ARE permitted. + $nameOptions = $bao::buildOptions($fieldSpec['name'], 'validate'); + if (!isset($nameOptions[$submittedValue])) { + $labelOptions = array_flip($bao::buildOptions($fieldSpec['name'], 'match')); + if (isset($labelOptions[$submittedValue])) { + return array_search($labelOptions[$submittedValue], $nameOptions, TRUE); + } + } + return ''; + } + } diff --git a/CRM/Member/Import/Parser/Membership.php b/CRM/Member/Import/Parser/Membership.php index 2501ba9d01..2cea95d33e 100644 --- a/CRM/Member/Import/Parser/Membership.php +++ b/CRM/Member/Import/Parser/Membership.php @@ -48,6 +48,13 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { private $_membershipTypeIndex; private $_membershipStatusIndex; + /** + * Array of metadata for all available fields. + * + * @var array + */ + protected $fieldMetadata = []; + /** * Array of successfully imported membership id's * @@ -59,12 +66,10 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { * Class constructor. * * @param $mapperKeys - * @param null $mapperLocType - * @param null $mapperPhoneType */ - public function __construct(&$mapperKeys, $mapperLocType = NULL, $mapperPhoneType = NULL) { + public function __construct($mapperKeys) { parent::__construct(); - $this->_mapperKeys = &$mapperKeys; + $this->_mapperKeys = $mapperKeys; } /** @@ -73,9 +78,10 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { * @return void */ public function init() { - $fields = CRM_Member_BAO_Membership::importableFields($this->_contactType, FALSE); + $this->fieldMetadata = CRM_Member_BAO_Membership::importableFields($this->_contactType, FALSE); - foreach ($fields as $name => $field) { + foreach ($this->fieldMetadata as $name => $field) { + // @todo - we don't really need to do all this.... fieldMetadata is just fine to use as is. $field['type'] = CRM_Utils_Array::value('type', $field, CRM_Utils_Type::T_INT); $field['dataPattern'] = CRM_Utils_Array::value('dataPattern', $field, '//'); $field['headerPattern'] = CRM_Utils_Array::value('headerPattern', $field, '//'); @@ -223,6 +229,7 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { break; case 'membership_type_id': + // @todo - squish into membership status - can use same lines here too. $membershipTypes = CRM_Member_PseudoConstant::membershipType(); if (!CRM_Utils_Array::crmInArray($val, $membershipTypes) && !array_key_exists($val, $membershipTypes) @@ -232,7 +239,7 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { break; case 'status_id': - if (!CRM_Utils_Array::crmInArray($val, CRM_Member_PseudoConstant::membershipStatus())) { + if (!empty($val) && !$this->parsePseudoConstantField($val, $this->fieldMetadata[$key])) { CRM_Contact_Import_Parser_Contact::addToErrorMsg('Membership Status', $errorMessage); } break; @@ -324,10 +331,8 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { break; case 'status_id': - if (!is_numeric($val)) { - unset($params['status_id']); - $params['membership_status'] = $val; - } + // @todo - we can do this based on the presence of 'pseudoconstant' in the metadata rather than field specific. + $params[$key] = $this->parsePseudoConstantField($val, $this->fieldMetadata[$key]); break; case 'is_override': @@ -347,12 +352,6 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { } //date-Format part ends - static $indieFields = NULL; - if ($indieFields == NULL) { - $tempIndieFields = CRM_Member_DAO_Membership::import(); - $indieFields = $tempIndieFields; - } - $formatValues = []; foreach ($params as $key => $field) { if ($field == NULL || $field === '') { @@ -721,30 +720,6 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { $values['membership_type_id'] = $membershipTypeId; break; - case 'status_id': - if (!CRM_Utils_Array::value($value, CRM_Member_PseudoConstant::membershipStatus())) { - throw new Exception('Invalid Membership Status Id'); - } - $values[$key] = $value; - break; - - case 'membership_status': - $membershipStatusId = CRM_Utils_Array::key(ucfirst($value), - CRM_Member_PseudoConstant::membershipStatus() - ); - if ($membershipStatusId) { - if (!empty($values['status_id']) && - $membershipStatusId != $values['status_id'] - ) { - throw new Exception('Mismatched membership Status and Membership Status Id'); - } - } - else { - throw new Exception('Invalid Membership Status'); - } - $values['status_id'] = $membershipStatusId; - break; - default: break; } diff --git a/CRM/Member/PseudoConstant.php b/CRM/Member/PseudoConstant.php index 417de01f0f..dedf38aaaf 100644 --- a/CRM/Member/PseudoConstant.php +++ b/CRM/Member/PseudoConstant.php @@ -132,6 +132,9 @@ class CRM_Member_PseudoConstant extends CRM_Core_PseudoConstant { if (isset(self::$$name)) { self::$$name = NULL; } + // The preferred source of membership pseudoconstants is in fact the Core class. + // which buildOptions accesses - better flush that too. + CRM_Core_PseudoConstant::flush(); } } diff --git a/tests/phpunit/CRM/Member/Import/Parser/MembershipTest.php b/tests/phpunit/CRM/Member/Import/Parser/MembershipTest.php index 4585ab9e77..06c2c2c738 100644 --- a/tests/phpunit/CRM/Member/Import/Parser/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Import/Parser/MembershipTest.php @@ -85,6 +85,8 @@ class CRM_Member_Import_Parser_MembershipTest extends CiviUnitTestCase { /** * Tears down the fixture, for example, closes a network connection. * This method is called after a test is executed. + * + * @throws \CRM_Core_Exception */ public function tearDown() { $tablesToTruncate = [ @@ -92,12 +94,12 @@ class CRM_Member_Import_Parser_MembershipTest extends CiviUnitTestCase { 'civicrm_membership_log', 'civicrm_contribution', 'civicrm_membership_payment', + 'civicrm_contact', ]; - $this->quickCleanup($tablesToTruncate); + $this->quickCleanup($tablesToTruncate, TRUE); $this->relationshipTypeDelete($this->_relationshipTypeId); $this->membershipTypeDelete(['id' => $this->_membershipTypeID]); $this->membershipStatusDelete($this->_mebershipStatusID); - $this->contactDelete($this->_orgContactID); } /** @@ -171,19 +173,20 @@ class CRM_Member_Import_Parser_MembershipTest extends CiviUnitTestCase { $this->assertContains('Required parameter missing: Status', $importValues); } + /** + * Test that the passed in status is respected. + * + * @throws \CRM_Core_Exception + */ public function testImportOverriddenMembershipWithStatus() { $this->individualCreate(['email' => 'anthony_anderson3@civicrm.org']); - - $fieldMapper = [ - 'mapper[0][0]' => 'email', - 'mapper[1][0]' => 'membership_type_id', - 'mapper[2][0]' => 'membership_start_date', - 'mapper[3][0]' => 'is_override', - 'mapper[4][0]' => 'status_id', - ]; - $membershipImporter = new CRM_Member_Import_Parser_Membership($fieldMapper); - $membershipImporter->init(); - $membershipImporter->_contactType = 'Individual'; + $membershipImporter = $this->createImportObject([ + 'email', + 'membership_type_id', + 'membership_start_date', + 'is_override', + 'status_id', + ]); $importValues = [ 'anthony_anderson3@civicrm.org', @@ -254,4 +257,64 @@ class CRM_Member_Import_Parser_MembershipTest extends CiviUnitTestCase { $this->assertContains('Required parameter missing: Status', $importValues); } + /** + * Test that memberships can still be imported if the status is renamed. + * + * @throws \CRM_Core_Exception + */ + public function testImportMembershipWithRenamedStatus() { + $this->individualCreate(['email' => 'anthony_anderson3@civicrm.org']); + + $this->callAPISuccess('MembershipStatus', 'get', [ + 'name' => 'New', + 'api.MembershipStatus.create' => [ + 'label' => 'New-renamed', + ], + ]); + $membershipImporter = $this->createImportObject([ + 'email', + 'membership_type_id', + 'membership_start_date', + 'is_override', + 'status_id', + ]); + + $importValues = [ + 'anthony_anderson3@civicrm.org', + $this->_membershipTypeID, + date('Y-m-d'), + TRUE, + 'New-renamed', + ]; + + $importResponse = $membershipImporter->import(CRM_Import_Parser::DUPLICATE_UPDATE, $importValues); + $this->assertEquals(CRM_Import_Parser::VALID, $importResponse); + $createdStatusID = $this->callAPISuccessGetValue('Membership', ['return' => 'status_id']); + $this->assertEquals(CRM_Core_PseudoConstant::getKey('CRM_Member_BAO_Membership', 'status_id', 'New'), $createdStatusID); + $this->callAPISuccess('MembershipStatus', 'get', [ + 'name' => 'New', + 'api.MembershipStatus.create' => [ + 'label' => 'New', + ], + ]); + } + + /** + * Create an import object. + * + * @param array $fields + * + * @return \CRM_Member_Import_Parser_Membership + */ + protected function createImportObject(array $fields): \CRM_Member_Import_Parser_Membership { + $fieldMapper = []; + foreach ($fields as $index => $field) { + $fieldMapper['mapper[' . $index . '][0]'] = $field; + } + $membershipImporter = new CRM_Member_Import_Parser_Membership($fieldMapper); + $membershipImporter->init(); + $membershipImporter->_contactType = 'Individual'; + return $membershipImporter; + } + } -- 2.25.1