Apply validation functions to mapping. by rossjones · Pull Request #192 · register-dynamics/data-import

rossjones · 2025-04-07T09:40:23Z

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.

Where we pass a mapping structure to the backend for mapping, also ensure that the attribute types are passed so that they can be processed.

And structural issues (empty rows) are warnings.

simonwo

Looks good so far, I haven't been able to see the review page yet.

simonwo · 2025-04-23T09:32:23Z

+                    <th scope="col" class="govuk-table__header">Error</th>
+                </tr>
+            </thead>
+            <tbody class="govuk-table__body"></tbody>


Suggested change

<tbody class="govuk-table__body"></tbody>

<tbody class="govuk-table__body">

Fixed in 8ebbd5b

simonwo · 2025-04-24T05:48:58Z

+      if (required_fields.length > 0) {
+        request.session.data[IMPORTER_ERROR_KEY] = "The following fields are required"
+        request.session.data[IMPORTER_ERROR_EXTRA_KEY] = required_fields
+        response.redirect(request.get('Referrer'));
+        return;
+      }


If we hit this, we lose all of the user's previous entries on the mapping page. I get that we aren't rendering the template here so can't push the previous entries onto the page, but could we be storing them in the session for the mapping macro to pick up?

Done in f2cbe70 but not super-happy with it as it doesn't feel generic enough to be re-usable elsewhere, but workable for now until we have that need.

simonwo · 2025-04-24T05:53:17Z

+      <ul class="govuk-list govuk-error-summary__list">
+        {% for e in error.extra %}
+        <li>
+          <a href="#">{{ e }}</a>


Why the anchor here?

I think in the design system it's intended to jump to the anchor where the error occurs, but I think I just forgot to go back and implement that by adding ids to the fields. Will sort it..

Fixed in e3ad256 but not sure scrolling down the page without a visual indicator on the erroring row is that helpful, but not sure I know how to visually indicate the error in a table.

simonwo · 2025-04-24T05:57:24Z

+      if (required_fields.length > 0) {
+        request.session.data[IMPORTER_ERROR_KEY] = "The following fields are required"
+        request.session.data[IMPORTER_ERROR_EXTRA_KEY] = required_fields
+        response.redirect(request.get('Referrer'));
+        return;
+      }


Also, this does not clear the error in the case where length == 0, meaning that returning to the mapping page continues to show an error.

The errors are cleaned up on each request so any request after an error will remove it from the session. The redirect here is working as we the referrer is a user provided url rather than a data-importer route

THat's not what I'm observing – if you go to the mapping page and don't map all the required fields, you see an error. If you then map the required fields you can progress. If you THEN use the breadcrumbs to go back to the mapping page, the error is still visible, even when refreshing?

Ah yeah, if you don't hit an importer route and then go back that's the behaviour. I think I've got a fix, will give it a test in 666d10d

simonwo · 2025-04-24T06:07:11Z

-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)


Something here doesn't seem to be working. If I have config:

{ "serviceName": "Upload monthly pension return", "fields": [ { "name": "Title", "type": "text", "required": false }, { "name": "First name", "type": "text", "required": false }, { "name": "Surname", "type": "text", "required": false }, { "name": "Employee number", "type": "text", "required": true }, { "name": "Employment start date", "type": "text", "required": false }, { "name": "Salary", "type": "text", "required": true }, { "name": "Contribution percentage", "type": "text", "required": true }, { "name": "Payment date", "type": "text", "required": false } ], "uploadPath": "" }

and I upload this CSV and map the columns in the obvious way:

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

Then I am told that I have uploaded two rows and there are no errors. But there should be errors because one of the rows is missing values for required fields.

More tests needed?

Resolved in 7e82f2d, tests to follow.

Previously fields that were not present (and had no value) where not being processed by the validation which iterated over the keys present in the object to decide what to validate.

When we submit values, if there is an error we currently end up with blank selections. This change will remember the previous submission and pre-select the values as they were submitted when there is an error.

rossjones added 9 commits April 7, 2025 10:38

Add validation functions to mapping input

3927fb3

Where we pass a mapping structure to the backend for mapping, also ensure that the attribute types are passed so that they can be processed.

Appease the linter!

579df1d

Ensure required fields are included in mapping

2ab7285

Appease the linter

e3a61c6

Show warnings+errors in review page

32334fc

Validation failures are errors

6244e87

And structural issues (empty rows) are warnings.

Store errors as index, field and message

76130c2

Add short rows as a warning

1062b2a

Add error row to errors when flag is set

edef69e

rossjones requested a review from simonwo April 22, 2025 14:38

Merge branch 'main' into validate-mapping

009e636

simonwo reviewed Apr 24, 2025

View reviewed changes

rossjones added 9 commits April 24, 2025 11:08

Remove extraneous closing tbody tags

8ebbd5b

Clear error on successful submission

666d10d

When mapping include all fields for validation

7e82f2d

Previously fields that were not present (and had no value) where not being processed by the validation which iterated over the keys present in the object to decide what to validate.

Fix ordering of errors in test

b2877d6

Autoselect submitted values on mapping error

f2cbe70

When we submit values, if there is an error we currently end up with blank selections. This change will remember the previous submission and pre-select the values as they were submitted when there is an error.

Use slugs to link errors to the correct row

e3ad256

Happy path validation test

6a13173

Missing values and incorrect type tests

9596c79

Test structural warnings

f46bd9a

simonwo approved these changes Apr 29, 2025

View reviewed changes

rossjones merged commit 6dd45ba into main Apr 29, 2025
5 checks passed

rossjones deleted the validate-mapping branch April 29, 2025 08:54

	<tbody class="govuk-table__body"></tbody>
	<tbody class="govuk-table__body">

Conversation

rossjones commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simonwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rossjones Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rossjones Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

rossjones commented Apr 7, 2025 •

edited

Loading

rossjones Apr 24, 2025 •

edited

Loading

rossjones Apr 24, 2025 •

edited

Loading