1121 design implementation 1 show all datasets#1128
Conversation
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
WalkthroughFetch logic now returns provisions for the organisation without a dataset filter; dataset grouping was extended from two groups to four (statutory, expected, prospective, other) and templates/schemas/tests updated to surface the new groups and adjusted UI text. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Server
participant DB as Database
participant Template
Browser->>Server: GET /organisations/:orgId/overview
Server->>DB: fetchProvisions(orgId) %% style rect with color `#DFF0D8`
note right of DB `#DFF0D8`: provision rows for org (no dataset filter)
DB-->>Server: provisions[]
Server->>Server: prepareOverviewTemplateParams(provisions[])
note right of Server `#E8F4FF`: group into statutory, expected,\nprospective, other
Server->>Template: render overview with grouped datasets
Template-->>Browser: HTML response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-10-21T08:57:17.338ZApplied to files:
🔇 Additional comments (3)
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 |
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/middleware/lpa-overview.middleware.js(2 hunks)src/routes/schemas.js(1 hunks)src/views/organisations/overview.html(4 hunks)test/unit/middleware/lpa-overview.middleware.test.js(3 hunks)test/unit/views/organisations/lpaOverviewPage.test.js(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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`.
📚 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/unit/views/organisations/lpaOverviewPage.test.jstest/unit/middleware/lpa-overview.middleware.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-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:
test/unit/middleware/lpa-overview.middleware.test.js
🧬 Code graph analysis (1)
test/unit/middleware/lpa-overview.middleware.test.js (1)
src/middleware/lpa-overview.middleware.js (7)
prepareOverviewTemplateParams(257-311)req(76-76)req(125-125)req(150-150)req(208-208)req(323-323)req(339-339)
⏰ 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)
- GitHub Check: run-tests / test
🔇 Additional comments (10)
src/middleware/lpa-overview.middleware.js (2)
287-290: LGTM!The new provision reason handling correctly maps 'expected' and 'prospective' to their respective groups whilst maintaining the existing 'statutory' and 'other' categories.
29-35: No changes required—the provisions query is correctly designed.The dataset filter removal is intentional and safe. Whilst the query now fetches all provisions for an organisation, the data flow ensures only relevant provisions are used:
prepareOverviewTemplateParamsreceives provisions as a lookup table and datasets that are already filtered toavailableDatasets- Provisions are accessed only via
provisionData.get(ds.dataset)for datasets in the filtered set- Unused provision rows cause no performance impact or logical errors
This design is actually more flexible than SQL-level filtering, as configuration changes to
availableDatasetsare automatically reflected without requiring query logic updates.src/routes/schemas.js (1)
139-140: LGTM!The schema correctly extends the datasets object to include the new optional
expectedandprospectivearrays, maintaining consistency with the middleware changes.test/unit/middleware/lpa-overview.middleware.test.js (3)
99-102: LGTM!The test expectations correctly reference
datasets.expectedinstead of the previousdatasets.other, aligning with the middleware changes.
117-133: LGTM!Good test coverage for the conditional rendering of the expected datasets section based on ODP membership. The test correctly verifies both the presence of the section for ODP members and its absence for non-members.
148-150: LGTM!The dataset patching test correctly references
datasets.expected[1]to align with the updated grouping structure.test/unit/views/organisations/lpaOverviewPage.test.js (2)
76-86: LGTM!The tests correctly verify rendering of the new 'expected' and 'prospective' dataset groups with appropriate conditional checks.
88-92: LGTM!The allDatasets aggregation properly includes all three dataset groups (statutory, expected, prospective) using the nullish coalescing operator to handle optional arrays.
src/views/organisations/overview.html (2)
94-104: LGTM!The deadline notice loops for the new
datasets.expectedanddatasets.prospectivegroups are correctly implemented, maintaining consistency with the existing statutory datasets loop.
116-126: LGTM!The updated copy for the dataset status boxes improves clarity:
- "authoritative dataset(s) provided" better describes the metric
- "error(s) accessing URLs" is more specific than endpoint errors
- "can be improved" is more user-friendly than "needs fixing"
| {% if datasets.expected.length > 0 and isODPMember %} | ||
| <div data-testid="datasetsExpected"> | ||
| <h2 class="govuk-heading-m">Datasets {{ organisation.name}} are expected to provide</h2> | ||
| <p class="org-membership-info">We expect you to provide these datasets as a condition of your funding with the Open Digital Planning community.</p> | ||
| <ul class="govuk-task-list govuk-!-margin-bottom-8" data-reason="expected"> | ||
| {% for dataset in datasets.expected %} | ||
| {{ datasetItem(dataset) }} | ||
| {% endfor %} | ||
| </ul> | ||
| </div> | ||
| {% endif %} |
There was a problem hiding this comment.
Fix spacing in template variable.
Missing space after the template variable will cause the text to run together.
Apply this diff:
- <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>📝 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.
| {% if datasets.expected.length > 0 and isODPMember %} | |
| <div data-testid="datasetsExpected"> | |
| <h2 class="govuk-heading-m">Datasets {{ organisation.name}} are expected to provide</h2> | |
| <p class="org-membership-info">We expect you to provide these datasets as a condition of your funding with the Open Digital Planning community.</p> | |
| <ul class="govuk-task-list govuk-!-margin-bottom-8" data-reason="expected"> | |
| {% for dataset in datasets.expected %} | |
| {{ datasetItem(dataset) }} | |
| {% endfor %} | |
| </ul> | |
| </div> | |
| {% endif %} | |
| {% if datasets.expected.length > 0 and isODPMember %} | |
| <div data-testid="datasetsExpected"> | |
| <h2 class="govuk-heading-m">Datasets {{ organisation.name }} are expected to provide</h2> | |
| <p class="org-membership-info">We expect you to provide these datasets as a condition of your funding with the Open Digital Planning community.</p> | |
| <ul class="govuk-task-list govuk-!-margin-bottom-8" data-reason="expected"> | |
| {% for dataset in datasets.expected %} | |
| {{ datasetItem(dataset) }} | |
| {% endfor %} | |
| </ul> | |
| </div> | |
| {% endif %} |
🤖 Prompt for AI Agents
In src/views/organisations/overview.html around lines 147 to 157, the template
variable spacing is incorrect causing the rendered text to run together; update
the template to add a space inside the moustache braces so the heading reads
"Datasets {{ organisation.name }} are expected to provide" (ensure a space
before the closing }} and maintain consistent spacing elsewhere in that block).
| {% if datasets.prospective.length > 0 %} | ||
| <div data-testid="datasetsProspective"> | ||
| <h2 class="govuk-heading-m">Datasets {{ organisation.name}} can provide</h2> | ||
| <p class="org-membership-info">Your organisation is the authoritative source of this data. Providing this data to the platform helps ensure it is accurate and up to date.</p> | ||
| <ul class="govuk-task-list" data-reason="prospective"> | ||
| {% for dataset in datasets.prospective %} | ||
| {{ datasetItem(dataset) }} | ||
| {% endfor %} | ||
| </ul> | ||
| </div> | ||
| {% endif %} |
There was a problem hiding this comment.
Fix spacing in template variable.
Missing space after the template variable will cause the text to run together.
Apply this diff:
- <h2 class="govuk-heading-m">Datasets {{ organisation.name}} can provide</h2>
+ <h2 class="govuk-heading-m">Datasets {{ organisation.name }} can provide</h2>📝 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.
| {% if datasets.prospective.length > 0 %} | |
| <div data-testid="datasetsProspective"> | |
| <h2 class="govuk-heading-m">Datasets {{ organisation.name}} can provide</h2> | |
| <p class="org-membership-info">Your organisation is the authoritative source of this data. Providing this data to the platform helps ensure it is accurate and up to date.</p> | |
| <ul class="govuk-task-list" data-reason="prospective"> | |
| {% for dataset in datasets.prospective %} | |
| {{ datasetItem(dataset) }} | |
| {% endfor %} | |
| </ul> | |
| </div> | |
| {% endif %} | |
| {% if datasets.prospective.length > 0 %} | |
| <div data-testid="datasetsProspective"> | |
| <h2 class="govuk-heading-m">Datasets {{ organisation.name }} can provide</h2> | |
| <p class="org-membership-info">Your organisation is the authoritative source of this data. Providing this data to the platform helps ensure it is accurate and up to date.</p> | |
| <ul class="govuk-task-list" data-reason="prospective"> | |
| {% for dataset in datasets.prospective %} | |
| {{ datasetItem(dataset) }} | |
| {% endfor %} | |
| </ul> | |
| </div> | |
| {% endif %} |
🤖 Prompt for AI Agents
In src/views/organisations/overview.html around lines 159 to 169, the template
variable in the heading "Datasets {{ organisation.name}} can provide" is missing
a space before the closing braces which causes the rendered text to run
together; update the template variable formatting to include a space after the
variable (i.e., use the standard "{{ organisation.name }}" spacing) so the
output separates properly.
pooleycodes
deleted the
1121-design-implementation-1-show-all-datasets
branch
2 participants
Description
Updated the designs of the LPA Dashboard in line with new Service Designs placing datasets in different sections on the dashboard depending on each dataset's provision reason for that organisation.
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.
QA sign off
Summary by CodeRabbit
New Features
Improvements
Tests
Chores