620 bfl dataset card on dashboard highlights if dataset needs updating#644
Conversation
…dataset-card-on-dashboard-highlights-if-dataset-needs-updating
WalkthroughThis pull request introduces several enhancements across multiple middleware, utility, and view files, primarily focused on managing dataset deadlines and notices. New utility functions, such as Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…dataset-card-on-dashboard-highlights-if-dataset-needs-updating
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…dataset-card-on-dashboard-highlights-if-dataset-needs-updating
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…on-dashboard-highlights-if-dataset-needs-updating
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
…dataset-card-on-dashboard-highlights-if-dataset-needs-updating
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
test/unit/views/organisations/lpaOverviewPage.test.js (2)
Line range hint
81-107: Improve maintainability of hint logicThe nested if-else conditions for determining hints make the code harder to maintain. Consider extracting this logic into a separate function using a more declarative approach.
Here's a suggested refactor:
+const getExpectedHint = (dataset) => { + const hintMap = { + 'due': (dataset) => `You must update this dataset by ${dataset.notice.deadline}`, + 'overdue': (dataset) => `Your ${datasetSlugToReadableName(dataset.dataset)} dataset is overdue`, + 'Not submitted': () => 'Data URL not submitted', + 'Needs fixing': () => 'in this dataset', + 'Error': (dataset) => { + if (dataset.error) return dataset.error + const count = dataset.issue_count + if (count === 0) return `There are ${count} issues in this dataset` + if (count === 1) return `There is ${count} issue in this dataset` + return `There are ${count} issues in this dataset` + } + } + + if (dataset.notice) { + if (!(dataset.notice.type in hintMap)) { + throw new Error('Notice type not recognised') + } + return hintMap[dataset.notice.type](dataset) + } + + return hintMap[dataset.status]?.(dataset) ?? 'Data URL submitted' +} it(`dataset cards are rendered with correct hints for dataset='${dataset.dataset}'`, () => { - let expectedHint = 'Data URL submitted' - if (dataset.notice) { - if (dataset.notice.type === 'due') { - expectedHint = `You must update this dataset by ${dataset.notice.deadline}` - } else if (dataset.notice.type === 'overdue') { - expectedHint = `Your ${datasetSlugToReadableName(dataset.dataset)} dataset is overdue` - } else { - throw new Error('Notice type not recognised') - } - } else if (dataset.status === 'Not submitted') { - expectedHint = 'Data URL not submitted' - } else if (dataset.status === 'Needs fixing') { - expectedHint = 'in this dataset' - } else if (dataset.status === 'Error') { - expectedHint = dataset.error || '' - } else if (dataset.status === 'Error' && dataset.issue_count === 0) { - expectedHint = `There are ${dataset.issue_count} issues in this dataset` - } else if (dataset.status === 'Error' && dataset.issue_count === 1) { - expectedHint = `There is ${dataset.issue_count} issue in this dataset` - } else if (dataset.status === 'Error' && dataset.issue_count > 1) { - expectedHint = `There are ${dataset.issue_count} issues in this dataset` - } + const expectedHint = getExpectedHint(dataset) const datasetCard = document.querySelector(`[data-dataset="${dataset.dataset}"]`) expect(datasetCard.querySelector('.govuk-task-list__hint').textContent.trim()).toContain(expectedHint) })
160-181: Improve notification banner test implementationThe current implementation uses mutable state and contains duplicated logic for determining notice text.
Consider this improved implementation:
+const getNoticeText = (dataset) => { + const baseText = `${datasetSlugToReadableName(dataset.dataset)} dataset` + + switch (dataset.notice.type) { + case 'due': + return { + header: `You must update your ${baseText} by ${dataset.notice.deadline}` + } + case 'overdue': + return { + header: `Your ${baseText} is overdue`, + hint: `It was due on ${dataset.notice.deadline}` + } + default: + throw new Error(`Unexpected notice type: ${dataset.notice.type}`) + } +} it(`Renders the notice for dataset ${dataset.dataset}`, () => { - let expectedHeader - let expectedHint - - if (dataset.notice.type === 'due') { - expectedHeader = `You must update your ${datasetSlugToReadableName(dataset.dataset)} dataset by ${dataset.notice.deadline}` - } else if (dataset.notice.type === 'overdue') { - expectedHeader = `Your ${datasetSlugToReadableName(dataset.dataset)} dataset is overdue` - expectedHint = `It was due on ${dataset.notice.deadline}` - } + const { header: expectedHeader, hint: expectedHint } = getNoticeText(dataset) const expectedLinkHref = `/organisations/${params.organisation.organisation}/${dataset.dataset}/get-started` expect(banner.textContent).toContain(expectedHeader) if (expectedHint) { expect(banner.textContent).toContain(expectedHint) } const link = banner.querySelector('.govuk-notification-banner__link') expect(link.getAttribute('href')).toEqual(expectedLinkHref) })src/middleware/overview.middleware.js (2)
68-80: Consider enhancing type safety and documentation of the accumulator arrayThe
orgStatsReducerfunction uses a numeric array for accumulation without clear indication of what each index represents.Consider this enhancement:
-const orgStatsReducer = (accumulator, dataset) => { +/** + * Reduces dataset information into counts of endpoints, issues, and errors + * @param {[number, number, number]} accumulator - Array of [endpointCount, needsFixingCount, errorCount] + * @param {{ endpoint?: string, status: string }} dataset - Dataset information + * @returns {[number, number, number]} Updated counts + */ +const orgStatsReducer = (accumulator, dataset) => { if (dataset.endpoint) accumulator[0]++ if (dataset.status === 'Needs fixing') accumulator[1]++ if (dataset.status === 'Error') accumulator[2]++ return accumulator }
Line range hint
200-240: Add input validation and handle potential undefined valuesThe function needs additional validation to prevent runtime errors.
Apply these enhancements:
export function aggregateOverviewData (req, res, next) { const { lpaOverview } = req if (!Array.isArray(lpaOverview)) { throw new Error('lpaOverview should be an array') } + if (lpaOverview.some(item => typeof item !== 'object' || !item.dataset)) { + throw new Error('Invalid lpaOverview item structure') + } const grouped = _.groupBy(lpaOverview, 'dataset') const datasets = [] for (const [dataset, rows] of Object.entries(grouped)) { let numIssues = 0 for (const row of rows) { if (row.status !== 'Needs fixing') { continue } if (row.issue_count) { const numFields = (row.fields ?? '').split(',').length if (row.issue_count >= row.entity_count) numIssues += numFields else numIssues += row.issue_count } } + const maxStatusRow = _.maxBy(rows, row => statusOrdering.get(row.status ?? 'Not submitted')) const info = { dataset, issue_count: numIssues, - endpoint: rows[0].endpoint, - error: rows[0].error, - status: _.maxBy(rows, row => statusOrdering.get(row.status)).status + endpoint: rows[0]?.endpoint, + error: rows[0]?.error, + status: maxStatusRow?.status ?? 'Not submitted' } datasets.push(info) } requiredDatasets.forEach(requiredDataset => { const hasDataset = datasets.findIndex(dataset => dataset.dataset === requiredDataset.dataset) >= 0 if (!hasDataset) { datasets.push({ dataset: requiredDataset.dataset, status: 'Not submitted' }) } }) req.datasets = datasets next() }test/unit/middleware/overview.middleware.test.js (3)
147-147: Fix typo in test descriptionChange
should'ttoshouldn'tin the test description.
88-153: Consider adding edge cases to dataset aggregation testsThe test suite would benefit from additional edge cases:
- Dataset with empty fields string
- Dataset with duplicate field names
- Dataset with very large number of fields
211-211: Consider timezone-safe date testingThe use of explicit timestamps in
vi.setSystemTimecould make tests fragile across different timezones. Consider using relative time offsets from a base date to make tests more robust.-vi.setSystemTime(new Date('1996-03-03T00:00:00.000Z')) +const baseDate = new Date('1996-04-17T10:00:00.000Z') // from mock +vi.setSystemTime(new Date(baseDate.getTime() - (45 * 24 * 60 * 60 * 1000))) // 45 days before deadlineAlso applies to: 227-227, 243-243, 254-254, 265-265
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/middleware/overview.middleware.js(8 hunks)src/routes/schemas.js(2 hunks)src/views/organisations/overview.html(2 hunks)test/unit/middleware/overview.middleware.test.js(5 hunks)test/unit/views/organisations/lpaOverviewPage.test.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/schemas.js
- src/views/organisations/overview.html
🧰 Additional context used
📓 Learnings (1)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🪛 GitHub Check: test
test/unit/views/organisations/lpaOverviewPage.test.js
[failure] 155-155: test/unit/views/organisations/lpaOverviewPage.test.js
TypeError: params.datasets.forEach is not a function
❯ test/unit/views/organisations/lpaOverviewPage.test.js:155:19
🔇 Additional comments (1)
src/middleware/overview.middleware.js (1)
319-321: LGTM! Middleware chain order is correct
The middleware functions are arranged in the correct order to ensure proper data flow:
aggregateOverviewDataprocesses the overview data firstdatasetSubmissionDeadlineCheckchecks deadlines using the processed dataaddNoticesToDatasetsadds notices based on the deadline checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
83-90: Improve notice type handlingThe current if-else structure for notice types could be improved:
- The error message doesn't specify which notice type was received
- The code could be more maintainable using a lookup object
Consider this refactor:
-if (dataset.notice) { - if (dataset.notice.type === 'due') { - expectedHint = `You must update this dataset by ${dataset.notice.deadline}` - } else if (dataset.notice.type === 'overdue') { - expectedHint = `Your ${datasetSlugToReadableName(dataset.dataset)} dataset is overdue` - } else { - throw new Error('Notice type not recognised') - } -} +if (dataset.notice) { + const noticeHints = { + due: (deadline) => `You must update this dataset by ${deadline}`, + overdue: (_, dataset) => `Your ${datasetSlugToReadableName(dataset)} dataset is overdue` + }; + const hintFn = noticeHints[dataset.notice.type]; + if (!hintFn) { + throw new Error(`Unrecognised notice type: ${dataset.notice.type}`); + } + expectedHint = hintFn(dataset.notice.deadline, dataset.dataset); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
test/unit/views/organisations/lpaOverviewPage.test.js(6 hunks)
🧰 Additional context used
📓 Learnings (1)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/views/organisations/dataset-overview.test.js:3-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Ensure to verify import paths in all relevant test files before flagging inconsistencies.
🪛 GitHub Check: test
test/unit/views/organisations/lpaOverviewPage.test.js
[failure] 173-173: test/unit/views/organisations/lpaOverviewPage.test.js > LPA Overview Page (seed: 1732031027266) > Renders the notice for dataset fugiat sunt dolore
TypeError: Cannot read properties of undefined (reading 'textContent')
❯ test/unit/views/organisations/lpaOverviewPage.test.js:173:23
🔇 Additional comments (1)
test/unit/views/organisations/lpaOverviewPage.test.js (1)
Line range hint 13-13: Fix non-deterministic test seed
Using new Date().getTime() as the seed will make tests non-deterministic, potentially causing flaky tests that pass sometimes and fail others.
Apply this diff to ensure consistent test results:
-const seed = new Date().getTime()
+const seed = 1731493378197
…-if-dataset-needs-updating
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/views/organisations/overview.html (2)
29-32: Consider defensive programming for notice accessThe notice check could benefit from more robust validation. Consider using optional chaining or nullish coalescing.
-{% if dataset.notice %} +{% if dataset.notice and dataset.notice.length %}
82-92: Consider consolidating duplicate notification loopsThe notification loops for statutory and other datasets are identical. Consider extracting this into a reusable macro to reduce code duplication.
{% macro renderDatasetNotices(datasets) %} {% for dataset in datasets %} {% if dataset.notice %} {{ deadlineNotice(dataset.dataset, dataset.notice, organisation) }} {% endif %} {% endfor %} {% endmacro %}Then use it like:
{{ renderDatasetNotices(datasets.statutory) }} {{ renderDatasetNotices(datasets.other) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/views/organisations/overview.html(3 hunks)test/unit/views/organisations/lpaOverviewPage.test.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/views/organisations/lpaOverviewPage.test.js
🔇 Additional comments (3)
src/views/organisations/overview.html (3)
5-6: LGTM: Component imports are properly structured
The new imports for deadline notice components align well with the PR's objective of implementing notification banners.
13-13: Verify URL structure changes
The change from dataset.slug to dataset.dataset in URLs and data attributes appears intentional. Please verify this change is consistent with your routing configuration.
Also applies to: 18-18, 23-23
135-140: LGTM: Clear membership status presentation
The addition of organisation membership status provides valuable context for users, clearly indicating whether the organisation is part of the Open Digital Planning programme.
DilwoarH
left a comment
DilwoarH
left a comment
There was a problem hiding this comment.
Just some minor changes to test file structure. The most comment method is the Arrange-Act-Assert Pattern:
https://learn.microsoft.com/en-us/visualstudio/test/unit-test-basics?view=vs-2022#write-your-tests
it('should return the correct sum when adding two numbers', () => {
// Arrange
const a = 2, b = 3;
// Act
const result = add(a, b);
// Assert
expect(result).toBe(5);
});
Co-authored-by: Dilwoar Hussain <DilwoarH@users.noreply.github.com>
Co-authored-by: Dilwoar Hussain <DilwoarH@users.noreply.github.com>
Co-authored-by: Dilwoar Hussain <DilwoarH@users.noreply.github.com>
Co-authored-by: Dilwoar Hussain <DilwoarH@users.noreply.github.com>
|
Code looks all good now - over to product and design sign off |
…-if-dataset-needs-updating
|
Looks good to me 🚀 |
7 participants
What type of PR is this? (check all applicable)
Description
Adds notification banners for due and overdue datasets
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Preview Link: https://submit-pr-644.herokuapp.com/
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests