Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions homeassistant/components/samsungtv/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

from .const import CONF_ON_ACTION, DEFAULT_NAME, DOMAIN

UNDO_UPDATE_LISTENER = "update_update_listener"


def ensure_unique_hosts(value):
"""Validate that all configs have a unique host."""
Expand Down Expand Up @@ -62,8 +64,33 @@ async def async_setup(hass, config):

async def async_setup_entry(hass, entry):
"""Set up the Samsung TV platform."""
undo_listener = entry.add_update_listener(_update_listener)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to unload and setup the entry again? The action is created at runtime and saved at hass.data[DOMAIN][ip_address][CONF_ON_ACTION]. Why do we not override it and use this directly in the entity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting point. TBH I haven't thought of this direction until you wrote this.
Updating hass.data[DOMAIN][ip_address][CONF_ON_ACTION] won't be enough - the entity currently gets its processed on_script on initialization. We can have the entity listen to the update and handle accordingly. It might be a bit simpler, but potentially a bit more error prone.
The advantages of the unload/reload approach are:

  1. Works through standard HA mechanisms.
  2. Supports other unload scenarios. For example, removing a Samsung TV config entry currently suggests restarting HA. This wont' be needed if the integration can unload. Another example: if in the future we support editing the IP address (see Ability to edit/update config entries architecture#377), it will work out of the box.

Anyway, pros and cons to both approaches - I'm fine with whatever you choose.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we going to wait for the linked issue? I would decide for the most future proof way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't see a reason to wait - this PR provides benefits as it is, even without the arch issue ever being implemented. The link is just to support the unload route as the more future proof one.


hass.data.setdefault(DOMAIN, {})
hass.data[DOMAIN][entry.entry_id] = {
UNDO_UPDATE_LISTENER: undo_listener,
}

hass.async_create_task(
hass.config_entries.async_forward_entry_setup(entry, MP_DOMAIN)
)

return True


async def async_unload_entry(hass, entry):
"""Unload a config entry."""
unload_ok = await hass.async_create_task(
hass.config_entries.async_forward_entry_unload(entry, MP_DOMAIN)
)

if unload_ok:
hass.data[DOMAIN][entry.entry_id][UNDO_UPDATE_LISTENER]()
hass.data[DOMAIN].pop(entry.entry_id)

return unload_ok


async def _update_listener(hass, entry):
"""Handle options update."""
await hass.config_entries.async_reload(entry.entry_id)
43 changes: 42 additions & 1 deletion homeassistant/components/samsungtv/config_flow.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
"""Config flow for Samsung TV."""
from functools import partial
import socket
from urllib.parse import urlparse

from getmac import get_mac_address
import voluptuous as vol

from homeassistant import config_entries
from homeassistant import config_entries, core
from homeassistant.components.ssdp import (
ATTR_SSDP_LOCATION,
ATTR_UPNP_MANUFACTURER,
Expand All @@ -15,6 +17,7 @@
CONF_HOST,
CONF_ID,
CONF_IP_ADDRESS,
CONF_MAC,
CONF_METHOD,
CONF_NAME,
CONF_PORT,
Expand Down Expand Up @@ -174,3 +177,41 @@ async def async_step_reauth(self, user_input=None):
self.context["title_placeholders"] = {"model": self._title}

return await self.async_step_confirm()

@staticmethod
@core.callback
def async_get_options_flow(config_entry):
"""Define the config flow to handle options."""
return SamsungTVOptionsFlowHandler(config_entry)


class SamsungTVOptionsFlowHandler(config_entries.OptionsFlow):
"""Handle a SamsungTV options flow."""

def __init__(self, config_entry):
"""Initialize."""
self.config_entry = config_entry

async def async_step_init(self, user_input=None):
"""Manage the options."""
if user_input is not None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No validation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The only risk here is a malformed MAC address, which is no different than a wrong MAC address. This is an advanced feature, and the input will most likely be pasted (unless obtained automatically). The wrong/malformed address will fail quickly and be easy to fix. Given all of the above, I don't think the validation overhead is worth it.
I've also quickly searched through the code base and couldn't find an integration that validates mac addresses.

Comment thread
OnFreund marked this conversation as resolved.
if CONF_MAC not in user_input or user_input[CONF_MAC].strip() == "":
user_input.pop(CONF_MAC, None)
return self.async_create_entry(title="", data=user_input)

if CONF_MAC in self.config_entry.options:
current_mac = self.config_entry.options[CONF_MAC]
else:
hostname = self.config_entry.data[CONF_HOST]
try:
current_mac = await self.hass.async_add_executor_job(
partial(get_mac_address, **{"hostname": hostname})
)
except OSError:
LOGGER.info("Could not find mac address for %s", hostname)
current_mac = ""

schema = {
vol.Optional(CONF_MAC, description={"suggested_value": current_mac}): str
}
return self.async_show_form(step_id="init", data_schema=vol.Schema(schema))
3 changes: 2 additions & 1 deletion homeassistant/components/samsungtv/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
"documentation": "https://www.home-assistant.io/integrations/samsungtv",
"requirements": [
"samsungctl[websocket]==0.7.1",
"samsungtvws[websocket]==1.4.0"
"samsungtvws[websocket]==1.4.0",
"getmac==0.8.2"
],
"ssdp": [
{
Expand Down
22 changes: 21 additions & 1 deletion homeassistant/components/samsungtv/media_player.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import voluptuous as vol

from homeassistant import core
from homeassistant.components.media_player import DEVICE_CLASS_TV, MediaPlayerEntity
from homeassistant.components.media_player.const import (
MEDIA_TYPE_CHANNEL,
Expand All @@ -22,9 +23,12 @@
CONF_HOST,
CONF_ID,
CONF_IP_ADDRESS,
CONF_MAC,
CONF_METHOD,
CONF_NAME,
CONF_PORT,
CONF_SERVICE,
CONF_SERVICE_DATA,
CONF_TOKEN,
STATE_OFF,
STATE_ON,
Expand Down Expand Up @@ -52,11 +56,27 @@
)


@core.callback
def async_get_on_script_from_mac(hass, mac):
"""Create an on script from a mac address."""
script = cv.SCRIPT_SCHEMA(
{
CONF_SERVICE: "wake_on_lan.send_magic_packet",
CONF_SERVICE_DATA: {CONF_MAC: mac},
}
)
return Script(hass, script)


async def async_setup_entry(hass, config_entry, async_add_entities):
"""Set up the Samsung TV from a config entry."""
ip_address = config_entry.data[CONF_IP_ADDRESS]
on_script = None
if (

if CONF_MAC in config_entry.options:
Comment thread
OnFreund marked this conversation as resolved.
mac = config_entry.options[CONF_MAC]
on_script = async_get_on_script_from_mac(hass, mac)
elif (
DOMAIN in hass.data
and ip_address in hass.data[DOMAIN]
and CONF_ON_ACTION in hass.data[DOMAIN][ip_address]
Expand Down
10 changes: 10 additions & 0 deletions homeassistant/components/samsungtv/strings.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,15 @@
"not_successful": "Unable to connect to this Samsung TV device.",
"not_supported": "This Samsung TV device is currently not supported."
}
},
"options": {
"step": {
"init": {
"title": "Configure MAC address",
"data": {
"mac": "MAC address"
}
}
}
}
}
1 change: 1 addition & 0 deletions requirements_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,7 @@ georss_qld_bushfire_alert_client==0.3
# homeassistant.components.kef
# homeassistant.components.minecraft_server
# homeassistant.components.nmap_tracker
# homeassistant.components.samsungtv
getmac==0.8.2

# homeassistant.components.gios
Expand Down
1 change: 1 addition & 0 deletions requirements_test_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ georss_qld_bushfire_alert_client==0.3
# homeassistant.components.kef
# homeassistant.components.minecraft_server
# homeassistant.components.nmap_tracker
# homeassistant.components.samsungtv
getmac==0.8.2

# homeassistant.components.gios
Expand Down
101 changes: 100 additions & 1 deletion tests/components/samsungtv/test_config_flow.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
"""Tests for Samsung TV config flow."""
import socket

import pytest
from samsungctl.exceptions import AccessDenied, UnhandledResponse
from samsungtvws.exceptions import ConnectionFailure
Expand All @@ -16,9 +18,17 @@
ATTR_UPNP_MODEL_NAME,
ATTR_UPNP_UDN,
)
from homeassistant.const import CONF_HOST, CONF_ID, CONF_METHOD, CONF_NAME, CONF_TOKEN
from homeassistant.const import (
CONF_HOST,
CONF_ID,
CONF_MAC,
CONF_METHOD,
CONF_NAME,
CONF_TOKEN,
)

from tests.async_mock import DEFAULT as DEFAULT_MOCK, Mock, PropertyMock, call, patch
from tests.common import MockConfigEntry

MOCK_USER_DATA = {CONF_HOST: "fake_host", CONF_NAME: "fake_name"}
MOCK_SSDP_DATA = {
Expand Down Expand Up @@ -571,3 +581,92 @@ async def test_autodetect_none(hass, remote, remotews):
call(**AUTODETECT_WEBSOCKET_PLAIN),
call(**AUTODETECT_WEBSOCKET_SSL),
]


async def test_options_with_obtained_mac(hass, remote):
"""Test the options flow with obtained mac."""
config_entry = MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "hostname"},)

config_entry.add_to_hass(hass)

# by default, the mac address should be automatically obtained
with patch(
"homeassistant.components.samsungtv.config_flow.get_mac_address",
return_value="11:11:11",
):
result = await hass.config_entries.options.async_init(config_entry.entry_id)

assert result["type"] == "form"
assert result["step_id"] == "init"

result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={},
)

assert result["type"] == "create_entry"
assert config_entry.options.get(CONF_MAC) is None

# override the auto-obtained mac address
result = await hass.config_entries.options.async_init(config_entry.entry_id)

result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={CONF_MAC: "22:22:22"},
)
assert result["type"] == "create_entry"
assert config_entry.options[CONF_MAC] == "22:22:22"


async def test_options_without_obtained_mac(hass, remote):
"""Test the options flow without obtained mac."""
config_entry = MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "hostname"},)

config_entry.add_to_hass(hass)

# simulate an error in getting the mac address
with patch(
"homeassistant.components.samsungtv.config_flow.get_mac_address",
side_effect=socket.gaierror(),
):
result = await hass.config_entries.options.async_init(config_entry.entry_id)

assert result["type"] == "form"
assert result["step_id"] == "init"

result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={},
)

assert result["type"] == "create_entry"
assert config_entry.options.get(CONF_MAC) is None

result = await hass.config_entries.options.async_init(config_entry.entry_id)

result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={CONF_MAC: "22:22:22"},
)
assert result["type"] == "create_entry"
assert config_entry.options[CONF_MAC] == "22:22:22"


async def test_options_with_empty_mac(hass, remote):
"""Test the options flow with an empty string for mac."""
config_entry = MockConfigEntry(domain=DOMAIN, data={CONF_HOST: "hostname"},)

config_entry.add_to_hass(hass)

# simulate an error in getting the mac address
with patch(
"homeassistant.components.samsungtv.config_flow.get_mac_address",
return_value="11:11:11",
):
result = await hass.config_entries.options.async_init(config_entry.entry_id)

assert result["type"] == "form"
assert result["step_id"] == "init"

result = await hass.config_entries.options.async_configure(
result["flow_id"], user_input={CONF_MAC: " "},
)

assert result["type"] == "create_entry"
assert config_entry.options.get(CONF_MAC) is None
57 changes: 56 additions & 1 deletion tests/components/samsungtv/test_media_player.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@
CONF_ON_ACTION,
DOMAIN as SAMSUNGTV_DOMAIN,
)
from homeassistant.components.samsungtv.media_player import SUPPORT_SAMSUNGTV
from homeassistant.components.samsungtv.media_player import (
SUPPORT_SAMSUNGTV,
async_get_on_script_from_mac,
)
from homeassistant.const import (
ATTR_DEVICE_CLASS,
ATTR_ENTITY_ID,
Expand Down Expand Up @@ -101,6 +104,8 @@
"token": "abcde",
}

MOCK_OPTIONS = {"mac": "11:11:11"}

ENTITY_ID_NOTURNON = f"{DOMAIN}.fake_noturnon"
MOCK_CONFIG_NOTURNON = {
SAMSUNGTV_DOMAIN: [
Expand Down Expand Up @@ -742,3 +747,53 @@ async def test_select_source_invalid_source(hass, remote):
assert remote.control.call_count == 0
assert remote.close.call_count == 0
assert remote.call_count == 1


async def test_on_script_from_mac(hass, remote):
"""Test creation of on-script from mac address."""

result = async_get_on_script_from_mac(hass, "11:11:11")

assert len(result.sequence) == 1
assert result.sequence[0]["service"] == "wake_on_lan.send_magic_packet"
assert result.sequence[0]["data"] == {"mac": "11:11:11"}


async def test_setup_with_options(hass, remote):
"""Test setup with mac address in options."""
entity_id = f"{DOMAIN}.fake"
entry = MockConfigEntry(
domain=SAMSUNGTV_DOMAIN,
data=MOCK_ENTRY_WS,
unique_id=entity_id,
options=MOCK_OPTIONS,
)
entry.add_to_hass(hass)

with patch(
"homeassistant.components.samsungtv.media_player.async_get_on_script_from_mac"
) as on_script:
await async_setup_component(hass, SAMSUNGTV_DOMAIN, {})
# await hass.config_entries.async_setup(entry.entry_id)
await hass.async_block_till_done()

assert on_script.call_count == 1
assert on_script.call_args_list == [call(hass, MOCK_OPTIONS["mac"])]


async def test_setup_without_options(hass, remote):
"""Test setup without mac address in options."""
entity_id = f"{DOMAIN}.fake"
entry = MockConfigEntry(
domain=SAMSUNGTV_DOMAIN, data=MOCK_ENTRY_WS, unique_id=entity_id
)
entry.add_to_hass(hass)

with patch(
"homeassistant.components.samsungtv.media_player.async_get_on_script_from_mac"
) as on_script:
await async_setup_component(hass, SAMSUNGTV_DOMAIN, {})
# await hass.config_entries.async_setup(entry.entry_id)
await hass.async_block_till_done()

assert on_script.call_count == 0