From 3817addccdbf590e8250da23d7d917a90a6c0e8d Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Sat, 24 Sep 2022 19:41:18 +1200 Subject: [PATCH 1/4] Fix civiimport crash on unmapped fields --- CRM/Contribute/Import/Parser/Contribution.php | 4 ++-- .../CRM/Contribute/Import/Parser/ContributionTest.php | 1 + .../Contribute/Import/Parser/data/soft_credit_extended.csv | 6 +++--- tests/phpunit/CRMTraits/Import/ParserTrait.php | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CRM/Contribute/Import/Parser/Contribution.php b/CRM/Contribute/Import/Parser/Contribution.php index 03eabb63dee8..a4b8476844bb 100644 --- a/CRM/Contribute/Import/Parser/Contribution.php +++ b/CRM/Contribute/Import/Parser/Contribution.php @@ -208,7 +208,7 @@ public function getRequiredFieldsForMatch(): array { public function getMappedRow(array $values): array { $params = []; foreach ($this->getFieldMappings() as $i => $mappedField) { - if ($mappedField['name'] === 'do_not_import' || !$mappedField['name']) { + if (empty($mappedField['name']) || $mappedField['name'] === 'do_not_import') { continue; } $fieldSpec = $this->getFieldMetadata($mappedField['name']); @@ -774,7 +774,7 @@ public function getMappedFieldLabel(array $mappedField): string { if (empty($this->importableFieldsMetadata)) { $this->setFieldMetadata(); } - if ($mappedField['name'] === '') { + if (empty($mappedField['name'])) { return ''; } $title = []; diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php index d6ce4319d6e8..20bd1aaaf3f5 100644 --- a/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Import/Parser/ContributionTest.php @@ -214,6 +214,7 @@ public function testImportFromUserJobConfiguration(): void { ['name' => 'soft_credit.contact.email_primary.email', 'entity_data' => ['soft_credit' => ['soft_credit_type_id' => 5]]], ['name' => 'soft_credit.contact.first_name', 'entity_data' => ['soft_credit' => ['soft_credit_type_id' => 5]]], ['name' => 'soft_credit.contact.last_name', 'entity_data' => ['soft_credit' => ['soft_credit_type_id' => 5]]], + [], ]; $submittedValues = [ 'skipColumnHeader' => TRUE, diff --git a/tests/phpunit/CRM/Contribute/Import/Parser/data/soft_credit_extended.csv b/tests/phpunit/CRM/Contribute/Import/Parser/data/soft_credit_extended.csv index a7bdb279ac81..fa95767bc010 100644 --- a/tests/phpunit/CRM/Contribute/Import/Parser/data/soft_credit_extended.csv +++ b/tests/phpunit/CRM/Contribute/Import/Parser/data/soft_credit_extended.csv @@ -1,3 +1,3 @@ -Organization Name,Legal Name,Amount,Financial Type,Source,Received Date,External identifier,Soft credit email,Soft credit first name,Soft Credit last name -Big Firm,Big Firm inc.,800,,Import,2022-09-08,abc,jenny@example.org,Jenny,Hawthorn -Small Firm,Small Firm inc.,70,Donation,Import,2022-09-08,zyx,sarah@example.org,Sarah,Windsor +Organization Name,Legal Name,Amount,Financial Type,Source,Received Date,External identifier,Soft credit email,Soft credit first name,Soft Credit last name,Just some cruft +Big Firm,Big Firm inc.,800,,Import,2022-09-08,abc,jenny@example.org,Jenny,Hawthorn, +Small Firm,Small Firm inc.,70,Donation,Import,2022-09-08,zyx,sarah@example.org,Sarah,Windsor, diff --git a/tests/phpunit/CRMTraits/Import/ParserTrait.php b/tests/phpunit/CRMTraits/Import/ParserTrait.php index bc199e6e0f18..338acd76b3a2 100644 --- a/tests/phpunit/CRMTraits/Import/ParserTrait.php +++ b/tests/phpunit/CRMTraits/Import/ParserTrait.php @@ -87,7 +87,7 @@ protected function submitPreviewForm(array $submittedValues): void { protected function getMapperFromFieldMappings(array $mappings): array { $mapper = []; foreach ($mappings as $mapping) { - $fieldInput = [$mapping['name']]; + $fieldInput = [$mapping['name'] ?? '']; if (!empty($mapping['soft_credit_type_id'])) { $fieldInput[1] = $mapping['soft_credit_type_id']; } From 923c8e87aad6bfae6f07e6c3d740d810c338dc85 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Mon, 26 Sep 2022 14:23:16 +1300 Subject: [PATCH 2/4] Remove legacy table delete method --- CRM/Import/DataSource.php | 12 ------- ext/civiimport/Civi/Api4/Import/Import.php | 1 + .../Civi/Api4/Import/ImportProcessTrait.php | 31 +++++++++++++++++++ ext/civiimport/Civi/Api4/Import/Validate.php | 21 +++++++++++++ 4 files changed, 53 insertions(+), 12 deletions(-) create mode 100644 ext/civiimport/Civi/Api4/Import/Import.php create mode 100644 ext/civiimport/Civi/Api4/Import/ImportProcessTrait.php create mode 100644 ext/civiimport/Civi/Api4/Import/Validate.php diff --git a/CRM/Import/DataSource.php b/CRM/Import/DataSource.php index e592c5a906f3..236e65afc02a 100644 --- a/CRM/Import/DataSource.php +++ b/CRM/Import/DataSource.php @@ -384,18 +384,6 @@ protected function getTableName(): ?string { return NULL; } if (!$this->tableName) { - // If we are just loading this table we will do some validation. - // In the case of viewing historical jobs the table could have - // been deleted so we check that when we first load it. - if (strpos($tableName, 'civicrm_tmp_') !== 0 - || !CRM_Utils_Rule::alphanumeric($tableName)) { - // The table name is generated and stored by code, not users so it - // should be safe - but a check seems prudent all the same. - throw new CRM_Core_Exception('Table cannot be deleted'); - } - if (!CRM_Core_DAO::singleValueQuery('SHOW TABLES LIKE %1', [1 => [$tableName, 'String']])) { - throw new CRM_Import_Exception_ImportTableUnavailable('table deleted'); - } $this->tableName = $tableName; } return $this->tableName; diff --git a/ext/civiimport/Civi/Api4/Import/Import.php b/ext/civiimport/Civi/Api4/Import/Import.php new file mode 100644 index 000000000000..b3d9bbc7f371 --- /dev/null +++ b/ext/civiimport/Civi/Api4/Import/Import.php @@ -0,0 +1 @@ +_entityName); + foreach ($items as &$item) { + $item['_user_job_id'] = (int) $userJobID; + } + return parent::write($items); + } + +} diff --git a/ext/civiimport/Civi/Api4/Import/Validate.php b/ext/civiimport/Civi/Api4/Import/Validate.php new file mode 100644 index 000000000000..57fccc2b13d4 --- /dev/null +++ b/ext/civiimport/Civi/Api4/Import/Validate.php @@ -0,0 +1,21 @@ + Date: Mon, 26 Sep 2022 15:26:38 +1300 Subject: [PATCH 3/4] [REF] Extract validateRow --- CRM/Import/Parser.php | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index 65eba6fdf1c6..22a14bcafa4f 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -1820,15 +1820,7 @@ protected function getFieldEntity(string $fieldName) { public function validate(): void { $dataSource = $this->getDataSourceObject(); while ($row = $dataSource->getRow()) { - try { - $rowNumber = $row['_id']; - $values = array_values($row); - $this->validateValues($values); - $this->setImportStatus($rowNumber, 'VALID', ''); - } - catch (CRM_Core_Exception $e) { - $this->setImportStatus($rowNumber, 'ERROR', $e->getMessage()); - } + $this->validateRow($row); } } @@ -2551,4 +2543,19 @@ protected function getDedupeRules(array $dedupeRuleIDs, $contact_type) { return $dedupeRules; } + /** + * @param array|null $row + */ + public function validateRow(?array $row): void { + try { + $rowNumber = $row['_id']; + $values = array_values($row); + $this->validateValues($values); + $this->setImportStatus($rowNumber, 'VALID', ''); + } + catch (CRM_Core_Exception $e) { + $this->setImportStatus($rowNumber, 'ERROR', $e->getMessage()); + } + } + } From fffce686e35498139b8afd510af5177a6610af5b Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Mon, 26 Sep 2022 16:55:08 +1300 Subject: [PATCH 4/4] Add import & validate actions --- CRM/Import/Parser.php | 7 +- ext/civiimport/Civi/Api4/Import.php | 37 +++++++++- ext/civiimport/Civi/Api4/Import/Import.php | 41 ++++++++++++ .../Civi/Api4/Import/ImportProcessTrait.php | 67 +++++++++++++++++-- ext/civiimport/Civi/Api4/Import/Validate.php | 24 ++++++- .../Spec/Provider/ImportSpecProvider.php | 1 + .../tests/phpunit/CiviApiImportTest.php | 28 ++++++-- 7 files changed, 189 insertions(+), 16 deletions(-) diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index 22a14bcafa4f..6e5947515d92 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -632,7 +632,12 @@ protected function getRequiredFieldsContactCreate(): array { ]; } - protected function doPostImportActions() { + /** + * Core function - do not call from outside core. + * + * @internal + */ + public function doPostImportActions() { $userJob = $this->getUserJob(); $summaryInfo = $userJob['metadata']['summary_info'] ?? []; $actions = $userJob['metadata']['post_actions'] ?? []; diff --git a/ext/civiimport/Civi/Api4/Import.php b/ext/civiimport/Civi/Api4/Import.php index c44a68a92170..c68f42fd5888 100644 --- a/ext/civiimport/Civi/Api4/Import.php +++ b/ext/civiimport/Civi/Api4/Import.php @@ -17,6 +17,8 @@ use Civi\Api4\Import\Create; use Civi\Api4\Import\Save; use Civi\Api4\Import\Update; +use Civi\Api4\Import\Import as ImportAction; +use Civi\Api4\Import\Validate; /** * Import entity. @@ -27,6 +29,14 @@ */ class Import { + /** + * Constructor. + * + * This is here cos otherwise phpcs complains about the `import` function + * having the same name as the class. + */ + public function __construct() {} + /** * @param int $userJobID * @param bool $checkPermissions @@ -63,7 +73,8 @@ public static function save(int $userJobID, bool $checkPermissions = TRUE): Save /** * @param int $userJobID * @param bool $checkPermissions - * @return \Civi\Api4\Generic\DAOCreateAction + * + * @return \Civi\Api4\Import\Create * @throws \API_Exception */ public static function create(int $userJobID, bool $checkPermissions = TRUE): Create { @@ -101,6 +112,30 @@ public static function checkAccess(int $userJobID): CheckAccessAction { return new CheckAccessAction('Import_' . $userJobID, __FUNCTION__); } + /** + * @param int $userJobID + * @param bool $checkPermissions + * + * @return \Civi\Api4\Import\Import + * + * @throws \API_Exception + */ + public static function import(int $userJobID, bool $checkPermissions = TRUE): ImportAction { + return (new ImportAction('Import_' . $userJobID, __FUNCTION__)) + ->setCheckPermissions($checkPermissions); + } + + /** + * @param int $userJobID + * @param bool $checkPermissions + * + * @return \Civi\Api4\Import\Validate + * @throws \API_Exception + */ + public static function validate(int $userJobID, bool $checkPermissions = TRUE): Validate { + return (new Validate('Import_' . $userJobID, __FUNCTION__))->setCheckPermissions($checkPermissions); + } + /** * We need to implement these elsewhere as we permit based on 'created_id'. * diff --git a/ext/civiimport/Civi/Api4/Import/Import.php b/ext/civiimport/Civi/Api4/Import/Import.php index b3d9bbc7f371..60099303bd80 100644 --- a/ext/civiimport/Civi/Api4/Import/Import.php +++ b/ext/civiimport/Civi/Api4/Import/Import.php @@ -1 +1,42 @@ _entityName); + $where = $this->where; + $this->addWhere('_status', 'IN', ['new', 'valid']); + $this->getImportRows($result); + $parser = $this->getParser($userJobID); + foreach ($result as $row) { + $parser->import(array_values($row)); + } + $parser->doPostImportActions(); + + // Re-fetch the validated result with updated messages. + $this->where = $where; + $this->addSelect('*'); + parent::_run($result); + } + +} diff --git a/ext/civiimport/Civi/Api4/Import/ImportProcessTrait.php b/ext/civiimport/Civi/Api4/Import/ImportProcessTrait.php index 1a9227e6305a..c60a4d5b4e21 100644 --- a/ext/civiimport/Civi/Api4/Import/ImportProcessTrait.php +++ b/ext/civiimport/Civi/Api4/Import/ImportProcessTrait.php @@ -12,20 +12,73 @@ namespace Civi\Api4\Import; +use Civi\Api4\Generic\Result; +use Civi\Api4\Import; +use Civi\Api4\UserJob; + /** * Code shared by Import Save/Update actions. + * + * @method getCheckPermissions() */ -trait ImportSaveTrait { +trait ImportProcessTrait { + + /** + * Get the parser for the import + * + * @return \CRM_Import_Parser|\CRM_Contribute_Import_Parser_Contribution + * + * @throws \CRM_Core_Exception + */ + protected function getParser(int $userJobID) { + $userJob = UserJob::get($this->getCheckPermissions()) + ->addWhere('id', '=', $userJobID) + ->addSelect('job_type') + ->execute() + ->first(); + $parserClass = NULL; + foreach (\CRM_Core_BAO_UserJob::getTypes() as $userJobType) { + if ($userJob['job_type'] === $userJobType['id']) { + $parserClass = $userJobType['class']; + } + } + /** @var \CRM_Import_Parser|\CRM_Contribute_Import_Parser_Contribution $parser */ + $parser = new $parserClass(); + $parser->setUserJobID($userJobID); + $parser->init(); + return $parser; + } /** - * @inheritDoc + * Get the selected import rows. + * + * @param \Civi\Api4\Generic\Result $result + * + * @throws \CRM_Core_Exception */ - protected function write(array $items) { - $userJobID = str_replace('Import_', '', $this->_entityName); - foreach ($items as &$item) { - $item['_user_job_id'] = (int) $userJobID; + protected function getImportRows(Result $result): void { + $userJobID = (int) str_replace('Import_', '', $this->_entityName); + $this->addSelect('*'); + $importFields = array_keys((array) Import::getFields($userJobID, $this->getCheckPermissions()) + ->addSelect('name') + ->addWhere('name', 'NOT LIKE', '_%') + ->execute() + ->indexBy('name')); + $importFields[] = '_id'; + $importFields[] = '_entity_id'; + $this->setSelect($importFields); + parent::_run($result); + foreach ($result as &$row) { + if ($row['_entity_id']) { + // todo - how should we handle this? Skip, exception. At this case + // it is non ui accessible so this is good for now. + throw new \CRM_Core_Exception('Row already imported'); + } + // Push ID to the end as the get has moved it to the front & order matters here. + $rowID = $row['_id']; + unset($row['_id'], $row['_entity_id']); + $row['_id'] = $rowID; } - return parent::write($items); } } diff --git a/ext/civiimport/Civi/Api4/Import/Validate.php b/ext/civiimport/Civi/Api4/Import/Validate.php index 57fccc2b13d4..00db93c33554 100644 --- a/ext/civiimport/Civi/Api4/Import/Validate.php +++ b/ext/civiimport/Civi/Api4/Import/Validate.php @@ -12,10 +12,28 @@ namespace Civi\Api4\Import; -use Civi\Api4\Generic\DAOCreateAction; +use Civi\Api4\Generic\DAOGetAction; +use Civi\Api4\Generic\Result; -class Import extends DAOCreateAction { +class Validate extends DAOGetAction { - use ImportSaveTrait; + use ImportProcessTrait; + + /** + * @throws \CRM_Core_Exception + */ + public function _run(Result $result): void { + $userJobID = (int) str_replace('Import_', '', $this->_entityName); + + $this->getImportRows($result); + $parser = $this->getParser($userJobID); + foreach ($result as $row) { + $parser->validateRow($row); + } + + // Re-fetch the validated result with updated messages. + $this->addSelect('*'); + parent::_run($result); + } } diff --git a/ext/civiimport/Civi/Api4/Service/Spec/Provider/ImportSpecProvider.php b/ext/civiimport/Civi/Api4/Service/Spec/Provider/ImportSpecProvider.php index e81e4108c456..c6288d4719dd 100644 --- a/ext/civiimport/Civi/Api4/Service/Spec/Provider/ImportSpecProvider.php +++ b/ext/civiimport/Civi/Api4/Service/Spec/Provider/ImportSpecProvider.php @@ -41,6 +41,7 @@ public function modifySpec(RequestSpec $spec): void { $field = new FieldSpec($column['name'], $spec->getEntity(), 'String'); $field->setTitle(ts('Import field') . ':' . $column['label']); $field->setLabel($column['label']); + $field->setType('Field'); $field->setReadonly($isInternalField); $field->setDescription(ts('Data being imported into the field.')); $field->setColumnName($column['name']); diff --git a/ext/civiimport/tests/phpunit/CiviApiImportTest.php b/ext/civiimport/tests/phpunit/CiviApiImportTest.php index ecf19f164b01..eb5b158a3946 100644 --- a/ext/civiimport/tests/phpunit/CiviApiImportTest.php +++ b/ext/civiimport/tests/phpunit/CiviApiImportTest.php @@ -63,6 +63,13 @@ public function testApiActions():void { 'dedupe_rule_id' => NULL, 'dateFormats' => CRM_Core_Form_Date::DATE_yyyy_mm_dd, ], + 'import_mappings' => [ + ['name' => 'external_identifier'], + ['name' => 'total_amount'], + ['name' => 'receive_date'], + ['name' => 'financial_type_id'], + [], + ], ], 'status_id:name' => 'draft', 'job_type' => 'contribution_import', @@ -82,11 +89,12 @@ public function testApiActions():void { ])->execute(); $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->execute()->first(); + $rowID = $import['_id']; $this->assertEquals('80', $import['amount_given']); Import::update($userJobID)->setValues([ 'amount_given' => NULL, - '_id' => $import['_id'], + '_id' => $rowID, '_status' => 'IMPORTED', ])->execute(); @@ -98,23 +106,35 @@ public function testApiActions():void { 'external_identifier' => 999, 'amount_given' => 9, '_status' => 'ERROR', + '_id' => $rowID, ], ])->execute(); - $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->addWhere('_id', '>', $import['_id'])->execute()->first(); + $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->addWhere('_id', '=', $rowID)->execute()->first(); $this->assertEquals(9, $import['amount_given']); Import::save($userJobID)->setRecords([ [ 'external_identifier' => 777, - '_id' => $import['_id'], + '_id' => $rowID, '_status' => 'ERROR', ], ])->execute(); - $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->addWhere('_id', '=', $import['_id'])->execute()->first(); + $import = Import::get($userJobID)->setSelect(['external_identifier', 'amount_given', '_status'])->addWhere('_id', '=', $rowID)->execute()->first(); $this->assertEquals(777, $import['external_identifier']); + $validate = Import::validate($userJobID)->addWhere('_id', '=', $rowID)->setLimit(1)->execute()->first(); + $this->assertEquals('Missing required fields: Contribution ID OR Invoice Reference OR Transaction ID OR Financial Type ID', $validate['_status_message']); + $this->assertEquals('ERROR', $validate['_status']); + + Import::update($userJobID)->setValues(['financial_type' => 'Donation'])->addWhere('_id', '=', $rowID)->execute(); + $validate = Import::validate($userJobID)->addWhere('_id', '=', $rowID)->setLimit(1)->execute()->first(); + $this->assertEquals('', $validate['_status_message']); + $this->assertEquals('VALID', $validate['_status']); + $imported = Import::import($userJobID)->addWhere('_id', '=', $rowID)->setLimit(1)->execute()->first(); + $this->assertEquals('ERROR', $imported['_status']); + $this->assertEquals('No matching Contact found', $imported['_status_message']); } /**