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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in ecobee integration #32008

Merged
merged 3 commits into from Feb 20, 2020
Merged

Fix bug in ecobee integration #32008

merged 3 commits into from Feb 20, 2020

Conversation

@marthoc
Copy link
Contributor

marthoc commented Feb 20, 2020

Breaking change

Not a breaking change.

Proposed change

Fixes the bug identified in #28531.

Type of change

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

Example entry for configuration.yaml:

No configuration.yaml entry as the integration is configured via config entry.

Additional information

This PR fixes or closes issue: fixes #28531.

Before config entries, ecobee authentication tokens were stored in a file called ecobee.conf that was stored in the Home Assistant config directory. On HA startup, the ecobee component would read the tokens from this file and use them to authenticate. The python library took care of retrieving the tokens from ecobee, storing them in the file, and (importantly) refreshing them and updating ecobee.conf with the new tokens if the old tokens became stale. If HA was shut down, on the next startup, ecobee.conf would contain the most recent tokens as the python library had been updating that file each time the tokens were refreshed.

Moving ecobee to using config entries meant that the tokens would now be stored in the entry itself, rather than in ecobee.conf. From a logic perspective, this meant that HA itself would now need to be in charge of refreshing the tokens and updating the config entry with any refreshed tokens. Easy enough, and this logic was added.

However, the python library still contained logic as though it was responsible for refreshing tokens. On a failed API access attempt (because of connectivity issues or otherwise), the library would (without being prompted by the logic in HA) retry the failed call but also (and the origin of the bug) attempt to refresh the authentication tokens. If this succeeded, HA would go on using the correct tokens for the time being, but, when those tokens became stale (as they do every 3600 seconds/1 hour) the attempt by HA to refresh them would fail as the tokens in the config entry had been invalidated by the issuing of new tokens to the request made by the python library, and HA was presenting those stale tokens on the refresh request.

This explains why this issue is not cropping up for everyone - if you have no connectivity issues to ecobee (i.e. calls to the API don't fail), then the library would never independently refresh your tokens and this error would not happen.

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast 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:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 馃 Silver
  • 馃 Gold
  • 馃弳 Platinum
marthoc added 3 commits Feb 19, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 20, 2020

Codecov Report

鉂楋笍 No coverage uploaded for pull request base (dev@4e76539). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev   #32008   +/-   ##
======================================
  Coverage       ?   94.69%           
======================================
  Files          ?      766           
  Lines          ?    55578           
  Branches       ?        0           
======================================
  Hits           ?    52631           
  Misses         ?     2947           
  Partials       ?        0           
Impacted Files Coverage 螖
homeassistant/components/demo/calendar.py 100.00% <0.00%> (酶)
homeassistant/components/automation/device.py 100.00% <0.00%> (酶)
homeassistant/util/async_.py 96.66% <0.00%> (酶)
homeassistant/components/config/device_registry.py 100.00% <0.00%> (酶)
homeassistant/components/plex/config_flow.py 100.00% <0.00%> (酶)
homeassistant/helpers/collection.py 95.58% <0.00%> (酶)
homeassistant/components/vera/binary_sensor.py 100.00% <0.00%> (酶)
homeassistant/helpers/intent.py 93.93% <0.00%> (酶)
homeassistant/helpers/event.py 99.54% <0.00%> (酶)
homeassistant/util/yaml/loader.py 93.58% <0.00%> (酶)
... and 756 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 4e76539...828d197. Read the comment docs.

@balloob balloob added this to the 0.106.0 milestone Feb 20, 2020
Dev automation moved this from By Code Owner to Reviewer approved Feb 20, 2020
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Feb 20, 2020

Nice catch !

@balloob balloob merged commit 832337f into home-assistant:dev Feb 20, 2020
11 checks passed
11 checks passed
CI Build #20200220.11 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 4e76539...828d197
Details
codecov/project 94.69% (target 90.00%)
Details
docs-missing Documentation ok.
Dev automation moved this from Reviewer approved to Done Feb 20, 2020
balloob added a commit that referenced this pull request Feb 20, 2020
* Bump python-ecobee-api to 0.2.1

* Update log messages for clarity

* Update requirements_all
@lock lock bot locked and limited conversation to collaborators Feb 21, 2020
@marthoc marthoc deleted the marthoc:fix-ecobee-bug branch Feb 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can鈥檛 perform that action at this time.