iaqualink: add system selection options flow#167988
iaqualink: add system selection options flow#167988flz wants to merge 6 commits intohome-assistant:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds system selection to the iAqualink integration during initial config entry setup and exposes the same selection via an options flow, while refactoring client/system loading helpers and keeping entries loadable even when no systems are active/selected.
Changes:
- Introduce a new “systems” step in the config flow and an options flow (with reload) to choose which systems to include.
- Refactor Aqualink client creation into a shared helper and centralize system-fetch error mapping.
- Update setup logic and tests to support “no systems detected/selected” lifecycle, reload/unload behavior, and options flow coverage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/iaqualink/config_flow.py | Adds systems-selection step and options flow; centralizes system loading and error mapping. |
| homeassistant/components/iaqualink/init.py | Filters loaded systems by selected options and allows entries to remain loadable with no systems/selection. |
| homeassistant/components/iaqualink/utils.py | Adds shared helper for creating an Aqualink client using HA’s HTTPX client. |
| homeassistant/components/iaqualink/const.py | Introduces CONF_SYSTEMS option key constant. |
| homeassistant/components/iaqualink/strings.json | Adds new UI strings for the systems selection step and options flow, plus a “no systems” error. |
| tests/components/iaqualink/test_config_flow.py | Expands config flow tests for systems selection and adds options flow tests. |
| tests/components/iaqualink/test_init.py | Updates setup tests for “no systems detected/selected” to support reload/unload. |
Comments suppressed due to low confidence (2)
homeassistant/components/iaqualink/config_flow.py:92
- Handle TimeoutError during credential validation by treating it as "cannot_connect" (matching the integration setup path, which retries on TimeoutError).
try:
async with await async_get_aqualink_client(
self.hass, user_input[CONF_USERNAME], user_input[CONF_PASSWORD]
):
pass
except AqualinkServiceUnauthorizedException:
return {"base": "invalid_auth"}
except AqualinkServiceException, httpx.HTTPError:
return {"base": "cannot_connect"}
tests/components/iaqualink/test_config_flow.py:83
- Add a test case for TimeoutError (from login and/or get_systems) to ensure the flow maps it to the expected "cannot_connect" error reason.
async def test_with_invalid_credentials(
hass: HomeAssistant, config_data: dict[str, str]
) -> None:
"""Test config flow with invalid username and/or password."""
flow = config_flow.AqualinkFlowHandler()
flow.hass = hass
with patch(
"homeassistant.components.iaqualink.utils.AqualinkClient.login",
side_effect=AqualinkServiceUnauthorizedException,
):
result = await flow.async_step_user(config_data)
assert result["type"] is FlowResultType.FORM
assert result["step_id"] == "user"
assert result["errors"] == {"base": "invalid_auth"}
async def test_service_exception(
hass: HomeAssistant, config_data: dict[str, str]
) -> None:
"""Test config flow encountering service exception."""
flow = config_flow.AqualinkFlowHandler()
flow.hass = hass
with patch(
"homeassistant.components.iaqualink.utils.AqualinkClient.login",
side_effect=AqualinkServiceException,
):
result = await flow.async_step_user(config_data)
assert result["type"] is FlowResultType.FORM
assert result["step_id"] == "user"
assert result["errors"] == {"base": "cannot_connect"}
flz
force-pushed
the
iaqualink-options-flow
branch
from
5825d67 to
16d454d
Compare
|
Why do we want to select systems in the first place? If users don't want a system they can disable its device right? |
|
@joostlek This facilitates experimental testing and reduce API calls for a multi-locations account. We've had people with eXO systems encountering 429 errors which caused overall integration issues so the ability to stop handling a given system can be beneficial. |
|
Oh boy. They will love the change you did with the coordinators. The moment you now disable a device, and there are no entities anymore to the coordinator, it will stop polling that coordinator altogether. I think this would be a very home assistanty way of solving this In general I might recommend to look into dynamic polling, I'm not sure if you know how many calls you can do, otherwise you can lower the polling the more systems someone has |
|
The next version of the iaqualink library has better handling of 429 (retry with experimental backoff). It's hard to figure out the right balance for the polling interval in general. Some systems seem to be fine with every 5s but some others start spewing 429 when queried every 15s. I'd like to build in some smarts that would start conservative and gradually reduce until getting 429s, something like TCP slow-start. That might be overkill though. |
| return systems, None | ||
|
|
||
|
|
||
| def _build_systems_schema( |
There was a problem hiding this comment.
I don't think you need a specific method for this and you can do this inline.
There was a problem hiding this comment.
It's called three times, would you rather it be copied as is in each callsite?
There was a problem hiding this comment.
I get the DRY concern, but even though this is called a few times, it’s still just assembling a small, parameterized schema rather than complex reusable logic. I’d lean toward keeping it inline or extracting the repeated structure into a module-level constant where possible, instead of introducing a helper. That keeps the flow definition straightforward. :)
| assert result["errors"] == {"base": "cannot_connect"} | ||
|
|
||
|
|
||
| async def test_timeout_during_credential_check( |
There was a problem hiding this comment.
You can group the exceptions with parameterization. That way you reduce some code and can easily expand upon if new exceptions in the flow are added.
There was a problem hiding this comment.
That makes sense, thanks!
|
|
||
| systems, error_reason = await async_get_systems(self.hass, username, password) | ||
|
|
||
| if systems is None: |
There was a problem hiding this comment.
What happens or should happen if an error is raised from async_get_systems here?
There was a problem hiding this comment.
I think it should go back to prompting the user for username/password.
I'm separately realizing that "no_systems" (i.e. no systems detected) shouldn't be an error anymore so I'll update that logic.
There was a problem hiding this comment.
That does lead me to the question, since you made the username and password inclusive in the user step schema: are the username and password required or not? :)
There was a problem hiding this comment.
Most definitely required but I'm not sure what change you're hinting at :-)
There was a problem hiding this comment.
Skip that, I am confusing it with another PR I was reviewing. All good. :)
| try: | ||
| systems = await aqualink.get_systems() | ||
| except AqualinkServiceUnauthorizedException as auth_exception: | ||
| await aqualink.close() | ||
| raise ConfigEntryAuthFailed( | ||
| "Invalid credentials for iAqualink" | ||
| ) from auth_exception | ||
| except AqualinkServiceException as svc_exception: | ||
| await aqualink.close() | ||
| raise ConfigEntryNotReady( | ||
| f"Error while attempting to retrieve systems list: {svc_exception}" | ||
| ) from svc_exception |
There was a problem hiding this comment.
Handle timeouts/HTTP errors when fetching the systems list by catching TimeoutError and httpx.HTTPError the same way as in the login path.
aqualink.get_systems() can raise network-related exceptions, but this block only catches AqualinkServiceException, which will currently bubble up as an unhandled exception instead of being mapped to ConfigEntryNotReady (setup retry).
| async def async_step_user( | ||
| self, user_input: dict[str, Any] | None = None | ||
| ) -> ConfigFlowResult: | ||
| """Handle a flow start.""" | ||
| errors = {} | ||
| errors: dict[str, str] = {} | ||
|
|
||
| if user_input is not None: | ||
| errors = await self._async_test_credentials(user_input) | ||
| if not errors: | ||
| return self.async_create_entry( | ||
| title=user_input[CONF_USERNAME], data=user_input | ||
| systems, systems_error = await async_get_systems( | ||
| self.hass, | ||
| user_input[CONF_USERNAME], | ||
| user_input[CONF_PASSWORD], | ||
| ) | ||
| if systems is None: | ||
| assert systems_error is not None | ||
| errors = {"base": systems_error} | ||
| else: | ||
| self._pending_user_input = user_input | ||
| self._system_keys = { | ||
| system.serial: system.name for system in systems.values() | ||
| } | ||
| return await self.async_step_systems() |
There was a problem hiding this comment.
Avoid performing two separate client sessions/logins during initial setup by combining credential validation and system retrieval into a single call.
async_step_user calls _async_test_credentials() and then calls async_get_systems(), which will establish another client context and likely repeat the same network/auth work; you can simplify by relying on async_get_systems() alone (it already maps invalid_auth/cannot_connect) and only proceed to the systems step when it succeeds.
Add system selection during config entry setup and expose the same selection through an options flow. Refactor system loading into shared helpers, surface flow errors consistently, and keep entries loadable when no systems are active or selected. Update config flow and setup tests to cover the new options flow, error handling, and no-active-systems lifecycle.
flz
force-pushed
the
iaqualink-options-flow
branch
from
1e2a494 to
2232776
Compare
Co-authored-by: Erwin Douna <e.douna@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| # Get currently selected systems from options (or default to all) | ||
| current_systems = self.config_entry.options.get( | ||
| CONF_SYSTEMS, list(self._system_keys.keys()) | ||
| ) | ||
|
|
||
| return self.async_show_form( | ||
| step_id="init", | ||
| data_schema=_build_systems_schema(self._system_keys, current_systems), | ||
| ) |
There was a problem hiding this comment.
Filter the saved/default system IDs to only those present in the freshly fetched self._system_keys before building the cv.multi_select schema, otherwise stale options (e.g., a system removed from the account) can cause the options form to render/validate with an invalid default selection and fail on submit.
Breaking change
Proposed change
Add system selection during config entry setup and expose the same selection through an options flow.
Refactor system loading into shared helpers, surface flow errors consistently, and keep entries loadable when no systems are active or selected.
Update config flow and setup tests to cover the new options flow, error handling, and no-active-systems lifecycle.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: