Handle router initialization, connection errors, and missing interfaces in options flow #143475

foxel · 2025-04-22T18:21:09Z

Breaking change

Proposed change

Added checks in the Keenetic NDMS2 options flow to handle cases where the integration is not initialized or there are connection errors. Relevant user feedback and abort reasons are now provided to ensure a better user experience.

Also added filtering out unavailable options for the config flow, since the frontent does not do that

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes Problem with Keenetic Giga 1010 connection #142267 Keenetic NDMS2 Router detects only 1 device #145970
This PR is related to issue: Change password to keenetic_ndms2 frontend#24890
Link to documentation pull request:
Link to developer documentation pull request:
Link to frontend pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Ruff (ruff format homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

joostlek · 2025-05-14T15:22:48Z

homeassistant/components/keenetic_ndms2/config_flow.py

+        try:
+            router: KeeneticRouter = self.hass.data[DOMAIN][self.config_entry.entry_id][
+                ROUTER
+            ]
+        except KeyError:
+            return self.async_abort(reason="not_initialized")


Instead let's check if the value we want to use is there instead of letting it raise a KeyError

@joostlek , thanks for the review. done

joostlek · 2025-05-14T15:23:05Z

tests/components/keenetic_ndms2/test_config_flow.py

+    entry = MockConfigEntry(domain=keenetic.DOMAIN, data=MOCK_DATA)
+    entry.add_to_hass(hass)
+
+    # no router set
+    hass.data.setdefault(keenetic.DOMAIN, {})
+


Suggested change

entry = MockConfigEntry(domain=keenetic.DOMAIN, data=MOCK_DATA)

entry.add_to_hass(hass)

# no router set

hass.data.setdefault(keenetic.DOMAIN, {})

Hi. I don't understand this suggestion. Is it to remove an empty line? I think it's good there to separate the preparation and the 'result = ...' line

Okay let me rephrase, we should not touch internals like hass.data and I think you can create the mock config entry as something we can reuse

We should mock the library and have HA set up the integration like normal

This part is not touching any internals. hass.data is the state collection where any integration is free to store its state under their domain. And we just put a desired test state there (no state in this particular test).

The test verifies the config flow in case the integration initialization is not complete for any reason. Since this test does not check the integration entirely and only the configuration, we are not trying to make the integration fail. Instead, we mock the integration part in a state which is the testing condition for the config flow.

So this is perfectly normal.

This part is not touching any internals. hass.data is the state collection where any integration is free to store its state under their domain.

No, hass.data is considered internal. We can set that to anything we like and then we have the assumption that HA sets it like that. Those lead to fragile tests and thus we consider hass.data as internal. Many integrations are moving to storing their runtime data in entry.runtime_data and that migration ideally should happen without changes in the tests as the tests should not care where the data comes from

OK. This is something new AFAIK.
However, since this PR does not perform a migration, the tests match how the integration and options flow work. I can do the migration to entry.runtime_data later along with the strict typing changes I have in my stash. Then there will be no touching of hass.data.

We can set that to anything we like and then we have the assumption that HA sets it like that.

You mean "writing a test we have the assumption that HA sets it like that"?. But that's not exactly true. Writing a test for the flow we know: a. the flow will be reading this, b. the integration is expected to be setting this if it's loaded.

In summary, this is a unit test for the flow, and it's specifically testing the case where the integration is not yet loaded. So anyway, no need to initialize the integration entity in the test.

Hi @joostlek

So somebody migrated the integration to runtime_data for me. No touching of hass anymore. Please review

Fun fact, entry.runtime_data is still considered internal. I merged that PR as that didn't change it, but I think we should refactor the tests to make sure we only take control of the library and then just have HA do its think and observe via the state machine.
But let's merge this for now and I would appreciate if we can refactor the tests.

home-assistant · 2025-05-14T15:23:47Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

foxel · 2025-06-13T11:35:59Z

Hi, @joostlek . Can you please take another look at the PR?

joostlek · 2025-06-13T11:44:37Z

It's not marked as ready for review yet

foxel · 2025-06-13T14:01:05Z

@joostlek I was sure it is. Set ready

joostlek · 2025-06-16T20:50:02Z

tests/components/keenetic_ndms2/test_config_flow.py

+    entry = MockConfigEntry(domain=keenetic.DOMAIN, data=MOCK_DATA)
+    entry.add_to_hass(hass)
+
+    # no router set
+    hass.data.setdefault(keenetic.DOMAIN, {})
+


Okay let me rephrase, we should not touch internals like hass.data and I think you can create the mock config entry as something we can reuse

joostlek · 2025-06-16T20:50:21Z

tests/components/keenetic_ndms2/test_config_flow.py

+    entry = MockConfigEntry(domain=keenetic.DOMAIN, data=MOCK_DATA)
+    entry.add_to_hass(hass)
+
+    # no router set
+    hass.data.setdefault(keenetic.DOMAIN, {})
+


We should mock the library and have HA set up the integration like normal

Added checks in the Keenetic NDMS2 options flow to handle cases where the integration is not initialized or there are connection errors. Relevant user feedback and abort reasons are now provided to ensure a better user experience.

…n options form

…es in options flow (home-assistant#143475) * Handle router initialization and connection errors in options flow Added checks in the Keenetic NDMS2 options flow to handle cases where the integration is not initialized or there are connection errors. Relevant user feedback and abort reasons are now provided to ensure a better user experience. * Add filtering saved/default options for interfaces before preparing an options form

home-assistant bot added bugfix cla-signed has-tests integration: keenetic_ndms2 small-pr PRs with less than 30 lines. by-code-owner Quality Scale: No score labels Apr 22, 2025

foxel mentioned this pull request Apr 22, 2025

Add reconfiguration support for keenetic_ndms2 integration #142191

Merged

19 tasks

joostlek requested changes May 14, 2025

View reviewed changes

home-assistant bot marked this pull request as draft May 14, 2025 15:23

foxel force-pushed the keenetic-fix-options-flow-fail branch from 86ae6d3 to cb442c6 Compare June 7, 2025 19:22

foxel requested a review from joostlek June 7, 2025 19:24

foxel changed the title ~~Handle router initialization and connection errors in options flow~~ Handle router initialization, connection errors, and missing interfaces in options flow Jun 13, 2025

foxel marked this pull request as ready for review June 13, 2025 14:00

joostlek requested changes Jun 16, 2025

View reviewed changes

home-assistant bot marked this pull request as draft June 16, 2025 20:50

foxel marked this pull request as ready for review June 17, 2025 03:54

home-assistant bot requested a review from joostlek June 17, 2025 03:54

foxel marked this pull request as draft June 17, 2025 03:55

foxel marked this pull request as ready for review June 17, 2025 04:07

joostlek marked this pull request as draft June 17, 2025 08:28

foxel marked this pull request as ready for review June 18, 2025 03:12

foxel added 2 commits June 21, 2025 03:09

Add filtering saved/default options for interfaces before preparing a…

040f57c

…n options form

foxel force-pushed the keenetic-fix-options-flow-fail branch from 7922c69 to 040f57c Compare June 20, 2025 23:10

joostlek approved these changes Jun 23, 2025

View reviewed changes

joostlek merged commit 6641cb3 into home-assistant:dev Jun 23, 2025
34 checks passed

github-actions bot locked and limited conversation to collaborators Jun 24, 2025

Uh oh!

Handle router initialization, connection errors, and missing interfaces in options flow #143475

Handle router initialization, connection errors, and missing interfaces in options flow #143475

Uh oh!

Conversation

foxel commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foxel Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

home-assistant bot commented May 14, 2025

Uh oh!

foxel commented Jun 13, 2025

Uh oh!

joostlek commented Jun 13, 2025

Uh oh!

foxel commented Jun 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

foxel commented Apr 22, 2025 •

edited

Loading

foxel Jun 17, 2025 •

edited

Loading