Refactor: real mixin, shared event helper, structural tests

FreshHarvestActions inherited from itself to graft on basket switching, which
is legal Python and a trap. BasketMixin now sits above it and is inherited
normally.

The four control platforms each carried an identical private _fire; it moves to
the base entity as fire_action. Rewriting those call sites mechanically broke
two of them - select fired 'donate' instead of 'change_basket', and button lost
an argument entirely, a TypeError reachable only by pressing it. Both fixed,
and two tests now assert call arity and that no class inherits from itself,
because neither fault is reachable from a unit test.

custom_components/freshharvest/actions.py

@@ -202,7 +202,116 @@ def _confirmation_date(text: str) -> date | None:
         return None
 
 
-class FreshHarvestActions:
+@dataclass
+class Basket:
+    """A produce box you can switch to."""
+
+    item_id: str
+    name: str
+    is_current: bool = False
+    token: str = ""
+
+
+class BasketMixin:
+    """Produce-box switching. Mixed into FreshHarvestActions.
+
+    A box is not an add-on: you swap which box arrives, you do not add or
+    remove the produce inside it. The switch also has a scope the add-on
+    endpoints do not — one delivery, or every future one.
+    """
+
+    async def async_list_baskets(self) -> list[Basket]:
+        """Every box you could switch TO.
+
+        Each option's real name lives in its own popup rather than the grid
+        (the grid calls them all "Georgia Box"), so this reads them there.
+
+        The box you are already on is deliberately absent: the site offers no
+        "switch to this" control for it, so there is no popup and no name. Its
+        id shows up as every popup's `ReplaceItemID`, which is what
+        `current_basket_id` returns; its NAME comes from the subscription list.
+        """
+        baskets: list[Basket] = []
+        seen: set[str] = set()
+        for group in BASKET_GROUPS:
+            page = await self._client.async_fetch(f"{BASKET_TYPES}/{group}")
+            tokens = dict.fromkeys(
+                re.findall(r'openPopup\("select-basket",\s*"([^"]+)"', page)
+            )
+            for token in tokens:
+                popup = await self._client.async_fetch(
+                    f"/x/popup/select-basket/{token}", is_page=False
+                )
+                soup = BeautifulSoup(popup, "html.parser")
+                form = _find_form(soup, SUBMIT_BASKET)
+                if form is None:
+                    continue  # a catch-all page, not a real popup
+                fields = _hidden_fields(form)
+                item_id = fields.get("ItemID", "")
+                if not item_id or item_id in seen:
+                    continue
+                seen.add(item_id)
+                heading = soup.select_one("h4, h5, h6")
+                baskets.append(
+                    Basket(
+                        item_id=item_id,
+                        name=(heading.get_text(" ", strip=True) if heading else item_id),
+                        is_current=False,  # see the docstring: never offered
+                        token=token,
+                    )
+                )
+        return baskets
+
+    async def async_current_basket_id(self) -> str | None:
+        """The id of the box currently subscribed, read off any switch popup."""
+        for group in BASKET_GROUPS:
+            page = await self._client.async_fetch(f"{BASKET_TYPES}/{group}")
+            for token in dict.fromkeys(
+                re.findall(r'openPopup\("select-basket",\s*"([^"]+)"', page)
+            ):
+                popup = await self._client.async_fetch(
+                    f"/x/popup/select-basket/{token}", is_page=False
+                )
+                form = _find_form(BeautifulSoup(popup, "html.parser"), SUBMIT_BASKET)
+                if form is not None:
+                    return _hidden_fields(form).get("ReplaceItemID")
+        return None
+
+    async def async_change_basket(
+        self, name_or_id: str, all_future: bool = False, dry_run: bool = True
+    ) -> ActionResult:
+        """Switch to a different produce box.
+
+        `all_future=False` changes only the next delivery; True changes the
+        standing order. Defaulting to the one-off is deliberate — a mistaken
+        permanent change is the more annoying of the two to undo.
+        """
+        wanted = str(name_or_id).strip().lower()
+        for basket in await self.async_list_baskets():
+            if wanted not in (basket.item_id.lower(), basket.name.lower()):
+                continue
+            popup = await self._client.async_fetch(
+                f"/x/popup/select-basket/{basket.token}", is_page=False
+            )
+            form = _find_form(BeautifulSoup(popup, "html.parser"), SUBMIT_BASKET)
+            if form is None:
+                raise FreshHarvestActionError("basket form disappeared mid-flight")
+            payload = _hidden_fields(form)
+            payload["popup-toggle"] = SCOPE_STANDING if all_future else SCOPE_ONCE
+            scope = "all future orders" if all_future else "the next delivery only"
+            result = ActionResult(
+                action="change_basket", ok=True, target=basket.name,
+                submitted=payload, dry_run=dry_run,
+                detail=f"switch to {basket.name} for {scope}",
+            )
+            if not dry_run:
+                await self._post(SUBMIT_BASKET, payload)
+            return result
+
+        raise FreshHarvestActionError(f"no box matches {name_or_id!r}")
+
+
+class FreshHarvestActions(BasketMixin):
     """Mutating operations, each re-deriving its tokens from a live page."""
 
     def __init__(self, client: FreshHarvestClient) -> None:
@@ -492,116 +601,3 @@ class FreshHarvestActions:
         if not dry_run:
             await self._post(SUBMIT_HOLD_REMOVE, payload)
         return result
-
-
-@dataclass
-class Basket:
-    """A produce box you can switch to."""
-
-    item_id: str
-    name: str
-    is_current: bool = False
-    token: str = ""
-
-
-class BasketMixin:
-    """Produce-box switching, mixed into FreshHarvestActions below.
-
-    A box is not an add-on: you swap which box arrives, you do not add or
-    remove the produce inside it. The switch also has a scope the add-on
-    endpoints do not — one delivery, or every future one.
-    """
-
-    async def async_list_baskets(self) -> list[Basket]:
-        """Every box you could switch TO.
-
-        Each option's real name lives in its own popup rather than the grid
-        (the grid calls them all "Georgia Box"), so this reads them there.
-
-        The box you are already on is deliberately absent: the site offers no
-        "switch to this" control for it, so there is no popup and no name. Its
-        id shows up as every popup's `ReplaceItemID`, which is what
-        `current_basket_id` returns; its NAME comes from the subscription list.
-        """
-        baskets: list[Basket] = []
-        seen: set[str] = set()
-        for group in BASKET_GROUPS:
-            page = await self._client.async_fetch(f"{BASKET_TYPES}/{group}")
-            tokens = dict.fromkeys(
-                re.findall(r'openPopup\("select-basket",\s*"([^"]+)"', page)
-            )
-            for token in tokens:
-                popup = await self._client.async_fetch(
-                    f"/x/popup/select-basket/{token}", is_page=False
-                )
-                soup = BeautifulSoup(popup, "html.parser")
-                form = _find_form(soup, SUBMIT_BASKET)
-                if form is None:
-                    continue  # a catch-all page, not a real popup
-                fields = _hidden_fields(form)
-                item_id = fields.get("ItemID", "")
-                if not item_id or item_id in seen:
-                    continue
-                seen.add(item_id)
-                heading = soup.select_one("h4, h5, h6")
-                baskets.append(
-                    Basket(
-                        item_id=item_id,
-                        name=(heading.get_text(" ", strip=True) if heading else item_id),
-                        is_current=False,  # see the docstring: never offered
-                        token=token,
-                    )
-                )
-        return baskets
-
-    async def async_current_basket_id(self) -> str | None:
-        """The id of the box currently subscribed, read off any switch popup."""
-        for group in BASKET_GROUPS:
-            page = await self._client.async_fetch(f"{BASKET_TYPES}/{group}")
-            for token in dict.fromkeys(
-                re.findall(r'openPopup\("select-basket",\s*"([^"]+)"', page)
-            ):
-                popup = await self._client.async_fetch(
-                    f"/x/popup/select-basket/{token}", is_page=False
-                )
-                form = _find_form(BeautifulSoup(popup, "html.parser"), SUBMIT_BASKET)
-                if form is not None:
-                    return _hidden_fields(form).get("ReplaceItemID")
-        return None
-
-    async def async_change_basket(
-        self, name_or_id: str, all_future: bool = False, dry_run: bool = True
-    ) -> ActionResult:
-        """Switch to a different produce box.
-
-        `all_future=False` changes only the next delivery; True changes the
-        standing order. Defaulting to the one-off is deliberate — a mistaken
-        permanent change is the more annoying of the two to undo.
-        """
-        wanted = str(name_or_id).strip().lower()
-        for basket in await self.async_list_baskets():
-            if wanted not in (basket.item_id.lower(), basket.name.lower()):
-                continue
-            popup = await self._client.async_fetch(
-                f"/x/popup/select-basket/{basket.token}", is_page=False
-            )
-            form = _find_form(BeautifulSoup(popup, "html.parser"), SUBMIT_BASKET)
-            if form is None:
-                raise FreshHarvestActionError("basket form disappeared mid-flight")
-            payload = _hidden_fields(form)
-            payload["popup-toggle"] = SCOPE_STANDING if all_future else SCOPE_ONCE
-            scope = "all future orders" if all_future else "the next delivery only"
-            result = ActionResult(
-                action="change_basket", ok=True, target=basket.name,
-                submitted=payload, dry_run=dry_run,
-                detail=f"switch to {basket.name} for {scope}",
-            )
-            if not dry_run:
-                await self._post(SUBMIT_BASKET, payload)
-            return result
-
-        raise FreshHarvestActionError(f"no box matches {name_or_id!r}")
-
-
-class FreshHarvestActions(FreshHarvestActions, BasketMixin):  # noqa: F811
-    """Actions plus basket switching."""

custom_components/freshharvest/button.py

@@ -13,7 +13,6 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback
 
 from . import FreshHarvestConfigEntry
 from .api import FreshHarvestError
-from .const import DOMAIN, EVENT_ACTION
 from .coordinator import FreshHarvestCoordinator
 from .entity import FreshHarvestEntity
 
@@ -42,7 +41,6 @@ class FreshHarvestDonate(FreshHarvestEntity, ButtonEntity):
         self, coordinator: FreshHarvestCoordinator, entry: FreshHarvestConfigEntry
     ) -> None:
         super().__init__(coordinator, entry, DESCRIPTION.key)
-        self._entry = entry
 
     @property
     def available(self) -> bool:
@@ -53,15 +51,8 @@ class FreshHarvestDonate(FreshHarvestEntity, ButtonEntity):
         try:
             result = await self.coordinator.actions.async_donate(dry_run=False)
         except FreshHarvestError as err:
-            self._fire(False, str(err))
+            self.fire_action("donate", False, "open order", str(err))
             raise HomeAssistantError(f"could not donate: {err}") from err
-        self._fire(True, result.detail)
+        self.fire_action("donate", True, "open order", result.detail)
         await self.coordinator.async_request_refresh()
 
-    def _fire(self, ok: bool, detail: str) -> None:
-        self.hass.bus.async_fire(
-            EVENT_ACTION,
-            {"domain": DOMAIN, "entry_id": self._entry.entry_id,
-             "action": "donate", "success": ok, "target": "open order",
-             "detail": detail},
-        )

custom_components/freshharvest/entity.py

@@ -15,7 +15,7 @@ from homeassistant.helpers.device_registry import DeviceEntryType, DeviceInfo
 from homeassistant.helpers.update_coordinator import CoordinatorEntity
 
 from .api import AccountSnapshot, DeliveryOrder
-from .const import DOMAIN
+from .const import DOMAIN, EVENT_ACTION
 from .coordinator import FreshHarvestCoordinator
 
 Scope = Literal["account", "next_order", "open_order"]
@@ -54,3 +54,21 @@ class FreshHarvestEntity(CoordinatorEntity[FreshHarvestCoordinator]):
     def target(self, scope: Scope) -> AccountSnapshot | DeliveryOrder | None:
         """Resolve this entity's scope against the latest snapshot."""
         return resolve_scope(self.coordinator.data, scope)
+
+    def fire_action(self, action: str, ok: bool, target: str, detail: str) -> None:
+        """Announce a write action's outcome so automations can notify on it.
+
+        Lives here because all four control platforms need it identically; four
+        private copies drifted apart the moment one of them gained a field.
+        """
+        self.hass.bus.async_fire(
+            EVENT_ACTION,
+            {
+                "domain": DOMAIN,
+                "entry_id": self.coordinator.config_entry.entry_id,
+                "action": action,
+                "success": ok,
+                "target": target,
+                "detail": detail,
+            },
+        )

custom_components/freshharvest/select.py

@@ -20,7 +20,6 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback
 
 from . import FreshHarvestConfigEntry
 from .api import FreshHarvestError
-from .const import DOMAIN, EVENT_ACTION
 from .coordinator import FreshHarvestCoordinator
 from .entity import FreshHarvestEntity
 
@@ -51,7 +50,6 @@ class FreshHarvestBoxSelect(FreshHarvestEntity, SelectEntity):
         self, coordinator: FreshHarvestCoordinator, entry: FreshHarvestConfigEntry
     ) -> None:
         super().__init__(coordinator, entry, DESCRIPTION.key)
-        self._entry = entry
         self._options: list[str] = []
 
     @property
@@ -98,15 +96,8 @@ class FreshHarvestBoxSelect(FreshHarvestEntity, SelectEntity):
                 option, all_future=False, dry_run=False
             )
         except FreshHarvestError as err:
-            self._fire(False, option, str(err))
+            self.fire_action("change_basket", False, option, str(err))
             raise HomeAssistantError(f"could not switch to {option}: {err}") from err
-        self._fire(True, option, result.detail)
+        self.fire_action("change_basket", True, option, result.detail)
         await self.coordinator.async_request_refresh()
 
-    def _fire(self, ok: bool, target: str, detail: str) -> None:
-        self.hass.bus.async_fire(
-            EVENT_ACTION,
-            {"domain": DOMAIN, "entry_id": self._entry.entry_id,
-             "action": "change_basket", "success": ok, "target": target,
-             "detail": detail},
-        )

custom_components/freshharvest/switch.py

@@ -16,7 +16,6 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback
 
 from . import FreshHarvestConfigEntry
 from .api import FreshHarvestError
-from .const import DOMAIN, EVENT_ACTION
 from .coordinator import FreshHarvestCoordinator
 from .entity import FreshHarvestEntity
 
@@ -47,7 +46,6 @@ class FreshHarvestSkip(FreshHarvestEntity, SwitchEntity):
         self, coordinator: FreshHarvestCoordinator, entry: FreshHarvestConfigEntry
     ) -> None:
         super().__init__(coordinator, entry, DESCRIPTION.key)
-        self._entry = entry
 
     @property
     def available(self) -> bool:
@@ -70,9 +68,9 @@ class FreshHarvestSkip(FreshHarvestEntity, SwitchEntity):
                 order.delivery_date, dry_run=False
             )
         except FreshHarvestError as err:
-            self._fire("skip", False, str(order.delivery_date), str(err))
+            self.fire_action("skip", False, str(order.delivery_date), str(err))
             raise HomeAssistantError(f"could not skip: {err}") from err
-        self._fire("skip", True, str(order.delivery_date), result.detail)
+        self.fire_action("skip", True, str(order.delivery_date), result.detail)
         await self.coordinator.async_request_refresh()
 
     async def async_turn_off(self, **kwargs: Any) -> None:
@@ -85,14 +83,8 @@ class FreshHarvestSkip(FreshHarvestEntity, SwitchEntity):
                 order.delivery_date, dry_run=False
             )
         except FreshHarvestError as err:
-            self._fire("restore", False, str(order.delivery_date), str(err))
+            self.fire_action("restore", False, str(order.delivery_date), str(err))
             raise HomeAssistantError(f"could not restore: {err}") from err
-        self._fire("restore", True, str(order.delivery_date), result.detail)
+        self.fire_action("restore", True, str(order.delivery_date), result.detail)
         await self.coordinator.async_request_refresh()
 
-    def _fire(self, action: str, ok: bool, target: str, detail: str) -> None:
-        self.hass.bus.async_fire(
-            EVENT_ACTION,
-            {"domain": DOMAIN, "entry_id": self._entry.entry_id, "action": action,
-             "success": ok, "target": target, "detail": detail},
-        )

custom_components/freshharvest/todo.py

@@ -40,7 +40,6 @@ from homeassistant.helpers.entity_platform import AddEntitiesCallback
 
 from . import FreshHarvestConfigEntry
 from .api import FreshHarvestError
-from .const import DOMAIN, EVENT_ACTION
 from .coordinator import FreshHarvestCoordinator
 from .entity import FreshHarvestEntity
 
@@ -73,7 +72,6 @@ class FreshHarvestBox(FreshHarvestEntity, TodoListEntity):
         self, coordinator: FreshHarvestCoordinator, entry: FreshHarvestConfigEntry
     ) -> None:
         super().__init__(coordinator, entry, "add_ons")
-        self._entry = entry
 
     @property
     def todo_items(self) -> list[TodoItem] | None:
@@ -154,10 +152,10 @@ class FreshHarvestBox(FreshHarvestEntity, TodoListEntity):
                 item_id, dry_run=False, name=name
             )
         except FreshHarvestError as err:
-            self._fire(EVENT_ACTION, "add_item", False, query, str(err))
+            self.fire_action("add_item", False, query, str(err))
             raise HomeAssistantError(f"could not add {name}: {err}") from err
 
-        self._fire(EVENT_ACTION, "add_item", True, name, result.detail)
+        self.fire_action("add_item", True, name, result.detail)
         await self.coordinator.async_request_refresh()
 
     async def async_delete_todo_items(self, uids: list[str]) -> None:
@@ -169,21 +167,8 @@ class FreshHarvestBox(FreshHarvestEntity, TodoListEntity):
                     item_id, dry_run=False, name=name
                 )
             except FreshHarvestError as err:
-                self._fire(EVENT_ACTION, "remove_item", False, uid, str(err))
+                self.fire_action("remove_item", False, uid, str(err))
                 raise HomeAssistantError(f"could not remove {name}: {err}") from err
-            self._fire(EVENT_ACTION, "remove_item", True, name, "removed")
+            self.fire_action("remove_item", True, name, "removed")
         await self.coordinator.async_request_refresh()
 
-    def _fire(self, event: str, action: str, ok: bool, target: str, detail: str) -> None:
-        """Announce the outcome so automations can notify on it."""
-        self.hass.bus.async_fire(
-            event,
-            {
-                "domain": DOMAIN,
-                "entry_id": self._entry.entry_id,
-                "action": action,
-                "success": ok,
-                "target": target,
-                "detail": detail,
-            },
-        )

tests/test_translations.py

@@ -91,3 +91,33 @@ def test_manifest_is_well_formed():
 def test_todo_entity_is_named(strings):
     """todo.py declares its name via _attr_translation_key, not a description."""
     assert attr_keys("todo.py") == set(strings["entity"]["todo"])
+
+
+def test_fire_action_call_sites_are_well_formed():
+    """Every fire_action call passes (action, ok, target, detail).
+
+    A refactor that moved this helper onto the base entity rewrote the call
+    sites mechanically and left one with three arguments — a TypeError that
+    only fires when a user presses the button, which no unit test reaches.
+    """
+    bad = []
+    for path in COMPONENT.glob("*.py"):
+        tree = ast.parse(path.read_text(encoding="utf-8"))
+        for node in ast.walk(tree):
+            if (
+                isinstance(node, ast.Call)
+                and getattr(node.func, "attr", None) == "fire_action"
+                and len(node.args) != 4
+            ):
+                bad.append(f"{path.name}:{node.lineno} takes {len(node.args)}")
+    assert not bad, f"malformed fire_action calls: {bad}"
+
+
+def test_no_class_inherits_from_itself():
+    """`class X(X, Mixin)` is legal Python and a maintenance trap; it was here."""
+    for path in COMPONENT.glob("*.py"):
+        tree = ast.parse(path.read_text(encoding="utf-8"))
+        for node in ast.walk(tree):
+            if isinstance(node, ast.ClassDef):
+                names = {getattr(b, "id", None) for b in node.bases}
+                assert node.name not in names, f"{path.name}: {node.name} inherits itself"