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

Fix vizio bug that occurs when CONF_APPS isn't in config entry options #33857

Merged
merged 5 commits into from Apr 9, 2020

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Apr 9, 2020

Proposed change

A user reported an error (#33851) when CONF_APPS isn't in the config entry options that causes the UI for options not to load. This fix sets a default when CONF_APPS isn't found ({}) which should prevent the error. This should be included in the next minor release that goes out for 0.108. I don't see a need to add a test for this because it's a straightforward bug with a straightforward fix, but I will add one if needed.

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

@probot-home-assistant probot-home-assistant bot added integration: vizio small-pr PRs with less than 30 lines. labels Apr 9, 2020
@project-bot project-bot bot added this to Needs review in Dev Apr 9, 2020
@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Apr 9, 2020
@MartinHjelmare MartinHjelmare added this to the 0.108.1 milestone Apr 9, 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.

If we would have had a test we would have caught this. 😉

Dev automation moved this from By Code Owner to Reviewer approved Apr 9, 2020
@raman325
Copy link
Contributor Author

raman325 commented Apr 9, 2020

...touche, working on a test now

@raman325
Copy link
Contributor Author

raman325 commented Apr 9, 2020

actually not sure if I can test this... I think my tests would have covered this if this was something that could have been tested on the backend. See the following tests:

async def test_tv_options_flow_no_apps(hass: HomeAssistantType) -> None:
"""Test options config flow for TV without providing apps option."""
entry = MockConfigEntry(domain=DOMAIN, data=MOCK_USER_VALID_TV_CONFIG)
entry.add_to_hass(hass)
assert not entry.options
result = await hass.config_entries.options.async_init(entry.entry_id, data=None)
assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
assert result["step_id"] == "init"
options = {CONF_VOLUME_STEP: VOLUME_STEP}
options.update(MOCK_INCLUDE_NO_APPS)
result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input=options
)
assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
assert result["title"] == ""
assert result["data"][CONF_VOLUME_STEP] == VOLUME_STEP
assert CONF_APPS not in result["data"]

async def test_tv_options_flow_with_apps(hass: HomeAssistantType) -> None:
"""Test options config flow for TV with providing apps option."""
entry = MockConfigEntry(domain=DOMAIN, data=MOCK_USER_VALID_TV_CONFIG)
entry.add_to_hass(hass)
assert not entry.options
result = await hass.config_entries.options.async_init(entry.entry_id, data=None)
assert result["type"] == data_entry_flow.RESULT_TYPE_FORM
assert result["step_id"] == "init"
options = {CONF_VOLUME_STEP: VOLUME_STEP}
options.update(MOCK_INCLUDE_APPS)
result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input=options
)
assert result["type"] == data_entry_flow.RESULT_TYPE_CREATE_ENTRY
assert result["title"] == ""
assert result["data"][CONF_VOLUME_STEP] == VOLUME_STEP
assert CONF_APPS in result["data"]
assert result["data"][CONF_APPS] == {CONF_INCLUDE: [CURRENT_APP]}

If this could be tested these code blocks should have resulted in an error because the issue occurs when options haven't been set yet, and in these tests I only set options after creating the entry 😦

@MartinHjelmare
Copy link
Member

Make the test add CONF_VOLUME_STEP option to entry options before starting the flow.

@raman325
Copy link
Contributor Author

raman325 commented Apr 9, 2020

just to note, options always get set with DEFAULT_VOLUME_STEP if no CONF_VOLUME_STEP is provided so this is probably a redundant test but added it anyway

EDIT: nevermind, because I'm adding MockConfigEntrys directly in these tests, what I said is false

@balloob balloob merged commit b46eee0 into home-assistant:dev Apr 9, 2020
Dev automation moved this from Reviewer approved to Done Apr 9, 2020
balloob pushed a commit that referenced this pull request Apr 9, 2020
* fix bug when search for string in dict fails when dict is null

* another bug fix that I only noticed because of this other bug

* add test to cover failure scenario

* update docstring

* add additional assertions to cover failure scenario that's being fixed
@balloob balloob mentioned this pull request Apr 9, 2020
@tteck
Copy link

tteck commented Apr 9, 2020

108.1 broke Vizio SmartCast bar. Only the Google cast. This bar has Google assistant built in. Worked in 108
ERROR (Thread-8) [pychromecast.socket_client] [Soundbar:8009] Failed to connect to service SmartCast-Sound-Bar–1eabbb691d70ebdd4fc823a1f3ac4039._googlecast._tcp.local

@frenck
Copy link
Member

frenck commented Apr 9, 2020

@tteck Please don't report problems on merged issues. Instead, raise an issue here on GitHub. Thanks 👍

@home-assistant home-assistant locked and limited conversation to collaborators Apr 9, 2020
@raman325 raman325 deleted the vizio_bugfix branch April 9, 2020 21:41
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.

Integration options for Vizio not working in 0.108
6 participants