423 add issues table component to issues details page part 2#690
423 add issues table component to issues details page part 2#690GeorgeGoodall-GovUk wants to merge 131 commits into
Conversation
…t-schemas' into 423-add-issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
…issues-table-component-to-issues-details-page-part-2
WalkthroughThe changes in this pull request involve updates to several files, including modifications to Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (52)
test/unit/landingPage.test.js (2)
Line range hint
1-1: Address the TODO comment for submit start page test coverageThe TODO comment indicates missing test coverage for the submit start page. This should be addressed to ensure comprehensive test coverage.
Would you like me to help create a new test file for the submit start page or create a GitHub issue to track this requirement?
Line range hint
8-15: Consider enhancing test coverage and documentationThe test implementation could benefit from:
- Documentation explaining the purpose of the empty
paramsobject- More explicit documentation about why the page title check is skipped
- Additional assertions for landing page specific elements
Would you like me to propose additional test cases specific to the landing page functionality?
vite.config.js (1)
19-21: Consider refining the exclude patternThe current exclude pattern
'./public/*'might be too broad. Consider being more specific about which files to exclude, perhaps targeting specific file types or subdirectories that don't require coverage.exclude: [ - './public/*' + './public/assets/**', + './public/images/**', + './public/static/**' ]test/unit/sharedTests/tableTests.js (4)
1-4: Add JSDoc documentation for the exported functionThe
runTableTestsfunction would benefit from documentation describing the expected structure of its parameters. This would help other developers understand how to use these shared tests correctly.Add documentation like this:
+/** + * Runs shared tests for table components + * @param {Object} tableParams - Configuration object containing columns and rows data + * @param {string[]} tableParams.columns - Array of column names + * @param {Object[]} tableParams.rows - Array of row data objects + * @param {Document} document - DOM document containing the rendered table + */ export const runTableTests = (tableParams, document) => {
5-14: Add error handling for missing table elementsThe test could fail with unclear errors if the table structure is incorrect. Consider adding validation for required elements.
Consider this improvement:
describe('Table Component', () => { const tableHead = document.querySelector('.govuk-table__head') + if (!tableHead) { + throw new Error('Table header not found. Ensure the table follows GOV.UK Design System structure.') + } it('Renders the correct table headings', () => { + if (!Array.isArray(tableParams.columns) || tableParams.columns.length === 0) { + throw new Error('tableParams.columns must be a non-empty array') + } const columnHeadings = tableHead.children[0].children
19-19: Fix typo in test descriptionThere's a duplicate word in the test description.
- it('Renders the correct number of rows rows', () => { + it('Renders the correct number of rows', () => {
32-46: Enhance content validation robustnessThe content validation could be more thorough and include additional test cases.
Consider these improvements:
- Add validation for rowData structure:
function checkRowContent (columns, rowData) { + if (!rowData?.columns || typeof rowData.columns !== 'object') { + throw new Error('Row data must contain a columns object') + } expect(columns.length).toEqual(Object.keys(rowData.columns).length)
- Add test cases for:
- Empty field values
- HTML content with special characters
- Multiple errors per field
- Invalid error message format
Would you like me to provide example test cases for these scenarios?
test/unit/get-startedPage.test.js (1)
14-14: Consider documenting the seed's purpose.Adding a brief comment explaining how the seed affects test data generation would help other developers understand the test setup better.
+// Seed ensures consistent test data generation across runs const seed = getSeed()src/views/components/table.html (1)
4-6: LGTM! Consider documenting the custom classes.The addition of custom 'dl-' prefixed classes alongside GOV.UK Design System classes is well-structured and follows BEM naming conventions. The scrollable container is a good addition for responsive design.
Consider adding a comment block at the top of the file documenting the purpose of these custom classes and how they complement the GOV.UK Design System classes.
{% from 'govuk/components/inset-text/macro.njk' import govukInsetText %} +{# Custom classes prefixed with 'dl-' extend GOV.UK Design System functionality: + - dl-scrollable: Enables horizontal scrolling for wide tables + - dl-table: Custom table styling + - dl-table__head: Sticky header functionality + - dl-table__header: Header cell styling +#}Also applies to: 9-9
test/unit/tableComponent.test.js (3)
3-4: Well-structured modularisation of test utilities!Good practice moving to shared test utilities and using a proper filter implementation. This improves maintainability and reduces code duplication.
Consider adding a comment explaining the purpose of each template path in the Nunjucks configuration for better maintainability.
Also applies to: 16-16
18-61: Consider improving test data readabilityThe test data structure is comprehensive but could be more maintainable. Consider extracting it to a separate test fixtures file and using more descriptive variable names.
Example refactor:
-const params = { +// Import from test/fixtures/tableTestData.js +import { generateTableTestParams } from './fixtures/tableTestData.js' + +const params = generateTableTestParams({ tableParams: { - columns: ['col1', 'col2', 'col3'], + columns: { + col1: 'First Column', + col2: 'Second Column', + col3: 'Third Column' + }, rows: [ { columns: { field1: { error: false, - value: 'value11' + value: 'Sample Value 1' }, // ... rest of the structure } } ] } -} +})
63-72: Enhance test robustness and maintainabilityThe test setup is clean, but consider these improvements:
- Move the Nunjucks template to a separate file
- Add error handling for JSDOM setup
Example implementation:
+// Import from test/templates/table.njk +import { readFileSync } from 'fs' +import { join } from 'path' + +const templatePath = join(__dirname, '../templates/table.njk') +const htmlString = readFileSync(templatePath, 'utf-8') -const htmlString = ` - {% from "components/table.html" import table %} - {{ table(tableParams) }} - ` -const html = nunjucks.renderString(htmlString, params) -const dom = new JSDOM(html) -const document = dom.window.document +let document +try { + const html = nunjucks.renderString(htmlString, params) + const dom = new JSDOM(html) + document = dom.window.document +} catch (error) { + console.error('Failed to setup JSDOM:', error) + throw error +}test/unit/middleware/datasetOverview.middleware.test.js (2)
Line range hint
4-17: Consider adding edge cases to improve test coverage.The basic mock setup is good, but consider adding test cases for:
- Invalid/missing LPA or dataset parameters
- Empty dataset information
- Error scenarios
describe('Dataset Overview Middleware', () => { const req = { params: { lpa: 'mock-lpa', dataset: 'mock-dataset' }, dataset: { name: 'mock dataset', dataset: 'mock-dataset', collection: 'mock-collection' } } const res = {} + + // Add test cases for edge cases + const reqWithMissingParams = { + params: {}, + dataset: {} + }
Line range hint
19-55: The test structure is solid, but could be more comprehensive.The current test effectively validates the happy path, but consider adding:
- Tests for error handling scenarios
- Validation of different issue statuses (Warning, Info)
- Cases with empty or malformed source data
describe('prepareDatasetOverviewTemplateParams', () => { it('should prepare template params for dataset overview', async () => { // ... existing test ... }) + + it('should handle missing source data gracefully', async () => { + const reqWithMissingData = { + ...req, + orgInfo: { name: 'mock-org' }, + specification: { fields: [] }, + columnSummary: [], + entityCount: null, + sources: [], + issues: [] + } + // Add test implementation + }) + + it('should process different issue status types correctly', async () => { + const reqWithMultipleIssueTypes = { + ...req, + issues: [ + { status: 'Error', num_issues: 1 }, + { status: 'Warning', num_issues: 2 }, + { status: 'Info', num_issues: 3 } + ] + } + // Add test implementation + })src/views/organisations/issueDetails.html (1)
60-62: LGTM: Proper implementation of GOV.UK error summary patternThe error summary implementation follows the GOV.UK Design System pattern and includes a sensible fallback heading. Consider extracting the fallback text "There is a problem" to a configuration file for consistency across the application.
src/views/organisations/issueTable.html (4)
10-16: Consider more specific variable names for better clarityThe variable
issueEntitiesCountcould be renamed tototalPagesas it appears to represent pagination information rather than the total count of issues.-{% if issueEntitiesCount > 1 %} - {% set pageName %}{{organisation.name}} - {{dataset.name}} - Issues (Page {{pageNumber}} of {{issueEntitiesCount}}){% endset %} +{% if totalPages > 1 %} + {% set pageName %}{{organisation.name}} - {{dataset.name}} - Issues (Page {{pageNumber}} of {{totalPages}}){% endset %}
57-61: Enhance error summary accessibilityConsider adding more specific ARIA attributes to improve screen reader experience.
{{ govukErrorSummary({ titleText: errorSummary.heading if errorSummary.heading else 'There is a problem', + titleId: 'error-summary-title', + descriptionText: 'Please address the following issues:', errorList: errorSummary.items }) }}
62-69: Add loading state for map componentConsider adding a loading state for the map to improve user experience while the map data is being loaded.
{% if geometries and geometries.length %} + <div class="app-map-container"> + <div class="app-map-loading" id="map-loading">Loading map...</div> <div id="map" class="app-map" role="region" + aria-busy="true" aria-label="Static map showing {{dataset.name}} for {{organisation.name}}."> </div> + </div> {% endif %}
94-103: Make guidance section more context-awareConsider customising the improvement steps based on the specific issue type being displayed.
<h3 class="govuk-heading-m"> How to improve {{ organisation.name }}'s data </h3> <ol class="govuk-list govuk-list--number"> - <li>Fix the errors indicated</li> + <li>Fix the {{ issueType | lower }} indicated above</li> <li>Use the <a href="/check">check service</a> to make sure the data meets the standard</li> <li>Publish the updated data on the data URL</li> </ol>test/unit/datasetTaskListPage.test.js (1)
5-5: Good improvement: More deterministic test data generationThe switch from timestamp-based seeds to
getSeed()makes the tests more reproducible and easier to debug, as the same seed will be used across test runs. This is particularly important for snapshot testing and debugging test failures.Consider documenting the seed generation strategy in the test documentation to help other developers understand how to reproduce specific test scenarios.
Also applies to: 10-10
src/middleware/datasetTaskList.middleware.js (1)
46-52: Consider enhancing error handling and type safetyWhilst the fallback for
issuesWithCountsis good practice, consider adding:
- Type validation for issue objects within the map function
- Error handling for malformed issue objects to prevent runtime errors
Here's a suggested improvement:
const issuesWithCountsOrDefault = issuesWithCounts || [] - const taskList = issuesWithCountsOrDefault.map((issue) => { + const taskList = issuesWithCountsOrDefault.map((issue) => { + if (!issue?.field || !issue?.issue_type || !issue?.status) { + console.warn('Malformed issue object:', issue) + return null + } return { title: { text: performanceDbApi.getTaskMessage({ ...issue, entityCount, field: issue.field }) }, href: `/organisations/${lpa}/${datasetId}/${issue.issue_type}/${issue.field}`, status: getStatusTag(issue.status) } + }).filter(Boolean)test/unit/middleware/datasetTaskList.middleware.test.js (1)
Line range hint
22-36: Consider adding test cases for edge scenarios.The current test covers the happy path well, but consider adding test cases for:
- Empty issues array
- Issues with zero count
- Issues with missing status
Example test data structure:
const edgeCaseTests = [ { name: 'empty issues array', issuesWithCounts: [], expected: { taskList: [] } }, { name: 'issue with zero count', issuesWithCounts: [{ issue: 'Example issue', issue_type: 'Example type', field: 'Example field', num_issues: 0, status: 'Error' }] } ];src/assets/scss/index.scss (2)
174-178: Consider enhancing mobile and keyboard accessibilityThe scrollable container should include touch-friendly scrolling indicators and focus styles for keyboard navigation.
Add these improvements:
.dl-scrollable { max-width: 100%; overflow-x: auto; padding: 0px; + -webkit-overflow-scrolling: touch; + &:focus { + outline: $govuk-focus-width solid $govuk-focus-colour; + } }
206-210: Consider text overflow handling for long headersThe
text-wrap: nowrapproperty might cause layout issues with long header text. Consider adding text truncation with ellipsis.Add text overflow handling:
.dl-table__header { border: 1px solid $govuk-border-colour; text-wrap: nowrap; background-color: govuk-colour("light-grey"); + overflow: hidden; + text-overflow: ellipsis; + max-width: 300px; // Adjust based on your needs }test/unit/views/organisations/dataset-overview.test.js (2)
Line range hint
11-46: Consider extracting test fixtures to a separate fileThe test parameters could be moved to a separate fixtures file to improve maintainability and reusability. This would make the test file more focused and allow the test data to be shared across related test files.
Line range hint
34-35: Fix invalid date format in mock dataThe mock endpoint data contains invalid dates (2024-19-19). Consider using valid dates for consistency with real-world scenarios.
- lastAccessed: '2024-19-19', - lastUpdated: '2024-19-19', + lastAccessed: '2024-09-19', + lastUpdated: '2024-09-19',src/middleware/datasetOverview.middleware.js (1)
167-168: Consider grouping related middleware functions.The middleware chain follows a logical flow, but it's getting quite long. Consider grouping related functions into composable middleware chains for better maintainability.
You could create separate chains for:
- Resource fetching (active resources, latest resource)
- Error handling (resource accessibility checks)
- Data enrichment (specification, entity count)
Example refactor:
const resourceChain = [ fetchActiveResourcesForOrganisationAndDataset, fetchIf(isResourceIdNotInParams, fetchLatestResource, takeResourceIdFromParams) ] const errorHandlingChain = [ fetchIf(isResourceAccessible, fetchLpaDatasetIssues), onlyIf(isResourceNotAccessible, prepareDatasetTaskListErrorTemplateParams), onlyIf(isResourceNotAccessible, getDatasetTaskListError) ] export default [ ...resourceChain, ...errorHandlingChain, // ... remaining middleware ]test/unit/middleware/issueTable.middleware.test.js (3)
3-3: Remove commented-out import statement.Remove the commented import if it's no longer needed. If it might be needed in the future, add a TODO comment explaining why it's kept.
-// import { pagination } from '../../../src/utils/pagination.js'
9-15: Consider enhancing pagination mock with more test cases.The current mock returns a fixed array
[1, '...', 4, 5]. Consider adding more test cases to cover different pagination scenarios (e.g., first page, last page, single page).vi.mock('../../../src/utils/pagination.js', () => { return { - pagination: vi.fn().mockReturnValue([ - 1, '...', 4, 5 - ]) + pagination: vi.fn().mockImplementation((total, current, perPage) => { + if (total <= perPage) return [1] + if (current === 1) return [1, 2, 3, '...', 10] + if (current === total) return [1, '...', 8, 9, 10] + return [1, '...', current - 1, current, current + 1, '...', 10] + }) } })
66-147: Improve test readability and add field validation.The test setup is quite complex. Consider:
- Extracting test data setup into helper functions
- Adding tests for required field validation
- Breaking down the large test into smaller, focused test cases
// Extract test data setup const createTestRequest = (overrides = {}) => ({ params: mocker(IssueTableQueryParams), orgInfo: mocker(OrgField), dataset: mocker(DatasetNameField), errorSummary: mocker(errorSummaryField), entities: [{ entry_number: 10, 'start-date': { value: 'start-date', issue: { message: 'invalid', value: 'invalid-start-date' } }, reference: { value: 'reference' }, entityPageNumber: 1 }], ...overrides }) // Add focused test cases it('should handle missing required fields', () => { const req = createTestRequest({ dataset: undefined }) expect(() => prepareIssueTableTemplateParams(req, res, next)).toThrow() }) it('should format dates correctly', () => { const req = createTestRequest({ entities: [{ 'start-date': { value: '2023-01-01' } }] }) prepareIssueTableTemplateParams(req, res, next) expect(req.templateParams.tableParams.rows[0].columns['start-date'].value) .toBe('1 January 2023') })test/unit/views/organisations/issueTablePage.test.js (4)
10-10: Consider adding error handling for Nunjucks setupThe Nunjucks setup could benefit from error handling to gracefully handle template loading failures.
-const nunjucks = setupNunjucks({}) +const nunjucks = setupNunjucks({}) || throw new Error('Failed to setup Nunjucks')
27-27: Parameterise the mock issue textThe mock issue text is hardcoded. Consider deriving it from the seed data for more comprehensive testing.
- { text: 'mock issue' } + { text: params.dataset.issueType || 'mock issue' }
67-67: Document table test expectationsAdd a comment block explaining what aspects of the table are being tested through the shared utility.
+// Runs shared table tests to verify: +// - Table headers and their order +// - Row data rendering +// - Sorting functionality +// - Responsive behaviour runTableTests(params.tableParams, document)
70-100: Refactor pagination test dataConsider moving the pagination test data to a separate fixture file for better maintainability and reuse.
Create a new file
test/fixtures/pagination.js:export const paginationItems = [ { type: 'number', current: false, number: 1, href: 'organisations/mock-org/mock-dataset/1' }, // ... rest of the items ]src/routes/schemas.js (4)
41-42: LGTM! Consider adding JSDoc comments.The extraction of these common field schemas promotes reusability. Consider adding JSDoc comments to document the purpose and structure of these exported schemas.
+/** + * Schema for organisation fields including name, organisation code, and optional geographic identifiers + */ export const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()) }) + +/** + * Schema for dataset identification including name, dataset type, and collection + */ export const DatasetNameField = v.strictObject({ name: NonEmptyString, dataset: NonEmptyString, collection: NonEmptyString })
44-50: LGTM! Consider enhancing type safety for URLs.The error summary schema is well-structured and aligns with GOV.UK design patterns. Consider using a more specific URL pattern validation for the
hreffield to ensure it matches your application's URL structure.export const errorSummaryField = v.strictObject({ heading: v.optional(v.string()), items: v.array(v.strictObject({ html: v.string(), - href: v.url() + href: v.pipe( + v.url(), + v.regex(/^\/[a-zA-Z0-9-/]*$/, 'URL must be a valid application path') + ) })) })
52-67: Consider adding type annotations for better maintainability.The table parameters schema is well-structured but could benefit from explicit TypeScript-style type annotations in comments to improve maintainability and documentation.
+/** + * @typedef {Object} TableColumn + * @property {Object} [error] - Optional error details + * @property {string} [error.message] - Error message + * @property {string} [value] - Column value + * @property {string} [html] - HTML content + */ const tableParams = v.strictObject({ columns: v.array(NonEmptyString), rows: v.array(v.strictObject({ columns: v.objectWithRest( {}, v.object({ error: v.optional(v.object({ message: v.string() })), value: v.optional(v.string()), html: v.optional(v.string()) }) ) })), fields: v.array(NonEmptyString) })
69-89: LGTM! Consider extracting pagination types to constants.The pagination schema is well-structured and handles all cases appropriately. Consider extracting the pagination item types to named constants for better maintainability.
+const PAGINATION_ITEM_TYPES = { + NUMBER: 'number', + ELLIPSIS: 'ellipsis' +} as const const paginationParams = v.optional(v.strictObject({ previous: v.optional(v.strictObject({ href: v.string() })), next: v.optional(v.strictObject({ href: v.string() })), items: v.array(v.variant('type', [ v.strictObject({ - type: v.literal('number'), + type: v.literal(PAGINATION_ITEM_TYPES.NUMBER), number: v.integer(), href: v.string(), current: v.boolean() }), v.strictObject({ - type: v.literal('ellipsis'), + type: v.literal(PAGINATION_ITEM_TYPES.ELLIPSIS), ellipsis: v.literal(true), href: v.string() }) ])) }))test/unit/issueDetailsPage.test.js (1)
Line range hint
40-64: Consider adding edge cases to error summary tests.The current test coverage is good, but consider adding these scenarios:
- Empty items array
- Items with special characters or HTML entities
- Very long error messages
test/unit/middleware/issueDetails.middleware.test.js (3)
10-36: Consider adding test for undefined error messageThe test suite has good coverage for null cases, but could be more comprehensive by adding a test for undefined error message.
Add this test case:
+ it('should return an empty string if errorMessage is undefined', () => { + const issue = { value: '02-02-2022' } + const result = issueErrorMessageHtml(undefined, issue) + expect(result).toBe('') + })
38-127: Consider refactoring tests to reduce duplicationWhile the test coverage is excellent, there's significant duplication in the test cases for null/undefined scenarios.
Consider using test.each to make the tests more maintainable:
+ const testCases = [ + { scenario: 'text is null', text: null, html: '<p>Mock html</p>', classes: 'mock-classes', expected: { text: '', html: '<p>Mock html</p>', classes: 'mock-classes' } }, + { scenario: 'text is undefined', text: undefined, html: '<p>Mock html</p>', classes: 'mock-classes', expected: { text: '', html: '<p>Mock html</p>', classes: 'mock-classes' } }, + // ... add other cases + ] + + it.each(testCases)('should return an object with default values when $scenario', ({ text, html, classes, expected }) => { + const result = getIssueField(text, html, classes) + expect(result).toEqual({ + key: { text: expected.text }, + value: { html: expected.html }, + classes: expected.classes + }) + })
282-286: Consider using beforeEach for mock data setupThe mock data setup could be moved to a beforeEach block to avoid duplication and improve maintainability.
Refactor the mock setup:
+ let mockedOrg, mockedDataset, mockErrorSummary + + beforeEach(() => { + mockedOrg = mocker(OrgField) + mockedDataset = mocker(DatasetNameField) + mockErrorSummary = mocker(errorSummaryField) + })test/utils/mocker.js (1)
48-51: Avoid mutating input objectsModifying the
dataobject directly can cause unintended side effects elsewhere in the code. Consider creating a new object to maintain immutability.Suggested change:
- data.tableParams = mockTableParams(data.tableParams, schema.properties.tableParams) - - return data + const newTableParams = mockTableParams(data.tableParams, schema.properties.tableParams) + const newData = { ...data, tableParams: newTableParams } + + return newDatasrc/middleware/issueTable.middleware.js (1)
77-83: Simplify conditional checks using optional chainingYou can simplify the conditional statements by using optional chaining, which enhances readability and conciseness.
Apply this diff to update the code:
const geometries = entities.map(entity => { - if (entity && entity.geometry && entity.geometry.value) { + if (entity?.geometry?.value) { return entity.geometry.value - } else if (entity && entity.point && entity.point.value) { + } else if (entity?.point?.value) { return entity.point.value } else { return null } }).filter(geometry => geometry !== null)🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/middleware/issueDetails.middleware.js (1)
121-133: Consistently usefieldValueto improve code clarityIn line 132, consider using
fieldValue.valueinstead ofentity[datasetField]?.value || ''to maintain consistency and improve readability, sincefieldValuehas already been defined and accounts for undefined cases.Apply this diff to implement the suggestion:
- valueHtml += escapeHtml(entity[datasetField]?.value || '') + valueHtml += escapeHtml(fieldValue.value)src/middleware/common.middleware.js (4)
117-154: Improve readability by refactoring complex functionThe
formatErrorSummaryParamsfunction is quite long and complex. Consider breaking it into smaller helper functions to improve readability and maintainability.
238-275: Handle errors during JSON parsing more robustlyIn
extractJsonFieldFromEntities, when JSON parsing fails, a warning is logged, but the function proceeds without modifying the entity. Consider whether additional error handling is needed, such as marking the entity as invalid or excluding it from further processing.
313-334: Avoid mutating original entitiesIn
addIssuesToEntities, the original entity is deep-copied usingJSON.parse(JSON.stringify(origionalEntity)). This method is inefficient and may cause performance issues. Consider using a more efficient cloning method or ensuring that mutation of the original entity is safe.Suggested fix:
Consider using
Object.assign({}, originalEntity)or the spread operator{ ...originalEntity }for a shallow copy if deep cloning is not necessary.
389-412: Clarify the use offlatMapand improve code clarityIn
addDatabaseFieldToSpecification, the use offlatMapand the conditional return of arrays or objects can make the code harder to understand. Ensure consistent return types and consider refactoring for better readability.test/unit/middleware/common.middleware.test.js (3)
22-31: Consider using numerical HTTP status codesIn the tests for
isResourceAccessible, thestatusproperty is a string (e.g.,'200','404'). Since HTTP status codes are conventionally numbers, using numerical values might prevent potential type-related issues.
45-53: Clarify test descriptions forisResourceIdNotInParamsThe test descriptions might be misleading. The function
isResourceIdNotInParamsreturns**true**whenresourceIdis not inparams. The description "returns true if resourceId is in params" should be updated to "returns false if resourceId is in params" for accuracy.
275-288: Handle JSON parsing errors explicitlyIn
extractJsonFieldFromEntities, if an entity'sjsonfield contains invalid JSON, the function silently fails by skipping that entity. Consider handling JSON parsing errors explicitly by logging an error message or adding error handling to aid in debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest/unit/middleware/__snapshots__/common.middleware.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (40)
package.json(2 hunks)src/assets/scss/index.scss(1 hunks)src/controllers/OrganisationsController.js(2 hunks)src/middleware/common.middleware.js(4 hunks)src/middleware/datasetOverview.middleware.js(2 hunks)src/middleware/datasetTaskList.middleware.js(3 hunks)src/middleware/issueDetails.middleware.js(3 hunks)src/middleware/issueTable.middleware.js(1 hunks)src/routes/organisations.js(1 hunks)src/routes/schemas.js(3 hunks)src/services/performanceDbApi.js(6 hunks)src/views/components/table.html(2 hunks)src/views/organisations/issueDetails.html(3 hunks)src/views/organisations/issueTable.html(1 hunks)test/unit/check-answers.test.js(1 hunks)test/unit/check/confirmationPage.test.js(1 hunks)test/unit/choose-datasetPage.test.js(1 hunks)test/unit/dataset-details.test.js(2 hunks)test/unit/datasetTaskListPage.test.js(1 hunks)test/unit/endpointSubmissionForm/confirmationPage.test.js(1 hunks)test/unit/errorsPage.test.js(1 hunks)test/unit/findPage.test.js(1 hunks)test/unit/get-startedPage.test.js(1 hunks)test/unit/http-errorPage.test.js(1 hunks)test/unit/issueDetailsPage.test.js(6 hunks)test/unit/landingPage.test.js(1 hunks)test/unit/lpa-detailsPage.test.js(1 hunks)test/unit/lpaOverviewPage.test.js(1 hunks)test/unit/middleware/common.middleware.test.js(1 hunks)test/unit/middleware/datasetOverview.middleware.test.js(1 hunks)test/unit/middleware/datasetTaskList.middleware.test.js(1 hunks)test/unit/middleware/issueDetails.middleware.test.js(2 hunks)test/unit/middleware/issueTable.middleware.test.js(1 hunks)test/unit/noErrorsPage.test.js(1 hunks)test/unit/sharedTests/tableTests.js(1 hunks)test/unit/tableComponent.test.js(2 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)test/unit/views/organisations/issueTablePage.test.js(1 hunks)test/utils/mocker.js(3 hunks)vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/unit/errorsPage.test.js
- test/unit/noErrorsPage.test.js
🧰 Additional context used
📓 Learnings (2)
test/unit/middleware/datasetOverview.middleware.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/middleware/datasetOverview.middleware.test.js:1-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.
test/unit/views/organisations/dataset-overview.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🪛 Biome (1.9.4)
src/middleware/issueTable.middleware.js
[error] 77-77: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (72)
test/unit/landingPage.test.js (1)
5-5: Verify consistent usage of new shared tests path
The import path has been updated to use the new shared tests location. Let's verify this change is consistent across all test files.
✅ Verification successful
Import path change is consistently applied
The new path ./sharedTests/generic-page.js is correctly used across all test files, with appropriate relative paths (e.g., ../../sharedTests/generic-page.js for nested test files). The shared test file exists in the correct location and serves its intended purpose as a common test utility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify new path exists
# Verify the new shared tests file exists
fd generic-page.js test/unit/sharedTests
# Check for any remaining old import paths
rg "from './generic-page'" test/unit/
Length of output: 126
Script:
#!/bin/bash
# Let's check for all imports of generic-page.js across test files
rg "from.*generic-page" test/unit/
# Also check the actual location and content of the file
ls -la test/unit/sharedTests/generic-page.js
head -n 5 test/unit/sharedTests/generic-page.js
Length of output: 1943
vite.config.js (1)
17-17: Good addition for robust test coverage reporting!
Setting reportOnFailure to true is a valuable addition that ensures coverage data is available even when tests fail, making it easier to diagnose issues.
src/routes/organisations.js (2)
8-8: Verify the impact of route pattern changes
The updated route pattern with the new 'entry' segment and optional pagination might affect existing client-side route handling.
Let's verify the route usage and potential conflicts:
✅ Verification successful
Let me gather more information about the client-side route handling and middleware implementation.
Let me try one more search to find any client-side references with properly escaped patterns.
Route pattern changes are properly integrated
The updated route pattern with the 'entry' segment is well-integrated across the codebase:
- The middleware implementation in
issueDetails.middleware.jscorrectly handles the new route structure - Client-side references are properly updated, as seen in the test files using the new URL pattern
- The pagination logic in
setPagePaginationOptionsfunction correctly builds URLs with the new 'entry' segment - The route changes are supported by corresponding database fetch operations in
performanceDbApi.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to the old route pattern and verify client-side routing
# Search for route references in JavaScript/TypeScript files
rg -t js -t ts "/:lpa/:dataset/:issue_type/:issue_field" --glob '!src/routes/organisations.js'
# Search for potential route handling in client-side components
rg -t js -t ts "getIssueDetailsMiddleware" -A 5
Length of output: 1793
Script:
#!/bin/bash
# Check for client-side route handling and middleware implementation
# Search for the middleware implementation
rg -t js -t ts "issueDetails\.middleware" -A 10
# Look for any client-side route configurations or API calls
rg -t js -t ts "fetch\(.*\/entry\/" -A 2
rg -t js -t ts "\.href.*\/entry\/" -A 2
Length of output: 3168
Script:
#!/bin/bash
# Search for client-side route handling with fixed patterns
# Look for any client-side fetch calls or href references
rg -t js -t ts "fetch.*entry" -A 2
rg -t js -t ts "href.*entry" -A 2
# Check the middleware implementation
fd "issueDetails.middleware.js" --exec cat {}
Length of output: 6902
9-9: Ensure proper error handling for the new table middleware
The new route for issue tables appears logically structured, but we should verify the middleware implementation.
Let's check the middleware implementation:
✅ Verification successful
Let me gather more information about the middleware implementation.
Error handling is properly implemented in the middleware chain
The middleware implementation shows robust error handling through multiple layers:
- Input validation via
validateIssueTableQueryParamsusing Valibot schema - Error handling in the middleware chain through the
logPageErrormiddleware at the end - Comprehensive error handling in the underlying fetch operations through the middleware builders
- Proper HTTP error responses for invalid requests
The middleware chain is well-structured with proper validation, data fetching, transformation, and error handling at each step. The implementation follows best practices for Express.js middleware error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of getIssueTableMiddleware
# Search for the middleware implementation
ast-grep --pattern 'getIssueTableMiddleware = async (req, res, next) {
$$$
}'
# Check for error handling patterns
rg "getIssueTableMiddleware" -A 10 -B 2
Length of output: 2451
Script:
#!/bin/bash
# Search for the implementation of issueTable.middleware.js
rg -l "issueTable.middleware.js"
cat $(fd "issueTable.middleware.js")
# Look for try-catch blocks and error handling patterns in the middleware
ast-grep --pattern 'try {
$$$
} catch (error) {
$$$
}'
Length of output: 22838
src/controllers/OrganisationsController.js (2)
14-14: LGTM! Consistent with existing pattern
The addition of getIssueTableMiddleware to the controller object maintains consistency with the existing implementation pattern and preserves alphabetical ordering.
4-4: Verify the imported middleware implementation
The import statement follows the established pattern. Let's verify the implementation of the imported middleware.
✅ Verification successful
Import of getIssueTableMiddleware is properly implemented
The imported middleware module is well-implemented with a comprehensive set of functions for handling issue table functionality:
- Validates query parameters with proper schema validation
- Manages pagination with configurable page length
- Handles entity processing and geometry extraction
- Prepares template parameters for rendering
- Includes error handling and logging
The middleware follows established patterns and integrates with the existing codebase through common middleware functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the imported middleware
# Check if the middleware file exists and examine its implementation
fd "issueTable.middleware.js" --type f --exec cat {}
Length of output: 5479
test/unit/check/confirmationPage.test.js (1)
5-5: LGTM! Good refactoring of test structure.
The move to a shared tests directory improves the organisation and maintainability of the test suite.
Let's verify the consistency of this change across other test files:
✅ Verification successful
Import path change is correct and consistent across the codebase
The verification confirms that:
- The shared test file exists at the new location with proper implementation
- All test files, including the one under review, correctly import from '../sharedTests/generic-page.js'
- No remaining imports from the old path '../generic-page.js' were found
- The change aligns with the project-wide structure where 15 other test files use the same shared test utility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration of generic page tests to shared directory
# and ensure no old imports remain
# Check if the new shared tests file exists
fd generic-page.js test/unit/sharedTests
# Check for any remaining old imports
rg "from '\.\./generic-page'" test/unit/
# List all files importing from the new shared location
rg "from '\.\./sharedTests/generic-page'" test/unit/
Length of output: 190
Script:
#!/bin/bash
# Let's verify the imports more thoroughly with broader patterns and context
# Check content of the shared test file
cat test/unit/sharedTests/generic-page.js
# Look for any imports of generic-page with more flexible patterns and context
rg -A 2 "generic-page" test/unit/ --type js
# Check all test files in the check directory to understand the context
ls -la test/unit/check/
Length of output: 5751
test/unit/endpointSubmissionForm/confirmationPage.test.js (1)
5-5: LGTM! Good refactoring of test utilities.
The move of runGenericPageTests to a shared directory improves code organisation and reusability.
Let's verify this refactoring is consistent across the codebase:
✅ Verification successful
Import path change is consistent across the codebase
All test files are correctly importing runGenericPageTests from the shared location in sharedTests/generic-page.js. The refactoring has been applied consistently, with proper relative paths adjusted based on the importing file's location:
- Files in
test/unit/use./sharedTests/generic-page.js - Files in subdirectories use the correct relative path (e.g.,
../sharedTests/generic-page.jsor../../sharedTests/generic-page.js)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old imports of generic-page.js
# and confirm all tests now use the new shared location
# Check for any remaining old imports
echo "Checking for any remaining old imports..."
rg "from '\.\./generic-page'" test/
# Verify consistent usage of new shared location
echo "Verifying usage of new shared location..."
rg "from '\.\./sharedTests/generic-page'" test/
Length of output: 285
Script:
#!/bin/bash
# Let's try a broader search to understand the test structure and imports
echo "Searching for generic-page.js files..."
fd "generic-page.js" test/
echo -e "\nSearching for any imports containing 'generic-page'..."
rg -l "generic-page" test/
echo -e "\nChecking content of found files..."
fd "generic-page.js" test/ -x cat {}
Length of output: 2281
Script:
#!/bin/bash
# Let's examine the imports in the files that reference generic-page
echo "Checking imports in files that use generic-page..."
rg "import.*generic-page" test/ -A 1
Length of output: 3092
test/unit/choose-datasetPage.test.js (1)
3-3: LGTM! Good refactoring to centralise shared test logic.
The move of runGenericPageTests to a shared directory improves code organisation and maintainability.
Let's verify this refactoring is consistent across other test files:
✅ Verification successful
Refactoring verified - all test files consistently use the new shared location
The verification shows that all test files are using the correct import path ./sharedTests/generic-page.js or the appropriate relative path (../sharedTests/generic-page.js for nested directories). No instances of the old import path were found, confirming the refactoring is complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all test files have been updated to use the new shared test location
# Search for any remaining imports from the old location
echo "Checking for any remaining old import paths..."
rg "from './generic-page'" "test/unit"
# Verify consistent usage of the new shared location
echo "Verifying usage of new shared location..."
rg "from './sharedTests/generic-page'" "test/unit"
Length of output: 297
Script:
#!/bin/bash
# Let's try a broader search to understand the test file structure and imports
# Find all test files first
echo "Test files in unit directory:"
fd -e test.js . test/unit
# Look for any imports containing 'generic-page' with more context
echo -e "\nSearching for generic-page imports with context:"
rg -B1 -A1 "generic-page" test/unit
# Check if the sharedTests directory exists and list its contents
echo -e "\nContents of sharedTests directory (if exists):"
fd . test/unit/sharedTests
Length of output: 7109
test/unit/get-startedPage.test.js (1)
4-4: LGTM! Good standardisation of test utilities.
The changes improve test consistency by:
- Centralising common test functions in a shared directory
- Standardising seed generation across test files
- Making tests more deterministic and reproducible
Also applies to: 6-6, 12-12
src/views/components/table.html (1)
19-19: Verify HTML content sanitisation.
The new cellText handling introduces potential security risks with the safe filter. While striptags is used for error messages, we should ensure HTML content is properly sanitised before reaching this template.
Let's check how the HTML content is sanitised:
Also applies to: 23-23, 25-26
test/unit/tableComponent.test.js (1)
3-3: Verify shared test implementation
Let's ensure the shared test implementation exists and follows best practices.
Also applies to: 72-72
✅ Verification successful
Shared test implementation verified and properly structured
The shared test implementation exists at test/unit/sharedTests/tableTests.js and follows good testing practices:
- Exports a well-structured
runTableTestsfunction that accepts table parameters and document - Contains comprehensive test cases for table headings, row count, and row content
- Is properly imported and used across multiple test files:
test/unit/tableComponent.test.jstest/unit/views/organisations/issueTablePage.test.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify shared test implementation
# Check if the shared test file exists and examine its structure
echo "Checking shared test implementation..."
rg -A 10 "export function runTableTests" ./test/unit/sharedTests/tableTests.js
Length of output: 167
Script:
#!/bin/bash
# Let's first check if the file exists and its content
fd "tableTests.js" ./test/unit/sharedTests/ --type f --exec cat {}
# As a fallback, let's search for any file containing "runTableTests"
echo "Searching for runTableTests in any file..."
rg "runTableTests" --type js
Length of output: 2133
test/unit/lpa-detailsPage.test.js (1)
3-3: LGTM! Verify consistent usage across test files.
The move to use a shared test utility is a good refactoring that improves maintainability.
Let's verify that this change is consistent across all test files:
✅ Verification successful
Let me verify the content of the shared test file and its usage to ensure completeness.
Let me verify the import statements in all the test files to ensure they're using the correct path.
Import path change is properly implemented across the codebase
The shared test utility generic-page.js exists in the correct location and all test files are consistently using the proper relative paths to import it from the sharedTests directory. The implementation maintains proper path resolution regardless of the test file's location in the directory structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify new import paths
# are used consistently across all test files
echo "Checking for any remaining old imports..."
rg "from './generic-page'" test/
echo "Verifying consistent usage of new import path..."
rg "from './sharedTests/generic-page'" test/
echo "Checking if the shared test file exists..."
fd "generic-page.js" test/unit/sharedTests/
Length of output: 470
Script:
#!/bin/bash
# Check the content and usage of the shared test file
echo "Content of the shared test file:"
cat test/unit/sharedTests/generic-page.js
echo -e "\nChecking all imports of generic-page.js across test files:"
rg -l "generic-page" test/
Length of output: 2126
Script:
#!/bin/bash
# Check the actual import statements in all files that reference generic-page
rg "import.*generic-page" test/unit/ -A 1
Length of output: 2986
test/unit/middleware/datasetOverview.middleware.test.js (2)
1-2: LGTM! Imports are clean and focused.
The imports are appropriately scoped to only what's needed for these tests.
Line range hint 1-55: Verify the moved tests are in their new location.
The removal of pullOutDatasetSpecification tests aligns with the previous learning that these tests were moved to common.middleware.test.js.
✅ Verification successful
Tests have been correctly relocated as per previous learning
The verification confirms that the pullOutDatasetSpecification tests were properly moved to common.middleware.test.js and are implemented there with appropriate test cases. This aligns with the learning note and maintains proper test organisation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the moved tests exist in the new location
# Check if the tests were properly moved
rg -l "pullOutDatasetSpecification" "test/unit/middleware/common.middleware.test.js"
Length of output: 129
Script:
#!/bin/bash
# Description: Check the actual test implementation in common.middleware.test.js
rg "pullOutDatasetSpecification" "test/unit/middleware/common.middleware.test.js" -A 5
Length of output: 1033
test/unit/http-errorPage.test.js (2)
4-5: Well-structured import reorganisation!
The movement of shared test utilities to a dedicated directory improves code organisation and reusability. This change aligns well with the broader test restructuring effort across the codebase.
12-12: Good improvement to test determinism!
Using getSeed() instead of a timestamp makes the tests more reproducible. This is crucial for debugging test failures.
Let's verify the implementation of the getSeed function:
✅ Verification successful
getSeed() implementation is appropriate for test determinism
The implementation in test/utils/mocker.js shows that getSeed() either uses a value from TEST_SEED environment variable or falls back to a timestamp. This is a good pattern as it:
- Allows deterministic test runs via environment variable
- Provides a fallback timestamp for regular test runs
- Is consistently used across all test files for mock data generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of getSeed to ensure it provides consistent values
# Expected: Find the implementation and verify it returns deterministic values
# Search for getSeed implementation
ast-grep --pattern 'export function getSeed() {
$$$
}'
# Backup: Use ripgrep if ast-grep doesn't find TypeScript/JavaScript implementation
rg -A 5 'export.+function.+getSeed'
Length of output: 94
Script:
#!/bin/bash
# Let's try different search patterns to locate the getSeed function
# First, search for any file containing 'getSeed'
rg -l "getSeed"
# Then search for the function definition with more context
rg -A 10 "function.*getSeed|const.*getSeed.*="
# Also search in test files specifically
fd "test.js$" --exec rg -A 5 "getSeed"
Length of output: 3519
test/unit/findPage.test.js (2)
11-11: LGTM! Enhanced test determinism.
Using getSeed() instead of timestamps makes the tests more deterministic and reproducible, which is crucial for reliable testing.
4-4: LGTM! Good improvements to test organisation.
The changes to import paths and mock utilities reflect good practices:
- Moving shared tests to a dedicated directory improves maintainability
- Using named imports makes the code's dependencies more explicit
Let's verify if other test files have been updated consistently:
Also applies to: 6-6
✅ Verification successful
Import path changes are consistently applied across the codebase
The verification shows that all test files are correctly using the shared test utilities from the sharedTests directory. The import paths are consistent across all files that use the generic page tests, with proper relative paths based on their location in the directory structure.
Examples of consistent usage:
- Files in
test/unit/:'./sharedTests/generic-page.js' - Files in subdirectories:
'../sharedTests/generic-page.js'or'../../sharedTests/generic-page.js'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent test imports across test files
# Expected: All test files should use the new shared test path
# Search for any remaining old-style imports
rg -l "from './generic-page'" test/unit/
Length of output: 48
Script:
#!/bin/bash
# Let's try a broader search to understand the test organization pattern
# Search for any imports of generic-page tests across the codebase
rg "import.*generic-page" -A 2 test/
Length of output: 4218
Script:
#!/bin/bash
# Let's also check the structure of test files to understand the pattern
fd "test.js$" test/unit/
Length of output: 2323
Script:
#!/bin/bash
# Check the content of the shared tests directory
fd . "test/unit/sharedTests/"
Length of output: 152
test/unit/check-answers.test.js (2)
Line range hint 8-77: Well-structured and comprehensive test coverage
The test suite demonstrates good practices:
- Comprehensive coverage of all form fields
- Clear test descriptions
- Robust regex patterns for flexible matching
- Good separation of test cases
5-5: Verify the new shared test utility path
The reorganisation of test utilities into a shared directory is a good practice for better maintainability.
Let's verify the new location exists:
✅ Verification successful
Import path change verified successfully
The shared test utility file exists at the new location test/unit/sharedTests/generic-page.js, confirming the import path change is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the shared test utility exists and is accessible
# Expected: File exists at the new location
fd "generic-page.js" test/unit/sharedTests
Length of output: 80
src/views/organisations/issueDetails.html (2)
7-7: LGTM: Proper import of GOV.UK Design System component
The govukBackLink import follows the established pattern and is correctly placed with other GOV.UK component imports.
40-44: Verify XSS protection in URL construction
Whilst the breadcrumb structure is logical, please ensure that the variables used in URL construction (organisation.organisation, dataset.dataset, issueType, issueField) are properly sanitised to prevent XSS attacks.
✅ Verification successful
URL parameters are properly protected against XSS
The codebase shows that URL parameters in the issue details view are properly encoded using encodeURIComponent() in the middleware layer (src/middleware/issueDetails.middleware.js). The same pattern is consistently applied across related components, including the issue table middleware. Additionally, HTML content is properly escaped using the escape-html package where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for URL sanitisation or encoding functions in the codebase
rg -t js -t py "encodeURIComponent|escape|sanitise|sanitize|xss"
Length of output: 1677
Script:
#!/bin/bash
# Let's verify if the issueDetails.html file is using any router or middleware
rg -A 5 "issueDetails" src/routes/
Length of output: 381
Script:
#!/bin/bash
# Let's check the implementation of OrgIssueDetails and related middleware
ast-grep --pattern 'const OrgIssueDetails = $_'
# Also check the route handling for this view
rg -B 2 -A 5 "issueDetails.middleware" src/
Length of output: 2064
src/views/organisations/issueTable.html (1)
1-9: LGTM! Proper template setup with GOV.UK Design System components
The template correctly extends the main layout and imports all necessary GOV.UK Design System macros and custom components.
test/unit/datasetTaskListPage.test.js (1)
3-3: Verify the new shared test utility path exists
The import path has been updated to use a centralised location for shared test utilities.
✅ Verification successful
Import path verified successfully
The shared test utility file exists at the expected location test/unit/sharedTests/generic-page.js, confirming that the import path change is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new shared test utility path exists
# Expected: File should exist at the new location
fd "generic-page.js" "test/unit/sharedTests"
Length of output: 80
src/middleware/datasetTaskList.middleware.js (2)
1-1: LGTM: Import changes are well-structured
The new imports align with their usage in the middleware chain and reflect the enhanced resource handling mentioned in the PR objectives.
121-125: Verify middleware execution order dependencies
The middleware chain looks well-structured, but let's verify that fetchActiveResourcesForOrganisationAndDataset and fetchIssuesWithCounts don't have any timing dependencies with fetchEntityCount.
✅ Verification successful
Let me generate another verification script to check the implementation details of these middleware functions and their dependencies.
Based on the implementation details revealed by the scripts, I can now provide the final response:
The middleware execution order is correctly structured
The implementation shows that:
fetchActiveResourcesForOrganisationAndDatasetmust run first as it provides theresourcesdata required byfetchIssuesWithCountsfetchEntityCountoperates independently using theorgInfo.entityfrom the request object- All middleware functions are built using the
fetchManyandfetchOnebuilders which handle async operations properly
The current order in the middleware chain ensures proper data flow and maintains execution dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any timing dependencies between middleware functions
# Search for await/Promise usage in the middleware functions
echo "Checking for async dependencies in middleware functions..."
ast-grep --pattern 'const $name = async ($_, $_) => { $$$ }' src/middleware/common.middleware.js
# Check if these functions are used elsewhere in the codebase
echo "Checking for usage patterns..."
rg -A 5 "fetchActiveResourcesForOrganisationAndDataset|fetchIssuesWithCounts|fetchEntityCount" src/
Length of output: 9104
Script:
#!/bin/bash
# Check the implementation of fetchMany and fetchOne builders
echo "Checking middleware builders implementation..."
cat src/middleware/middleware.builders.js
# Check the implementation of these specific middleware functions
echo "\nChecking common middleware implementations..."
ast-grep --pattern 'export const $name = $_ => { $$$ }' src/middleware/common.middleware.js
# Check for any await dependencies in the performanceDbApi
echo "\nChecking API dependencies..."
rg "getEntityCounts|entityCountQuery|issuesWithCountsQuery|activeResourcesForOrganisationAndDataset" src/services/performanceDbApi.js -A 5
Length of output: 11199
test/unit/dataset-details.test.js (3)
5-5: LGTM! Good improvements to test reproducibility
The changes improve the test organisation and determinism:
- Moving shared tests to a dedicated directory enhances maintainability
- Using
getSeed()instead of timestamps ensures consistent test runs
Also applies to: 11-11, 43-43
Line range hint 16-42: Well-structured test helper function
The errorTestFn helper is well-designed:
- Reusable across different field validations
- Clear parameter structure
- Good separation of concerns
Line range hint 45-121: Comprehensive test coverage
The test suite demonstrates good practices:
- Clear test descriptions
- Thorough validation scenarios for each field
- Consistent error message format
- Good use of regex for flexible header matching
Let's verify the validation messages match the form requirements:
✅ Verification successful
Test coverage matches implementation perfectly
The test suite's validation error messages exactly match the actual validation rules and error messages defined in the codebase:
- The endpoint URL validation in
fields.jsenforces required field and 2048 character limit - The documentation URL has the same validation rules
- Error messages in
validationMessageLookup.jscorrespond precisely to the test cases:- "Enter an endpoint URL" for required
- "Enter a valid endpoint URL" for format
- "The URL must be less than 2048 characters" for maxlength
The test coverage is comprehensive and accurately reflects the implemented validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if validation messages are consistent with form requirements
# Search for validation messages in templates
rg -A 2 'data-error-message=' 'src/views/**/*.html'
# Search for validation schema definitions
ast-grep --pattern 'const $_ = v.object({
$$$
})'
Length of output: 3601
Script:
#!/bin/bash
# Let's check the actual form validation implementation and template
# Find the dataset details template
fd -e html "dataset-details"
# Find the validation schema for dataset details
ast-grep --pattern 'const $_ = v.object({
$$$
"endpoint-url": $$$,
$$$
"documentation-url": $$$,
$$$
})'
# Search for validation setup in the controller
rg -A 5 "endpoint.*url.*validation|documentation.*url.*validation"
Length of output: 1132
Script:
#!/bin/bash
# Let's check the validation message lookup implementation and the template content
# Find the validation message lookup filter
rg -A 10 "validationMessageLookup"
# Get the full content of the dataset details template
cat src/views/submit/dataset-details.html
Length of output: 16018
Script:
#!/bin/bash
# Let's check the validation messages definitions
# Find the validation messages configuration
rg -A 10 "validationMessages = {"
# Find any validation schema for URL fields
ast-grep --pattern 'const $_ = {
$$$
"endpoint-url": $$$,
$$$
}'
Length of output: 216
Script:
#!/bin/bash
# Let's try a different approach to find validation messages
# Search for validation message definitions
rg -A 10 "const.*validation.*messages"
# Search for any URL validation related code
rg -A 5 "url.*validation|maxlength.*2048"
# Look for the validation setup file
fd -e js validation
Length of output: 3881
package.json (2)
46-46: LGTM: Minor version update for test coverage tool
The patch version update for @vitest/coverage-v8 is a safe change that only affects the development environment.
76-76: Verify escape-html package version and implementation
Whilst adding HTML escaping is a good security practice, especially for rendering user input in tables, the specified version (1.0.3) is quite dated. Additionally, we should verify its implementation.
Let's check for:
- Latest version and any security advisories
- Implementation in the codebase
Consider alternatives:
- Use built-in template engine escaping if available
- Consider more actively maintained packages like
heorhtml-escaper
✅ Verification successful
Let me gather more information about the actual usage of escape-html in the codebase.
Keep escape-html package as is - implementation is secure and appropriate
The escape-html package is being used correctly for its intended purpose in the issueDetails middleware to safely escape user input before HTML rendering. While the package is from 2015, it:
- Has no known security vulnerabilities
- Performs a focused, single responsibility of HTML escaping
- Is used appropriately in template string contexts
- Is implemented in security-critical areas where user input is displayed
The suggestion to switch to alternative packages would introduce unnecessary churn without clear benefits, as the current implementation is secure and follows best practices for XSS prevention.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify escape-html package version and usage
# Check NPM for latest versions and security advisories
echo "Checking latest version and security info..."
npm view escape-html time --json | jq '."1.0.3"'
npm audit --json escape-html
# Find usage in the codebase to ensure proper implementation
echo "Checking usage in codebase..."
rg -A 3 "escape-html" --type js
Length of output: 3778
Script:
#!/bin/bash
# Check how escape-html is actually used in the code
rg -B 3 -A 10 "escapeHtml\(" --type js
# Check if there are any template engines or HTML rendering libraries in use
cat package.json | jq '.dependencies + .devDependencies' | grep -i "template\|html\|render\|sanitize"
# Look for other HTML escaping/sanitization usage
rg -i "sanitize|escape.*html|xss" --type js
Length of output: 2438
test/unit/middleware/datasetTaskList.middleware.test.js (1)
Line range hint 22-36: LGTM! The test data structure accurately reflects the middleware changes.
The renamed property issuesWithCounts and its data structure properly test the middleware's handling of issue counts and status mappings.
src/assets/scss/index.scss (2)
194-203: Excellent implementation of sticky header
The sticky header implementation follows GDS design patterns and provides good visual hierarchy with appropriate use of z-index and box shadow.
181-191: Review table overflow behaviour
The fixed max-height and hidden horizontal overflow might cause issues:
- 80vh height might be too large on mobile devices
- Hidden horizontal overflow might clip content
Consider testing with various viewport sizes and content lengths. Run this script to find potential content that might be affected:
test/unit/lpaOverviewPage.test.js (1)
3-3: LGTM! Good refactoring of test utilities.
The reorganisation of test utilities into a shared directory and the addition of the getSeed import improve the test structure and maintainability.
Also applies to: 6-6
test/unit/views/organisations/dataset-overview.test.js (1)
3-3: Verify the shared tests directory structure
The import path change aligns with the broader restructuring effort to centralise shared test logic. However, let's ensure the new structure is consistently implemented.
✅ Verification successful
Import path change is correctly implemented and consistently used
The new shared tests directory structure exists at test/unit/sharedTests/generic-page.js, and the updated import path is consistently used across multiple test files in the organisation views, confirming proper implementation of the restructuring effort.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the shared tests directory structure and usage
# Check if the new shared tests file exists
fd "generic-page.js" test/unit/sharedTests
# Verify consistent usage across other test files
rg -l "from '\.\./\.\./sharedTests/generic-page\.js'" test/unit/
Length of output: 261
src/middleware/datasetOverview.middleware.js (1)
154-160: LGTM! Verify error handling in the API service.
The implementation follows the established patterns and properly integrates with the middleware builder system.
Let's verify the error handling in the performance DB API service:
✅ Verification successful
Error handling is properly implemented - no action needed
The implementation is safe as it uses the fetchMany builder which has robust error handling:
- Errors from
datasette.runQueryare caught and logged - Errors are properly propagated to the error middleware via
next(error) - The datasette service includes proper error handling and logging
- The middleware builder pattern ensures consistent error handling across all fetch operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling in datasetIssuesQuery
ast-grep --pattern 'datasetIssuesQuery($_, $_) {
$$$
try {
$$$
} catch {
$$$
}
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Let's first locate the performanceDbApi implementation
rg -l "datasetIssuesQuery" --type js
# Then examine the implementation
rg "datasetIssuesQuery" -A 10 --type js
Length of output: 3071
Script:
#!/bin/bash
# Let's check how datasette.runQuery handles errors
rg "runQuery" -B 5 -A 10 --type js
# Also check if there's any error handling middleware
rg "error.*middleware" --type js
Length of output: 32093
src/routes/schemas.js (3)
176-188: LGTM! Schema updates are well-structured.
The modifications to the OrgIssueDetails schema properly integrate the new error summary structure and pagination parameters whilst maintaining backwards compatibility.
193-201: LGTM! Schema composition is well-structured.
The new OrgIssueTable schema effectively combines the various schema components whilst maintaining a clear and consistent structure.
254-254: LGTM! Template mapping is correctly configured.
The new template-to-schema mapping is properly added to the registry.
test/unit/issueDetailsPage.test.js (2)
4-4: LGTM: Import and setup changes look good!
The changes improve test determinism by using getSeed() and maintain consistency with other test files through the updated import path.
Also applies to: 7-7, 11-11
127-127: LGTM: Pagination title section logic is well-implemented.
The conditional logic for pagination title section handles both single and multi-page scenarios correctly.
test/unit/middleware/issueDetails.middleware.test.js (1)
1-5: LGTM: Well-structured import statements and test setup
The import statements are properly organised, importing all necessary dependencies and test utilities.
src/middleware/issueTable.middleware.js (7)
34-41: Query parameters validation is correctly defined
The IssueTableQueryParams schema is appropriately defined using valibot and includes all necessary parameters, ensuring robust validation.
43-45: Query parameters validation middleware is properly set up
The validateIssueTableQueryParams middleware correctly applies the schema for query parameter validation, promoting consistent request handling.
47-52: Default query parameters are properly set
The setDefaultQueryParams middleware accurately assigns a default pageNumber of 1 when it's not provided, ensuring pagination functions correctly.
54-60: Entity page numbers are correctly added
The addEntityPageNumberToEntity middleware effectively adds an entityPageNumber to each entity, which is essential for pagination and entity identification.
62-71: Pagination options are correctly set
The setPagePageOptions middleware accurately sets pagination options based on the request parameters and entities count, facilitating correct pagination behaviour.
109-117: Ensure entity[field] exists before accessing its properties
When accessing entity[field].value and entity[field].issue, ensure that entity[field] exists to prevent potential runtime errors.
Consider adding a check to confirm that entity[field] is defined before accessing its properties.
143-173: Middleware sequence is well-structured
The exported default middleware array is correctly structured, ensuring that data fetching and processing occur in the proper order before rendering the issue table.
src/middleware/issueDetails.middleware.js (6)
1-23: Imports are correctly structured
The required middleware functions are properly imported from './common.middleware.js'.
26-27: Utility modules are appropriately imported
The escapeHtml and logger modules are correctly imported and utilised within the code.
29-34: Query parameter validation is effectively implemented
The IssueDetailsQueryParams schema enforces strict validation rules, ensuring that pageNumber is a positive integer and other parameters are correctly validated.
48-53: Error message HTML generation is secure and correct
The issueErrorMessageHtml function properly handles error messages and securely escapes issue values to prevent XSS vulnerabilities.
62-70: Function handles default values effectively
The getIssueField function correctly provides default values and ensures safe handling of null or undefined inputs.
74-82: Pagination options are correctly set
The setPagePaginationOptions middleware correctly configures pagination parameters on the request object.
src/middleware/common.middleware.js (4)
6-6: Importing pagination module is appropriate
The import statement correctly includes the pagination utility, which is utilised later in the code.
66-66: Verify initialisation of req.orgInfo.entity
Ensure that req.orgInfo.entity is properly initialised before it's used in fetchEntityCount to prevent potential runtime errors due to undefined properties.
156-169: Possible off-by-one error in pagination
In paginateEntitiesAndPullOutCount, the calculation might not correctly handle pagination if pageNumber does not start at 1. Verify that paginationIndex and slicing logic correctly paginate the entities.
44-44: Verify all references to updated function name
The function isResourceIdInParams has been renamed to isResourceIdNotInParams. Please ensure that all references to the old function name have been updated throughout the codebase to reflect this change and prevent any potential errors.
Run the following script to check for any remaining references to isResourceIdInParams:
✅ Verification successful
Function rename has been properly implemented across the codebase
The verification shows that only the new function name isResourceIdNotInParams is being used consistently throughout the codebase, with no instances of the old name isResourceIdInParams found. The function is:
- Exported in
src/middleware/common.middleware.js - Imported and used correctly in:
src/middleware/datasetOverview.middleware.jssrc/middleware/issueTable.middleware.jstest/unit/middleware/common.middleware.test.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any usages of the old function name `isResourceIdInParams`
# Expected result: No matches found, indicating all references have been updated
rg 'isResourceIdInParams' --type js
Length of output: 35
Script:
#!/bin/bash
# Let's verify both the old and new function names with more context
rg -A 2 -B 2 'isResourceId(Not)?InParams' --type js
# Also check for any exports of these functions
ast-grep --pattern 'export const isResourceId$_InParams'
# Let's also check for any imports of these functions
ast-grep --pattern 'import { $$$isResourceId$_InParams$$$ } from'
Length of output: 4220
src/services/performanceDbApi.js (9)
275-275: Potential SQL Injection Vulnerability: Use Parameterised Queries
The query in activeResourcesForOrganisationAndDatasetQuery concatenates lpa and dataset directly into the SQL string, which can result in SQL injection vulnerabilities. Consider using parameterised queries to safely include these variables.
362-362: Potential SQL Injection Vulnerability: Use Parameterised Queries
376-380: Potential SQL Injection Vulnerability: Use Parameterised Queries
In the issuesQuery, variables dataset, issueType, and issueField are concatenated into the SQL query without proper sanitisation. This can expose the application to SQL injection attacks. Use parameterised queries to securely include these variables.
385-413: Potential SQL Injection Vulnerability: Use Parameterised Queries
The same issue applies to issuesWithCountsQuery where user-supplied input is concatenated into the SQL query. Ensure that all variables are safely parameterised to prevent SQL injection.
415-427: Potential SQL Injection Vulnerability: Use Parameterised Queries
In issuesWithReferenceFromResourcesDatasetIssueTypeFieldQuery, variables are directly inserted into the SQL string. This poses a security risk and should be addressed by using parameterised queries.
429-440: Potential SQL Injection Vulnerability: Use Parameterised Queries
Similarly, in fetchIssuesWithoutReferences, the SQL query is constructed with concatenated variables. This should be refactored to use parameterised queries to ensure security.
501-513: Potential SQL Injection Vulnerability: Use Parameterised Queries
In fetchEntitiesFromEntryNumbers, the organisationEntity and entryNumbers are inserted into the SQL query without sanitisation, which could lead to SQL injection. Use parameterised queries to include these variables safely.
515-523: Potential SQL Injection Vulnerability: Use Parameterised Queries
In fetchEntityFromEntryNumber, the variables are directly concatenated into the SQL query. This approach is insecure and should be replaced with parameterised queries.
525-530: Potential SQL Injection Vulnerability: Use Parameterised Queries
In fetchEntitiesFromReferencesAndOrganisationEntity, the references and organisationEntity variables are directly used in the SQL query. This can cause SQL injection vulnerabilities. Utilise parameterised queries to mitigate this risk.
test/unit/middleware/common.middleware.test.js (2)
33-42: Consider using numerical HTTP status codes
Similarly, in the tests for isResourceNotAccessible, using numerical status codes instead of strings would align with standard practices and improve type consistency.
644-670: Verify field mapping logic for database fields
In addDatabaseFieldToSpecification, when a field does not have a corresponding replacement_field in fieldMappings, the datasetField defaults to the original field name. Verify that this behaviour is intentional and that it won't cause issues if unmapped fields are expected to have a different handling.
| const tableBody = document.querySelector('.govuk-table__body') | ||
| const rows = tableBody.children | ||
|
|
||
| it('Renders the correct number of rows rows', () => { | ||
| expect(rows.length).toEqual(tableParams.rows.length) | ||
| }) | ||
|
|
||
| it('Renders the correct row content', () => { | ||
| tableParams.rows.forEach((rowData, i) => { | ||
| const columns = rows[i].children | ||
| checkRowContent(columns, rowData) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for table body and rows data
The test assumes the table body exists and rows data is valid without proper validation.
Add these checks:
const tableBody = document.querySelector('.govuk-table__body')
+ if (!tableBody) {
+ throw new Error('Table body not found. Ensure the table follows GOV.UK Design System structure.')
+ }
+ if (!Array.isArray(tableParams.rows)) {
+ throw new Error('tableParams.rows must be an array')
+ }
const rows = tableBody.children📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tableBody = document.querySelector('.govuk-table__body') | |
| const rows = tableBody.children | |
| it('Renders the correct number of rows rows', () => { | |
| expect(rows.length).toEqual(tableParams.rows.length) | |
| }) | |
| it('Renders the correct row content', () => { | |
| tableParams.rows.forEach((rowData, i) => { | |
| const columns = rows[i].children | |
| checkRowContent(columns, rowData) | |
| }) | |
| }) | |
| const tableBody = document.querySelector('.govuk-table__body') | |
| if (!tableBody) { | |
| throw new Error('Table body not found. Ensure the table follows GOV.UK Design System structure.') | |
| } | |
| if (!Array.isArray(tableParams.rows)) { | |
| throw new Error('tableParams.rows must be an array') | |
| } | |
| const rows = tableBody.children | |
| it('Renders the correct number of rows rows', () => { | |
| expect(rows.length).toEqual(tableParams.rows.length) | |
| }) | |
| it('Renders the correct row content', () => { | |
| tableParams.rows.forEach((rowData, i) => { | |
| const columns = rows[i].children | |
| checkRowContent(columns, rowData) | |
| }) | |
| }) |
| {{ govukBackLink({ | ||
| text: "Back", | ||
| href: '/organisations/' + organisation.organisation + '/' + dataset.dataset + '/' + issueType + '/' + issueField |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider centralising URL construction
The URL construction is duplicated between the breadcrumbs and back link. Consider extracting this logic to a shared helper function to maintain consistency and reduce duplication.
Example approach:
// helpers/urls.js
export const getIssueTypeUrl = (org, dataset, issueType, issueField) =>
`/organisations/${encodeURIComponent(org)}/${encodeURIComponent(dataset)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}`| <link href='https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.css' rel='stylesheet' /> | ||
| <script> | ||
| window.serverContext = { | ||
| containerId: 'map', | ||
| geometries: {{ geometries | dump | safe }}, | ||
| mapType: "static" | ||
| } | ||
| </script> | ||
| <script src="/public/js/map.bundle.js"></script> |
There was a problem hiding this comment.
Add integrity hash for CDN resources and type safety
- Add integrity hash for the MapLibre GL JS CDN resource to prevent supply chain attacks.
- Consider adding type safety for the window context object.
- <link href='https://unpkg.com/maplibre-gl@latest/dist/maplibre-gl.css' rel='stylesheet' />
+ <link href='https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.css' rel='stylesheet' integrity="sha384-..." crossorigin="anonymous" />
<script>
+ interface MapContext {
+ containerId: string;
+ geometries: Array<GeoJSON.Feature>;
+ mapType: 'static';
+ }
+
window.serverContext = {
containerId: 'map',
geometries: {{ geometries | dump | safe }},
mapType: "static"
- }
+ } as MapContext;
</script>Also, consider hosting the MapLibre GL JS files locally instead of relying on a CDN.
Committable suggestion skipped: line range outside the PR's diff.
| {{ govukBreadcrumbs({ | ||
| items: [ | ||
| { | ||
| text: "Home", | ||
| href: "/" | ||
| }, | ||
| { | ||
| text: "Organisations", | ||
| href: '/organisations' | ||
| }, | ||
| { | ||
| text: organisation.name, | ||
| href: '/organisations/' + organisation.organisation | ||
| }, | ||
| { | ||
| text: dataset.name | capitalize, | ||
| href: '/organisations/' + organisation.organisation + '/' + dataset.dataset | ||
| }, | ||
| { | ||
| text: issueType | capitalize | ||
| } | ||
| ] | ||
| }) }} |
There was a problem hiding this comment.
💡 Codebase verification
Dataset name capitalisation is inconsistent across templates
The codebase shows inconsistent handling of dataset names:
- Breadcrumbs and error pages use the
capitalizefilter - Dataset task list, get started, and dataset overview pages display the raw dataset name without capitalisation
This inconsistency could lead to an inconsistent user experience. Consider:
- Either removing the
capitalizefilter from breadcrumbs to maintain the original dataset name format - Or applying the
capitalizefilter consistently across all dataset name displays
🔗 Analysis chain
Verify dataset name capitalisation behaviour
The capitalize filter is applied to the dataset name. Ensure this matches the intended display format, as some dataset names might be acronyms or have specific capitalisation requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of dataset name display to verify consistency
rg -g '*.html' -g '*.njk' 'dataset.name.*capitalize'
Length of output: 292
Script:
#!/bin/bash
# Let's check for any other dataset name displays without capitalize filter
rg -g '*.html' -g '*.njk' 'dataset\.name(?!.*capitalize)'
Length of output: 305
Script:
#!/bin/bash
# Let's try a simpler search for dataset.name without the negative lookahead
rg -g '*.html' -g '*.njk' 'dataset\.name'
Length of output: 2797
| .dl-table__cell--error { | ||
| border-left: govuk-spacing(1) solid $govuk-error-colour; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error cell accessibility
Consider adding additional visual indicators and ARIA attributes for better accessibility.
Add these improvements:
.dl-table__cell--error {
border-left: govuk-spacing(1) solid $govuk-error-colour;
+ background-color: govuk-tint($govuk-error-colour, 95);
+ font-weight: bold;
+ position: relative;
+
+ &::before {
+ content: "";
+ position: absolute;
+ left: -#{govuk-spacing(1)};
+ top: 50%;
+ transform: translateY(-50%);
+ width: govuk-spacing(1);
+ height: 100%;
+ background-color: $govuk-error-colour;
+ }
}Remember to add appropriate ARIA attributes in the HTML template:
<td class="dl-table__cell--error" aria-invalid="true" aria-describedby="error-message-id">| export const createPaginationTemplateParams = (req, res, next) => { | ||
| const { resultsCount, urlSubPath, paginationPageLength } = req | ||
| let { pageNumber } = req.params | ||
| pageNumber = parseInt(pageNumber) | ||
|
|
||
| const totalPages = Math.floor(resultsCount / paginationPageLength) | ||
|
|
||
| const paginationObj = {} | ||
| if (pageNumber > 1) { | ||
| paginationObj.previous = { | ||
| href: `${urlSubPath}${pageNumber - 1}` | ||
| } | ||
| } | ||
|
|
||
| if (pageNumber < totalPages) { | ||
| paginationObj.next = { | ||
| href: `${urlSubPath}${pageNumber + 1}` | ||
| } | ||
| } | ||
|
|
||
| paginationObj.items = pagination(totalPages, pageNumber).map(item => { | ||
| if (item === '...') { | ||
| return { | ||
| type: 'ellipsis', | ||
| ellipsis: true, | ||
| href: '#' | ||
| } | ||
| } else { | ||
| return { | ||
| type: 'number', | ||
| number: item, | ||
| href: `${urlSubPath}${item}`, | ||
| current: pageNumber === parseInt(item) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| req.pagination = paginationObj | ||
|
|
||
| next() | ||
| } |
There was a problem hiding this comment.
Ensure correct total pages calculation in pagination
In createPaginationTemplateParams, the total pages are calculated using Math.floor(resultsCount / paginationPageLength). This may not account for cases where resultsCount is not an exact multiple of paginationPageLength. Consider using Math.ceil to ensure all items are included.
Suggested fix:
-const totalPages = Math.floor(resultsCount / paginationPageLength)
+const totalPages = Math.ceil(resultsCount / paginationPageLength)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createPaginationTemplateParams = (req, res, next) => { | |
| const { resultsCount, urlSubPath, paginationPageLength } = req | |
| let { pageNumber } = req.params | |
| pageNumber = parseInt(pageNumber) | |
| const totalPages = Math.floor(resultsCount / paginationPageLength) | |
| const paginationObj = {} | |
| if (pageNumber > 1) { | |
| paginationObj.previous = { | |
| href: `${urlSubPath}${pageNumber - 1}` | |
| } | |
| } | |
| if (pageNumber < totalPages) { | |
| paginationObj.next = { | |
| href: `${urlSubPath}${pageNumber + 1}` | |
| } | |
| } | |
| paginationObj.items = pagination(totalPages, pageNumber).map(item => { | |
| if (item === '...') { | |
| return { | |
| type: 'ellipsis', | |
| ellipsis: true, | |
| href: '#' | |
| } | |
| } else { | |
| return { | |
| type: 'number', | |
| number: item, | |
| href: `${urlSubPath}${item}`, | |
| current: pageNumber === parseInt(item) | |
| } | |
| } | |
| }) | |
| req.pagination = paginationObj | |
| next() | |
| } | |
| export const createPaginationTemplateParams = (req, res, next) => { | |
| const { resultsCount, urlSubPath, paginationPageLength } = req | |
| let { pageNumber } = req.params | |
| pageNumber = parseInt(pageNumber) | |
| const totalPages = Math.ceil(resultsCount / paginationPageLength) | |
| const paginationObj = {} | |
| if (pageNumber > 1) { | |
| paginationObj.previous = { | |
| href: `${urlSubPath}${pageNumber - 1}` | |
| } | |
| } | |
| if (pageNumber < totalPages) { | |
| paginationObj.next = { | |
| href: `${urlSubPath}${pageNumber + 1}` | |
| } | |
| } | |
| paginationObj.items = pagination(totalPages, pageNumber).map(item => { | |
| if (item === '...') { | |
| return { | |
| type: 'ellipsis', | |
| ellipsis: true, | |
| href: '#' | |
| } | |
| } else { | |
| return { | |
| type: 'number', | |
| number: item, | |
| href: `${urlSubPath}${item}`, | |
| current: pageNumber === parseInt(item) | |
| } | |
| } | |
| }) | |
| req.pagination = paginationObj | |
| next() | |
| } |
| export const fetchActiveResourcesForOrganisationAndDataset = fetchMany({ | ||
| query: ({ params }) => performanceDbApi.activeResourcesForOrganisationAndDatasetQuery(params.lpa, params.dataset), | ||
| result: 'resources' | ||
| }) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
SQL injection vulnerability confirmed in database queries
The code review has revealed that SQL queries are constructed using direct string interpolation without any parameter sanitisation. This is evident in multiple places:
- In
performanceDbApi.js, theactiveResourcesForOrganisationAndDatasetQueryuses template literals with direct variable interpolation:WHERE REPLACE(ro.organisation, '-eng', '') = '${lpa}' - The
datasette.jsservice only performs URL encoding of the complete query but doesn't handle SQL parameter sanitisation - No SQL sanitisation or prepared statement functionality is implemented anywhere in the codebase
This poses a significant security risk as malicious input in params.lpa or params.dataset could be used to inject arbitrary SQL commands.
Recommendations:
- Implement proper SQL parameter sanitisation
- Use a query builder library or an ORM
- If using raw queries, escape special characters and validate input parameters before interpolation
🔗 Analysis chain
Ensure query parameters are safely handled to prevent SQL injection
The params.lpa and params.dataset are used directly in the query. Please verify that performanceDbApi.activeResourcesForOrganisationAndDatasetQuery properly sanitises these inputs or uses parameterised queries to prevent SQL injection vulnerabilities.
Run the following script to check if activeResourcesForOrganisationAndDatasetQuery uses parameterised queries:
The initial search reveals that activeResourcesForOrganisationAndDatasetQuery uses string interpolation directly in the SQL query, which is concerning. However, let's verify if there's any sanitisation happening at a higher level by checking the implementation of the fetchMany function and any potential database connection setup.
Let's check the database query execution implementation to see how queries are actually run.
Let's check the datasette service implementation to understand how queries are executed and if there's any sanitisation at that level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of `activeResourcesForOrganisationAndDatasetQuery` for secure parameter handling.
# Expected result: The function should use parameterised queries or proper input sanitisation.
rg 'activeResourcesForOrganisationAndDatasetQuery' -A 10
Length of output: 1818
Script:
#!/bin/bash
# Check implementation of fetchMany and database connection setup
rg "fetchMany" -A 10
# Look for database connection setup
rg -i "createConnection|knex|sequelize|database.connect" -A 5
Length of output: 15520
Script:
#!/bin/bash
# Check the datasette implementation and database connection handling
rg "datasette.runQuery" -B 5 -A 5
# Also check for any SQL sanitization utilities
rg -i "sanitize|escape|prepare" --type js
Length of output: 28477
Script:
#!/bin/bash
# Check the datasette service implementation
rg -A 10 "class.*Datasette|const.*datasette" src/services/
cat src/services/datasette.js
Length of output: 8094
| issue_type it ON i.issue_type = it.issue_type | ||
| WHERE | ||
| i.resource = '${resource}' | ||
| i.resource in ('${resources.join("', '")}') |
There was a problem hiding this comment.
Potential SQL Injection Vulnerability: Use Parameterised Queries
The SQL query in datasetIssuesQuery is constructed by directly injecting the resources array values into the query string. If any element in resources contains malicious input, this can lead to SQL injection attacks. It's recommended to use parameterised queries to safely include user-supplied data.
Apply this diff to refactor the query to use parameter placeholders:
- i.resource in ('${resources.join("', '")}')
+ i.resource in (${resources.map(() => '?').join(', ')})When executing the query, pass the resources array as parameters:
const result = await datasette.runQuery(sql, resources);| it('handles entities with undefined specification', () => { | ||
| const req = { | ||
| entities: [ | ||
| { id: 1, name: 'John' } | ||
| ], | ||
| specification: null | ||
| } | ||
| const res = {} | ||
| const next = vi.fn() | ||
|
|
||
| nestEntityFields(req, res, next) | ||
|
|
||
| expect(next).toHaveBeenCalledWith(new Error('Specification is not defined')) | ||
| }) |
There was a problem hiding this comment.
Ensure consistent error handling when specification is undefined
In the test handles entities with undefined specification, the middleware expects an error when specification is null. Ensure that nestEntityFields properly throws or handles this error consistently, and consider providing a more descriptive error message for easier debugging.
| it('handles special case for GeoX and GeoY fields', () => { | ||
| const req = { | ||
| specification: { | ||
| fields: [ | ||
| { field: 'GeoX' }, | ||
| { field: 'GeoY' } | ||
| ] | ||
| }, | ||
| fieldMappings: [] | ||
| } | ||
| const res = {} | ||
| const next = vi.fn() | ||
|
|
||
| addDatabaseFieldToSpecification(req, res, next) | ||
| expect(req.specification.fields).toHaveLength(2) | ||
| expect(req.specification.fields[0]).toEqual({ | ||
| field: 'GeoX', | ||
| datasetField: 'point' | ||
| }) | ||
| expect(req.specification.fields[1]).toEqual({ | ||
| field: 'GeoY', | ||
| datasetField: 'point' | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential redundancy with 'GeoX' and 'GeoY' mapping
Both 'GeoX' and 'GeoY' fields are mapped to the same datasetField value 'point'. This could cause confusion or data overwriting in downstream processing. Consider combining these fields appropriately or differentiating their datasetField mappings.
3 participants
What type of PR is this? (check all applicable)
Description
Please replace this line with a brief description of the changes made.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
Before screenshot here
After
After screenshot here
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Release Notes
New Features
issueTable.htmltemplate for displaying issue-related data.Bug Fixes
Documentation
Tests