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

Allow linking statistics graph to datetime entities #20876

Closed

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented May 26, 2024

Proposed change

Given that we allow users to use energy cards on custom dashboards as an option, I thought it would be a neat idea to allow further extension by allowing more generic statistics cards to better link with the energy date selection.

For example if user wants to make a more dynamic dashboard with additional things (like display temperature graphs next to gas consumption), there's no good way to reflexively update additional cards to match the dynamic selected dates of energy cards.

To that end, I thought we can allow some dynamic date selection for statistics graph cards as well. I've implemented this by allowing providing two datetime entities to two config options of the statistics card (start_date and end_date), which would take precedence over the days_to_show property.

I considered bypassing the entities and just having an option to link statisistics cards to the energy collection and get the dates directly from that, but this seems like it may end up being more flexible, for example if user wants to build a statistics viewer dashboard that doesn't really have anything to do with energy at all, they can populate the datetime helpers any way they wish.

This PR is just a start as a proof of concept. To fully implement the idea, I think we might also want:

  • implement the same for single statistic card
  • implement an option for the energy-date-selection-card to push its selection to two input_datetime entities. So user can have one date selector on the dashboard that can control energy cards and statistics cards together
  • an auto resolution option for statistics graph card, so it can perform the dynamic hour/day/month view like energy cards do based on length of the selection window
  • maybe something similar for history graph card?

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Summary by CodeRabbit

  • New Features
    • Enhanced statistics graph card with improved date entity change detection and handling.
    • More accurate start and end date calculations for statistics based on entity attributes.

This comment was marked as off-topic.

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: 2

Outside diff range and nitpick comments (1)
src/panels/lovelace/cards/hui-statistics-graph-card.ts (1)

[!TIP]
Codebase Verification

The recent script results indicate that there are no direct unit tests for the _getStatistics method in the identified files. This suggests that the new logic paths introduced in _getStatistics are not covered by existing tests.

  • Ensure that unit tests are added to cover the new logic paths in the _getStatistics method, especially the conditions handling startDate and endDate calculations.
Analysis chain

Line range hint 274-323: The _getStatistics method has been significantly modified to support dynamic date selection. Ensure that all new logic paths are covered by unit tests, especially the conditions handling startDate and endDate calculations.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method.

# Test: Search for unit tests related to `_getStatistics`. Expect: Tests covering new logic paths.
rg --type typescript $'_getStatistics' --files-with-matches | xargs cat | rg 'test'

Length of output: 115



Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method.

# Test: Search for unit tests related to `_getStatistics`. Expect: Tests covering new logic paths.
rg --type ts $'_getStatistics' --files-with-matches | xargs cat | rg 'test'

Length of output: 72



Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method by checking tests for the containing component/class.

# Identify the file containing `_getStatistics`
file_with_method=$(rg --type ts --files-with-matches '_getStatistics' | head -n 1)

# Extract the class or component name from the identified file
class_name=$(rg --type ts 'class ' "$file_with_method" | head -n 1 | awk '{print $2}')

# Search for test files related to the identified class/component
rg --type ts "$class_name" --files-with-matches | xargs cat | rg 'test'

Length of output: 9622



Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method by checking tests for the containing component/class.

# Identify the file containing `_getStatistics`
file_with_method=$(rg --type ts --files-with-matches '_getStatistics' | head -n 1)

# Extract the class or component name from the identified file
class_name=$(rg --type ts 'class ' "$file_with_method" | head -n 1 | sed -n 's/class \([a-zA-Z0-9_]*\).*/\1/p')

# Search for test files related to the identified class/component
rg --type ts "$class_name" --files-with-matches | xargs cat | rg 'test'

Length of output: 471



Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method by checking tests for the containing component/class.

# Identify the file containing `_getStatistics`
file_with_method=$(rg --type ts --files-with-matches '_getStatistics' | head -n 1)

# Extract the class or component name from the identified file, excluding the `export` keyword
class_name=$(rg --type ts 'class ' "$file_with_method" | head -n 1 | sed -n 's/class \([a-zA-Z0-9_]*\).*/\1/p' | sed 's/export //')

# Search for test files related to the identified class/component
rg --type ts "$class_name" --files-with-matches | xargs cat | rg 'test'

Length of output: 414



Script:

#!/bin/bash
# Description: Verify unit test coverage for `_getStatistics` method by checking tests for the containing component/class.

# Identify test files related to `HuiStatisticsGraphCard`
test_files=$(rg --type ts 'HuiStatisticsGraphCard' --files-with-matches)

# Inspect the contents of the identified test files for coverage of `_getStatistics`
for test_file in $test_files; do
  echo "Inspecting $test_file for _getStatistics coverage..."
  cat "$test_file" | rg '_getStatistics'
done

Length of output: 1295

@karwosts karwosts closed this May 27, 2024
@madsciencetist
Copy link

madsciencetist commented May 30, 2024

@karwosts may I ask why you closed this PR? I just implemented the same thing (tied to energy collection though) and was planning to PR it: 5facb37

@karwosts
Copy link
Contributor Author

After thinking about it for a day it I decided I didn't think this was a feasible approach as linking a card's visual behavior to entities would not really work for multiple concurrent users, as one user changing the date would change it for all users.

As much as I would still like to see this functionality, this current approach felt wrong to me because of the multi user issue.

Trying to get at the energy collection's dates directly seems like it could work. I thought about attempting it but it seemed a bit hairy, and I wasn't sure if that would be too hacky of a solution to accept.

But if you've got a PR ready, I encourage you to give it a shot.

@madsciencetist
Copy link

Just waiting for #20903 to go in first, then I'll PR it. I couldn't make the two totally independent.

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

Successfully merging this pull request may close these issues.

2 participants