Harmony config flow improvements (#33018)

* Harmony config flow improvements

* Address followup review comments from #32919

* pylint -- catching my naming error

* remove leftovers from refactor

homeassistant/components/harmony/.translations/en.json

@@ -17,7 +17,6 @@
     },
     "error": {
       "cannot_connect": "Failed to connect, please try again",
-      "invalid_auth": "Invalid authentication",
       "unknown": "Unexpected error"
     },
     "abort": {

homeassistant/components/harmony/__init__.py

@@ -5,11 +5,12 @@ import logging
 from homeassistant.components.remote import ATTR_ACTIVITY, ATTR_DELAY_SECS
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.const import CONF_HOST, CONF_NAME
-from homeassistant.core import HomeAssistant
+from homeassistant.core import HomeAssistant, callback
 from homeassistant.exceptions import ConfigEntryNotReady
+from homeassistant.helpers.dispatcher import async_dispatcher_send
 
-from .const import DOMAIN, PLATFORMS
-from .remote import DEVICES, HarmonyRemote
+from .const import DOMAIN, HARMONY_OPTIONS_UPDATE, PLATFORMS
+from .remote import HarmonyRemote
 
 _LOGGER = logging.getLogger(__name__)
 
@@ -23,19 +24,16 @@ async def async_setup(hass: HomeAssistant, config: dict):
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
     """Set up Logitech Harmony Hub from a config entry."""
+    # As there currently is no way to import options from yaml
+    # when setting up a config entry, we fallback to adding
+    # the options to the config entry and pull them out here if
+    # they are missing from the options
+    _async_import_options_from_data_if_missing(hass, entry)
 
-    conf = entry.data
-    address = conf[CONF_HOST]
-    name = conf.get(CONF_NAME)
-    activity = conf.get(ATTR_ACTIVITY)
-    delay_secs = conf.get(ATTR_DELAY_SECS)
-
-    _LOGGER.info(
-        "Loading Harmony Platform: %s at %s, startup activity: %s",
-        name,
-        address,
-        activity,
-    )
+    address = entry.data[CONF_HOST]
+    name = entry.data[CONF_NAME]
+    activity = entry.options.get(ATTR_ACTIVITY)
+    delay_secs = entry.options.get(ATTR_DELAY_SECS)
 
     harmony_conf_file = hass.config.path(f"harmony_{entry.unique_id}.conf")
     try:
@@ -45,7 +43,6 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
         raise ConfigEntryNotReady
 
     hass.data[DOMAIN][entry.entry_id] = device
-    DEVICES.append(device)
 
     entry.add_update_listener(_update_listener)
 
@@ -57,16 +54,24 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry):
     return True
 
 
-async def _update_listener(hass, entry):
+@callback
+def _async_import_options_from_data_if_missing(hass: HomeAssistant, entry: ConfigEntry):
+    options = dict(entry.options)
+    modified = 0
+    for importable_option in [ATTR_ACTIVITY, ATTR_DELAY_SECS]:
+        if importable_option not in entry.options and importable_option in entry.data:
+            options[importable_option] = entry.data[importable_option]
+            modified = 1
+
+    if modified:
+        hass.config_entries.async_update_entry(entry, options=options)
+
+
+async def _update_listener(hass: HomeAssistant, entry: ConfigEntry):
     """Handle options update."""
-
-    device = hass.data[DOMAIN][entry.entry_id]
-
-    if ATTR_DELAY_SECS in entry.options:
-        device.delay_seconds = entry.options[ATTR_DELAY_SECS]
-
-    if ATTR_ACTIVITY in entry.options:
-        device.default_activity = entry.options[ATTR_ACTIVITY]
+    async_dispatcher_send(
+        hass, f"{HARMONY_OPTIONS_UPDATE}-{entry.unique_id}", entry.options
+    )
 
 
 async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry):
@@ -85,7 +90,6 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry):
     await device.shutdown()
 
     if unload_ok:
-        DEVICES.remove(hass.data[DOMAIN][entry.entry_id])
         hass.data[DOMAIN].pop(entry.entry_id)
 
     return unload_ok

homeassistant/components/harmony/config_flow.py

@@ -147,18 +147,12 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     async def _async_create_entry_from_valid_input(self, validated, user_input):
         """Single path to create the config entry from validated input."""
         await self.async_set_unique_id(validated[UNIQUE_ID])
-        if self._host_already_configured(validated):
-            return self.async_abort(reason="already_configured")
         self._abort_if_unique_id_configured()
-        config_entry = self.async_create_entry(
-            title=validated[CONF_NAME],
-            data={CONF_NAME: validated[CONF_NAME], CONF_HOST: validated[CONF_HOST]},
-        )
-        # Options from yaml are preserved
-        options = _options_from_user_input(user_input)
-        if options:
-            config_entry["options"] = options
-        return config_entry
+        data = {CONF_NAME: validated[CONF_NAME], CONF_HOST: validated[CONF_HOST]}
+        # Options from yaml are preserved, we will pull them out when
+        # we setup the config entry
+        data.update(_options_from_user_input(user_input))
+        return self.async_create_entry(title=validated[CONF_NAME], data=data)
 
     def _host_already_configured(self, user_input):
         """See if we already have a harmony matching user input configured."""

homeassistant/components/harmony/const.py

@@ -4,3 +4,5 @@ SERVICE_SYNC = "sync"
 SERVICE_CHANGE_CHANNEL = "change_channel"
 PLATFORMS = ["remote"]
 UNIQUE_ID = "unique_id"
+ACTIVITY_POWER_OFF = "PowerOff"
+HARMONY_OPTIONS_UPDATE = "harmony_options_update"

homeassistant/components/harmony/remote.py

@@ -25,8 +25,15 @@ from homeassistant.config_entries import SOURCE_IMPORT, ConfigEntry
 from homeassistant.const import ATTR_ENTITY_ID, CONF_HOST, CONF_NAME
 from homeassistant.core import HomeAssistant
 import homeassistant.helpers.config_validation as cv
+from homeassistant.helpers.dispatcher import async_dispatcher_connect
 
-from .const import DOMAIN, SERVICE_CHANGE_CHANNEL, SERVICE_SYNC
+from .const import (
+    ACTIVITY_POWER_OFF,
+    DOMAIN,
+    HARMONY_OPTIONS_UPDATE,
+    SERVICE_CHANGE_CHANNEL,
+    SERVICE_SYNC,
+)
 from .util import find_unique_id_for_remote
 
 _LOGGER = logging.getLogger(__name__)
@@ -34,13 +41,12 @@ _LOGGER = logging.getLogger(__name__)
 ATTR_CHANNEL = "channel"
 ATTR_CURRENT_ACTIVITY = "current_activity"
 
-DEVICES = []
 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
     {
         vol.Optional(ATTR_ACTIVITY): cv.string,
         vol.Required(CONF_NAME): cv.string,
         vol.Optional(ATTR_DELAY_SECS, default=DEFAULT_DELAY_SECS): vol.Coerce(float),
-        vol.Optional(CONF_HOST): cv.string,
+        vol.Required(CONF_HOST): cv.string,
         # The client ignores port so lets not confuse the user by pretenting we do anything with this
     },
     extra=vol.ALLOW_EXTRA,
@@ -63,13 +69,6 @@ async def async_setup_platform(hass, config, async_add_entities, discovery_info=
         # Now handled by ssdp in the config flow
         return
 
-    if CONF_HOST not in config:
-        _LOGGER.error(
-            "The harmony remote '%s' cannot be setup because configuration now requires a host when configured manually.",
-            config[CONF_NAME],
-        )
-        return
-
     hass.async_create_task(
         hass.config_entries.flow.async_init(
             DOMAIN, context={"source": SOURCE_IMPORT}, data=config
@@ -84,7 +83,7 @@ async def async_setup_entry(
 
     device = hass.data[DOMAIN][entry.entry_id]
 
-    _LOGGER.info("Harmony Remote: %s", device)
+    _LOGGER.debug("Harmony Remote: %s", device)
 
     async_add_entities([device])
     register_services(hass)
@@ -92,6 +91,30 @@ async def async_setup_entry(
 
 def register_services(hass):
     """Register all services for harmony devices."""
+
+    async def _apply_service(service, service_func, *service_func_args):
+        """Handle services to apply."""
+        entity_ids = service.data.get("entity_id")
+
+        want_devices = [
+            hass.data[DOMAIN][config_entry_id] for config_entry_id in hass.data[DOMAIN]
+        ]
+
+        if entity_ids:
+            want_devices = [
+                device for device in want_devices if device.entity_id in entity_ids
+            ]
+
+        for device in want_devices:
+            await service_func(device, *service_func_args)
+
+    async def _sync_service(service):
+        await _apply_service(service, HarmonyRemote.sync)
+
+    async def _change_channel_service(service):
+        channel = service.data.get(ATTR_CHANNEL)
+        await _apply_service(service, HarmonyRemote.change_channel, channel)
+
     hass.services.async_register(
         DOMAIN, SERVICE_SYNC, _sync_service, schema=HARMONY_SYNC_SCHEMA
     )
@@ -104,28 +127,6 @@ def register_services(hass):
     )
 
 
-async def _apply_service(service, service_func, *service_func_args):
-    """Handle services to apply."""
-    entity_ids = service.data.get("entity_id")
-
-    if entity_ids:
-        _devices = [device for device in DEVICES if device.entity_id in entity_ids]
-    else:
-        _devices = DEVICES
-
-    for device in _devices:
-        await service_func(device, *service_func_args)
-
-
-async def _sync_service(service):
-    await _apply_service(service, HarmonyRemote.sync)
-
-
-async def _change_channel_service(service):
-    channel = service.data.get(ATTR_CHANNEL)
-    await _apply_service(service, HarmonyRemote.change_channel, channel)
-
-
 class HarmonyRemote(remote.RemoteDevice):
     """Remote representation used to control a Harmony device."""
 
@@ -135,26 +136,12 @@ class HarmonyRemote(remote.RemoteDevice):
         self.host = host
         self._state = None
         self._current_activity = None
-        self._default_activity = activity
+        self.default_activity = activity
         self._client = HarmonyClient(ip_address=host)
         self._config_path = out_path
-        self._delay_secs = delay_secs
+        self.delay_secs = delay_secs
         self._available = False
-
-    @property
-    def delay_secs(self):
-        """Delay seconds between sending commands."""
-        return self._delay_secs
-
-    @delay_secs.setter
-    def delay_secs(self, delay_secs):
-        """Update the delay seconds (from options flow)."""
-        self._delay_secs = delay_secs
-
-    @property
-    def default_activity(self):
-        """Activity used when non specified."""
-        return self._default_activity
+        self._undo_dispatch_subscription = None
 
     @property
     def activity_names(self):
@@ -164,15 +151,23 @@ class HarmonyRemote(remote.RemoteDevice):
         # Remove both ways of representing PowerOff
         if None in activities:
             activities.remove(None)
-        if "PowerOff" in activities:
-            activities.remove("PowerOff")
+        if ACTIVITY_POWER_OFF in activities:
+            activities.remove(ACTIVITY_POWER_OFF)
 
         return activities
 
-    @default_activity.setter
-    def default_activity(self, activity):
-        """Update the default activity (from options flow)."""
-        self._default_activity = activity
+    async def async_will_remove_from_hass(self):
+        """Undo subscription."""
+        if self._undo_dispatch_subscription:
+            self._undo_dispatch_subscription()
+
+    async def _async_update_options(self, data):
+        """Change options when the options flow does."""
+        if ATTR_DELAY_SECS in data:
+            self.delay_secs = data[ATTR_DELAY_SECS]
+
+        if ATTR_ACTIVITY in data:
+            self.default_activity = data[ATTR_ACTIVITY]
 
     async def async_added_to_hass(self):
         """Complete the initialization."""
@@ -185,6 +180,12 @@ class HarmonyRemote(remote.RemoteDevice):
             disconnect=self.got_disconnected,
         )
 
+        self._undo_dispatch_subscription = async_dispatcher_connect(
+            self.hass,
+            f"{HARMONY_OPTIONS_UPDATE}-{self.unique_id}",
+            self._async_update_options,
+        )
+
         # Store Harmony HUB config, this will also update our current
         # activity
         await self.new_config()
@@ -294,7 +295,7 @@ class HarmonyRemote(remote.RemoteDevice):
         """Start an activity from the Harmony device."""
         _LOGGER.debug("%s: Turn On", self.name)
 
-        activity = kwargs.get(ATTR_ACTIVITY, self._default_activity)
+        activity = kwargs.get(ATTR_ACTIVITY, self.default_activity)
 
         if activity:
             activity_id = None
@@ -351,7 +352,7 @@ class HarmonyRemote(remote.RemoteDevice):
             return
 
         num_repeats = kwargs[ATTR_NUM_REPEATS]
-        delay_secs = kwargs.get(ATTR_DELAY_SECS, self._delay_secs)
+        delay_secs = kwargs.get(ATTR_DELAY_SECS, self.delay_secs)
         hold_secs = kwargs[ATTR_HOLD_SECS]
         _LOGGER.debug(
             "Sending commands to device %s holding for %s seconds "

homeassistant/components/harmony/strings.json

@@ -17,7 +17,6 @@
     },
     "error": {
       "cannot_connect": "Failed to connect, please try again",
-      "invalid_auth": "Invalid authentication",
       "unknown": "Unexpected error"
     },
     "abort": {

homeassistant/components/harmony/util.py

@@ -8,8 +8,5 @@ def find_unique_id_for_remote(harmony: HarmonyClient):
     if websocket_unique_id is not None:
         return websocket_unique_id
 
-    xmpp_unique_id = harmony.config.get("global", {}).get("timeStampHash")
-    if not xmpp_unique_id:
-        return None
-
-    return xmpp_unique_id.split(";")[-1]
+    # fallback to the xmpp unique id if websocket is not available
+    return harmony.config["global"]["timeStampHash"].split(";")[-1]

tests/components/harmony/test_config_flow.py

@@ -76,11 +76,12 @@ async def test_form_import(hass):
     assert result["data"] == {
         "host": "1.2.3.4",
         "name": "friend",
-    }
-    assert result["options"] == {
         "activity": "Watch TV",
         "delay_secs": 0.9,
     }
+    # It is not possible to import options at this time
+    # so they end up in the config entry data and are
+    # used a fallback when they are not in options
     await hass.async_block_till_done()
     assert len(mock_setup.mock_calls) == 1
     assert len(mock_setup_entry.mock_calls) == 1