1180 display tasks in lpa dashboard - #1213
Merged
Merged
Conversation
2 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Platform API task fetching and rewires several middleware flows to use ChangesPlatform API task counts migration
Per-environment mainWebsiteUrl config
get-started ArcGIS help block UI update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
pooleycodes
marked this pull request as ready for review
June 23, 2026 16:01
![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: 1
🧹 Nitpick comments (4)
src/middleware/common.middleware.js (1)
1236-1244: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFail closed when organisation is unavailable.
On Line 1239, this can call
/task.jsonwithout anorganisationfilter whenreq.orgInfois missing, which risks surfacing cross-authority task data. Add an early guard before the API call.Proposed change
export const fetchTasksFromPlatformApi = async (req, res, next) => { const dataset = req.params.dataset + const organisation = req.orgInfo?.organisation + if (!organisation) { + req.tasks = { tasks: [], count: 0 } + return next() + } try { const { formattedData } = await platformApi.fetchTasks({ - organisation: req.orgInfo?.organisation, + organisation, ...(dataset && { dataset }), severity: 'error', task_source: 'issue', limit: dataset ? 100 : 500 })🤖 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/middleware/common.middleware.js` around lines 1236 - 1244, The platformApi.fetchTasks call on line 1239 can be executed without an organisation filter when req.orgInfo is missing or undefined, which risks exposing cross-authority task data. Add an early guard check before the try block to verify that req.orgInfo and req.orgInfo.organisation exist. If either is missing, the function should return early with an appropriate error response to fail closed and prevent unauthorised data exposure.test/unit/middleware/common.middleware.test.js (1)
1659-1734: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for missing
orgInfo.organisation.Please add a case that verifies no unscoped task fetch occurs and
req.tasksremains empty when organisation context is absent.Suggested test case
describe('fetchTasksFromPlatformApi', () => { const next = vi.fn() @@ it('falls back to empty tasks on API error', async () => { @@ expect(next).toHaveBeenCalledTimes(1) }) + + it('fails closed when organisation is missing', async () => { + const req = { orgInfo: {}, params: { dataset: 'brownfield-land' } } + await fetchTasksFromPlatformApi(req, {}, next) + + expect(platformApi.fetchTasks).not.toHaveBeenCalled() + expect(req.tasks).toEqual({ tasks: [], count: 0 }) + expect(next).toHaveBeenCalledTimes(1) + }) })🤖 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/middleware/common.middleware.test.js` around lines 1659 - 1734, Add a new test case in the fetchTasksFromPlatformApi test suite that verifies the behavior when orgInfo.organisation is missing or undefined. The test should create a request object where orgInfo lacks the organisation property, call fetchTasksFromPlatformApi, and assert that platformApi.fetchTasks is either not called or called without an organisation parameter, and that req.tasks is set to an empty object with an empty tasks array and count of 0 to ensure the function handles missing organisation context gracefully.test/unit/middleware/dataview.middleware.test.js (1)
181-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd branch coverage for the new
taskCountpaths.This suite currently validates only the zero/default case. Please add explicit cases for out-of-bounds incrementing and the
authority === 'some'override so regressions in the new logic are caught.Proposed test additions
describe('prepareTemplateParams', () => { + it('increments taskCount when out-of-bounds expectations exist', () => { + const req = { + orgInfo: { name: 'Mock Org', entity: 'mock-entity' }, + dataset: { name: 'Mock Dataset', dataset: 'mock-dataset' }, + tableParams: { columns: ['foo'], fields: ['foo'] }, + tasks: { tasks: [{ dataset: 'mock-dataset' }], count: 1 }, + expectationOutOfBounds: [{ dataset: 'mock-dataset' }], + pagination: {}, + dataRange: { minRow: 1, maxRow: 1, totalRows: 1 }, + authority: 'mock-authority' + } + const next = vi.fn() + prepareTemplateParams(req, {}, next) + expect(req.templateParams.taskCount).toBe(2) + expect(next).toHaveBeenCalledTimes(1) + }) + + it('forces taskCount to 1 when authority is some', () => { + const req = { + orgInfo: { name: 'Mock Org', entity: 'mock-entity' }, + dataset: { name: 'Mock Dataset', dataset: 'mock-dataset' }, + tableParams: { columns: ['foo'], fields: ['foo'] }, + tasks: { tasks: [{ dataset: 'mock-dataset' }], count: 5 }, + expectationOutOfBounds: [{ dataset: 'mock-dataset' }], + pagination: {}, + dataRange: { minRow: 1, maxRow: 1, totalRows: 1 }, + authority: 'some' + } + const next = vi.fn() + prepareTemplateParams(req, {}, next) + expect(req.templateParams.taskCount).toBe(1) + expect(next).toHaveBeenCalledTimes(1) + })🤖 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/middleware/dataview.middleware.test.js` around lines 181 - 219, The current test for prepareTemplateParams in the describe block only validates the default case where taskCount is 0. Add two additional test cases within the same describe block to cover the new taskCount logic: one test case where the tasks array contains items to verify taskCount increments correctly (out-of-bounds case), and another test case where authority is set to 'some' to verify the authority override behavior is properly applied to the templateParams object. Ensure both new test cases follow the same structure as the existing test, setting up appropriate req and res objects, calling prepareTemplateParams, and asserting the expected templateParams values.test/unit/middleware/datasetOverview.middleware.test.js (1)
27-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for non-displayable platform tasks.
Please add a case where
req.tasks.countis non-zero but task entries are missingdetails.issue_type/details.field, and assert the overview count matches displayable tasks. This will guard cross-page count consistency.🤖 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/middleware/datasetOverview.middleware.test.js` around lines 27 - 60, Add a new test case after the existing 'should prepare template params for dataset overview' test to verify regression handling of non-displayable platform tasks. Create a test scenario where req.tasks has a non-zero count property but the task entries are missing the required details.issue_type and details.field properties. Call prepareDatasetOverviewTemplateParams with this configuration and assert that the resulting reqWithResults.templateParams.taskCount reflects only the count of displayable tasks (those with proper details), not the total tasks.count value, to ensure cross-page count consistency.
🤖 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/middleware/datasetOverview.middleware.js`:
- Around line 222-224: The taskCount calculation in the
datasetOverview.middleware.js file at line 222 uses the raw tasks.count without
filtering, but the task-list rendering filters tasks to only include those with
both details.issue_type and details.field properties. Update the taskCount
calculation to apply the same filtering logic - instead of using tasks?.count
directly, filter the tasks array to count only those items that have both
details.issue_type and details.field defined before adding the other count
values (endpointErrorIssues and expectationOutOfBounds check).
---
Nitpick comments:
In `@src/middleware/common.middleware.js`:
- Around line 1236-1244: The platformApi.fetchTasks call on line 1239 can be
executed without an organisation filter when req.orgInfo is missing or
undefined, which risks exposing cross-authority task data. Add an early guard
check before the try block to verify that req.orgInfo and
req.orgInfo.organisation exist. If either is missing, the function should return
early with an appropriate error response to fail closed and prevent unauthorised
data exposure.
In `@test/unit/middleware/common.middleware.test.js`:
- Around line 1659-1734: Add a new test case in the fetchTasksFromPlatformApi
test suite that verifies the behavior when orgInfo.organisation is missing or
undefined. The test should create a request object where orgInfo lacks the
organisation property, call fetchTasksFromPlatformApi, and assert that
platformApi.fetchTasks is either not called or called without an organisation
parameter, and that req.tasks is set to an empty object with an empty tasks
array and count of 0 to ensure the function handles missing organisation context
gracefully.
In `@test/unit/middleware/datasetOverview.middleware.test.js`:
- Around line 27-60: Add a new test case after the existing 'should prepare
template params for dataset overview' test to verify regression handling of
non-displayable platform tasks. Create a test scenario where req.tasks has a
non-zero count property but the task entries are missing the required
details.issue_type and details.field properties. Call
prepareDatasetOverviewTemplateParams with this configuration and assert that the
resulting reqWithResults.templateParams.taskCount reflects only the count of
displayable tasks (those with proper details), not the total tasks.count value,
to ensure cross-page count consistency.
In `@test/unit/middleware/dataview.middleware.test.js`:
- Around line 181-219: The current test for prepareTemplateParams in the
describe block only validates the default case where taskCount is 0. Add two
additional test cases within the same describe block to cover the new taskCount
logic: one test case where the tasks array contains items to verify taskCount
increments correctly (out-of-bounds case), and another test case where authority
is set to 'some' to verify the authority override behavior is properly applied
to the templateParams object. Ensure both new test cases follow the same
structure as the existing test, setting up appropriate req and res objects,
calling prepareTemplateParams, and asserting the expected templateParams values.
🪄 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: 1da74ec0-acb2-4280-b002-8be9c44a0e7e
📒 Files selected for processing (16)
config/default.yamlconfig/development.yamlconfig/production.yamlconfig/staging.yamlsrc/middleware/common.middleware.jssrc/middleware/datasetOverview.middleware.jssrc/middleware/datasetTaskList.middleware.jssrc/middleware/dataview.middleware.jssrc/middleware/lpa-overview.middleware.jssrc/services/platformApi.jstest/unit/middleware/common.middleware.test.jstest/unit/middleware/datasetOverview.middleware.test.jstest/unit/middleware/datasetTaskList.middleware.test.jstest/unit/middleware/dataview.middleware.test.jstest/unit/middleware/lpa-overview.middleware.test.jstest/unit/services/platformApi.test.js
…com/digital-land/submit into 1180-display-tasks-in-lpa-dashboard
pooleycodes
had a problem deploying
to
development
June 24, 2026 12:32 — with 
GitHub Actions
Failure
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
June 24, 2026 13:39 — with
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/middleware/common.middleware.js`:
- Around line 805-811: Guard the entry issue query in the same place that builds
the SQL for fetchIssueCount so it does not dereference req.resources[0].resource
when no resource exists. Update the query function in common.middleware.js to
check req.resources[0] before constructing the WHERE clause, and return an
empty/no-op result path for datasets without a resource so the entry details
route stays safe. Use the existing fetchIssueCount guard pattern and the query
builder that references issueTypeClause and issueFieldClause to locate the fix.
In `@src/middleware/entryIssueDetails.middleware.js`:
- Around line 50-56: The count query in the middleware is missing the same
dataset and entry-row constraints used by fetchEntryIssues, so align the query
in the entry issue details logic with that method’s filtering. Update the count
builder to use the same resource, dataset, and entry row conditions as
fetchEntryIssues, keeping the query shape consistent so issueCount and
pagination match the actual rows returned.
🪄 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: 94df23dd-bf06-4b79-b233-388bbf99c8a9
📒 Files selected for processing (2)
src/middleware/common.middleware.jssrc/middleware/entryIssueDetails.middleware.js
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
The tasks in the LPA overview page, dataset details, data view and task list are all now sourced from the Platform API instead of Datasette.
As far as review; the pages should look identical except for the caveat that it can't differentiate individual tasks that are duplicated across different resources, so say if two endpoints for the same provision are provided, with some of the same data in it, no longer can the service count distinct entities in the issue log.
The fix for this; for the time being is to just take the endpoint with the larger number of tasks. An example of where this can lead to problem would be say Brighton and Hove where the task count is smaller then when you click through and it queries the issue logs directly.
Also two extra changes:
All issue loading now goes directly to the issue logs in the specific database, so instead of digital-land/issues table it would go to brownfield-land/issues table, the only loss here is the drop of the issue-type table, this means that a user could edit the url to looks at any issue type even if it is a internal issue. This is not considered dangerous and helps avoid the problem of very slow task loading.
The details drop down for arcgis URL has also been updated. (to be a dropdown).
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Go to as many task lists provision pages as possible and confirm they are identical.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
mainWebsiteUrlsettings for development, staging and production.Bug Fixes
Documentation