Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/importer/nunjucks/importer/macros/table_view.njk
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ Renders a table view using the provided parameters to determine what is being sh
{% for rowObj in params.rows %}
<tr class="govuk-table__row {% if importerRowHasHeader(rowObj) %}
validation-error{%endif%}">


{% if params.showRowNumbers %}
<th scope="row" class="rowIndex">{{ rowObj.index }}</th>
{% endif %}
Expand Down
12 changes: 7 additions & 5 deletions lib/importer/src/dudk/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const assert = require('node:assert').strict;
const attributeTypes = require('./types/attribute-types');
const guesser = require('./types/guesser');
const store = require("./session_store")
const errorTypes = require('./util/errors');

// An implementation of the interface described in https://struct.register-dynamics.co.uk/trac/wiki/DataImporter/API

Expand Down Expand Up @@ -625,6 +626,7 @@ exports.SessionDelete = (sid) => {
dimensionsCache.delete(sid);
}


class JobResult {
constructor(records, errors, warnings) {
this.records = records;
Expand Down Expand Up @@ -697,7 +699,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
}

if (row.length < expectedColumnCount) {
warnings.push({row: rowIdx, field: "", message: "Row has fewer columns than expected, wanted " + expectedColumnCount + " but found " + row.length})
warnings.push({row: rowIdx, field: "", type: errorTypes.ValidationError.ShortRow({ expected: expectedColumnCount, actual: row.length})});
}

attrMap.forEach((element) => {
Expand Down Expand Up @@ -736,7 +738,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
const result = attrType(inputVal, {name: attr});

result.warnings.forEach((text) => {
warnings.push({ row: rowIdx, field: attr, message: text });
warnings.push({ row: rowIdx, field: attr, type: text });
});

if (result.valid) {
Expand All @@ -749,9 +751,9 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
result.errors.forEach((text) => {
rowErrorCount += 1;
if (includeErrorRow) {
errors.push({ row: rowIdx, field: attr, message: text, data: row });
errors.push({ row: rowIdx, field: attr, type: text, data: row });
} else {
errors.push({ row: rowIdx, field: attr, message: text });
errors.push({ row: rowIdx, field: attr, type: text });
}

});
Expand All @@ -762,7 +764,7 @@ exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false
records.push(mappedRecord);
}
} else {
warnings.push({ row: rowIdx, message: "Row is empty" })
warnings.push({ row: rowIdx, type: errorTypes.ValidationError.EmptyRow({}) })
}
}

Expand Down
14 changes: 7 additions & 7 deletions lib/importer/src/dudk/backend.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -571,15 +571,15 @@ test('validation', () => {
});

const warnings = backend.JobGetWarnings(sid, jid); // Weight is optional so errors here become warnings
expect(warnings).toStrictEqual([
{ "row": 4, "message": 'Row is empty' },
{ "row": 8, "field": "Weight", "message": "This is not a valid number" }]);
expect(warnings).toMatchObject([
{ "row": 4, "type": {variant: "EmptyRow"} },
{ "row": 8, "field": "Weight", "type": {variant: "IncorrectType"} }]);

const errors = backend.JobGetErrors(sid, jid);
expect(errors).toStrictEqual([
{ "row": 6, "field": 'Age', "message": "This is not a valid number" },
{ "row": 7, "field": 'Name', "message": "'Name' must be supplied" },
{ "row": 7, "field": 'Age', "message": "This is not a valid number" }
expect(errors).toMatchObject([
{ "row": 6, "field": 'Age', "type": { variant: "IncorrectType"} },
{ "row": 7, "field": 'Name', "type": { variant: "FieldRequired"} },
{ "row": 7, "field": 'Age', "type": { variant: "IncorrectType"} }
]);

const allRecords = backend.JobGetRecords(sid, jid, 0, 3);
Expand Down
32 changes: 19 additions & 13 deletions lib/importer/src/dudk/sheets.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,14 +318,14 @@ exports.MapData = (sid, sheet, mapping, fields, previewLimit = DEFAULT_PREVIEW_L

// Remaps the errors/warnings structure from
// [
// { row: [{index:1, row:[]}], field: "A", message: "An error occurred" },
// { row: [{index:1, row:[]}], field: "B", message: "An error occurred" },
// { row: [{index:2, row:[]}], field: "B", message: "An error occurred" }
// { row: [{index:1, row:[]}], field: "A", type: {variant: ..., value: ..., type: ...} },
// { row: [{index:1, row:[]}], field: "B", type: {variant: ..., value: ..., type: ...} },
// { row: [{index:2, row:[]}], field: "B", type: {variant: ..., value: ..., type: ...} }
// ]
// to
// [
// {
// message: "An error occurred",
// type: {variant: ..., value: ..., type: ...},
// meta: {first: 1, count: 2}
// fields: [ "A", "B"] },
// rows: [[{index:1, row:[]}], [{index:2, row:[]}]] },
Expand All @@ -346,14 +346,17 @@ const remapErrorStructure = (errors, row_data, mappingIndices, headerRangeOffset
return row.some(cell => cell && cell.error);
}


for (const error of errors) {
if (!messageMap.has(error.message)) {
messageMap.set(error.message, new Map());
const key = `${error.type.variant}-${error.field}`;

if (!messageMap.has(key)) {
messageMap.set(key, new Map());
}

const errorObj = messageMap.get(error.message);
errorObj.message = error.message;
const errorObj = messageMap.get(key);
errorObj.type = error.type;
errorObj.type.field = error.field;

if( !errorObj.fields) {
errorObj.fields = new Set();
}
Expand All @@ -370,11 +373,13 @@ const remapErrorStructure = (errors, row_data, mappingIndices, headerRangeOffset
}

if (row_data && Array.isArray(row_data) && realErrorIndex < row_data.length ) {

let previousRow = structuredClone(row_data[realErrorIndex - 1]);
let currentRow = structuredClone(row_data[realErrorIndex]);
let nextRow = structuredClone(row_data[realErrorIndex + 1]);

// currentRow is the error and so should have the error flag
currentRow.error = true;

if (previousRow) {
if (!rowHasError(errorObj.rows.get(previousRow.index)?.row)) {
errorObj.rows.set(previousRow.index, previousRow);
Expand All @@ -400,12 +405,13 @@ const remapErrorStructure = (errors, row_data, mappingIndices, headerRangeOffset
}

// Convert the nested maps/sets to the desired array structure
return Array.from(messageMap.entries().map(([message, errorObj]) => {
return Array.from(messageMap.values().map((errorObj) => {
// Some rows may not have any errors because they are for surrounding context,
// so we need to filter them out for the meta info
const errorRows = Array.from(errorObj.rows.values()).filter(rowObj => rowObj.row.some(element=>element && element.error) );
const errorRows = Array.from(errorObj.rows.values()).filter(rowObj => rowObj.error);

return {
message: message,
type: errorObj.type,
meta: {
first: errorRows[0]?.index || 0,
count: errorRows?.length || 0
Expand Down
76 changes: 39 additions & 37 deletions lib/importer/src/dudk/sheets.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ test('trailing columns', () => {
describe('remapErrorStructure', () => {
test('groups errors by message, collects rows', () => {
const errors = [
{ row: 1, field: "A", message: "Error 1" },
{ row: 1, field: "B", message: "Error 2" },
{ row: 2, field: "B", message: "Error 2" },
{ row: 3, field: "A", message: "Error 1" },
{ row: 1, field: "A", type: {variant: "IncorrectType"} },
{ row: 1, field: "B", type: {variant: "IncorrectType"} },
{ row: 2, field: "B", type: {variant: "IncorrectType"} },
{ row: 3, field: "A", type: {variant: "IncorrectType"} },
];
// Adjusted expected result to match grouping by error, then by field
const row_data = [
Expand All @@ -38,30 +38,32 @@ describe('remapErrorStructure', () => {
const result = sheets_lib.RemapErrorStructure(errors, row_data, RewrittenMapping);
expect(result).toEqual([
{
message: "Error 1",
type: {variant: "IncorrectType", field: "A"},
fields: [ "A"],
meta: { first: 1, count: 2 },
// Only 1 error (in the two rows) because A appears twice but in
// rows 1 and 3 and here we have rows 1 and 2
meta: { first: 1, count: 1 },
rows: [
{index:1, row:[{value:"0", error: true},{value:"1", error: true}]},
{index:1, error: true, row:[{value:"0", error: true},{value:"1", error: true}]},
{index:2, row:[{value:"2", error: true}, {value:"3", error: true}]}
]
},
{
message: "Error 2",
type: {variant: "IncorrectType", field: "B"},
fields: ["B"],
meta: { first: 1, count: 2 },
rows: [
{index:1, row: [{value:"0", error: true},{value:"1", error: true}]},
{index:2, row: [{value:"2", error: true}, {value:"3", error: true}]}
{index:1, error: true, row: [{value:"0", error: true},{value:"1", error: true}]},
{index:2, error: true, row: [{value:"2", error: true}, {value:"3", error: true}]}
]
}
]);
});

test('with row_data, includes adjacent rows', () => {
const errors = [
{ row: 1, field: "A", message: "Error 1" },
{ row: 2, field: "A", message: "Error 1" },
{ row: 1, field: "A", type: "Error 1" },
{ row: 2, field: "A", type: "Error 1" },
];
const row_data = [
{index: 1, row: [{value:"0", error: true},{value:"1"}]},
Expand All @@ -70,21 +72,21 @@ describe('remapErrorStructure', () => {
const result = sheets_lib.RemapErrorStructure(errors, row_data, RewrittenMapping);
expect(result).toEqual([
{
message: "Error 1",
type: "Error 1",
fields: ["A"],
meta: {first: 1, count: 2},
rows: [
{index: 1, row: [{value:"0", error: true},{value:"1"}]},
{index: 2, row: [{value:"2", error: true}, {value:"3"}]}
{index: 1, error: true, row: [{value:"0", error: true},{value:"1"}]},
{index: 2, error: true, row: [{value:"2", error: true}, {value:"3"}]}
]
}
]);
});

test('does not add out-of-bounds adjacent rows', () => {
const errors = [
{ row: 1, field: "A", message: "Error 1" },
{ row: 3, field: "A", message: "Error 1" },
{ row: 1, field: "A", type: "Error 1" },
{ row: 3, field: "A", type: "Error 1" },
];
const row_data = [
{index: 1, row: [{value:"0", error: true},{value:"1"}]},
Expand All @@ -96,11 +98,11 @@ describe('remapErrorStructure', () => {
// For row 3: 2, 3
expect(result).toEqual([
{
message: "Error 1",
type: "Error 1",
fields: ["A"],
meta: { first: 1, count: 1 },
rows: [
{index: 1, row: [{value:"0", error: true},{value:"1"}]},
{index: 1, error: true, row: [{value:"0", error: true},{value:"1"}]},
{index: 2, row: [{value:"2"},{value:"3"}]}
]
}
Expand All @@ -115,23 +117,23 @@ describe('remapErrorStructure', () => {

test('ignores errors with missing row in row_data', () => {
const errors = [
{ row: 5, field: "A", message: "Error 1" }
{ row: 5, field: "A", type: "Error 1" }
];
const row_data = [
{index: 1, row: [{value:"0"},{value:"1"}]},
{index: 2, row: [{value:"2"}, {value:"3"}]}
];
const result = sheets_lib.RemapErrorStructure(errors, row_data, RewrittenMapping);
expect(result).toEqual([
{ message: "Error 1", meta: { count: 0, first: 0}, fields: ["A"], rows: [] }
{ type: "Error 1", meta: { count: 0, first: 0}, fields: ["A"], rows: [] }
]);
});

test('remapping errors - missing fields', () => {
const errors = [
{ row: 2, field: 'Salary', message: "'Salary' must be supplied" },
{ row: 3, field: 'Salary', message: "'Salary' must be supplied" },
{ row: 5, field: 'Salary', message: "'Salary' must be supplied" }
{ row: 2, field: 'Salary', type: { variant: "FieldRequired" } },
{ row: 3, field: 'Salary', type: { variant: "FieldRequired" } },
{ row: 5, field: 'Salary', type: { variant: "FieldRequired" } }
] ;
const row_data =
[
Expand All @@ -157,16 +159,16 @@ describe('remapErrorStructure', () => {
const result = sheets_lib.RemapErrorStructure(errors, row_data, mappingIndices, headerRangeOffset);
expect(result).toEqual([
{
message: "'Salary' must be supplied",
type: {variant: "FieldRequired", field: "Salary"},
fields: ['Salary'],
meta: { first: 3, count: 3 },
rows: [
// errors in index 3, 5 and 6
{ index: 2, row: [{value: "2"}, {value: "Mr"}, {value: "Odie"}, {value: "Arbuckle"}, {value: "2025-02-01"}, {value: "123" }, {value: "12%"}, {value: "2025-04-01"}]},
{ index: 3, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 4, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 3, error: true, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 4, error: true, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 5, row: [{value: "2"}, {value: "Mr"}, {value: "Odie"}, {value: "Arbuckle"}, {value: "2025-02-01"}, {value: "123"}, {value: "12%"}, {value: "2025-04-01"}]},
{ index: 6, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 6, error: true, row: [{ value: "1" }, { value: "Mr" }, { value: "Jon" }, { value: "Arbuckle" }, { value: "2025-01-01" }, { value: " ", error: true}, undefined, { value: "2025-04-01" }] },
{ index: 7, row: [{value: "2"}, {value: "Mr"}, {value: "Odie"}, {value: "Arbuckle"}, {value: "2025-02-01"}, {value: "123"}, {value: "12%"}, {value: "2025-04-01"}]}
]
}
Expand All @@ -176,12 +178,12 @@ describe('remapErrorStructure', () => {

test('remapping errors - multiple errors', () => {
const errors = [
{ row: 3, field: 'Salary', message: "'Salary' must be supplied" },
{ row: 4, field: 'Salary', message: 'This is not a valid number' },
{ row: 5, field: 'Salary', message: "'Salary' must be supplied" },
{ row: 6, field: 'Salary', message: 'This is not a valid number' },
{ row: 7, field: 'Salary', message: "'Salary' must be supplied" },
{ row: 8, field: 'Salary', message: "'Salary' must be supplied" }
{ row: 3, field: 'Salary', type: {variant: "FieldRequired"} },
{ row: 4, field: 'Salary', type: {variant: "IncorrectType"} },
{ row: 5, field: 'Salary', type: {variant: "FieldRequired"} },
{ row: 6, field: 'Salary', type: {variant: "IncorrectType"} },
{ row: 7, field: 'Salary', type: {variant: "FieldRequired"} },
{ row: 8, field: 'Salary', type: {variant: "FieldRequired"} }
] ;
const row_data = [
{ index: 4, row: [
Expand All @@ -207,9 +209,9 @@ describe('remapErrorStructure', () => {
const headerRangeOffset = 3 ;

const result = sheets_lib.RemapErrorStructure(errors, row_data, mappingIndices, headerRangeOffset);
expect(result).toEqual([
expect(result).toMatchObject([
{
message: "'Salary' must be supplied",
type: { variant: "FieldRequired" },
meta: { first: 4, count: 4 },
fields: [ 'Salary' ],
rows:[
Expand All @@ -222,7 +224,7 @@ describe('remapErrorStructure', () => {
]
},
{
message: 'This is not a valid number',
type: { variant: "IncorrectType" },
meta: { first: 5, count: 2 },
fields: [ 'Salary' ],
rows: [
Expand Down
5 changes: 3 additions & 2 deletions lib/importer/src/dudk/types/attribute-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// An attribute type is a function from an input value to a result.

const ati = require('./attribute-type-internals');
const errors = require('../util/errors')

// Create an optional version of an existing type, that allows empty strings or
// undefined values and maps then to the empty mapping, and converts any validation errors to warnings
Expand Down Expand Up @@ -32,7 +33,7 @@ exports.requiredType = (baseType, field) => {
return baseType(inputValue);
}
else {
return ati.failedMapping([], [`'${field.name}' must be supplied`]);
return ati.failedMapping([], [errors.ValidationError.FieldRequired({value: inputValue?.v, type: field.type})]);
}
};
}
Expand All @@ -46,7 +47,7 @@ exports.basicStringType = (inputCell) => {
exports.basicNumberType = (inputCell) => {
const result = parseFloat(inputCell.v);
if (isNaN(result)) {
return ati.failedMapping([], ["This is not a valid number"]);
return ati.failedMapping([], [errors.ValidationError.IncorrectType({ value: inputCell.v, type: "number"})]);
} else {
return ati.successfulMapping(result);
}
Expand Down
Loading
Loading