From 9cf618c4e194355ee1c358b9b09587ee8e2f5e72 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Fri, 17 Jun 2022 12:11:12 +1200 Subject: [PATCH] dev/core#3663 fix output regression on csv output This fixes a very-recent rc regression whereby the file error output was not correctly referring to the fields to be exported. Note that in order to make it testable I had to - remove header-setting that duplicates work done by the csv package - remove extraneous echos in the GenCode --- CRM/Core/CodeGen/Main.php | 19 +++++++++++++- CRM/Import/DataSource.php | 20 ++++++++++++++- CRM/Import/DataSource/CSV.php | 1 + CRM/Import/DataSource/SQL.php | 6 ++--- CRM/Import/Forms.php | 14 ++++------- CRM/Import/Parser.php | 2 +- Civi/Test.php | 1 + .../Import/Form/data/column_names_casing.csv | 2 ++ .../CRM/Contact/Import/Parser/ContactTest.php | 25 +++++++++++++++---- 9 files changed, 69 insertions(+), 21 deletions(-) create mode 100644 tests/phpunit/CRM/Contact/Import/Form/data/column_names_casing.csv diff --git a/CRM/Core/CodeGen/Main.php b/CRM/Core/CodeGen/Main.php index d108102259..fe90d38f50 100644 --- a/CRM/Core/CodeGen/Main.php +++ b/CRM/Core/CodeGen/Main.php @@ -56,6 +56,23 @@ class CRM_Core_CodeGen_Main { */ public $sourceDigest; + /** + * Should the specification be allowed to echo output. + * + * @var bool + */ + protected $verbose = TRUE; + + /** + * @param bool $verbose + * + * @return CRM_Core_CodeGen_Main + */ + public function setVerbose(bool $verbose): CRM_Core_CodeGen_Main { + $this->verbose = $verbose; + return $this; + } + /** * @param $CoreDAOCodePath * @param $sqlCodePath @@ -142,7 +159,7 @@ Alternatively you can get a version of CiviCRM that matches your PHP version public function init() { if (!$this->database || !$this->tables) { $specification = new CRM_Core_CodeGen_Specification(); - $specification->parse($this->schemaPath, $this->buildVersion); + $specification->parse($this->schemaPath, $this->buildVersion, $this->verbose); $this->database = $specification->database; $this->tables = $specification->tables; } diff --git a/CRM/Import/DataSource.php b/CRM/Import/DataSource.php index df8ea13d12..078211df7a 100644 --- a/CRM/Import/DataSource.php +++ b/CRM/Import/DataSource.php @@ -272,6 +272,24 @@ abstract class CRM_Import_DataSource { return $this->getUserJob()['metadata']['DataSource']['column_headers']; } + /** + * Get the field names of the fields holding data in the import tracking table. + * + * @return array + * @throws \API_Exception + * @throws \CRM_Core_Exception + */ + public function getDataFieldNames(): array { + $result = CRM_Core_DAO::executeQuery( + 'SHOW FIELDS FROM ' . $this->getTableName() . " + WHERE Field NOT LIKE '\_%'"); + $fields = []; + while ($result->fetch()) { + $fields[] = $result->Field; + } + return $fields; + } + /** * Get an array of column headers, if any. * @@ -572,7 +590,7 @@ abstract class CRM_Import_DataSource { * @return string */ private function getSelectClause(): string { - return $this->getSelectFields() ? implode(', ', $this->getSelectFields()) : '*'; + return $this->getSelectFields() ? '`' . implode('`, `', $this->getSelectFields()) . '`' : '*'; } /** diff --git a/CRM/Import/DataSource/CSV.php b/CRM/Import/DataSource/CSV.php index 4712ce30e9..2c8a19b3c4 100644 --- a/CRM/Import/DataSource/CSV.php +++ b/CRM/Import/DataSource/CSV.php @@ -131,6 +131,7 @@ class CRM_Import_DataSource_CSV extends CRM_Import_DataSource { $strtolower = function_exists('mb_strtolower') ? 'mb_strtolower' : 'strtolower'; $columns = array_map($strtolower, $firstrow); + $columns = array_map('trim', $columns); $columns = str_replace(' ', '_', $columns); $columns = preg_replace('/[^a-z_]/', '', $columns); diff --git a/CRM/Import/DataSource/SQL.php b/CRM/Import/DataSource/SQL.php index 78b20839f9..d552d45103 100644 --- a/CRM/Import/DataSource/SQL.php +++ b/CRM/Import/DataSource/SQL.php @@ -84,11 +84,9 @@ class CRM_Import_DataSource_SQL extends CRM_Import_DataSource { $tableName = $table->getName(); $table->createWithQuery($this->getSubmittedValue('sqlQuery')); - // Get the names of the fields to be imported. Any fields starting with an - // underscore are considered to be internal to the import process) + // Get the names of the fields to be imported. $columnsResult = CRM_Core_DAO::executeQuery( - 'SHOW FIELDS FROM ' . $tableName . " - WHERE Field NOT LIKE '\_%'"); + 'SHOW FIELDS FROM ' . $tableName); $columnNames = []; while ($columnsResult->fetch()) { diff --git a/CRM/Import/Forms.php b/CRM/Import/Forms.php index b2f96352e4..633bb5cb34 100644 --- a/CRM/Import/Forms.php +++ b/CRM/Import/Forms.php @@ -468,9 +468,9 @@ class CRM_Import_Forms extends CRM_Core_Form { */ protected function getOutputRows($statuses = [], int $limit = 0) { $statuses = (array) $statuses; - return $this->getDataSourceObject()->setLimit($limit)->setStatuses($statuses) - ->setSelectFields(array_merge(['_id', '_status_message'], $this->getColumnHeaders())) - ->setStatuses($statuses)->getRows(); + $dataSource = $this->getDataSourceObject()->setLimit($limit)->setStatuses($statuses)->setStatuses($statuses); + $dataSource->setSelectFields(array_merge(['_id', '_status_message'], $dataSource->getDataFieldNames())); + return $dataSource->getRows(); } /** @@ -512,7 +512,7 @@ class CRM_Import_Forms extends CRM_Core_Form { */ public static function outputCSV(): void { $userJobID = CRM_Utils_Request::retrieveValue('user_job_id', 'Integer', NULL, TRUE); - $status = CRM_Utils_Request::retrieveValue('status', 'String', NULL, TRUE); + $status = (int) CRM_Utils_Request::retrieveValue('status', 'String', NULL, TRUE); $saveFileName = CRM_Import_Parser::saveFileName($status); $form = new CRM_Import_Forms(); @@ -523,13 +523,9 @@ class CRM_Import_Forms extends CRM_Core_Form { $writer = Writer::createFromFileObject(new SplTempFileObject()); $headers = $form->getOutputColumnsHeaders(); $writer->insertOne($headers); - // Note this might be more inefficient that iterating the result + // Note this might be more inefficient by iterating the result // set & doing insertOne - possibly something to explore later. $writer->insertAll($form->getOutputRows($status)); - - CRM_Utils_System::setHttpHeader('Cache-Control', 'must-revalidate, post-check=0, pre-check=0'); - CRM_Utils_System::setHttpHeader('Content-Description', 'File Transfer'); - CRM_Utils_System::setHttpHeader('Content-Type', 'text/csv; charset=UTF-8'); $writer->output($saveFileName); CRM_Utils_System::civiExit(); } diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index 39ab0f52eb..006d202534 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -795,7 +795,7 @@ abstract class CRM_Import_Parser { /** * Determines the file name based on error code. * - * @var $type error code constant + * @var int $type code constant * @return string */ public static function saveFileName($type) { diff --git a/Civi/Test.php b/Civi/Test.php index 3deca30fbc..c19a400c04 100644 --- a/Civi/Test.php +++ b/Civi/Test.php @@ -166,6 +166,7 @@ class Test { if (!isset(self::$singletons['codeGen'])) { $civiRoot = str_replace(DIRECTORY_SEPARATOR, '/', dirname(__DIR__)); $codeGen = new \CRM_Core_CodeGen_Main("$civiRoot/CRM/Core/DAO", "$civiRoot/sql", $civiRoot, "$civiRoot/templates", NULL, "UnitTests", NULL, "$civiRoot/xml/schema/Schema.xml", NULL); + $codeGen->setVerbose(FALSE); $codeGen->init(); self::$singletons['codeGen'] = $codeGen; } diff --git a/tests/phpunit/CRM/Contact/Import/Form/data/column_names_casing.csv b/tests/phpunit/CRM/Contact/Import/Form/data/column_names_casing.csv new file mode 100644 index 0000000000..e019c694ea --- /dev/null +++ b/tests/phpunit/CRM/Contact/Import/Form/data/column_names_casing.csv @@ -0,0 +1,2 @@ +FIRSTNAME,LASTNAME,ID,IND/ORG,Do Not Mail,Valid Address +Mary,Jones,1,Individual,0,Yes diff --git a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php index 01150919d3..f9f97dd817 100644 --- a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php +++ b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php @@ -1061,13 +1061,27 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @throws \API_Exception * @throws \CRM_Core_Exception + * @throws \League\Csv\CannotInsertRecord */ - public function testImport($csv, $mapper, $expectedError, $expectedOutcomes = [], $submittedValues = []): void { + public function testImport($csv, $mapper, $expectedOutcomes = [], $submittedValues = []): void { $this->importCSV($csv, $mapper, $submittedValues); $dataSource = new CRM_Import_DataSource_CSV(UserJob::get(FALSE)->setSelect(['id'])->execute()->first()['id']); foreach ($expectedOutcomes as $outcome => $count) { $this->assertEquals($dataSource->getRowCount([$outcome]), $count); } + ob_start(); + $_REQUEST['user_job_id'] = $dataSource->getUserJobID(); + $_REQUEST['status'] = array_key_first($expectedOutcomes); + try { + CRM_Import_Forms::outputCSV(); + } + catch (CRM_Core_Exception_PrematureExitException $e) { + // For now just check it got this far without error. + ob_end_clean(); + return; + } + ob_end_clean(); + $this->fail('Should have resulted in a premature exit exception'); } /** @@ -1093,23 +1107,25 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { */ public function importDataProvider(): array { return [ + 'column_names_casing.csv' => [ + 'csv' => 'column_names_casing.csv', + 'mapper' => [['first_name'], ['last_name'], ['do_not_import'], ['do_not_import'], ['do_not_import'], ['do_not_import']], + 'expected_outcomes' => [CRM_Import_Parser::VALID => 1], + ], 'individual_unicode.csv' => [ 'csv' => 'individual_unicode.csv', 'mapper' => [['first_name'], ['last_name'], ['url', 1], ['country', 1]], - 'expected_error' => '', 'expected_outcomes' => [CRM_Import_Parser::VALID => 1], ], 'individual_invalid_sub_type' => [ 'csv' => 'individual_invalid_contact_sub_type.csv', 'mapper' => [['first_name'], ['last_name'], ['contact_sub_type']], - 'expected_error' => '', 'expected_outcomes' => [CRM_Import_Parser::ERROR => 1], ], //Record duplicates multiple contacts 'organization_multiple_duplicates_invalid' => [ 'csv' => 'organization_multiple_duplicates_invalid.csv', 'mapper' => [['organization_name'], ['email']], - 'expected_error' => '', 'expected_outcomes' => [ CRM_Import_Parser::VALID => 2, CRM_Import_Parser::ERROR => 1, @@ -1122,7 +1138,6 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { 'individual_invalid_external_identifier_email_mismatch' => [ 'csv' => 'individual_invalid_external_identifier_email_mismatch.csv', 'mapper' => [['first_name'], ['last_name'], ['email'], ['external_identifier']], - 'expected_error' => '', 'expected_outcomes' => [ CRM_Import_Parser::VALID => 2, CRM_Import_Parser::ERROR => 1, -- 2.25.1