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

Fix Storage Chart #2828

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix Storage Chart #2828

wants to merge 2 commits into from

Conversation

Sn0w3y
Copy link
Contributor

@Sn0w3y Sn0w3y commented Oct 3, 2024

🛠️ 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.

# 🛠️ Fix Data Fetching and Conditional Logic in History Charts

## **Overview**

This PR addresses critical issues in the `AbstractHistoryChart` component related to data fetching and conditional logic, ensuring that historical timeseries data is correctly retrieved and processed for accurate chart rendering.

## **Issues Addressed**

1. **Incorrect Data Fetching Logic**
   - **Problem:** The `queryHistoricTimeseriesData` method was improperly returning empty data regardless of the API response, leading to charts displaying 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
     ```typescript
     if ("_sum/EssActivePowerL1" && "_sum/EssActivePowerL2" && "_sum/EssActivePowerL3" in result.data && this.showPhases == true) {
     ```
     was always evaluating to `true` due to operator precedence, causing logical errors.
   - **Solution:** Updated the condition to explicitly check each channel using the `in` operator:
     ```typescript
     if ("_sum/EssActivePowerL1" in result.data && "_sum/EssActivePowerL2" in result.data && "_sum/EssActivePowerL3" in result.data && this.showPhases === true) {
     ```

## **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.

## **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 prevents the application from masking issues and aids in debugging.
- **Improved Code Quality:** Addressing TypeScript warnings leads to cleaner, more maintainable code.

## **Testing**

- Verified that charts now display correct data when API responses are received.
- Confirmed that conditional statements behave as expected without triggering TypeScript warnings.
- Ensured that error scenarios gracefully initialize empty charts without breaking the application.

---

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

codecov bot commented Oct 3, 2024

Codecov Report

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     

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Oct 6, 2024

@lukasrgr could you review?

@da-Kai da-Kai requested a review from lukasrgr October 13, 2024 10:29
@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Oct 30, 2024

@lukasrgr any news ?

@lukasrgr lukasrgr requested review from venu-sagar and fabian94533 and removed request for lukasrgr and venu-sagar November 8, 2024 12:15
@fabian94533
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants