Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check tool: update 'errors' page design & content #640

Merged
merged 30 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
607f2da
check: error result page - design and content changes
rosado Nov 7, 2024
2cc6a49
check: error result page - update tests
rosado Nov 7, 2024
e949297
check: update integration tests
rosado Nov 8, 2024
66d062e
check: update acceptance tests
rosado Nov 8, 2024
e67e730
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 11, 2024
c08a0f0
update guidance link
rosado Nov 12, 2024
210669a
Revert "update guidance link"
rosado Nov 12, 2024
6e2a8ad
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 12, 2024
42d10c3
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 12, 2024
b9d41b3
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 12, 2024
ffc1f42
check tool: add CSS class to date columns in errors table
rosado Nov 14, 2024
b6c36e7
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 14, 2024
c35c66c
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 18, 2024
03e3301
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 19, 2024
7cbbdb9
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 19, 2024
923d8d0
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 20, 2024
0165933
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Nov 21, 2024
de3edc3
add back links on status and results pages
rosado Nov 21, 2024
0033733
update result controller test
rosado Nov 21, 2024
580e703
use proper name of the dataset on confirmation page
rosado Nov 21, 2024
44ff474
remove back button - wrong error page
rosado Nov 21, 2024
4670e90
back clink on confirmation page
rosado Nov 21, 2024
5e65c9a
check: move the map above the table
rosado Nov 21, 2024
b4c1d51
check: add aria-label to the element containing the map
rosado Nov 21, 2024
d89899d
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Dec 2, 2024
64efa68
check: fix capitalisation
rosado Dec 2, 2024
aeab45d
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Dec 2, 2024
c53b036
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Dec 3, 2024
9e0fcfe
Merge branch 'main' into rosado/571-content-and-styling-2
rosado Dec 4, 2024
d1f8aa5
ensure readable dataset name is displayed in dataset banner
rosado Dec 4, 2024
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: 1 addition & 1 deletion src/content/statusPage.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export const headingTexts = {
checking: 'Checking your data',
checked: 'Data Checked'
checked: 'Data checked'
}

export const messageTexts = {
Expand Down
8 changes: 7 additions & 1 deletion src/controllers/resultsController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
20 changes: 20 additions & 0 deletions src/controllers/statusController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions src/models/requestData.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
Expand Down
12 changes: 10 additions & 2 deletions src/views/check/confirmation.html
Original file line number Diff line number Diff line change
@@ -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 %}

Expand All @@ -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
Expand All @@ -31,7 +39,7 @@
Details about what to do next are also available [in the guidance](/guidance)

{% if options.deepLink %}
<p><a href="{{ options.deepLink.referrer }}">Return to {{ options.deepLink.dataset }} overview</a></p>
<p><a href="{{ options.deepLink.referrer }}">Return to {{ options.deepLink.dataset | datasetSlugToReadableName }} overview</a></p>
{% else %}
<p><a href="/">Return to Home</a></p>
{% endif %}
Expand Down
55 changes: 40 additions & 15 deletions src/views/check/results/errors.html
Original file line number Diff line number Diff line change
Expand Up @@ -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' %}
Expand Down Expand Up @@ -36,23 +36,22 @@ <h1 class="govuk-heading-l">
{{pageName}}
</h1>

<ul class="govuk-list govuk-list--bullet">
{% for summaryMessage in options.errorSummary %}
<li>
{{summaryMessage}}
</li>
{% endfor %}
</ul>
{{ govukErrorSummary({
titleText: "There’s a problem",
errorList: options.errorSummary
}) }}
rosado marked this conversation as resolved.
Show resolved Hide resolved

</div>
</div>

<div class="govuk-grid-row">

<div class="govuk-grid-column-full">
{% if options.tableParams.rows|length > 0 %}
<h2 class="govuk-heading-m">
Check your errors
</h2>
{% if options.geometries %}
<div id="map" class="app-map" aria-label="Map showing contour of the submitted data on a map"></div>
{% endif %}

{% if options.tableParams.rows|length > 0 %}
{{ table(options.tableParams) }}
{% endif %}

Expand All @@ -72,11 +71,37 @@ <h2 class="govuk-heading-m">

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">

{% if options.deepLink.orgName %}
<h3 class="govuk-heading-m">How to improve {{ options.deepLink.orgName }}'s data</h3>
{% else %}
<h3 class="govuk-heading-m">How to improve the data</h3>
{% endif %}

<ol class="govuk-list govuk-list--number">
<li>Fix the errors indicated. You should use the <a href="/guidance/specifications/">data specifications</a> to help you do this.</li>
<li>Check your updated data to make sure it meets the specifications.</li>
</ol>

{{ govukButton({
text: "Upload a new version",
text: "Check your data",
href: "/check/upload-method"
}) }}

</div>
</div>
{% endblock %}

{% block scripts %}
{{ super() }}
{% if options.geometries %}
<link href='https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.css' rel='stylesheet' />
rosado marked this conversation as resolved.
Show resolved Hide resolved
<script>
window.serverContext = {
containerId: 'map',
geometries: {{ options.geometries | dump | safe }},
}
</script>
<script src="/public/js/map.bundle.js"></script>
{% endif %}
{% endblock %}
11 changes: 4 additions & 7 deletions src/views/check/results/no-errors.html
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ <h1 class="govuk-heading-l">
<div class="govuk-grid-row">
<div class="govuk-grid-column-full">

{% if options.geometries %}
<div id="map" class="app-map" aria-label="Map showing contour of the submitted data on a map"></div>
{% endif %}

{{ table(options.tableParams) }}

{% if options.pagination.items | length > 1 %}
Expand All @@ -69,13 +73,6 @@ <h1 class="govuk-heading-l">
{% endif %}



{% if options.geometries %}
<p>Here’s your data on a map:</p>
<div id="map" class="app-map"></div>
{% endif %}


</div>
</div>

Expand Down
10 changes: 10 additions & 0 deletions src/views/check/statusPage/status.html
Original file line number Diff line number Diff line change
@@ -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 %}

Expand All @@ -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 %}
<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds" aria-live="assertive">
Expand Down
2 changes: 1 addition & 1 deletion src/views/components/dataset-banner.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{% macro datasetBanner(datasetName) %}
<span class="govuk-caption-xl">{{ datasetName }}</span>
<span class="govuk-caption-xl">{{ datasetName | datasetSlugToReadableName }}</span>
{% endmacro %}
5 changes: 5 additions & 0 deletions src/views/components/table.html
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@
{% macro table(params) %}
<div class="app-scrollable-container dl-scrollable">
<table class="govuk-table dl-table">
<colgroup>
{% for field in params.fields %}
<col class="{% if (r/.*-date/g).test(field) %}govuk-table__cell--numeric{% endif %}" />
{% endfor %}
</colgroup>
rosado marked this conversation as resolved.
Show resolved Hide resolved
<thead class="govuk-table__head dl-table__head">
<tr class="govuk-table__row">
{% for column in params.columns %}
Expand Down
2 changes: 0 additions & 2 deletions src/views/error.html
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@

{% from "govuk/components/button/macro.njk" import govukButton %}

{% set pageName = "Error" %}

{% extends "layouts/main.html" %}

{% block content %}
<h1 class="govuk-heading-xl">Error</h1>

Expand Down
4 changes: 2 additions & 2 deletions test/PageObjectModels/resultsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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())
rosado marked this conversation as resolved.
Show resolved Hide resolved
})

Expand Down
4 changes: 1 addition & 3 deletions test/integration/test_recieving_results.playwright.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
11 changes: 9 additions & 2 deletions test/unit/errorsPage.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ describe('errors page', () => {
limit: 50
}

const summaryItem = (errorMessage) => {
return {
text: errorMessage,
href: ''
}
}

const params = {
options: {
tableParams: {
Expand All @@ -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)
Expand All @@ -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
Expand Down
5 changes: 3 additions & 2 deletions test/unit/resultsController.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}`
})
})

Expand Down
Loading