423 add issues table component to issues details page part 2

[GeorgeGoodall-GovUk]

What type of PR is this? (check all applicable)

• Refactor
• Feature

Description

Adds an issue table page that displays all entries with issues for a specific provision, issue type and issue field

During development, after speaking with Owen a lot of our queries were incorrect. so I've made some additional changes with them

• we now get entries and issues for all active resources
• we are now showing entities instead of entries, and searching for issues with references (aka, they have an underlying reference against them)
• We also search for issues without references, and show them in addition

Other additional changes include:

• rename isResourceIdInParams to isResourceIdNotInParams so it represents functionality
• moved several middleware's to the common.middleware file, as multiple middleware chains now use them
• updated the valibot schemas so to facilitate using them in mocking
• moved shared test utils to sharedTests folder
• improved mocker so it could mock table params with preserved columns and row value indexes

Related Tickets & Documents

• Closes Add issues table component to issues details page #423

New tickets to make:

• We have some disparities between the table and summary messages / task list messages that arise due to us having some issues that dont map to an entity. we might need to think of a design fix for this?

QA Instructions, Screenshots, Recordings

Preview Link: https://submit-pr-510.herokuapp.com/

Added/updated tests?

We encourage you to keep the code coverage percentage at 80% and above.

• Yes

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced styling for tables with new CSS classes for scrollable layouts and error indications.
  • New middleware functions added for improved handling of datasets and issues.
  • New routes for fetching issue details and tables with optional pagination.
  • New test suite for validating the rendering and functionality of the issue table.

• Bug Fixes

  • Improved error handling and validation for various middleware functions.

• Documentation

  • Updated schema definitions for better clarity and modularity.

• Tests

  • Comprehensive test coverage added for new and existing functionalities, including unit tests for middleware and templates.
  • Enhanced mock data generation for improved test consistency.
  • Restructured test organisation for better modularity and clarity.
  • Updated test paths to reflect new directory structure for shared tests.

[coderabbitai]

Actionable comments posted: 14

🧹 Outside diff range and nitpick comments (22)

test/utils/mocker.js (1)

112-114: LGTM with a minor suggestion

The getSeed function is well-implemented, correctly prioritising the TEST_SEED environment variable and using the current timestamp as a fallback. This ensures consistent and reproducible test results when needed, while still providing unique seeds for each run by default.

For improved clarity, consider adding a brief comment explaining the purpose of this function:

/**
 * Retrieves a seed value for random number generation.
 * Uses TEST_SEED environment variable if set, otherwise uses current timestamp.
 */
export const getSeed = () => {
  return process.env.TEST_SEED || new Date().getTime()
}

src/middleware/issueDetails.middleware.js (1)

97-145: Improved template parameter preparation with enhanced security

The prepareIssueDetailsTemplateParams function has been significantly improved:

1. It now handles cases where pageNumber is out of bounds, returning a 400 error.
2. The use of issueErrorMessageHtml and getIssueField functions improves code organisation and reusability.
3. Entity values are now properly escaped, addressing potential XSS vulnerabilities.

These changes greatly enhance the function's robustness and security.

However, there's a minor improvement that could be made:

On line 123, consider using optional chaining with the nullish coalescing operator to handle potential undefined values more safely:

-    valueHtml += escape(entity[datasetField]?.value || '')
+    valueHtml += escape(entity[datasetField]?.value ?? '')

This change ensures that empty strings are preserved rather than being replaced with an empty string, which might be the desired behaviour in some cases.

test/unit/issueDetailsPage.test.js (4)

40-40: LGTM: Enhanced error summary structure.

The updated error summary handling improves flexibility and consistency. This aligns well with the changes in the schemas.js file.

Consider adding a test case to verify the behaviour when errorSummary is undefined or null, ensuring robust error handling.

Also applies to: 46-49, 61-61, 64-64

---

127-130: LGTM: Improved pagination title handling.

The addition of pagination information to the page title enhances user experience and accessibility. The conditional logic is appropriate for handling single-page results.

Consider extracting the pagination title logic into a separate function for better testability and reusability.

---

182-182: LGTM: Updated geometry handling for map rendering.

The changes to geometry handling, particularly the shift from an array to a string, align with the modifications mentioned in the AI-generated summary. The tests have been appropriately updated to reflect this new structure.

Consider adding test cases for:

1. Multiple geometries concatenated in the string.
2. Invalid geometry strings to ensure proper error handling.

Also applies to: 185-185, 187-188, 205-205, 208-208, 210-211

---

Line range hint 1-224: LGTM: Well-structured and comprehensive test suite.

The overall organization of the test suite is logical and covers key functionalities. The modifications align well with the changes in the application code.

Consider adding a describe block for testing edge cases, such as handling of unexpected input values or error states, to further enhance the robustness of the test suite.

test/unit/middleware/issueDetails.middleware.test.js (3)

10-36: Consider adding a test case for empty string errorMessage

The test suite for issueErrorMessageHtml is comprehensive, covering various scenarios including null values. However, it might be beneficial to add a test case for when errorMessage is an empty string, as this could be a potential edge case in real-world usage.

Consider adding the following test case:

it('should return an empty string if errorMessage is an empty string', () => {
  const issue = { value: '02-02-2022' }
  const result = issueErrorMessageHtml('', issue)
  expect(result).toBe('')
})

This will ensure the function behaves correctly when given an empty string as the error message.

---

129-209: Enhance test coverage for prepareIssueDetailsTemplateParams

The current test suite for prepareIssueDetailsTemplateParams provides good coverage of the happy path scenarios. However, to ensure robustness, consider adding the following test cases:

1. Error handling: Test the function's behaviour when required properties are missing from the request object.
2. Edge cases: Test with empty arrays for entities and specification fields.
3. Boundary testing: Test with minimum and maximum values for pagination.

Here's an example of an additional test case for error handling:

it('should handle missing required properties gracefully', () => {
  const incompleteReq = { params: {} }
  prepareIssueDetailsTemplateParams(incompleteReq, res, next)
  expect(incompleteReq.templateParams).toBeDefined()
  // Add more specific expectations based on how your function should handle this case
})

Adding these tests will increase confidence in the function's ability to handle various scenarios robustly.

---

Line range hint 211-270: Expand test coverage for getIssueDetails function

The current test for getIssueDetails covers the happy path scenario, which is a good start. However, to ensure the function behaves correctly in all situations, consider adding the following test cases:

1. Error handling: Test the function's behaviour when req.templateParams is undefined or missing required properties.
2. Edge cases: Test with empty or minimal templateParams objects.
3. Verify error scenarios: Test how the function handles rendering errors.

Here's an example of an additional test case for error handling:

it('should handle missing templateParams gracefully', () => {
  const reqWithoutTemplateParams = {}
  const res = { render: vi.fn() }
  const next = vi.fn()

  getIssueDetails(reqWithoutTemplateParams, res, next)

  expect(next).toHaveBeenCalledWith(expect.any(Error))
  expect(res.render).not.toHaveBeenCalled()
})

Adding these tests will provide a more comprehensive coverage of the getIssueDetails function and increase confidence in its robustness.

src/middleware/common.middleware.js (1)

276-291: Consider adding explanatory comments for special cases

The function handles a special case for 'GeoX,GeoY' fields, but the reason for this special treatment isn't immediately clear from the code.

Consider adding explanatory comments:

 export const addDatasetFieldsToIssues = (req, res, next) => {
   const { issuesWithReferences, specification } = req
 
   req.issuesWithReferences = issuesWithReferences.map(issue => {
     let datasetField
+    // Special case: For brownfield land, GeoX and GeoY are combined into a single 'point' field
     if (issue.field === 'GeoX,GeoY') { // special case for brownfield land
       datasetField = 'point'
     } else {
       const specificationEntry = specification.fields.find(field => field.field === issue.field)
       datasetField = specificationEntry ? specificationEntry.datasetField : specificationEntry?.field || issue.field
     }
     return { ...issue, datasetField }
   })
 
   next()
 }

These comments provide context for why certain fields are treated differently, making the code more maintainable and easier to understand for other developers.

test/unit/middleware/common.middleware.test.js (12)

7-19: Consider adding more test cases for 'logPageError'.

While the current test covers the basic functionality, consider adding the following improvements:

1. Test with different types of errors (e.g., TypeError, RangeError).
2. Verify the exact arguments passed to logger.warn.
3. Test the case where handlerName is not present in the request object.

These additions would improve the robustness of the test suite.

---

21-62: Enhance test coverage for resource middleware functions.

The current test suite covers basic scenarios well. Consider adding the following test cases to improve coverage:

1. For isResourceAccessible and isResourceNotAccessible:

   • Test with undefined or null resourceStatus.
   • Test with non-string status values.

2. For isResourceIdNotInParams:

   • Test with an empty string as resourceId.
   • Test with non-string resourceId values.

3. For takeResourceIdFromParams:

   • Test behaviour when resourceId is not present in params.
   • Test with different types of resourceId values.

These additional tests will help ensure the middleware functions handle various edge cases correctly.

---

64-102: Enhance test coverage and assertions for 'formatErrorSummaryParams'.

The current test suite provides good coverage of main scenarios. Consider the following improvements:

1. Add test cases for edge scenarios:

   • Empty issuesWithReferences and issuesWithoutReferences arrays.
   • Very large number of issues to test pagination or truncation if implemented.

2. Replace snapshot testing with explicit assertions where possible:

   • Assert the structure and content of the errorSummary object directly.
   • This will make the tests more resilient to minor changes and easier to debug.

3. Test error handling:

   • Verify behaviour when required parameters are missing or in unexpected formats.

These enhancements will improve the robustness and maintainability of the test suite.

---

104-130: Improve test coverage for 'pullOutDatasetSpecification'.

While the current test covers the basic functionality, consider adding the following test cases:

1. Error handling:

   • Test with invalid JSON in the specification.
   • Test when the specification is missing or null.

2. Edge cases:

   • Test with an empty array in the JSON.
   • Test with multiple dataset entries in the JSON array.

3. Verify that the function doesn't modify other properties of the request object.

These additional tests will ensure the function behaves correctly under various scenarios and improve overall robustness.

---

132-235: Enhance test coverage for pagination functions.

The current test suite provides good coverage of main scenarios. Consider the following improvements:

1. For getPaginationOptions:

   • Test with negative page numbers.
   • Test with non-integer page numbers.
   • Test with page numbers exceeding the total number of pages.

2. For paginateEntitiesAndPullOutCount:

   • Test with an empty entities array.
   • Test with a number of entities less than the pagination limit.

3. For createPaginationTemplateParams:

   • Test with a very large number of pages to ensure proper ellipsis placement.
   • Test with resultsCount of 0.

4. For all functions:

   • Test error handling when required parameters are missing or in unexpected formats.

These additional test cases will help ensure the pagination functions handle various edge cases correctly and improve overall robustness.

---

238-304: Improve test coverage for 'extractJsonFieldFromEntities'.

The current test suite covers basic scenarios well. Consider adding the following test cases to improve coverage:

1. Error handling:

   • Test with malformed JSON in the 'json' field.
   • Test with non-string values in the 'json' field.

2. Edge cases:

   • Test with nested JSON objects.
   • Test with JSON arrays.
   • Test with very large JSON objects to ensure performance.

3. Verify that the function doesn't modify other properties of the entities.

4. Test with a mix of entities, some with valid JSON, some without, and some with invalid JSON.

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

306-358: Enhance test coverage for 'replaceUnderscoreWithHyphenForEntities'.

The current test suite covers basic scenarios well. Consider adding the following test cases to improve coverage:

1. Edge cases:

   • Test with keys that have multiple underscores.
   • Test with keys that start or end with an underscore.
   • Test with very long key names.

2. Error handling:

   • Test with non-object entities.
   • Test with null or undefined entities.

3. Verify that the function doesn't modify values, only keys.

4. Test with nested objects to ensure deep replacement if that's a requirement.

5. Test with a mix of entities, some with underscores and some without.

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

360-426: Improve test coverage for 'nestEntityFields'.

The current test suite covers basic scenarios well. Consider adding the following test cases to improve coverage:

1. Edge cases:

   • Test with fields in the specification that don't exist in the entities.
   • Test with fields in the entities that aren't in the specification.
   • Test with nested fields in the specification.

2. Error handling:

   • Test with invalid or missing specification.
   • Test with non-object entities.

3. Verify that the function doesn't modify fields not listed in the specification.

4. Test with a mix of entities, some with all fields, some with missing fields.

5. Test with very large entities and specifications to ensure performance.

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

428-519: Enhance test coverage for 'addDatasetFieldsToIssues'.

The current test suite covers main scenarios well. Consider adding the following test cases to improve coverage:

1. Edge cases:

   • Test with empty issuesWithReferences array.
   • Test with issues that have fields not present in the specification.
   • Test with specification fields that don't match any issues.

2. Error handling:

   • Test with invalid or missing specification.
   • Test with malformed issues (missing required properties).

3. Verify that the function doesn't modify other properties of the issues.

4. Test with a large number of issues and specification fields to ensure performance.

5. Test the behaviour when multiple specification fields match a single issue field.

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

521-585: Improve test coverage for 'addIssuesToEntities'.

The current test suite covers basic scenarios well. Consider adding the following test cases to improve coverage:

1. Edge cases:

   • Test with empty entities array.
   • Test with issues that don't match any entity.
   • Test with entities that have no matching issues.

2. Error handling:

   • Test with malformed entities or issues (missing required properties).
   • Test with non-array inputs for entities and issuesWithReferences.

3. Verify that the function doesn't modify properties of entities that don't have issues.

4. Test with a large number of entities and issues to ensure performance.

5. Test the behaviour when multiple issues match a single entity field.

6. Test with nested entity structures if that's a possibility in the actual data.

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

587-678: Enhance test coverage for 'addDatabaseFieldToSpecification'.

The current test suite covers main scenarios well. Consider adding the following test cases to improve coverage:

1. Edge cases:

   • Test with empty specification fields array.
   • Test with empty fieldMappings array.
   • Test with field mappings that don't match any specification fields.

2. Error handling:

   • Test with invalid or missing specification or fieldMappings.
   • Test with malformed specification fields or field mappings (missing required properties).

3. Verify that the function doesn't modify other properties of the specification.

4. Test with a large number of specification fields and field mappings to ensure performance.

5. Test the behaviour when multiple field mappings match a single specification field.

6. Test with different combinations of GeoX and GeoY fields (e.g., only GeoX present, only GeoY present, both present but not adjacent).

These additional tests will help ensure the function behaves correctly under various scenarios and improve overall robustness.

---

1-678: Overall suggestions for improving test coverage and quality

The test file provides good coverage of the middleware functions. To further enhance the quality and robustness of the tests, consider the following general improvements:

1. Consistency: Ensure a consistent structure across all test suites, including setup, execution, and assertions.

2. Error handling: Add more tests for error scenarios across all functions, including invalid inputs and missing required data.

3. Edge cases: Increase coverage of edge cases, such as empty arrays, null values, and boundary conditions.

4. Performance: Include tests with large datasets to ensure functions perform well at scale.

5. Mocking: Use mocking more extensively to isolate units under test and control test environments.

6. Parameterized tests: Consider using parameterized tests for scenarios that differ only in input data.

7. Cleanup: Ensure proper cleanup after each test to prevent inter-test dependencies.

8. Documentation: Add brief comments explaining the purpose of each test suite and any complex test scenarios.

Implementing these suggestions will result in a more comprehensive and maintainable test suite, increasing confidence in the robustness of the middleware functions.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9f8032f and aac6641.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (9)

• package.json (2 hunks)
• src/middleware/common.middleware.js (4 hunks)
• src/middleware/issueDetails.middleware.js (3 hunks)
• test/unit/issueDetailsPage.test.js (6 hunks)
• test/unit/middleware/common.middleware.test.js (1 hunks)
• test/unit/middleware/issueDetails.middleware.test.js (2 hunks)
• test/unit/middleware/issueTable.middleware.test.js (1 hunks)
• test/unit/views/organisations/issueTablePage.test.js (1 hunks)
• test/utils/mocker.js (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• package.json
• test/unit/middleware/issueTable.middleware.test.js
• test/unit/views/organisations/issueTablePage.test.js

🧰 Additional context used

🪛 Biome

src/middleware/issueDetails.middleware.js

[error] 26-26: Do not shadow the global "escape" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

🔇 Additional comments (11)

test/utils/mocker.js (2)

46-52: LGTM!

The enhanceMockedData function is well-implemented. It correctly checks for the presence of tableParams and applies the mockTableParams function when necessary, enhancing the generated mock data.

---

78-80: LGTM!

The modification to the main exported function is correct. It now properly applies the enhanceMockedData function to the generated data, ensuring that the enhanced data is returned.

src/middleware/issueDetails.middleware.js (5)

48-53: Improved security in issueErrorMessageHtml function

The changes to the issueErrorMessageHtml function effectively address previous security concerns related to XSS vulnerabilities. The use of the escape function ensures that user-generated content is properly sanitised before being rendered as HTML.

The function now also handles cases where errorMessage or issue might be undefined, improving its robustness.

---

62-71: Enhanced robustness in getIssueField function

The getIssueField function has been improved with the use of the nullish coalescing operator (??). This change enhances the function's ability to handle falsy values correctly, distinguishing between undefined and other falsy values like empty strings.

This modification aligns with modern JavaScript best practices and improves the overall robustness of the function.

---

74-82: Secure and well-structured pagination setup

The setPagePaginationOptions function effectively sets up the necessary pagination parameters based on the request data. The use of encodeURIComponent for constructing the urlSubPath is a crucial security measure, preventing potential URL injection attacks.

This function provides a solid foundation for handling pagination in the issue details page, ensuring both security and proper functionality.

---

Line range hint 158-182: Well-structured middleware pipeline

The default export array has been updated with several new middleware functions, improving the overall flow of data processing and preparation for the issue details page. The order of execution is logical, with data fetching and processing occurring before template preparation.

This restructuring aligns well with the changes observed in the imports and function definitions, creating a more comprehensive and efficient middleware pipeline.

---

Line range hint 1-182: Overall improvements in security, robustness, and code organisation

This update to the issueDetails.middleware.js file brings significant enhancements:

1. Improved input validation and error handling.
2. Enhanced security measures, particularly in preventing XSS vulnerabilities.
3. Better code organisation through the use of helper functions and logical structuring of the middleware pipeline.
4. Improved robustness in handling edge cases and potential undefined values.

These changes collectively contribute to a more secure, efficient, and maintainable codebase. The minor suggestions provided in the review comments, if implemented, will further refine the code quality.

test/unit/issueDetailsPage.test.js (2)

4-4: LGTM: Improved test utility organization and seed initialization.

The changes to import paths and seed initialization enhance consistency across the test suite. The use of getSeed() standardizes mock data generation, which is a positive improvement.

Also applies to: 7-7, 11-11

---

Line range hint 1-224: Overall assessment: Excellent improvements to the test suite.

The changes in this file significantly enhance the test coverage, consistency, and alignment with the application code. The modifications to error handling, pagination, and geometry processing are particularly noteworthy. The suggestions provided in the individual comments, if implemented, would further strengthen the robustness of the tests.

Great work on maintaining and improving the quality of the test suite!

test/unit/middleware/issueDetails.middleware.test.js (1)

38-127: Excellent test coverage for getIssueField function

The test suite for getIssueField is impressively comprehensive. It covers a wide range of scenarios, including:

• Valid inputs
• Missing optional parameters
• Null and undefined values for each parameter

This thorough approach ensures that the function behaves correctly under various conditions, enhancing the robustness of the codebase.

Well done on providing such detailed test coverage. This level of testing will greatly contribute to the reliability of the getIssueField function.

src/middleware/common.middleware.js (1)

44-44: Improved function naming and logic

The renaming of this function from isResourceIdInParams to isResourceIdNotInParams and the inversion of its logic make its purpose clearer and more intuitive. This change enhances code readability and maintainability.

[coderabbitai]

🛠️ Refactor suggestion

Consider alternative to delete for better performance

The use of the delete operator can impact performance as it modifies the object's structure. Consider using an alternative approach.

Instead of using delete, you could set the property to undefined:

 export const extractJsonFieldFromEntities = (req, res, next) => {
   const { entities } = req
 
   req.entities = entities.map(entity => {
     const jsonField = entity.json
     if (!jsonField || jsonField === '') {
       logger.info(`common.middleware/extractJsonField: No json field for entity ${entity.toString()}`)
       return entity
     }
-    delete entity.json
+    entity.json = undefined
     const parsedJson = JSON.parse(jsonField)
     entity = { ...entity, ...parsedJson }
     return entity
   })
 
   next()
 }

This approach is generally more performant and doesn't change the object's structure.

Committable suggestion was skipped due to low confidence.

[coderabbitai]

⚠️ Potential issue

SQL injection vulnerability in query

The current implementation uses string interpolation to construct the SQL query, which could lead to SQL injection attacks. It's crucial to use parameterised queries instead.

Consider refactoring the query to use parameterised input:

 export const fetchSpecification = fetchOne({
-  query: ({ req }) => `select * from specification WHERE specification = '${req.dataset.collection}'`,
+  query: ({ req }) => ({
+    text: 'SELECT * FROM specification WHERE specification = $1',
+    values: [req.dataset.collection]
+  }),
   result: 'specification'
 })

Ensure that the fetchOne function supports this parameterised query format.

📝 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.

Suggested change

	export const fetchSpecification = fetchOne({
	query: ({ req }) => `select * from specification WHERE specification = '${req.dataset.collection}'`,
	result: 'specification'
	})
	export const fetchSpecification = fetchOne({
	query: ({ req }) => ({
	text: 'SELECT * FROM specification WHERE specification = $1',
	values: [req.dataset.collection]
	}),
	result: 'specification'
	})

[coderabbitai]

🛠️ Refactor suggestion

Consider making results count configurable

The current implementation uses a fixed results count per page, which might not be flexible enough for different use cases or user preferences.

Consider making the results count configurable:

-export const getPaginationOptions = (resultsCount) => (req, res, next) => {
+export const getPaginationOptions = (defaultResultsCount = 10) => (req, res, next) => {
   const { pageNumber } = req.params
+  const resultsCount = req.query.resultsPerPage || defaultResultsCount
 
   req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
 
   next()
 }

This change allows the results count to be set via a query parameter, falling back to a default value if not specified. It provides more flexibility while maintaining backwards compatibility.

📝 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.

Suggested change

	export const getPaginationOptions = (resultsCount) => (req, res, next) => {
	const { pageNumber } = req.params
	req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
	next()
	}
	export const getPaginationOptions = (defaultResultsCount = 10) => (req, res, next) => {
	const { pageNumber } = req.params
	const resultsCount = req.query.resultsPerPage || defaultResultsCount
	req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
	next()
	}

[coderabbitai]

Actionable comments posted: 17

🧹 Outside diff range and nitpick comments (9)

src/middleware/issueDetails.middleware.js (3)

1-36: Approve changes with a minor suggestion

The updates to imports and the validation schema enhance the overall structure and type safety of the middleware. However, there's a minor concern:

The imported 'escape' function shadows the global 'escape' property. To improve clarity, consider renaming it:

-import escape from 'escape-html'
+import escapeHtml from 'escape-html'

Then update all occurrences of escape to escapeHtml in the rest of the file.

🧰 Tools

🪛 Biome

[error] 26-26: Do not shadow the global "escape" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

---

74-82: Approve with suggestion for improved error handling

The setPagePaginationOptions function is well-implemented, particularly in its use of encodeURIComponent for URL parameters, which helps prevent injection attacks. However, consider adding error handling for cases where req.params properties might be undefined:

export const setPagePaginationOptions = (req, res, next) => {
  const { entities } = req
  const { lpa, dataset: datasetId, issue_type: issueType, issue_field: issueField } = req.params

  if (!lpa || !datasetId || !issueType || !issueField) {
    return next(new Error('Missing required URL parameters'))
  }

  req.resultsCount = entities.length
  req.urlSubPath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(datasetId)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/`
  req.paginationPageLength = 1

  next()
}

This addition will make the function more robust against potential errors.

---

85-154: Approve with suggestion for safer property access

The prepareIssueDetailsTemplateParams function has been significantly improved. The new error handling for undefined entities and specification enhances robustness, and the use of helper functions improves code clarity.

However, there's a potential issue on line 132. To prevent potential runtime errors when accessing properties of undefined objects, consider using optional chaining and nullish coalescing:

-    valueHtml += escape(entity[datasetField]?.value || '')
+    valueHtml += escape(entity[datasetField]?.value ?? '')

This change ensures that valueHtml will be an empty string if entity[datasetField] is undefined, rather than the string 'undefined'.

test/unit/middleware/issueDetails.middleware.test.js (3)

10-36: Consider adding a test for empty string error message

The test suite for issueErrorMessageHtml is comprehensive, covering various scenarios including null values. However, it might be beneficial to add a test case for an empty string error message to ensure the function handles this edge case correctly.

Consider adding the following test:

it('should return an empty string if errorMessage is an empty string', () => {
  const issue = { value: '02-02-2022' }
  const result = issueErrorMessageHtml('', issue)
  expect(result).toBe('')
})

---

129-279: Consider adding a test for pagination object structure

The test suite for prepareIssueDetailsTemplateParams has been significantly improved with the addition of error handling tests. However, the test for the pagination object (lines 200-203) only checks if the object is set correctly. Consider adding a more detailed test to verify the structure of the pagination object.

You could add a test like this:

it('should set pagination on templateParams with correct structure', () => {
  const mockPagination = {
    items: [
      { current: true, href: '/page/1', number: 1, type: 'number' },
      { href: '/page/2', number: 2, type: 'number' }
    ],
    previous: { href: '/page/1' },
    next: { href: '/page/3' }
  }
  req.pagination = mockPagination
  prepareIssueDetailsTemplateParams(req, res, next)
  expect(req.templateParams.pagination).toEqual(mockPagination)
  expect(req.templateParams.pagination.items).toHaveLength(2)
  expect(req.templateParams.pagination.previous).toBeDefined()
  expect(req.templateParams.pagination.next).toBeDefined()
})

---

Line range hint 281-341: Consider expanding test coverage for getIssueDetails

The current test for getIssueDetails covers the basic functionality of rendering with the correct template and parameters. However, to ensure robustness, consider adding more test cases to cover different scenarios.

Some suggestions for additional tests:

1. Test with empty templateParams to ensure proper error handling.
2. Test with different issueType and issueField values to ensure they're correctly passed to the template.
3. Test with different pagination scenarios (e.g., first page, last page, middle page) to ensure correct rendering of pagination controls.

Example:

it('should handle empty templateParams', () => {
  const req = { templateParams: {} }
  const res = { render: vi.fn() }
  getIssueDetails(req, res, () => {})
  expect(res.render).toHaveBeenCalledWith('organisations/issueDetails.html', {})
})

src/services/performanceDbApi.js (3)

Line range hint 48-72: Use parameterised queries to prevent SQL injection

The current implementation uses string interpolation for resources and datasetId, which poses a SQL injection risk. Consider using parameterised queries to enhance security.

Refactor the query to use parameterised placeholders:

- i.resource in ('${resources.join("', '")}')
- AND i.dataset = '${datasetId}'
+ i.resource in (${resources.map((_, index) => `:resource${index}`).join(', ')})
+ AND i.dataset = :datasetId

When executing the query, pass the parameters:

const parameters = {
  ...resources.reduce((acc, resource, index) => ({ ...acc, [`resource${index}`]: resource }), {}),
  datasetId
};
const result = await datasette.runQuery(sql, 'digital-land', parameters);

---

348-352: Update JSDoc comment to reflect parameter change

The JSDoc comment for the getEntitiesWithIssuesCount function hasn't been updated to reflect the change in the resources parameter from a single string to an array of strings.

Update the JSDoc comment as follows:

- * @param {string} params.resource - Resource to filter by
+ * @param {string[]} params.resources - Resources to filter by

---

Line range hint 1-530: Enhance overall security and maintain consistency

The changes in this file significantly improve the API's flexibility and querying capabilities, aligning well with the PR objectives. However, there's a consistent pattern of potential SQL injection vulnerabilities due to the use of string interpolation in SQL queries.

1. Implement a consistent approach to parameterised queries across all functions in this file. This will greatly enhance the security of the API.

2. Consider creating a utility function for building parameterised queries to reduce code duplication and ensure consistency. For example:

function buildParameterisedQuery(baseQuery, params) {
  let query = baseQuery;
  const parameters = {};
  
  Object.entries(params).forEach(([key, value]) => {
    if (Array.isArray(value)) {
      const placeholders = value.map((_, index) => `:${key}${index}`).join(', ');
      query = query.replace(`:${key}`, `(${placeholders})`);
      value.forEach((item, index) => {
        parameters[`${key}${index}`] = item;
      });
    } else {
      parameters[key] = value;
    }
  });
  
  return { query, parameters };
}

3. Review and update the JSDoc comments for all modified functions to ensure they accurately reflect the current parameters and return values.

4. Consider adding unit tests for these new and modified functions to ensure they behave correctly with various input scenarios.

By addressing these points, you'll significantly improve the security and maintainability of this module.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between aac6641 and 1f34a01.

📒 Files selected for processing (5)

• src/middleware/common.middleware.js (4 hunks)
• src/middleware/issueDetails.middleware.js (3 hunks)
• src/services/performanceDbApi.js (6 hunks)
• test/unit/middleware/common.middleware.test.js (1 hunks)
• test/unit/middleware/issueDetails.middleware.test.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/unit/middleware/common.middleware.test.js

🧰 Additional context used

🪛 Biome

src/middleware/issueDetails.middleware.js

[error] 26-26: Do not shadow the global "escape" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)

🔇 Additional comments (10)

src/middleware/issueDetails.middleware.js (3)

48-53: LGTM: Proper error message handling and XSS prevention

The issueErrorMessageHtml function is well-implemented. It correctly handles null or undefined error messages and uses the escape function to prevent XSS attacks. Good job on prioritising security!

---

62-71: LGTM: Well-structured issue field object creation

The getIssueField function is well-implemented. It provides a consistent structure for issue fields and handles potential null or undefined inputs gracefully using the nullish coalescing operator. This approach enhances the robustness of the code.

---

Line range hint 166-191: LGTM: Well-structured middleware pipeline

The changes to the middleware array are well-thought-out and align with the modifications made throughout the file. The new middleware functions for data fetching and processing are logically ordered, creating a robust pipeline for handling issue details. This structure should improve the overall flow and maintainability of the code.

test/unit/middleware/issueDetails.middleware.test.js (1)

38-127: Comprehensive test coverage for getIssueField

The test suite for getIssueField is thorough and well-structured. It covers all possible combinations of null/undefined inputs and ensures the function returns the expected object structure in each case. No additional tests are necessary.

src/middleware/common.middleware.js (6)

44-44: Improved function naming and logic

The renaming of this function from isResourceIdInParams to isResourceIdNotInParams and the inversion of its logic make its purpose clearer and more consistent with its implementation. This change enhances code readability and maintainability.

---

170-176: 🛠️ Refactor suggestion

Consider making results count configurable

The current implementation uses a fixed results count per page, which might not be flexible enough for different use cases or user preferences.

Consider making the results count configurable:

-export const getPaginationOptions = (resultsCount) => (req, res, next) => {
+export const getPaginationOptions = (defaultResultsCount = 10) => (req, res, next) => {
   const { pageNumber } = req.params
+  const resultsCount = req.query.resultsPerPage || defaultResultsCount
 
   req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
 
   next()
 }

This change allows the results count to be set via a query parameter, falling back to a default value if not specified. It provides more flexibility while maintaining backwards compatibility.

Likely invalid or redundant comment.

---

272-284: 🛠️ Refactor suggestion

Consider optimising object creation

The current implementation creates a new object for each entity using the spread operator. This might be unnecessary if the original entity object can be modified in place.

Consider modifying the entities in place to avoid creating new objects:

 export const nestEntityFields = (req, res, next) => {
   const { entities, specification } = req
 
-  req.entities = entities.map(entity => {
+  entities.forEach(entity => {
     const columnHeaders = [...new Set(specification.fields.map(field => field.datasetField || field.field))]
     columnHeaders.forEach(field => {
-      entity[field] = { value: entity[field] }
+      if (typeof entity[field] !== 'object' || entity[field] === null) {
+        entity[field] = { value: entity[field] }
+      }
     })
-    return entity
   })
 
   next()
 }

This approach modifies the entities in place, which can be more efficient, especially for large datasets. It also checks if the field is already an object before nesting, preventing unnecessary nesting of already nested fields.

Likely invalid or redundant comment.

---

98-101: ⚠️ Potential issue

SQL injection vulnerability in query

The current implementation uses string interpolation to construct the SQL query, which could lead to SQL injection attacks. It's crucial to use parameterised queries instead.

Consider refactoring the query to use parameterised input:

 export const fetchSpecification = fetchOne({
-  query: ({ req }) => `select * from specification WHERE specification = '${req.dataset.collection}'`,
+  query: ({ req }) => ({
+    text: 'SELECT * FROM specification WHERE specification = $1',
+    values: [req.dataset.collection]
+  }),
   result: 'specification'
 })

Ensure that the fetchOne function supports this parameterised query format.

Likely invalid or redundant comment.

---

234-254: 🛠️ Refactor suggestion

Consider alternative to delete for better performance

The use of the delete operator can impact performance as it modifies the object's structure. Consider using an alternative approach.

Instead of using delete, you could set the property to undefined:

 export const extractJsonFieldFromEntities = (req, res, next) => {
   const { entities } = req
 
   req.entities = entities.map(entity => {
     const jsonField = entity.json
     if (!jsonField || jsonField === '') {
       logger.info(`common.middleware/extractJsonField: No json field for entity ${entity.toString()}`)
       return entity
     }
-    delete entity.json
+    entity.json = undefined
     try {
       const parsedJson = JSON.parse(jsonField)
       entity = { ...entity, ...parsedJson }
     } catch (err) {
       logger.warn(`common.middleware/extractJsonField: Error parsing JSON for entity ${entity.toString()}: ${err.message}`)
     }
     return entity
   })
 
   next()
 }

This approach is generally more performant and doesn't change the object's structure.

Likely invalid or redundant comment.

---

303-322: 🛠️ Refactor suggestion

Consider creating new entity objects to avoid side effects

The current implementation modifies entity objects directly. While efficient, this approach might lead to unintended side effects if the original entity objects are used elsewhere in the code.

Consider creating new entity objects instead:

 export const addIssuesToEntities = (req, res, next) => {
   const { entities, issuesWithReferences } = req
 
-  req.entitiesWithIssues = entities.map(entity => {
+  req.entitiesWithIssues = entities.map(originalEntity => {
+    const entity = { ...originalEntity }
     const entityIssues = issuesWithReferences.filter(issue => issue.entryNumber === entity.entryNumber)
 
     entityIssues.forEach(issue => {
       if (!entity[issue.datasetField]) {
         entity[issue.datasetField] = {}
       }
 
       entity[issue.datasetField].value = issue.value || entity[issue.datasetField].value || ''
       entity[issue.datasetField].issue = issue
     })
 
     return entity
   })
 
   next()
 }

This approach creates a new object for each entity, ensuring that the original entities remain unmodified. While slightly less efficient, it prevents potential bugs caused by unexpected mutations of shared objects.

Likely invalid or redundant comment.

[coderabbitai]

🛠️ Refactor suggestion

Consider simplifying key replacement logic

The current implementation replaces underscores with hyphens in entity keys, but it unnecessarily sets the old keys to undefined. This could potentially cause issues if the code elsewhere expects these keys to be completely absent.

Consider simplifying the logic:

 export const replaceUnderscoreWithHyphenForEntities = (req, res, next) => {
   const { entities } = req
 
   entities.forEach(entity => {
-    Object.keys(entity).forEach(key => {
+    const keys = Object.keys(entity)
+    keys.forEach(key => {
       if (key.includes('_')) {
         const newKey = key.replace(/_/g, '-')
         entity[newKey] = entity[key]
-        entity[key] = undefined
+        delete entity[key]
       }
     })
   })
 
   next()
 }

This approach completely removes the old keys instead of setting them to undefined, which is more in line with the function's purpose. It also avoids potential issues with undefined values.

📝 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.

Suggested change

	export const replaceUnderscoreWithHyphenForEntities = (req, res, next) => {
	const { entities } = req
	entities.forEach(entity => {
	Object.keys(entity).forEach(key => {
	if (key.includes('_')) {
	const newKey = key.replace(/_/g, '-')
	entity[newKey] = entity[key]
	entity[key] = undefined
	}
	})
	})
	next()
	}
	export const replaceUnderscoreWithHyphenForEntities = (req, res, next) => {
	const { entities } = req
	entities.forEach(entity => {
	const keys = Object.keys(entity)
	keys.forEach(key => {
	if (key.includes('_')) {
	const newKey = key.replace(/_/g, '-')
	entity[newKey] = entity[key]
	delete entity[key]
	}
	})
	})
	next()
	}

[coderabbitai]

⚠️ Potential issue

Add error handling for JSON parsing and null checks

The current implementation lacks error handling for JSON parsing and doesn't check if specification.json exists before parsing. This could lead to unhandled exceptions.

Consider adding error handling and null checks:

 export const pullOutDatasetSpecification = (req, res, next) => {
   const { specification } = req
-  let collectionSpecifications
-  try {
-    collectionSpecifications = JSON.parse(specification.json)
-  } catch (error) {
-    logger.error('Invalid JSON in specification.json', { error })
-    return next(new Error('Invalid specification format'))
+  if (!specification || !specification.json) {
+    return next(new Error('Specification or specification.json is missing'))
   }
-  const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset)
-  req.specification = datasetSpecification
-  next()
+  try {
+    const collectionSpecifications = JSON.parse(specification.json)
+    const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset)
+    req.specification = datasetSpecification
+    next()
+  } catch (error) {
+    next(new Error(`Failed to parse specification JSON: ${error.message}`))
+  }
 }

📝 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.

Suggested change

	export const pullOutDatasetSpecification = (req, res, next) => {
	const { specification } = req
	let collectionSpecifications
	try {
	collectionSpecifications = JSON.parse(specification.json)
	} catch (error) {
	logger.error('Invalid JSON in specification.json', { error })
	return next(new Error('Invalid specification format'))
	}
	const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset)
	req.specification = datasetSpecification
	next()
	}
	export const pullOutDatasetSpecification = (req, res, next) => {
	const { specification } = req
	if (!specification || !specification.json) {
	return next(new Error('Specification or specification.json is missing'))
	}
	try {
	const collectionSpecifications = JSON.parse(specification.json)
	const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset)
	req.specification = datasetSpecification
	next()
	} catch (error) {
	next(new Error(`Failed to parse specification JSON: ${error.message}`))
	}
	}

[coderabbitai]

💡 Codebase verification

Missing input validation and error handling

The fetchActiveResourcesForOrganisationAndDataset function is utilized in multiple middleware files without any visible input validation or error handling. Implementing input validation for params.lpa and params.dataset, along with proper error handling for the database queries, will enhance the robustness of the function and mitigate potential issues arising from invalid inputs or database failures.

🔗 Analysis chain

Consider adding input validation and error handling

The new fetchActiveResourcesForOrganisationAndDataset function is a good addition for fetching active resources. However, consider adding input validation for params.lpa and params.dataset, as well as error handling for the database query. This would make the function more robust and prevent potential issues with invalid inputs or database errors.

To verify the usage and potential error scenarios, you could run:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of fetchActiveResourcesForOrganisationAndDataset
rg -n "fetchActiveResourcesForOrganisationAndDataset" 

# Check if there's any error handling around its usage
rg -n "fetchActiveResourcesForOrganisationAndDataset" -A 5 -B 5

Length of output: 9229

---

Script:

#!/bin/bash
# Search for try-catch blocks around fetchActiveResourcesForOrganisationAndDataset
rg -n "try\s*{[^}]*fetchActiveResourcesForOrganisationAndDataset[^}]*}" 

# Search for .catch handlers following fetchActiveResourcesForOrganisationAndDataset
rg -n "fetchActiveResourcesForOrganisationAndDataset\([^)]*\)\.catch\(" 

Length of output: 300

---

Script:

#!/bin/bash
# Search for try-catch blocks around fetchActiveResourcesForOrganisationAndDataset
rg -n "try\s*\{[^}]*fetchActiveResourcesForOrganisationAndDataset[^}]*\}"

# Search for .catch handlers following fetchActiveResourcesForOrganisationAndDataset
rg -n "fetchActiveResourcesForOrganisationAndDataset\([^)]*\)\.catch\s*\("

Length of output: 152

[coderabbitai]

⚠️ Potential issue

Implement parameterised queries for security

The new function uses string interpolation for lpa and dataset, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Apply this change to use parameterised queries:

- WHERE REPLACE(ro.organisation, '-eng', '') = '${lpa}'
- AND pipeline = '${dataset}'
+ WHERE REPLACE(ro.organisation, '-eng', '') = :lpa
+ AND pipeline = :dataset

When calling the function, pass the parameters:

const parameters = { lpa, dataset };
const result = await datasette.runQuery(sql, 'digital-land', parameters);

[coderabbitai]

⚠️ Potential issue

Enhance security with parameterised queries

The function uses string interpolation for resources, issueType, and issueField, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Implement parameterised queries as follows:

- WHERE resource in ('${resources.join("',' ")}')
- AND issue_type = '${issueType}'
- AND field = '${issueField}'
+ WHERE resource in (${resources.map((_, index) => `:resource${index}`).join(', ')})
+ AND issue_type = :issueType
+ AND field = :issueField

When executing the query, pass the parameters:

const parameters = {
  ...resources.reduce((acc, resource, index) => ({ ...acc, [`resource${index}`]: resource }), {}),
  issueType,
  issueField
};
const result = await datasette.runQuery(sql, database, parameters);

[coderabbitai]

⚠️ Potential issue

Implement parameterised queries for security

The issuesWithReferenceFromResourcesDatasetIssueTypeFieldQuery function uses string interpolation for all parameters, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Implement parameterised queries as follows:

const query = `
  SELECT DISTINCT i.message, i.value, i.field, i.issue_type, i.entry_number, i.resource, f.value as reference
  FROM issue i
  LEFT JOIN fact_resource fr ON i.entry_number = fr.entry_number AND i.resource = fr.resource
  LEFT JOIN fact f ON fr.fact = f.fact
  WHERE i.resource IN (${resources.map((_, index) => `:resource${index}`).join(', ')})
  AND dataset = :dataset
  AND issue_type = :issueType
  AND i.field = :issueField
  AND f.field = 'reference'
`;

const parameters = {
  ...resources.reduce((acc, resource, index) => ({ ...acc, [`resource${index}`]: resource }), {}),
  dataset,
  issueType,
  issueField
};

return { query, parameters };

This approach allows for safe parameter handling while maintaining the query structure.

[coderabbitai]

⚠️ Potential issue

Enhance security with parameterised queries

The fetchIssuesWithoutReferences function uses string interpolation for all parameters, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Implement parameterised queries as follows:

const query = `
  SELECT DISTINCT i.message, i.value, i.field, i.issue_type, i.entry_number, i.resource, f.value as reference
  FROM issue i
  LEFT JOIN fact_resource fr ON i.entry_number = fr.entry_number AND i.resource = fr.resource
  LEFT JOIN fact f ON fr.fact = f.fact
  WHERE i.resource IN (${resources.map((_, index) => `:resource${index}`).join(', ')})
  AND dataset = :dataset
  AND issue_type = :issueType
  AND i.field = :issueField
  AND f.field IS NULL
`;

const parameters = {
  ...resources.reduce((acc, resource, index) => ({ ...acc, [`resource${index}`]: resource }), {}),
  dataset,
  issueType,
  issueField
};

return { query, parameters };

This approach allows for safe parameter handling while maintaining the query structure.

[coderabbitai]

⚠️ Potential issue

Implement parameterised queries and consider pagination

The fetchEntitiesFromEntryNumbers function uses string interpolation, which could lead to SQL injection vulnerabilities. Additionally, there's commented-out pagination code that should be addressed.

1. Implement parameterised queries for security:

const query = `
  SELECT DISTINCT f.entity, fr.entry_number, fr.resource, e.* FROM fact f
  LEFT JOIN fact_resource fr ON f.fact = fr.fact
  LEFT JOIN entity e ON f.entity = e.entity
  WHERE e.organisation_entity = :organisationEntity
  AND entry_number IN (${entryNumbers.map((_, index) => `:entryNumber${index}`).join(', ')})
`;

const parameters = {
  organisationEntity,
  ...entryNumbers.reduce((acc, num, index) => ({ ...acc, [`entryNumber${index}`]: num }), {})
};

return { query, parameters };

2. Regarding pagination:

The commented-out pagination code suggests that pagination might be implemented in the future. Consider adding a TODO comment explaining the plan for pagination or remove the commented code if it's no longer relevant.

// TODO: Implement pagination once we have a solution for counting all entities with issues

[coderabbitai]

⚠️ Potential issue

Enhance security with parameterised queries

The fetchEntityFromEntryNumber function uses string interpolation for organisationEntity and entryNumber, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Implement parameterised queries as follows:

const query = `
  SELECT DISTINCT f.entity, fr.entry_number, fr.resource, e.* FROM fact f
  LEFT JOIN fact_resource fr ON f.fact = fr.fact
  LEFT JOIN entity e ON f.entity = e.entity
  WHERE e.organisation_entity = :organisationEntity
  AND entry_number = :entryNumber
`;

const parameters = {
  organisationEntity,
  entryNumber
};

return { query, parameters };

This approach allows for safe parameter handling while maintaining the query structure.

[coderabbitai]

⚠️ Potential issue

Implement parameterised queries for security

The fetchEntitiesFromReferencesAndOrganisationEntity function uses string interpolation for references and organisationEntity, which could lead to SQL injection vulnerabilities. Refactor to use parameterised queries for improved security.

Implement parameterised queries as follows:

const query = `
  SELECT * FROM entity 
  WHERE reference IN (${references.map((_, index) => `:reference${index}`).join(', ')})
  AND organisation_entity = :organisationEntity
`;

const parameters = {
  ...references.reduce((acc, ref, index) => ({ ...acc, [`reference${index}`]: ref }), {}),
  organisationEntity
};

return { query, parameters };

This approach allows for safe parameter handling while maintaining the query structure.

[coderabbitai]

Actionable comments posted: 12

🧹 Outside diff range and nitpick comments (6)

src/middleware/issueTable.middleware.js (5)

34-45: Validation schema looks good, consider adding more specific constraints

The validation schema for IssueTableQueryParams is well-structured and includes appropriate type checking. The use of optional fields and transformations, particularly for pageNumber, is commendable.

To further enhance data integrity, consider adding more specific constraints to the required string fields. For example, you could add pattern matching for lpa and dataset if they follow specific formats.

Here's an example of how you might enhance the schema:

export const IssueTableQueryParams = v.object({
  lpa: v.string([v.minLength(1), v.maxLength(50)]),
  dataset: v.string([v.minLength(1), v.maxLength(100)]),
  issue_type: v.string([v.minLength(1), v.maxLength(50)]),
  issue_field: v.string([v.minLength(1), v.maxLength(50)]),
  // ... rest of the schema remains the same
})

This addition would ensure that these fields are not only strings but also have reasonable length constraints.

---

54-60: Entity page number addition looks good, consider adding a safeguard

The addEntityPageNumberToEntity function effectively adds a unique entityPageNumber to each entity in the request. The use of the spread operator to create a new object for each entity is a good practice for maintaining immutability.

To enhance robustness, consider adding a check to ensure req.entities exists and is an array before proceeding. This could prevent potential errors if the middleware is used in an unexpected context.

Here's a suggested improvement:

export const addEntityPageNumberToEntity = (req, res, next) => {
  if (Array.isArray(req.entities)) {
    req.entities = req.entities.map((entity, index) => ({ ...entity, entityPageNumber: index + 1 }))
  } else {
    console.warn('req.entities is not an array in addEntityPageNumberToEntity middleware')
  }
  next()
}

This addition would make the function more resilient to potential issues in the middleware chain.

---

73-88: Consider refactoring geometry extraction for improved readability and performance

The getGeometriesFromEntities function effectively extracts and filters geometries from entities. However, the nested if-else structure could be simplified for better readability and potentially improved performance.

Consider refactoring the function as follows:

export const getGeometriesFromEntities = (req, res, next) => {
  const { entities } = req

  const geometries = entities
    .map(entity => entity?.geometry?.value || entity?.point?.value || null)
    .filter(Boolean)

  req.geometries = geometries
  next()
}

This refactored version:

1. Uses the nullish coalescing operator (??) to simplify the logic.
2. Leverages short-circuit evaluation to reduce nesting.
3. Uses filter(Boolean) as a concise way to remove null values.

These changes make the code more concise and potentially more performant, as it reduces the number of condition checks.

The use of optional chaining in the original code was good practice for avoiding errors, which is maintained in this refactored version.

🧰 Tools

🪛 Biome

[error] 77-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 79-79: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

90-135: Consider optimising row construction and addressing potential undefined access

The prepareIssueTableTemplateParams function effectively prepares the necessary data for template rendering. The use of a Set for unique column headers is a good practice. However, there are a few areas where the function could be improved:

1. The nested loops for constructing rows could potentially be optimised for better performance, especially for large datasets.

2. There's a potential issue when accessing entity[field].value and entity[field].issue. If entity[field] is undefined, this could lead to runtime errors.

Consider refactoring the row construction as follows:

const rows = entities.map((entity, index) => ({
  columns: Object.fromEntries(columnHeaders.map(field => {
    if (field === 'reference') {
      const entityLink = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(datasetId)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/entry/${entity.entityPageNumber}`
      return [field, { html: `<a href="${entityLink}">${entity[field]?.value || ''}</a>`, error: entity[field]?.issue }]
    }
    return [field, { value: entity[field]?.value || '', error: entity[field]?.issue }]
  }))
}))

This refactored version:

1. Uses Object.fromEntries and map to construct the columns object, which can be more efficient than forEach.
2. Uses optional chaining (?.) to safely access potentially undefined properties.
3. Provides default empty string values to prevent undefined from being rendered.

The use of template literals for constructing URLs is good for readability and maintainability.

---

143-173: Middleware sequence looks comprehensive, consider adding documentation

The default export array of middleware functions presents a logical and comprehensive sequence of operations. It effectively covers all necessary steps from initial validation to final error logging. The use of fetchIf for conditional execution is a good practice for optimising performance.

To enhance maintainability and make it easier for other developers to understand the flow, consider adding a brief comment before each middleware function or group of related functions. This documentation could explain the purpose of each step in the sequence.

For example:

export default [
  // Validate and prepare request parameters
  validateIssueTableQueryParams,
  setDefaultQueryParams,

  // Fetch necessary information
  fetchOrgInfo,
  fetchDatasetInfo,
  fetchIf(isResourceIdNotInParams, fetchLatestResource, takeResourceIdFromParams),

  // Process and transform data
  fetchSpecification,
  pullOutDatasetSpecification,
  // ... (continue with brief comments for each group)

  // Prepare and render the response
  prepareIssueTableTemplateParams,
  getIssueTable,
  logPageError
]

This addition would make the code more self-documenting and easier to maintain in the long run.

src/middleware/common.middleware.js (1)

103-115: Enhance error handling and add null checks.

The pullOutDatasetSpecification function has good error handling for JSON parsing, but it could be improved. Consider adding null checks for specification and specification.json, and provide more specific error messages.

Here's a suggested improvement:

 export const pullOutDatasetSpecification = (req, res, next) => {
   const { specification } = req
+  if (!specification || !specification.json) {
+    return next(new Error('Specification or specification.json is missing'))
+  }
   let collectionSpecifications
   try {
     collectionSpecifications = JSON.parse(specification.json)
   } catch (error) {
-    logger.error('Invalid JSON in specification.json', { error })
-    return next(new Error('Invalid specification format'))
+    logger.error('Invalid JSON in specification.json', { error, specification: specification.json })
+    return next(new Error(`Invalid specification format: ${error.message}`))
   }
   const datasetSpecification = collectionSpecifications.find((spec) => spec.dataset === req.dataset.dataset)
+  if (!datasetSpecification) {
+    return next(new Error(`No specification found for dataset: ${req.dataset.dataset}`))
+  }
   req.specification = datasetSpecification
   next()
 }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1f34a01 and 1acd964.

📒 Files selected for processing (4)

• src/middleware/common.middleware.js (4 hunks)
• src/middleware/issueTable.middleware.js (1 hunks)
• test/unit/middleware/common.middleware.test.js (1 hunks)
• test/unit/sharedTests/tableTests.js (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• test/unit/middleware/common.middleware.test.js
• test/unit/sharedTests/tableTests.js

🧰 Additional context used

🪛 Biome

src/middleware/issueTable.middleware.js

[error] 77-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 79-79: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🔇 Additional comments (6)

src/middleware/issueTable.middleware.js (3)

47-52: Default query params setter looks good

The setDefaultQueryParams function effectively ensures that pagination always starts from page 1 if not specified in the request parameters. This is a simple yet crucial functionality for maintaining consistent behaviour in the application.

The use of the next() callback at the end of the function is correct, ensuring that the middleware chain continues to execute after this function completes its task.

---

137-141: Issue table rendering setup looks good

The getIssueTable function effectively sets up the rendering of the issue table using the renderTemplate utility. This approach suggests a consistent method for rendering across the application, which is a good practice for maintaining code consistency and reusability.

The function is concise and clearly specifies its purpose by providing the template parameters, template file, and handler name. This makes the code easy to understand and maintain.

---

1-173: Overall, well-structured middleware with room for minor improvements

This middleware file for handling issue table queries is well-structured and comprehensive. It effectively covers all necessary steps from request validation to response rendering, utilising a composable middleware pattern that enhances maintainability and readability.

Key strengths:

1. Clear separation of concerns with distinct functions for each step.
2. Effective use of validation schemas and utility functions.
3. Thoughtful error handling and logging.

Areas for potential improvement:

1. Enhance URL parameter encoding to prevent XSS vulnerabilities.
2. Optimise nested loops in data transformation functions for better performance.
3. Add safeguards against potential undefined property access.
4. Consider making some hardcoded values (like pagination length) configurable.
5. Improve documentation, especially for the middleware sequence.

Overall, this is a solid implementation that forms a robust foundation for handling issue table queries. Addressing the suggested improvements will further enhance its security, performance, and maintainability.

🧰 Tools

🪛 Biome

[error] 77-77: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

---

[error] 79-79: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/middleware/common.middleware.js (3)

260-274: 🛠️ Refactor suggestion

Consider simplifying key replacement logic.

The current implementation replaces underscores with hyphens in entity keys, but it unnecessarily sets the old keys to undefined. This could potentially cause issues if the code elsewhere expects these keys to be completely absent.

Consider simplifying the logic:

 export const replaceUnderscoreWithHyphenForEntities = (req, res, next) => {
   const { entities } = req
 
   entities.forEach(entity => {
-    Object.keys(entity).forEach(key => {
+    const keys = Object.keys(entity)
+    keys.forEach(key => {
       if (key.includes('_')) {
         const newKey = key.replace(/_/g, '-')
         entity[newKey] = entity[key]
-        entity[key] = undefined
+        delete entity[key]
       }
     })
   })
 
   next()
 }

This approach completely removes the old keys instead of setting them to undefined, which is more in line with the function's purpose. It also avoids potential issues with undefined values.

Likely invalid or redundant comment.

---

98-101: ⚠️ Potential issue

Fix potential SQL injection vulnerability.

The fetchSpecification function uses string interpolation in the SQL query, which could lead to SQL injection attacks. Consider using parameterised queries instead.

Apply this change to fix the vulnerability:

 export const fetchSpecification = fetchOne({
-  query: ({ req }) => `select * from specification WHERE specification = '${req.dataset.collection}'`,
+  query: ({ req }) => ({
+    text: 'SELECT * FROM specification WHERE specification = $1',
+    values: [req.dataset.collection]
+  }),
   result: 'specification'
 })

Likely invalid or redundant comment.

---

312-331: 🛠️ Refactor suggestion

Consider optimising array creation.

The current implementation creates a new array of entities using map. This might be unnecessary if the original entities array can be modified in place.

Consider modifying the entities in place to avoid creating a new array:

 export const addIssuesToEntities = (req, res, next) => {
   const { entities, issuesWithReferences } = req
 
-  req.entitiesWithIssues = entities.map(entity => {
+  entities.forEach(entity => {
     const entityIssues = issuesWithReferences.filter(issue => issue.entryNumber === entity.entryNumber)
 
     entityIssues.forEach(issue => {
       if (!entity[issue.datasetField]) {
         entity[issue.datasetField] = {}
       }
 
       entity[issue.datasetField].value = issue.value || entity[issue.datasetField].value || ''
       entity[issue.datasetField].issue = issue
     })
-
-    return entity
   })
 
+  req.entitiesWithIssues = entities
   next()
 }

This approach modifies the entities in place, which can be more efficient, especially for large datasets. It avoids creating a new array and new objects for each entity.

Likely invalid or redundant comment.

[coderabbitai]

🛠️ Refactor suggestion

Consider making pagination page length configurable

The paginationPageLength is currently hardcoded to 20. To improve flexibility, consider making this value configurable, perhaps through environment variables or a configuration file. This would allow for easier adjustments based on different use cases or performance requirements.

You could implement this by:

1. Creating a configuration file (e.g., config.js) if it doesn't already exist.
2. Moving the paginationPageLength to this file.
3. Importing the value in this middleware file.

Example:

// In config.js
export const PAGINATION_PAGE_LENGTH = process.env.PAGINATION_PAGE_LENGTH || 20;

// In this file
import { PAGINATION_PAGE_LENGTH } from '../config.js';

const paginationPageLength = PAGINATION_PAGE_LENGTH;

This approach allows for easy configuration changes without modifying the code.

[coderabbitai]

⚠️ Potential issue

Improve URL parameter encoding to prevent potential XSS

The setPagePageOptions function effectively sets up pagination options and constructs the URL subpath. However, the current implementation of URL construction could be vulnerable to XSS attacks if the parameters contain malicious content.

To mitigate this risk, ensure all URL parameters are properly encoded. Here's an improved version of the URL construction:

req.urlSubPath = `/organisations/${encodeURIComponent(lpa)}/${encodeURIComponent(datasetId)}/${encodeURIComponent(issueType)}/${encodeURIComponent(issueField)}/`

This change ensures that any special characters in the parameters are properly encoded, preventing potential XSS vulnerabilities.

The use of a higher-order function to allow flexible page length configuration is a good design choice, promoting reusability and configurability.

[coderabbitai]

🛠️ Refactor suggestion

Consider refactoring for improved maintainability.

The formatErrorSummaryParams function handles various scenarios well, but its complexity might make it difficult to maintain and test. Consider breaking it down into smaller, more focused functions.

You could refactor the function like this:

1. Create a helper function to generate issue items:

   function generateIssueItems(entities, issueType, issueField, BaseSubpath) {
     // Logic for generating issue items
   }

2. Create a helper function to determine the error heading:

   function determineErrorHeading(totalIssues, entityCount, issueType, issueField) {
     // Logic for determining error heading
   }

3. Use these helper functions in the main formatErrorSummaryParams function:

   export function formatErrorSummaryParams(req, res, next) {
     // ... (existing code for extracting parameters)
     
     const errorHeading = determineErrorHeading(totalIssues, entityCount, issueType, issueField)
     const issueItems = generateIssueItems(entities, issueType, issueField, BaseSubpath)
     
     req.errorSummary = {
       heading: errorHeading,
       items: issueItems
     }
     next()
   }

This refactoring would make the function easier to understand, maintain, and test.

[coderabbitai]

🛠️ Refactor suggestion

Consider database-level pagination for improved performance.

The current implementation loads all entities into memory before slicing, which might not be efficient for large datasets. For better performance and scalability, consider implementing pagination at the database level.

You could modify the database query to include LIMIT and OFFSET clauses:

export const fetchPaginatedEntities = fetchMany({
  query: ({ req }) => `
    SELECT * FROM entities
    WHERE ... -- Your existing conditions
    LIMIT ${req.pagination.limit} OFFSET ${req.pagination.offset}
  `,
  result: 'entities'
})

Then, modify this function to use the paginated results:

export const paginateEntitiesAndPullOutCount = (req, res, next) => {
  const { entities, pagination } = req
  req.entitiesWithIssuesCount = req.totalEntitiesCount // Fetch this separately
  req.entities = entities // These are already paginated
  next()
}

This approach would be more efficient for large datasets and reduce memory usage.

[coderabbitai]

🛠️ Refactor suggestion

Consider refactoring for improved readability.

While the function handles pagination link generation well, its length and complexity might make it difficult to maintain and understand at a glance.

Consider breaking down the function into smaller, more focused helper functions:

1. Create a helper function for generating previous/next links:

   function generateNavigationLinks(pageNumber, totalPages, urlSubPath) {
     // Logic for generating previous and next links
   }

2. Create a helper function for generating pagination items:

   function generatePaginationItems(totalPages, pageNumber, urlSubPath) {
     // Logic for generating pagination items
   }

3. Use these helper functions in the main createPaginationTemplateParams function:

   export const createPaginationTemplateParams = (req, res, next) => {
     const { resultsCount, urlSubPath, paginationPageLength } = req
     let { pageNumber } = req.params
     pageNumber = parseInt(pageNumber)
   
     const totalPages = Math.ceil(resultsCount / paginationPageLength)
   
     const navigationLinks = generateNavigationLinks(pageNumber, totalPages, urlSubPath)
     const paginationItems = generatePaginationItems(totalPages, pageNumber, urlSubPath)
   
     req.pagination = {
       ...navigationLinks,
       items: paginationItems
     }
   
     next()
   }

This refactoring would make the function easier to understand and maintain, while also making the individual parts more reusable.

[coderabbitai]

🛠️ Refactor suggestion

Consider making results count configurable.

The current implementation uses a fixed results count per page, which might not be flexible enough for different use cases or user preferences.

Consider making the results count configurable:

-export const getPaginationOptions = (resultsCount) => (req, res, next) => {
+export const getPaginationOptions = (defaultResultsCount = 10) => (req, res, next) => {
   let pageNumber = parseInt(req.params.pageNumber, 10) || 1
 
   if (pageNumber <= 0) {
     pageNumber = 1
   }
 
+  const resultsCount = req.query.resultsPerPage || defaultResultsCount
   req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
 
   next()
 }

This change allows the results count to be set via a query parameter, falling back to a default value if not specified. It provides more flexibility while maintaining backwards compatibility.

📝 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.

Suggested change

	export const getPaginationOptions = (resultsCount) => (req, res, next) => {
	let pageNumber = parseInt(req.params.pageNumber, 10) || 1
	if (pageNumber <= 0) {
	pageNumber = 1
	}
	req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
	next()
	}
	export const getPaginationOptions = (defaultResultsCount = 10) => (req, res, next) => {
	let pageNumber = parseInt(req.params.pageNumber, 10) || 1
	if (pageNumber <= 0) {
	pageNumber = 1
	}
	const resultsCount = req.query.resultsPerPage || defaultResultsCount
	req.pagination = { offset: (pageNumber - 1) * resultsCount, limit: resultsCount }
	next()
	}

[coderabbitai]

🛠️ Refactor suggestion

Consider optimising object creation.

The current implementation creates a new object for each entity using the spread operator. This might be unnecessary if the original entity object can be modified in place.

Consider modifying the entities in place to avoid creating new objects:

 export const nestEntityFields = (req, res, next) => {
   const { entities, specification } = req
 
   if (!specification) {
     const error = new Error('Specification is not defined')
     return next(error)
   }
 
-  req.entities = entities.map(entity => {
+  entities.forEach(entity => {
     const columnHeaders = [...new Set(specification.fields.map(field => field.datasetField || field.field))]
     columnHeaders.forEach(field => {
-      entity[field] = { value: entity[field] }
+      if (typeof entity[field] !== 'object' || entity[field] === null) {
+        entity[field] = { value: entity[field] }
+      }
     })
-    return entity
   })
 
   next()
 }

This approach modifies the entities in place, which can be more efficient, especially for large datasets. It also checks if the field is already an object before nesting, preventing unnecessary nesting of already nested fields.

📝 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.

Suggested change

	export const nestEntityFields = (req, res, next) => {
	const { entities, specification } = req
	if (!specification) {
	const error = new Error('Specification is not defined')
	return next(error)
	}
	req.entities = entities.map(entity => {
	const columnHeaders = [...new Set(specification.fields.map(field => field.datasetField || field.field))]
	columnHeaders.forEach(field => {
	entity[field] = { value: entity[field] }
	})
	return entity
	})
	next()
	}
	export const nestEntityFields = (req, res, next) => {
	const { entities, specification } = req
	if (!specification) {
	const error = new Error('Specification is not defined')
	return next(error)
	}
	entities.forEach(entity => {
	const columnHeaders = [...new Set(specification.fields.map(field => field.datasetField || field.field))]
	columnHeaders.forEach(field => {
	if (typeof entity[field] !== 'object' || entity[field] === null) {
	entity[field] = { value: entity[field] }
	}
	})
	})
	next()
	}

[coderabbitai]

🛠️ Refactor suggestion

Consider creating new entity objects to avoid side effects.

The current implementation modifies entity objects directly. While efficient, this approach might lead to unintended side effects if the original entity objects are used elsewhere in the code.

Consider creating new entity objects instead:

 export const extractJsonFieldFromEntities = (req, res, next) => {
   const { entities } = req
 
-  req.entities = entities.map(entity => {
+  req.entities = entities.map(originalEntity => {
+    const entity = { ...originalEntity }
     const jsonField = entity.json
     if (!jsonField || jsonField === '') {
       logger.info(`common.middleware/extractJsonField: No json field for entity ${entity.toString()}`)
       return entity
     }
-    entity.json = undefined
+    delete entity.json
     try {
       const parsedJson = JSON.parse(jsonField)
-      entity = { ...entity, ...parsedJson }
+      return { ...entity, ...parsedJson }
     } catch (err) {
       logger.warn(`common.middleware/extractJsonField: Error parsing JSON for entity ${entity.toString()}: ${err.message}`)
+      return entity
     }
-    return entity
   })
 
   next()
 }

This approach creates a new object for each entity, ensuring that the original entities remain unmodified. While slightly less efficient, it prevents potential bugs caused by unexpected mutations of shared objects.

📝 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.

Suggested change

	export const extractJsonFieldFromEntities = (req, res, next) => {
	const { entities } = req
	req.entities = entities.map(entity => {
	const jsonField = entity.json
	if (!jsonField || jsonField === '') {
	logger.info(`common.middleware/extractJsonField: No json field for entity ${entity.toString()}`)
	return entity
	}
	entity.json = undefined
	try {
	const parsedJson = JSON.parse(jsonField)
	entity = { ...entity, ...parsedJson }
	} catch (err) {
	logger.warn(`common.middleware/extractJsonField: Error parsing JSON for entity ${entity.toString()}: ${err.message}`)
	}
	return entity
	})
	next()
	}
	export const extractJsonFieldFromEntities = (req, res, next) => {
	const { entities } = req
	req.entities = entities.map(originalEntity => {
	const entity = { ...originalEntity }
	const jsonField = entity.json
	if (!jsonField || jsonField === '') {
	logger.info(`common.middleware/extractJsonField: No json field for entity ${entity.toString()}`)
	return entity
	}
	delete entity.json
	try {
	const parsedJson = JSON.parse(jsonField)
	return { ...entity, ...parsedJson }
	} catch (err) {
	logger.warn(`common.middleware/extractJsonField: Error parsing JSON for entity ${entity.toString()}: ${err.message}`)
	return entity
	}
	})
	next()
	}

[coderabbitai]

🛠️ Refactor suggestion

Consider optimising array creation.

The current implementation creates a new array of issues using map. This might be unnecessary if the original issues array can be modified in place.

Consider modifying the issues in place to avoid creating a new array:

 export const addDatasetFieldsToIssues = (req, res, next) => {
   const { issuesWithReferences, specification } = req
 
-  req.issuesWithReferences = issuesWithReferences.map(issue => {
+  issuesWithReferences.forEach(issue => {
     let datasetField
     if (issue.field === 'GeoX,GeoY') { // special case for brownfield land
       datasetField = 'point'
     } else {
       const specificationEntry = specification.fields.find(field => field.field === issue.field)
       datasetField = specificationEntry ? specificationEntry.datasetField : specificationEntry?.field || issue.field
     }
-    return { ...issue, datasetField }
+    issue.datasetField = datasetField
   })
 
   next()
 }

This approach modifies the issues in place, which can be more efficient, especially for large datasets. It avoids creating a new array and new objects for each issue.

📝 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.

Suggested change

	export const addDatasetFieldsToIssues = (req, res, next) => {
	const { issuesWithReferences, specification } = req
	req.issuesWithReferences = issuesWithReferences.map(issue => {
	let datasetField
	if (issue.field === 'GeoX,GeoY') { // special case for brownfield land
	datasetField = 'point'
	} else {
	const specificationEntry = specification.fields.find(field => field.field === issue.field)
	datasetField = specificationEntry ? specificationEntry.datasetField : specificationEntry?.field || issue.field
	}
	return { ...issue, datasetField }
	})
	next()
	}
	export const addDatasetFieldsToIssues = (req, res, next) => {
	const { issuesWithReferences, specification } = req
	issuesWithReferences.forEach(issue => {
	let datasetField
	if (issue.field === 'GeoX,GeoY') { // special case for brownfield land
	datasetField = 'point'
	} else {
	const specificationEntry = specification.fields.find(field => field.field === issue.field)
	datasetField = specificationEntry ? specificationEntry.datasetField : specificationEntry?.field || issue.field
	}
	issue.datasetField = datasetField
	})
	next()
	}

[coderabbitai]

💡 Codebase verification

Consider adding error handling around fetchActiveResourcesForOrganisationAndDataset usage.

Error handling appears to be missing where fetchActiveResourcesForOrganisationAndDataset is utilised. Implementing error handling in the following locations will enhance the robustness of the function's usage:

• src/middleware/issueTable.middleware.js:20
• src/middleware/issueDetails.middleware.js:175
• src/middleware/common.middleware.js:55
• src/middleware/datasetOverview.middleware.js:167
• src/middleware/datasetTaskList.middleware.js:121

🔗 Analysis chain

LGTM! Consider adding error handling.

The new import and function fetchActiveResourcesForOrganisationAndDataset are good additions for fetching active resources. However, consider adding error handling to make the function more robust.

To verify the usage and potential error scenarios, you could run:

Also applies to: 55-58

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of fetchActiveResourcesForOrganisationAndDataset
rg -n "fetchActiveResourcesForOrganisationAndDataset" 

# Check if there's any error handling around its usage
rg -n "fetchActiveResourcesForOrganisationAndDataset" -A 5 -B 5

Length of output: 9229

[alextea]

Back link text should change to "Return to table view"

Current:

Proposed:

[alextea]

Column headings in the table should be lower / kebab case, like the record view:

[alextea]

Error messages should be updated using this PR #567