Address late review of kostal plenticore (#75297)

* Changtes from review #64927

* Fix unit tests for number.

* Changes from review.

homeassistant/components/kostal_plenticore/__init__.py

@@ -12,7 +12,7 @@ from .helper import Plenticore
 
 _LOGGER = logging.getLogger(__name__)
 
-PLATFORMS = [Platform.SELECT, Platform.SENSOR, Platform.SWITCH, Platform.NUMBER]
+PLATFORMS = [Platform.NUMBER, Platform.SELECT, Platform.SENSOR, Platform.SWITCH]
 
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:

homeassistant/components/kostal_plenticore/const.py

@@ -1,8 +1,6 @@
 """Constants for the Kostal Plenticore Solar Inverter integration."""
-from dataclasses import dataclass
 from typing import NamedTuple
 
-from homeassistant.components.number import NumberEntityDescription
 from homeassistant.components.sensor import (
     ATTR_STATE_CLASS,
     SensorDeviceClass,
@@ -18,7 +16,6 @@ from homeassistant.const import (
     PERCENTAGE,
     POWER_WATT,
 )
-from homeassistant.helpers.entity import EntityCategory
 
 DOMAIN = "kostal_plenticore"
 
@@ -794,57 +791,6 @@ SENSOR_PROCESS_DATA = [
 ]
 
 
-@dataclass
-class PlenticoreNumberEntityDescriptionMixin:
-    """Define an entity description mixin for number entities."""
-
-    module_id: str
-    data_id: str
-    fmt_from: str
-    fmt_to: str
-
-
-@dataclass
-class PlenticoreNumberEntityDescription(
-    NumberEntityDescription, PlenticoreNumberEntityDescriptionMixin
-):
-    """Describes a Plenticore number entity."""
-
-
-NUMBER_SETTINGS_DATA = [
-    PlenticoreNumberEntityDescription(
-        key="battery_min_soc",
-        entity_category=EntityCategory.CONFIG,
-        entity_registry_enabled_default=False,
-        icon="mdi:battery-negative",
-        name="Battery min SoC",
-        native_unit_of_measurement=PERCENTAGE,
-        native_max_value=100,
-        native_min_value=5,
-        native_step=5,
-        module_id="devices:local",
-        data_id="Battery:MinSoc",
-        fmt_from="format_round",
-        fmt_to="format_round_back",
-    ),
-    PlenticoreNumberEntityDescription(
-        key="battery_min_home_consumption",
-        device_class=SensorDeviceClass.POWER,
-        entity_category=EntityCategory.CONFIG,
-        entity_registry_enabled_default=False,
-        name="Battery min Home Consumption",
-        native_unit_of_measurement=POWER_WATT,
-        native_max_value=38000,
-        native_min_value=50,
-        native_step=1,
-        module_id="devices:local",
-        data_id="Battery:MinHomeComsumption",
-        fmt_from="format_round",
-        fmt_to="format_round_back",
-    ),
-]
-
-
 class SwitchData(NamedTuple):
     """Representation of a SelectData tuple."""
 

homeassistant/components/kostal_plenticore/number.py

@@ -2,25 +2,82 @@
 from __future__ import annotations
 
 from abc import ABC
+from dataclasses import dataclass
 from datetime import timedelta
-from functools import partial
 import logging
 
 from kostal.plenticore import SettingsData
 
-from homeassistant.components.number import NumberEntity, NumberMode
+from homeassistant.components.number import (
+    NumberEntity,
+    NumberEntityDescription,
+    NumberMode,
+)
+from homeassistant.components.sensor import SensorDeviceClass
 from homeassistant.config_entries import ConfigEntry
+from homeassistant.const import PERCENTAGE, POWER_WATT
 from homeassistant.core import HomeAssistant
-from homeassistant.helpers.entity import DeviceInfo
+from homeassistant.helpers.entity import DeviceInfo, EntityCategory
 from homeassistant.helpers.entity_platform import AddEntitiesCallback
 from homeassistant.helpers.update_coordinator import CoordinatorEntity
 
-from .const import DOMAIN, NUMBER_SETTINGS_DATA, PlenticoreNumberEntityDescription
+from .const import DOMAIN
 from .helper import PlenticoreDataFormatter, SettingDataUpdateCoordinator
 
 _LOGGER = logging.getLogger(__name__)
 
 
+@dataclass
+class PlenticoreNumberEntityDescriptionMixin:
+    """Define an entity description mixin for number entities."""
+
+    module_id: str
+    data_id: str
+    fmt_from: str
+    fmt_to: str
+
+
+@dataclass
+class PlenticoreNumberEntityDescription(
+    NumberEntityDescription, PlenticoreNumberEntityDescriptionMixin
+):
+    """Describes a Plenticore number entity."""
+
+
+NUMBER_SETTINGS_DATA = [
+    PlenticoreNumberEntityDescription(
+        key="battery_min_soc",
+        entity_category=EntityCategory.CONFIG,
+        entity_registry_enabled_default=False,
+        icon="mdi:battery-negative",
+        name="Battery min SoC",
+        native_unit_of_measurement=PERCENTAGE,
+        native_max_value=100,
+        native_min_value=5,
+        native_step=5,
+        module_id="devices:local",
+        data_id="Battery:MinSoc",
+        fmt_from="format_round",
+        fmt_to="format_round_back",
+    ),
+    PlenticoreNumberEntityDescription(
+        key="battery_min_home_consumption",
+        device_class=SensorDeviceClass.POWER,
+        entity_category=EntityCategory.CONFIG,
+        entity_registry_enabled_default=False,
+        name="Battery min Home Consumption",
+        native_unit_of_measurement=POWER_WATT,
+        native_max_value=38000,
+        native_min_value=50,
+        native_step=1,
+        module_id="devices:local",
+        data_id="Battery:MinHomeComsumption",
+        fmt_from="format_round",
+        fmt_to="format_round_back",
+    ),
+]
+
+
 async def async_setup_entry(
     hass: HomeAssistant, entry: ConfigEntry, async_add_entities: AddEntitiesCallback
 ) -> None:
@@ -54,10 +111,9 @@ async def async_setup_entry(
             continue
 
         setting_data = next(
-            filter(
-                partial(lambda id, sd: id == sd.id, description.data_id),
-                available_settings_data[description.module_id],
-            )
+            sd
+            for sd in available_settings_data[description.module_id]
+            if description.data_id == sd.id
         )
 
         entities.append(

tests/components/kostal_plenticore/test_number.py

@@ -1,72 +1,98 @@
 """Test Kostal Plenticore number."""
 
-from unittest.mock import AsyncMock, MagicMock
+from collections.abc import Generator
+from datetime import timedelta
+from unittest.mock import patch
 
-from kostal.plenticore import SettingsData
+from kostal.plenticore import PlenticoreApiClient, SettingsData
 import pytest
 
-from homeassistant.components.kostal_plenticore.const import (
-    PlenticoreNumberEntityDescription,
+from homeassistant.components.number import (
+    ATTR_VALUE,
+    DOMAIN as NUMBER_DOMAIN,
+    SERVICE_SET_VALUE,
 )
-from homeassistant.components.kostal_plenticore.number import PlenticoreDataNumber
+from homeassistant.components.number.const import ATTR_MAX, ATTR_MIN
+from homeassistant.const import ATTR_ENTITY_ID, STATE_UNAVAILABLE
 from homeassistant.core import HomeAssistant
 from homeassistant.helpers.entity_registry import async_get
+from homeassistant.util import dt
 
-from tests.common import MockConfigEntry
+from tests.common import MockConfigEntry, async_fire_time_changed
 
 
 @pytest.fixture
-def mock_coordinator() -> MagicMock:
-    """Return a mocked coordinator for tests."""
-    coordinator = MagicMock()
-    coordinator.async_write_data = AsyncMock()
-    coordinator.async_refresh = AsyncMock()
-    return coordinator
+def mock_plenticore_client() -> Generator[PlenticoreApiClient, None, None]:
+    """Return a patched PlenticoreApiClient."""
+    with patch(
+        "homeassistant.components.kostal_plenticore.helper.PlenticoreApiClient",
+        autospec=True,
+    ) as plenticore_client_class:
+        yield plenticore_client_class.return_value
 
 
 @pytest.fixture
-def mock_number_description() -> PlenticoreNumberEntityDescription:
-    """Return a PlenticoreNumberEntityDescription for tests."""
-    return PlenticoreNumberEntityDescription(
-        key="mock key",
-        module_id="moduleid",
-        data_id="dataid",
-        native_min_value=0,
-        native_max_value=1000,
-        fmt_from="format_round",
-        fmt_to="format_round_back",
-    )
+def mock_get_setting_values(mock_plenticore_client: PlenticoreApiClient) -> list:
+    """Add a setting value to the given Plenticore client.
 
+    Returns a list with setting values which can be extended by test cases.
+    """
 
-@pytest.fixture
-def mock_setting_data() -> SettingsData:
-    """Return a default SettingsData for tests."""
-    return SettingsData(
-        {
-            "default": None,
-            "min": None,
-            "access": None,
-            "max": None,
-            "unit": None,
-            "type": None,
-            "id": "data_id",
-        }
-    )
-
-
-async def test_setup_all_entries(
-    hass: HomeAssistant, mock_config_entry: MockConfigEntry, mock_plenticore: MagicMock
-):
-    """Test if all available entries are setup up."""
-    mock_plenticore.client.get_settings.return_value = {
+    mock_plenticore_client.get_settings.return_value = {
         "devices:local": [
-            SettingsData({"id": "Battery:MinSoc", "min": None, "max": None}),
             SettingsData(
-                {"id": "Battery:MinHomeComsumption", "min": None, "max": None}
+                {
+                    "default": None,
+                    "min": 5,
+                    "max": 100,
+                    "access": "readwrite",
+                    "unit": "%",
+                    "type": "byte",
+                    "id": "Battery:MinSoc",
+                }
+            ),
+            SettingsData(
+                {
+                    "default": None,
+                    "min": 50,
+                    "max": 38000,
+                    "access": "readwrite",
+                    "unit": "W",
+                    "type": "byte",
+                    "id": "Battery:MinHomeComsumption",
+                }
             ),
         ]
     }
 
+    # this values are always retrieved by the integration on startup
+    setting_values = [
+        {
+            "devices:local": {
+                "Properties:SerialNo": "42",
+                "Branding:ProductName1": "PLENTICORE",
+                "Branding:ProductName2": "plus 10",
+                "Properties:VersionIOC": "01.45",
+                "Properties:VersionMC": " 01.46",
+            },
+            "scb:network": {"Hostname": "scb"},
+        }
+    ]
+
+    mock_plenticore_client.get_setting_values.side_effect = setting_values
+
+    return setting_values
+
+
+async def test_setup_all_entries(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_plenticore_client: PlenticoreApiClient,
+    mock_get_setting_values: list,
+    entity_registry_enabled_by_default,
+):
+    """Test if all available entries are setup."""
+
     mock_config_entry.add_to_hass(hass)
 
     assert await hass.config_entries.async_setup(mock_config_entry.entry_id)
@@ -78,10 +104,16 @@ async def test_setup_all_entries(
 
 
 async def test_setup_no_entries(
-    hass: HomeAssistant, mock_config_entry: MockConfigEntry, mock_plenticore: MagicMock
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_plenticore_client: PlenticoreApiClient,
+    mock_get_setting_values: list,
+    entity_registry_enabled_by_default,
 ):
-    """Test that no entries are setup up."""
-    mock_plenticore.client.get_settings.return_value = []
+    """Test that no entries are setup if Plenticore does not provide data."""
+
+    mock_plenticore_client.get_settings.return_value = []
+
     mock_config_entry.add_to_hass(hass)
 
     assert await hass.config_entries.async_setup(mock_config_entry.entry_id)
@@ -92,106 +124,81 @@ async def test_setup_no_entries(
     assert ent_reg.async_get("number.scb_battery_min_home_consumption") is None
 
 
-def test_number_returns_value_if_available(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-    mock_setting_data: SettingsData,
+async def test_number_has_value(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_plenticore_client: PlenticoreApiClient,
+    mock_get_setting_values: list,
+    entity_registry_enabled_by_default,
 ):
-    """Test if value property on PlenticoreDataNumber returns an int if available."""
+    """Test if number has a value if data is provided on update."""
 
-    mock_coordinator.data = {"moduleid": {"dataid": "42"}}
+    mock_get_setting_values.append({"devices:local": {"Battery:MinSoc": "42"}})
 
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, mock_setting_data
-    )
+    mock_config_entry.add_to_hass(hass)
 
-    assert entity.value == 42
-    assert type(entity.value) == int
+    await hass.config_entries.async_setup(mock_config_entry.entry_id)
+    await hass.async_block_till_done()
+
+    async_fire_time_changed(hass, dt.utcnow() + timedelta(seconds=3))
+    await hass.async_block_till_done()
+
+    state = hass.states.get("number.scb_battery_min_soc")
+    assert state.state == "42"
+    assert state.attributes[ATTR_MIN] == 5
+    assert state.attributes[ATTR_MAX] == 100
 
 
-def test_number_returns_none_if_unavailable(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-    mock_setting_data: SettingsData,
+async def test_number_is_unavailable(
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_plenticore_client: PlenticoreApiClient,
+    mock_get_setting_values: list,
+    entity_registry_enabled_by_default,
 ):
-    """Test if value property on PlenticoreDataNumber returns none if unavailable."""
+    """Test if number is unavailable if no data is provided on update."""
 
-    mock_coordinator.data = {}  # makes entity not available
+    mock_config_entry.add_to_hass(hass)
 
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, mock_setting_data
-    )
+    await hass.config_entries.async_setup(mock_config_entry.entry_id)
+    await hass.async_block_till_done()
 
-    assert entity.value is None
+    async_fire_time_changed(hass, dt.utcnow() + timedelta(seconds=3))
+    await hass.async_block_till_done()
+
+    state = hass.states.get("number.scb_battery_min_soc")
+    assert state.state == STATE_UNAVAILABLE
 
 
 async def test_set_value(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-    mock_setting_data: SettingsData,
+    hass: HomeAssistant,
+    mock_config_entry: MockConfigEntry,
+    mock_plenticore_client: PlenticoreApiClient,
+    mock_get_setting_values: list,
+    entity_registry_enabled_by_default,
 ):
-    """Test if set value calls coordinator with new value."""
+    """Test if a new value could be set."""
 
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, mock_setting_data
-    )
+    mock_get_setting_values.append({"devices:local": {"Battery:MinSoc": "42"}})
 
-    await entity.async_set_native_value(42)
+    mock_config_entry.add_to_hass(hass)
 
-    mock_coordinator.async_write_data.assert_called_once_with(
-        "moduleid", {"dataid": "42"}
-    )
-    mock_coordinator.async_refresh.assert_called_once()
+    await hass.config_entries.async_setup(mock_config_entry.entry_id)
+    await hass.async_block_till_done()
 
+    async_fire_time_changed(hass, dt.utcnow() + timedelta(seconds=3))
+    await hass.async_block_till_done()
 
-async def test_minmax_overwrite(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-):
-    """Test if min/max value is overwritten from retrieved settings data."""
-
-    setting_data = SettingsData(
+    await hass.services.async_call(
+        NUMBER_DOMAIN,
+        SERVICE_SET_VALUE,
         {
-            "min": "5",
-            "max": "100",
-        }
+            ATTR_ENTITY_ID: "number.scb_battery_min_soc",
+            ATTR_VALUE: 80,
+        },
+        blocking=True,
     )
 
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, setting_data
+    mock_plenticore_client.set_setting_values.assert_called_once_with(
+        "devices:local", {"Battery:MinSoc": "80"}
     )
-
-    assert entity.min_value == 5
-    assert entity.max_value == 100
-
-
-async def test_added_to_hass(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-    mock_setting_data: SettingsData,
-):
-    """Test if coordinator starts fetching after added to hass."""
-
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, mock_setting_data
-    )
-
-    await entity.async_added_to_hass()
-
-    mock_coordinator.start_fetch_data.assert_called_once_with("moduleid", "dataid")
-
-
-async def test_remove_from_hass(
-    mock_coordinator: MagicMock,
-    mock_number_description: PlenticoreNumberEntityDescription,
-    mock_setting_data: SettingsData,
-):
-    """Test if coordinator stops fetching after remove from hass."""
-
-    entity = PlenticoreDataNumber(
-        mock_coordinator, "42", "scb", None, mock_number_description, mock_setting_data
-    )
-
-    await entity.async_will_remove_from_hass()
-
-    mock_coordinator.stop_fetch_data.assert_called_once_with("moduleid", "dataid")