core icon indicating copy to clipboard operation
core copied to clipboard

Add Tado add meter readings service

Open niro1987 opened this issue 1 year ago • 2 comments

Proposed change

Add service Add meter readings to the Tado integration. Adding meter readings to Tado helps the user to gain usefull energy insights like cost savings and increased accuracy on estimations.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

None applicable.

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

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

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [x] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

niro1987 avatar Jan 15 '24 19:01 niro1987

Hey there @chiefdragon, @erwindouna, mind taking a look at this pull request as it has been labeled with an integration (tado) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of tado can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign tado Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Jan 15 '24 19:01 home-assistant[bot]

I like the idea of adding more insightful information! Why did you choose to go far a service only? Having this periodically updated, saves the hazzle of manually running the service.

Also, please add a fixture for the test cases.

erwindouna avatar Jan 15 '24 22:01 erwindouna

Why did you choose to go far a service only? Having this periodically updated, saves the hazzle of manually running the service.

The Tado integration is not aware of the meter reading, as those come from another integration or are manual input. This service call is needed to collect the meter reading before sending it to Tado. A simple automation on an interval can be created to send the new meter reading.

please add a fixture for the test cases.

There is no response in the service call, what would you like me to add?

niro1987 avatar Jan 16 '24 09:01 niro1987

The Tado integration is not aware of the meter reading, as those come from another integration or are manual input. This service call is needed to collect the meter reading before sending it to Tado. A simple automation on an interval can be created to send the new meter reading.

Where do you obtain the meter information? Right now it's a manual entry. An ideal use case would be to utilize the options of the config flow to let the user select which sensors should be used to populate the meter reading for Tado. Is the meter reading of Tado both compatible with m3 and kWh?

There is no response in the service call, what would you like me to add?

True, this is indeed the setter, not the getter.

erwindouna avatar Jan 16 '24 09:01 erwindouna

please add a fixture for the test cases

Hi @erwindouna When adding a meter reading, Tado is could respond with an error, I thought it best to validate the response and raise a ServiceValidationError when needed. I found three different responses; a success response, an invalid_meter_reading response when you add a meter reading below the previous one or a duplicated_meter_reading response when you add multiple readings on the same day.

I wanted to add test cases for each of these, but I'm not sure how. Can you help out?

add_readings_success.json add_readings_invalid_meter_reading.json add_readings_duplicated_meter_reading.json

niro1987 avatar Jan 17 '24 11:01 niro1987

You can patch the Tado library (PyTado, the upper laying library), and return fixtures with the desired results per call. Make three test cases. Use the side_effect to mimic the errors (invalid and duplicate). Check the Fastdotcom service test for an example. Use something like json.loads(load_fixture("your_fixture.json")) as a return value, to let it run through the PyTado function for the meter readings.

erwindouna avatar Jan 17 '24 13:01 erwindouna

@zweckj @erwindouna please review

niro1987 avatar Jan 17 '24 18:01 niro1987

@zweckj @erwindouna please review

don't ping people, reviewers get a notification anyways

zweckj avatar Jan 17 '24 21:01 zweckj

Very nice progress thus far. @niro1987! We're getting close to completion. :)

erwindouna avatar Jan 18 '24 09:01 erwindouna

Looks good to me. Let's await a Core Members review now. :)

erwindouna avatar Jan 22 '24 21:01 erwindouna

await a Core Member

it needs an initial accepted review to get that kind of attention. @erwindouna You're a code-owner for this integration, you are able to provide that initial review.

niro1987 avatar Jan 22 '24 21:01 niro1987

@zweckj can you complete the requested review please? I'm hoping to get this done before 2024.02

niro1987 avatar Jan 24 '24 12:01 niro1987

@zweckj can you complete the requested review please? I'm hoping to get this done before 2024.02

I already told you: Don't ping people it's unpolite! I've already looked at it and it looks good to me, but I am no maintainer here, so my review doesn't have any point, except leaving (hopefully) better code when a maintainer finally looks at it (which can take weeks to months)

zweckj avatar Jan 24 '24 12:01 zweckj

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Jan 28 '24 14:01 home-assistant[bot]

1 review requesting changes by reviewers

All reviews are resolved

niro1987 avatar Jan 29 '24 11:01 niro1987

@niro1987 you included a lot of unrelated commits. Please rebase and open a new PR, closing this one to avoid spamming linked developers.

jbouwh avatar Feb 26 '24 21:02 jbouwh

You have now quite a lot unneeded extra commits. Is there a reason you asked so many people for the review?

erwindouna avatar Feb 26 '24 21:02 erwindouna

You have now quite a lot unneeded extra commits. Is there a reason you asked so many people for the review?

It is due to a wrong rebase, Bot automatically ask codeowners for changed files.

thecode avatar Feb 26 '24 21:02 thecode

https://github.com/home-assistant/core/pull/111552

niro1987 avatar Feb 26 '24 21:02 niro1987