482 dataset details page provision level view table #609
Conversation
…set-details-page-provision-level-view-table
…age-provision-level-view-table
WalkthroughThe pull request introduces several changes across multiple files, primarily adding new middleware functions and modifying existing ones. Key updates include the addition of Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…set-details-page-provision-level-view-table
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
|
One minor comment about the @alextea are you happy with the implementation? If you are - happy to approve |
|
All LGTM 🙌 |
…set-details-page-provision-level-view-table
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
|
Can we make the column names in the table headings use the same format as the guidance: lowercase, hyphen-separated eg: |
…set-details-page-provision-level-view-table
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…age-provision-level-view-table
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/controllers/resultsController.js (1)
Line range hint
11-82: Consider refactoring the locals method for improved maintainabilityThe
localsmethod handles multiple responsibilities including request state management, error handling, and table parameter construction. Consider breaking it down into smaller, focused methods:class ResultsController extends PageController { + async buildTableParams(responseDetails, requestData) { + const rows = this._processRows(responseDetails, requestData); + return { + columns: responseDetails.getColumns().map(column => prettifyColumnName(column)), + rows, + fields: responseDetails.getFields() + }; + } + + _processRows(responseDetails, requestData) { + let rows = responseDetails.getRowsWithVerboseColumns(requestData.hasErrors()); + return rows.map(this._filterErrorSeverity); + } + + _filterErrorSeverity({ columns, ...rest }) { + const columnsOnlyErrors = Object.fromEntries( + Object.entries(columns).map(([key, value]) => { + const error = value.error?.severity === 'error' ? value.error : undefined; + return [key, { ...value, error }]; + }) + ); + return { ...rest, columns: columnsOnlyErrors }; + } async locals(req, res, next) { // existing implementation but calling new methods } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/controllers/resultsController.js(2 hunks)src/routes/schemas.js(5 hunks)src/views/components/table.html(3 hunks)test/unit/errorsPage.test.js(1 hunks)test/unit/noErrorsPage.test.js(1 hunks)test/unit/resultsController.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/schemas.js
- src/views/components/table.html
🔇 Additional comments (3)
src/controllers/resultsController.js (2)
3-3: LGTM: Clean import addition
The import of the prettifyColumnName utility follows proper ES6 module syntax and project conventions.
63-63: Verify column name formatting implementation
The column name formatting looks good, but let's ensure the prettifyColumnName function handles all possible column name formats correctly.
✅ Verification successful
Column name formatting implementation is robust and well-tested
The prettifyColumnName function is well-implemented with comprehensive test coverage that handles:
- Capitalisation of first words
- Special handling of "URL" text
- Various delimiter scenarios (hyphens and spaces)
- Mixed format cases
The implementation aligns perfectly with the table display requirements and includes proper test cases for all edge cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check prettifyColumnName implementation and usage
# Look for the implementation and any test coverage
echo "Checking prettifyColumnName implementation:"
rg -A 5 "export.*prettifyColumnName" src/filters/
echo "\nChecking test coverage:"
rg -l "prettifyColumnName" "test/"
echo "\nChecking current column names in use:"
ast-grep --pattern 'getColumns()' -f .js
Length of output: 809
Script:
#!/bin/bash
# Let's check the implementation and test coverage more thoroughly
echo "Checking prettifyColumnName implementation:"
cat src/filters/prettifyColumnName.js
echo -e "\nChecking test cases:"
cat test/unit/prettifyColumnName.test.js
echo -e "\nChecking actual column usage:"
ast-grep --pattern 'getColumns()'
Length of output: 1726
test/unit/errorsPage.test.js (1)
78-78: Verify column header formatting consistency
The removal of prettifyColumnName from the test assertion suggests that column formatting is now handled at the controller level. Let's ensure this aligns with the table styling requirements mentioned in the PR comments.
Run the following script to verify column header formatting across the application:
✅ Verification successful
Column header formatting is consistently implemented
The verification shows that column header formatting is properly handled:
- The
prettifyColumnNamefunction is used inResultsControllerto format column headers before they reach the view - The test file correctly expects the formatted headers since they are transformed at the controller level
- Other tests (integration and unit) consistently use
prettifyColumnNamefor header validation - The table component in
src/views/components/table.htmlreceives and displays the pre-formatted column headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent column header formatting implementation
# Expected results: Column headers should follow lowercase, hyphen-separated format
# Check for column header formatting in controllers
ast-grep --pattern 'prettifyColumnName($$$)'
# Check for direct column header usage in templates
rg -g '*.html' -A 2 'govuk-table__header'
Length of output: 4339
| @@ -65,7 +65,7 @@ describe('no Errors Page', () => { | |||
| const columnHeaders = tHead.querySelectorAll('.govuk-table__header') | |||
|
|
|||
| params.options.tableParams.columns.forEach((header, i) => { | |||
| expect(columnHeaders[i].textContent).toContain(prettifyColumnName(header)) | |||
| expect(columnHeaders[i].textContent).toContain(header) | |||
There was a problem hiding this comment.
Restore column header formatting validation
The removal of prettifyColumnName from the column header validation seems inconsistent with:
- The PR requirements for specific column name formatting (lowercase, hyphen-separated)
- The continued use of
prettifyColumnNamefor error message validation - Alex's feedback about following the guidance format for column names
This change might allow tests to pass even when column headers don't follow the required format.
Apply this diff to restore the formatting check:
- expect(columnHeaders[i].textContent).toContain(header)
+ expect(columnHeaders[i].textContent).toContain(prettifyColumnName(header))📝 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.
| expect(columnHeaders[i].textContent).toContain(header) | |
| expect(columnHeaders[i].textContent).toContain(prettifyColumnName(header)) |
There was a problem hiding this comment.
we don't want this anymore as alex requested that the table contains spec fields sometimes
There was a problem hiding this comment.
@GeorgeGoodall-GovUk, understood. Since the table may contain specification fields as per Alex's request, it's appropriate to remove the prettifyColumnName function from the column header validation in test/unit/noErrorsPage.test.js. Thanks for the clarification.
✏️ Learnings added
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#609
File: test/unit/noErrorsPage.test.js:68-68
Timestamp: 2024-11-12T10:54:09.278Z
Learning: In `test/unit/noErrorsPage.test.js`, avoid using `prettifyColumnName` for column headers because the table can contain spec fields, as requested by Alex.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| getColumns: () => ['mock Columns'], | ||
| getFields: () => ['mock fields'], |
There was a problem hiding this comment.
Ensure consistent casing in mock data
The mock data uses inconsistent casing between 'mock Columns' and 'mock fields'. This could lead to test failures and makes the test harder to maintain.
Apply this diff to maintain consistent casing:
- getColumns: () => ['mock Columns'],
- getFields: () => ['mock fields'],
+ getColumns: () => ['mock-columns'],
+ getFields: () => ['mock-fields'],📝 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.
| getColumns: () => ['mock Columns'], | |
| getFields: () => ['mock fields'], | |
| getColumns: () => ['mock-columns'], | |
| getFields: () => ['mock-fields'], |
DilwoarH
deleted the
482-dataset-details-page-provision-level-view-table
branch
What type of PR is this? (check all applicable)
Description
This adds a data table displaying all entities for a given lpa/dataset
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Preview Link: https://submit-pr-609.herokuapp.com/organisations/local-authority:LBH/article-4-direction-area/data/1
It could be worth testing the pagination, as well as several different datasets
Added/updated tests?
Summary by CodeRabbit
Release Notes
New Features
Improvements
task_countinstead ofissue_count.Documentation