8000 Fix vizio bug that occurs when CONF_APPS isn't in config entry options by raman325 · Pull Request #33857 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 9, 2020
Merged

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

merged 5 commits into from
Apr 9, 2020

Conversation

raman325
Copy link
Contributor
@raman325 raman325 commented Apr 9, 2020 •
edited
Loading

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

@MartinHjelmare MartinHjelmare added this to the 0.108.1 milestone Apr 9, 2020
MartinHjelmare
MartinHjelmare approved these changes Apr 9, 2020
View reviewed changes
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. 😉

@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 •
edited
Loading

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:

core/tests/components/vizio/test_config_flow.py

Lines 128 to 150 in ff93630

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"]

core/tests/components/vizio/test_config_flow.py

Lines 153 to 176 in ff93630

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 •
edited
Loading

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
balloob pushed a commit that referenced this pull request Apr 9, 2020
@raman325 @balloob
Fix vizio bug that occurs when CONF_APPS isn't in config entry… (#33857)
dceb0d9 
* 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
Merged
@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.
Reviewers

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees
No one assigned
Labels
by-code-owner cherry-picked cla-signed integration: vizio small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
0.108.1
Development

Successfully merging this pull request may close these issues.

Integration options for Vizio not working in 0.108
6 participants
@raman325 @MartinHjelmare @tteck @frenck @balloob @homeassistant
0