dev/core#183 Convert database testing to use CRM_Utils_SQL_TempTable … #15798
Conversation
…fuctionality and remove Incomplete Jobs handling as it is not called
|
(Standard links)
|
| * This class mainly exists to allow imports to be triggered synchronously (i.e. | ||
| * via a form post) and asynchronously (i.e. by the workflow system) | ||
| */ | ||
| class CRM_Contact_Import_Importer { |
There was a problem hiding this comment.
@eileenmcnaughton i couldn't find this class called from anywhere
| /** | ||
| * @return array | ||
| */ | ||
| public static function getIncompleteImportTables() { |
There was a problem hiding this comment.
This is only called from the class that is being removed
| @@ -38,7 +38,7 @@ class CRM_Import_DataSource_CsvTest extends CiviUnitTestCase { | |||
| * @dataProvider getCsvFiles | |||
| */ | |||
| public function testToCsv($fileName) { | |||
| $dataSource = new CRM_Import_DataSource_Csv(); | |||
| $dataSource = new CRM_Import_DataSource_CSV(); | |||
There was a problem hiding this comment.
This is just to match to the actual class name
| $daoTestPrivilege->query("CREATE TEMPORARY TABLE import_job_permission_one(test int) ENGINE=InnoDB"); | ||
| $daoTestPrivilege->query("CREATE TEMPORARY TABLE import_job_permission_two(test int) ENGINE=InnoDB"); | ||
| $daoTestPrivilege->query("DROP TEMPORARY TABLE IF EXISTS import_job_permission_one, import_job_permission_two"); | ||
| $tempTable1 = CRM_Utils_SQL_TempTable::build()->getName(); |
There was a problem hiding this comment.
This feels a bit odd if we are actually creating tables (non temporary) why is the test here creating temporary tables
There was a problem hiding this comment.
Oh wow - that routine is kinda silly - that errorScope stuff should be a try-catch.
Anyway I tested this & it works
There was a problem hiding this comment.
Think about the importer pageflow:
- You upload a large a CSV file
- It shows a config screen -- where you can choose field-mappings
- It shows progress as it imports each of the records
In step 1, you need to put the uploaded content somewhere that will endure long enough to run the other steps (maybe 5 min; maybe an hour; maybe more). The importer uses CREATE TABLE to produce such a semi-durable storage place. CREATE TEMPORARY TABLE is not going to give you a place that lives long enough. (You could plausibly rewrite the importer to use other places for temporary storage -- like a cache record or civicrm.files -- but that's a hypothetical and broader proposition.)
This test is more informative if it's doing CREATE TABLE not CREATE TEMPORARY TABLE -- because those two may be stored in different places with different permissioning. For example: A MySQL user could have permission to CREATE TEMPORARY TABLE but not toCREATE new tables. (It's very easy to imagine a sysadmin improvising those permissions because there are use-cases/apps where it's sensible to split those permissions.) But in this case it would be a false-pass.
|
I stepped through the loading of the import form & that silliness is all about testing db perm to create a temp table. Still works - mop |
|
Test fail unrelated |
…fuctionality and remove Incomplete Jobs handling as it is not called
Overview
Ok this is a much more limited PR than https://github.com/civicrm/civicrm-core/pull/15797/files but much more correct in that it only touches on the temporary tables and also removes a function and class that are not called by anything else
Before
Temporary table name not standard
After
Temp table name standard and cruft code removed
ping @eileenmcnaughton