Solax: implement reconfiguration #160320
Solax: implement reconfiguration #160320
Conversation
|
Hey there @squishykid, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Darsstar
force-pushed
the
solax/reconfiguration
branch
6 times, most recently
from
768e5e8 to
25ecbcd
Compare
| """Handle reconfiguration.""" | ||
| errors: dict[str, Any] = {} | ||
| if user_input is None: | ||
| if (entry_id := self.context.get("entry_id")) and ( |
There was a problem hiding this comment.
What happens when these variables are not set?
There was a problem hiding this comment.
"entry_id" should always exist as a key since HA refuses to initialize a "reconfigure" flow without it.
If the config entry was deleted after initializing the "reconfigure" flow but before this code gets executed than config_entry would be None. Want me to use async_get_known_entry, which raises and UnkownEntry exception, instead?
Anyway, if that race occurs the form is show without the previous configuration values, but the default values instead.
There was a problem hiding this comment.
How would that even occur that during the reconfugure flow the config entry gets deleted?
There was a problem hiding this comment.
Oh during the reconfigure flow is easy, that early during the reconfigure flow is not as easy, might be impossible for all I know. But as far as the type checkers are conserned it is possible.
There was a problem hiding this comment.
Why not use self._get_reconfigure_entry(). Save you the walrus operator and makes mypy happy out of the box.
There was a problem hiding this comment.
For a practical example, look here:
Darsstar
force-pushed
the
solax/reconfiguration
branch
from
25ecbcd to
9ca9b13
Compare
Darsstar
force-pushed
the
solax/reconfiguration
branch
from
9ca9b13 to
302f14a
Compare
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Feel free to dm me if you need help :)
| if user_input is None: | ||
| return self.async_show_form( | ||
| step_id="user", data_schema=STEP_USER_DATA_SCHEMA, errors=errors | ||
| step_id=SOURCE_USER, |
There was a problem hiding this comment.
| step_id=SOURCE_USER, | |
| step_id="user", |
While they mean the same, source_user has different uses than the step_id being user
| return self.async_update_reload_and_abort( | ||
| self._get_reconfigure_entry(), | ||
| data=user_input, | ||
| reload_even_if_entry_is_unchanged=False, |
There was a problem hiding this comment.
Any reason we do this explicitly? If users encounter issues with this and try to reconfigure for an attempt to fix it, a reload might save things instead
| assert "type" in entry_result | ||
| assert "title" in entry_result | ||
| assert "data" in entry_result |
There was a problem hiding this comment.
Not needed, your IDE linting is probably too harsh
| flow = await hass.config_entries.flow.async_init( | ||
| DOMAIN, context={"source": config_entries.SOURCE_USER} | ||
| ) | ||
| with ( | ||
| patch( | ||
| "homeassistant.components.solax.config_flow.real_time_api", | ||
| return_value=__mock_real_time_api_success(), | ||
| ), | ||
| patch("solax.RealTimeAPI.get_data", return_value=__mock_get_data()), | ||
| patch( | ||
| "homeassistant.components.solax.async_setup_entry", | ||
| return_value=True, | ||
| ) as mock_setup_entry, | ||
| ): | ||
| entry_result = await hass.config_entries.flow.async_configure( | ||
| flow["flow_id"], | ||
| {CONF_IP_ADDRESS: "192.168.1.87", CONF_PORT: 80, CONF_PASSWORD: "password"}, | ||
| ) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert "result" in entry_result | ||
| assert "data" in entry_result | ||
| assert "context" in entry_result |
There was a problem hiding this comment.
Instead, please use a MockConfigEntry and add it to hass, and use that to reconfigure, instead of going through a whole flow here
| flow = await hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={ | ||
| "entry_id": entry_result["result"].entry_id, | ||
| "source": config_entries.SOURCE_RECONFIGURE, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Instead use await mock_config_entry.start_reconfigure_flow(hass)
| await hass.config_entries.flow.async_configure( | ||
| flow["flow_id"], | ||
| { | ||
| CONF_IP_ADDRESS: "192.168.1.187", | ||
| CONF_PORT: 8080, | ||
| CONF_PASSWORD: "password123", | ||
| }, | ||
| ) | ||
| await hass.async_block_till_done() | ||
|
|
||
| config_entry = hass.config_entries.async_entry_for_domain_unique_id( | ||
| DOMAIN, | ||
| entry_result["context"]["unique_id"], | ||
| ) | ||
| assert config_entry is not None | ||
| assert config_entry.data == { | ||
| CONF_IP_ADDRESS: "192.168.1.187", | ||
| CONF_PORT: 8080, | ||
| CONF_PASSWORD: "password123", | ||
| } |
There was a problem hiding this comment.
keep the result of async_configure and assert it
| flow["flow_id"], | ||
| {CONF_IP_ADDRESS: "192.168.1.87", CONF_PORT: 80, CONF_PASSWORD: "password"}, | ||
| ) | ||
| await hass.async_block_till_done() |
There was a problem hiding this comment.
It's not a blocking operation, so we can remove the block_till_done here
| assert len(mock_setup_entry.mock_calls) == 2 | ||
|
|
||
|
|
||
| async def test_form_configure_connect_error(hass: HomeAssistant) -> None: |
There was a problem hiding this comment.
Personal preference: Keep the user flow tests near each other (with the happy flow at the top). Now the file is
- user flow happy flow
- reconfigure
- user flow rest
- reconfigure error
| assert entry_result["type"] is FlowResultType.FORM | ||
| assert entry_result["errors"] == {"base": "cannot_connect"} |
There was a problem hiding this comment.
Same comment applies to this test as for the happy flow reconfiguration.
But here we should also repatch the library and try again to test that the flow is able to recover to make sure there are no race conditions
There was a problem hiding this comment.
And I am missing a test for trying to reconfigure into a different device (so if I reconfigure entry A to work with device with serial number B), that should abort
4 participants
Proposed change
Implement reconfiguration.
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: