-
Notifications
You must be signed in to change notification settings - Fork 402
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
Sn0w3y
wants to merge
2
commits into
OpenEMS:develop
Choose a base branch
from
Sn0w3y:fix-storage-chart
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Fix Storage Chart #2828
+41
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# 🛠️ 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.**
Codecov ReportAll 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 |
@lukasrgr could you review? |
@lukasrgr any news ? |
lukasrgr
requested review from
venu-sagar and
fabian94533
and removed request for
lukasrgr and
venu-sagar
November 8, 2024 12:15
@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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🛠️ Fix Data Fetching, Conditional Logic, and Runtime Error in History Charts
Overview
This PR addresses critical issues in the
AbstractHistoryChart
andSingleChartComponent
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
Incorrect Data Fetching Logic
queryHistoricTimeseriesData
method was improperly returning empty data regardless of the API response, causing charts to display undefined or empty datasets.Faulty Conditional Statement
true
due to operator precedence, leading to logical errors.in
operator:Runtime Error in SingleChartComponent
forEach
on an undefined property, likely due to missing data validation.forEach
. Enhanced error handling to initialize empty charts gracefully when data is missing.Changes Made
Refactored
queryHistoricTimeseriesData
Method:Fixed Conditional Logic:
if
statement to individually check each required channel's existence inresult.data
.===
) forthis.showPhases
to ensure type-safe comparison.Resolved Runtime Error:
forEach
on data arrays.result.data
contains the necessary properties before processing.Why These Changes?
Testing
Please review the changes and approve the merge to enhance the reliability and accuracy of our history chart components.