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

Support vizio pairing through config flow #31520

Merged
merged 17 commits into from Feb 28, 2020

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Feb 6, 2020

Proposed change

The vizio integration currently requires users to pair to their device using pyvizio on the command line. This PR allows the pairing process to be completed through config flow for all source types (this also means the additional custom validation for configuration.yaml entries can be removed because pairing for config imports is also supported). For config imports, a message is now logged anytime the import does not get aborted because aborts typically occur due to a previous import whereas setup failures are silent without the logging.

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

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

@project-bot project-bot bot added this to Needs review in Dev Feb 6, 2020
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Feb 6, 2020
@raman325 raman325 changed the title Support pairing through config flow for vizio component Support pairing through config flow for vizio component and update integration name Feb 6, 2020
@MartinHjelmare MartinHjelmare changed the title Support pairing through config flow for vizio component and update integration name Support vizio pairing through config flow and update integration name Feb 6, 2020
@JeffLIrion
Copy link
Contributor

A couple questions / comments:

  1. If the user pairs the device through HA, the access token will always be stored, correct? The pairing_complete message states this, I just want to confirm.

  2. The pairing_complete message is very long and wordy. I can think of two ways to improve this:

    1. (Preferred) If a user configured the integration via YAML, try to look up the access token from storage. That way, they don't have to worry about adding it to their configuration.
    2. Use two different "pairing complete" messages, depending on whether the user configured the integration via YAML or config flow.

@raman325
Copy link
Contributor Author

raman325 commented Feb 6, 2020

  1. Yes, in the config_entries storage file, and it will stay in storage for the life of the config entry

  2. I thought about adding it to storage, and I can still take that route, but the access token is already in storage in the config entries file. To me, the value of adding the token to configuration.yaml is mainly for when you want to rebuild your instance from scratch and use configuration.yaml to achieve your previous setup. At that point, your storage is probably wiped, so I'm not sure how much extra value it adds to save the token to a second file in storage. I like your second suggestion though. Commit incoming

@JeffLIrion
Copy link
Contributor

I don't know what the difference is between storing it in the config_entries storage file vs. storing it in storage. But ultimately, my question is as follows.

Suppose a user configures their Vizio TV as

vizio:
  host: 192.168.0.123:7345

And suppose they go through the pairing process in HA but they don't add the access token to their configuration. Then every time they restart HA, they'll need to go through the pairing process again, right?

@raman325
Copy link
Contributor Author

raman325 commented Feb 6, 2020

No, they won't have to do that.

By moving to config flow, entities are no longer directly loaded from configuration.yaml. Instead, they are loaded from a ConfigEntry. When you launch HA for the first time with the config you posted, the config gets imported into the vizio config flow and a ConfigEntry gets created, which is then persisted to storage and will survive through restarts. When a user goes through the pairing process, the access token gets stored in the config data for the ConfigEntry. This means it is available on restart to reload the entity even if it hasn't been added to configuration.yaml, and will remain there until you delete the ConfigEntry entirely from the Integrations menu or wipe your storage.

After a config gets imported into a ConfigEntry, the ConfigEntry is independent from configuration.yaml. What happens on restarts is that HA attempts to import the configuration again, but sees that a vizio ConfigEntry for the specified host already exists and aborts the import process. This means that you can remove that piece from your configuration.yaml entirely, restart, and the entity will still be there, and the only way to completely remove it is to both delete it from configuration.yaml and from the Integrations menu in the frontend.

I think I'm overexplaining at this point but just to complete the thought, the only way configuration.yaml affects your vizio entities after the initial startup is if you specify a different name or volume_step in your config, as those changes are detected and get applied to the ConfigEntry before import is aborted.

@JeffLIrion
Copy link
Contributor

Thanks for the detailed explanation!

@codecov
Copy link

codecov bot commented Feb 9, 2020

Codecov Report

❗ No coverage uploaded for pull request base (dev@4236d62). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev   #31520   +/-   ##
======================================
  Coverage       ?   94.76%           
======================================
  Files          ?      768           
  Lines          ?    55686           
  Branches       ?        0           
======================================
  Hits           ?    52769           
  Misses         ?     2917           
  Partials       ?        0
Impacted Files Coverage Δ
homeassistant/components/vizio/config_flow.py 100% <100%> (ø)

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 4236d62...6b3e887. Read the comment docs.

@raman325 raman325 force-pushed the vizio_add_pairing_to_config_flow branch from 17c9d02 to e791e15 Compare February 11, 2020 00:50
@raman325
Copy link
Contributor Author

@MartinHjelmare I've learned a lot from your reviews of my past PRs, would you mind reviewing this one as well when you have a moment?

@raman325 raman325 force-pushed the vizio_add_pairing_to_config_flow branch from e791e15 to 9820e27 Compare February 15, 2020 01:08
@raman325 raman325 force-pushed the vizio_add_pairing_to_config_flow branch from 9820e27 to 47f2e77 Compare February 20, 2020 21:06
@raman325 raman325 changed the title Support vizio pairing through config flow and update integration name Support vizio pairing through config flow Feb 20, 2020
@raman325 raman325 force-pushed the vizio_add_pairing_to_config_flow branch from 47f2e77 to 8c1e2e1 Compare February 25, 2020 03:58
@raman325 raman325 requested review from balloob and removed request for MartinHjelmare February 25, 2020 05:52
Dev automation moved this from By Code Owner to Reviewer approved Feb 28, 2020
@balloob balloob merged commit b9fa324 into home-assistant:dev Feb 28, 2020
Dev automation moved this from Reviewer approved to Done Feb 28, 2020
@raman325 raman325 deleted the vizio_add_pairing_to_config_flow branch February 28, 2020 15:52
@lock lock bot locked and limited conversation to collaborators Mar 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants