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

Add config flow to ecobee #26634

Merged

Conversation

@marthoc
Copy link
Contributor

commented Sep 14, 2019

Breaking Change:

Ecobee will now be set up via config flow. Existing users will have their config imported from ecobee.conf via an import flow so it shouldn't break their experience. Users configuring via configuration.yaml will have their api key and options imported into the flow but will still need to finish authorization via the flow (instead of the configurator component as previously).

The configuration parameter hold_temp has been removed, as it was not being used in the climate platform and had no effect on whether the temperature was held indefinitely or not. Users will need to remove the parameter hold_temp from configuration.yaml.

Ecobee-specific services will now be registered under the ecobee domain rather than the climate domain, and service names will not include the prefix "ecobee_" (e.g. the service "climate.ecobee_resume_program" will become "ecobee.resume_program").

Description:

Adds a config flow to the ecobee component. This PR is Phase 1 of my planned updates to the ecobee component to make it a first-class integration - in this PR I've tried to make only the minimal changes that were necessary to support loading ecobee via a config entry to aid reviewability.

Phase 2 will follow the merging of this PR and add support for the entity and device registries. Phase 3 will make everything async.

Upstream library has been updated with needed changes for the config flow.

Tests have been added for the config flow.

Related issue (if applicable): N/A

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10428

Example entry for configuration.yaml (if applicable): N/A

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@project-bot project-bot bot added this to Needs review in Dev Sep 14, 2019
@marthoc marthoc changed the title Implement ecobee config flow Add config flow to ecobee Sep 14, 2019
Copy link
Member

left a comment

The config flow module requires 100 % test coverage. Please see eg hue config flow tests for examples and make sure that the config flow module isn't excluded from coverage calculation.

homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/manifest.json Outdated Show resolved Hide resolved
homeassistant/components/ecobee/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/strings.json Outdated Show resolved Hide resolved
homeassistant/components/ecobee/strings.json Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Sep 14, 2019
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/ecobee/__init__.py Outdated Show resolved Hide resolved
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Please run script/gen_requirements_all.py.

@frenck frenck added the docs-missing label Sep 17, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Please solve the merge conflict.

@frenck

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

@marthoc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

@MartinHjelmare I changed the instances of Mock() in the tests to patches - if you have a minute, would you mind taking a look at the last test? I can't get the test_pin_request_fails to pass and I'm not sure why - I'm mocking the Ecobee object and setting the return value of request_pin() to False, which should cause async_step_user to show a form with an error, but it doesn't do that and just proceeds to async_step_authorize. The request_pin method gets wrapped in hass.async_add_executor_job as follows:

 self._ecobee = Ecobee(config={ECOBEE_API_KEY: user_input[CONF_API_KEY]})

            if await self.hass.async_add_executor_job(self._ecobee.request_pin):
                """We have a PIN; move to the next step of the flow."""
                return await self.async_step_authorize()
            errors["base"] = "pin_request_failed"

I'm new to unittest so I'm sure I'm missing something obvious but I'm not sure what...

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Please solve the merge conflict.

@marthoc marthoc force-pushed the marthoc:implement-ecobee-config-flow branch from 32cec92 to ef936ac Sep 22, 2019
@marthoc marthoc referenced this pull request Sep 22, 2019
2 of 2 tasks complete
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

Please move the service descriptions to a services.yaml file in the ecobee integration package:

ecobee_set_fan_min_on_time:
description: Set the minimum fan on time.
fields:
entity_id:
description: Name(s) of entities to change.
example: 'climate.kitchen'
fan_min_on_time:
description: New value of fan min on time.
example: 5
ecobee_resume_program:
description: Resume the programmed schedule.
fields:
entity_id:
description: Name(s) of entities to change.
example: 'climate.kitchen'
resume_all:
description: Resume all events and return to the scheduled program. This default to false which removes only the top event.
example: true

We should also rename the services to not prefix with ecobee_.

Copy link
Member

left a comment

Looks great!

Dev automation moved this from Review in progress to Reviewer approved Sep 23, 2019
Copy link
Member

left a comment

Sorry, one more thing. Please update .coveragerc and don't exclude the config flow module.

Dev automation moved this from Reviewer approved to Review in progress Sep 23, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

Also please update the breaking change section to mention the change of service names.

@frenck frenck removed the docs-missing label Sep 23, 2019
@marthoc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

Sorry, one more thing. Please update .coveragerc and don't exclude the config flow module.

I've updated it - will just need to make sure that coverage on the config_flow is 100%.

Dev automation moved this from Cancelled to Needs review Sep 25, 2019
@FuzzyMistborn

This comment has been minimized.

Copy link

commented Sep 25, 2019

@marthoc just FYI, with my update to https://github.com/nkgilley/python-ecobee-api the release was 0.1.3, not 0.1.2 like you have in the requirements/manifest.json.

Ensure that the import step calls the user step with the user's api key as user input if importing from ecobee.conf/validating imported keys fails.
@marthoc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

@marthoc just FYI, with my update to https://github.com/nkgilley/python-ecobee-api the release was 0.1.3, not 0.1.2 like you have in the requirements/manifest.json.

@FuzzyMistborn that's fine - the changes I needed for this PR are in 0.1.2. After this is merged I will be opening another PR to add create_vacation and delete_vacation services and in that PR I will bump python-ecobee-api to 0.1.3.

@FuzzyMistborn

This comment has been minimized.

Copy link

commented Sep 25, 2019

Just saying i don't think there was a 0.1.2 release which is why I point it out, so PyPi won't be able to find 0.1.2.

@marthoc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Just saying i don't think there was a 0.1.2 release which is why I point it out, so PyPi won't be able to find 0.1.2.

There was: https://pypi.org/project/python-ecobee-api/#history 👍

@marthoc marthoc closed this Sep 25, 2019
Dev automation moved this from Needs review to Cancelled Sep 25, 2019
@marthoc marthoc reopened this Sep 25, 2019
Dev automation moved this from Cancelled to Needs review Sep 25, 2019
Copy link
Member

left a comment

Great!

Dev automation moved this from Needs review to Reviewer approved Sep 25, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

Can be merged when build passes.

@marthoc

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Looks like we are good to go!

@MartinHjelmare MartinHjelmare merged commit f6995b8 into home-assistant:dev Sep 25, 2019
11 checks passed
11 checks passed
CI Build #20190925.53 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 Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94.14%)
Details
codecov/project 94.16% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 25, 2019
@marthoc marthoc deleted the marthoc:implement-ecobee-config-flow branch Sep 26, 2019
KJonline added a commit to KJonline/home-assistant that referenced this pull request Sep 26, 2019
* dev: (87 commits)
  Add ecobee services to create and delete vacations (home-assistant#26923)
  Centralize rainbird config and add binary sensor platform (home-assistant#26393)
  Add config flow to transmission (home-assistant#26434)
  Add Plex config options support (home-assistant#26870)
  Bump pyobihai, add unique ID and availability (home-assistant#26922)
  Add mysensors codeowner (home-assistant#26917)
  [ci skip] Translation update
  Add MySensors ACK (home-assistant#26894)
  Remove lamps and groups from ha when removed from Hue (home-assistant#26881)
  Add config flow to ecobee (home-assistant#26634)
  deCONZ - Increase bridge discovery robustness in config flow (home-assistant#26911)
  Add call direction sensor for Obihai (home-assistant#26867)
  Bumped version to 0.99.3
  HM-CC-TC was not recognized (home-assistant#26623)
  Add google_assistant alarm_control_panel (home-assistant#26249)
  deCONZ - Improve ssdp discovery by storing uuid in config entry (home-assistant#26882)
  Fix missing whitespace around arithmetic operator (home-assistant#26908)
  Fix bed_activity history chart of the Xiaomi Aqara vibration sensor (home-assistant#26875)
  Add voltage attribute to Xiaomi Aqara devices (home-assistant#26876)
  Bump ndms2-client to 0.0.9 (home-assistant#26899)
  ...
@FuzzyMistborn

This comment has been minimized.

Copy link

commented Sep 27, 2019

I tried testing this in my dev instance (fresh install, no ecobee in config.yaml) and every time I click the box to add the integration, i see a new window but it closes and i'm back at the integration menu. Tried in Chrome and Firefox. Adding it to config.yaml it worked fine.

First time testing something in dev so wasn't sure if I should mention it here or open a new issue.

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019 — with Octobox

@FuzzyMistborn, in general, opening an issue is a good idea. This allows us to track things easier.

@lock lock bot locked and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.