From b6004d3cb779cf172773aed54586dbd7d91374c4 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 15:28:19 +0100 Subject: [PATCH 01/16] fix wording for status messagaes --- src/views/organisations/overview.html | 6 +++--- test/unit/lpaOverviewPage.test.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index e83a6064..cf413784 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -40,17 +40,17 @@

{{datasetsWithEndpoints}}/{{totalDatasets}} - datasets provided + datasets submitted
{{datasetsWithErrors}} - datasets with errors + data URL with errors
{{datasetsWithIssues}} - datasets with issues + datasets need fixing
diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index 0f1518ce..e9817d26 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -91,17 +91,17 @@ describe('LPA Overview Page', () => { const statsBoxes = document.querySelector('.dataset-status').children it('Datasets provided gives the correct value', () => { expect(statsBoxes[0].textContent).toContain('2/8') - expect(statsBoxes[0].textContent).toContain('datasets provided') + expect(statsBoxes[0].textContent).toContain('datasets submitted') }) it('Datasets with errors gives the correct value', () => { expect(statsBoxes[1].textContent).toContain('2') - expect(statsBoxes[1].textContent).toContain('datasets with errors') + expect(statsBoxes[1].textContent).toContain('data URL with errors') }) it('Datasets with issues gives the correct value', () => { expect(statsBoxes[2].textContent).toContain('2') - expect(statsBoxes[2].textContent).toContain('datasets with issues') + expect(statsBoxes[2].textContent).toContain('datasets need fixing') }) const datasetCards = document.querySelector('.govuk-task-list').children From eea521991cd42547ca393b84d205dec4820eef67 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 16:30:32 +0100 Subject: [PATCH 02/16] remove secondary links from dataset cards, update wording --- src/views/organisations/overview.html | 30 ++++----------------------- test/unit/lpaOverviewPage.test.js | 9 +------- 2 files changed, 5 insertions(+), 34 deletions(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index cf413784..d0251bcb 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -81,36 +81,14 @@

{% else %}

Endpoint provided

{% endif %} - -
    - {% if dataset.endpoint is null %} -
  • - Add endpoint -
    • - {% endif %} - {% if dataset.error %} -
  • - Fix errors -
    • - {% elseif dataset.issue %} -
  • - Fix issues -
    • - {% endif %} - {% if dataset.endpoint %} -
  • - View data -
    • - {% endif %} -
- +
{% if dataset.endpoint is null %} {{govukTag({ - text: "Not provided", + text: "Not submitted", classes: "govuk-tag--grey" })}} {% elif dataset.error %} @@ -120,12 +98,12 @@

})}} {% elif dataset.issue %} {{govukTag({ - text: "Issues", + text: "Needs fixing", classes: "govuk-tag--yellow" })}} {% else %} {{govukTag({ - text: "No issues", + text: "Live", classes: "govuk-tag--green" })}} {% endif %} diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index e9817d26..73fff999 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -136,19 +136,12 @@ describe('LPA Overview Page', () => { if (dataset.endpoint) { expectedActions.push({ text: 'View data', href: '/taskLists/taskChecklist' }) } - - const actions = datasetCards[i].querySelector('.planning-data-actions').children - expectedActions.forEach((expectedAction, j) => { - expect(actions[j].textContent, `expect action ${expectedAction.text} for dataset ${dataset.slug}`).toContain(expectedAction.text) - const actionLink = actions[j].querySelector('a') - expect(actionLink.href).toBe(expectedAction.href) - }) }) }) it('Renders the correct status on each dataset card', () => { params.datasets.forEach((dataset, i) => { - const expectedHint = !dataset.endpoint ? 'Not provided' : dataset.error ? 'Error' : dataset.issue ? 'Issues' : 'No issues' + const expectedHint = !dataset.endpoint ? 'Not submitted' : dataset.error ? 'Error' : dataset.issue ? 'Needs fixing' : 'Live' const statusIndicator = datasetCards[i].querySelector('.govuk-task-list__status') expect(statusIndicator.textContent).toContain(expectedHint) From d402aef40dcd414792f667e7adea413823f61e9a Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 16:32:40 +0100 Subject: [PATCH 03/16] wording ('endpoint provided' -> 'data URL submitted' etc) --- src/views/organisations/overview.html | 6 +++--- test/unit/lpaOverviewPage.test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index d0251bcb..72aacbfd 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -73,15 +73,15 @@

{% if dataset.endpoint is null %} -

Endpoint not provided

+

Data URL not submitted

{% elif dataset.error %}

{{dataset.error}}

{% elif dataset.issue %}

{{dataset.issue}}

{% else %} -

Endpoint provided

+

Data URL submitted

{% endif %} - +

diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index 73fff999..e092967f 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -117,7 +117,7 @@ describe('LPA Overview Page', () => { it('The dataset cards are rendered with the correct hints', () => { params.datasets.forEach((dataset, i) => { - const expectedHint = !dataset.endpoint ? 'Endpoint not provided' : dataset.error ? dataset.error : dataset.issue ? dataset.issue : 'Endpoint provided' + const expectedHint = !dataset.endpoint ? 'Data URL not submitted' : dataset.error ? dataset.error : dataset.issue ? dataset.issue : 'Data URL submitted' expect(datasetCards[i].querySelector('.govuk-task-list__hint').textContent).toContain(expectedHint) }) }) From f9d142d7874e7509ffe0a4a7c4c5435f8590f6df Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 16:39:15 +0100 Subject: [PATCH 04/16] change wording of HTTP error status message --- src/services/performanceDbApi.js | 2 +- test/unit/performanceDbApi.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 830d3606..3e7f1020 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -132,7 +132,7 @@ ORDER BY const datasets = result.formattedData.reduce((accumulator, row) => { let error if (row.http_status !== '200' || row.exception) { - error = row.exception ? row.exception : `endpoint returned with a status of ${row.http_status}` + error = row.exception ? row.exception : `There was a ${row.http_status} error accessing the data URL` } let issue diff --git a/test/unit/performanceDbApi.test.js b/test/unit/performanceDbApi.test.js index 71740cd7..eb4df7e6 100644 --- a/test/unit/performanceDbApi.test.js +++ b/test/unit/performanceDbApi.test.js @@ -45,7 +45,7 @@ describe('performanceDbApi', () => { name: 'Some Organisation', organisation: 'some-organisation', datasets: { - 'dataset-slug-1': { endpoint: 'https://example.com/endpoint-1', error: 'endpoint returned with a status of 404', issue: undefined }, + 'dataset-slug-1': { endpoint: 'https://example.com/endpoint-1', error: 'There was a 404 error accessing the data URL', issue: undefined }, 'dataset-slug-2': { endpoint: 'https://example.com/endpoint-2', error: 'resource not found', issue: undefined }, 'dataset-slug-3': { endpoint: 'https://example.com/endpoint-3', error: undefined, issue: 'There are 4 issues in this dataset' } } From 126299208dd44304f9009f59c6d555fc03a32e75 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 16:39:42 +0100 Subject: [PATCH 05/16] remove CTA from overview page --- src/views/organisations/overview.html | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 72aacbfd..250304ed 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -115,10 +115,4 @@

-
-
-

View your task list to fix and improve your datasets

-
-
- {% endblock %} \ No newline at end of file From 497303f88276ea3e4b05d3f88d3aaa6a71c17070 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Fri, 9 Aug 2024 16:41:04 +0100 Subject: [PATCH 06/16] only provide a link to dataset when it submission was successful --- src/views/organisations/overview.html | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 250304ed..98796567 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -66,10 +66,14 @@

Datasets

  • - + {% if dataset.endpoint %} + + {{dataset.slug | datasetSlugToReadableName}} + + {% else %} {{dataset.slug | datasetSlugToReadableName}} - + {% endif %}

    {% if dataset.endpoint is null %} From 884ede941bbdfcd6ce2c300d56793fa7d940e004 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 12 Aug 2024 14:53:30 +0100 Subject: [PATCH 07/16] content changes related to dataset 'status' - arbitrary strings that were sometimes used, sometimes used in combination with other columns, even when it looked like the same information was needed (e.g. `status`, which had values inconsistent with the ones we actually want to surface to the users, I can't find any reason why we would want them to be different) - some post-processing of the data was done (e.g. adding `error` and `issue` columns in code) - I removed that and now the query returns the `error` column, the issue column was removed. - remove needless checks from `perfomanceDbApi.test.js` - another issue 'testing mock data' - no real benefit of doing extensive tests here, better taken care of in other tests (e.g. in `lpaOverviewPage.test.js`) --- src/services/performanceDbApi.js | 44 ++++++++----------- src/views/organisations/overview.html | 41 ++++++++--------- test/unit/lpaOverviewPage.test.js | 63 +++++++++++++++++++-------- test/unit/performanceDbApi.test.js | 31 ++----------- 4 files changed, 85 insertions(+), 94 deletions(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 3e7f1020..dca46cb7 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -80,12 +80,17 @@ export default { rle.resource, rle.exception, rle.status as http_status, - case - when (rle.status != '200') then 'Error' - when (it.severity = 'error') then 'Issue' - when (it.severity = 'warning') then 'Warning' - else 'No issues' - end as status, + case + when (rle.status is null) then 'Not Submitted' + when (rle.status != '200') then 'Error' + when (it.severity = 'error') then 'Need fixing' + when (it.severity = 'warning') then 'Warning' + else 'Live' + end as status, + case + when ((cast(rle.status as integer) > 200)) then format('There was a %s error accessing the data URL', rle.status) + else null + end as error, case when (it.severity = 'info') then '' else i.issue_type @@ -130,24 +135,16 @@ ORDER BY const result = await datasette.runQuery(query) const datasets = result.formattedData.reduce((accumulator, row) => { - let error - if (row.http_status !== '200' || row.exception) { - error = row.exception ? row.exception : `There was a ${row.http_status} error accessing the data URL` - } - - let issue - if (row.issue_count > 0) { - issue = `There are ${row.issue_count} issues in this dataset` - } - accumulator[row.dataset] = { - endpoint: row.endpoint, - error, - issue + endpoint: row.endpoint } return accumulator }, {}) + if (result.formattedData.length === 0) { + throw new Error(`No records found for LPA=${lpa}`) + } + return { name: result.formattedData[0].name, organisation: result.formattedData[0].organisation, @@ -206,13 +203,8 @@ ORDER BY ORDER BY it.severity` const result = await datasette.runQuery(sql) - return result.formattedData.map((row) => { - return { - num_issues: row.num_issues, - issue_type: row.issue_type, - resource: row.resource, - status: row.status - } + return result.formattedData.map(({num_issues, issue_type, resource, status}) => { + return { num_issues, issue_type, resource, status } }) }, diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 98796567..bf0a4e51 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -6,6 +6,18 @@ {% set pageName = organisation.name + " overview" %} {% set serviceType = 'Manage' %} +{% macro tagCardClass(status) %} + {% if status == 'Not submitted' %} + {% set statusCardClasses = 'govuk-tag--grey' %} + {% elif status == 'Error' %} + {% set statusCardClasses = 'govuk-tag--red' %} + {% elif status == 'Need fixing' %} + {% set statusCardClasses = 'govuk-tag--yellow' %} + {% else %} + {% set statusCardClasses = 'govuk-tag--green' %} + {% endif %} +{% endmacro %} + {% block beforeContent %} {{ super() }} @@ -80,8 +92,8 @@

    Data URL not submitted

    {% elif dataset.error %}

    {{dataset.error}}

    - {% elif dataset.issue %} -

    {{dataset.issue}}

    + {% elif dataset.issue_count > 0 %} +

    There are {{ dataset.issue_count }} issues in this dataset

    {% else %}

    Data URL submitted

    {% endif %} @@ -90,27 +102,10 @@

    - {% if dataset.endpoint is null %} - {{govukTag({ - text: "Not submitted", - classes: "govuk-tag--grey" - })}} - {% elif dataset.error %} - {{govukTag({ - text: "Error", - classes: "govuk-tag--red" - })}} - {% elif dataset.issue %} - {{govukTag({ - text: "Needs fixing", - classes: "govuk-tag--yellow" - })}} - {% else %} - {{govukTag({ - text: "Live", - classes: "govuk-tag--green" - })}} - {% endif %} + {{govukTag({ + text: dataset.status, + classes: tagCardClass(dataset.status) + })}}
    • {% endfor %} diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index e092967f..76e02bca 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -35,47 +35,59 @@ describe('LPA Overview Page', () => { datasets: [ { slug: 'article-4-direction', - endpoint: null + endpoint: null, + status: 'Not submitted', + issue_count: 0 }, { slug: 'article-4-direction-area', - endpoint: null + endpoint: null, + status: 'Not submitted' }, { slug: 'conservation-area', endpoint: 'http://conservation-area.json', + status: 'Need fixing', error: null, - issue: 'Endpoint has not been updated since 21 May 2023' + issue: 'Endpoint has not been updated since 21 May 2023', + issue_count: 1 }, { slug: 'conservation-area-document', endpoint: 'http://conservation-area-document.json', + status: 'Live', error: null, - issue: null + issue_count: 0 }, { slug: 'listed-building-outline', endpoint: 'http://listed-building-outline.json', + status: 'Live', error: null, - issue: null + issue_count: 0 }, { slug: 'tree', endpoint: 'http://tree.json', error: null, - issue: 'There are 20 issues in this dataset' + status: 'Need fixing', + issue: 'There are 20 issues in this dataset', + issue_count: 1 }, { slug: 'tree-preservation-order', endpoint: 'http://tree-preservation-order.json', - error: 'Error connecting to endpoint', - issue: null + http_error: '404', + error: 'There was 404 error accessing the data URL', + status: 'Error', + issue_count: 0 }, { slug: 'tree-preservation-zone', endpoint: 'http://tree-preservation-zone.json', - error: 'Error connecting to endpoint', - issue: null + status: 'Error', + error: '400', + issue_count: 0 } ] } @@ -115,10 +127,17 @@ describe('LPA Overview Page', () => { }) }) - it('The dataset cards are rendered with the correct hints', () => { - params.datasets.forEach((dataset, i) => { - const expectedHint = !dataset.endpoint ? 'Data URL not submitted' : dataset.error ? dataset.error : dataset.issue ? dataset.issue : 'Data URL submitted' - expect(datasetCards[i].querySelector('.govuk-task-list__hint').textContent).toContain(expectedHint) + params.datasets.forEach((dataset, i) => { + it(`dataset cards are rendered with correct hints for dataset='${dataset.slug}'`, () => { + let expectedHint = 'Data URL submitted' + if (dataset.status === 'Not submitted') { + expectedHint = 'Data URL not submitted' + } else if (dataset.error) { + expectedHint = dataset.error + } else if (dataset.issue_count > 0) { + expectedHint = `There are ${dataset.issue_count} issues in this dataset` + } + expect(datasetCards[i].querySelector('.govuk-task-list__hint').textContent.trim()).toContain(expectedHint) }) }) @@ -139,12 +158,20 @@ describe('LPA Overview Page', () => { }) }) - it('Renders the correct status on each dataset card', () => { - params.datasets.forEach((dataset, i) => { - const expectedHint = !dataset.endpoint ? 'Not submitted' : dataset.error ? 'Error' : dataset.issue ? 'Needs fixing' : 'Live' + params.datasets.forEach((dataset, i) => { + it(`Renders the correct status on each dataset card for dataset='${dataset.slug}'`, () => { + let expectedHint = 'Live' + if (dataset.status === 'Not submitted') { + expectedHint = 'Not submitted' + } else if (dataset.status === 'Error') { + expectedHint = dataset.status + } else if (dataset.status === 'Need fixing') { + expectedHint = 'Need fixing' + } + const statusIndicator = datasetCards[i].querySelector('.govuk-task-list__status') - expect(statusIndicator.textContent).toContain(expectedHint) + expect(statusIndicator.textContent.trim()).toContain(expectedHint) }) }) }) diff --git a/test/unit/performanceDbApi.test.js b/test/unit/performanceDbApi.test.js index eb4df7e6..82dd6fd2 100644 --- a/test/unit/performanceDbApi.test.js +++ b/test/unit/performanceDbApi.test.js @@ -14,42 +14,19 @@ describe('performanceDbApi', () => { dataset: 'dataset-slug-1', endpoint: 'https://example.com/endpoint-1', exception: null, - http_status: '404' - }, - { - organisation: 'some-organisation', - name: 'Some Organisation', - dataset: 'dataset-slug-2', - endpoint: 'https://example.com/endpoint-2', - exception: 'resource not found', - http_status: '404' - }, - { - organisation: 'some-organisation', - name: 'Some Organisation', - dataset: 'dataset-slug-3', - endpoint: 'https://example.com/endpoint-3', - http_status: '200', - issue_count: 4 + http_status: undefined, + error: undefined, + status: 'Live' } ] } vi.spyOn(datasette, 'runQuery').mockResolvedValue(mockResponse) - const result = await performanceDbApi.getLpaOverview(lpa) + await performanceDbApi.getLpaOverview(lpa) expect(datasette.runQuery).toHaveBeenCalledTimes(1) expect(datasette.runQuery).toHaveBeenCalledWith(expect.stringContaining(lpa)) - expect(result).toEqual({ - name: 'Some Organisation', - organisation: 'some-organisation', - datasets: { - 'dataset-slug-1': { endpoint: 'https://example.com/endpoint-1', error: 'There was a 404 error accessing the data URL', issue: undefined }, - 'dataset-slug-2': { endpoint: 'https://example.com/endpoint-2', error: 'resource not found', issue: undefined }, - 'dataset-slug-3': { endpoint: 'https://example.com/endpoint-3', error: undefined, issue: 'There are 4 issues in this dataset' } - } - }) }) it('adds the filter if a dataset list is passed into the params', async () => { From bb283cbd163ca71bda93ef25acf23a0d561b05d8 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 12 Aug 2024 14:59:08 +0100 Subject: [PATCH 08/16] fix linter issues --- src/services/performanceDbApi.js | 3 ++- test/unit/lpaOverviewPage.test.js | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index dca46cb7..243574bb 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -203,7 +203,8 @@ ORDER BY ORDER BY it.severity` const result = await datasette.runQuery(sql) - return result.formattedData.map(({num_issues, issue_type, resource, status}) => { + /* eslint camelcase: "off" */ + return result.formattedData.map(({ num_issues, issue_type, resource, status }) => { return { num_issues, issue_type, resource, status } }) }, diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index 76e02bca..09216406 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -158,7 +158,6 @@ describe('LPA Overview Page', () => { }) }) - params.datasets.forEach((dataset, i) => { it(`Renders the correct status on each dataset card for dataset='${dataset.slug}'`, () => { let expectedHint = 'Live' @@ -169,7 +168,7 @@ describe('LPA Overview Page', () => { } else if (dataset.status === 'Need fixing') { expectedHint = 'Need fixing' } - + const statusIndicator = datasetCards[i].querySelector('.govuk-task-list__status') expect(statusIndicator.textContent.trim()).toContain(expectedHint) }) From 735b73f86fee98cca4cfcd47dc0466836e3cf9f1 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Mon, 12 Aug 2024 15:14:20 +0100 Subject: [PATCH 09/16] fix wording --- src/services/performanceDbApi.js | 2 +- src/views/organisations/overview.html | 2 +- test/unit/lpaOverviewPage.test.js | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 243574bb..1a46d3ea 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -83,7 +83,7 @@ export default { case when (rle.status is null) then 'Not Submitted' when (rle.status != '200') then 'Error' - when (it.severity = 'error') then 'Need fixing' + when (it.severity = 'error') then 'Needs fixing' when (it.severity = 'warning') then 'Warning' else 'Live' end as status, diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index bf0a4e51..9b5462ba 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -11,7 +11,7 @@ {% set statusCardClasses = 'govuk-tag--grey' %} {% elif status == 'Error' %} {% set statusCardClasses = 'govuk-tag--red' %} - {% elif status == 'Need fixing' %} + {% elif status == 'Needs fixing' %} {% set statusCardClasses = 'govuk-tag--yellow' %} {% else %} {% set statusCardClasses = 'govuk-tag--green' %} diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index 09216406..a504695a 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -47,7 +47,7 @@ describe('LPA Overview Page', () => { { slug: 'conservation-area', endpoint: 'http://conservation-area.json', - status: 'Need fixing', + status: 'Needs fixing', error: null, issue: 'Endpoint has not been updated since 21 May 2023', issue_count: 1 @@ -70,7 +70,7 @@ describe('LPA Overview Page', () => { slug: 'tree', endpoint: 'http://tree.json', error: null, - status: 'Need fixing', + status: 'Needs fixing', issue: 'There are 20 issues in this dataset', issue_count: 1 }, @@ -165,8 +165,8 @@ describe('LPA Overview Page', () => { expectedHint = 'Not submitted' } else if (dataset.status === 'Error') { expectedHint = dataset.status - } else if (dataset.status === 'Need fixing') { - expectedHint = 'Need fixing' + } else if (dataset.status === 'Needs fixing') { + expectedHint = 'Needs fixing' } const statusIndicator = datasetCards[i].querySelector('.govuk-task-list__status') From 5d29d240189e7cabcadaed254ef4bbeb8fca81c6 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 13:04:26 +0100 Subject: [PATCH 10/16] fix wrong breadcrumb url --- src/views/organisations/overview.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 9b5462ba..59d011e8 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -29,7 +29,7 @@ }, { text: "Organisations", - href: "/overview/organisations" + href: "/organisations" } ] }) }} From afd7f951036f78ae7c62fb52076f7306fd93f576 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 13:05:17 +0100 Subject: [PATCH 11/16] fix: more useful log messages --- src/controllers/OrganisationsController.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 8c254f27..86ead25c 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -100,7 +100,7 @@ const organisationsController = { res.render('organisations/overview.html', params) } catch (error) { - logger.error(error) + logger.warn('organisationsController.getOverview(): ' + error.message ?? error.errorMessage, {type: types.App}) next(error) } }, @@ -130,7 +130,7 @@ const organisationsController = { res.render('organisations/find.html', { alphabetisedOrgs }) } catch (err) { - logger.warn(err) + logger.warn('organisationsController.getOrganisations(): ' + err.message ?? err.errorMessage, {type: types.App}) next(err) } }, From 294ae9666f0d7f2caf6b8d4c7650a505984f286e Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 13:06:00 +0100 Subject: [PATCH 12/16] more useful log messages --- src/controllers/OrganisationsController.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 86ead25c..5d32f943 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -100,7 +100,7 @@ const organisationsController = { res.render('organisations/overview.html', params) } catch (error) { - logger.warn('organisationsController.getOverview(): ' + error.message ?? error.errorMessage, {type: types.App}) + logger.warn('organisationsController.getOverview(): ' + error.message ?? error.errorMessage, { type: types.App }) next(error) } }, @@ -130,7 +130,7 @@ const organisationsController = { res.render('organisations/find.html', { alphabetisedOrgs }) } catch (err) { - logger.warn('organisationsController.getOrganisations(): ' + err.message ?? err.errorMessage, {type: types.App}) + logger.warn('organisationsController.getOrganisations(): ' + err.message ?? err.errorMessage, { type: types.App }) next(err) } }, From b0cc7393d3a22bf755b9a2c68ea31f4282703743 Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 15:45:16 +0100 Subject: [PATCH 13/16] fix: tag class lookup, missing 'status', incorrect status counts --- src/controllers/OrganisationsController.js | 19 ++++------- src/filters/filters.js | 20 ++++++++++- src/services/datasette.js | 2 +- src/services/performanceDbApi.js | 39 +++++++++++----------- src/views/organisations/overview.html | 14 +------- 5 files changed, 48 insertions(+), 46 deletions(-) diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 5d32f943..9085b4b8 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -3,6 +3,7 @@ import performanceDbApi from '../services/performanceDbApi.js' // Assume you hav import logger from '../utils/logger.js' import { types } from '../utils/logging.js' import { dataSubjects } from '../utils/utils.js' +import { statusToTagClass } from '../filters/filters.js' // get a list of available datasets const availableDatasets = Object.values(dataSubjects) @@ -19,17 +20,10 @@ const availableDatasets = Object.values(dataSubjects) * @returns {object} - An object with a `tag` property containing the text label and CSS class. */ function getStatusTag (status) { - const statusToTagClass = { - Error: 'govuk-tag--red', - 'Needs fixing': 'govuk-tag--yellow', - Warning: 'govuk-tag--blue', - Issue: 'govuk-tag--blue' - } - return { tag: { text: status, - classes: statusToTagClass[status] + classes: statusToTagClass(status) } } } @@ -73,16 +67,17 @@ const organisationsController = { if (!keys.includes(dataset)) { datasets.push({ slug: dataset, - endpoint: null + endpoint: null, + status: 'Not submitted' }) } }) const totalDatasets = datasets.length const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = datasets.reduce((accumulator, dataset) => { - if (dataset.endpoint !== null) accumulator[0]++ - if (dataset.issue) accumulator[1]++ - if (dataset.error) accumulator[2]++ + if (dataset.status === 'Live' || dataset.status === 'Warning') accumulator[0]++ + if (dataset.status === 'Needs fixing') accumulator[1]++ + if (dataset.status === 'Error') accumulator[2]++ return accumulator }, [0, 0, 0]) diff --git a/src/filters/filters.js b/src/filters/filters.js index 2ce7bdb5..514caedf 100644 --- a/src/filters/filters.js +++ b/src/filters/filters.js @@ -6,6 +6,23 @@ import prettifyColumnName from './prettifyColumnName.js' import getFullServiceName from './getFullServiceName.js' import { makeDatasetSlugToReadableNameFilter } from './makeDatasetSlugToReadableNameFilter.js' +/** maps dataset status (as returned by performanceDbApi/getLpaOverview()) to a + * CSS class used by the govuk-tag component + */ +const statusToTagClassMapping = { + Error: 'govuk-tag--red', + 'Not submitted': 'govuk-tag--red', + 'Needs fixing': 'govuk-tag--yellow', + Warning: 'govuk-tag--blue', + Issue: 'govuk-tag--blue', // deprecated + Live: 'govuk-tag--green' +} + +export function statusToTagClass(status) { + console.assert(status in statusToTagClassMapping, `statusToTagClass: unknown status ${status}`) + return statusToTagClassMapping[status] +} + const { govukMarkdown, govukDateTime } = xGovFilters const addFilters = (nunjucksEnv, { datasetNameMapping }) => { @@ -19,7 +36,8 @@ const addFilters = (nunjucksEnv, { datasetNameMapping }) => { nunjucksEnv.addFilter('validationMessageLookup', validationMessageLookup) nunjucksEnv.addFilter('toErrorList', toErrorList) nunjucksEnv.addFilter('prettifyColumnName', prettifyColumnName) - nunjucksEnv.addFilter('getFullServiceName', getFullServiceName) + nunjucksEnv.addFilter('getFullServiceName', getFullServiceName), + nunjucksEnv.addFilter('statusToTagClass', statusToTagClass) } export default addFilters diff --git a/src/services/datasette.js b/src/services/datasette.js index 0048269b..976b1bb9 100644 --- a/src/services/datasette.js +++ b/src/services/datasette.js @@ -8,7 +8,7 @@ export default { * Executes a SQL query on the Datasette instance and returns the results. * * @param {string} query - The SQL query to execute. - * @returns {Promise<{data: object, formattedData: object}>} - A promise that resolves to an object with the following properties: + * @returns {Promise<{data: object, formattedData: object[]}>} - A promise that resolves to an object with the following properties: * - `data`: The raw data returned by Datasette. * - `formattedData`: The formatted data, with columns and rows parsed into a usable format. * @throws {Error} If the query fails or there is an error communicating with Datasette. diff --git a/src/services/performanceDbApi.js b/src/services/performanceDbApi.js index 1a46d3ea..19fb6700 100644 --- a/src/services/performanceDbApi.js +++ b/src/services/performanceDbApi.js @@ -41,9 +41,9 @@ fs.createReadStream('src/content/entityIssueMessages.csv') /** * @typedef {object} Dataset + * @property {'Not submitted' | 'Error' | 'Needs fixing' | 'Warning' | 'Live' } status * @property {string} endpoint * @property {?string} error - * @property {?string} issue */ /** @@ -86,26 +86,26 @@ export default { when (it.severity = 'error') then 'Needs fixing' when (it.severity = 'warning') then 'Warning' else 'Live' - end as status, + end as status, case when ((cast(rle.status as integer) > 200)) then format('There was a %s error accessing the data URL', rle.status) else null - end as error, - case - when (it.severity = 'info') then '' - else i.issue_type - end as issue_type, - case - when (it.severity = 'info') then '' - else it.severity - end as severity, - it.responsibility, - COUNT( - case - when it.severity != 'info' then 1 - else null - end - ) as issue_count + end as error, + case + when (it.severity = 'info') then '' + else i.issue_type + end as issue_type, + case + when (it.severity = 'info') then '' + else it.severity + end as severity, + it.responsibility, + COUNT( + case + when it.severity != 'info' then 1 + else null + end + ) as issue_count FROM provision p LEFT JOIN @@ -136,7 +136,8 @@ ORDER BY const datasets = result.formattedData.reduce((accumulator, row) => { accumulator[row.dataset] = { - endpoint: row.endpoint + endpoint: row.endpoint, + status: row.status } return accumulator }, {}) diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 59d011e8..dcd7a6da 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -6,18 +6,6 @@ {% set pageName = organisation.name + " overview" %} {% set serviceType = 'Manage' %} -{% macro tagCardClass(status) %} - {% if status == 'Not submitted' %} - {% set statusCardClasses = 'govuk-tag--grey' %} - {% elif status == 'Error' %} - {% set statusCardClasses = 'govuk-tag--red' %} - {% elif status == 'Needs fixing' %} - {% set statusCardClasses = 'govuk-tag--yellow' %} - {% else %} - {% set statusCardClasses = 'govuk-tag--green' %} - {% endif %} -{% endmacro %} - {% block beforeContent %} {{ super() }} @@ -104,7 +92,7 @@

    {{govukTag({ text: dataset.status, - classes: tagCardClass(dataset.status) + classes: dataset.status | statusToTagClass })}}
    From 62dbb8ed741ed949deb256f6b770fe920d912e9d Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 15:47:05 +0100 Subject: [PATCH 14/16] fix linter issues --- src/filters/filters.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/filters/filters.js b/src/filters/filters.js index 514caedf..0dcdb5cb 100644 --- a/src/filters/filters.js +++ b/src/filters/filters.js @@ -6,8 +6,8 @@ import prettifyColumnName from './prettifyColumnName.js' import getFullServiceName from './getFullServiceName.js' import { makeDatasetSlugToReadableNameFilter } from './makeDatasetSlugToReadableNameFilter.js' -/** maps dataset status (as returned by performanceDbApi/getLpaOverview()) to a - * CSS class used by the govuk-tag component +/** maps dataset status (as returned by performanceDbApi/getLpaOverview()) to a + * CSS class used by the govuk-tag component */ const statusToTagClassMapping = { Error: 'govuk-tag--red', @@ -18,7 +18,7 @@ const statusToTagClassMapping = { Live: 'govuk-tag--green' } -export function statusToTagClass(status) { +export function statusToTagClass (status) { console.assert(status in statusToTagClassMapping, `statusToTagClass: unknown status ${status}`) return statusToTagClassMapping[status] } @@ -36,7 +36,7 @@ const addFilters = (nunjucksEnv, { datasetNameMapping }) => { nunjucksEnv.addFilter('validationMessageLookup', validationMessageLookup) nunjucksEnv.addFilter('toErrorList', toErrorList) nunjucksEnv.addFilter('prettifyColumnName', prettifyColumnName) - nunjucksEnv.addFilter('getFullServiceName', getFullServiceName), + nunjucksEnv.addFilter('getFullServiceName', getFullServiceName) nunjucksEnv.addFilter('statusToTagClass', statusToTagClass) } From e08c3847f3693f4ff706bcfdad51a63884990c3e Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 15:59:47 +0100 Subject: [PATCH 15/16] count all datasets with set endpoints, ignoring status --- src/controllers/OrganisationsController.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 9085b4b8..6bed4b65 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -75,7 +75,7 @@ const organisationsController = { const totalDatasets = datasets.length const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = datasets.reduce((accumulator, dataset) => { - if (dataset.status === 'Live' || dataset.status === 'Warning') accumulator[0]++ + if (dataset.endpoint) accumulator[0]++ if (dataset.status === 'Needs fixing') accumulator[1]++ if (dataset.status === 'Error') accumulator[2]++ return accumulator From c5a29ca6e607842416a86a89f47064d8c6c5c64a Mon Sep 17 00:00:00 2001 From: Roland Sadowski Date: Tue, 13 Aug 2024 15:59:58 +0100 Subject: [PATCH 16/16] fix test data --- test/unit/organisationsController.test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/unit/organisationsController.test.js b/test/unit/organisationsController.test.js index a2fca79e..14d0af06 100644 --- a/test/unit/organisationsController.test.js +++ b/test/unit/organisationsController.test.js @@ -31,9 +31,9 @@ describe('OrganisationsController.js', () => { const expectedResponse = { name: 'Test LPA', datasets: { - dataset1: { endpoint: 'https://example.com', issue: false, error: false }, - dataset2: { endpoint: null, issue: true, error: false }, - dataset3: { endpoint: 'https://example.com', issue: false, error: true } + dataset1: { endpoint: 'https://example.com', status: 'Live' }, + dataset2: { endpoint: null, status: 'Needs fixing' }, + dataset3: { endpoint: 'https://example.com', status: 'Error' } } } @@ -45,9 +45,9 @@ describe('OrganisationsController.js', () => { expect(res.render).toHaveBeenCalledWith('organisations/overview.html', expect.objectContaining({ organisation: { name: 'Test LPA' }, datasets: expect.arrayContaining([ - { endpoint: 'https://example.com', issue: false, error: false, slug: 'dataset1' }, - { endpoint: null, issue: true, error: false, slug: 'dataset2' }, - { endpoint: 'https://example.com', issue: false, error: true, slug: 'dataset3' } + { endpoint: 'https://example.com', status: 'Live', slug: 'dataset1' }, + { endpoint: null, status: 'Needs fixing', slug: 'dataset2' }, + { endpoint: 'https://example.com', status: 'Error', slug: 'dataset3' } ]), totalDatasets: 3, datasetsWithEndpoints: 2,