Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set dataset status to 'Error' if any active endpoints have 'Error' status #716

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

rosado
Copy link
Collaborator

@rosado rosado commented Dec 4, 2024

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

  • Feature

Description

On LPA overview page, we want to show datasets' status as 'Error' when any active endpoints are in error state. To do it we fetch extra info from provision_summary table.

Related Tickets & Documents

Note: this should be merged after #714

QA Instructions, Screenshots, Recordings

Example: http://submit.planning.data.gov.uk/organisations/local-authority:ROH

Before

SCR-20241204-ktvw

After

SCR-20241204-ktxy

Added/updated tests?

  • Yes

QA sign off

  • Code has been checked and approved
  • Design has been checked and approved
  • Product and business logic has been checked and proved

Summary by CodeRabbit

  • New Features

    • Introduced functionality to retrieve datasets with error statuses.
  • Bug Fixes

    • Updated dataset status handling to reflect errors accurately.
  • Tests

    • Added new test cases to ensure dataset status updates correctly based on error conditions.
    • Streamlined test setup with a new request template for improved clarity and maintainability.

@rosado rosado added the enhancement New feature or request label Dec 4, 2024

This comment was marked as outdated.

Copy link

github-actions bot commented Dec 4, 2024

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 70.52% 5212 / 7390
🔵 Statements 70.52% 5212 / 7390
🔵 Functions 68.72% 211 / 307
🔵 Branches 83.17% 687 / 826
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/middleware/overview.middleware.js 88.98% 85.45% 54.54% 88.98% 11-13, 25, 33, 46, 54-56, 69-73, 119-121, 127-129, 164-166, 176-178, 216-217, 267-276
src/services/performanceDbApi.js 63.5% 92.59% 33.33% 63.5% 49-77, 114-116, 179-183, 234-240, 251-265, 275-284, 293-313, 317-325, 334-346, 357-384, 394-399
Generated in workflow #471 for commit e471a6b by the Vitest Coverage Report Action

@rosado rosado marked this pull request as ready for review December 10, 2024 10:22
Copy link
Contributor

@DilwoarH DilwoarH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good - can we get product sign off on this? cc @CharliePatterson

@CharliePatterson
Copy link
Contributor

LGTM

Base automatically changed from rosado/659-default-error-status-hint to main December 12, 2024 14:57
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
src/middleware/overview.middleware.js (1)

284-290: Consider caching the Set lookup result.

While the implementation is correct, for better performance, consider caching the Set lookup result when iterating through datasets:

  const datasetsWithEndpointErrors = new Set(datasetErrorStatus.map(item => item.dataset))
  for (const dataset of datasets) {
    dataset.project = provisionData.get(dataset.dataset)?.project
-   if (dataset.status !== 'Error' && datasetsWithEndpointErrors.has(dataset.dataset)) {
+   const hasEndpointErrors = datasetsWithEndpointErrors.has(dataset.dataset)
+   if (dataset.status !== 'Error' && hasEndpointErrors) {
      dataset.status = 'Error'
    }
  }
test/unit/middleware/overview.middleware.test.js (1)

106-121: Consider adding edge case tests.

While the current test is good, consider adding these test cases:

  1. Multiple datasets with errors
  2. Empty datasetErrorStatus array
  3. Dataset already in error state
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa6942c and 23aabf3.

📒 Files selected for processing (3)
  • src/middleware/overview.middleware.js (4 hunks)
  • src/services/performanceDbApi.js (1 hunks)
  • test/unit/middleware/overview.middleware.test.js (8 hunks)
🔇 Additional comments (3)
src/middleware/overview.middleware.js (2)

39-50: LGTM! Well-structured middleware implementation.

The middleware function is correctly implemented using the fetchMany builder and properly integrated into the middleware chain.


293-296: LGTM! Efficient statistics calculation.

The use of array reduce for calculating multiple statistics in a single pass is an excellent approach.

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

21-64: LGTM! Well-structured test template.

The request template provides a comprehensive set of test data and follows good practices for test setup.

src/services/performanceDbApi.js Outdated Show resolved Hide resolved
@eveleighoj eveleighoj temporarily deployed to submit-pr-716 December 12, 2024 15:21 Inactive
@rosado rosado merged commit a0b54a8 into main Dec 12, 2024
5 checks passed
@rosado rosado deleted the rosado/671-mutliple-endpoints-error-state branch December 12, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Endpoint erroring state if dataset has multiple endpoints
4 participants