add dataset action section#652
Conversation
WalkthroughThe pull request introduces significant modifications to the dataset configuration in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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: 7
🧹 Outside diff range and nitpick comments (6)
src/filters/getDatasetConfig.js (1)
1-1: Consider adding JSDoc type information for the config importAdding type documentation would improve maintainability and help catch potential configuration structure changes early.
+/** @typedef {import('../../types/config').Config} Config */ +/** @type {Config} */ import config from '../../config/index.js'test/unit/filters/getDatasetConfig.test.js (1)
4-14: Consider expanding test coverage with additional scenariosWhilst the current tests cover basic success and failure cases, consider adding tests for:
- Edge cases (empty string, null, undefined input)
- Different dataset types mentioned in the configuration
- Case sensitivity handling
This would provide more robust validation of the function's behaviour across various scenarios.
Example additions:
describe('getDatasetGuidanceUrl', () => { it('should return the correct guidance URL for a valid datasetId', () => { const result = getDatasetGuidanceUrl('article-4-direction') expect(result).toBe('/guidance/specifications/article-4-direction') }) it('should return undefined for an invalid datasetId', () => { const result = getDatasetGuidanceUrl('invalidDataset') expect(result).toBeUndefined() }) + + it('should handle edge cases gracefully', () => { + expect(getDatasetGuidanceUrl('')).toBeUndefined() + expect(getDatasetGuidanceUrl(null)).toBeUndefined() + expect(getDatasetGuidanceUrl(undefined)).toBeUndefined() + }) + + it('should be case sensitive for dataset IDs', () => { + expect(getDatasetGuidanceUrl('ARTICLE-4-DIRECTION')).toBeUndefined() + }) })config/default.yaml (1)
44-64: Consider improving URL maintainability.There are several opportunities to enhance maintainability:
- The guidance URLs are duplicated between
datasetsConfigandguidanceNavigation- Multiple datasets share the same guidance URL (e.g., tree-related datasets)
Consider introducing URL variables at the top of the config:
urls: guidance: base: /guidance/specifications article4: ${urls.guidance.base}/article-4-direction conservation: ${urls.guidance.base}/conservation-area tree: ${urls.guidance.base}/tree-preservation-order listed: ${urls.guidance.base}/listed-building brownfield: https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data datasetsConfig: article-4-direction: guidanceUrl: ${urls.guidance.article4} article-4-direction-area: guidanceUrl: ${urls.guidance.article4} # ... and so onThis would:
- Reduce duplication
- Make URL updates easier
- Ensure consistency across the config
src/views/organisations/dataset-overview.html (1)
182-189: Consider accessibility and UX improvementsWhilst the implementation is functional, consider these enhancements:
- Make links more descriptive for screen readers
- Add visual indication for external links
- Add a brief description below the heading
Apply this diff to improve accessibility and user experience:
<section> <h2 class="govuk-heading-m">Dataset actions</h2> + <p class="govuk-body">Tools and guidance to help you manage your dataset.</p> <ul class="govuk-list"> <li> - <a class="govuk-link" href="{{ organisation | checkToolDeepLink(dataset) }}">Check {{ dataset.name }} dataset</a> + <a class="govuk-link govuk-link--no-visited-state" href="{{ organisation | checkToolDeepLink(dataset) }}" aria-label="Use tool to check {{ dataset.name }} dataset">Check {{ dataset.name }} dataset <span class="govuk-visually-hidden">(opens in new tab)</span></a> </li> {% if dataset.dataset | getDatasetGuidanceUrl %} <li> - <a class="govuk-link" href="{{ dataset.dataset | getDatasetGuidanceUrl }}">{{ dataset.name }} guidance</a> + <a class="govuk-link govuk-link--no-visited-state" href="{{ dataset.dataset | getDatasetGuidanceUrl }}" aria-label="View guidance for {{ dataset.name }}" target="_blank" rel="noopener noreferrer">{{ dataset.name }} guidance <span class="govuk-visually-hidden">(opens in new tab)</span></a> </li> {% endif %} </ul> </section>test/unit/views/organisations/dataset-overview.test.js (2)
119-143: Well-structured test implementation, consider adding edge cases.The test case effectively validates the dataset actions section requirements. However, consider enhancing the test coverage with:
- Negative test cases (e.g., datasets without actions)
- Accessibility checks (e.g., ARIA attributes)
139-139: Consider making URL assertions more maintainable.The hardcoded URLs in the assertions could make the tests fragile. Consider extracting these as constants or using configuration values that can be easily updated if the URL structure changes.
Example refactor:
+ const CHECK_TOOL_URL_TEMPLATE = '/check/link?dataset={dataset}&orgName={orgName}' + const GUIDANCE_URL_TEMPLATE = '/guidance/specifications/{collection}' expect(links[0].querySelector('.govuk-link').href).toEqual( - '/check/link?dataset=article-4-direction-area&orgName=Mock%20org' + CHECK_TOOL_URL_TEMPLATE + .replace('{dataset}', 'article-4-direction-area') + .replace('{orgName}', 'Mock%20org') ) expect(links[1].querySelector('.govuk-link').href).toEqual( - '/guidance/specifications/article-4-direction' + GUIDANCE_URL_TEMPLATE.replace('{collection}', 'article-4-direction') )Also applies to: 142-142
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
config/default.yaml(1 hunks)src/filters/filters.js(2 hunks)src/filters/getDatasetConfig.js(1 hunks)src/middleware/overview.middleware.js(1 hunks)src/serverSetup/nunjucks.js(1 hunks)src/views/organisations/dataset-overview.html(1 hunks)test/unit/filters/getDatasetConfig.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
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.
🔇 Additional comments (8)
test/unit/filters/getDatasetConfig.test.js (2)
1-2: LGTM! Clean and clear imports
The imports are appropriately structured, importing specific testing utilities from Vitest and the function under test.
6-7: Verify the hardcoded URL path structure
Let's ensure the expected URL path /guidance/specifications/article-4-direction matches the structure defined in the configuration.
✅ Verification successful
Hardcoded URL path structure is verified
The URL /guidance/specifications/article-4-direction in the test matches the configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the URL path structure in configuration matches test expectations
# Search for the URL path structure in configuration files
rg -g '*.{yaml,yml}' --no-filename '/guidance/specifications/' -A 2
Length of output: 1736
src/filters/filters.js (1)
45-45: LGTM! Filter registration follows established patterns
The registration of getDatasetGuidanceUrl as a Nunjucks filter is implemented consistently with other filters in the file. This addition aligns well with the PR objective of providing guidance access to data providers.
config/default.yaml (2)
44-64: LGTM! The new dataset configuration structure looks good.
The transition from datasetsFilter to datasetsConfig provides a more organised way to manage dataset-specific configurations, aligning well with the PR's objective of adding a dataset actions section.
50-50: Verify the external brownfield-land guidance URL.
The brownfield-land dataset points to an external gov.uk URL, unlike other datasets that use internal guidance pages.
✅ Verification successful
External brownfield-land guidance URL is accessible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the accessibility of the external brownfield guidance URL
# Test: Check if the URL is accessible
curl -I "https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data"
Length of output: 3688
src/middleware/overview.middleware.js (2)
32-32: Similar validation needed for config.datasetsConfig
This line has the same validation issue as discussed in the previous comment.
24-32: Verify dataset configuration completeness
Let's ensure that all required datasets have proper configuration entries to support the new actions section.
src/views/organisations/dataset-overview.html (1)
178-192: Implementation aligns well with requirements
The new dataset actions section successfully implements the requirements from issue #617, providing data providers with easy access to both the check tool and relevant guidance.
| export const getDatasetGuidanceUrl = (datasetId) => { | ||
| return config?.datasetsConfig?.[datasetId]?.guidanceUrl | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve function documentation
The function would benefit from proper parameter validation and comprehensive documentation. Consider adding error handling for invalid dataset IDs to assist with debugging.
+/**
+ * Retrieves the guidance URL for a specific dataset
+ * @param {string} datasetId - The unique identifier for the dataset
+ * @returns {string|undefined} The guidance URL if found, undefined otherwise
+ * @throws {Error} If datasetId is not provided or is invalid
+ */
export const getDatasetGuidanceUrl = (datasetId) => {
+ if (!datasetId || typeof datasetId !== 'string') {
+ throw new Error('Dataset ID must be a non-empty string')
+ }
+
return config?.datasetsConfig?.[datasetId]?.guidanceUrl
}📝 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 getDatasetGuidanceUrl = (datasetId) => { | |
| return config?.datasetsConfig?.[datasetId]?.guidanceUrl | |
| } | |
| /** | |
| * Retrieves the guidance URL for a specific dataset | |
| * @param {string} datasetId - The unique identifier for the dataset | |
| * @returns {string|undefined} The guidance URL if found, undefined otherwise | |
| * @throws {Error} If datasetId is not provided or is invalid | |
| */ | |
| export const getDatasetGuidanceUrl = (datasetId) => { | |
| if (!datasetId || typeof datasetId !== 'string') { | |
| throw new Error('Dataset ID must be a non-empty string') | |
| } | |
| return config?.datasetsConfig?.[datasetId]?.guidanceUrl | |
| } |
DilwoarH
force-pushed
the
617-dataset-details-page-dataset-actions-section
branch
from
252e078 to
6488f84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
config/default.yaml (1)
50-50: Consider standardising guidance URL patternsThe brownfield-land dataset uses an absolute URL to gov.uk, while other datasets use relative paths. This inconsistency might impact maintenance and URL handling in the frontend.
Consider either:
- Using relative paths consistently and maintaining redirects for external resources
- Documenting why this specific dataset requires an external URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
config/default.yaml(1 hunks)src/filters/filters.js(2 hunks)src/filters/getDatasetConfig.js(1 hunks)src/middleware/overview.middleware.js(1 hunks)src/views/organisations/dataset-overview.html(1 hunks)test/unit/filters/getDatasetConfig.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/filters/filters.js
- src/filters/getDatasetConfig.js
- src/middleware/overview.middleware.js
- src/views/organisations/dataset-overview.html
- test/unit/filters/getDatasetConfig.test.js
- test/unit/views/organisations/dataset-overview.test.js
🔇 Additional comments (2)
config/default.yaml (2)
44-64: Well-structured dataset configuration!
The new datasetsConfig section is well-organised with consistent formatting and clear mapping between datasets and their guidance URLs. This structure aligns well with the PR objective of providing easy access to dataset-specific guidance.
50-50: Ensure secure URL handling
The external URL for brownfield-land guidance should enforce HTTPS. Additionally, consider adding URL validation to ensure all external URLs are secure.
#!/bin/bash
# Description: Check for insecure URLs in configuration
# Search for URLs not using HTTPS
echo "URLs not using HTTPS:"
yq e '.. | select(type == "string" and (. | test("^http[^s]")))' config/default.yaml
rosado
left a comment
rosado
left a comment
There was a problem hiding this comment.
Please update ConfigSchema in config/util.js.
Thanks will do, is there a reason why it doesn't cause the tests to fail? |
That schema is permissive (in contrast to our template schemas, which use |
DilwoarH
force-pushed
the
617-dataset-details-page-dataset-actions-section
branch
from
9a3e747 to
5f636a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
config/util.js (1)
56-65: Add constraints to prevent empty navigation itemsThe navigation structure should ensure that labels aren't empty strings and that arrays aren't empty.
Apply these improvements:
guidanceNavigation: v.object({ - title: v.string(), + title: NonEmptyString, items: v.array(v.object({ - label: v.string(), - url: v.string(), + label: NonEmptyString, + url: v.url(), items: v.array(v.object({ - label: v.string(), - url: v.string() + label: NonEmptyString, + url: v.url() })).optional() - })) + }), v.minLength(1)) })🧰 Tools
🪛 GitHub Check: test
[failure] 64-64: test/unit/PageController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/check-answers.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/checkAnswersController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/choose-datasetPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/chooseDatasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/configValidation.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ test/unit/configValidation.test.js:1:1
[failure] 64-64: test/unit/dataset-details.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetTaskListPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasette.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31config/default.yaml (2)
45-64: Consider using YAML anchors to reduce duplicationSeveral datasets share the same guidance URLs. Using YAML anchors would make the configuration more maintainable and reduce duplication.
Here's how you could refactor it:
datasetsConfig: + # Define anchors for shared URLs + x-urls: &article4Url + guidanceUrl: /guidance/specifications/article-4-direction + x-urls: &tpoUrl + guidanceUrl: /guidance/specifications/tree-preservation-order + x-urls: &listedBuildingUrl + guidanceUrl: /guidance/specifications/listed-building + article-4-direction: - guidanceUrl: /guidance/specifications/article-4-direction + <<: *article4Url article-4-direction-area: - guidanceUrl: /guidance/specifications/article-4-direction + <<: *article4Url # ... similar changes for other shared URLs
50-50: Maintain URL consistency across configurationsThe brownfield-land guidance URL uses an absolute URL while others use relative paths. Consider standardising the URL format for consistency.
brownfield-land: - guidanceUrl: https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data + guidanceUrl: /guidance/specifications/brownfield-land
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
config/default.yaml(1 hunks)config/util.js(1 hunks)src/filters/filters.js(2 hunks)src/filters/getDatasetConfig.js(1 hunks)src/middleware/overview.middleware.js(1 hunks)src/views/organisations/dataset-overview.html(1 hunks)test/unit/filters/getDatasetConfig.test.js(1 hunks)test/unit/views/organisations/dataset-overview.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/filters/filters.js
- src/filters/getDatasetConfig.js
- src/middleware/overview.middleware.js
- src/views/organisations/dataset-overview.html
- test/unit/filters/getDatasetConfig.test.js
- test/unit/views/organisations/dataset-overview.test.js
🧰 Additional context used
🪛 GitHub Check: test
config/util.js
[failure] 64-64: test/unit/PageController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/check-answers.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/checkAnswersController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/choose-datasetPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/chooseDatasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/configValidation.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ test/unit/configValidation.test.js:1:1
[failure] 64-64: test/unit/dataset-details.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetTaskListPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasette.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
🔇 Additional comments (2)
config/util.js (1)
53-65: Verify existing configuration files
Let's ensure all existing configuration files comply with the new schema additions.
✅ Verification successful
The initial results show that only default.yaml contains these sections, but we need to see their complete structure to verify against the schema. This script will help us see the full structure of both sections.
The awk command didn't show the complete structure. Let's try using grep with more context lines to see the full configuration sections.
Configuration files comply with the new schema
The existing configuration in default.yaml matches the schema requirements:
datasetsConfigcontains records with objects havingguidanceUrlas stringguidanceNavigationhas the required structure withtitle(string), anditemsarray containing objects withlabel,url, and optional nesteditems
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all yaml config files for compatibility with new schema
# Find and check all yaml config files
fd -e yaml -e yml . config/ --exec sh -c '
echo "Checking {}"
cat {} | grep -A 5 "datasetsConfig\|guidanceNavigation" || echo "No matching sections in {}"
'
Length of output: 1115
Script:
#!/bin/bash
# Let's check the complete structure of the guidanceNavigation section in default.yaml
echo "Full guidanceNavigation structure in default.yaml:"
cat config/default.yaml | awk '/^guidanceNavigation:/,/^[^ ]/'
echo -e "\nFull datasetsConfig structure in default.yaml:"
cat config/default.yaml | awk '/^datasetsConfig:/,/^[^ ]/'
Length of output: 381
Script:
#!/bin/bash
# Let's try a different approach using grep with context
echo "Checking guidanceNavigation structure:"
grep -A 20 "^guidanceNavigation:" config/default.yaml
echo -e "\nChecking datasetsConfig structure:"
grep -A 20 "^datasetsConfig:" config/default.yaml
Length of output: 2226
🧰 Tools
🪛 GitHub Check: test
[failure] 64-64: test/unit/PageController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/check-answers.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/checkAnswersController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/choose-datasetPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/chooseDatasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/configValidation.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ test/unit/configValidation.test.js:1:1
[failure] 64-64: test/unit/dataset-details.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetController.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasetTaskListPage.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
[failure] 64-64: test/unit/datasette.test.js
TypeError: array(...).optional is not a function
❯ config/util.js:64:11
❯ config/index.js:2:31
config/default.yaml (1)
44-44: Well-structured configuration section added
The new datasetsConfig section provides a clear and maintainable way to manage dataset-specific configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
config/util.js (1)
53-69: Consider extracting dataset identifiers to a separate constantThe hardcoded array of dataset identifiers could become difficult to maintain. Consider extracting these to a separate exported constant that can be reused elsewhere in the codebase.
+export const DATASET_TYPES = [ + 'article-4-direction', + 'article-4-direction-area', + 'brownfield-land', + 'conservation-area', + 'conservation-area-document', + 'tree-preservation-order', + 'tree-preservation-zone', + 'tree', + 'listed-building', + 'listed-building-outline' +] as const; datasetsConfig: v.object( - [ - 'article-4-direction', - 'article-4-direction-area', - // ... - ].reduce((acc, key) => { + DATASET_TYPES.reduce((acc, key) => {config/default.yaml (1)
44-64: Consider standardising URL formatsThe structure is well-organised, but there's inconsistency in URL formats:
- Most URLs use relative paths (e.g.,
/guidance/specifications/...)- Brownfield land uses an absolute URL
Consider standardising to relative paths for maintainability, making it easier to change the base URL if needed.
brownfield-land: - guidanceUrl: https://www.gov.uk/government/publications/brownfield-land-registers-data-standard/publish-your-brownfield-land-data + guidanceUrl: /guidance/specifications/brownfield-land
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
config/default.yaml(1 hunks)config/util.js(1 hunks)
🔇 Additional comments (4)
config/util.js (2)
66-66: Use v.url() for guidanceUrl validation
The guidanceUrl field should be validated to ensure it's a valid URL format.
55-64: Verify dataset coverage against requirements
Let's ensure all required datasets from issue #617 are included in the configuration.
✅ Verification successful
All required datasets are properly configured
Based on the comprehensive search results, all dataset types mentioned in the configuration file are consistently referenced and used throughout the codebase, including:
- article-4-direction and article-4-direction-area
- brownfield-land
- conservation-area and conservation-area-document
- tree-preservation-order and tree-preservation-zone
- tree
- listed-building and listed-building-outline
The dataset types are properly integrated across various components:
- Configuration files (config/default.yaml)
- Documentation (src/views/guidance/specifications/)
- Data schemas (src/routes/schemas.js)
- Test fixtures and performance tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all required datasets are covered in the configuration
# Get all dataset references from the codebase
echo "Finding dataset references in the codebase..."
rg -g '!node_modules' -g '!*.test.js' -g '!config/util.js' -o -w \
'article-4-direction|article-4-direction-area|brownfield-land|conservation-area|conservation-area-document|tree-preservation-order|tree-preservation-zone|tree|listed-building|listed-building-outline' \
. | sort -u
echo -e "\nChecking for any additional dataset types..."
# Look for potential dataset types we might have missed
rg -g '!node_modules' -g '!*.test.js' 'dataset.*type' -C 2
Length of output: 11880
config/default.yaml (2)
Line range hint 15-22: Plan deprecation of serviceName
The configuration indicates that serviceName is deprecated in favour of serviceNames. Consider:
- Setting a deprecation timeline
- Adding a migration guide
- Updating any remaining code that uses the deprecated property
#!/bin/bash
# Description: Find usage of deprecated serviceName in the codebase
rg -l 'serviceName' --type-add 'template:*.njk' --type templateLine range hint 13-13: Address the TODO comment
The comment suggests that the URL and name might need updating. Please verify if these updates are still needed and remove the TODO comment once addressed.
Hey - done, I also added a missing one |
4 participants
What type of PR is this? (check all applicable)
Description
Add dataset action section on dataset overview page
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
[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
Bug Fixes
Tests
getDatasetGuidanceUrlfunction and the dataset actions section rendering.