785 add task list to results page #842
Conversation
…task-list-to-results-page
…-list-to-results-page
…task-list-to-results-page
…task-list-to-results-page
…task-list-to-results-page
…task-list-to-results-page
…task-list-to-results-page
…task-list-to-results-page
WalkthroughThis PR introduces a series of middleware functions in the ResultsController to improve the processing of issues and tasks. The error handling in templates has been refactored by replacing the previous errorsTemplate with a unified resultsTemplate, and related functions have been renamed accordingly. The views are updated to present passed checks, blocking, and non-blocking tasks. Import paths in several test files have been amended to reflect a new directory structure, and additional tests have been introduced along with updates to page object models, configuration files, routing, and API stub JSON. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant RC as ResultsController
participant MW as Middleware Chain
participant T as Template Engine
C->>RC: Send results check request
RC->>MW: Execute checkForErroredResponse
MW->>MW: Process getIssueTypesWithQualityCriteriaLevels
MW->>MW: Execute extractIssuesFromResults & addQualityCriteriaLevelsToIssues
MW->>MW: Aggregate issues and filter by quality criteria level
MW->>MW: Calculate total rows, blocking & non-blocking tasks, passed checks
MW->>RC: Return processed data
RC->>T: Setup resultsTemplate and error (if any) via setupError
T->>C: Render updated results page
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/controllers/resultsController.js (4)
159-163: Potential parameterization of the SQL query.
Currently, the query is hard-coded and does not incorporate user inputs, reducing the risk of SQL injection. However, consider parameterizing future expansions to this query for best practices.
189-190: Typographical fix for the function name.
“aggrogate” is likely misspelt. Rename it to “aggregateIssues” for clarity and consistency.-// aggrogate issues by issue_type into tasks -export function aggrogateIssues (req, res, next) { +// aggregate issues by issue_type into tasks +export function aggregateIssues (req, res, next) {
226-247: Consider adding meaningful links for blocking tasks.
Currently,hrefis blank. Provide a link to the relevant page or resource that helps resolve each blocking task.
249-268: Add helpful links for non-blocking issues.
Similar to blocking tasks, adding a meaningfulhrefto guide users toward fixing these issues can improve user experience.src/views/check/results/errors.html (1)
73-93: Provide synergy with the “passed checks” section.
Having a "Data that passed checks" section alongside errors is informative. Double-check that the “50 rows” text is dynamic if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/controllers/resultsController.js(5 hunks)src/views/check/results/errors.html(2 hunks)src/views/check/results/results.html(1 hunks)
🔇 Additional comments (14)
src/controllers/resultsController.js (6)
4-4: Consider removing unused imports.
It appears that you are importingfetchManyhere. Please verify that all imported functions are indeed used in this file to avoid adding unnecessary dependencies.
8-8: Template naming looks good.
Defining a constant forresultsTemplateimproves clarity around which template is used for successful checks.
19-24: Validate middleware order for data integrity.
By adding the new issue-related middleware in this sequence, you ensure that issue data is processed after core request details and error summaries. This order seems coherent.
66-66: Explicit assignment of theresultsTemplate.
Your fallback to useresultsTemplatewhen the request is not failed is clear. Confirm that you do not inadvertently return to the old template for partial or mixed states.
78-78: Conditional logic for fetching response details is consistent.
Using the sameresultsTemplatecheck ensures that failure templates are bypassed.
164-173: EnsureresponseDetails.responseis always defined.
IfresponseDetails.responseorissue_logsis undefined or unexpectedly structured, this could throw an error. Consider extra guards or fallback values around.map()or.flat().src/views/check/results/results.html (3)
1-143: Guard usage of| safewhen dumping geospatial data.
At lines 137–138, you render potentially large or user-provided geometry data as raw JSON. If the data is untrusted, it could pose an XSS risk. Consider sanitising or ensuring all geometry data is trusted before rendering with| safe.
1-143: Check for undefined pagination fields.
You referenceoptions.paginationproperties in lines 17–20 and line 55. Ensure thatpaginationis defined when the results page is rendered, or provide safe defaults to prevent runtime errors.
1-143: Overall layout is consistent and user-friendly.
The modular usage of GOV.UK task lists for passed checks, blocking issues, and non-blocking issues enhances clarity. Consider adding accessible text for screen readers where needed (e.g., describing table data).src/views/check/results/errors.html (5)
10-10: Introduction of GOV.UK task list macro.
Importing thegovukTaskListcomponent is a good step toward consistent UI presentation.
45-71: Retain geometry, table, and pagination logic.
Maintaining these checks ensures that a user with errors can still review the data structure. No concerns here.
96-131: Hard-coded blocking issues.
You have example tasks referencingversion_path,organisation, anddataset. Ensure these variables are defined or replaced with dynamic references as used in the controller.
134-195: Hard-coded non-blocking issues.
Similar to blocking tasks, confirm that the placeholders (version_path,organisation,dataset) are replaced with real variables or a dynamic approach.
198-206: Good call to action for uploading a new version.
Inviting the user to re-check their data fosters a clear submission workflow. Keep it concise and user-friendly.
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/controllers/resultsController.js (1)
175-187:⚠️ Potential issueHandle missing issueTypes gracefully.
When
issueTypeis not found inissueTypes,issueType.quality_criteria_levelwill cause an error.export function addQualityCriteriaLevelsToIssues (req, res, next) { + try { const { issues, issueTypes } = req req.issues = issues.map(issue => { const issueType = issueTypes.find(issueType => issueType.issue_type === issue['issue-type']) + if (!issueType) { + return { ...issue, quality_criteria_level: null } + } return { ...issue, quality_criteria_level: issueType.quality_criteria_level } }) next() + } catch (error) { + next(error) + } }
🧹 Nitpick comments (2)
src/controllers/resultsController.js (2)
66-66: Consider extracting template logic into a configuration object.The template handling logic could be more maintainable by using a configuration object to map templates to their respective response detail fetching strategies.
+const TEMPLATE_CONFIG = { + [resultsTemplate]: { + fetchDetails: async (requestData, pageNumber) => + requestData.fetchResponseDetails(pageNumber, 50, 'error'), + }, + [failedFileRequestTemplate]: { fetchDetails: null }, + [failedUrlRequestTemplate]: { fetchDetails: null }, + default: { + fetchDetails: async (requestData, pageNumber) => + requestData.fetchResponseDetails(pageNumber), + } +} + export async function fetchResponseDetails (req, res, next) { try { - if (req.locals.template !== failedFileRequestTemplate && req.locals.template !== failedUrlRequestTemplate) { - const responseDetails = req.locals.template === resultsTemplate - ? await req.locals.requestData.fetchResponseDetails(req.params.pageNumber, 50, 'error') - : await req.locals.requestData.fetchResponseDetails(req.params.pageNumber) + const config = TEMPLATE_CONFIG[req.locals.template] || TEMPLATE_CONFIG.default + if (config.fetchDetails) { + const responseDetails = await config.fetchDetails( + req.locals.requestData, + req.params.pageNumber + ) req.locals.responseDetails = responseDetails }Also applies to: 78-81
159-162: Consider parameterising the SQL query.The hardcoded SQL query could be moved to a configuration file or query builder for better maintainability.
+const ISSUE_TYPE_QUERY = ` + SELECT description, issue_type, name, severity, responsibility, + quality_dimension, quality_criteria, quality_criteria_level + FROM issue_type +` + export const getIssueTypesWithQualityCriteriaLevels = fetchMany({ - query: ({ req }) => 'select description, issue_type, name, severity, responsibility, quality_dimension, quality_criteria, quality_criteria_level from issue_type', + query: ({ req }) => ISSUE_TYPE_QUERY, result: 'issueTypes' })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/resultsController.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
src/controllers/resultsController.js (1)
4-4: LGTM! Well-structured middleware pipeline.The middleware setup follows a logical flow, starting with issue types and progressing through issue extraction, processing, and task categorisation.
Also applies to: 19-24
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/controllers/resultsController.js (1)
294-324: Optimize repeated array operations.Consider caching the results of
tasks.findIndexoperations to avoid repeated array traversals.export function getPassedChecks (req, res, next) { const { tasks } = req const { responseDetails } = req.locals const passedChecks = [] + const taskTypes = new Set(tasks.map(task => task.issueType)) // add task complete for how many rows are in the table const rowCount = responseDetails.getRows().length if (rowCount > 0) { passedChecks.push(makePassedCheck(`Found ${responseDetails.getRows().length} rows`)) } // add task complete for no duplicate refs - if (tasks.findIndex(task => task.issueType === 'reference values are not unique') < 0) { + if (!taskTypes.has('reference values are not unique')) { passedChecks.push(makePassedCheck('All rows have unique references')) } // add task complete for valid geoms - if (tasks.findIndex(task => task.issueType === 'invalid WKT') < 0) { + if (!taskTypes.has('invalid WKT')) { passedChecks.push(makePassedCheck('All rows have valid geometry')) } // add task complete for all data is valid if (tasks.length === 0) { passedChecks.push(makePassedCheck('All data is valid')) } req.locals.passedChecks = passedChecks next() }src/views/check/results/results.html (2)
27-66: Enhance map accessibility.Consider adding more descriptive ARIA labels and keyboard navigation support for the map component.
- <div id="map" class="app-map" aria-label="Map showing contour of the submitted data on a map"></div> + <div id="map" + class="app-map" + role="region" + aria-label="Interactive map showing geographical data" + tabindex="0"> + <div class="govuk-visually-hidden"> + Use arrow keys to navigate the map. Press Tab to exit the map. + </div> + </div>
141-153: Add error handling for map script loading.Consider adding error handling for map script loading and initialization failures.
<script> window.serverContext = { containerId: 'map', geometries: {{ options.geometries | dump | safe }}, {% if options.deepLink.orgId %}boundaryGeoJsonUrl: "/api/lpa-boundary/{{ options.deepLink.orgId }}"{% endif %} } + + window.addEventListener('error', function(e) { + if (e.target.src && e.target.src.includes('map.bundle.js')) { + console.error('Failed to load map script:', e); + document.getElementById('map').innerHTML = '<p class="govuk-body">Sorry, we were unable to load the map. Please try refreshing the page.</p>'; + } + }, true); </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/resultsController.js(5 hunks)src/views/check/results/results.html(1 hunks)
🔇 Additional comments (8)
src/controllers/resultsController.js (7)
19-26: LGTM! Well-structured middleware setup.The middleware functions are arranged in a logical sequence, ensuring proper data flow from fetching to processing and categorization.
166-175: Add error handling for issue extraction.The function should handle potential errors when accessing responseDetails or mapping issue logs.
177-189: Handle missing issueTypes gracefully.When
issueTypeis not found inissueTypes,issueType.quality_criteria_levelwill cause an error.
191-210: Fix typo in function name and add error handling.The function name 'aggrogateIssues' should be 'aggregateIssues'.
235-277: Reduce code duplication in task mapping functions.The task mapping logic is duplicated between
getBlockingTasksandgetNonBlockingIssues.
279-292: LGTM! Well-structured helper function.The function is pure, reusable, and has a clear single responsibility.
161-164: Add error handling for missing quality_criteria_level.Consider handling cases where quality_criteria_level might be null in the database.
src/views/check/results/results.html (1)
1-23: LGTM! Well-structured template setup.The template follows GOV.UK Design System best practices with proper component imports and page title handling.
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
config/acceptanceTests.yaml (3)
1-3: Consider using block style for the asyncRequestApi configuration.
It is recommended to quote URL strings in YAML to avoid parsing ambiguities caused by colon characters. Converting from an inline mapping with curly braces to a block style improves readability and consistency with the rest of the file.Diff suggestion:
+asyncRequestApi:
4-7: Standardise the AWS configuration style.
For clarity and consistency, consider converting the inline mapping for the AWS configuration to block style and quote string values (e.g. region and bucket) to avoid potential YAML parsing pitfalls.Diff suggestion:
-aws: {
- region: eu-west-2,
- bucket: 'development-pub-async-request-files',
-}
+aws:
- region: "eu-west-2"
- bucket: "development-pub-async-request-files"
8-12: Ensure consistent mapping style in the Redis configuration.
While the Redis configuration is functionally correct, aligning its style with the rest of the file by switching from inline mapping to block style can increase the readability and maintainability of the configuration.Diff suggestion:
-redis: {
- secure: false,
- host: 'localhost',
- port: 6379
-}
+redis:
- secure: false
- host: "localhost"
- port: 6379
test/acceptance/request_check.test.js (4)
14-17: Consider parameterising the navigateToCheck function.The function hardcodes the dataset and organisation details. Consider making these parameters to improve reusability across different test scenarios.
-const navigateToCheck = async (page) => { +const navigateToCheck = async (page, { dataset = 'article-4-direction', orgName = 'Adur District Council', orgId = 'local-authority:ADU' } = {}) => { - await page.goto('/check/link?dataset=article-4-direction&orgName=Adur%20District%20Council&orgId=local-authority%3AADU') + const params = new URLSearchParams({ + dataset, + orgName, + orgId + }) + await page.goto(`/check/link?${params}`) return new UploadMethodPage(page) }
21-31: Consider enhancing the log function with error handling.The log function provides useful timing information but could benefit from error handling and consistent timestamp formatting.
function log (message, start = false) { + try { if (start) { lastTimestamp = new Date().getTime() - console.log(message) + console.log(`${new Date().toISOString()} - ${message}`) return } const currentTimestamp = new Date().getTime() const elapsed = (currentTimestamp - lastTimestamp) / 1000 - console.log(`${message} (Elapsed: ${elapsed.toFixed(2)}s)`) + console.log(`${new Date().toISOString()} - ${message} (Elapsed: ${elapsed.toFixed(2)}s)`) lastTimestamp = currentTimestamp + } catch (error) { + console.error('Error in log function:', error) + } }
83-85: Consider consolidating page assertions.The sequence of assertions
expectPageHasTitle()andexpectPageHasTabs()appears frequently. Consider creating a helper method to reduce duplication.+async function expectStandardPageElements(resultsPage, { hasTabs = true, hasBlockingTasks = false } = {}) { + await resultsPage.expectPageHasTitle() + if (hasBlockingTasks) { + await resultsPage.expectPageHasBlockingTasks() + } + await resultsPage.expectPageHasTabs(hasTabs) +} -await resultsPage.expectPageHasTitle() -await resultsPage.expectPageHasBlockingTasks() -await resultsPage.expectPageHasTabs() +await expectStandardPageElements(resultsPage, { hasBlockingTasks: true })
141-252: Consider using test parameterisation to reduce duplication.The JavaScript enabled and disabled test suites share similar structure and assertions. Consider using test parameterisation to reduce code duplication.
const testScenarios = [ { name: 'with javascript enabled', jsEnabled: true }, { name: 'with javascript disabled', jsEnabled: false } ] for (const { name, jsEnabled } of testScenarios) { test.describe(name, () => { if (!jsEnabled) { test.use({ javaScriptEnabled: false }) } // Your test cases here, with conditional logic based on jsEnabled test('request check of a @datafile', async ({ page }) => { // ... existing test logic with conditional branches }) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/acceptanceTests.yaml(1 hunks)test/acceptance/playwright.config.js(1 hunks)test/acceptance/request_check.test.js(7 hunks)test/integration/playwright.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (5)
config/acceptanceTests.yaml (2)
13-13: Base URL configuration is satisfactory.
The base URL is properly quoted and adheres to YAML best practices.
14-18: Features configuration is clear and well-structured.
The feature flags are defined using a block style, which enhances readability and explicitness. No changes needed here.test/acceptance/playwright.config.js (1)
25-25: Verify CI environment resources for increased worker count.Increasing workers from 1 to 3 will improve test execution speed. However, with 3 browser configurations, this could lead to up to 9 concurrent processes.
Please ensure your CI environment has sufficient resources (CPU/memory) to handle the increased concurrency. You can check the CI logs for any performance degradation or resource constraints after this change.
test/integration/playwright.config.js (2)
19-20: Consider aligning parallel execution settings.There's an inconsistency in parallel execution settings between acceptance and integration tests:
- Acceptance tests:
fullyParallel: false- Integration tests:
fullyParallel: trueWhile both configurations now use 3 workers, the different parallel execution settings might lead to inconsistent resource utilisation.
Could you clarify the reasoning behind keeping different parallel execution settings? This might affect the benefits of the increased worker count differently in each test suite.
Also applies to: 25-26
26-26: Monitor test stability with increased parallelisation.The combination of
fullyParallel: trueand increased workers could significantly impact test stability, especially with the local web server running.Please monitor:
- Test flakiness in CI logs
- Web server stability under concurrent load
- Memory usage patterns
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/acceptance/request_check.test.js (1)
163-163:⚠️ Potential issueReplace fixed timeout with dynamic waiting.
Using fixed timeouts could lead to flaky tests. Consider using Playwright's built-in waiting mechanisms.
-await page.waitForTimeout(5000) +await statusPage.waitForSelector('[data-testid="processing-complete"]', { state: 'visible' })Also applies to: 192-192, 219-219, 249-249
🧹 Nitpick comments (4)
test/acceptance/request_check.test.js (4)
14-17: Add error handling to the navigation helper.The
navigateToCheckfunction should handle navigation failures gracefully.const navigateToCheck = async (page) => { - await page.goto('/check/link?dataset=article-4-direction&orgName=Adur%20District%20Council&orgId=local-authority%3AADU') + try { + await page.goto('/check/link?dataset=article-4-direction&orgName=Adur%20District%20Council&orgId=local-authority%3AADU', { waitUntil: 'networkidle' }) + } catch (error) { + throw new Error(`Failed to navigate to check page: ${error.message}`) + } return new UploadMethodPage(page) }
21-31: Consider enhancing the logging utility.The logging function is a good addition for debugging and performance monitoring. Consider adding log levels (e.g., INFO, DEBUG) and the ability to disable logging in production tests.
+const LogLevel = { + INFO: 'INFO', + DEBUG: 'DEBUG' +} + +const isTestMode = process.env.NODE_ENV === 'test' + function log (message, start = false) { + if (!isTestMode) return + if (start) { lastTimestamp = new Date().getTime() - console.log(message) + console.log(`[${LogLevel.INFO}] ${message}`) return } const currentTimestamp = new Date().getTime() const elapsed = (currentTimestamp - lastTimestamp) / 1000 - console.log(`${message} (Elapsed: ${elapsed.toFixed(2)}s)`) + console.log(`[${LogLevel.DEBUG}] ${message} (Elapsed: ${elapsed.toFixed(2)}s)`) lastTimestamp = currentTimestamp }
97-97: Extract URLs into constants.Move hardcoded URLs into named constants at the top of the file to improve maintainability.
+const TEST_URLS = { + VALID_ARTICLE_4: 'https://raw.githubusercontent.com/digital-land/PublishExamples/refs/heads/main/Article4Direction/Files/Article4DirectionArea/article4directionareas-(Permitted-development-rights%20column%20missing).csv', + ERROR_ARTICLE_4: 'https://raw.githubusercontent.com/digital-land/PublishExamples/refs/heads/main/Article4Direction/Files/Article4DirectionArea/article4directionareas-errors.csv' +} // In the test -await submitURLPage.enterURL('https://raw.githubusercontent.com/digital-land/PublishExamples/refs/heads/main/Article4Direction/Files/Article4DirectionArea/article4directionareas-(Permitted-development-rights%20column%20missing).csv') +await submitURLPage.enterURL(TEST_URLS.VALID_ARTICLE_4)Also applies to: 125-125
44-44: Extract test file paths into constants.Move hardcoded test file paths into named constants at the top of the file to improve maintainability.
+const TEST_FILES = { + VALID_ARTICLE_4: 'test/datafiles/article4directionareas-ok.csv', + ERROR_ARTICLE_4: 'test/datafiles/article4directionareas-error.csv' +} // In the test -await uploadFilePage.uploadFile('test/datafiles/article4directionareas-ok.csv') +await uploadFilePage.uploadFile(TEST_FILES.VALID_ARTICLE_4)Also applies to: 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/acceptance/request_check.test.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
|
When I submit this URL Update: ok, that behaviour was agreed upon (temporarily). There will be a followup task for that. |
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/controllers/resultsController.js (4)
174-187: Improve error handling and add logging for missing issue types.When an issue type is not found, we should log this occurrence to help track potential data inconsistencies.
export function addQualityCriteriaLevelsToIssues (req, res, next) { + try { const { issues, issueTypes } = req const issueTypeMap = new Map(issueTypes.map(it => [it.issue_type, it])) req.issues = issues.map(issue => { const issueType = issueTypeMap.get(issue['issue-type']) + if (!issueType) { + console.warn(`Issue type not found: ${issue['issue-type']}`) + } return { ...issue, quality_criteria_level: issueType ? issueType.quality_criteria_level : null } }) next() + } catch (error) { + next(error) + } }
250-263: Extract tag configuration to constants and add error handling.The tag colours and text should be defined as constants, and error handling should be added.
+const TASK_LEVELS = { + BLOCKING: { + level: 2, + color: 'red', + text: 'Must fix' + }, + NON_BLOCKING: { + level: 3, + color: 'yellow', + text: 'Should fix' + } +} + export function getTasksByLevel (req, res, next, level, tagColor, tagText) { + try { const { tasks, totalRows } = req const filteredTasks = tasks.filter(task => task.qualityCriteriaLevel === level) const taskParams = filteredTasks.map(task => { const taskMessage = performanceDbApi.getTaskMessage({ issue_type: task.issueType, num_issues: task.count, rowCount: totalRows, field: task.field }) return makeTaskParam(taskMessage, tagText, `govuk-tag--${tagColor}`) }) req.locals[`tasks${level === 2 ? 'Blocking' : 'NonBlocking'}`] = taskParams + } catch (error) { + next(error) + } }
138-138: Fix typo in variable name.The variable name contains a typo.
-const pageNumer = Number.parseInt(req.params.pageNumber) +const pageNumber = Number.parseInt(req.params.pageNumber)
265-268: Combine blocking and non-blocking task functions.The
getBlockingTasksandgetNonBlockingTasksfunctions share similar logic and can be combined into a single function.-export function getBlockingTasks (req, res, next) { - getTasksByLevel(req, res, next, 2, 'red', 'Must fix') - next() -} - -export function getNonBlockingTasks (req, res, next) { - const { responseDetails } = req.locals - getTasksByLevel(req, res, next, 3, 'yellow', 'Should fix') - - const columnFieldLog = responseDetails.getColumnFieldLog() - - const missingColumnTasks = columnFieldLog - .filter(column => column.missing) - .map(({ field }) => makeTaskParam(`${field} Column is missing`, 'Must fix', 'govuk-tag--red')) - - req.locals.tasksBlocking = req.locals.tasksBlocking.concat(missingColumnTasks) - - // add tasks from missing columns - next() -} +export function getTasks (req, res, next) { + try { + // Get blocking tasks + getTasksByLevel(req, res, next, TASK_LEVELS.BLOCKING.level, TASK_LEVELS.BLOCKING.color, TASK_LEVELS.BLOCKING.text) + + // Get non-blocking tasks + getTasksByLevel(req, res, next, TASK_LEVELS.NON_BLOCKING.level, TASK_LEVELS.NON_BLOCKING.color, TASK_LEVELS.NON_BLOCKING.text) + + // Add missing column tasks to blocking tasks + const { responseDetails } = req.locals + const columnFieldLog = responseDetails.getColumnFieldLog() + const missingColumnTasks = columnFieldLog + .filter(column => column.missing) + .map(({ field }) => makeTaskParam(`${field} Column is missing`, TASK_LEVELS.BLOCKING.text, `govuk-tag--${TASK_LEVELS.BLOCKING.color}`)) + + req.locals.tasksBlocking = req.locals.tasksBlocking.concat(missingColumnTasks) + + next() + } catch (error) { + next(error) + } +}Also applies to: 270-284
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/resultsController.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run-tests / test
- GitHub Check: run-tests / test
🔇 Additional comments (2)
src/controllers/resultsController.js (2)
254-260: 🛠️ Refactor suggestionAdd error handling for task message generation.
The call to
performanceDbApi.getTaskMessageshould be wrapped in a try-catch block to handle potential API errors gracefully.- const taskMessage = performanceDbApi.getTaskMessage({ - issue_type: task.issueType, - num_issues: task.count, - rowCount: totalRows, - field: task.field - }) - return makeTaskParam(taskMessage, tagText, `govuk-tag--${tagColor}`) + try { + const taskMessage = performanceDbApi.getTaskMessage({ + issue_type: task.issueType, + num_issues: task.count, + rowCount: totalRows, + field: task.field + }) + return makeTaskParam(taskMessage, tagText, `govuk-tag--${tagColor}`) + } catch (error) { + console.error('Failed to generate task message:', error) + return makeTaskParam( + `${task.count} ${task.issueType} issues found`, + tagText, + `govuk-tag--${tagColor}` + ) + }Likely invalid or redundant comment.
298-306: 🛠️ Refactor suggestionExtract magic strings to constants and optimise task lookup.
The issue type strings should be constants, and the linear search with
findIndexshould be replaced with a Set for better performance.+const ISSUE_TYPES = { + DUPLICATE_REFS: 'reference values are not unique', + INVALID_WKT: 'invalid WKT' +} + export function getPassedChecks (req, res, next) { const { tasks, totalRows } = req const passedChecks = [] + const taskTypes = new Set(tasks.map(task => task.issueType)) const makePassedCheck = (text) => makeTaskParam(text, 'Passed', 'govuk-tag--green') // add task complete for no duplicate refs - if (tasks.findIndex(task => task.issueType === 'reference values are not unique') < 0) { + if (!taskTypes.has(ISSUE_TYPES.DUPLICATE_REFS)) { passedChecks.push(makePassedCheck('All rows have unique references')) } // add task complete for valid geoms - if (tasks.findIndex(task => task.issueType === 'invalid WKT') < 0) { + if (!taskTypes.has(ISSUE_TYPES.INVALID_WKT)) { passedChecks.push(makePassedCheck('All rows have valid geometry')) }Likely invalid or redundant comment.
Preview Link
https://submit-pr-842.herokuapp.com/
What type of PR is this? (check all applicable)
Description
This ticket adds the task list to the check results page. it also gets rid of the separate errors and no errors results pages and replaces them with a single results page
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
api error page
when the request processor fails due to for example an invalid file type, we currently just show an empty error summary. I've now changed this to show a 500 page. ideally we need a better design for this that gives more info
I've drafted a ticket here to complete this work: https://github.com/orgs/digital-land/projects/7/views/11?pane=issue&itemId=96985825
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests