Arcgis Detail, Auth Data Only, Capitalization and No Dataset Spec Fallback#1152
Arcgis Detail, Auth Data Only, Capitalization and No Dataset Spec Fallback#1152pooleycodes merged 10 commits intomainfrom
Conversation
WalkthroughEnables provision-based datasets; swaps entity/entry issue-count fetching to a performance-DB-backed path; updates performanceDbApi to accept lpa and optional dataset; changes dataset slug-to-readable-name filter to accept an optional capitalize flag and updates many templates and tests to use slug-based readable names. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Middleware as Middleware Chain
participant PerfDB as Performance DB API
participant DB as Database
Client->>Middleware: Request dataset overview
Middleware->>Middleware: Execute middleware chain (includes onlyIf/fallback)
Middleware->>PerfDB: fetchEntityIssueCounts(lpa, dataset?)
PerfDB->>DB: Run simplified aggregation query (counts by field, issue_type, dataset)
DB-->>PerfDB: Return aggregated counts
PerfDB-->>Middleware: Return issue counts
Middleware->>Middleware: Set req.entityIssueCounts / req.entityCount
Middleware-->>Client: Render template using slug-to-readable-name filter and performance DB counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
… through only auth or some, work with no dataset but collection
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
changed the title
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
…task count from performancedb
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/organisations/dataset-overview.html (1)
235-235:⚠️ Potential issue | 🟡 MinorInconsistency: guidance link still uses
dataset.namewhile sibling links use the filter.Lines 228 and 231 were updated to use
dataset.dataset | datasetSlugToReadableName, but the guidance link on line 235 still reads{{ dataset.name }} guidance. This creates a mixed sourcing of display labels within the same<ul>, which could produce visually inconsistent results ifdataset.nameand the filter output diverge — or ifdataset.nameis unavailable in fallback scenarios (which this PR aims to handle).Suggested fix
- <a class="govuk-link" href="{{ dataset.dataset | getDatasetGuidanceUrl }}">{{ dataset.name }} guidance</a> + <a class="govuk-link" href="{{ dataset.dataset | getDatasetGuidanceUrl }}">{{ dataset.dataset | datasetSlugToReadableName(true) }} guidance</a>
🤖 Fix all issues with AI agents
In `@src/middleware/dataview.middleware.js`:
- Around line 84-87: The taskCount computation uses entityIssueCounts.length
without guarding for undefined and can throw; update the expression that defines
taskCount in dataview.middleware.js (the line assigning taskCount based on
authority) to mirror the safe pattern used in datasetOverview.middleware.js by
checking entityIssueCounts (e.g., use entityIssueCounts ?
entityIssueCounts.length : 0) so when req.entityIssueCounts is missing the code
returns 0 instead of throwing; keep the authority conditional (authority !==
'some' ? ...) but replace the right-hand side with the guarded expression.
In `@src/views/check/error-redirect.html`:
- Around line 58-62: In the ArcGIS error block (the {% elif errorMessage == "URL
must be the data layer" %} branch), close the mailto anchor and fix the
duplicated word: update the paragraph containing the email link (the <a
href="mailto:digitalland@communities.gov.uk"> element) to include a closing </a>
before </p>, and remove the extra "the" in "If you believe you have provided the
information correctly and the the problem persists" so it reads "...and the
problem persists"; the affected elements are the paragraphs with id="bad-upload"
and the mailto anchor.
In `@src/views/organisations/http-error.html`:
- Line 5: The breadcrumb and page title are inconsistent: the page title uses
the slug-based filter datasetSlugToReadableName applied to dataset.dataset
inside the pageName set, while the breadcrumb still renders dataset.name |
capitalize; update the breadcrumb rendering to use the same slug-to-readable
transformation (i.e., replace uses of dataset.name | capitalize with
dataset.dataset | datasetSlugToReadableName(true)) so both the page title and
breadcrumb display the same readable dataset name (ensure you update any
breadcrumb template code that references dataset.name).
In `@src/views/organisations/issueDetails.html`:
- Around line 10-12: Breadcrumb still uses dataset.name | capitalize while the
page title uses dataset.dataset | datasetSlugToReadableName(true); update the
breadcrumb in issueDetails.html (around the breadcrumb block at line 37) to use
the same filter call dataset.dataset | datasetSlugToReadableName(true) instead
of dataset.name | capitalize so the readable dataset name is consistent with the
page title.
In `@src/views/submit/dataset-details.html`:
- Line 30: The template currently applies the Nunjucks capitalize filter to
options.datasetName but the rest of the codebase uses
datasetSlugToReadableName(true) at render time; fix this for consistency by
removing the template capitalize filter and either (A) have the controller set
options.datasetName = datasetSlugToReadableName(dataset, true) before rendering,
or (B) pass the raw slug into the template and replace the template expression
with the datasetSlugToReadableName(true) filter (e.g., use options.dataset or
the slug variable and call datasetSlugToReadableName(true)); update the template
expression and controller assignment accordingly so capitalization is handled in
one consistent place.
In `@test/unit/http-errorPage.test.js`:
- Line 22: The test expectation for pageTitle uses the raw slug in
params.dataset.dataset but the template applies datasetSlugToReadableName(true)
which capitalises the first letter; update the assertion in
test/unit/http-errorPage.test.js so pageTitle uses the same transformed value
(i.e., combine params.organisation.name + " - " + the output of
datasetSlugToReadableName(true) for params.dataset.dataset + " - Task list -
Check and provide planning data") or otherwise assert the capitalised form to
match the template/filter behavior.
In `@test/unit/views/organisations/issueDetailsPage.test.js`:
- Line 25: Update the breadcrumb assertion in
test/unit/views/organisations/issueDetailsPage.test.js so it expects the dataset
display name used by the template: replace the use of params.dataset.dataset
with params.dataset.name (the same property the template renders via
dataset.name | capitalize) so the test matches
src/views/organisations/issueDetails.html's output.
🧹 Nitpick comments (9)
test/integration/authoritative_data.playwright.test.js (1)
17-17:.first()silences strict-mode but reduces specificity.Using
.first()works around Playwright's strict-mode error when multiple elements match'Data is not from an authoritative source'. This is a reasonable pragmatic fix, though be aware it will silently pass even if the first matching element is no longer the intended one (e.g. if page structure changes). A narrower locator scoped to a specific parent container would be more resilient, but not essential here.src/middleware/entityIssueDetails.middleware.js (1)
55-67: Subtle inconsistency betweenhtmlandoriginalValuetruthiness checks.Line 62 uses a truthy check (
html ? ...), so values like0orfalseproduce an empty string forhtml. Line 63 useshtml != null, so those same values would produce"0"or"false"fororiginalValue. This meanshtmlcould be""whileoriginalValueis"0"for a numeric-zero input.If this is intentional (preserving the original value for display/comparison while rendering empty HTML), it's fine — but worth a brief inline comment to prevent future confusion.
src/views/submit/lpa-details.html (1)
19-19: Inconsistent capitalisation approach compared to other templates.This line uses the Nunjucks
capitalizefilter onoptions.datasetName, whereas the rest of the PR consistently usesdatasetSlugToReadableName(true)on the dataset slug. Ifoptions.datasetNameis already a readable name,capitalizewill lowercase everything after the first character (e.g. "Tree Preservation Order" → "Tree preservation order"). Consider aligning with the pattern used elsewhere.Also note that line 27 renders
options.datasetNamewithout any capitalisation at all — this may be intentional for the back-link text, but worth confirming consistency.src/views/organisations/issueTable.html (1)
37-37: Breadcrumb still usesdataset.name | capitalizeinstead ofdataset.dataset | datasetSlugToReadableName(true).The pageName on lines 10/12 was updated to use the new filter, but the breadcrumb text on line 37 still uses the old
dataset.name | capitalizeapproach. This could produce mismatched naming between the breadcrumb and the page title. Consider aligning for consistency.♻️ Suggested fix
- text: dataset.name | capitalize, + text: dataset.dataset | datasetSlugToReadableName(true),test/unit/views/organisations/get-startedPage.test.js (1)
22-22: Organisation breadcrumb missinghrefin test expectation.The template at
get-started.htmlLine 27 renders the organisation breadcrumb withhref: '/organisations/' + organisation.organisation, but this test expectation omits thehreffor that item. IfrunGenericPageTestsvalidates hrefs, this could cause the test to miss a regression. Consider adding it for completeness:- breadcrumbs: [{ text: 'Home', href: '/' }, { text: 'Organisations', href: '/organisations' }, { text: params.organisation.name }, { text: 'Get started' }] + breadcrumbs: [{ text: 'Home', href: '/' }, { text: 'Organisations', href: '/organisations' }, { text: params.organisation.name, href: `/organisations/${params.organisation.organisation}` }, { text: 'Get started' }]src/views/organisations/dataset-overview.html (1)
207-207:aria-labelstill referencesdataset.nameinstead of the filter.The map
aria-labeluses{{ dataset.name }}directly. For consistency and to support the fallback scenario wheredataset.namemay be absent, consider switching to the filter here as well.Suggested fix
- <div id="map" class="app-map" role="region" aria-label="Map illustrating {{ dataset.name }} geometries. Use your keyboard to interact with the map. For screen reader users, use the arrow keys to navigate the map and the plus and minus keys to zoom in and out."></div> + <div id="map" class="app-map" role="region" aria-label="Map illustrating {{ dataset.dataset | datasetSlugToReadableName }} geometries. Use your keyboard to interact with the map. For screen reader users, use the arrow keys to navigate the map and the plus and minus keys to zoom in and out."></div>src/filters/makeDatasetSlugToReadableNameFilter.js (1)
17-17: Stale JSDoc:@throws {Error}is no longer accurate.The function no longer throws when a slug is not found in the mapping — it falls back to returning the transformed slug with a debug log. The
@throwsannotation should be removed to avoid misleading consumers.Suggested fix
* `@param` {boolean} [capitalize=false] - Whether to capitalize the first letter. * `@returns` {string} The readable name corresponding to the provided slug. - * `@throws` {Error} - If the provided slug is not found in the dataset name mapping. */src/middleware/datasetTaskList.middleware.js (1)
93-93: Stale JSDoc:req.entryIssueCountsis no longer used.Line 93 documents
req.entryIssueCountsas a parameter, but this property is no longer destructured or referenced inprepareTasks. Remove it to keep the documentation accurate.Suggested fix
* `@param` {Object} req.entityCount total entity count under `count` field * `@param` {Object[]} req.resources: An array of resource objects. * `@param` {Object[]} req.sources: An array of source objects. - * `@param` {Object} req.entryIssueCounts: An object containing the issue counts for the entries in the dataset. * `@param` {Object} req.entityIssueCounts: An object containing the issue counts for the entities in the dataset.src/middleware/common.middleware.js (1)
206-212: Log message on line 211 always fires, even whencountis undefined.When
countisundefined, the log message will read"Authoritative data found with count undefined, skipping non-authoritative check". This is harmless but slightly misleading. Consider moving it inside theifblock or adjusting the message.✏️ Suggestion — log conditionally or adjust wording
if (count !== undefined) { req.entityCount = { entity_count: count } } - logger.info(`Authoritative data found with count ${count}, skipping non-authoritative check`) + logger.info(`Authoritative data found${count !== undefined ? ` with count ${count}` : ''}, skipping non-authoritative check`)
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
2 participants
Description
Few things added to this:
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Updates