681 dashboard fetch data from all active resources#730
Conversation
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
|
Warning Rate limit exceeded@GeorgeGoodall-GovUk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 23 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces comprehensive modifications across multiple middleware files in the project. The changes primarily focus on enhancing data fetching, error handling, and issue tracking mechanisms. Key updates include restructuring how resources and issues are processed, introducing new utility functions for entity and entry issue counts, and updating naming conventions across various middleware components. The modifications aim to improve the robustness and flexibility of data retrieval and processing in the application. Changes
Possibly related PRs
Suggested labels
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 (
|
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
…board-fetch-data-from-all-active-resources
047580b to
05940f5
Compare
…board-fetch-data-from-all-active-resources
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
test/unit/middleware/entryIssueDetails.middleware.test.js (1)
Line range hint
125-153: Fix failing test assertions for error messages.The pipeline indicates test failures due to incorrect error message format in the assertions. The error messages should be consistent with the implementation.
Apply this diff to fix the test assertions:
- expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=missing resources=present')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: entryIssues[pageNumber-1]=missing resources=present')) - expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: issues[pageNumber-1]=present resources=missing')) + expect(next).toHaveBeenCalledWith(new Error('Missing required values on request object: entryIssues[pageNumber-1]=present resources=missing'))
🧹 Nitpick comments (6)
test/unit/performanceDbApi.test.js (1)
90-92: Add test coverage for edge cases.While the test verifies the case where
num_issuesequalsrowCount, consider adding test cases for:
- When
num_issuesis greater thanrowCount- When
num_issuesis less thanrowCountit('returns an "all rows" message when num_issues >= rowCount', () => { const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 5, rowCount: 5, field: 'some_field' }) expect(message).toContain('all rows message for some_field') }) + +it('returns a plural message when num_issues > rowCount', () => { + const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 6, rowCount: 5, field: 'some_field' }) + expect(message).toContain('plural message for some_field with count 6') +}) + +it('returns a plural message when num_issues < rowCount', () => { + const message = performanceDbApi.getTaskMessage({ issue_type: 'some_issue_type', num_issues: 4, rowCount: 5, field: 'some_field' }) + expect(message).toContain('plural message for some_field with count 4') +})src/middleware/entryIssueDetails.middleware.js (1)
75-81: Consider enhancing the error message with more details.While the validation is thorough, the error message could be more descriptive about which specific fields are missing or invalid.
- const error = new Error('Invalid entry issue structure') + const error = new Error(`Invalid entry issue structure: missing ${!issue.entry_number ? 'entry_number' : !issue.issue_type ? 'issue_type' : 'line_number'}`)test/unit/middleware/dataview.middleware.test.js (1)
157-158: Add test cases for non-empty issue counts.While the test covers the case with empty arrays, consider adding test cases for:
- Non-empty
entityIssueCounts- Non-empty
entryIssueCounts- Mixed scenarios with both types of issues
const req = { orgInfo: { name: 'Mock Org' }, dataset: { name: 'Mock Dataset' }, tableParams: { columns: ['foo'], fields: ['foo'] }, - entityIssueCounts: [], - entryIssueCounts: [], + entityIssueCounts: [{ issue_type: 'error', count: 2 }], + entryIssueCounts: [{ issue_type: 'warning', count: 3 }], pagination: {},src/views/organisations/overview.html (1)
40-40: Consider enhancing the accessibility of issue count messages.The current message structure could be improved for screen readers.
Consider this alternative that provides more context:
- <p>There {{ "is" | pluralise(dataset.issueCount) }} {{ dataset.issueCount }} {{ "issue" | pluralise(dataset.issueCount) }} in this dataset</p> + <p>This dataset contains {{ dataset.issueCount }} {{ "issue" | pluralise(dataset.issueCount) }} that {{ "needs" | pluralise(dataset.issueCount) }} fixing</p>src/utils/utils.js (1)
97-110: Add JSDoc documentation for the new constant.Consider adding JSDoc documentation to improve maintainability and provide better context for the
entryIssueGroupsconstant.Add documentation like this:
+/** + * Defines groups of entry-related issues with their types and associated fields. + * Used for categorizing and processing data entry validation issues. + * @type {Array<{type: string, field: string}>} + */ export const entryIssueGroups = [ { type: 'missing value', field: 'reference' }, { type: 'reference values are not unique', field: 'reference' }, { type: 'unknown entity - missing reference', field: 'entity' } ]test/unit/middleware/datasetTaskList.middleware.test.js (1)
96-242: Consider adding more edge cases to the test suite.While the test suite covers many scenarios, consider adding these additional test cases:
- Test with malformed issue types (null, undefined, special characters)
- Test with negative or zero count values
- Test with extremely large count values
- Test with duplicate issue types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/middleware/common.middleware.js(8 hunks)src/middleware/datasetOverview.middleware.js(8 hunks)src/middleware/datasetTaskList.middleware.js(4 hunks)src/middleware/dataview.middleware.js(4 hunks)src/middleware/entryIssueDetails.middleware.js(3 hunks)src/middleware/issueTable.middleware.js(3 hunks)src/middleware/overview.middleware.js(10 hunks)src/routes/schemas.js(2 hunks)src/services/datasette.js(1 hunks)src/services/performanceDbApi.js(3 hunks)src/utils/utils.js(1 hunks)src/views/organisations/overview.html(1 hunks)test/unit/middleware/common.middleware.test.js(4 hunks)test/unit/middleware/datasetOverview.middleware.test.js(8 hunks)test/unit/middleware/datasetTaskList.middleware.test.js(1 hunks)test/unit/middleware/dataview.middleware.test.js(1 hunks)test/unit/middleware/entryIssueDetails.middleware.test.js(3 hunks)test/unit/middleware/overview.middleware.test.js(10 hunks)test/unit/performanceDbApi.test.js(1 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- test/unit/middleware/overview.middleware.test.js
- src/routes/schemas.js
- src/services/performanceDbApi.js
- test/unit/middleware/common.middleware.test.js
🧰 Additional context used
📓 Learnings (2)
test/unit/middleware/dataview.middleware.test.js (1)
Learnt from: rosado
PR: digital-land/submit#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`.
test/unit/middleware/overview.middleware.test.js (1)
Learnt from: GeorgeGoodall-GovUk
PR: digital-land/submit#510
File: test/unit/middleware/datasetOverview.middleware.test.js:1-3
Timestamp: 2024-11-12T10:13:49.419Z
Learning: Tests for `pullOutDatasetSpecification` have been moved to `common.middleware.test`.
🪛 GitHub Actions: Unit test coverage report
test/unit/middleware/entryIssueDetails.middleware.test.js
[error] 132-132: Test failed: Assertion error in prepareEntry test - unexpected error message format for missing issue case
[error] 153-153: Test failed: Assertion error in prepareEntry test - unexpected error message format for missing resources case
test/unit/middleware/overview.middleware.test.js
[error] 38-38: Test suite failed: TypeError: fetchOneFromAllDatasets is not a function
🪛 GitHub Check: test
src/middleware/overview.middleware.js
[failure] 38-38: test/unit/middleware/overview.middleware.test.js
TypeError: fetchOneFromAllDatasets is not a function
❯ src/middleware/overview.middleware.js:38:27
❯ test/unit/middleware/overview.middleware.test.js:14:25
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (15)
src/middleware/datasetTaskList.middleware.js (1)
103-103: Ensure 'type' and 'field' are defined to prevent invalid URLsWhen constructing the
hrefproperty for tasks,typeandfieldare used without checks. If eithertypeorfieldisundefined, the generated URL will include the string'undefined', leading to broken links.Please verify that
typeandfieldare always defined before encoding them in the URL. If they can beundefined, consider providing default values or adding checks to handle such cases.src/middleware/overview.middleware.js (1)
38-44:⚠️ Potential issueResolve undefined function 'fetchOneFromAllDatasets'
The function
fetchOneFromAllDatasetsused on line 38 is not defined or imported correctly, leading to aTypeError. This issue is causing the tests to fail and will prevent the application from running as expected.Apply this diff to correct the import and function usage:
-import { fetchMany, FetchOptions, renderTemplate, fetchOneFromAllDatasets } from './middleware.builders.js' +import { fetchMany, FetchOptions, renderTemplate, fetchOne } from './middleware.builders.js' ... -const fetchEntityCounts = fetchOneFromAllDatasets({ +const fetchEntityCounts = fetchOne({Alternatively, if
fetchOneFromAllDatasetsis a new utility function you intended to use, ensure that it is properly defined and exported from'./middleware.builders.js'.Please verify whether
fetchOneFromAllDatasetsis correctly implemented and exported. If not, adjust the import and function accordingly.✅ Verification successful
The review comment is correct -
fetchOneFromAllDatasetsis undefinedThe function
fetchOneFromAllDatasetsis not defined or exported inmiddleware.builders.js. The suggested fix to usefetchOneinstead is correct, as this function is properly defined and exported from the module.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check if middleware.builders.js exists and examine its contents fd middleware.builders.js --exec cat {} # Search for fetchOneFromAllDatasets usage across the codebase rg "fetchOneFromAllDatasets" -A 2 # Look for exports in middleware.builders.js ast-grep --pattern 'export { $$$ }'Length of output: 10878
🧰 Tools
🪛 GitHub Check: test
[failure] 38-38: test/unit/middleware/overview.middleware.test.js
TypeError: fetchOneFromAllDatasets is not a function
❯ src/middleware/overview.middleware.js:38:27
❯ test/unit/middleware/overview.middleware.test.js:14:25src/services/datasette.js (1)
Line range hint
21-27: Ensure query parameters are validated to prevent injection attacksWhile the
queryis URL-encoded usingencodeURIComponent, it's important to validate or sanitise thequeryparameter to prevent potential injection attacks or misuse. Make sure that thequerydoes not contain any malicious content that could affect the Datasette instance.src/middleware/dataview.middleware.js (2)
2-3: Imports and middleware chain updated correctlyThe import statements and middleware chain modifications appropriately incorporate new middleware functions. The changes are implemented correctly and enhance the data retrieval process.
70-75: Verify the accuracy of task count calculationIn the
prepareTemplateParamsfunction,taskCountis calculated asentityIssueCounts.length + entryIssueCounts.length. Ensure that this calculation accurately reflects the total number of outstanding tasks, and consider whether there might be overlap betweenentityIssueCountsandentryIssueCountsthat could lead to double-counting.test/unit/performanceDbApi.test.js (1)
86-88: LGTM! Parameter update aligns with row-based counting.The test case correctly verifies entity-level message generation with the new
rowCountparameter.src/middleware/entryIssueDetails.middleware.js (2)
63-70: LGTM! Improved error handling with detailed diagnostics.The error handling now provides clear diagnostics about which required values are missing, making it easier to troubleshoot issues.
136-144: Verify error logging placement in middleware chain.The
logPageErrormiddleware has been added at the end of the chain, which is the correct placement for catching any unhandled errors from previous middleware.✅ Verification successful
Error logging placement is consistently correct across all middleware chains
The
logPageErrormiddleware is correctly placed as the final middleware in all 9 middleware chains across the codebase, ensuring comprehensive error capture from preceding middleware. This confirms the approved placement inentryIssueDetails.middleware.jsfollows the established pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling middleware placement across files # Test: Search for logPageError placement in middleware chains rg -A 5 "logPageError" --type jsLength of output: 7278
src/middleware/issueTable.middleware.js (2)
95-95: LGTM! Using centralized issue groups improves maintainability.The change to use
entryIssueGroupsfrom utils ensures consistency across the application.
128-129: LGTM! Consistent error logging across middleware.Adding
logPageErrorat the end of the middleware chain ensures consistent error handling.src/views/organisations/overview.html (2)
40-40: Naming convention change looks good.The change from
issue_counttoissueCountaligns with the broader shift in naming conventions across the codebase.
37-43: Verify error message consistency with the reported bugs.Based on the PR objectives, there are discrepancies in error summaries and data display. The template should ensure consistent error reporting.
Run this script to check for inconsistencies in error handling across the codebase:
✅ Verification successful
Error message handling is consistent and properly implemented ✓
The error messages in the template are consistent with the error states tracked in the middleware and validated by the test suite. The implementation properly handles:
- Custom error messages
- Default fallback messages
- Pluralised issue counts
- All dataset status states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error message patterns in templates and middleware rg -g '*.html' -g '*.js' -A 2 'dataset.(error|issueCount|status)' # Search for error summary implementations ast-grep --pattern 'function $_(req, res) { $$$ summary $$$ }'Length of output: 6856
test/unit/middleware/entryIssueDetails.middleware.test.js (1)
82-90: LGTM! Test data structure updated correctly.The test data has been properly updated to use
entryIssuesinstead ofissues, with the addition of theissue_typefield.test/unit/middleware/datasetOverview.middleware.test.js (1)
Line range hint
32-41: LGTM! Test data structure updated consistently.The test data has been properly updated across all test cases to:
- Use arrays for the dataset property
- Include the new
entryIssueCountsandentityIssueCountsproperties- Maintain consistent structure throughout the file
Also applies to: 88-93, 109-114, 132-137, 147-152, 168-173, 197-202
test/unit/middleware/datasetTaskList.middleware.test.js (1)
30-94: LGTM! Comprehensive test coverage for the happy path.The test suite thoroughly covers the main functionality of the
prepareTasksfunction, including proper assertions and mocking.
| const entityCount = entities.length | ||
| let issues = [...entryIssueCounts, ...entityIssueCounts] |
There was a problem hiding this comment.
Prevent runtime errors by handling undefined 'entities' and issue counts
The code assumes that entities, entryIssueCounts, and entityIssueCounts are defined and are arrays. If any of these are undefined, accessing properties like entities.length or using the spread operator could cause runtime errors.
Apply this diff to safeguard against undefined variables:
- const entityCount = entities.length
- let issues = [...entryIssueCounts, ...entityIssueCounts]
+ const entityCount = entities ? entities.length : 0
+ const issues = [...(entryIssueCounts || []), ...(entityIssueCounts || [])]📝 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.
| const entityCount = entities.length | |
| let issues = [...entryIssueCounts, ...entityIssueCounts] | |
| const entityCount = entities ? entities.length : 0 | |
| const issues = [...(entryIssueCounts || []), ...(entityIssueCounts || [])] |
| import { statusToTagClass } from '../filters/filters.js' | ||
| import '../types/datasette.js' | ||
| import logger from '../utils/logger.js' | ||
| import { types } from 'util' |
There was a problem hiding this comment.
Fix incorrect import of 'types' to enable proper logging
The import statement on line 16 imports types from the Node.js built-in 'util' module, which does not export a types.App property. This will result in a runtime error when types.App is accessed in the logger. You should import types from '../utils/logging.js', where types.App is defined.
Apply this diff to correct the import:
-import { types } from 'util'
+import { types } from '../utils/logging.js'Committable suggestion skipped: line range outside the PR's diff.
| const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' | ||
| const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' |
There was a problem hiding this comment.
Fix SQL injection vulnerabilities in query construction
In the fetchEntityIssuesForFieldAndType function, the issueTypeClause, issueFieldClause, and req.params.dataset are directly embedded into the SQL query without sanitisation. This poses a SQL injection risk. Please use parameterised queries or properly escape user inputs.
Modify the query to use parameters. For example:
query: ({ req, params }) => {
const issueTypeParam = params.issue_type || ''
const issueFieldParam = params.issue_field || ''
const datasetParam = req.params.dataset || ''
const issueTypeClause = issueTypeParam ? 'AND i.issue_type = ?' : ''
const issueFieldClause = issueFieldParam ? 'AND field = ?' : ''
const queryParams = []
const resourcePlaceholders = req.resources.map(() => '?').join(', ')
const queryString = `
SELECT i.issue_type, field, entity, message, severity, value
FROM issue i
LEFT JOIN issue_type it ON i.issue_type = it.issue_type
WHERE resource IN (${resourcePlaceholders})
${issueTypeClause}
AND it.responsibility = 'external'
AND it.severity = 'error'
${issueFieldClause}
AND i.dataset = ?
AND entity != ''`
queryParams.push(...req.resources.map(resource => resource.resource))
if (issueTypeParam) queryParams.push(issueTypeParam)
if (issueFieldParam) queryParams.push(issueFieldParam)
queryParams.push(datasetParam)
return { queryString, queryParams }
}Ensure that fetchMany or the database layer supports parameterised queries.
Also applies to: 419-419
| const issueTypeClause = params.issue_type ? `AND i.issue_type = '${params.issue_type}'` : '' | ||
| const issueFieldClause = params.issue_field ? `AND field = '${params.issue_field}'` : '' |
There was a problem hiding this comment.
Fix SQL injection vulnerabilities in fetchEntryIssues
The issueTypeClause, issueFieldClause, and req.params.dataset in fetchEntryIssues are directly inserted into the SQL query without sanitisation, which can lead to SQL injection attacks. Please use parameterised queries to prevent this security risk.
Modify the query to safely include parameters similar to previous suggestions.
Also applies to: 528-529
| WHERE resource in ('${req.resources.map(resource => resource.resource).join("', '")}') | ||
| AND (entity != '' AND entity IS NOT NULL) |
There was a problem hiding this comment.
Fix SQL injection vulnerabilities in fetchEntityIssueCounts
In fetchEntityIssueCounts, the req.params.dataset is directly embedded into the SQL query. This exposes the application to SQL injection attacks. Use parameterised queries or sanitise the inputs.
Revise the query to include parameters securely.
Also applies to: 548-549
| select dataset, field, i.issue_type, COUNT(resource + line_number) as count | ||
| from issue i |
There was a problem hiding this comment.
Fix SQL injection vulnerabilities in fetchEntryIssueCounts
Similarly, in fetchEntryIssueCounts, the req.params.dataset is directly used in the SQL query without sanitisation. Ensure that you use parameterised queries to prevent SQL injection vulnerabilities.
Update the query to use parameters safely.
Also applies to: 582-583
| // arguably removeIssuesThatHaveBeenFixed should be s step however we have only currently found one organisation, | ||
| // however this step is very time consuming, so in order to progress im commenting it out for now | ||
| // removeIssuesThatHaveBeenFixed, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the decision to comment out removeIssuesThatHaveBeenFixed
The removeIssuesThatHaveBeenFixed function is commented out due to performance concerns. However, omitting this step may lead to displaying issues that have already been resolved, potentially confusing users. Consider optimising the function for performance or finding an alternative solution to ensure only current issues are presented.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/middleware/middleware.builders.js (1)
20-24: Consider caching the available datasets calculationSince this calculation is performed on a constant and the result doesn't change during runtime, consider memoising it to avoid repeated computation.
-const availableDatasets = Object.values(dataSubjects).flatMap((dataSubject) => +const availableDatasets = Object.freeze(Object.values(dataSubjects).flatMap((dataSubject) => (dataSubject.dataSets || []) .filter((dataset) => dataset.available) .map((dataset) => dataset.value) -) +))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/middleware/middleware.builders.js(3 hunks)test/unit/middleware/entryIssueDetails.middleware.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/middleware/entryIssueDetails.middleware.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
🔇 Additional comments (1)
src/middleware/middleware.builders.js (1)
275-291: LGTM! Well-documented context binding functionsThe implementation follows the established pattern in the file and includes comprehensive JSDoc documentation.
| }) | ||
| }) | ||
|
|
||
| describe('aggregateOverviewData middleware', () => { |
There was a problem hiding this comment.
removed as aggregateOverviewData middleware was removed
…board-fetch-data-from-all-active-resources
What type of PR is this? (check all applicable)
Description
This ticket updated the dashboard to get issues from all active resources
Related Tickets & Documents
#681
#683
QA Instructions, Screenshots, Recordings
Visually not much has changed, but some of the data we are playing back will have changed due to us now getting data from all active resources. unfortunately this is the case with mostly just BFL data. and the only content we would change on the dashboard is the hint on each dataset card. but this is currently being overwritten by the BFL notice.
you can however see the results of these changes in the tasklist and also taskcount in the tabs
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Refactoring