diff --git a/ehr/resources/web/ehr/data/DataEntryServerStore.js b/ehr/resources/web/ehr/data/DataEntryServerStore.js index 3e5eda44f..bbaf9198f 100644 --- a/ehr/resources/web/ehr/data/DataEntryServerStore.js +++ b/ehr/resources/web/ehr/data/DataEntryServerStore.js @@ -64,9 +64,17 @@ Ext4.define('EHR.data.DataEntryServerStore', { } }, this); - if (ret){ - return ret; - } + // An empty key cannot identify an existing record, so stop here rather than falling + // through to findExact() below -- an empty value matches ANY earlier unsaved record + // whose key is likewise empty, so the 2nd and later new rows all resolve to the 1st + // and collapse onto it, leaving only the last row's values. Returning nothing lets + // processServerRecords() create a server record bound to this client model instead. + // + // Only tables keyed on a server-assigned identity column hit this: study datasets key + // on objectid, which is generated client-side per row and so is never empty. A table + // whose sole key is a SERIAL (nbri_ehr.Conception.rowId) has an empty key on every new + // row, making it impossible to add more than one row per save. + return ret; } var idx = this.findExact(fieldName, value); diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 84244bbe3..3626fc86e 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -55,6 +55,14 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return f.name; }); + // Header resolution needs both halves of the section separately: the importable fields decide first, + // and the ones the importer skips (hidden, taskid, qcstate) are consulted only to tell a header this + // form knows but will not import from one it does not recognize at all. + this.allFieldConfigs = allConfigs; + this.skippedFieldConfigs = allConfigs.filter((f) => { + return this.fieldConfigs.indexOf(f) === -1; + }); + this.items = [{ html : 'This allows you to import data using a simple Excel or TSV file. To import, cut/paste the contents of the Excel or TSV file (Ctl + A is a good way to select all) into the box below and hit submit. The limit for import is 250 rows.', style: 'padding-bottom: 10px;' @@ -104,7 +112,13 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return; } - const parsed = LDK.Utils.CSVToArray(Ext4.String.trim(text), '\t'); + // Spaces and line endings are trimmed off the block, but never tabs. Ext4.String.trim() counts a tab + // as whitespace, so it stripped the last row's trailing tabs along with them, dropping that row's empty + // final cells and leaving it narrower than the rest -- which then read as a truncated row even though + // every value in it was correct. Spaces still have to go: a stray one after the final newline would + // otherwise parse as a junk one-cell row, and one on a leading blank line would be read as the header + // row itself. Individual cells are trimmed downstream by normalizeValue() and buildHeaderMap(). + const parsed = LDK.Utils.CSVToArray(text.replace(/^[ \r\n]+|[ \r\n]+$/g, ''), '\t'); if (!parsed){ Ext4.Msg.alert('Error', 'There was an error parsing the data.'); return; @@ -130,17 +144,25 @@ Ext4.define('EHR.window.FormBulkAddWindow', { errors.push('Row Count - ' + dataRowCount + ': Import maximum is 250 rows. Please split your import into multiple uploads and submit the form between each upload.'); } else { - - for (let i = 1; i < parsed.length; i++) { - const row = parsed[i]; - if (!row || row.length < this.requiredFieldNames.length) { - errors.push('Row ' + i + ': not enough items in row'); - continue; - } - - const newRow = this.processRow(parsed[0], row, errors, i); - if (newRow) { - records.push(this.targetStore.createModel(newRow)); + const headerMap = this.buildHeaderMap(parsed[0], errors); + + // Only parse rows once the header row is sound. A misspelled header for a required column + // otherwise reports as a missing value on every one of up to 250 rows, burying the one error + // that explains all of them. + if (!errors.length) { + const columnCount = this.getRequiredColumnCount(headerMap); + + for (let i = 1; i < parsed.length; i++) { + const row = parsed[i]; + if (!row || row.length < columnCount) { + errors.push('Row ' + i + ': not enough items in row -- has ' + (row ? row.length : 0) + ', needs at least ' + columnCount); + continue; + } + + const newRow = this.processRow(headerMap, row, errors, i); + if (newRow) { + records.push(this.targetStore.createModel(newRow)); + } } } @@ -148,7 +170,9 @@ Ext4.define('EHR.window.FormBulkAddWindow', { } if (errors.length){ - Ext4.Msg.alert('Error', 'There following errors were found:

' + errors.join('
')); + // A condition that holds for a whole column rather than a single row is reported without a + // row number, so the identical copy pushed by every row collapses to one line here. + Ext4.Msg.alert('Error', 'There following errors were found:

' + Ext4.Array.unique(errors).join('
')); return; } @@ -159,7 +183,152 @@ Ext4.define('EHR.window.FormBulkAddWindow', { this.close(); }, - resolveLookup: function(field, value, errors){ + // Cells pasted out of a spreadsheet routinely carry stray whitespace, which no exact match would + // survive. Trim before parsing or matching so a padded cell resolves and a blank one reads as empty. + normalizeValue: function(value){ + return Ext4.isString(value) ? Ext4.String.trim(value) : value; + }, + + // Identifies the field a pasted header names, which is what LABKEY.ext4.Util.resolveFieldNameFromLabel() + // exists for: it compares case-insensitively against a field's name, the display names the UI shows for it + // (label, caption, shortCaption) and every one of its import aliases. Matching only the name and the first + // alias, as this window used to, drops the column for any other spelling. + // + // Two things are decided here rather than left to the helper, because it breaks out of its search on the + // first name or display-name hit: it cannot distinguish one match from several, and so resolves a shared + // display name to whichever field is declared first. + resolveHeaderToFieldName: function(header){ + // An exact name match wins outright, so the field this header actually names cannot lose to another + // field that merely shares its display name. Importable fields are searched first here for the same + // reason they are below: a section can declare one name in two queries, and letting the skipped copy + // win would drop the column in silence. + const exact = this.findFieldByExactName(this.fieldConfigs, header) + || this.findFieldByExactName(this.skippedFieldConfigs, header); + if (exact) { + return exact.name; + } + + // Several fields answering to one display name is undecidable, and guessing is how a column lands in + // the wrong field. Report it instead by resolving to nothing. + if (this.countDisplayNameClaimants(this.fieldConfigs, header) > 1) { + return null; + } + + // Importable fields resolve before the skipped ones so a header cannot be captured by a field the + // importer ignores that happens to share its display name -- that would drop the column silently, + // since a header resolving to a skipped field is deliberately not reported. + return LABKEY.ext4.Util.resolveFieldNameFromLabel(header, this.fieldConfigs) + || LABKEY.ext4.Util.resolveFieldNameFromLabel(header, this.skippedFieldConfigs); + }, + + findFieldByExactName: function(configs, header){ + const lower = header.toLowerCase(); + + return configs.find((f) => { + return Ext4.isString(f.name) && f.name.toLowerCase() === lower; + }); + }, + + // Counts the fields answering to this header by one of the names the helper treats as identifying. Import + // aliases are deliberately excluded: the helper does gather every alias match and rejects an ambiguous one + // on its own, so only the display names need counting here. + countDisplayNameClaimants: function(configs, header){ + const lower = header.toLowerCase(); + + return configs.filter((f) => { + return [f.name, f.label, f.caption, f.shortCaption].some((n) => { + return Ext4.isString(n) && n.toLowerCase() === lower; + }); + }).length; + }, + + // Maps each field named by the header row to the column holding its values, and reports any header that + // resolves to no field or to a field another header already claimed -- either way a column would be read + // from the wrong place or not at all. Keyed on the resolved field name rather than the pasted text, so + // this is the single answer to "which column holds this field" that processRow() then reads. + // + // Reported without a row number, since each is a property of the header row as a whole. + buildHeaderMap: function(headers, errors){ + const map = {}; + const claimedBy = {}; + + Ext4.each(headers, function(header, idx){ + const text = Ext4.isString(header) ? Ext4.String.trim(header) : ''; + // A spreadsheet copy routinely leaves empty cells past the last real column. + if (!text) { + return; + } + + const encoded = Ext4.util.Format.htmlEncode(text); + const fieldName = this.resolveHeaderToFieldName(text); + if (!fieldName) { + // The helper reports nothing both for a header no field claims and for an alias several + // fields share, so the message cannot promise which of the two it was. + errors.push('Unrecognized column: ' + encoded + '. It matches no field in this form, or matches more than one.'); + return; + } + + if (Object.prototype.hasOwnProperty.call(claimedBy, fieldName)) { + errors.push('Duplicate column: ' + encoded + ' names the same field as ' + Ext4.util.Format.htmlEncode(claimedBy[fieldName]) + '. Remove one so it is unambiguous which is imported.'); + return; + } + + claimedBy[fieldName] = text; + map[fieldName] = idx; + }, this); + + if (!Object.keys(map).length) { + // Every cell of an entirely blank header row was skipped above without comment, so say what is + // wrong rather than reporting a missing value on all 250 rows that follow. + if (!errors.length) { + errors.push('The first row must name the columns being imported.'); + } + + return map; + } + + // A required field with no column at all is a property of the header row, not of each row, so report it + // once here rather than as a missing value repeated down every row. + Ext4.each(this.requiredFieldNames, function(fieldName){ + if (this.getFieldIndex(map, fieldName) === -1) { + errors.push('Missing required column: ' + Ext4.util.Format.htmlEncode(fieldName) + '.'); + } + }, this); + + return map; + }, + + getFieldIndex: function(headerMap, fieldName){ + return Object.prototype.hasOwnProperty.call(headerMap, fieldName) ? headerMap[fieldName] : -1; + }, + + // How wide a data row has to be: far enough to reach the last column a REQUIRED field was mapped to. + // Measuring against every mapped column instead would reject a row whose only absent cells were empty in + // the source anyway, which is an ordinary shape -- the last row of a spreadsheet selection often leaves an + // optional trailing cell blank. + getRequiredColumnCount: function(headerMap){ + return this.requiredFieldNames.reduce((count, fieldName) => { + const idx = this.getFieldIndex(headerMap, fieldName); + + return idx === -1 ? count : Math.max(count, idx + 1); + }, 0); + }, + + resolveDate: function(fieldName, value, errors, rowIdx){ + value = this.normalizeValue(value); + + const parsed = LDK.ConvertUtils.parseDate(value); + + // parseDate returns null for anything it cannot match. Report it rather than letting the + // column silently arrive empty, which only surfaces at all when the field is required. + if (Ext4.isEmpty(parsed) && !Ext4.isEmpty(value)) { + errors.push('Row ' + rowIdx + ': unable to parse date for ' + fieldName + ': ' + Ext4.util.Format.htmlEncode(value)); + } + + return parsed; + }, + + resolveLookup: function(field, value, errors, rowIdx){ if (!field || !field.lookup) return value; @@ -174,30 +343,90 @@ Ext4.define('EHR.window.FormBulkAddWindow', { } } - const lookupRecord = field.lookup.store.findRecord(field.lookup.displayColumn, value); - if (lookupRecord) { - return lookupRecord.data[field.lookup.keyColumn]; + value = this.normalizeValue(value); + if (Ext4.isEmpty(value)) { + return value; } + // Some client-side lookup configs declare only a key column, so neither of these is guaranteed. + // With nothing to match on, pass the value through as the missing-store case above does. + const columns = Ext4.Array.unique([field.lookup.displayColumn, field.lookup.keyColumn]).filter(function(c){ + return !Ext4.isEmpty(c); + }); + if (!columns.length) { + return value; + } + + // A store holding no records cannot resolve anything, so say so rather than writing the raw text + // through as though it had resolved. getCount() on its own is not a load-state check -- it reads + // 0 both while records are still in flight and for a store that loaded no rows, and neither can + // produce a match. Reported without a row number so every row's copy collapses to one line. + if (field.lookup.store.isLoading() || !field.lookup.store.getCount()) { + errors.push('No lookup values are loaded for ' + field.name + '. Please retry the import.'); + return value; + } + + // findRecord(fieldName, value, startIndex, anyMatch, caseSensitive, exactMatch) defaults to a + // PREFIX match, which silently resolves a code to any record whose display value merely starts + // with it -- e.g. source 'LABS' matched the record whose meaning is 'LABSINDO' and stored + // 'LABSINDO'. Require an exact (still case-insensitive) match. + // + // Try the display value first, then the key. A pasted cell legitimately holds either, and before + // an exact match was required a pasted key still resolved by falling through as raw text. + // EHR.form.field.ProjectEntryField resolves the same two ways. + for (let i = 0; i < columns.length; i++) { + const lookupRecord = field.lookup.store.findRecord(columns[i], value, 0, false, false, true); + if (lookupRecord) { + return lookupRecord.data[field.lookup.keyColumn]; + } + } + + // Report rather than silently storing the raw text in place of the lookup's key. + errors.push('Row ' + rowIdx + ': unrecognized value for ' + field.name + ': ' + Ext4.util.Format.htmlEncode(value)); + return value; }, - processRow: function(headers, row, errors, rowIdx){ - const obj = { - Id: this.upperCaseAnimalId ? row[headers.indexOf('Id')].toUpperCase() : row[headers.indexOf('Id')], - date: LDK.ConvertUtils.parseDate(row[headers.indexOf('date')]), - project: this.resolveProjectByName(row[headers.indexOf('project')], errors, rowIdx) + processRow: function(headerMap, row, errors, rowIdx){ + const obj = {}; + + // Seeded only when the column was actually pasted, so that a field this method does not handle is + // left for the loop below. + const idIdx = this.getFieldIndex(headerMap, 'Id'); + if (idIdx !== -1) { + // A row shorter than the header row leaves this undefined, which checkRequired() reports. + obj.Id = this.upperCaseAnimalId && Ext4.isString(row[idIdx]) ? row[idIdx].toUpperCase() : row[idIdx]; + } + + const dateIdx = this.getFieldIndex(headerMap, 'date'); + if (dateIdx !== -1) { + obj.date = this.resolveDate('date', row[dateIdx], errors, rowIdx); + } + + const projectIdx = this.getFieldIndex(headerMap, 'project'); + if (projectIdx !== -1) { + obj.project = this.resolveProjectByName(row[projectIdx], errors, rowIdx); } Ext4.each(this.fieldConfigs, function(field) { - if (!obj[field.name]) { - let index = headers.indexOf(field.name); - if (index === -1 && field.importAliases?.[0]) { - index = headers.indexOf(field.importAliases?.[0]); - } - if (index !== -1) { - obj[field.name] = this.resolveLookup(field, row[index], errors); - } + // Skip by name rather than by truthiness: a column handled above whose value came back null -- + // an unknown project, an unparsable date -- must not be resolved a second time here, or the one + // cell is reported twice by two paths that match on different columns and disagree. + if (obj.hasOwnProperty(field.name)) { + return; + } + + // Every spelling the header row might have used was resolved to a field name up front, so a + // label or any import alias the user pasted is already accounted for by this one lookup. + const index = this.getFieldIndex(headerMap, field.name); + if (index !== -1) { + // Every date column needs parseDate, not just the one named 'date'. Left to the + // raw string, an Ext date field applies JS new Date() semantics, and ES5+ parses a + // bare yyyy-MM-dd as UTC -- so '1965-04-01' lands as the previous day in any + // negative-offset timezone, and '1970-01-01' becomes 0 and is discarded as empty. + obj[field.name] = field.jsonType === 'date' + ? this.resolveDate(field.name, row[index], errors, rowIdx) + : this.resolveLookup(field, row[index], errors, rowIdx); } }, this); @@ -221,15 +450,27 @@ Ext4.define('EHR.window.FormBulkAddWindow', { }, resolveProjectByName: function(projectName, errors, rowIdx){ + projectName = this.normalizeValue(projectName); if (!projectName){ return null; } - projectName = Ext4.String.leftPad(projectName, 4, '0'); + // Same load-state check as resolveLookup(): an unloaded store matches nothing, and blaming the + // pasted value for that sends the user looking for a problem their source file does not have. The + // store is autoLoad, so a submit can race it. Reported without a row number so every row's copy + // collapses to one line. + if (this.projectStore.isLoading() || !this.projectStore.getCount()){ + errors.push('No projects are loaded yet. Please retry the import.'); + return null; + } - const recIdx = this.projectStore.find('name', projectName); + // find() shares findRecord()'s prefix-match default, so '0123' would otherwise resolve to an + // unrelated project named '01234'. Require an exact (still case-insensitive) match. + const recIdx = this.projectStore.find('name', Ext4.String.leftPad(projectName, 4, '0'), 0, false, false, true); if (recIdx === -1){ - errors.push('Row ' + rowIdx + ': unknown project ' + projectName); + // Echo what was pasted rather than the zero-padded form, so the message names something the + // user can find in their source file. + errors.push('Row ' + rowIdx + ': unknown project ' + Ext4.util.Format.htmlEncode(projectName)); return null; }