Re-Auth Config Flow for vesync #137398
Re-Auth Config Flow for vesync #137398
Conversation
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, @iprak, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| if login: | ||
| return self.async_update_reload_and_abort(reauth_entry, data=data) |
There was a problem hiding this comment.
If we can't login, I think we should show an error. We should also take in account any exceptions raised because we could not connect to VeSync (for whatever reason)
There was a problem hiding this comment.
I think the usual pattern to raise ConfigEntryAuthFailed which causes HomeAssistant to present authentication flow.
| async def test_reauth_flow_invalid_auth(hass: HomeAssistant) -> None: | ||
| """Test an authorization error reauth flow.""" | ||
|
|
||
| mock_entry = MockConfigEntry( | ||
| domain=DOMAIN, | ||
| unique_id="test-username", | ||
| ) | ||
| mock_entry.add_to_hass(hass) | ||
|
|
||
| result = await mock_entry.start_reauth_flow(hass) | ||
| assert result["step_id"] == "reauth_confirm" | ||
| assert result["type"] is FlowResultType.FORM | ||
|
|
||
| with patch("pyvesync.vesync.VeSync.login", return_value=False): | ||
| result2 = await hass.config_entries.flow.async_configure( | ||
| result["flow_id"], | ||
| {CONF_USERNAME: "new-username", CONF_PASSWORD: "new-password"}, | ||
| ) | ||
|
|
||
| assert result2["type"] is FlowResultType.FORM |
There was a problem hiding this comment.
Let's have this test repatched so it will succeed (and thus end in an ABORT), this way we test that the flow is able to finish
There was a problem hiding this comment.
Wouldn't that be the test_reauth_flow test further up? It completes the reconfig, where as this one is a reauth with invalid details.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
It is not clear from the PR statement how we know that login failed. Would some kind of downtime at Vesync end resulting in failed request appear the same ? |
Co-authored-by: Joost Lekkerkerker <[email protected]>
|
Correct downtime right now would trigger this same flow if response is false on login. Now this is only on init so if by chance you reload the integration at the time of an outage you would get stuck within that until the API is back up. Right now if you have a password issue it has no way out. An API outage in the code here I don't think generally would trigger since you are already logged in and that exists in the coordinator code vs the init of the integration. |
Proposed change
Allow re-auth when failed to login. As a note we don't know the cause of login for from the library just that it fails.
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: