feat: add column mapping functionality to C&P#1196
Open
gibahjoe wants to merge 24 commits into
Open
Conversation
4 tasks
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a column→field mapping workflow and task-log driven issue handling: new column-mapping route/controller and helpers, StatusController decision logic, ResultsController task-log extraction and optional mapping rows, Datasette/Redis caching for provisioning reasons, new templates/styles, updated fixtures, and unit/acceptance tests. ChangesColumn Mapping Feature & task-log integration
Sequence DiagramsequenceDiagram
participant User
participant StatusController
participant StatusLogic as shouldShowColumnMapping
participant ColumnController as ColumnMappingController
participant AsyncAPI as postCheckRequest
participant ResultsView
User->>StatusController: POST /status/:id
StatusController->>StatusLogic: shouldShowColumnMapping(requestData)
alt mapping needed
StatusController->>User: redirect /column-mapping/:id
User->>ColumnController: GET /column-mapping/:id
ColumnController->>User: render mapping form
User->>ColumnController: POST fieldMap
ColumnController->>ColumnController: validateColumnMapping
alt invalid
ColumnController->>User: 400 re-render with errors
else
ColumnController->>AsyncAPI: postCheckRequest(mapping)
AsyncAPI->>ColumnController: response(request_id)
ColumnController->>User: redirect /check/status/:newId
end
else
StatusController->>User: redirect /results/:id/1
end
User->>ResultsView: view results
ResultsView->>User: show Field mapping summary if present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/unit/redisLoader.test.js (1)
93-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate TTL assertions to the new 6-hour cache expiry.
These expectations still assert
300, but runtime now uses21600(6 hours), which is causing the current pipeline failures.Suggested fix
- 300, + 21600, @@ - 300, + 21600, @@ - 300, + 21600,Also applies to: 173-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/redisLoader.test.js` around lines 93 - 97, Update the TTL used in the test assertions from 300 to 21600 to match the new 6-hour cache expiry: change the expect calls that reference mockRedisClient.setEx (e.g., the assertion verifying setEx was called with 'deploy-123:dataset:example-dataset') to expect 21600 as the TTL (this same change needs to be applied to the second block of assertions around the test lines 173-185 that also call mockRedisClient.setEx); keep the other arguments (key and JSON.stringify payload) unchanged.
🧹 Nitpick comments (6)
test/unit/views/check/columnMapping.test.js (2)
46-46: ⚡ Quick winAvoid asserting inline style implementation details.
This assertion is brittle and can fail on harmless CSS/template refactors; prefer asserting behaviour/content rather than inline
styletext.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/views/check/columnMapping.test.js` at line 46, The test is asserting an inline style string which is brittle; update the assertion that currently reads against document.querySelector('td').getAttribute('style') to instead assert behavior from the element itself—for example, get the td via document.querySelector('td') and assert its computed style verticalAlign (via window.getComputedStyle(td).verticalAlign) equals 'middle', or assert that a specific CSS class is present on the td that conveys the same layout; replace the brittle inline-style check with one of these behavior-oriented assertions in columnMapping.test.js.
50-53: ⚡ Quick winUse field-scoped selectors instead of first-match
.govuk-form-group.These checks depend on DOM order and can become flaky. Target the row/control for
notesdirectly (or assert error-row count) so the test validates intent, not incidental ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/views/check/columnMapping.test.js` around lines 50 - 53, Replace brittle first-match selectors with field-scoped ones that target the "notes" control: find the form-group/row that contains the notes label or input (e.g., locate the label or input with text/for "notes" and get its closest .govuk-form-group or tr) and run your assertions against that element (check its class includes or excludes govuk-form-group--error and that the error-row tr with .govuk-form-group--error contains the text "notes"); alternatively assert the count of error rows if appropriate. Ensure you still assert the button text using button[form="columnMappingForm"].src/views/check/column-mapping.html (1)
43-43: ⚡ Quick winMove inline styles to CSS.
The inline style
style="background-color:#f3f2f1; padding: 15px 20px;"should be defined as a CSS class to maintain separation of concerns and reusability.♻️ Define CSS class for grey panel
Add to
src/assets/scss/index.scss:.column-mapping-panel { background-color: `#f3f2f1`; padding: govuk-spacing(3) govuk-spacing(4); }Then update the template:
- <div class="govuk-!-margin-bottom-6" style="background-color: `#f3f2f1`; padding: 15px 20px;"> + <div class="govuk-!-margin-bottom-6 column-mapping-panel">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/check/column-mapping.html` at line 43, Replace the inline style on the div in src/views/check/column-mapping.html by creating a reusable CSS class (e.g., .column-mapping-panel) in your global stylesheet (src/assets/scss/index.scss) that sets background-color and padding, then update the div (which currently uses govuk-!-margin-bottom-6) to include the new .column-mapping-panel class instead of the inline style so styling is separated and reusable.src/controllers/resultsController.js (1)
238-243: 💤 Low valueSimplify filter condition.
The filter checks
!mappedField || !colandtoUpperCase() === 'IGNORE'separately. The logic is correct but could be slightly clearer with early returns or combined conditions.♻️ Alternative filter for clarity
- .filter(({ mappedField, col }) => { - // skip empty values and explicit IGNORE mappings - if (!mappedField || !col) return false - if (typeof mappedField === 'string' && mappedField.toUpperCase() === 'IGNORE') return false - return true - }) + .filter(({ mappedField, col }) => + mappedField && col && + !(typeof mappedField === 'string' && mappedField.toUpperCase() === 'IGNORE') + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/resultsController.js` around lines 238 - 243, The filter callback in resultsController.js currently uses multiple if returns to exclude empty values and 'IGNORE' mappings; replace it with a single, clearer boolean return expression in the .filter callback (refer to the .filter(({ mappedField, col }) => { ... }) function) that returns true only when mappedField and col are truthy and mappedField is not the string 'IGNORE' (case-insensitive). Ensure you preserve the case-insensitive check for mappedField by normalizing it (e.g., toUpperCase) only when it's a string before comparing.src/controllers/columnMappingController.js (1)
177-177: ⚡ Quick winGuard against undefined
body.columns.If
body.columnsisundefined, the expression[body.columns].filter(Boolean)creates[undefined]which then filters to[]. While this works, it's clearer to handleundefinedexplicitly.♻️ Clearer handling for undefined columns
- const columns = Array.isArray(body.columns) ? body.columns : [body.columns].filter(Boolean) + const columns = body.columns ? (Array.isArray(body.columns) ? body.columns : [body.columns]) : []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/columnMappingController.js` at line 177, The current assignment to const columns (in columnMappingController.js) uses Array.isArray(body.columns) ? body.columns : [body.columns].filter(Boolean), which is unclear when body.columns is undefined; update the expression to explicitly handle undefined/null by checking body.columns first (e.g., if Array.isArray(body.columns) return body.columns, else if body.columns is truthy wrap it in [body.columns], otherwise return an empty array) so columns is always a clear array value.src/views/check/components/column-mapping.html (1)
80-83: ⚡ Quick winMove inline styles to CSS for the un-map dropdown.
The native
<select>element uses extensive inline styles that should be extracted to a CSS class for maintainability and consistency with GOV.UK patterns.♻️ Define CSS class for mapped-status dropdown
Add to
src/assets/scss/index.scss:.column-mapping__unmap-select { background-color: govuk-colour('green'); color: govuk-colour('white'); font-weight: 700; font-size: 0.875rem; padding: 2px 8px; border: none; border-radius: 0; cursor: pointer; appearance: auto; }Then update the template:
- <select name="map[{{ row.column }}]" style="background-color: `#00703c`; color: `#ffffff`; font-weight: 700; font-size: 0.875rem; padding: 2px 8px; border: none; border-radius: 0; cursor: pointer; appearance: auto;"> + <select name="map[{{ row.column }}]" class="column-mapping__unmap-select">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/views/check/components/column-mapping.html` around lines 80 - 83, Extract the inline styles from the select element (name="map[{{ row.column }}]") into a reusable CSS class (e.g., .column-mapping__unmap-select) in your main SCSS file, implementing the visual rules shown (green background, white text, bold, 0.875rem font-size, 2px 8px padding, no border, no radius, pointer cursor, appearance:auto); then update the template to remove the inline style and add class="column-mapping__unmap-select" to the select so the styling is maintained via CSS instead of inline attributes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/statusController.js`:
- Around line 81-83: The blocking-error gate currently only checks the first 50
rows because statusController.js calls fetchResponseDetails(0, 50); replace that
single-page fetch with a pagination loop that repeatedly calls
fetchResponseDetails(offset, limit) (e.g., offset += limit) and accumulates
responseDetails.getRows() until a call returns fewer rows than limit (or none),
then compute the blocking-error gate from the full accumulated rows; apply the
same pagination fix to the other occurrence around the block that spans lines
referenced (the 91-98 logic) so both places use the full dataset rather than
just the first 50 rows.
In `@src/utils/redisLoader.js`:
- Line 97: The call to normaliseDatasetFields is missing the dataset argument so
the helper's dataset !== field guard never runs; update the invocation where
response.formattedData is used (the line calling normaliseDatasetFields) to pass
the current dataset variable as the second parameter (i.e.,
normaliseDatasetFields(response.formattedData, dataset)) so the function
receives both formatted data and dataset and the guard in normaliseDatasetFields
can operate correctly.
---
Outside diff comments:
In `@test/unit/redisLoader.test.js`:
- Around line 93-97: Update the TTL used in the test assertions from 300 to
21600 to match the new 6-hour cache expiry: change the expect calls that
reference mockRedisClient.setEx (e.g., the assertion verifying setEx was called
with 'deploy-123:dataset:example-dataset') to expect 21600 as the TTL (this same
change needs to be applied to the second block of assertions around the test
lines 173-185 that also call mockRedisClient.setEx); keep the other arguments
(key and JSON.stringify payload) unchanged.
---
Nitpick comments:
In `@src/controllers/columnMappingController.js`:
- Line 177: The current assignment to const columns (in
columnMappingController.js) uses Array.isArray(body.columns) ? body.columns :
[body.columns].filter(Boolean), which is unclear when body.columns is undefined;
update the expression to explicitly handle undefined/null by checking
body.columns first (e.g., if Array.isArray(body.columns) return body.columns,
else if body.columns is truthy wrap it in [body.columns], otherwise return an
empty array) so columns is always a clear array value.
In `@src/controllers/resultsController.js`:
- Around line 238-243: The filter callback in resultsController.js currently
uses multiple if returns to exclude empty values and 'IGNORE' mappings; replace
it with a single, clearer boolean return expression in the .filter callback
(refer to the .filter(({ mappedField, col }) => { ... }) function) that returns
true only when mappedField and col are truthy and mappedField is not the string
'IGNORE' (case-insensitive). Ensure you preserve the case-insensitive check for
mappedField by normalizing it (e.g., toUpperCase) only when it's a string before
comparing.
In `@src/views/check/column-mapping.html`:
- Line 43: Replace the inline style on the div in
src/views/check/column-mapping.html by creating a reusable CSS class (e.g.,
.column-mapping-panel) in your global stylesheet (src/assets/scss/index.scss)
that sets background-color and padding, then update the div (which currently
uses govuk-!-margin-bottom-6) to include the new .column-mapping-panel class
instead of the inline style so styling is separated and reusable.
In `@src/views/check/components/column-mapping.html`:
- Around line 80-83: Extract the inline styles from the select element
(name="map[{{ row.column }}]") into a reusable CSS class (e.g.,
.column-mapping__unmap-select) in your main SCSS file, implementing the visual
rules shown (green background, white text, bold, 0.875rem font-size, 2px 8px
padding, no border, no radius, pointer cursor, appearance:auto); then update the
template to remove the inline style and add class="column-mapping__unmap-select"
to the select so the styling is maintained via CSS instead of inline attributes.
In `@test/unit/views/check/columnMapping.test.js`:
- Line 46: The test is asserting an inline style string which is brittle; update
the assertion that currently reads against
document.querySelector('td').getAttribute('style') to instead assert behavior
from the element itself—for example, get the td via document.querySelector('td')
and assert its computed style verticalAlign (via
window.getComputedStyle(td).verticalAlign) equals 'middle', or assert that a
specific CSS class is present on the td that conveys the same layout; replace
the brittle inline-style check with one of these behavior-oriented assertions in
columnMapping.test.js.
- Around line 50-53: Replace brittle first-match selectors with field-scoped
ones that target the "notes" control: find the form-group/row that contains the
notes label or input (e.g., locate the label or input with text/for "notes" and
get its closest .govuk-form-group or tr) and run your assertions against that
element (check its class includes or excludes govuk-form-group--error and that
the error-row tr with .govuk-form-group--error contains the text "notes");
alternatively assert the count of error rows if appropriate. Ensure you still
assert the button text using button[form="columnMappingForm"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9a6981dc-bd09-4f51-bb50-085725fa446e
📒 Files selected for processing (15)
config/default.yamlsrc/assets/scss/index.scsssrc/controllers/columnMappingController.jssrc/controllers/resultsController.jssrc/controllers/statusController.jssrc/routes/form-wizard/check/steps.jssrc/services/asyncRequestApi.jssrc/utils/redisLoader.jssrc/views/check/column-mapping.htmlsrc/views/check/components/column-mapping.htmlsrc/views/check/results/results.htmltest/unit/columnMappingController.test.jstest/unit/redisLoader.test.jstest/unit/statusController.test.jstest/unit/views/check/columnMapping.test.js
Contributor
|
Actionable comments posted: 0 |
- Refactored StatusController to include middleware for fetching dataset information from platform API. - Updated shouldShowColumnMapping function to consider unique dataset fields and handle geometry/point field logic. - Removed unused getDatasetFields function and related normalisation logic from redisLoader. - Modified requestData model to include mandatory field information in column mapping. - Improved column mapping HTML to provide clearer user instructions and handle user ignored fields. - Enhanced unit tests for StatusController and column mapping to cover new functionality and edge cases.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/views/check/columnMapping.test.js (1)
86-113:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest name contradicts assertions.
The test is named "does not show not provided for required fields" but lines 111-112 assert that
option[value="na"]does exist and contains "Not provided". Either the test name is misleading or the assertions are incorrect.Proposed fix (if the option should appear)
- it('does not show not provided for required fields', () => { + it('shows not provided option for required fields', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/views/check/columnMapping.test.js` around lines 86 - 113, The test "does not show not provided for required fields" is inconsistent with its assertions which expect option[value="na"] to exist and contain "Not provided"; either rename the test or update the assertions: if the UI should show the "Not provided" option for required fields, rename the it(...) description to "shows not provided for required fields" (in test file columnMapping.test.js) so it matches the expectations, otherwise change the two expect(...) lines to assert absence (use toBeNull() / not.toContain) to reflect that "na" should not be rendered for required mappings.config/default.yaml (1)
127-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrailing space in
variablevalue appears unintentional.The value
planhas a trailing space which will affect display text formatting. This looks like an accidental edit.Proposed fix
- variable: plan + variable: plan🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/default.yaml` at line 127, The YAML entry where the key "variable" is set to the string "plan " contains an unintended trailing space; update the value for the "variable" field from "plan " to "plan" (remove the trailing whitespace) and verify there are no other occurrences of the same trailing-space value in the config/default.yaml so display/formatting is correct.
🧹 Nitpick comments (2)
src/controllers/resultsController.js (1)
276-296: 💤 Low value
extractIssuesFromResultsappears to be dead code.
extractIssuesFromResults(lines 276-285) is no longer used in the middleware pipeline since it was replaced byextractIssuesFromTaskLog. Consider removing it if it's not needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/resultsController.js` around lines 276 - 296, The function extractIssuesFromResults appears to be dead code; search for any remaining references to extractIssuesFromResults and, if none exist, remove the entire function (exported symbol extractIssuesFromResults) from the file and exports so only the active middleware (extractIssuesFromTaskLog and filterOutInternalIssues) remain; if there are other callers that still need the behavior, instead update those callers to use req.locals.requestData.getIssueTasks() or adapt extractIssuesFromResults to delegate to that method (using responseDetails.response -> issue_logs) and keep the export.test/unit/requestData.test.js (1)
9-15: 💤 Low valueIncorrect mock path for logger.
The mock path
'../utils/logger.js'doesn't match the actual import path'../../src/utils/logger.js'. This mock may not intercept the real logger, though thevi.spyOnon line 17 provides a fallback.Proposed fix
-vi.mock('../utils/logger.js', () => { +vi.mock('../../src/utils/logger.js', () => { return { default: { error: vi.fn() } } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/requestData.test.js` around lines 9 - 15, The test's vi.mock call is targeting the wrong module path so it doesn't intercept the real logger import; update the vi.mock invocation to match the actual import used in the test (replace the current mock path with the same module string used by the code under test, ensure the mock returns an object with default.error: vi.fn()), and then you can rely on that mock instead of the vi.spyOn fallback currently present (adjust or remove the vi.spyOn on the logger's error if redundant).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/production.yaml`:
- Line 36: The production config currently sets the feature flag
checkIssueDetailsPage (enabled: true) which exposes issue-detail pages to all
users; confirm whether this is intentional for this release and if not revert to
disabled or gate it behind a rollout/feature-flag system. If it should remain
off until QA/design/product sign-off, change the production setting for
checkIssueDetailsPage back to false (or implement conditional gating/percentage
rollout) and add a short comment referencing the pending sign-offs to prevent
accidental enablement.
In `@src/controllers/statusController.js`:
- Around line 38-40: The error handling uses the wrong variable for the dataset
name: replace references to req.params.dataset with the already-extracted
params.dataset inside fetchDatasetPlatformInfo so the thrown Error and
logger.warn use params.dataset (the variable set on line 33) instead of
req.params.dataset; update both the Error constructor and the logger.warn call
to reference params.dataset to ensure accurate diagnostics.
---
Outside diff comments:
In `@config/default.yaml`:
- Line 127: The YAML entry where the key "variable" is set to the string "plan "
contains an unintended trailing space; update the value for the "variable" field
from "plan " to "plan" (remove the trailing whitespace) and verify there are no
other occurrences of the same trailing-space value in the config/default.yaml so
display/formatting is correct.
In `@test/unit/views/check/columnMapping.test.js`:
- Around line 86-113: The test "does not show not provided for required fields"
is inconsistent with its assertions which expect option[value="na"] to exist and
contain "Not provided"; either rename the test or update the assertions: if the
UI should show the "Not provided" option for required fields, rename the it(...)
description to "shows not provided for required fields" (in test file
columnMapping.test.js) so it matches the expectations, otherwise change the two
expect(...) lines to assert absence (use toBeNull() / not.toContain) to reflect
that "na" should not be rendered for required mappings.
---
Nitpick comments:
In `@src/controllers/resultsController.js`:
- Around line 276-296: The function extractIssuesFromResults appears to be dead
code; search for any remaining references to extractIssuesFromResults and, if
none exist, remove the entire function (exported symbol
extractIssuesFromResults) from the file and exports so only the active
middleware (extractIssuesFromTaskLog and filterOutInternalIssues) remain; if
there are other callers that still need the behavior, instead update those
callers to use req.locals.requestData.getIssueTasks() or adapt
extractIssuesFromResults to delegate to that method (using
responseDetails.response -> issue_logs) and keep the export.
In `@test/unit/requestData.test.js`:
- Around line 9-15: The test's vi.mock call is targeting the wrong module path
so it doesn't intercept the real logger import; update the vi.mock invocation to
match the actual import used in the test (replace the current mock path with the
same module string used by the code under test, ensure the mock returns an
object with default.error: vi.fn()), and then you can rely on that mock instead
of the vi.spyOn fallback currently present (adjust or remove the vi.spyOn on the
logger's error if redundant).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 59c1cceb-ac4a-4b37-8304-7735d291d1ee
📒 Files selected for processing (25)
.gitignoreconfig/default.yamlconfig/production.yamldocker/request-api-stub/wiremock/__files/check_file/article-4/request-complete-errors.jsondocker/request-api-stub/wiremock/__files/check_file/article-4/request-complete.jsondocker/request-api-stub/wiremock/__files/check_url/article-4/request-complete.jsondocker/request-api-stub/wiremock/mappings/requests_bbw7ebivop3leg7ue9vka4-31b3b282-b265-4f7c-ac66-db61e3f81be2.jsondocker/request-api-stub/wiremock/mappings/requests_bbw7ebivop3leg7ue9vka4-7ffceaf2-278c-46d3-9b8e-f403a36bd93f.jsonsrc/assets/js/map.jssrc/controllers/columnMappingController.jssrc/controllers/issueDetailsController.jssrc/controllers/resultsController.jssrc/controllers/statusController.jssrc/models/requestData.jssrc/utils/redisLoader.jssrc/views/check/column-mapping.htmlsrc/views/check/components/column-mapping.htmltest/acceptance/request_check.test.jstest/unit/columnMappingController.test.jstest/unit/javascript/map.test.jstest/unit/redisLoader.test.jstest/unit/requestData.test.jstest/unit/resultsController.test.jstest/unit/statusController.test.jstest/unit/views/check/columnMapping.test.js
💤 Files with no reviewable changes (1)
- src/utils/redisLoader.js
✅ Files skipped from review due to trivial changes (4)
- docker/request-api-stub/wiremock/__files/check_url/article-4/request-complete.json
- .gitignore
- docker/request-api-stub/wiremock/mappings/requests_bbw7ebivop3leg7ue9vka4-7ffceaf2-278c-46d3-9b8e-f403a36bd93f.json
- test/unit/javascript/map.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/views/check/components/column-mapping.html
- src/views/check/column-mapping.html
- test/unit/statusController.test.js
- test/unit/columnMappingController.test.js
- src/controllers/columnMappingController.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controllers/columnMappingController.js (1)
97-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
spareUploadedColumnsis always empty on POST — spare-column auto-IGNORE never executes.
buildSelectableColumns(columnMappingRows)is called withoutresponseRows, so it defaults to[]and the function returns[]. ConsequentlybuildSubmittedColumnMappingnever reaches the branch that auto-assignsIGNOREto spare uploaded columns, so the submittedcolumn_mappingomits them and the backend re-check will treat them as unmapped again.buildColumnMappingOptionsalready computed the populated list asoptions.uploadedColumns(usingresponseDetails.getRows()), so reuse it and drop the redundantprepareColumnMappingContextcall.🐛 Proposed fix
const params = requestData.getParams() ?? {} - const uniqueDatasetFields = req.uniqueDatasetFields || [] - const { columnMappingRows } = await prepareColumnMappingContext(requestData, uniqueDatasetFields) - const spareUploadedColumns = buildSelectableColumns(columnMappingRows) + const spareUploadedColumns = options.uploadedColumns const columnMapping = buildSubmittedColumnMapping({ existingMapping: params.column_mapping, body: req.body, spareUploadedColumns })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/controllers/columnMappingController.js` around lines 97 - 105, The spareUploadedColumns list is empty because buildSelectableColumns is called with columnMappingRows instead of the full response rows; update the controller to stop calling prepareColumnMappingContext redundantly and instead reuse the uploaded columns computed by buildColumnMappingOptions (options.uploadedColumns, which comes from responseDetails.getRows()) when building spareUploadedColumns for buildSubmittedColumnMapping; ensure you pass that uploadedColumns value into buildSubmittedColumnMapping so the auto-IGNORE branch for spare uploaded columns can execute (references: prepareColumnMappingContext, buildColumnMappingOptions, options.uploadedColumns, responseDetails.getRows, buildSelectableColumns, buildSubmittedColumnMapping, columnMappingRows, spareUploadedColumns).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/resultsController.js`:
- Around line 229-240: The columnMappingRows creation in resultsController.js
uses value: { html: column } which allows stored XSS; change it to use value: {
text: column } so the template (which feeds options.columnMappingRows into
govukSummaryList) treats the column as escaped text. Update the block that maps
requestData.getColumnFieldLog() into columnMappingRows (variable name
columnMappingRows) to set value.text instead of value.html and ensure column is
coerced to a string or empty string as before.
---
Outside diff comments:
In `@src/controllers/columnMappingController.js`:
- Around line 97-105: The spareUploadedColumns list is empty because
buildSelectableColumns is called with columnMappingRows instead of the full
response rows; update the controller to stop calling prepareColumnMappingContext
redundantly and instead reuse the uploaded columns computed by
buildColumnMappingOptions (options.uploadedColumns, which comes from
responseDetails.getRows()) when building spareUploadedColumns for
buildSubmittedColumnMapping; ensure you pass that uploadedColumns value into
buildSubmittedColumnMapping so the auto-IGNORE branch for spare uploaded columns
can execute (references: prepareColumnMappingContext, buildColumnMappingOptions,
options.uploadedColumns, responseDetails.getRows, buildSelectableColumns,
buildSubmittedColumnMapping, columnMappingRows, spareUploadedColumns).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d60cf08b-b639-4e1f-b277-0fb85a39a1b1
📒 Files selected for processing (9)
config/default.yamlsrc/controllers/columnMappingController.jssrc/controllers/resultsController.jssrc/controllers/statusController.jstest/unit/columnMappingController.test.jstest/unit/requestData.test.jstest/unit/statusController.test.jstest/unit/views/check/columnMapping.test.jsvite.config.js
🚧 Files skipped from review as they are similar to previous changes (5)
- test/unit/statusController.test.js
- test/unit/requestData.test.js
- test/unit/views/check/columnMapping.test.js
- test/unit/columnMappingController.test.js
- src/controllers/statusController.js
…nal note functionality in JIRA service
… notes in Jira issue
…sController and shortened manage service url
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds column-mapping feature for the Check & Provide flow: implements
columnMappingController, new check views/components, supporting service and config updates, styles, and unit tests.What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Added/updated tests?
(Added/updated unit tests for column mapping, redis loader and status controller)
QA sign off
[optional] Are there any post-deployment tasks we need to perform?
None
[optional] Are there any dependencies on other PRs or Work?
None
Summary by CodeRabbit