diff --git a/src/content/statusPage.js b/src/content/statusPage.js index 3288b011..81dffbee 100644 --- a/src/content/statusPage.js +++ b/src/content/statusPage.js @@ -1,6 +1,6 @@ export const headingTexts = { checking: 'Checking your data', - checked: 'Data Checked' + checked: 'Data checked' } export const messageTexts = { diff --git a/src/controllers/resultsController.js b/src/controllers/resultsController.js index bc84c94a..aee6192e 100644 --- a/src/controllers/resultsController.js +++ b/src/controllers/resultsController.js @@ -65,11 +65,17 @@ class ResultsController extends PageController { fields: responseDetails.getFields() } - req.form.options.errorSummary = requestData.getErrorSummary() + req.form.options.errorSummary = requestData.getErrorSummary().map(message => { + return { + text: message, + href: '' + } + }) req.form.options.mappings = responseDetails.getFieldMappings() req.form.options.geometries = responseDetails.getGeometries() req.form.options.pagination = responseDetails.getPagination(req.params.pageNumber) req.form.options.id = req.params.id + req.form.options.lastPage = `/check/status/${req.params.id}` } else { req.form.options.error = requestData.getError() } diff --git a/src/controllers/statusController.js b/src/controllers/statusController.js index afbd8216..9afcae2e 100644 --- a/src/controllers/statusController.js +++ b/src/controllers/statusController.js @@ -3,6 +3,22 @@ import { getRequestData } from '../services/asyncRequestApi.js' import { finishedProcessingStatuses } from '../utils/utils.js' import { headingTexts, messageTexts } from '../content/statusPage.js' +/** + * Attempts to infer how we ended up on this page. + * + * @param req + * @returns {string?} + */ +function getLastPage (req) { + let lastPage + if ('url' in req.form.options.data.params) { + lastPage = '/check/url' + } else if ('original_filename' in req.form.options.data.params) { + lastPage = '/check/upload' + } + return lastPage +} + class StatusController extends PageController { async locals (req, res, next) { try { @@ -11,6 +27,10 @@ class StatusController extends PageController { req.form.options.headingTexts = headingTexts req.form.options.messageTexts = messageTexts req.form.options.pollingEndpoint = `/api/status/${req.form.options.data.id}` + const lastPage = getLastPage(req) + if (lastPage) { + req.form.options.lastPage = lastPage + } super.locals(req, res, next) } catch (error) { next(error, req, res, next) diff --git a/src/middleware/common.middleware.js b/src/middleware/common.middleware.js index b3db166f..184fbe69 100644 --- a/src/middleware/common.middleware.js +++ b/src/middleware/common.middleware.js @@ -620,43 +620,15 @@ export const getSetDataRange = (pageLength) => (req, res, next) => { export function getErrorSummaryItems (req, res, next) { const { issue_type: issueType, issue_field: issueField } = req.params - const { baseSubpath, issues, issueCount, entities, resources } = req - - let errorHeading = '' - let issueItems + const { issues, issueCount, entities, resources } = req const totalRecordCount = entities ? entities.length : resources[0].entry_count const totalIssues = issueCount?.count || issues.length - if (issues.length <= 0) { - // currently the task list page is getting its issues incorrectly, not factoring in the fact that an issue might have been fixed. - logger.warn(`entry issue details was accessed from ${req.headers.referer} but there was no issues`) - const error = new Error('issue count must be larger than 0') - return next(error) - } else if (issues.length < totalRecordCount) { - errorHeading = performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, rowCount: totalRecordCount, field: issueField }, true) - issueItems = issues.map((issue, i) => { - const pageNum = i + 1 - let inString = '' - - // we have to hard code this in because though the entitiy has beeb assigned, it has been assigned incorrectly - const specialIssueTypeCases = ['reference values are not unique'] - - if (issue.reference && !specialIssueTypeCases.includes(issue.issue_type)) { - inString = ` in entity with reference ${issue.reference}` - } else if (issue.line_number) { - inString = ` on row ${issue.line_number}` - } - return { - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: 1, field: issueField }) + inString, - href: `${baseSubpath}/${pageNum}` - } - }) - } else { - issueItems = [{ - html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, rowCount: totalRecordCount, field: issueField }, true) - }] - } + const errorHeading = '' + const issueItems = [{ + html: performanceDbApi.getTaskMessage({ issue_type: issueType, num_issues: totalIssues, entityCount: totalRecordCount, field: issueField }, true) + }] req.errorSummary = { heading: errorHeading, diff --git a/src/models/requestData.js b/src/models/requestData.js index badac124..74458e31 100644 --- a/src/models/requestData.js +++ b/src/models/requestData.js @@ -32,6 +32,10 @@ export default class ResultData { return new ResponseDetails(this.id, response.data, pagination, this.getColumnFieldLog()) } + /** + * + * @returns {string[]} + */ getErrorSummary () { if (!this.response || !this.response.data || !this.response.data['error-summary']) { logger.warn('trying to get error summary when there is none', { requestId: this.id }) diff --git a/src/views/check/confirmation.html b/src/views/check/confirmation.html index c72c3db0..6ff79bd7 100644 --- a/src/views/check/confirmation.html +++ b/src/views/check/confirmation.html @@ -1,4 +1,4 @@ - +{% from 'govuk/components/back-link/macro.njk' import govukBackLink %} {% from 'govuk/components/panel/macro.njk' import govukPanel %} {% from "govuk/components/details/macro.njk" import govukDetails %} @@ -7,6 +7,14 @@ {% set serviceType = 'Check' %} {% set pageName = "You can now publish your data" %} +{% block beforeContent %} + {{ govukBackLink({ + text: "Back", + href: "javascript:window.history.back()" + }) }} +{% endblock %} + + {% set content %} # What to do next @@ -31,7 +39,7 @@ Details about what to do next are also available [in the guidance](/guidance) {% if options.deepLink %} -

Return to {{ options.deepLink.dataset }} overview

+

Return to {{ options.deepLink.dataset | datasetSlugToReadableName }} overview

{% else %}

Return to Home

{% endif %} diff --git a/src/views/check/results/errors.html b/src/views/check/results/errors.html index c83a1bfb..1edc23c2 100644 --- a/src/views/check/results/errors.html +++ b/src/views/check/results/errors.html @@ -4,9 +4,9 @@ {% from 'govuk/components/radios/macro.njk' import govukRadios %} {% from 'govuk/components/inset-text/macro.njk' import govukInsetText %} {% from "govuk/components/pagination/macro.njk" import govukPagination %} - -{% from 'components/dataset-banner.html' import datasetBanner %} -{% from 'components/table.html' import table %} +{% from 'govuk/components/error-summary/macro.njk' import govukErrorSummary %} +{% from "../../components/table.html" import table %} +{% from '../../components/dataset-banner.html' import datasetBanner %} {% set serviceType = 'Check' %} {% set pageName = 'Your data has errors' %} @@ -36,23 +36,22 @@

{{pageName}}

- + {{ govukErrorSummary({ + titleText: "There’s a problem", + errorList: options.errorSummary + }) }} +
+
- {% if options.tableParams.rows|length > 0 %} -

- Check your errors -

+ {% if options.geometries %} +
+ {% endif %} + {% if options.tableParams.rows|length > 0 %} {{ table(options.tableParams) }} {% endif %} @@ -72,11 +71,37 @@

+ + {% if options.deepLink.orgName %} +

How to improve {{ options.deepLink.orgName }}'s data

+ {% else %} +

How to improve the data

+ {% endif %} + +
    +
  1. Fix the errors indicated. You should use the data specifications to help you do this.
  2. +
  3. Check your updated data to make sure it meets the specifications.
  4. +
+ {{ govukButton({ - text: "Upload a new version", + text: "Check your data", href: "/check/upload-method" }) }}
{% endblock %} + +{% block scripts %} + {{ super() }} + {% if options.geometries %} + + + + {% endif %} +{% endblock %} diff --git a/src/views/check/results/no-errors.html b/src/views/check/results/no-errors.html index 2dc44b4b..cb4fc127 100644 --- a/src/views/check/results/no-errors.html +++ b/src/views/check/results/no-errors.html @@ -54,6 +54,10 @@

+ {% if options.geometries %} +
+ {% endif %} + {{ table(options.tableParams) }} {% if options.pagination.items | length > 1 %} @@ -69,13 +73,6 @@

{% endif %} - - {% if options.geometries %} -

Here’s your data on a map:

-
- {% endif %} - -

diff --git a/src/views/check/statusPage/status.html b/src/views/check/statusPage/status.html index 08ab5a4f..f8676ee4 100644 --- a/src/views/check/statusPage/status.html +++ b/src/views/check/statusPage/status.html @@ -1,4 +1,5 @@ {% from "govuk/components/button/macro.njk" import govukButton %} +{% from 'govuk/components/back-link/macro.njk' import govukBackLink %} {% from "./statusContentMacro.html" import statusContent %} {% from '../../components/dataset-banner.html' import datasetBanner %} @@ -17,6 +18,15 @@ {% set buttonClasses = "js-hidden" %} {% endif %} +{% block beforeContent %} + {% if options.lastPage %} + {{ govukBackLink({ + text: options.backLinkText | default("Back"), + href: options.lastPage + }) }} + {% endif %} +{% endblock %} + {% block content %}
diff --git a/src/views/components/dataset-banner.html b/src/views/components/dataset-banner.html index 1ab28274..e99e1e4c 100644 --- a/src/views/components/dataset-banner.html +++ b/src/views/components/dataset-banner.html @@ -1,3 +1,3 @@ {% macro datasetBanner(datasetName) %} -{{ datasetName }} +{{ datasetName | datasetSlugToReadableName }} {% endmacro %} \ No newline at end of file diff --git a/src/views/components/table.html b/src/views/components/table.html index f8808648..b62748a0 100644 --- a/src/views/components/table.html +++ b/src/views/components/table.html @@ -37,6 +37,11 @@ {% macro table(params) %}
+ + {% for field in params.fields %} + + {% endfor %} + {% for column in params.columns %} diff --git a/src/views/error.html b/src/views/error.html index 87f60603..aca677e5 100644 --- a/src/views/error.html +++ b/src/views/error.html @@ -1,10 +1,8 @@ - {% from "govuk/components/button/macro.njk" import govukButton %} {% set pageName = "Error" %} {% extends "layouts/main.html" %} - {% block content %}

Error

diff --git a/src/views/organisations/dataview.html b/src/views/organisations/dataview.html index eff57629..343e7879 100644 --- a/src/views/organisations/dataview.html +++ b/src/views/organisations/dataview.html @@ -61,7 +61,7 @@ {% if pagination.items | length > 1 %}
-

Showing rows {{dataRange.minRow}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

+

Showing rows {{dataRange.minRow + 1}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

{{ govukPagination(pagination) }}
diff --git a/src/views/organisations/issueTable.html b/src/views/organisations/issueTable.html index 30dfef4c..d9e838fe 100644 --- a/src/views/organisations/issueTable.html +++ b/src/views/organisations/issueTable.html @@ -84,7 +84,7 @@ {% if pagination.items | length > 1 %}
-

Showing rows {{dataRange.minRow}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

+

Showing rows {{dataRange.minRow + 1}} to {{dataRange.maxRow}} of {{dataRange.totalRows}}

{{ govukPagination(pagination) }}
diff --git a/test/PageObjectModels/resultsPage.js b/test/PageObjectModels/resultsPage.js index 417b9ac8..2ca0a495 100644 --- a/test/PageObjectModels/resultsPage.js +++ b/test/PageObjectModels/resultsPage.js @@ -39,10 +39,10 @@ export default class ResultsPage extends BasePage { // Check if there's a table expect(await this.page.locator('table').isVisible()) - await this.page.waitForSelector('.govuk-list.govuk-list--bullet') + await this.page.waitForSelector('.govuk-error-summary .govuk-list') // Get the text content of the bullet points const summarytext = await this.page.evaluate(() => { - const bulletPoints = Array.from(document.querySelectorAll('.govuk-list.govuk-list--bullet li')) + const bulletPoints = Array.from(document.querySelectorAll('.govuk-error-summary .govuk-list li')) return bulletPoints.map(li => li.textContent.trim()) }) diff --git a/test/integration/test_recieving_results.playwright.test.js b/test/integration/test_recieving_results.playwright.test.js index 881151d2..d9cc3393 100644 --- a/test/integration/test_recieving_results.playwright.test.js +++ b/test/integration/test_recieving_results.playwright.test.js @@ -40,10 +40,8 @@ test('receiving a result with errors', async ({ page }) => { await resultsPage.navigateToRequest('complete-errors') await resultsPage.expectPageIsErrorsPage() - await page.getByText('1 geometry must be in Well-Known Text (WKT) format 1 documentation URL must be').click() - for (const error of errorResponse.response.data['error-summary']) { - await expect(page.locator('.govuk-list')).toContainText(error) + await expect(page.locator('ul.govuk-list')).toContainText(error) } const tableValues = await getTableContents(page, 'govuk-table') diff --git a/test/unit/errorsPage.test.js b/test/unit/errorsPage.test.js index 7f8f4a6b..84118cf2 100644 --- a/test/unit/errorsPage.test.js +++ b/test/unit/errorsPage.test.js @@ -38,6 +38,13 @@ describe('errors page', () => { limit: 50 } + const summaryItem = (errorMessage) => { + return { + text: errorMessage, + href: '' + } + } + const params = { options: { tableParams: { @@ -46,7 +53,7 @@ describe('errors page', () => { fields: responseDetails.getFields() }, requestParams: requestData.getParams(), - errorSummary: requestData.getErrorSummary(), + errorSummary: requestData.getErrorSummary().map(summaryItem), mappings: responseDetails.getFieldMappings(), verboseRows: responseDetails.getRowsWithVerboseColumns(), pagination: responseDetails.getPagination(0) @@ -65,7 +72,7 @@ describe('errors page', () => { expect(errorItems.length).toEqual(6) params.options.errorSummary.forEach((message, i) => { - expect(errorItems[i].textContent).toContain(message) + expect(errorItems[i].textContent).toContain(message.text) }) // table diff --git a/test/unit/middleware/common.middleware.test.js b/test/unit/middleware/common.middleware.test.js index bd4b199b..7af60dd6 100644 --- a/test/unit/middleware/common.middleware.test.js +++ b/test/unit/middleware/common.middleware.test.js @@ -1315,10 +1315,15 @@ describe('getErrorSummaryItems', () => { getErrorSummaryItems(req, null, next) - expect(next).toHaveBeenCalledWith(new Error('issue count must be larger than 0')) + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type-value', + num_issues: 0, + entityCount: 0, + field: 'issue-field-value' + }, true) }) - it('handles if every entity has the issue', () => { + it('handles if entities are provided', () => { const req = { params: { issue_type: 'issue-type-value', @@ -1341,70 +1346,16 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.heading).toBe('') - }) - it('handles some entities not having the issue', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value' - }, - baseSubpath: 'baseSubpath-value', - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount' }, - { entity: 'entity2', error: ' Invalid Name' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('message') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.heading).toBe('message') + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ + issue_type: 'issue-type-value', + num_issues: 2, + entityCount: 2, + field: 'issue-field-value' + }, true) }) - it('Correctly sets the issue items when some entities dont have issues', () => { - const req = { - params: { - issue_type: 'issue-type-value', - issue_field: 'issue-field-value' - }, - baseSubpath: 'baseSubpath-value/entity', - entities: [ - { reference: 'entity1', name: 'Name 1', amount: 100, error: 'Invalid Amount' }, - { reference: 'entity2', name: 'Name 2', amount: 200, error: ' Invalid Name' }, - { reference: 'entity3', name: 'Name 3', amount: 300 } - ], - issues: [ - { entity: 'entity1', error: 'Invalid Amount', reference: 'entity1' }, - { entity: 'entity2', error: ' Invalid Name', reference: 'entity2' } - ] - } - - vi.mocked(performanceDbApi.getTaskMessage).mockReturnValue('issue') - - getErrorSummaryItems(req, null, vi.fn()) - - const errorSummary = req.errorSummary - expect(errorSummary.items).toEqual([ - { - html: 'issue in entity with reference entity1', - href: 'baseSubpath-value/entity/1' - }, - { - html: 'issue in entity with reference entity2', - href: 'baseSubpath-value/entity/2' - } - ]) - }) - - it('handles no entities provided, but a resource provided with less issues than entries', () => { + it('handles no entities provided, but a resource provided', () => { const req = { params: { issue_type: 'issue-type-value', @@ -1429,14 +1380,11 @@ describe('getErrorSummaryItems', () => { const errorSummary = req.errorSummary expect(errorSummary.items).toEqual([ { - html: 'issue in entity with reference 1', - href: 'baseSubpath-value/entry/1' - }, - { - html: 'issue in entity with reference 2', - href: 'baseSubpath-value/entry/2' + html: 'issue' } ]) + + expect(performanceDbApi.getTaskMessage).toHaveBeenCalledWith({ issue_type: 'issue-type-value', num_issues: 2, entityCount: 3, field: 'issue-field-value' }, true) }) }) diff --git a/test/unit/resultsController.test.js b/test/unit/resultsController.test.js index fd0c5976..18249e0d 100644 --- a/test/unit/resultsController.test.js +++ b/test/unit/resultsController.test.js @@ -136,13 +136,14 @@ describe('ResultsController', () => { }, datasetName: req.sessionModel.get('dataset'), - errorSummary: ['error summary'], + errorSummary: [{ text: 'error summary', href: '' }], mappings: { fields: 'geometries' }, geometries: ['geometries'], pagination: 'pagination', requestParams: 'params', template: 'results/no-errors', - id: req.params.id + id: req.params.id, + lastPage: `/check/status/${req.params.id}` }) })