From 5a6d04d36a86827e12bc7b23d111b728d6991b5d Mon Sep 17 00:00:00 2001 From: George Goodall Date: Tue, 13 Aug 2024 16:57:46 +0100 Subject: [PATCH] Revert "Merge pull request #265 from digital-land/rosado/content-changes" This reverts commit 2d55d3269063e0a4083dc25a180994765d41b171, reversing changes made to f00bbb0c86897cac25a0d33d91b421330927ddac. --- src/controllers/OrganisationsController.js | 23 ++++--- src/filters/filters.js | 18 ----- src/services/datasette.js | 2 +- src/services/performanceDbApi.js | 74 +++++++++++---------- src/views/organisations/overview.html | 77 +++++++++++++++++----- test/unit/lpaOverviewPage.test.js | 75 ++++++++------------- test/unit/organisationsController.test.js | 12 ++-- test/unit/performanceDbApi.test.js | 31 +++++++-- 8 files changed, 175 insertions(+), 137 deletions(-) diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 7674525f..34ef8eff 100644 --- a/src/controllers/OrganisationsController.js +++ b/src/controllers/OrganisationsController.js @@ -3,7 +3,6 @@ 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) @@ -20,10 +19,17 @@ 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] } } } @@ -70,17 +76,16 @@ const organisationsController = { if (!keys.includes(dataset)) { datasets.push({ slug: dataset, - endpoint: null, - status: 'Not submitted' + endpoint: null }) } }) const totalDatasets = datasets.length const [datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors] = datasets.reduce((accumulator, dataset) => { - if (dataset.endpoint) accumulator[0]++ - if (dataset.status === 'Needs fixing') accumulator[1]++ - if (dataset.status === 'Error') accumulator[2]++ + if (dataset.endpoint !== null) accumulator[0]++ + if (dataset.issue) accumulator[1]++ + if (dataset.error) accumulator[2]++ return accumulator }, [0, 0, 0]) @@ -95,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.error(error) next(error) } }, @@ -125,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(err) next(err) } }, diff --git a/src/filters/filters.js b/src/filters/filters.js index 0dcdb5cb..2ce7bdb5 100644 --- a/src/filters/filters.js +++ b/src/filters/filters.js @@ -6,23 +6,6 @@ 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 }) => { @@ -37,7 +20,6 @@ const addFilters = (nunjucksEnv, { datasetNameMapping }) => { nunjucksEnv.addFilter('toErrorList', toErrorList) nunjucksEnv.addFilter('prettifyColumnName', prettifyColumnName) nunjucksEnv.addFilter('getFullServiceName', getFullServiceName) - nunjucksEnv.addFilter('statusToTagClass', statusToTagClass) } export default addFilters diff --git a/src/services/datasette.js b/src/services/datasette.js index 976b1bb9..0048269b 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 e5785dd7..4352b3ed 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 */ /** @@ -80,32 +80,27 @@ export default { rle.resource, rle.exception, rle.status as http_status, - case - when (rle.status is null) then 'Not Submitted' - when (rle.status != '200') then 'Error' - when (it.severity = 'error') then 'Needs fixing' - when (it.severity = 'warning') then 'Warning' - else 'Live' - end as 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 ((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 + 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 @@ -135,17 +130,24 @@ 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 : `endpoint returned with a status of ${row.http_status}` + } + + let issue + if (row.issue_count > 0) { + issue = `There are ${row.issue_count} issues in this dataset` + } + accumulator[row.dataset] = { endpoint: row.endpoint, - status: row.status + error, + issue } return accumulator }, {}) - if (result.formattedData.length === 0) { - throw new Error(`No records found for LPA=${lpa}`) - } - return { datasets } @@ -202,9 +204,13 @@ ORDER BY ORDER BY it.severity` const result = await datasette.runQuery(sql) - /* eslint camelcase: "off" */ - return result.formattedData.map(({ num_issues, issue_type, resource, status }) => { - return { num_issues, issue_type, resource, status } + return result.formattedData.map((row) => { + return { + num_issues: row.num_issues, + issue_type: row.issue_type, + resource: row.resource, + status: row.status + } }) }, diff --git a/src/views/organisations/overview.html b/src/views/organisations/overview.html index 48f43f08..1325780e 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -43,17 +43,17 @@

{{datasetsWithEndpoints}}/{{totalDatasets}} - datasets submitted + datasets provided
{{datasetsWithErrors}} - data URL with errors + datasets with errors
{{datasetsWithIssues}} - datasets need fixing + datasets with issues
@@ -69,34 +69,69 @@

Datasets

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

    {% if dataset.endpoint is null %} -

    Data URL not submitted

    +

    Endpoint not provided

    {% elif dataset.error %}

    {{dataset.error}}

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

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

    + {% elif dataset.issue %} +

    {{dataset.issue}}

    {% else %} -

    Data URL submitted

    +

    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 %} +
    +
    - {{govukTag({ - text: dataset.status, - classes: dataset.status | statusToTagClass - })}} + {% if dataset.endpoint is null %} + {{govukTag({ + text: "Not provided", + classes: "govuk-tag--grey" + })}} + {% elif dataset.error %} + {{govukTag({ + text: "Error", + classes: "govuk-tag--red" + })}} + {% elif dataset.issue %} + {{govukTag({ + text: "Issues", + classes: "govuk-tag--yellow" + })}} + {% else %} + {{govukTag({ + text: "No issues", + classes: "govuk-tag--green" + })}} + {% endif %}
    • {% endfor %} @@ -105,4 +140,10 @@

    +
    +
    +

    View your task list to fix and improve your datasets

    +
    +
    + {% endblock %} \ No newline at end of file diff --git a/test/unit/lpaOverviewPage.test.js b/test/unit/lpaOverviewPage.test.js index 2c89567a..8b1cbaa2 100644 --- a/test/unit/lpaOverviewPage.test.js +++ b/test/unit/lpaOverviewPage.test.js @@ -36,59 +36,47 @@ describe('LPA Overview Page', () => { datasets: [ { slug: 'article-4-direction', - endpoint: null, - status: 'Not submitted', - issue_count: 0 + endpoint: null }, { slug: 'article-4-direction-area', - endpoint: null, - status: 'Not submitted' + endpoint: null }, { slug: 'conservation-area', endpoint: 'http://conservation-area.json', - status: 'Needs fixing', error: null, - issue: 'Endpoint has not been updated since 21 May 2023', - issue_count: 1 + issue: 'Endpoint has not been updated since 21 May 2023' }, { slug: 'conservation-area-document', endpoint: 'http://conservation-area-document.json', - status: 'Live', error: null, - issue_count: 0 + issue: null }, { slug: 'listed-building-outline', endpoint: 'http://listed-building-outline.json', - status: 'Live', error: null, - issue_count: 0 + issue: null }, { slug: 'tree', endpoint: 'http://tree.json', error: null, - status: 'Needs fixing', - issue: 'There are 20 issues in this dataset', - issue_count: 1 + issue: 'There are 20 issues in this dataset' }, { slug: 'tree-preservation-order', endpoint: 'http://tree-preservation-order.json', - http_error: '404', - error: 'There was 404 error accessing the data URL', - status: 'Error', - issue_count: 0 + error: 'Error connecting to endpoint', + issue: null }, { slug: 'tree-preservation-zone', endpoint: 'http://tree-preservation-zone.json', - status: 'Error', - error: '400', - issue_count: 0 + error: 'Error connecting to endpoint', + issue: null } ] } @@ -105,17 +93,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 submitted') + expect(statsBoxes[0].textContent).toContain('datasets provided') }) it('Datasets with errors gives the correct value', () => { expect(statsBoxes[1].textContent).toContain('2') - expect(statsBoxes[1].textContent).toContain('data URL with errors') + expect(statsBoxes[1].textContent).toContain('datasets with errors') }) it('Datasets with issues gives the correct value', () => { expect(statsBoxes[2].textContent).toContain('2') - expect(statsBoxes[2].textContent).toContain('datasets need fixing') + expect(statsBoxes[2].textContent).toContain('datasets with issues') }) const datasetCards = document.querySelector('.govuk-task-list').children @@ -129,17 +117,10 @@ describe('LPA Overview Page', () => { }) }) - 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) + 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' + expect(datasetCards[i].querySelector('.govuk-task-list__hint').textContent).toContain(expectedHint) }) }) @@ -157,22 +138,22 @@ 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) + }) }) }) - 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 === 'Needs fixing') { - expectedHint = 'Needs fixing' - } + 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 statusIndicator = datasetCards[i].querySelector('.govuk-task-list__status') - expect(statusIndicator.textContent.trim()).toContain(expectedHint) + expect(statusIndicator.textContent).toContain(expectedHint) }) }) }) diff --git a/test/unit/organisationsController.test.js b/test/unit/organisationsController.test.js index c185e3f1..525f77dc 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', status: 'Live' }, - dataset2: { endpoint: null, status: 'Needs fixing' }, - dataset3: { endpoint: 'https://example.com', status: 'Error' } + 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 } } } @@ -47,9 +47,9 @@ describe('OrganisationsController.js', () => { expect(res.render).toHaveBeenCalledWith('organisations/overview.html', expect.objectContaining({ organisation: { name: 'Test lpa', organisation: 'test-lpa' }, datasets: expect.arrayContaining([ - { endpoint: 'https://example.com', status: 'Live', slug: 'dataset1' }, - { endpoint: null, status: 'Needs fixing', slug: 'dataset2' }, - { endpoint: 'https://example.com', status: 'Error', slug: 'dataset3' } + { 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' } ]), totalDatasets: 3, datasetsWithEndpoints: 2, diff --git a/test/unit/performanceDbApi.test.js b/test/unit/performanceDbApi.test.js index 82dd6fd2..71740cd7 100644 --- a/test/unit/performanceDbApi.test.js +++ b/test/unit/performanceDbApi.test.js @@ -14,19 +14,42 @@ describe('performanceDbApi', () => { dataset: 'dataset-slug-1', endpoint: 'https://example.com/endpoint-1', exception: null, - http_status: undefined, - error: undefined, - status: 'Live' + 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 } ] } vi.spyOn(datasette, 'runQuery').mockResolvedValue(mockResponse) - await performanceDbApi.getLpaOverview(lpa) + const result = 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: 'endpoint returned with a status of 404', 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 () => {