openems icon indicating copy to clipboard operation
openems copied to clipboard

Fix Storage Chart

Open Sn0w3y opened this issue 1 year ago • 7 comments

🛠️ Fix Data Fetching, Conditional Logic, and Runtime Error in History Charts

Overview

This PR addresses critical issues in the AbstractHistoryChart and SingleChartComponent related to data fetching, conditional logic, and a runtime error. These fixes ensure that historical timeseries data is accurately retrieved and processed, and that the application handles data conditions correctly to prevent crashes.

Issues Addressed

  1. Incorrect Data Fetching Logic

    • Problem: The queryHistoricTimeseriesData method was improperly returning empty data regardless of the API response, causing charts to display undefined or empty datasets.
    • Solution: Refactored the method to properly handle asynchronous API responses, ensuring that actual data is returned and errors are handled gracefully.
  2. Faulty Conditional Statement

    • Problem: The condition
      if ("_sum/EssActivePowerL1" && "_sum/EssActivePowerL2" && "_sum/EssActivePowerL3" in result.data && this.showPhases == true) {
      
      was always evaluating to true due to operator precedence, leading to logical errors.
    • Solution: Updated the condition to explicitly check each channel using the in operator:
      if ("_sum/EssActivePowerL1" in result.data && "_sum/EssActivePowerL2" in result.data && "_sum/EssActivePowerL3" in result.data && this.showPhases === true) {
      
  3. Runtime Error in SingleChartComponent

    • Problem: Encountered a runtime error:
      singlechart.component.ts:184 TypeError: Cannot read properties of undefined (reading 'forEach')
          at singlechart.component.ts:89:57
          at _ZoneDelegate.invoke (zone.js:369:28)
          ...
      
      This was caused by attempting to call forEach on an undefined property, likely due to missing data validation.
    • Solution: Added checks to ensure that the data arrays are defined before invoking forEach. Enhanced error handling to initialize empty charts gracefully when data is missing.

Changes Made

  • Refactored queryHistoricTimeseriesData Method:

    • Removed the immediate return of empty data.
    • Ensured the method resolves with actual API responses.
    • Implemented proper error handling by rejecting promises on failures.
    • Added a stale response check to prevent race conditions.
  • Fixed Conditional Logic:

    • Rewrote the if statement to individually check each required channel's existence in result.data.
    • Used strict equality (===) for this.showPhases to ensure type-safe comparison.
  • Resolved Runtime Error:

    • Added null and undefined checks before calling forEach on data arrays.
    • Ensured that result.data contains the necessary properties before processing.
    • Updated error handling to log meaningful messages and prevent application crashes.

Why These Changes?

  • Accurate Data Representation: Ensures charts display real and complete historical data, improving reliability.
  • Elimination of Logical Errors: Correct conditional checks prevent unintended code execution, maintaining data integrity.
  • Enhanced Error Handling: Properly managing API errors and runtime exceptions prevents the application from crashing and aids in debugging.
  • Improved Code Quality: Addressing TypeScript warnings and runtime errors leads to cleaner, more maintainable code.

Testing

  • Manual Testing:
    • Verified that charts now display correct data when API responses are received.
    • Confirmed that conditional statements behave as expected without triggering TypeScript warnings.
    • Reproduced and fixed the runtime error by ensuring data arrays are defined before processing.
    • Tested error scenarios to ensure charts initialize gracefully without breaking the application.

Please review the changes and approve the merge to enhance the reliability and accuracy of our history chart components.

Sn0w3y avatar Oct 03 '24 23:10 Sn0w3y

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #2828   +/-   ##
==========================================
  Coverage      56.01%   56.01%           
  Complexity      8197     8197           
==========================================
  Files           2103     2103           
  Lines          89017    89017           
  Branches        6560     6560           
==========================================
  Hits           49854    49854           
+ Misses         37445    37442    -3     
- Partials        1718     1721    +3     
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Oct 03 '24 23:10 codecov[bot]

@lukasrgr could you review?

Sn0w3y avatar Oct 06 '24 00:10 Sn0w3y

@lukasrgr any news ?

Sn0w3y avatar Oct 30 '24 15:10 Sn0w3y

@Sn0w3y i am currently working on this issue as well. Your changes in singlechart and totalchart component look fine to me, but there are some changes in abstracthistorychart i solved differently. Anyways thanks for this contribution.

fabian94533 avatar Nov 12 '24 11:11 fabian94533

@Sn0w3y sorry for my late response, im working on refactoring the storage chart anyway, than we dont have to touch this implementation of the abstracthistorychart

lukasrgr avatar Nov 25 '24 12:11 lukasrgr

Has this already been fixed in a PR ?

Sn0w3y avatar Jan 20 '25 14:01 Sn0w3y

Has this already been fixed in a PR ?

One of our colleagues is currently working on refactoring the AbstractHistoryChart. Unfortunately, I don't know how long it will take to complete. The other two changed files can be added.

fabian94533 avatar Jan 24 '25 10:01 fabian94533

This PR has been automatically marked as stale due to inactivity. It will be closed in 7 days if no further activity occurs.

github-actions[bot] avatar Sep 18 '25 02:09 github-actions[bot]

This PR has been closed due to inactivity

It was automatically closed because there has been no recent activity. If the PR is still relevant, please feel free to reopen and update it and add any new information.

github-actions[bot] avatar Oct 09 '25 02:10 github-actions[bot]