Apply validation functions to mapping. (#192)

Use the attribute-types to ensure that rows are validated according to the type and required fields when performing the validation. This should generate errors and warnings for the review page.

In addition to these changes, the warning/errors are defined such that validation failures are errors, but structural errors (such as missing rows, or mismatched row lengths) are recorded as warnings. For errors, it is now possible to request the row data in code for any errors, which in future will allow the review page to show exactly where the errors occur. To achieve this the column index will need to be available to the frontend in the same way that the row index is, currently there's no simple way to map the column name where the error occurred to the index in the error data (see #199)

Also resolves #191 as it now shows an error summary when a required field is not used in the mapping step.

Author: rossjones

fixtures/validation-struct-err.csv

@@ -0,0 +1,5 @@
+Employee number,Title,First name,Surname,Employment start date,Salary,Contribution percentage,Payment date
+1,Mr,Jon,Arbuckle,2025-01-01,,,2025-04-01
+,,,,,,,,
+,,,,,,,,
+2,Mr,Odie,Arbuckle,2025-02-01,123,12%,2025-04-01

fixtures/validation-test.csv

@@ -0,0 +1,3 @@
+Employee number,Title,First name,Surname,Employment start date,Salary,Contribution percentage,Payment date
+1,Mr,Jon,Arbuckle,2025-01-01,,,2025-04-01
+2,Mr,Odie,Arbuckle,2025-02-01,123,12%,2025-04-01

lib/importer/nunjucks/importer/macros/field_mapper.njk

@@ -17,14 +17,33 @@
 {% macro importerFieldMapper(data, caption='', columnTitle='Column', examplesTitle='Example values', fieldsTitle='Fields') %}
     {% set fields = data['importer.session']['fields'] %}
     {% set headings = importerGetHeaders(data) %}
-    {% set error = headings.error %}
+    {% set headingError = headings.error %}
+    {% set error = importerError(data) %}
 
-    {% if error %}
+    {% if headingError %}
     <p id="mapping-error" class="govuk-error-message">
-      <span class="govuk-visually-hidden">Error:</span> {{ error.text }}
+      <span class="govuk-visually-hidden">Error:</span> {{ headingError.text }}
     </p>
     {% endif %}
 
+    {% if error %}
+      <div class="govuk-error-summary" data-module="govuk-error-summary">
+        <div role="alert">
+          <h2 class="govuk-error-summary__title">
+            {{ error.text }}
+          </h2>
+          <div class="govuk-error-summary__body">
+            <ul class="govuk-list govuk-error-summary__list">
+              {% for e in error.extra %}
+              <li>
+                <a href="#field-{{ e | slugify }}">{{ e }}</a>
+              </li>
+              {% endfor %}
+            </ul>
+          </div>
+        </div>
+      </div>
+    {% endif %}
 
 <table class="govuk-table">
   {% if caption %}
@@ -39,14 +58,17 @@
   </thead>
   <tbody class="govuk-table__body">
     {% for h in headings.data %}
-    <tr class="govuk-table__row">
+      {% set hIndex = loop.index0 %}
+      {% set currentValue = importerErrorMappingData(error, hIndex) %}
+
+    <tr class="govuk-table__row" id="field-{{ h.name | slugify }}">
       <th scope="row" class="govuk-table__header">{{ h.name }}</th>
       <td class="govuk-table__cell">{{ h.examples }}</td>
       <td class="govuk-table__cell govuk-table__cell--numeric">
          <select class="govuk-select" style="float: right;" name="{{h.index}}">
                 <option name=""></option>
                 {% for field in fields %}
-                <option value="{{field.name}}">{{field.name}}</option>
+                    <option value="{{field.name}}" {% if field.name == currentValue %}selected{% endif%}>{{field.name}}</option>
                 {% endfor %}
             </select>
       </td>

lib/importer/src/attribute-types.js

@@ -5,14 +5,13 @@
 // An attribute type is a function from an input value to a result.
 
 class AttributeMappingResult {
-  constructor(value, warnings, errors) {
+  constructor(value, errors) {
     this.value = value;
-    this.warnings = warnings || [];
-    this.errors = errors;
+    this.errors = errors || [];
   }
 
   get valid() {
-    return !this.errors;
+    return this.errors.length == 0;
   }
 
   get empty() {
@@ -22,20 +21,20 @@ class AttributeMappingResult {
 
 // These helpers define the three kinds of results:
 
-function emptyMapping(warnings) {
-  return new AttributeMappingResult(undefined, warnings);
+function emptyMapping(errors) {
+  return new AttributeMappingResult(undefined, errors);
 }
 
-function successfulMapping(outputValue, warnings) {
-  return new AttributeMappingResult(outputValue, warnings);
+function successfulMapping(outputValue, errors) {
+  return new AttributeMappingResult(outputValue, errors);
 }
 
-function failedMapping(warnings, errors) {
-  return new AttributeMappingResult(undefined, warnings, errors);
+function failedMapping(errors) {
+  return new AttributeMappingResult(undefined, errors);
 }
 
 // Create an optional version of an existing type, that allows empty strings or
-// undefined values and maps then to "undefined", and converts any validation errors to warnings
+// undefined values and maps then to "undefined"
 exports.optionalType = (baseType) => {
   return (inputValue) => {
     if (inputValue !== undefined && inputValue != "") {
@@ -44,7 +43,7 @@ exports.optionalType = (baseType) => {
         return result;
       } else {
         // Convert any errors into warnings, as this is an optional field
-        return emptyMapping(result.warnings.concat(result.errors));
+        return emptyMapping(result.errors);
       }
     }
     else {
@@ -60,7 +59,7 @@ exports.requiredType = (baseType) => {
       return baseType(inputValue);
     }
     else {
-      return failedMapping([], ["A value must be provided"]);
+      return failedMapping(["A value must be provided"]);
     }
   };
 }
@@ -74,19 +73,28 @@ exports.basicStringType = (inputValue) => {
 exports.basicNumberType = (inputValue) => {
   const result = parseFloat(inputValue);
   if (isNaN(result)) {
-    return failedMapping([], ["This is not a valid number"]);
+    return failedMapping(["This is not a valid number"]);
   } else {
     return successfulMapping(result);
   }
 };
 
-exports.typeFromName = (name) => {
-  if (!name) return null
+exports.mapperForField = (field) => {
+  if (!field) return null
 
-  switch (name.toLowerCase()) {
+  let m = exports.basicStringType;
+
+  switch (field.type.toLowerCase()) {
     case "number":
-      return exports.basicNumberType
+      m = exports.basicNumberType
+      break
     default:
-      return exports.basicStringType
+      m = exports.basicStringType
+  }
+
+  if (field.required) {
+    return exports.requiredType(m)
   }
+
+  return exports.optionalType(m)
 }

lib/importer/src/attribute-types.test.js

@@ -6,23 +6,21 @@ test('required basic string', () => {
 
   let r = t(undefined);
   expect(r.valid).toBeFalsy();
-  expect(r.warnings).toMatchObject([]);
   expect(r.errors).toMatchObject(["A value must be provided"]);
 
   r = t("");
   expect(r.valid).toBeFalsy();
-  expect(r.warnings).toMatchObject([]);
   expect(r.errors).toMatchObject(["A value must be provided"]);
 
   r = t("foo");
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual("foo");
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 
   r = t(123);
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual("123");
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 });
 
 test('optional basic string', () => {
@@ -31,50 +29,49 @@ test('optional basic string', () => {
 
   let r = t(undefined);
   expect(r.valid).toBeTruthy();
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
   expect(r.value).toEqual(undefined);
 
   r = t("");
   expect(r.valid).toBeTruthy();
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
   expect(r.value).toEqual(undefined);
 
   r = t("foo");
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual("foo");
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 
   r = t(123);
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual("123");
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 });
 
 test('required basic number', () => {
   const t = attributeTypes.requiredType(attributeTypes.basicNumberType);
 
   let r = t("foo");
   expect(r.valid).toBeFalsy();
-  expect(r.warnings).toMatchObject([]);
   expect(r.errors).toMatchObject(["This is not a valid number"]);
 
   r = t(123);
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual(123);
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 });
 
 test('optional basic number', () => {
   // Also tests that optionalType converts errors into warnings
   const t = attributeTypes.optionalType(attributeTypes.basicNumberType);
 
   let r = t("foo");
-  expect(r.valid).toBeTruthy();
+  expect(r.valid).toBeFalsy();
   expect(r.value).toBeUndefined();
-  expect(r.warnings).toMatchObject(["This is not a valid number"]);
+  expect(r.errors).toMatchObject(["This is not a valid number"]);
 
   r = t(123);
   expect(r.valid).toBeTruthy();
   expect(r.value).toEqual(123);
-  expect(r.warnings).toMatchObject([]);
+  expect(r.errors).toMatchObject([]);
 });

lib/importer/src/backend.js

@@ -481,7 +481,7 @@ function mapCellValue(cell) {
   return cell.v;
 }
 
-exports.SessionPerformMappingJob = (sid, range, mapping) => {
+exports.SessionPerformMappingJob = (sid, range, mapping, includeErrorRow = false) => {
   assert(sessionStore.get(sid));
   assert(sessionStore.get(sid).wb.Sheets[range.sheet]);
   assert(range.end.row >= range.start.row);
@@ -499,13 +499,26 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
   const attrMap = Object.entries(mapping.attributeMappings);
   const attrTypes = mapping.attributeTypes || {};
 
+
+  // TODO: Replace this with a map/sum once range is a real type.
+  let expectedColumnCount = 0;
+  for (let rowIdx = range.start.row; rowIdx <= range.end.row; rowIdx++) {
+    let row = getMergedRow(data, merges, rowIdx);
+    if (row && row.length > expectedColumnCount) expectedColumnCount = row.length;
+  }
+
   for (let rowIdx = range.start.row; rowIdx <= range.end.row; rowIdx++) {
     let row = getMergedRow(data, merges, rowIdx);
     let record = {};
     let foundSomeValues = false;
     let rowWarnings = [];
     let rowErrors = [];
+
     if (row) {
+      if (row.length < expectedColumnCount) {
+        rowWarnings.push("Row has fewer columns than expected, wanted " + expectedColumnCount + " but found " + + row.length)
+      }
+
       attrMap.forEach((element) => {
         const [attr, m] = element;
 
@@ -518,6 +531,10 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
           if (cell && cell.v) {
             record[attr] = mapCellValue(cell);
             foundSomeValues = true;
+          } else {
+            // Ensure we add every cell so that validation can fail those without
+            // values (where they are required).
+            record[attr] = undefined
           }
         }
       });
@@ -533,10 +550,6 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
 
         const result = attrType(inputVal);
 
-        result.warnings.forEach((text) => {
-          rowWarnings.push([attr, text]);
-        });
-
         if (result.valid) {
           // Succeeded, but maybe an empty result
           if (result.value !== undefined) {
@@ -545,19 +558,27 @@ exports.SessionPerformMappingJob = (sid, range, mapping) => {
         } else {
           // Failed
           result.errors.forEach((text) => {
-            rowErrors.push([attr, text]);
+            if (includeErrorRow) {
+              rowErrors.push({ row: rowIdx, field: attr, message: text, data: row.map((x) => mapCellValue(x)) });
+            } else {
+              rowErrors.push({ row: rowIdx, field: attr, message: text });
+            }
+
           });
         }
       });
 
       if (rowErrors.length == 0) {
         records.push(mappedRecord);
       }
+    } else {
+      rowWarnings.push("Row is empty")
     }
 
     if (rowWarnings.length > 0) {
       warnings.set(rowIdx, rowWarnings);
     }
+
     if (rowErrors.length > 0) {
       errors.set(rowIdx, rowErrors);
     }
@@ -596,7 +617,6 @@ exports.JobGetWarnings = (jid) => {
 exports.JobGetErrors = (jid) => {
   assert(jobStore.has(jid));
   let job = jobStore.get(jid);
-
   return job.errors;
 }