Add reconfigure flow to Hot Spring - #179116
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a reconfiguration flow for updating a Hot Spring spa’s network address while verifying device identity.
Changes:
- Adds host reconfiguration with MAC validation and reload.
- Adds user-facing flow messages.
- Adds success and device-mismatch tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
homeassistant/components/hotspring/config_flow.py |
Implements reconfiguration. |
homeassistant/components/hotspring/strings.json |
Adds reconfiguration messages. |
homeassistant/components/hotspring/quality_scale.yaml |
Marks reconfiguration complete. |
tests/components/hotspring/test_config_flow.py |
Tests reconfiguration outcomes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
homeassistant/components/hotspring/config_flow.py:82
- Preserve the submitted host when redisplaying a failed reconfiguration. This always repopulates the form from the existing entry, so a
cannot_connectresponse replaces the attempted address with the old host instead of leaving it available for correction.
data_schema = self.add_suggested_values_to_schema(
data_schema,
self._get_reconfigure_entry().data,
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
homeassistant/components/hotspring/strings.json:7
- The string contains a literal
\n\nwith a leading space before the newline escape. That will render an extra space before the line break in the UI. Consider removing the space before\n\nto avoid odd formatting, and keep the line breaks consistent with other HA translations.
"unique_id_mismatch": "MAC address does not match the configured device. Expected to connect to device with MAC: `{expected_mac}`, but connected to device with MAC: `{actual_mac}`. \n\nPlease ensure you reconfigure against the same device."
tests/components/hotspring/test_config_flow.py:100
- This test now mutates
device_fixture.infoin-place instead of swapping the wholeSpaInfoobject and restoring it. If thedevice_fixtureis shared (fixture scope > function) or reused in other tests, this can introduce cross-test coupling. Prefer restoring the originalroot_topic(store/restore the old value) or reassigning a freshSpaInfocopy to keep the test isolated.
device_fixture.info.root_topic = "unknownTopic123"
tests/components/hotspring/test_config_flow.py:113
- This test now mutates
device_fixture.infoin-place instead of swapping the wholeSpaInfoobject and restoring it. If thedevice_fixtureis shared (fixture scope > function) or reused in other tests, this can introduce cross-test coupling. Prefer restoring the originalroot_topic(store/restore the old value) or reassigning a freshSpaInfocopy to keep the test isolated.
device_fixture.info.root_topic = "mySpaAABBCCDDEEFF"
b8d0690 to
a88734a
Compare
…ve/HA-core into hotspring-reconfigure
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
homeassistant/components/hotspring/strings.json:7
- The message includes an extra space before the newline sequence (
. \n\n), which will render as a trailing space in some contexts. Remove the space so it’s.\n\n...to avoid awkward formatting.
"cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
"reconfigure_successful": "[%key:common::config_flow::abort::reconfigure_successful%]",
"unique_id_mismatch": "MAC address does not match the configured device. Expected to connect to device with MAC: `{expected_mac}`, but connected to device with MAC: `{actual_mac}`. \n\nPlease ensure you reconfigure against the same device."
tests/components/hotspring/test_config_flow.py:185
- Setting
side_effectto an exception class can fail if the exception requires constructor args (it will be instantiated when raised). Prefer providing an instance (e.g.,HotSpringConnectionError(...)) to make the test robust to exception signature changes.
mock_hotspring.update.side_effect = HotSpringConnectionError
tests/components/hotspring/test_config_flow.py:100
- This test mutates
device_fixturestate in-place. If the fixture scope is broader than function (or the same object is reused), this can leak state into other tests. Consider capturing the originalroot_topicand restoring it in atry/finallyto keep the test isolated.
device_fixture.info.root_topic = "unknownTopic123"
tests/components/hotspring/test_config_flow.py:113
- This test mutates
device_fixturestate in-place. If the fixture scope is broader than function (or the same object is reused), this can leak state into other tests. Consider capturing the originalroot_topicand restoring it in atry/finallyto keep the test isolated.
device_fixture.info.root_topic = "mySpaAABBCCDDEEFF"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
homeassistant/components/hotspring/strings.json:7
- In translation strings, including literal
\n\ninside a JSON string tends to be error-prone for formatting and consistency across frontends. Prefer using actual newlines in the JSON value (or keep it as a single paragraph) to match how other HA integrations structure multi-line abort messages, and avoid trailing whitespace before the newline escape.
"unique_id_mismatch": "MAC address does not match the configured device. Expected to connect to device with MAC: `{expected_mac}`, but connected to device with MAC: `{actual_mac}`. \n\nPlease ensure you reconfigure against the same device."
tests/components/hotspring/test_config_flow.py:100
- This test now mutates
device_fixture.infoin-place rather than swapping the entireSpaInfo. Ifdevice_fixture(ordevice_fixture.info) is shared across tests via fixture scope, in-place mutation can leak state between tests. To keep tests isolated, consider cloning/copying theSpaInfoinstance (or setting the fixture scope to function) and restoring it afterward.
device_fixture.info.root_topic = "unknownTopic123"
tests/components/hotspring/test_config_flow.py:113
- This test now mutates
device_fixture.infoin-place rather than swapping the entireSpaInfo. Ifdevice_fixture(ordevice_fixture.info) is shared across tests via fixture scope, in-place mutation can leak state between tests. To keep tests isolated, consider cloning/copying theSpaInfoinstance (or setting the fixture scope to function) and restoring it afterward.
device_fixture.info.root_topic = "mySpaAABBCCDDEEFF"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/components/hotspring/test_config_flow.py:100
- This test now mutates
device_fixture.info.root_topicwithout restoring the original value at the end. Ifdevice_fixture(or itsinfo) is shared beyond function scope, this can leak state into other tests and cause order-dependent failures. Suggestion: store the originalroot_topic(or originalinfo) at the start of the test and restore it in afinallyblock (or at least before returning) to keep the test isolated.
device_fixture.info.root_topic = "unknownTopic123"
tests/components/hotspring/test_config_flow.py:113
- This test now mutates
device_fixture.info.root_topicwithout restoring the original value at the end. Ifdevice_fixture(or itsinfo) is shared beyond function scope, this can leak state into other tests and cause order-dependent failures. Suggestion: store the originalroot_topic(or originalinfo) at the start of the test and restore it in afinallyblock (or at least before returning) to keep the test isolated.
device_fixture.info.root_topic = "mySpaAABBCCDDEEFF"
homeassistant/components/hotspring/config_flow.py:88
async_step_useris annotated with@override, butasync_step_reconfigure(a framework entrypoint method) is not. Adding@overridehere improves consistency and helps static checking catch signature mismatches during future refactors.
async def async_step_reconfigure(
self, user_input: dict[str, Any] | None = None
) -> ConfigFlowResult:
"""Handle reconfiguration of the Hot Spring spa."""
return await self.async_step_user(user_input)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/components/hotspring/test_config_flow.py:100
- This test mutates the shared
device_fixture.info.root_topicin-place without restoring the original value. Ifdevice_fixturehas a broader fixture scope than function-level, this can leak state into other tests and cause order-dependent failures. Consider saving the originalroot_topicat the start of the test and restoring it in afinallyblock (or recreating/replacing theinfoobject as was done previously).
device_fixture.info.root_topic = "unknownTopic123"
tests/components/hotspring/test_config_flow.py:113
- This test mutates the shared
device_fixture.info.root_topicin-place without restoring the original value. Ifdevice_fixturehas a broader fixture scope than function-level, this can leak state into other tests and cause order-dependent failures. Consider saving the originalroot_topicat the start of the test and restoring it in afinallyblock (or recreating/replacing theinfoobject as was done previously).
device_fixture.info.root_topic = "mySpaAABBCCDDEEFF"
homeassistant/components/hotspring/config_flow.py:60
self._get_reconfigure_entry()is called multiple times in the same step. It would be clearer and less error-prone to fetch it once into a local variable (e.g.,entry = self._get_reconfigure_entry()) and reuse it both forasync_update_reload_and_abort(...)and for suggested values. This also avoids any chance of inconsistency if the underlying entry reference changes between calls.
if self.source == SOURCE_RECONFIGURE:
self._abort_if_unique_id_mismatch()
return self.async_update_reload_and_abort(
self._get_reconfigure_entry(),
data_updates={CONF_HOST: user_input[CONF_HOST]},
)
homeassistant/components/hotspring/config_flow.py:73
self._get_reconfigure_entry()is called multiple times in the same step. It would be clearer and less error-prone to fetch it once into a local variable (e.g.,entry = self._get_reconfigure_entry()) and reuse it both forasync_update_reload_and_abort(...)and for suggested values. This also avoids any chance of inconsistency if the underlying entry reference changes between calls.
if suggested_values is None and self.source == SOURCE_RECONFIGURE:
suggested_values = self._get_reconfigure_entry().data
homeassistant/components/hotspring/strings.json:7
- The
unique_id_mismatchabort message is somewhat ambiguous about what specifically mismatched (detected device vs configured entry). Consider tightening the wording to explicitly state that the discovered spa’s MAC/unique_id differs from the existing entry’s MAC/unique_id, and (if the flow has access) include the configured identifier to make troubleshooting easier.
"cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
"reconfigure_successful": "[%key:common::config_flow::abort::reconfigure_successful%]",
"unique_id_mismatch": "The MAC address does not match the configured device. Please ensure you reconfigure against the same device."
erwindouna
left a comment
There was a problem hiding this comment.
Looks good, thanks @Moustachauve!
Proposed change
Add reconfigure flow to Hot Spring integration
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: