Record current IQS state for AVM FRITZ!SmartHome #131363
Conversation
|
Hey there @flabbamann, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Note: hassfest failure is not unrelated... |
|
interesting - didn't get this hassfest error in my dev-container 🤔 |
|
CI error ist fixed - but still keep it as draft, since there are some small docs improvements pending, which were already considered for this. |
| dynamic-devices: done | ||
| entity-category: done | ||
| entity-device-class: done | ||
| entity-disabled-by-default: done |
There was a problem hiding this comment.
nothing is though, double check?
There was a problem hiding this comment.
you mean set it to exempt because nothing is disabled by default?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| comment: only set for sensors, yet | ||
| runtime-data: done | ||
| test-before-configure: done | ||
| test-before-setup: done |
There was a problem hiding this comment.
So with the coordinator, would it make sense to refactor to use _async_setup?
There was a problem hiding this comment.
we have an own async_setup in the coordinator, as we need to do a first data fetch and afterwards run the internal cleanup - this flow wouldn't be possible when using the _asnyc_setup as it is called at the beginning of async_config_entry_first_refresh, those can't contain any actions which needs already fetched data
| runtime-data: done | ||
| test-before-configure: done | ||
| test-before-setup: done | ||
| unique-config-entry: done |
There was a problem hiding this comment.
To verify, users can add the same device twice with 2 accounts?
There was a problem hiding this comment.
nope, only one config entry per device
| test-coverage: done | ||
|
|
||
| # Gold | ||
| devices: done |
There was a problem hiding this comment.
What are those template devices in button.py? Should we link them to the main device with via_device?
There was a problem hiding this comment.
these templates are scripts defined in the fritzbox itself and with the button entities you can trigger/start them. so they aren't necessarily linked to a single device
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
Can we finish test_user_auth_failed and test_user_not_succesful?
There was a problem hiding this comment.
Also test_ssdp_auth_failed and test_ssdp_not_successful?
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
test_user_already_configured would be nicer if we added a mock config entry instead of doing the flow twice
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
Is there a way we can prevent a reconfigure happening to a different device? Comparing the device list or something?
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
We should avoid having if statements in the test_ssdp tests
| status: todo | ||
| comment: not set at the moment, we use a coordinator | ||
| reauthentication-flow: done | ||
| test-coverage: done |
There was a problem hiding this comment.
I personally find that the test_coordinator.py tests are more test_init.py tests
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Definitely not stale 😬 |
| entity-translations: done | ||
| exception-translations: done | ||
| icon-translations: done | ||
| reconfiguration-flow: done |
There was a problem hiding this comment.
While you have a reconfigure flow in fritzbox, it doesn't allow to reconfigure the username and password. Somebody needs to wait for the reauthentication flow to kick in or delete and reconfigure the config entry once again.
There was a problem hiding this comment.
the reconfiguration flow is not intended to update credentials, that's the intention for the reauth flow
There was a problem hiding this comment.
Currently the documentation mentions that it should not be used for reauthentication, but the quality scale actually says it should be included. Something needs to be changed to make it clearer and while it's being looked in what, I guess we're both right.
There was a problem hiding this comment.
The IQS rule about the reconfiguration says, that an integration should ideally inform the user, that a re-authentication is needed - this is done by the re-auth-flow. Only when we can't detect wrong credentials, then we need to provide to change them via the reconfiguration flow
Proposed change
SSIA
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: