Efficient fetch entity issue counts - #1129
Conversation
…cient-fetchEntityIssueCounts
GitHub Actions
Error
WalkthroughThis PR replaces the dataset-error status fetch with a performance-DB-based entity issue counts fetch, restructures the LPA overview middleware into parallel execution groups, introduces dataset categories "expected" and "prospective" (alongside "statutory"), updates schemas, views and tests to use the new categories, and adds a performance service method and a Playwright UI flag in package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as LPA Overview Middleware
participant DB as Performance DB
rect rgb(200,220,240)
Note over Middleware: Parallel Group 1
par fetchOrgInfo
Middleware->>DB: fetch org info
and fetchResources
Middleware->>DB: fetch resources
and fetchEndpointSummary
Middleware->>DB: fetch endpoint summary
and fetchEntityIssueCountsPerformanceDb
Middleware->>DB: fetch entity issue counts
and fetchProvisions
Middleware->>DB: fetch all provisions
end
end
rect rgb(220,240,200)
Note over Middleware: Parallel Group 2 (depends on Group 1)
par fetchEntryIssueCounts
Middleware->>DB: fetch entry issue counts (uses resources)
and fetchEntityCounts
Middleware->>DB: fetch entity counts (uses org info)
end
end
rect rgb(240,220,200)
Note over Middleware: Template preparation
Middleware->>Middleware: prepareOverviewTemplateParams
Note over Middleware: Categorise datasets as statutory, expected, prospective
end
Middleware-->>Client: render overview with new dataset categories
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/middleware/common.middleware.js (1)
645-646: Swap console.log for structured logging
We should not print SQL straight to stdout in production middleware; it bypasses our logging framework and makes log parsing noisy. Please route this throughlogger.debug(or drop it entirely) so we stay consistent with the rest of the service logging.- console.log('sqlQuery', sqlQuery) + logger.debug('fetchEntityIssueCounts query', { type: types.App, sqlQuery })src/views/organisations/overview.html (3)
116-116: Pluralisation approach is inconsistent across status items.Lines 116 and 121 use inline ternary expressions for pluralisation, whilst line 126 uses the
pluralisefilter. Both are functional, but using the same approach consistently would improve maintainability.Consider applying the
pluralisefilter uniformly to all three status items:- authoritative dataset{{ "s" if (datasetsWithEndpoints != 1) }} provided + authoritative {{ "dataset" | pluralise(datasetsWithEndpoints) }} provided- error{{ "s" if (datasetsWithErrors != 1) }} accessing URLs + {{ "error" | pluralise(datasetsWithErrors) }} accessing URLsAlso applies to: 121-121, 126-126
147-157: Structure is sound with clear conditional logic.The datasetsExpected section appropriately guards visibility behind both
datasets.expected.length > 0andisODPMember, ensuring it only displays when relevant. The data-testid is correctly set for testing.Minor: fix spacing in the template variable on line 149:
- <h2 class="govuk-heading-m">Datasets {{ organisation.name}} are expected to provide</h2> + <h2 class="govuk-heading-m">Datasets {{ organisation.name }} are expected to provide</h2>
159-169: Structure is sound; prospective section appropriately unrestricted.The datasetsProspective section correctly shows only when prospective datasets exist, without ODP membership restriction. The data-testid is correctly set.
Minor: fix spacing in the template variable on line 161 (matching datasetsExpected):
- <h2 class="govuk-heading-m">Datasets {{ organisation.name}} can provide</h2> + <h2 class="govuk-heading-m">Datasets {{ organisation.name }} can provide</h2>Also note that line 163 omits the
govuk-!-margin-bottom-8class present on the statutory and expected sections. If this is intentional (prospective is the final section), that's fine; otherwise, consider adding it for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
package.json(1 hunks)src/middleware/common.middleware.js(2 hunks)src/middleware/lpa-overview.middleware.js(3 hunks)src/routes/schemas.js(1 hunks)src/services/performanceDbApi.js(1 hunks)src/views/organisations/overview.html(4 hunks)test/acceptance/dataset_overview.test.js(2 hunks)test/unit/middleware/lpa-overview.middleware.test.js(3 hunks)test/unit/views/organisations/lpaOverviewPage.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-14T17:31:37.924Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: src/services/performanceDbApi.js:290-293
Timestamp: 2024-11-14T17:31:37.924Z
Learning: In the `getEntityCounts` function in `src/services/performanceDbApi.js`, errors are already logged through the `.catch(...)` handlers on each promise in the `requests` array.
Applied to files:
src/services/performanceDbApi.jssrc/middleware/lpa-overview.middleware.jssrc/middleware/common.middleware.js
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
src/services/performanceDbApi.jssrc/middleware/lpa-overview.middleware.jstest/unit/middleware/lpa-overview.middleware.test.jssrc/middleware/common.middleware.js
📚 Learning: 2024-10-21T08:57:17.338Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 510
File: test/unit/middleware/datasetOverview.middleware.test.js:1-3
Timestamp: 2024-10-21T08:57:17.338Z
Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.
Applied to files:
test/acceptance/dataset_overview.test.jstest/unit/middleware/lpa-overview.middleware.test.jstest/unit/views/organisations/lpaOverviewPage.test.js
📚 Learning: 2024-11-12T10:54:09.485Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 609
File: test/unit/noErrorsPage.test.js:68-68
Timestamp: 2024-11-12T10:54:09.485Z
Learning: In `test/unit/noErrorsPage.test.js`, avoid using `prettifyColumnName` for column headers because the table can contain spec fields, as requested by Alex.
Applied to files:
test/unit/views/organisations/lpaOverviewPage.test.js
📚 Learning: 2024-11-28T16:29:30.209Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 691
File: src/middleware/dataview.middleware.js:24-24
Timestamp: 2024-11-28T16:29:30.209Z
Learning: In `src/middleware/dataview.middleware.js`, when querying the database, interpolating variables directly into SQL queries is acceptable as the variables are sourced from the database.
Applied to files:
src/middleware/common.middleware.js
🧬 Code graph analysis (2)
src/middleware/lpa-overview.middleware.js (2)
src/middleware/middleware.builders.js (4)
fetchMany(297-299)FetchOptions(32-41)FetchOptions(32-41)parallel(418-420)src/middleware/common.middleware.js (8)
fetchOrgInfo(78-83)fetchOrgInfo(78-83)fetchResources(160-178)fetchResources(160-178)fetchEndpointSummary(822-835)fetchEndpointSummary(822-835)fetchEntryIssueCounts(662-683)fetchEntryIssueCounts(662-683)
test/unit/middleware/lpa-overview.middleware.test.js (1)
src/middleware/lpa-overview.middleware.js (7)
prepareOverviewTemplateParams(256-310)req(75-75)req(124-124)req(149-149)req(207-207)req(322-322)req(338-338)
🔇 Additional comments (3)
src/views/organisations/overview.html (3)
94-104: New deadline notice loops follow established pattern.The loops for datasets.expected and datasets.prospective mirror the existing datasets.statutory structure and correctly check for dataset.notice before rendering. This maintains consistency across the template.
However, verify that all middleware/route handlers passing context to this template define the datasets.expected and datasets.prospective properties, even when empty, to prevent Nunjucks from treating them as undefined.
138-138: Explanatory text provides helpful context.The new paragraph clearly communicates the statutory requirement and value proposition to users. Class reuse maintains design consistency.
94-104: Context variables are properly defined and handled across all rendering paths.The verification confirms that
prepareOverviewTemplateParamsinsrc/middleware/lpa-overview.middleware.jscorrectly manages these context variables:
datasets.expectedanddatasets.prospectiveare created via_.groupBy()and only included when matching datasets exist (provision_reason-based grouping), which is intentional design—the template safely handles undefined properties via Nunjucks iterationisODPMemberis always explicitly set as a boolean- All rendering paths (test and production) route through
prepareOverviewTemplateParams, ensuring consistent context preparation- No alternative rendering paths exist that bypass this middleware
The implementation is correct and the template sections will display/hide based on the presence of actual data.
| SELECT | ||
| dataset, | ||
| issue_type, | ||
| field, | ||
| COUNT(*) AS count | ||
| FROM endpoint_dataset_issue_type_summary | ||
| WHERE organisation = '${datasetId}' | ||
| AND severity = 'error' | ||
| AND responsibility = 'external' | ||
| AND (resource_end_date = '' OR resource_end_date IS NULL) | ||
| GROUP BY dataset, issue_type, field` |
There was a problem hiding this comment.
Use the precomputed count_issues values
endpoint_dataset_issue_type_summary already stores the actual issue totals in count_issues (see lpaOverviewQuery in this same file). Switching to COUNT(*) only returns “number of summary rows”, so any dataset with 5 unresolved issues now comes back as count = 1, which breaks the dashboard stats. Please aggregate with SUM(count_issues) instead so we preserve the real totals.
- COUNT(*) AS count
+ SUM(count_issues) AS count 📝 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.
| SELECT | |
| dataset, | |
| issue_type, | |
| field, | |
| COUNT(*) AS count | |
| FROM endpoint_dataset_issue_type_summary | |
| WHERE organisation = '${datasetId}' | |
| AND severity = 'error' | |
| AND responsibility = 'external' | |
| AND (resource_end_date = '' OR resource_end_date IS NULL) | |
| GROUP BY dataset, issue_type, field` | |
| SELECT | |
| dataset, | |
| issue_type, | |
| field, | |
| SUM(count_issues) AS count | |
| FROM endpoint_dataset_issue_type_summary | |
| WHERE organisation = '${datasetId}' | |
| AND severity = 'error' | |
| AND responsibility = 'external' | |
| AND (resource_end_date = '' OR resource_end_date IS NULL) | |
| GROUP BY dataset, issue_type, field` |
🤖 Prompt for AI Agents
In src/services/performanceDbApi.js around lines 256 to 266, the query uses
COUNT(*) which returns number of summary rows instead of the precomputed issue
totals; replace COUNT(*) AS count with SUM(count_issues) AS count (or similar
alias) so the query aggregates the stored count_issues values and returns actual
issue totals while keeping the same WHERE and GROUP BY clauses.
GitHub Actions
Inactive
2 participants
Description
fetchEntityIssueCounts seems to be very slow, a query to Datasette Fetch Entity Issue Counts will show how slow it is.
This function has been moved to the performance db with this lookup New Datasette Query. This seems to be a lot faster and currently seems to work as expected with this change having checked multiple development pages for LPA's to the production equivalent.
Also parallel execution and removal of datasetErrorStatus (not needed) has been done to speed up the LPA Dashboard.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
All tests check this lookup works correctly already.
QA sign off
Summary by CodeRabbit
New Features
Improvements
Tests