1150 get local plans data into provide tool#1159
Open
pooleycodes wants to merge 15 commits intomainfrom
Open
Conversation
…lanning group to see data
4 tasks
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds local planning‑group support: new dataset configs, Platform API pagination and organisation helpers, middleware to fetch planning groups and provisions, schema/view/template updates to surface planning‑group data, and tests for the planning‑group notice and organisation grouping. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as Middleware Pipeline
participant PlatformAPI as Platform API
participant Database as Database
participant Template as Template Engine
Client->>Middleware: Request (organisation, dataset)
Middleware->>PlatformAPI: fetchLocalPlanningGroups(params)
PlatformAPI-->>Middleware: planning group entities
Middleware->>Database: fetchProvisionsByOrgsAndDatasets(lpa + parentOrgs, dataset)
Database-->>Middleware: provision rows (with org names)
Middleware->>Middleware: compute planningGroupProvisions (exclude current LPA if multiple)
Middleware->>Template: render view with planningGroupProvisions, parentGroup, planningGroupMembers
Template->>Template: include _planning-group-notice when provisions present
Template-->>Client: HTML response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
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
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
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/middleware/lpa-overview.middleware.js (1)
271-287:⚠️ Potential issue | 🟠 MajorKeep
totalDatasetsaligned with the other overview counters.
prepareDatasetObjects()buildsdatasetsfrom the fullavailableDatasetslist, anddatasetsWithEndpoints/datasetsWithIssues/datasetsWithErrorsare still reduced over that full array. After this change,totalDatasetsdrops theotherbucket, so the overview can end up showing more datasets with endpoints or issues than the reported total. Either includeotherhere or calculate the other counters from the same filtered subset.♻️ Minimal fix if the summary is meant to cover every dataset
- const totalDatasets = (datasetsByReason.statutory?.length ?? 0) + (datasetsByReason.expected?.length ?? 0) + (datasetsByReason.prospective?.length ?? 0) + const totalDatasets = + (datasetsByReason.statutory?.length ?? 0) + + (datasetsByReason.expected?.length ?? 0) + + (datasetsByReason.prospective?.length ?? 0) + + (datasetsByReason.other?.length ?? 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/lpa-overview.middleware.js` around lines 271 - 287, totalDatasets currently sums only statutory/expected/prospective from datasetsByReason, causing it to mismatch the other counters (datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors) which were reduced over the full datasets array produced by prepareDatasetObjects / availableDatasets using orgStatsReducer; update totalDatasets to include the 'other' bucket (i.e. include datasetsByReason.other?.length) or instead compute totalDatasets from the same source as the other counters (e.g. use datasets.length or the orgStatsReducer result) so all overview counters are computed from the same filtered/complete set.src/middleware/common.middleware.js (1)
215-230:⚠️ Potential issue | 🟠 MajorRestore the non-authoritative record count.
After switching this branch to
platformApi.fetchAllEntities(),someResult?.data?.countis alwaysundefined, soreq.entityCountnever gets set forauthority === 'some'. The later fallback then counts unfiltered Datasette rows instead of the filtered Platform API result, which can skew pagination and the record summary on these pages.♻️ Suggested fix
if (someResult.formattedData && someResult.formattedData.length > 0) { req.authority = 'some' // Set list of alternate entities provided in req for later use const rows = someResult.formattedData || [] req.alternateEntityList = rows.map(({ entity }) => entity) - // Also set record count here if available - const someCount = someResult?.data?.count - if (someCount !== undefined) { - req.entityCount = { entity_count: someCount } - } + req.entityCount = { entity_count: rows.length } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/common.middleware.js` around lines 215 - 230, The branch that handles authority === 'some' now calls platformApi.fetchAllEntities, and someResult?.data?.count is undefined so req.entityCount never gets set; update the some-result handling in the someResult block (where platformApi.fetchAllEntities is called and req.alternateEntityList is computed) to restore the non-authoritative record count by falling back to a reliable source: if someResult?.data?.count is undefined, set req.entityCount = { entity_count: rows.length } (or use someResult?.count or someResult?.meta?.count if present) so pagination/summary use the filtered Platform API result; keep the existing assignments to req.authority and req.alternateEntityList.
🧹 Nitpick comments (5)
src/services/platformApi.js (2)
59-84: Incomplete JSDoc block and duplicated query construction.The
fetchDatasetsJSDoc block (lines 51-58) is left open, and a new JSDoc block forfetchAllEntitiesis started immediately after on line 59. This creates malformed documentation.Additionally, the query parameter construction logic (lines 70-75) duplicates what's in
fetchEntities(lines 30-36). Consider extracting a helper function to reduce duplication.♻️ Proposed fix for JSDoc and extraction suggestion
* `@throws` {Error} If the query fails or there is an error communicating with the Platform API */ + + /** + * Fetches all entities from the Platform API /entity.json endpoint, paginating through all results. + * Accepts the same params as fetchEntities (except limit/offset which are managed internally). + * + * `@param` {Object} params - Query parameters (same as fetchEntities) + * `@returns` {Promise<{formattedData: object[]}>} - All entities across all pages + */ - /** - * Fetches all entities from the Platform API /entity.json endpoint, paginating through all results. - * Accepts the same params as fetchEntities (except limit/offset which are managed internally). - */ fetchAllEntities: async (params) => {Consider extracting a helper to build query params:
function buildEntityQueryParams(params, limit, offset) { const queryParams = new URLSearchParams() if (params.organisation_entity) queryParams.append('organisation_entity', params.organisation_entity) if (params.dataset) queryParams.append('dataset', params.dataset) if (params.prefix) queryParams.append('prefix', params.prefix) if (params.organisation) queryParams.append('organisation', params.organisation) if (limit) queryParams.append('limit', limit) if (offset) queryParams.append('offset', offset) if (params.quality) queryParams.append('quality', params.quality) return queryParams }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/platformApi.js` around lines 59 - 84, The JSDoc for fetchDatasets is left unclosed and fetchAllEntities repeats query-building logic from fetchEntities; close/fix the JSDoc block for fetchDatasets and extract the duplicated URLSearchParams logic into a helper (e.g., buildEntityQueryParams) then use that helper from both fetchEntities and fetchAllEntities to append organisation_entity, dataset, prefix, organisation and limit/offset; ensure buildEntityQueryParams accepts params, limit, offset (and optional quality) and returns a URLSearchParams instance, then update fetchAllEntities to call the helper instead of duplicating the query construction.
68-81: Consider adding a safety limit to prevent infinite loops.The pagination logic is correct for normal API behaviour. However, if the Platform API has a bug returning
links.nextindefinitely, this could cause an infinite loop. Consider adding a maximum iteration safeguard.🛡️ Optional safeguard
fetchAllEntities: async (params) => { const pageSize = 100 let offset = 0 let allEntities = [] + const maxPages = 1000 // Safety limit - while (true) { + for (let page = 0; page < maxPages; page++) { const queryParams = new URLSearchParams() // ... existing code ... if (!data?.links?.next || entities.length < pageSize) break offset += pageSize } + + if (allEntities.length >= maxPages * pageSize) { + logger.warn({ message: 'fetchAllEntities reached maximum page limit', type: types.DataFetch, params }) + } return { formattedData: allEntities } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/platformApi.js` around lines 68 - 81, The while(true) pagination loop in src/services/platformApi.js that calls queryPlatformAPI should include a maximum iteration safeguard (e.g., MAX_PAGES) to avoid infinite loops if data?.links?.next is always present; modify the loop that builds queryParams and fetches entities so it increments a page counter each iteration and breaks (or throws/logs a clear error) when the counter exceeds MAX_PAGES, ensuring existing logic that checks data?.links?.next and entities.length < pageSize still runs; update any function signature or local variables if needed (e.g., add a configurable maxPages constant near pageSize/offset) and surface a clear error message mentioning queryPlatformAPI and the endpoint when the max is reached.src/views/organisations/overview.html (1)
181-209: Consider adding fallbacks for member/group names.In the
_planning-group-notice.htmlpartial, there's a fallback toprovision.organisationwhenprovision.nameis missing. However, lines 189 and 203 here usemember.nameandgroup.namedirectly without fallbacks. If the name property is missing, this could render empty link text.♻️ Proposed fix to add fallbacks
- <li><a class="govuk-link" href="/organisations/{{ member.organisation }}">{{ member.name }}</a></li> + <li><a class="govuk-link" href="/organisations/{{ member.organisation }}">{{ member.name or member.organisation }}</a></li>- <li><a class="govuk-link" href="/organisations/{{ group.organisation }}">{{ group.name }}</a></li> + <li><a class="govuk-link" href="/organisations/{{ group.organisation }}">{{ group.name or group.organisation }}</a></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/organisations/overview.html` around lines 181 - 209, The template renders member.name and group.name directly which can produce empty link text; update the two link texts in the planningGroupMembers and parentGroup loops to use a fallback (e.g., show member.name falling back to member.organisation, and group.name falling back to group.organisation) using your template engine’s default/or operator or filter so the <a> text is never empty (these are the <li> entries inside the planningGroupMembers loop and the parentGroup loop in the planning group block—match the fallback style used in _planning-group-notice.html).src/middleware/common.middleware.js (1)
1193-1227: Consider caching the planning-group metadata.This middleware now sits on several page paths, and each request performs a full
local-planning-groupscan, plus an organisation lookup when the current org is itself a planning group. A short TTL cache for the group list and org-name map would trim latency and make the new banners less dependent on the Platform API on every page view.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/middleware/common.middleware.js` around lines 1193 - 1227, The middleware fetchLocalPlanningGroups currently queries platformApi.fetchAllEntities and platformApi.fetchOrganisations on every request causing unnecessary latency; add a short TTL in-memory cache for the fetched local-planning-group list and the organisation name map so subsequent calls reuse cached data until expiry. Implement a module-level cache object (e.g., cachedGroups, cachedOrgNameMap, cachedAt, ttlMs) used by fetchLocalPlanningGroups: check cache validity before calling platformApi.fetchAllEntities/fetchOrganisations, populate the caches and cachedAt after a successful fetch, and fall back to existing behavior on cache miss or error; ensure the try/catch/error path still sets req.parentGroup and req.planningGroupMembers and does not throw. Reference symbols: fetchLocalPlanningGroups, platformApi.fetchAllEntities, platformApi.fetchOrganisations, req.parentGroup, req.planningGroupMembers.src/routes/schemas.js (1)
120-126: Consider makingplanningGroupProvisionsconsistently an array (not optional).Using
v.optional(v.array(...))means templates and callers must always handleundefined. If this field is part of the standard page contract, make it required as an array (empty when no data).Suggested refactor
-const PlanningGroupProvisionsField = v.optional(v.array(v.strictObject({ +const PlanningGroupProvisionsField = v.array(v.strictObject({ organisation: NonEmptyString, name: v.nullable(v.string()), dataset: NonEmptyString, project: v.nullable(v.string()), provision_reason: v.nullable(v.string()) -}))) +}))Also applies to: 175-175, 202-202, 221-221, 242-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/schemas.js` around lines 120 - 126, The schema currently defines PlanningGroupProvisionsField as optional (v.optional(v.array(...))) which forces callers to handle undefined; change it to a required array by replacing v.optional(v.array(...)) with v.array(...) so the field is always an array (empty when no items). Update the same pattern wherever planningGroupProvisions is defined (the other occurrences referenced in the review) so all corresponding schema constants use v.array(...) instead of v.optional(v.array(...)) to enforce a consistent required-array contract; keep the inner item shape (organisation, name, dataset, project, provision_reason) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/middleware/common.middleware.js`:
- Around line 1239-1250: The query in fetchProvisionsByOrgsAndDatasets builds
SQL by interpolating params.lpa, params.dataset and values from req.parentGroup
directly, creating an SQL injection risk; fix it by converting this into a
parameterised query (use prepared statement placeholders and pass orgs and
dataset as parameters) or, if parameterised SQL isn't supported here, properly
escape/quote each value (including those from req.parentGroup) using a safe
whitelist/quoting helper and avoid string concatenation; update the
query-building logic in fetchProvisionsByOrgsAndDatasets to use the safe
parameters/escaped values and ensure the IN clause is constructed from bound
parameters rather than raw interpolation.
In `@src/middleware/getStarted.middleware.js`:
- Around line 6-14: The planning-group notice is being suppressed by the
premature `provisions?.length > 1` guard; change the logic to first filter
provisions (use the existing variable planningGroupProvisions =
provisions?.filter(...) or similar) and then check the filtered array length to
decide whether to set planningGroupProvisions (i.e., set planningGroupProvisions
to the filtered array if filtered.length > 0, otherwise undefined). Update this
fix in the planningGroupProvisions builders found in getStarted.middleware.js
(planningGroupProvisions), dataview.middleware.js,
datasetOverview.middleware.js, and datasetTaskList.middleware.js so all use
filter-first-then-check semantics.
In `@src/middleware/organisations.middleware.js`:
- Around line 74-81: The middleware changed the template param key from
alphabetisedOrgs to orgsByDataset causing unit tests to fail; update
req.templateParams in the organisations middleware to use the original key
expected by tests (set req.templateParams = { alphabetisedOrgs: orgsByDataset }
or rename the variable) so tests referencing alphabetisedOrgs (and the strict
equality checks) pass, keeping the grouping logic in the reduce over
sortedResults intact (reference: orgsByDataset, sortedResults,
req.templateParams).
In `@src/routes/schemas.js`:
- Around line 156-160: parentGroup.entity is defined as NonEmptyString but
OrgField.entity is numeric and the platform returns numeric organisation-entity
IDs (they’re coerced with String() elsewhere), causing validation failures;
update both occurrences of parentGroup (the
v.optional(v.nullable(v.array(v.strictObject({...}))) ) definitions) to use the
same numeric type as OrgField.entity (i.e., change the entity field to the
numeric schema used by OrgField) so that parentGroup.entity accepts numeric IDs
from the middleware/API.
---
Outside diff comments:
In `@src/middleware/common.middleware.js`:
- Around line 215-230: The branch that handles authority === 'some' now calls
platformApi.fetchAllEntities, and someResult?.data?.count is undefined so
req.entityCount never gets set; update the some-result handling in the
someResult block (where platformApi.fetchAllEntities is called and
req.alternateEntityList is computed) to restore the non-authoritative record
count by falling back to a reliable source: if someResult?.data?.count is
undefined, set req.entityCount = { entity_count: rows.length } (or use
someResult?.count or someResult?.meta?.count if present) so pagination/summary
use the filtered Platform API result; keep the existing assignments to
req.authority and req.alternateEntityList.
In `@src/middleware/lpa-overview.middleware.js`:
- Around line 271-287: totalDatasets currently sums only
statutory/expected/prospective from datasetsByReason, causing it to mismatch the
other counters (datasetsWithEndpoints, datasetsWithIssues, datasetsWithErrors)
which were reduced over the full datasets array produced by
prepareDatasetObjects / availableDatasets using orgStatsReducer; update
totalDatasets to include the 'other' bucket (i.e. include
datasetsByReason.other?.length) or instead compute totalDatasets from the same
source as the other counters (e.g. use datasets.length or the orgStatsReducer
result) so all overview counters are computed from the same filtered/complete
set.
---
Nitpick comments:
In `@src/middleware/common.middleware.js`:
- Around line 1193-1227: The middleware fetchLocalPlanningGroups currently
queries platformApi.fetchAllEntities and platformApi.fetchOrganisations on every
request causing unnecessary latency; add a short TTL in-memory cache for the
fetched local-planning-group list and the organisation name map so subsequent
calls reuse cached data until expiry. Implement a module-level cache object
(e.g., cachedGroups, cachedOrgNameMap, cachedAt, ttlMs) used by
fetchLocalPlanningGroups: check cache validity before calling
platformApi.fetchAllEntities/fetchOrganisations, populate the caches and
cachedAt after a successful fetch, and fall back to existing behavior on cache
miss or error; ensure the try/catch/error path still sets req.parentGroup and
req.planningGroupMembers and does not throw. Reference symbols:
fetchLocalPlanningGroups, platformApi.fetchAllEntities,
platformApi.fetchOrganisations, req.parentGroup, req.planningGroupMembers.
In `@src/routes/schemas.js`:
- Around line 120-126: The schema currently defines PlanningGroupProvisionsField
as optional (v.optional(v.array(...))) which forces callers to handle undefined;
change it to a required array by replacing v.optional(v.array(...)) with
v.array(...) so the field is always an array (empty when no items). Update the
same pattern wherever planningGroupProvisions is defined (the other occurrences
referenced in the review) so all corresponding schema constants use v.array(...)
instead of v.optional(v.array(...)) to enforce a consistent required-array
contract; keep the inner item shape (organisation, name, dataset, project,
provision_reason) unchanged.
In `@src/services/platformApi.js`:
- Around line 59-84: The JSDoc for fetchDatasets is left unclosed and
fetchAllEntities repeats query-building logic from fetchEntities; close/fix the
JSDoc block for fetchDatasets and extract the duplicated URLSearchParams logic
into a helper (e.g., buildEntityQueryParams) then use that helper from both
fetchEntities and fetchAllEntities to append organisation_entity, dataset,
prefix, organisation and limit/offset; ensure buildEntityQueryParams accepts
params, limit, offset (and optional quality) and returns a URLSearchParams
instance, then update fetchAllEntities to call the helper instead of duplicating
the query construction.
- Around line 68-81: The while(true) pagination loop in
src/services/platformApi.js that calls queryPlatformAPI should include a maximum
iteration safeguard (e.g., MAX_PAGES) to avoid infinite loops if
data?.links?.next is always present; modify the loop that builds queryParams and
fetches entities so it increments a page counter each iteration and breaks (or
throws/logs a clear error) when the counter exceeds MAX_PAGES, ensuring existing
logic that checks data?.links?.next and entities.length < pageSize still runs;
update any function signature or local variables if needed (e.g., add a
configurable maxPages constant near pageSize/offset) and surface a clear error
message mentioning queryPlatformAPI and the endpoint when the max is reached.
In `@src/views/organisations/overview.html`:
- Around line 181-209: The template renders member.name and group.name directly
which can produce empty link text; update the two link texts in the
planningGroupMembers and parentGroup loops to use a fallback (e.g., show
member.name falling back to member.organisation, and group.name falling back to
group.organisation) using your template engine’s default/or operator or filter
so the <a> text is never empty (these are the <li> entries inside the
planningGroupMembers loop and the parentGroup loop in the planning group
block—match the fallback style used in _planning-group-notice.html).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fe0d727-13ea-47d7-be28-fccc06489ab6
📒 Files selected for processing (18)
config/default.yamlsrc/assets/scss/index.scsssrc/middleware/common.middleware.jssrc/middleware/datasetOverview.middleware.jssrc/middleware/datasetTaskList.middleware.jssrc/middleware/dataview.middleware.jssrc/middleware/getStarted.middleware.jssrc/middleware/lpa-overview.middleware.jssrc/middleware/organisations.middleware.jssrc/routes/schemas.jssrc/services/platformApi.jssrc/views/includes/_planning-group-notice.htmlsrc/views/organisations/dataset-overview.htmlsrc/views/organisations/datasetTaskList.htmlsrc/views/organisations/dataview.htmlsrc/views/organisations/find.htmlsrc/views/organisations/get-started.htmlsrc/views/organisations/overview.html
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/integration/planning_group.playwright.test.js (1)
27-27: Tighten the WireMock URL regex.Line 27 uses
entity.jsonwith an unescaped dot, so it matches more than the literal path and can mask unrelated requests.Suggested fix
- urlPattern: '/entity.json.*prefix=local-planning-group.*' + urlPattern: '/entity\\.json.*prefix=local-planning-group.*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/planning_group.playwright.test.js` at line 27, The WireMock urlPattern currently uses an unescaped dot in the string assigned to urlPattern (the '/entity.json.*prefix=local-planning-group.*' pattern), which allows unintended matches; update the urlPattern to escape the literal dot (use 'entity\\.json') and tighten the regex (e.g., anchor the query separator or start of pattern) so it matches the exact path and expected query parameter structure; modify the urlPattern assignment accordingly to use the corrected regex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/planning_group.playwright.test.js`:
- Around line 17-19: Tests in this file call resetWiremock() inside
test.beforeEach which races with other concurrently running suites
(fullyParallel: true) and can clear stubs from other workers; to fix, make the
suite run serially by adding a describe block and calling
test.describe.configure({ mode: 'serial' }) (or otherwise ensure this file is
executed serially) so resetWiremock() (and any test.beforeEach) cannot interfere
with other concurrent tests; locate test.beforeEach and replace/enclose tests in
a test.describe with test.describe.configure({ mode: 'serial' }) to enforce
serial execution.
- Around line 3-15: The WireMock admin helper functions resetWiremock and
addWiremockStub don't validate HTTP responses; modify both to capture the fetch
response, check response.ok, and throw a clear error if not ok (including status
and response text) so failures in POSTing to /__admin/mappings or
/__admin/mappings/reset are detected immediately and tests fail fast.
---
Nitpick comments:
In `@test/integration/planning_group.playwright.test.js`:
- Line 27: The WireMock urlPattern currently uses an unescaped dot in the string
assigned to urlPattern (the '/entity.json.*prefix=local-planning-group.*'
pattern), which allows unintended matches; update the urlPattern to escape the
literal dot (use 'entity\\.json') and tighten the regex (e.g., anchor the query
separator or start of pattern) so it matches the exact path and expected query
parameter structure; modify the urlPattern assignment accordingly to use the
corrected regex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4663f015-0184-4cf6-ab30-dbf42bdba373
📒 Files selected for processing (5)
src/views/organisations/find.htmltest/integration/planning_group.playwright.test.jstest/unit/findPage.test.jstest/unit/middleware/common.middleware.test.jstest/unit/middleware/organisations.middleware.test.js
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/routes/schemas.js (2)
118-118: TightenOrgField.datasettoNonEmptyString.Now that
OrgFindPageis keyed by dataset, allowingdataset: ''insideOrgFieldis looser than bothDatasetNameField.datasetand the neworgsByDatasetrecord key. ReusingNonEmptyStringhere keeps the dataset contract consistent.♻️ Proposed change
-const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()), dataset: v.optional(v.string()) }) +const OrgField = v.strictObject({ name: NonEmptyString, organisation: NonEmptyString, statistical_geography: v.optional(v.string()), entity: v.optional(v.integer()), dataset: v.optional(NonEmptyString) })Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/schemas.js` at line 118, OrgField currently allows dataset: v.optional(v.string()) which is too permissive; change the dataset schema in the OrgField definition to use NonEmptyString (e.g., dataset: v.optional(NonEmptyString)) so it matches DatasetNameField.dataset and the orgsByDataset record key; update the OrgField declaration (and the other occurrence at the second instance mentioned) to replace v.optional(v.string()) with v.optional(NonEmptyString).
155-164: Extract the repeated planning-group object schemas.These additions make sense, but
parentGroupis duplicated verbatim across both overview schemas, andplanningGroupMembersis another inline strict object. Pulling them into shared constants would make future field changes less error-prone.♻️ Proposed refactor
+const ParentGroupField = v.optional(v.nullable(v.array(v.strictObject({ + entity: v.integer(), + name: NonEmptyString, + organisation: NonEmptyString +})))) + +const PlanningGroupMembersField = v.optional(v.nullable(v.array(v.strictObject({ + organisation: NonEmptyString, + name: NonEmptyString +})))) + export const OrgOverviewPage = v.strictObject({ organisation: OrgField, datasets: v.object({ @@ datasetsWithErrors: v.integer(), isODPMember: v.boolean(), - parentGroup: v.optional(v.nullable(v.array(v.strictObject({ - entity: v.integer(), - name: NonEmptyString, - organisation: NonEmptyString - })))), - planningGroupMembers: v.optional(v.nullable(v.array(v.strictObject({ - organisation: NonEmptyString, - name: NonEmptyString - })))) + parentGroup: ParentGroupField, + planningGroupMembers: PlanningGroupMembersField }) @@ }), planningGroupProvisions: PlanningGroupProvisionsField, - parentGroup: v.optional(v.nullable(v.array(v.strictObject({ - entity: v.integer(), - name: NonEmptyString, - organisation: NonEmptyString - })))), + parentGroup: ParentGroupField, notice: v.optional(DeadlineNoticeField) })Also applies to: 203-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/schemas.js` around lines 155 - 164, The parentGroup and planningGroupMembers object schemas are duplicated inline; extract them into shared schema constants (e.g., ParentGroupSchema and PlanningGroupMemberSchema) defined once and reuse them where parentGroup and planningGroupMembers are declared: ParentGroupSchema should be v.strictObject({ entity: v.integer(), name: NonEmptyString, organisation: NonEmptyString }) and PlanningGroupMemberSchema should be v.strictObject({ organisation: NonEmptyString, name: NonEmptyString }); then replace the inline v.array(v.strictObject(...)) usages with v.array(ParentGroupSchema) and v.array(PlanningGroupMemberSchema) wrapped in the same v.optional(v.nullable(...)) as before so both overview schemas reference the single shared definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routes/schemas.js`:
- Line 118: OrgField currently allows dataset: v.optional(v.string()) which is
too permissive; change the dataset schema in the OrgField definition to use
NonEmptyString (e.g., dataset: v.optional(NonEmptyString)) so it matches
DatasetNameField.dataset and the orgsByDataset record key; update the OrgField
declaration (and the other occurrence at the second instance mentioned) to
replace v.optional(v.string()) with v.optional(NonEmptyString).
- Around line 155-164: The parentGroup and planningGroupMembers object schemas
are duplicated inline; extract them into shared schema constants (e.g.,
ParentGroupSchema and PlanningGroupMemberSchema) defined once and reuse them
where parentGroup and planningGroupMembers are declared: ParentGroupSchema
should be v.strictObject({ entity: v.integer(), name: NonEmptyString,
organisation: NonEmptyString }) and PlanningGroupMemberSchema should be
v.strictObject({ organisation: NonEmptyString, name: NonEmptyString }); then
replace the inline v.array(v.strictObject(...)) usages with
v.array(ParentGroupSchema) and v.array(PlanningGroupMemberSchema) wrapped in the
same v.optional(v.nullable(...)) as before so both overview schemas reference
the single shared definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bded4493-b1a4-4992-a78e-d704648bf16e
📒 Files selected for processing (2)
src/routes/schemas.jssrc/services/platformApi.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/platformApi.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
1 participant
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.
🚨 When merging and pushing to production, make sure redis is cleared 🚨
Description
Extension to show banners for joint planning groups.
Local plans / planning group banners
_planning-group-notice.htmlpartial — banner for LPAs in a joint planning groupoverview.htmlextended to show organisations breakdown and LPA membersConfig
config/default.yaml— addeddevelopment-plan-documentdataset config and hard-coded local plan datasets for production rolloutAPI / middleware
platformApi.js— new calls to fetch local plan / planning group datacommon.middleware.js— major additions to fetch and attach planning group context to requestsschemas.js— expanded validation schemasWhat type of PR is this? (check all applicable)
Related Tickets & Documents
Tests
Summary by CodeRabbit
New Features
Style
Chores
Tests