diff --git a/src/controllers/OrganisationsController.js b/src/controllers/OrganisationsController.js index 8c254f27..6bed4b65 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.endpoint) accumulator[0]++ + if (dataset.status === 'Needs fixing') accumulator[1]++ + if (dataset.status === 'Error') accumulator[2]++ return accumulator }, [0, 0, 0]) @@ -100,7 +95,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 +125,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) } }, diff --git a/src/filters/filters.js b/src/filters/filters.js index 2ce7bdb5..0dcdb5cb 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 }) => { @@ -20,6 +37,7 @@ 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 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 830d3606..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 */ /** @@ -80,27 +80,32 @@ 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 'Needs fixing' + when (it.severity = 'warning') then 'Warning' + else 'Live' + end as status, 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 ((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 FROM provision p LEFT JOIN @@ -130,24 +135,17 @@ 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, - error, - issue + status: row.status } 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 +204,9 @@ 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 - } + /* eslint camelcase: "off" */ + 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 e83a6064..dcd7a6da 100644 --- a/src/views/organisations/overview.html +++ b/src/views/organisations/overview.html @@ -17,7 +17,7 @@ }, { text: "Organisations", - href: "/overview/organisations" + href: "/organisations" } ] }) }} @@ -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
@@ -66,69 +66,34 @@

Datasets

{% endfor %} @@ -137,10 +102,4 @@

-
-
-

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 0f1518ce..a504695a 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: 'Needs 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: 'Needs 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 } ] } @@ -91,17 +103,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 @@ -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 ? 'Endpoint not provided' : dataset.error ? dataset.error : dataset.issue ? dataset.issue : 'Endpoint provided' - 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) }) }) @@ -136,22 +155,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) - }) }) }) - 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' + 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' + } 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/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, diff --git a/test/unit/performanceDbApi.test.js b/test/unit/performanceDbApi.test.js index 71740cd7..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: '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 () => {