From bf319d18310801217b79db72228efd2846c331b8 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Sun, 26 Jul 2026 20:30:12 -0600 Subject: [PATCH 1/7] Fix bulk add parsing dates as UTC and lookups by prefix Date columns other than the form's primary date field were read as UTC, so imported dates landed a day early or vanished when they hit the epoch, and lookup values matched on a prefix instead of exactly, letting one code silently store another. --- ehr/resources/web/ehr/window/FormBulkAddWindow.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 84244bbe3..14018ad4d 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -174,7 +174,11 @@ Ext4.define('EHR.window.FormBulkAddWindow', { } } - const lookupRecord = field.lookup.store.findRecord(field.lookup.displayColumn, 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. + const lookupRecord = field.lookup.store.findRecord(field.lookup.displayColumn, value, 0, false, false, true); if (lookupRecord) { return lookupRecord.data[field.lookup.keyColumn]; } @@ -196,7 +200,13 @@ Ext4.define('EHR.window.FormBulkAddWindow', { index = headers.indexOf(field.importAliases?.[0]); } if (index !== -1) { - obj[field.name] = this.resolveLookup(field, row[index], errors); + // 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' + ? LDK.ConvertUtils.parseDate(row[index]) + : this.resolveLookup(field, row[index], errors); } } }, this); From 96721c5515446140169f4d1c89ccadb6ce628c23 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 05:35:46 -0600 Subject: [PATCH 2/7] Report unresolved bulk add values and match projects exactly The bulk add importer silently stored raw text for unmatched lookups and dropped unparseable dates, and resolved project names by prefix. All three now surface a per-row error. --- .../web/ehr/window/FormBulkAddWindow.js | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 14018ad4d..c78ce165c 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -159,7 +159,19 @@ Ext4.define('EHR.window.FormBulkAddWindow', { this.close(); }, - resolveLookup: function(field, value, errors){ + resolveDate: function(field, value, errors, rowIdx){ + 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 ' + field.name + ': ' + value); + } + + return parsed; + }, + + resolveLookup: function(field, value, errors, rowIdx){ if (!field || !field.lookup) return value; @@ -183,6 +195,12 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return lookupRecord.data[field.lookup.keyColumn]; } + // Report rather than silently storing the raw text in place of the lookup's key. Skip an + // unloaded store, which would otherwise fail every row while its records are still in flight. + if (!Ext4.isEmpty(value) && field.lookup.store.getCount()) { + errors.push('Row ' + rowIdx + ': unrecognized value for ' + field.name + ': ' + value); + } + return value; }, @@ -205,8 +223,8 @@ Ext4.define('EHR.window.FormBulkAddWindow', { // 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' - ? LDK.ConvertUtils.parseDate(row[index]) - : this.resolveLookup(field, row[index], errors); + ? this.resolveDate(field, row[index], errors, rowIdx) + : this.resolveLookup(field, row[index], errors, rowIdx); } } }, this); @@ -237,7 +255,9 @@ Ext4.define('EHR.window.FormBulkAddWindow', { projectName = Ext4.String.leftPad(projectName, 4, '0'); - 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', projectName, 0, false, false, true); if (recIdx === -1){ errors.push('Row ' + rowIdx + ': unknown project ' + projectName); return null; From 12b112276beacbfa598f2a440244e1a78a458390 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 05:36:49 -0600 Subject: [PATCH 3/7] Stop new rows collapsing when the key is server-assigned Rows keyed solely on a server-assigned identity column have an empty key before save, so every new row after the first resolved to the first and collapsed onto it. Such tables could not add more than one row per save. --- ehr/resources/web/ehr/data/DataEntryServerStore.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) 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); From 858274dd226351d4eb818cd542034bb48c7ffceb Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 06:05:12 -0600 Subject: [PATCH 4/7] Escape, trim, and key-match bulk add values Error messages rendered pasted cell values as HTML, a lookup store with no records loaded silently wrote raw text through instead of reporting it, and exact matching rejected whitespace-padded cells and pasted key values that previously imported. --- .../web/ehr/window/FormBulkAddWindow.js | 59 +++++++++++++++---- 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index c78ce165c..8173031fb 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -148,7 +148,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,13 +161,21 @@ Ext4.define('EHR.window.FormBulkAddWindow', { this.close(); }, + // 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; + }, + resolveDate: function(field, 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 ' + field.name + ': ' + value); + errors.push('Row ' + rowIdx + ': unable to parse date for ' + field.name + ': ' + Ext4.util.Format.htmlEncode(value)); } return parsed; @@ -186,20 +196,46 @@ Ext4.define('EHR.window.FormBulkAddWindow', { } } + 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. - const lookupRecord = field.lookup.store.findRecord(field.lookup.displayColumn, value, 0, false, false, true); - if (lookupRecord) { - return lookupRecord.data[field.lookup.keyColumn]; + // + // 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. Skip an - // unloaded store, which would otherwise fail every row while its records are still in flight. - if (!Ext4.isEmpty(value) && field.lookup.store.getCount()) { - errors.push('Row ' + rowIdx + ': unrecognized value for ' + field.name + ': ' + value); - } + // 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; }, @@ -249,6 +285,7 @@ Ext4.define('EHR.window.FormBulkAddWindow', { }, resolveProjectByName: function(projectName, errors, rowIdx){ + projectName = this.normalizeValue(projectName); if (!projectName){ return null; } @@ -259,7 +296,7 @@ Ext4.define('EHR.window.FormBulkAddWindow', { // unrelated project named '01234'. Require an exact (still case-insensitive) match. const recIdx = this.projectStore.find('name', projectName, 0, false, false, true); if (recIdx === -1){ - errors.push('Row ' + rowIdx + ': unknown project ' + projectName); + errors.push('Row ' + rowIdx + ': unknown project ' + Ext4.util.Format.htmlEncode(projectName)); return null; } From 544018e54f6dd2d4685161f9c2f55d1a8866b26f Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 06:32:37 -0600 Subject: [PATCH 5/7] Match bulk add headers loosely and resolve each column once Header cells were matched exactly and case-sensitively, so a padded or recased header silently dropped its whole column; they are now normalized, and a header matching no field, or two naming the same one, is reported. Columns handled before the field loop are skipped by name rather than truthiness so a failed project or date is not resolved a second time by a path that disagrees, and unknown-project errors echo the pasted value instead of its zero-padded form. --- .../web/ehr/window/FormBulkAddWindow.js | 172 ++++++++++++++---- 1 file changed, 138 insertions(+), 34 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 8173031fb..7a2e1813b 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -55,6 +55,24 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return f.name; }); + // A header matching no field silently drops its whole column, so keep the full set of recognized + // names to check the pasted header row against. Built from allConfigs rather than fieldConfigs so + // the fields the importer itself skips (hidden, taskid, qcstate) count as known but not importable + // and are passed over without complaint. + this.knownHeaders = {}; + const addKnownHeader = (name) => { + const key = this.normalizeHeader(name); + if (key) { + this.knownHeaders[key] = true; + } + }; + allConfigs.forEach((f) => { + addKnownHeader(f.name); + (Ext4.isArray(f.importAliases) ? f.importAliases : []).forEach(addKnownHeader); + }); + // processRow() handles these directly, whether or not the section declares them. + ['Id', 'date', 'project'].forEach(addKnownHeader); + 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;' @@ -130,17 +148,24 @@ 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]); + this.checkHeaders(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) { + 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(headerMap, row, errors, i); + if (newRow) { + records.push(this.targetStore.createModel(newRow)); + } } } @@ -167,7 +192,58 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return Ext4.isString(value) ? Ext4.String.trim(value) : value; }, - resolveDate: function(field, value, errors, rowIdx){ + // Header cells carry the same stray whitespace and casing drift as any other pasted cell, and a header + // that fails to match drops its whole column, so normalize both sides before comparing them. + normalizeHeader: function(header){ + return Ext4.isString(header) ? Ext4.String.trim(header).toLowerCase() : ''; + }, + + // Maps each normalized header to its column index, first occurrence winning as indexOf() did. Keyed on + // pasted text, so membership is tested against own properties only rather than anything Object supplies. + buildHeaderMap: function(headers){ + const map = {}; + Ext4.each(headers, function(header, idx){ + const key = this.normalizeHeader(header); + if (key && !Object.prototype.hasOwnProperty.call(map, key)) { + map[key] = idx; + } + }, this); + + return map; + }, + + getHeaderIndex: function(headerMap, name){ + const key = this.normalizeHeader(name); + + return key && Object.prototype.hasOwnProperty.call(headerMap, key) ? headerMap[key] : -1; + }, + + // A header the form does not recognize, or two headers naming the same field, means a column is either + // dropped or read from the wrong place. Report both rather than importing part of the source silently. + // Reported without a row number, since each is a property of the header row as a whole. + checkHeaders: function(headers, errors){ + const seen = {}; + Ext4.each(headers, function(header){ + const key = this.normalizeHeader(header); + // Trailing empty cells are a routine artifact of a spreadsheet copy and name no column. + if (!key) { + return; + } + + const encoded = Ext4.util.Format.htmlEncode(Ext4.String.trim(header)); + if (seen[key] === true) { + errors.push('Duplicate column: ' + encoded + '. Remove one so it is unambiguous which is imported.'); + } + else { + seen[key] = true; + if (this.knownHeaders[key] !== true) { + errors.push('Unrecognized column: ' + encoded + '. It matches no field in this form and would not be imported.'); + } + } + }, this); + }, + + resolveDate: function(fieldName, value, errors, rowIdx){ value = this.normalizeValue(value); const parsed = LDK.ConvertUtils.parseDate(value); @@ -175,7 +251,7 @@ Ext4.define('EHR.window.FormBulkAddWindow', { // 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 ' + field.name + ': ' + Ext4.util.Format.htmlEncode(value)); + errors.push('Row ' + rowIdx + ': unable to parse date for ' + fieldName + ': ' + Ext4.util.Format.htmlEncode(value)); } return parsed; @@ -240,28 +316,47 @@ Ext4.define('EHR.window.FormBulkAddWindow', { 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 to match on its own name or import alias. + const idIdx = this.getHeaderIndex(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.getHeaderIndex(headerMap, 'date'); + if (dateIdx !== -1) { + obj.date = this.resolveDate('date', row[dateIdx], errors, rowIdx); + } + + const projectIdx = this.getHeaderIndex(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) { - // 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, row[index], errors, rowIdx) - : this.resolveLookup(field, row[index], errors, rowIdx); - } + // 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; + } + + let index = this.getHeaderIndex(headerMap, field.name); + if (index === -1 && field.importAliases?.[0]) { + index = this.getHeaderIndex(headerMap, field.importAliases?.[0]); + } + 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); @@ -290,12 +385,21 @@ Ext4.define('EHR.window.FormBulkAddWindow', { 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; + } // 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', projectName, 0, false, false, true); + const recIdx = this.projectStore.find('name', Ext4.String.leftPad(projectName, 4, '0'), 0, false, false, true); if (recIdx === -1){ + // 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; } From 84c7cf97cfb35074bb2971a2b88e24a2c90f6c2f Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 06:47:22 -0600 Subject: [PATCH 6/7] Resolve bulk add headers with the platform field-name helper Header matching covered only a field's name and first import alias, so a header spelled as the field's label, or as any later alias, resolved to nothing -- and since an unrecognized header now stops the import, the recognized-name set was also vouching for aliases the matcher could not actually place, dropping those columns silently. Resolution now runs through LABKEY.ext4.Util.resolveFieldNameFromLabel for both, keyed on the resolved field so duplicates are caught per field rather than per header spelling. An exact name match is tried first because that helper breaks on its first name/caption/label hit and would otherwise let a field merely labelled the same win on config order. --- .../web/ehr/window/FormBulkAddWindow.js | 122 ++++++++---------- 1 file changed, 57 insertions(+), 65 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 7a2e1813b..62664a5cc 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -55,23 +55,10 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return f.name; }); - // A header matching no field silently drops its whole column, so keep the full set of recognized - // names to check the pasted header row against. Built from allConfigs rather than fieldConfigs so - // the fields the importer itself skips (hidden, taskid, qcstate) count as known but not importable - // and are passed over without complaint. - this.knownHeaders = {}; - const addKnownHeader = (name) => { - const key = this.normalizeHeader(name); - if (key) { - this.knownHeaders[key] = true; - } - }; - allConfigs.forEach((f) => { - addKnownHeader(f.name); - (Ext4.isArray(f.importAliases) ? f.importAliases : []).forEach(addKnownHeader); - }); - // processRow() handles these directly, whether or not the section declares them. - ['Id', 'date', 'project'].forEach(addKnownHeader); + // Headers are resolved against every field the section declares, not just the importable ones, so a + // header naming a field the importer skips (hidden, taskid, qcstate) is recognized and then ignored + // rather than reported as unknown. + this.allFieldConfigs = allConfigs; 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.', @@ -148,8 +135,7 @@ 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 { - const headerMap = this.buildHeaderMap(parsed[0]); - this.checkHeaders(parsed[0], errors); + 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 @@ -192,55 +178,62 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return Ext4.isString(value) ? Ext4.String.trim(value) : value; }, - // Header cells carry the same stray whitespace and casing drift as any other pasted cell, and a header - // that fails to match drops its whole column, so normalize both sides before comparing them. - normalizeHeader: function(header){ - return Ext4.isString(header) ? Ext4.String.trim(header).toLowerCase() : ''; + // 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, label, caption and every import + // alias, and accepts importAliases as either an array or a comma-separated string. Matching only the name + // and the first alias, as this window used to, drops the column for any other spelling. + // + // An exact name match is tried first because the helper stops at its first name/caption/label hit, so a + // field whose LABEL matches this header could otherwise win over the field this header actually names, + // purely on config order. Only the broader spellings are left to the helper. + resolveHeaderToFieldName: function(header){ + const exact = this.allFieldConfigs.find((f) => { + return Ext4.isString(f.name) && f.name.toLowerCase() === header.toLowerCase(); + }); + + return exact ? exact.name : LABKEY.ext4.Util.resolveFieldNameFromLabel(header, this.allFieldConfigs); }, - // Maps each normalized header to its column index, first occurrence winning as indexOf() did. Keyed on - // pasted text, so membership is tested against own properties only rather than anything Object supplies. - buildHeaderMap: function(headers){ + // 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 key = this.normalizeHeader(header); - if (key && !Object.prototype.hasOwnProperty.call(map, key)) { - map[key] = 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; } - }, this); - - return map; - }, - - getHeaderIndex: function(headerMap, name){ - const key = this.normalizeHeader(name); - return key && Object.prototype.hasOwnProperty.call(headerMap, key) ? headerMap[key] : -1; - }, - - // A header the form does not recognize, or two headers naming the same field, means a column is either - // dropped or read from the wrong place. Report both rather than importing part of the source silently. - // Reported without a row number, since each is a property of the header row as a whole. - checkHeaders: function(headers, errors){ - const seen = {}; - Ext4.each(headers, function(header){ - const key = this.normalizeHeader(header); - // Trailing empty cells are a routine artifact of a spreadsheet copy and name no column. - if (!key) { + 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; } - const encoded = Ext4.util.Format.htmlEncode(Ext4.String.trim(header)); - if (seen[key] === true) { - errors.push('Duplicate column: ' + encoded + '. Remove one so it is unambiguous which is imported.'); - } - else { - seen[key] = true; - if (this.knownHeaders[key] !== true) { - errors.push('Unrecognized column: ' + encoded + '. It matches no field in this form and would not be imported.'); - } + 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); + + return map; + }, + + getFieldIndex: function(headerMap, fieldName){ + return Object.prototype.hasOwnProperty.call(headerMap, fieldName) ? headerMap[fieldName] : -1; }, resolveDate: function(fieldName, value, errors, rowIdx){ @@ -320,19 +313,19 @@ Ext4.define('EHR.window.FormBulkAddWindow', { 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 to match on its own name or import alias. - const idIdx = this.getHeaderIndex(headerMap, 'Id'); + // 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.getHeaderIndex(headerMap, 'date'); + const dateIdx = this.getFieldIndex(headerMap, 'date'); if (dateIdx !== -1) { obj.date = this.resolveDate('date', row[dateIdx], errors, rowIdx); } - const projectIdx = this.getHeaderIndex(headerMap, 'project'); + const projectIdx = this.getFieldIndex(headerMap, 'project'); if (projectIdx !== -1) { obj.project = this.resolveProjectByName(row[projectIdx], errors, rowIdx); } @@ -345,10 +338,9 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return; } - let index = this.getHeaderIndex(headerMap, field.name); - if (index === -1 && field.importAliases?.[0]) { - index = this.getHeaderIndex(headerMap, field.importAliases?.[0]); - } + // 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 From eec5cc87352d136279a020b14bcea6c8ffc8d78e Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 27 Jul 2026 08:56:37 -0600 Subject: [PATCH 7/7] Keep pasted tabs when trimming and gate rows on required columns Trimming the pasted block with Ext4.String.trim() counted a tab as whitespace, so the last row lost its empty trailing cells and read as truncated even though every value in it was correct; spaces and line endings are still trimmed, since a stray one at either end parses as a junk row or is mistaken for the header row. A row is now measured against the last column a required field maps to rather than the number of required fields, which was unrelated to where those fields sit, and a missing required column or an unusable header row is reported once instead of as a missing value on every row. Header resolution also prefers importable fields for exact names, matching the order already used for display names, so a field the importer skips cannot claim a name another query declares. --- .../web/ehr/window/FormBulkAddWindow.js | 108 +++++++++++++++--- 1 file changed, 93 insertions(+), 15 deletions(-) diff --git a/ehr/resources/web/ehr/window/FormBulkAddWindow.js b/ehr/resources/web/ehr/window/FormBulkAddWindow.js index 62664a5cc..3626fc86e 100644 --- a/ehr/resources/web/ehr/window/FormBulkAddWindow.js +++ b/ehr/resources/web/ehr/window/FormBulkAddWindow.js @@ -55,10 +55,13 @@ Ext4.define('EHR.window.FormBulkAddWindow', { return f.name; }); - // Headers are resolved against every field the section declares, not just the importable ones, so a - // header naming a field the importer skips (hidden, taskid, qcstate) is recognized and then ignored - // rather than reported as unknown. + // 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.', @@ -109,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; @@ -141,10 +150,12 @@ Ext4.define('EHR.window.FormBulkAddWindow', { // 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 < this.requiredFieldNames.length) { - errors.push('Row ' + i + ': not enough items in row'); + if (!row || row.length < columnCount) { + errors.push('Row ' + i + ': not enough items in row -- has ' + (row ? row.length : 0) + ', needs at least ' + columnCount); continue; } @@ -179,19 +190,56 @@ Ext4.define('EHR.window.FormBulkAddWindow', { }, // 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, label, caption and every import - // alias, and accepts importAliases as either an array or a comma-separated string. Matching only the name - // and the first alias, as this window used to, drops the column for any other spelling. + // 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. // - // An exact name match is tried first because the helper stops at its first name/caption/label hit, so a - // field whose LABEL matches this header could otherwise win over the field this header actually names, - // purely on config order. Only the broader spellings are left to the helper. + // 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){ - const exact = this.allFieldConfigs.find((f) => { - return Ext4.isString(f.name) && f.name.toLowerCase() === header.toLowerCase(); + // 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; }); + }, - return exact ? exact.name : LABKEY.ext4.Util.resolveFieldNameFromLabel(header, this.allFieldConfigs); + // 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 @@ -229,6 +277,24 @@ Ext4.define('EHR.window.FormBulkAddWindow', { 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; }, @@ -236,6 +302,18 @@ Ext4.define('EHR.window.FormBulkAddWindow', { 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);