From e8823ae929d6622c3a2282075709d8f51d6777d7 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 11 Sep 2024 14:38:44 +0100 Subject: [PATCH 1/4] Make mapdata limit optional --- lib/importer/sheets.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/importer/sheets.js b/lib/importer/sheets.js index 4614e26..cc17262 100644 --- a/lib/importer/sheets.js +++ b/lib/importer/sheets.js @@ -218,10 +218,10 @@ exports.GetColumnValues = (session, columnIndex, cellWidth=30, count=10) => { // Uses the session provided, which must contain a sheet name and a // mapping to perform the mapping of the data across the remaining // rows in the sheet to return an array of objects. -exports.MapData = (session, previewLimit = DEFAULT_PREVIEW_LIMIT) => { - // Construct the range to be mapped - everything but the first row - const sheetDimensions = backend.SessionGetInputDimensions(session.backendSid) - .sheetDimensions.get(session.sheet); +exports.MapData = (session, limit=0) => { + // Construct the range to be mapped - everything but the first row + const sheetDimensions = backend.SessionGetInputDimensions(session.backendSid) + .sheetDimensions.get(session.sheet); const hRange = session.headerRange; const rowRange = backend.SessionSuggestDataRange(session.backendSid, session.headerRange, session.footerRange); @@ -246,8 +246,8 @@ exports.MapData = (session, previewLimit = DEFAULT_PREVIEW_LIMIT) => { // Read all the results into memory const resultSummary = backend.JobGetSummary(backendJid); - const recordsToPreview = Math.min(resultSummary.recordCount, previewLimit); - const results = backend.JobGetRecords(backendJid, 0, recordsToPreview); + const recordsToView = limit > 0 ? Math.min(resultSummary.recordCount, limit) : resultSummary.recordCount; + const results = backend.JobGetRecords(backendJid, 0, recordsToView); // Rewrite the results to be arrays in session.fields order, rather than objects const fields = session.fields; From 1727de57de62356bba3f670ad167094b0936f2e0 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Wed, 11 Sep 2024 15:51:01 +0100 Subject: [PATCH 2/4] Clean up errors on each request --- lib/importer/index.js | 41 ++++++++++++++----- .../nunjucks/importer/macros/field_mapper.njk | 24 ++++++++--- prototypes/basic/app/views/mapping.html | 2 +- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/lib/importer/index.js b/lib/importer/index.js index 49d2108..7324d2c 100644 --- a/lib/importer/index.js +++ b/lib/importer/index.js @@ -30,28 +30,38 @@ exports.Initialise = (config, router, prototypeKit) => { // one created by us. config.uploadPath ??= path.join(os.tmpdir(), 'reg-dyn-importer'); ; + //-------------------------------------------------------------------- // Removes any previous importer error from the session. When we set // an error we redirect to the referer and we expect that page to show // the error. Calling this in each POST endpoint ensures that we don't // remember errors after they have been shown, //-------------------------------------------------------------------- - const cleanRequest = (request) => { + router.all("*", (request, res, next) => { delete request.session.data[IMPORTER_ERROR_KEY] - } + next(); + }); + //-------------------------------------------------------------------- // Make the route functions available in the templates. These functions // allow users to find the path that they should use to submit data, // and also allows them to specify which url to redirect to after - // the POST data has been processed. + // the POST data has been processed. Where an extra error url is provided + // it will be used if the POST request does not succeed (e.g. when + // mapping the data this can be used to redirect to the review page + // to view warnings) //-------------------------------------------------------------------- for ([key, value] of IMPORTER_ROUTE_MAP) { const k = key; const v = value; - prototypeKit.views.addFunction(k, (next)=>{ - return `${v}?next=${encodeURIComponent(next)}`; + prototypeKit.views.addFunction(k, (next, errorPage=null)=>{ + let url = `${v}?next=${encodeURIComponent(next)}`; + if (errorPage) { + url = url + `&error=${encodeURIComponent(errorPage)}` + } + return url }, {}) } @@ -169,8 +179,13 @@ exports.Initialise = (config, router, prototypeKit) => { // Redirects the current request to the 'next' URL after decoding the // encoded URI. //-------------------------------------------------------------------- - const redirectOnwards = (request, response) => { - response.redirect(decodeURIComponent(request.query.next)); + const redirectOnwards = (request, response, failed=false) => { + console.log(request.query) + if (failed && request.query.error) { + response.redirect(decodeURIComponent(request.query.error)); + } else { + response.redirect(decodeURIComponent(request.query.next)); + } } //-------------------------------------------------------------------- @@ -181,7 +196,6 @@ exports.Initialise = (config, router, prototypeKit) => { // time we run through the process. //-------------------------------------------------------------------- router.all(IMPORTER_ROUTE_MAP.get("importerStartPath"),(request, response) => { - cleanRequest(request); redirectOnwards(request, response); }); @@ -192,8 +206,6 @@ exports.Initialise = (config, router, prototypeKit) => { const uploader = getUploader(config); router.post(IMPORTER_ROUTE_MAP.get("importerUploadPath"), uploader.single("file"), (request, response) => { - cleanRequest(request); - let createResponse = session_lib.CreateSession(config, request); if (createResponse.error) { @@ -320,6 +332,15 @@ exports.Initialise = (config, router, prototypeKit) => { } session.mapping = request.body; + if (Object.values(session.mapping).every((v) => v=='') ) { + request.session.data[IMPORTER_ERROR_KEY] = "No columns were mapped to the expected fields" + if (!request.query.error) { + response.redirect(request.get('Referrer')) + } else { + redirectOnwards(request, response, failed=true); + } + return; + } // Ensure the session is persisted. Currently in session, eventually another way request.session.data[IMPORTER_SESSION_KEY] = session; diff --git a/lib/importer/nunjucks/importer/macros/field_mapper.njk b/lib/importer/nunjucks/importer/macros/field_mapper.njk index 8d2b494..e0f9d42 100644 --- a/lib/importer/nunjucks/importer/macros/field_mapper.njk +++ b/lib/importer/nunjucks/importer/macros/field_mapper.njk @@ -17,13 +17,25 @@ {% 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 error = importerError(data) %} - {% if error %} -

- Error: {{ error.text }} -

- {% endif %} + + {% if error %} +
+
+

+ There was a problem submitting this data +

+
+
    +
  • + {{ error.text }} +
    • +
+
+
+
+ {% endif %} diff --git a/prototypes/basic/app/views/mapping.html b/prototypes/basic/app/views/mapping.html index d76b088..bf60b15 100644 --- a/prototypes/basic/app/views/mapping.html +++ b/prototypes/basic/app/views/mapping.html @@ -14,7 +14,7 @@

Map columns

{{sheet}}

- + {{ importerFieldMapper(data) }} From 9502656a7ea1654a9047419b53715c1eb968adc1 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 19 Sep 2024 14:34:19 +0100 Subject: [PATCH 3/4] Add a macro to display errors/warnings during mapping --- lib/importer/govuk-prototype-kit.config.json | 6 +++- lib/importer/index.js | 29 +++++++++++++++++ .../importer/macros/mapping_review.njk | 32 +++++++++++++++++++ lib/importer/sheets.js | 2 +- lib/importer/templates/review.html | 9 ++---- prototypes/basic/app/views/review.html | 4 ++- 6 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 lib/importer/nunjucks/importer/macros/mapping_review.njk diff --git a/lib/importer/govuk-prototype-kit.config.json b/lib/importer/govuk-prototype-kit.config.json index 61077c6..525e1bd 100644 --- a/lib/importer/govuk-prototype-kit.config.json +++ b/lib/importer/govuk-prototype-kit.config.json @@ -81,6 +81,10 @@ { "macroName": "importerRangeSelector", "importFrom": "importer/macros/range_selector.njk" + }, + { + "macroName": "importerMappingReview", + "importFrom": "importer/macros/mapping_review.njk" } ] -} +} \ No newline at end of file diff --git a/lib/importer/index.js b/lib/importer/index.js index 7324d2c..6a3dd83 100644 --- a/lib/importer/index.js +++ b/lib/importer/index.js @@ -9,6 +9,8 @@ const os = require('node:os'); const IMPORTER_SESSION_KEY = "importer.session" const IMPORTER_ERROR_KEY = "importer.error" +const MAPPING_ERROR_KEY = "mapping.errors" +const MAPPING_WARNING_KEY = "mapping.warnings" //-------------------------------------------------------------------- // The endpoints used to receive POST requests from importer macros, @@ -39,6 +41,8 @@ exports.Initialise = (config, router, prototypeKit) => { //-------------------------------------------------------------------- router.all("*", (request, res, next) => { delete request.session.data[IMPORTER_ERROR_KEY] + delete request.session.data[MAPPING_ERROR_KEY] + delete request.session.data[MAPPING_WARNING_KEY] next(); }); @@ -94,6 +98,22 @@ exports.Initialise = (config, router, prototypeKit) => { return false; }, {}) + + //-------------------------------------------------------------------- + // Returns warnings or errors arising from applying a mapping. + //-------------------------------------------------------------------- + prototypeKit.views.addFunction("importerMappingFailures", (data) => { + if (data[MAPPING_ERROR_KEY] || data[MAPPING_WARNING_KEY]) { + return { + warnings: data[MAPPING_WARNING_KEY], + errors: data[MAPPING_ERROR_KEY] + }; + } + + return false; + }, {}) + + //-------------------------------------------------------------------- // Allows a template to obtain `count` rows from the start of the data // range. @@ -342,6 +362,15 @@ exports.Initialise = (config, router, prototypeKit) => { return; } + const mapResults = sheets_lib.MapData(session); + if (mapResults.warningCount >= 0 || mapResults.errorCount >= 0) { + request.session.data[MAPPING_ERROR_KEY] = ["a test error"] + request.session.data[MAPPING_WARNING_KEY] = ["a test warning", "another warning"] + + redirectOnwards(request, response, failed=true); + return + } + // Ensure the session is persisted. Currently in session, eventually another way request.session.data[IMPORTER_SESSION_KEY] = session; redirectOnwards(request, response); diff --git a/lib/importer/nunjucks/importer/macros/mapping_review.njk b/lib/importer/nunjucks/importer/macros/mapping_review.njk new file mode 100644 index 0000000..7eb4825 --- /dev/null +++ b/lib/importer/nunjucks/importer/macros/mapping_review.njk @@ -0,0 +1,32 @@ + + +{# + Shows warnings and errors generated when attempting to + perform a mapping, giving users a chance to attempt to + fix the problem, or to continue on. +#} +{% macro importerMappingReview(data) %} + {% set failures = importerMappingFailures(data) %} + + {% if failures.warnings %} +
+

Warnings

+
    + {% for warning in failures.warnings %} +
  • {{ warning }}
    • + {% endfor %} +
+
+ {% endif %} + + {% if failures.errors %} +
+

Warnings

+
    + {% for errors in failures.errors %} +
  • {{ errors }}
    • + {% endfor %} +
+
+ {% endif %} +{% endmacro %} diff --git a/lib/importer/sheets.js b/lib/importer/sheets.js index cc17262..1365dc4 100644 --- a/lib/importer/sheets.js +++ b/lib/importer/sheets.js @@ -274,7 +274,7 @@ exports.MapData = (session, limit=0) => { return { resultRecords: resultsAsArrays, totalCount: resultSummary.recordCount, - extraRecordCount: resultSummary.recordCount-recordsToPreview, + extraRecordCount: resultSummary.recordCount-recordsToView, warningCount: resultSummary.warningCount, errorCount: resultSummary.errorCount, warnings: warnings, diff --git a/lib/importer/templates/review.html b/lib/importer/templates/review.html index 42bb689..ab40e52 100644 --- a/lib/importer/templates/review.html +++ b/lib/importer/templates/review.html @@ -1,5 +1,5 @@ {% extends "layouts/main.html" %} -{% from "importer/macros/table_view.njk" import importerTableView %} +{% from "importer/macros/mapping_review.njk" import importerMappingReview %} {% block pageTitle %} {{ serviceName }} – GOV.UK Prototype Kit {% endblock %} @@ -12,11 +12,8 @@

Review your data

- -
- {{ govukButton({ text: "Submit" }) }} -
- + {{ importerMappingReview(data) }} +
{% endblock %} \ No newline at end of file diff --git a/prototypes/basic/app/views/review.html b/prototypes/basic/app/views/review.html index 42bb689..fcefcce 100644 --- a/prototypes/basic/app/views/review.html +++ b/prototypes/basic/app/views/review.html @@ -1,5 +1,5 @@ {% extends "layouts/main.html" %} -{% from "importer/macros/table_view.njk" import importerTableView %} +{% from "importer/macros/mapping_review.njk" import importerMappingReview %} {% block pageTitle %} {{ serviceName }} – GOV.UK Prototype Kit {% endblock %} @@ -12,6 +12,8 @@

Review your data

+ {{ importerMappingReview(data) }} +
{{ govukButton({ text: "Submit" }) }} From c7dd752c8af2f113ac0e63d24323d85a5a9fbe07 Mon Sep 17 00:00:00 2001 From: Ross Jones Date: Thu, 19 Sep 2024 14:44:19 +0100 Subject: [PATCH 4/4] Separate warning and error lists --- lib/importer/govuk-prototype-kit.config.json | 8 +++-- lib/importer/index.js | 17 ++++++---- .../importer/macros/mapping_error_list.njk | 19 +++++++++++ .../importer/macros/mapping_review.njk | 32 ------------------- .../importer/macros/mapping_warning_list.njk | 19 +++++++++++ lib/importer/templates/review.html | 7 ++-- prototypes/basic/app/views/review.html | 12 +++---- 7 files changed, 65 insertions(+), 49 deletions(-) create mode 100644 lib/importer/nunjucks/importer/macros/mapping_error_list.njk delete mode 100644 lib/importer/nunjucks/importer/macros/mapping_review.njk create mode 100644 lib/importer/nunjucks/importer/macros/mapping_warning_list.njk diff --git a/lib/importer/govuk-prototype-kit.config.json b/lib/importer/govuk-prototype-kit.config.json index 525e1bd..178daf4 100644 --- a/lib/importer/govuk-prototype-kit.config.json +++ b/lib/importer/govuk-prototype-kit.config.json @@ -83,8 +83,12 @@ "importFrom": "importer/macros/range_selector.njk" }, { - "macroName": "importerMappingReview", - "importFrom": "importer/macros/mapping_review.njk" + "macroName": "importerMappingWarningList", + "importFrom": "importer/macros/mapping_warning_list.njk" + }, + { + "macroName": "importerMappingErrorList", + "importFrom": "importer/macros/mapping_error_list.njk" } ] } \ No newline at end of file diff --git a/lib/importer/index.js b/lib/importer/index.js index 6a3dd83..3ebe7a6 100644 --- a/lib/importer/index.js +++ b/lib/importer/index.js @@ -102,12 +102,17 @@ exports.Initialise = (config, router, prototypeKit) => { //-------------------------------------------------------------------- // Returns warnings or errors arising from applying a mapping. //-------------------------------------------------------------------- - prototypeKit.views.addFunction("importerMappingFailures", (data) => { - if (data[MAPPING_ERROR_KEY] || data[MAPPING_WARNING_KEY]) { - return { - warnings: data[MAPPING_WARNING_KEY], - errors: data[MAPPING_ERROR_KEY] - }; + prototypeKit.views.addFunction("importerMappingErrors", (data) => { + if (data[MAPPING_ERROR_KEY]) { + return data[MAPPING_ERROR_KEY] + } + + return false; + }, {}) + + prototypeKit.views.addFunction("importerMappingWarnings", (data) => { + if (data[MAPPING_WARNING_KEY]) { + return data[MAPPING_WARNING_KEY] } return false; diff --git a/lib/importer/nunjucks/importer/macros/mapping_error_list.njk b/lib/importer/nunjucks/importer/macros/mapping_error_list.njk new file mode 100644 index 0000000..a6f6aa1 --- /dev/null +++ b/lib/importer/nunjucks/importer/macros/mapping_error_list.njk @@ -0,0 +1,19 @@ + + +{# + Shows errors generated when attempting to + perform a mapping +#} +{% macro importerMappingErrorList(data) %} + {% set errors = importerMappingErrors(data) %} + {% if errors %} +
+

Errors

+
    + {% for error in errors %} +
  • {{ error }}
    • + {% endfor %} +
+
+ {% endif %} +{% endmacro %} diff --git a/lib/importer/nunjucks/importer/macros/mapping_review.njk b/lib/importer/nunjucks/importer/macros/mapping_review.njk deleted file mode 100644 index 7eb4825..0000000 --- a/lib/importer/nunjucks/importer/macros/mapping_review.njk +++ /dev/null @@ -1,32 +0,0 @@ - - -{# - Shows warnings and errors generated when attempting to - perform a mapping, giving users a chance to attempt to - fix the problem, or to continue on. -#} -{% macro importerMappingReview(data) %} - {% set failures = importerMappingFailures(data) %} - - {% if failures.warnings %} -
-

Warnings

-
    - {% for warning in failures.warnings %} -
  • {{ warning }}
    • - {% endfor %} -
-
- {% endif %} - - {% if failures.errors %} -
-

Warnings

-
    - {% for errors in failures.errors %} -
  • {{ errors }}
    • - {% endfor %} -
-
- {% endif %} -{% endmacro %} diff --git a/lib/importer/nunjucks/importer/macros/mapping_warning_list.njk b/lib/importer/nunjucks/importer/macros/mapping_warning_list.njk new file mode 100644 index 0000000..a9ab2c0 --- /dev/null +++ b/lib/importer/nunjucks/importer/macros/mapping_warning_list.njk @@ -0,0 +1,19 @@ + + +{# + Shows warnings generated when attempting to + perform a mapping +#} +{% macro importerMappingWarningList(data) %} + {% set warnings = importerMappingWarnings(data) %} + {% if warnings %} +
+

Warnings

+
    + {% for warning in warnings %} +
  • {{ warning }}
    • + {% endfor %} +
+
+ {% endif %} +{% endmacro %} diff --git a/lib/importer/templates/review.html b/lib/importer/templates/review.html index ab40e52..1c4157c 100644 --- a/lib/importer/templates/review.html +++ b/lib/importer/templates/review.html @@ -1,5 +1,6 @@ {% extends "layouts/main.html" %} -{% from "importer/macros/mapping_review.njk" import importerMappingReview %} +{% from "importer/macros/mapping_warning_list.njk" import importerMappingWarningList %} +{% from "importer/macros/mapping_error_list.njk" import importerMappingErrorList %} {% block pageTitle %} {{ serviceName }} – GOV.UK Prototype Kit {% endblock %} @@ -12,7 +13,9 @@

Review your data

- {{ importerMappingReview(data) }} + {{ importerMappingWarningList(data) }} + + {{ importerMappingErrorList(data) }}
diff --git a/prototypes/basic/app/views/review.html b/prototypes/basic/app/views/review.html index fcefcce..1c4157c 100644 --- a/prototypes/basic/app/views/review.html +++ b/prototypes/basic/app/views/review.html @@ -1,5 +1,6 @@ {% extends "layouts/main.html" %} -{% from "importer/macros/mapping_review.njk" import importerMappingReview %} +{% from "importer/macros/mapping_warning_list.njk" import importerMappingWarningList %} +{% from "importer/macros/mapping_error_list.njk" import importerMappingErrorList %} {% block pageTitle %} {{ serviceName }} – GOV.UK Prototype Kit {% endblock %} @@ -12,13 +13,10 @@

Review your data

- {{ importerMappingReview(data) }} + {{ importerMappingWarningList(data) }} + + {{ importerMappingErrorList(data) }} - -
- {{ govukButton({ text: "Submit" }) }} -
-
{% endblock %} \ No newline at end of file