SmartThings remove SmartApp/Automation on integration removal (#21594)

* Add clean-up logic upon entry removal

* Removed unecessary app removal from migration

* Change log level and clarified code

homeassistant/components/smartthings/__init__.py

@@ -46,23 +46,7 @@ async def async_migrate_entry(hass: HomeAssistantType, entry: ConfigEntry):
     integration setup again so we can properly retrieve the needed data
     elements. Force this by removing the entry and triggering a new flow.
     """
-    from pysmartthings import SmartThings
-
-    # Remove the installed_app, which if already removed raises a 403 error.
-    api = SmartThings(async_get_clientsession(hass),
-                      entry.data[CONF_ACCESS_TOKEN])
-    installed_app_id = entry.data[CONF_INSTALLED_APP_ID]
-    try:
-        await api.delete_installed_app(installed_app_id)
-    except ClientResponseError as ex:
-        if ex.status == 403:
-            _LOGGER.exception("Installed app %s has already been removed",
-                              installed_app_id)
-        else:
-            raise
-    _LOGGER.debug("Removed installed app %s", installed_app_id)
-
-    # Delete the entry
+    # Remove the entry which will invoke the callback to delete the app.
     hass.async_create_task(
         hass.config_entries.async_remove(entry.entry_id))
     # only create new flow if there isn't a pending one for SmartThings.
@@ -194,6 +178,47 @@ async def async_unload_entry(hass: HomeAssistantType, entry: ConfigEntry):
     return all(await asyncio.gather(*tasks))
 
 
+async def async_remove_entry(
+        hass: HomeAssistantType, entry: ConfigEntry) -> None:
+    """Perform clean-up when entry is being removed."""
+    from pysmartthings import SmartThings
+
+    api = SmartThings(async_get_clientsession(hass),
+                      entry.data[CONF_ACCESS_TOKEN])
+
+    # Remove the installed_app, which if already removed raises a 403 error.
+    installed_app_id = entry.data[CONF_INSTALLED_APP_ID]
+    try:
+        await api.delete_installed_app(installed_app_id)
+    except ClientResponseError as ex:
+        if ex.status == 403:
+            _LOGGER.debug("Installed app %s has already been removed",
+                          installed_app_id, exc_info=True)
+        else:
+            raise
+    _LOGGER.debug("Removed installed app %s", installed_app_id)
+
+    # Remove the app if not referenced by other entries, which if already
+    # removed raises a 403 error.
+    app_id = entry.data[CONF_APP_ID]
+    app_count = sum(1 for entry in hass.config_entries.async_entries(DOMAIN)
+                    if entry.data[CONF_APP_ID] == app_id)
+    if app_count > 1:
+        _LOGGER.debug("App %s was not removed because it is in use by other"
+                      "config entries", app_id)
+        return
+    # Remove the app
+    try:
+        await api.delete_app(app_id)
+    except ClientResponseError as ex:
+        if ex.status == 403:
+            _LOGGER.debug("App %s has already been removed",
+                          app_id, exc_info=True)
+        else:
+            raise
+    _LOGGER.debug("Removed app %s", app_id)
+
+
 class DeviceBroker:
     """Manages an individual SmartThings config entry."""
 

homeassistant/components/smartthings/sensor.py

@@ -5,7 +5,7 @@ from typing import Optional, Sequence
 from homeassistant.const import (
     DEVICE_CLASS_BATTERY, DEVICE_CLASS_HUMIDITY, DEVICE_CLASS_ILLUMINANCE,
     DEVICE_CLASS_TEMPERATURE, DEVICE_CLASS_TIMESTAMP, MASS_KILOGRAMS,
-    TEMP_CELSIUS, TEMP_FAHRENHEIT, POWER_WATT)
+    POWER_WATT, TEMP_CELSIUS, TEMP_FAHRENHEIT)
 
 from . import SmartThingsEntity
 from .const import DATA_BROKERS, DOMAIN

tests/components/smartthings/test_init.py

@@ -13,7 +13,7 @@ from homeassistant.components.smartthings.const import (
 from homeassistant.exceptions import ConfigEntryNotReady
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
 
-from tests.common import mock_coro
+from tests.common import MockConfigEntry, mock_coro
 
 
 async def test_migration_creates_new_flow(
@@ -22,12 +22,14 @@ async def test_migration_creates_new_flow(
     config_entry.version = 1
     setattr(hass.config_entries, '_entries', [config_entry])
     api = smartthings_mock.return_value
-    api.delete_installed_app.return_value = mock_coro()
+    api.delete_installed_app.side_effect = lambda _: mock_coro()
+    api.delete_app.side_effect = lambda _: mock_coro()
 
     await smartthings.async_migrate_entry(hass, config_entry)
+    await hass.async_block_till_done()
 
     assert api.delete_installed_app.call_count == 1
-    await hass.async_block_till_done()
+    assert api.delete_app.call_count == 1
     assert not hass.config_entries.async_entries(DOMAIN)
     flows = hass.config_entries.flow.async_progress()
     assert len(flows) == 1
@@ -209,6 +211,113 @@ async def test_unload_entry(hass, config_entry):
         assert forward_mock.call_count == len(SUPPORTED_PLATFORMS)
 
 
+async def test_remove_entry(hass, config_entry, smartthings_mock):
+    """Test that the installed app and app are removed up."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro()
+    api.delete_app.side_effect = lambda _: mock_coro()
+    # Act
+    await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 1
+
+
+async def test_remove_entry_app_in_use(hass, config_entry, smartthings_mock):
+    """Test app is not removed if in use by another config entry."""
+    # Arrange
+    data = config_entry.data.copy()
+    data[CONF_INSTALLED_APP_ID] = str(uuid4())
+    entry2 = MockConfigEntry(version=2, domain=DOMAIN, data=data)
+    setattr(hass.config_entries, '_entries', [config_entry, entry2])
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro()
+    # Act
+    await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 0
+
+
+async def test_remove_entry_already_deleted(
+        hass, config_entry, smartthings_mock):
+    """Test handles when the apps have already been removed."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro(
+        exception=ClientResponseError(None, None, status=403))
+    api.delete_app.side_effect = lambda _: mock_coro(
+        exception=ClientResponseError(None, None, status=403))
+    # Act
+    await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 1
+
+
+async def test_remove_entry_installedapp_api_error(
+        hass, config_entry, smartthings_mock):
+    """Test raises exceptions removing the installed app."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro(
+        exception=ClientResponseError(None, None, status=500))
+    # Act
+    with pytest.raises(ClientResponseError):
+        await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 0
+
+
+async def test_remove_entry_installedapp_unknown_error(
+        hass, config_entry, smartthings_mock):
+    """Test raises exceptions removing the installed app."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro(
+        exception=Exception)
+    # Act
+    with pytest.raises(Exception):
+        await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 0
+
+
+async def test_remove_entry_app_api_error(
+        hass, config_entry, smartthings_mock):
+    """Test raises exceptions removing the app."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro()
+    api.delete_app.side_effect = lambda _: mock_coro(
+        exception=ClientResponseError(None, None, status=500))
+    # Act
+    with pytest.raises(ClientResponseError):
+        await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 1
+
+
+async def test_remove_entry_app_unknown_error(
+        hass, config_entry, smartthings_mock):
+    """Test raises exceptions removing the app."""
+    # Arrange
+    api = smartthings_mock.return_value
+    api.delete_installed_app.side_effect = lambda _: mock_coro()
+    api.delete_app.side_effect = lambda _: mock_coro(
+        exception=Exception)
+    # Act
+    with pytest.raises(Exception):
+        await smartthings.async_remove_entry(hass, config_entry)
+    # Assert
+    assert api.delete_installed_app.call_count == 1
+    assert api.delete_app.call_count == 1
+
+
 async def test_broker_regenerates_token(
         hass, config_entry):
     """Test the device broker regenerates the refresh token."""