From 73a340935306ac31efacf0531509da503d49bf2b Mon Sep 17 00:00:00 2001 From: flan Date: Mon, 3 Aug 2026 20:40:17 +0000 Subject: [PATCH] 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 | 224 +++++++++++----------- custom_components/freshharvest/button.py | 13 +- custom_components/freshharvest/entity.py | 20 +- custom_components/freshharvest/select.py | 13 +- custom_components/freshharvest/switch.py | 16 +- custom_components/freshharvest/todo.py | 23 +-- tests/test_translations.py | 30 +++ 7 files changed, 171 insertions(+), 168 deletions(-) diff --git a/custom_components/freshharvest/actions.py b/custom_components/freshharvest/actions.py index 575c479..7799847 100644 --- a/custom_components/freshharvest/actions.py +++ b/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.""" diff --git a/custom_components/freshharvest/button.py b/custom_components/freshharvest/button.py index 9a7eed1..bd2e44a 100644 --- a/custom_components/freshharvest/button.py +++ b/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}, - ) diff --git a/custom_components/freshharvest/entity.py b/custom_components/freshharvest/entity.py index e02840a..1e6317f 100644 --- a/custom_components/freshharvest/entity.py +++ b/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, + }, + ) diff --git a/custom_components/freshharvest/select.py b/custom_components/freshharvest/select.py index 20040bd..12bc6cc 100644 --- a/custom_components/freshharvest/select.py +++ b/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}, - ) diff --git a/custom_components/freshharvest/switch.py b/custom_components/freshharvest/switch.py index 8f5e409..edb2498 100644 --- a/custom_components/freshharvest/switch.py +++ b/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}, - ) diff --git a/custom_components/freshharvest/todo.py b/custom_components/freshharvest/todo.py index 78db086..b1e2199 100644 --- a/custom_components/freshharvest/todo.py +++ b/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, - }, - ) diff --git a/tests/test_translations.py b/tests/test_translations.py index 6022cc6..e83b89d 100644 --- a/tests/test_translations.py +++ b/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"