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 Avri config flow #34288

Merged
merged 12 commits into from
Jun 6, 2020
Merged

Conversation

timvancann
Copy link
Contributor

@timvancann timvancann commented Apr 16, 2020

Breaking change

The Avri component must now be setup using the Integrations page in the UI. Remove the integration from configuration.yaml and re-add it in the UI.

Proposed change

Implements the Config Flow for the Avri integration.

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

The format of the unique id of the entities changes. Orphaned entities (created by manually adding the integration to the configuration.yaml) must be removed manually. This may be accomplished by nativating to the Configuration -> Entities menu from the Home Assitant application and searching for sensor.avri. Removed any entities found.

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

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

@timvancann
Copy link
Contributor Author

I checked the silver score after reading the scoring guidelines. If I miss anything or if this is incorrect please let me know!

@springstan springstan changed the title Avri config flow Add Avri config flow Apr 16, 2020
@bdraco
Copy link
Member

bdraco commented May 10, 2020

@timvancann Thanks for adding the config flow.

Config flows require test coverage. There is a helper script that can help you get started:
python3 -m script.scaffold config_flow

@timvancann
Copy link
Contributor Author

@bdraco Thanks for the comment. I'll pick it up and implement the tests soon!

@timvancann
Copy link
Contributor Author

@bdraco I added some useful tests, however pylint failed for no real apparent reason (locally it doesn't fail). Do you know what is going on?

timvancann and others added 3 commits May 22, 2020 19:57
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick@koston.org>
@bdraco bdraco self-requested a review May 25, 2020 03:40
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments above

timvancann and others added 2 commits May 27, 2020 18:47
@bdraco bdraco self-requested a review May 30, 2020 01:50
@bdraco bdraco merged commit d73a4e1 into home-assistant:dev Jun 6, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the comments in a new PR. Thanks!

if user_input[CONF_HOUSE_NUMBER] <= 0:
errors[CONF_HOUSE_NUMBER] = "invalid_house_number"
return await self._show_setup_form(errors)
if not pycountry.countries.get(alpha_2=user_input[CONF_COUNTRY_CODE]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this look up might do I/O if the data hasn't been loaded yet. There's a lazy load mechanism in the library.

We're not allowed to do blocking I/O inside the event loop, ie inside coroutines. We need to schedule I/O on the executor thread pool with hass.async_add_executor_job.


VERSION = 1

async def _show_setup_form(self, errors=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no await inside this function so it doesn't need to be a coroutine. We prefer callbacks if a coroutine isn't needed since awaiting a coroutine has an overhead cost.

return self.async_create_entry(
title=unique_id,
data={
CONF_ID: unique_id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have access to the unique_id of the config entry via entry.unique_id. We don't need to store it separately.

country_code=config[CONF_COUNTRY_CODE],
)
client = hass.data[DOMAIN][entry.entry_id]
integration_id = entry.data[CONF_ID]

try:
each_upcoming = client.upcoming_of_each()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this does I/O it must be scheduled on the executor thread pool.


async def test_form(hass):
"""Test we get the form."""
await setup.async_setup_component(hass, "avri", {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to set up the avri component here.

await self.async_set_unique_id(unique_id)
self._abort_if_unique_id_configured()

return self.async_create_entry(
Copy link
Member

@MartinHjelmare MartinHjelmare Jun 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check that we can connect with the avri client before creating the entry.

I see now that there's no authentication. We might not have to check with the client of we're sure that the checks above are enough. Could the user somehow enter a non supported zip code or similar?

assert result["errors"] == {}

with patch(
"homeassistant.components.avri.async_setup_entry", return_value=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to patch all I/O of the library that happens in the config flow.

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.

6 participants