Update waterfurnace to use config flow, instead of YAML
#159908
Conversation
home-assistant
bot
added
cla-needed
code-quality
config-flow
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @masterkoppa
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
This is awesome! But, can we cut down the PR in size by removing the quality scale, sensor changes and coordinator? As ideally the config flow PR should only change the way entities are created. Happy to help out, feel free to message me on Discord
Modernize the existing waterfurnace integration and allow a UI based configuration option. This initial implementation includes unit testing for the config flow. For codeowners I have included myself, and the original author of the integration (and owner of the waterfurnace python library). Co-authored-by: Claude Sonnet 4.5 [email protected]
Add support for importing legacy login information from YAML config file as part of the config flow upgrade process. Co-authored-by: Claude Sonnet 4.5 [email protected]
masterkoppa
force-pushed
the
waterfurnace-config-flow
branch
from
29f9a42 to
6e01db0
Compare
Thanks for the quick response! I wasn't sure at first if anything not meeting the "bronze" quality would be acceptable for PR, so I was aiming for that as the minimum. I've force-pushed a new set of changes to this PR limited to only the config flow, and the import from YAML functionality. I effectively took an axe to everything until the diff was minimal in Happy to cut further scope if that's possible, but beyond some extra error handling I kept this seems pretty minimal. The bulk of the changes right now is unit tests. |
NoRi2909
left a comment
NoRi2909
left a comment
There was a problem hiding this comment.
We can use a bunch of common strings here to speed up translations.
Thanks for the call out, I was not aware of these shared strings. I took another look and found a re-auth related shared string that we could also use beyond the ones suggested! |
| if DOMAIN in config: | ||
| hass.async_create_task( | ||
| hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={"source": SOURCE_IMPORT}, | ||
| data=config[DOMAIN], | ||
| ) | ||
| ) |
There was a problem hiding this comment.
We should use the result to raise a proper issue for the user. For an example (albeit a platform setup, it's the same but you have to wrap it in a async_create_task like you do here) check nederlandse_spoorwegen
| except WFCredentialError as err: | ||
| _LOGGER.error("Invalid credentials for WaterFurnace device") | ||
| raise ConfigEntryAuthFailed( | ||
| "Authentication failed. Please update your credentials." | ||
| ) from err | ||
| except WFException as err: | ||
| _LOGGER.error("Failed to connect to WaterFurnace service: %s", err) | ||
| raise ConfigEntryNotReady( | ||
| f"Failed to connect to WaterFurnace service: {err}" |
There was a problem hiding this comment.
Let's not raise and log at the same time
| except Exception as err: | ||
| _LOGGER.exception("Unexpected error during WaterFurnace setup") | ||
| raise ConfigEntryNotReady(f"Unexpected error during setup: {err}") from err |
There was a problem hiding this comment.
We should only catch bare exceptions in the config flow, here we should be specific
| hass.data[DOMAIN].start() | ||
|
|
||
| discovery.load_platform(hass, Platform.SENSOR, DOMAIN, {}, config) | ||
| discovery.load_platform(hass, Platform.SENSOR, DOMAIN, {}, entry.as_dict()) |
There was a problem hiding this comment.
You should use await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) here and setup the sensor platform via async_setup_entry
| async def async_step_reauth( | ||
| self, entry_data: Mapping[str, Any] | ||
| ) -> ConfigFlowResult: | ||
| """Handle reauth flow when credentials expire.""" | ||
| self._username = entry_data.get(CONF_USERNAME) | ||
| return await self.async_step_reauth_confirm() | ||
|
|
||
| async def async_step_reauth_confirm( | ||
| self, user_input: dict[str, Any] | None = None | ||
| ) -> ConfigFlowResult: | ||
| """Handle reauth confirmation step.""" | ||
| errors: dict[str, str] = {} | ||
|
|
||
| if user_input is not None: | ||
| # Get the existing entry | ||
| entry = self._get_reauth_entry() | ||
|
|
||
| # Merge existing entry data with new credentials | ||
| full_input = {**entry.data, **user_input} | ||
|
|
||
| try: | ||
| info = await validate_input(self.hass, full_input) | ||
| except CannotConnect: | ||
| errors["base"] = "cannot_connect" | ||
| except InvalidAuth: | ||
| errors["base"] = "invalid_auth" | ||
| except Exception: | ||
| _LOGGER.exception("Unexpected exception") | ||
| errors["base"] = "unknown" | ||
| else: | ||
| # Verify the GWID matches the existing entry | ||
| await self.async_set_unique_id(info["gwid"]) | ||
| self._abort_if_unique_id_mismatch(reason="wrong_account") | ||
|
|
||
| return self.async_update_reload_and_abort( | ||
| entry, | ||
| data_updates=user_input, | ||
| ) | ||
|
|
||
| return self.async_show_form( | ||
| step_id="reauth_confirm", | ||
| data_schema=vol.Schema( | ||
| { | ||
| vol.Required(CONF_USERNAME, default=self._username): str, | ||
| vol.Required(CONF_PASSWORD): str, | ||
| } | ||
| ), | ||
| errors=errors, | ||
| ) |
There was a problem hiding this comment.
I would propose to split off reauth for this PR as it will make the amount of tests required less and easier to review
| async def validate_input(hass: HomeAssistant, data: dict[str, Any]) -> dict[str, Any]: | ||
| """Validate the user input allows us to connect. | ||
| Data has the keys from STEP_USER_DATA_SCHEMA with values provided by the user. | ||
| """ | ||
| username = data[CONF_USERNAME] | ||
| password = data[CONF_PASSWORD] | ||
|
|
||
| client = WaterFurnace(username, password) | ||
|
|
||
| try: | ||
| # Login is a blocking call, run in executor | ||
| await hass.async_add_executor_job(client.login) | ||
| except WFCredentialError as err: | ||
| _LOGGER.error("Invalid credentials for WaterFurnace login") | ||
| raise InvalidAuth from err | ||
| except WFException as err: | ||
| _LOGGER.error("Failed to connect to WaterFurnace service: %s", err) | ||
| raise CannotConnect from err | ||
| except Exception as err: | ||
| _LOGGER.exception("Unexpected error connecting to WaterFurnace") | ||
| raise CannotConnect from err | ||
|
|
||
| gwid = client.gwid | ||
| if not gwid: | ||
| _LOGGER.error("No GWID found for device") | ||
| raise CannotConnect | ||
|
|
||
| return { | ||
| "title": f"WaterFurnace {gwid}", | ||
| "gwid": gwid, | ||
| } | ||
|
|
||
|
|
||
| class WaterFurnaceConfigFlow(ConfigFlow, domain=DOMAIN): | ||
| """Handle a config flow for WaterFurnace.""" | ||
|
|
||
| VERSION = 1 | ||
| MINOR_VERSION = 1 | ||
|
|
||
| def __init__(self) -> None: | ||
| """Initialize the config flow.""" | ||
| self._username: str | None = None | ||
|
|
||
| async def async_step_user( | ||
| self, user_input: dict[str, Any] | None = None | ||
| ) -> ConfigFlowResult: | ||
| """Handle the initial step.""" | ||
| errors: dict[str, str] = {} | ||
|
|
||
| if user_input is not None: | ||
| try: | ||
| info = await validate_input(self.hass, user_input) |
| patch( | ||
| "homeassistant.components.waterfurnace.config_flow.WaterFurnace", | ||
| autospec=True, | ||
| ) as mock_client_class, | ||
| patch( | ||
| "homeassistant.components.waterfurnace.WaterFurnaceData", | ||
| new=mock_client_class, |
There was a problem hiding this comment.
Why do we patch WaterFurnaceData in the normal one and the library in the config flow? Shouldn't they be the same?
| # Mock the read method (blocking call) with fixture data | ||
| device_data = load_json_object_fixture("device_data.json", DOMAIN) | ||
|
|
||
| # Create a mock data object with attributes matching the fixture | ||
| mock_data = Mock() | ||
| for key, value in device_data.items(): | ||
| setattr(mock_data, key, value) |
There was a problem hiding this comment.
How does the object get created in the library? I would personally only opt to use JSON if the source data is also in JSON and you have an easy way to get an object like in the library
| @pytest.fixture | ||
| def mock_waterfurnace_client_no_gwid(mock_waterfurnace_client: Mock) -> Mock: | ||
| """Mock WaterFurnace client without GWID.""" | ||
| mock_waterfurnace_client.gwid = None | ||
| return mock_waterfurnace_client |
3 participants
Proposed change
Replace YAML based configuration for
waterfurnaceintegration with a Config Flow based setup.Type of change
Additional information
The primary focus of this PR is replacing the YAML based config flow, with the end goal of adding new features in subsequent PRs (new sensors, and multiple devices). I've done my best to keep the changes here to only have parity with the existing functionality.
Per initial PR feedback, the scope of the changes is strictly limited to the config flow introduction, importing existing YAML settings, and the associated tests. Further improvements will come in subsequent PRs.
This is my first contribution, any and all feedback welcome. If the size of the PR is too large, I'm happy to removed non-config related tests or any other changes that are deemed "extra" and should happen in subsequent PRs.
For codeowners, I've added the original author of the integration (and maintainer of the underlying library) as well as myself. If that's not appropriate, I'm happy to modify it.
waterfurnacedocs to remove references to legacy configuration home-assistant.io#42773Full disclosure: I'm not a python dev by trade, and I relied heavily on Claude Code for the initial work, but I've reviewed the code and made changes where appropriate. Claude Code (and the associated docs, those are super useful to more than just LLMs!) were extremely helpful in getting me up to speed. When possible I compared the changes made with adjacent modules to make sure the code looked appropriate and matched the rest of the codebase when possible.
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: