Add config_flow to bluesound integration #115207

LouisChrist · 2024-04-08T13:16:21Z

Proposed change

Adds user config_flow for bluesound integration and removes configurations via yaml.

The documentation for using the configuration.yaml is removed and replaced by documentation for using the config_flow(see matching PR for docs)

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 #
This PR is related to issue: -
Link to documentation pull request: Replace bluesound configuration with config_flow home-assistant.io#32218

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.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

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

home-assistant

Hi @LouisChrist

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant · 2024-04-08T13:16:29Z

Hey there @thrawnarn, mind taking a look at this pull request as it has been labeled with an integration (bluesound) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bluesound can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign bluesound Removes the current integration label and assignees on the pull request, add the integration domain after the command.
@home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
@home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

joostlek

Before this integration can be migrated to a config flow, it should adhere to ADR-0010, which states that all code used to connect to a device or service should be encapsulated in a python library published on pypi.

home-assistant · 2024-04-08T18:21:02Z

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.

LouisChrist · 2024-04-08T19:01:11Z

My Bad. My intent was to do it the other way around as this seemed to be the smaller task.

I'm working on a library in parallel: pyblu. Should I do all that in one PR or should I split it into a separate PR?

LouisChrist · 2024-04-27T15:31:29Z

I created and integrated a separate library for interacting with the devices: pyblu.

The parts regarding join/unjoin of devices are not tested with real devices as i have only one. They are tested to the degree that they do not return any 'Bad Request'. And there are unit tests in the library, which are based on the documentation.

I tried to change as little as possible in the integration itself.

Stale

joostlek · 2024-05-11T11:56:55Z

Can we first do a prelimary PR to make the integration use the library?

LouisChrist · 2024-05-11T12:41:51Z

I created a separate PR: Integrate pypi library: pyblu

fredrike · 2024-05-29T08:28:54Z

Wow great work in getting Bluesound up to standards with a real config-flow.

I didn't look before I created my own config-flow (with zeroconf discovery). Perhaps you find something useful from my code (or once we get this merged I can create a new PR with the zeroconf code).

dev...fredrike:home-assistant:bluesound-updates

LouisChrist · 2024-05-29T17:16:48Z

Wow great work in getting Bluesound up to standards with a real config-flow.

I didn't look before I created my own config-flow (with zeroconf discovery). Perhaps you find something useful from my code (or once we get this merged I can create a new PR with the zeroconf code).

dev...fredrike:home-assistant:bluesound-updates

Thank you.

I can integrate your zeroconf code into this PR, if you are ok with that. It seems to be a rather small addition. Or you can do it as a separate PR(doing zeroconf a separate PR was my initial plan). That is your decision, since it's your code i would be copying.

fredrike · 2024-05-29T17:54:24Z

Feel free to use "my" code in any way you want. I didn't think there were anyone actively developing for Bluesound and was quite annoyed that the integration didn't support config registry.

joostlek · 2024-07-27T13:05:13Z

What are we going to do with the entities in hass.data? They should be removed but if we keep the logic the same we can do that in a followup, wdyt?

LouisChrist · 2024-07-27T16:34:57Z

I would do the hass.data stuff in a different MR. There are quite few things that could be changed and simplified regarding the initialization of the entity. And this MR is already pretty large.

joostlek · 2024-07-27T16:41:20Z

I know, but i am wondering, because you moved the service things to the __init__.py from media_player.py, is that just moving around or did it server a purpose. Because if it did not i would think we should leave that at the old place

LouisChrist · 2024-07-27T16:55:54Z

I moved it because of a circular dependency. I could move setup_services back to media_player.py and move BluesoundConfigEntry from __init__.py to another file.

joostlek · 2024-07-27T16:57:59Z

I think you can do that, but instead of moving the config entry type you can do

if TYPE_CHECKING:
    from x import x

It might also need from __future__ import annotations

I think that might work

joostlek · 2024-07-27T17:29:11Z

homeassistant/components/bluesound/media_player.py

-        _LOGGER.info("Added device with name: %s", player.name)
+        hass.data[DATA_BLUESOUND].append(bluesound_player)
+        async_add_entities([bluesound_player])
+        _LOGGER.info("Added device with name: %s", bluesound_player.name)


Don't log on info

joostlek · 2024-07-27T17:30:02Z

homeassistant/components/bluesound/media_player.py

+        if result["type"] == FlowResultType.ABORT:
+            ir.async_create_issue(
+                hass,
+                DOMAIN,
+                f"deprecated_yaml_import_issue_{result['reason']}",
+                breaks_in_ha_version="2025.2.0",
+                is_fixable=False,
+                issue_domain=DOMAIN,
+                severity=ir.IssueSeverity.WARNING,
+                translation_key=f"deprecated_yaml_import_issue_{result['reason']}",
+                translation_placeholders={
+                    "domain": DOMAIN,
+                    "integration_title": INTEGRATION_TITLE,
+                },
+            )
+            return


If we can't import because the connection failed (cannot connect) we should raise a different issue. Please check incomfort for an example

Oh right, we do that here, but if we abort because the config entry already exists, we should raise the generic one

joostlek · 2024-07-27T17:31:43Z

homeassistant/components/bluesound/media_player.py

+    host = config_entry.data.get(CONF_HOST)
+    port = config_entry.data.get(CONF_PORT)


Suggested change

host = config_entry.data.get(CONF_HOST)

port = config_entry.data.get(CONF_PORT)

host = config_entry.data[CONF_HOST]

port = config_entry.data[CONF_PORT]

joostlek · 2024-07-27T17:32:00Z

homeassistant/components/bluesound/media_player.py

+    assert isinstance(host, str)
+    assert isinstance(port, int)


Double check if we need this after the previous comment

joostlek · 2024-07-27T17:32:55Z

homeassistant/components/bluesound/strings.json

+        "description": "[%key:common::config_flow::description::confirm_setup%]"
+      }
+    },
+    "abort": {


I think zeroconf also requires the flow_in_progress translation key

I cannot find flow_in_progress. Do you mean already_in_progress? That one seems to be thrown in async_set_unique_id.

Yes! I am on mobile so couldn't check the code

joostlek · 2024-07-27T17:34:57Z

tests/components/bluesound/test_config_flow.py

+    assert result["type"] is FlowResultType.CREATE_ENTRY
+    assert result["title"] == "player-name"
+    assert result["data"] == {CONF_HOST: "1.1.1.1", CONF_PORT: 11000}


Let's also assert unique id?

joostlek · 2024-07-27T17:35:30Z

tests/components/bluesound/test_config_flow.py

+        },
+    )
+
+    assert result["type"] is FlowResultType.FORM


We should test if the flow is able to recover an error, so rewatch and make sure we finish the flow

joostlek · 2024-07-27T17:35:52Z

tests/components/bluesound/test_config_flow.py

+    hass: HomeAssistant, mock_setup_entry: AsyncMock, mock_player_sync_status: AsyncMock
+) -> None:
+    """Test we get the form."""
+    hass.data[DOMAIN] = []


Don't touch internals

joostlek · 2024-07-27T17:36:08Z

tests/components/bluesound/test_config_flow.py

+    mock_entry = MockConfigEntry(
+        domain=DOMAIN,
+        data={
+            CONF_HOST: "1.1.1.1",
+            CONF_PORT: 11000,
+        },
+        unique_id="00:11:22:33:44:55-11000",
+    )


You can make this a fixture

joostlek · 2024-07-27T17:36:44Z

tests/components/bluesound/test_config_flow.py

+    assert result["type"] is FlowResultType.FORM
+    assert result["step_id"] == "confirm"
+
+    mock_setup_entry.assert_not_called()
+    mock_player_sync_status.assert_called_once()


Let's also make this flow finish in a create entry

joostlek · 2024-07-27T17:38:35Z

I think this is the last round. The beta cut for 2024.8 is next Wednesday, so I'm confident we make it

LouisChrist · 2024-07-27T22:50:40Z

I also made a small change: Zeroconf now uses the provided port(it was always the default port before).

joostlek · 2024-07-28T17:37:43Z

I fixed some tiny things to unify the fixtures and a small typo, didn't feel like burdening you with another pass. Thanks for helping out. Can you also send you discord username? (Or send me a message)

MartinHjelmare · 2024-08-25T19:10:41Z

homeassistant/components/bluesound/media_player.py

-                player
-                for player in hass.data[DATA_BLUESOUND]
-                if player.entity_id in entity_ids
+                player for player in hass.data[DOMAIN] if player.entity_id in entity_ids


Side note: In the future this should be refactored to not store the entities in hass.data but use our entity service helper to register entity services, and also use our standard ways to group media player entities (which doesn't require custom services).

https://developers.home-assistant.io/docs/core/entity/media-player#grouping-player-entities-together

To keep track of added entities we can eg store the player id. Look at the heos integration for an example how to map players to media player entities and group media players.

I already have a branch for implementing the standard way to group media player entities. My solution was:

get entity_entry from entity_registry

get config_entry from hass.config_entries.async_get_entry(entity_entry.config_entry_id)

the config_entry has everything needed for grouping

But there is one big problem: I have only one Player. I cannot test it.

Are there any examples for entity services? I couldn't find any.

Here's an example:

core/homeassistant/components/denonavr/media_player.py

Lines 142 to 147 in 8bc9fd2

platform = entity_platform.async_get_current_platform()

platform.async_register_entity_service(

SERVICE_GET_COMMAND,

{vol.Required(ATTR_COMMAND): cv.string},

f"async_{SERVICE_GET_COMMAND}",

)

But there is one big problem: I have only one Player. I cannot test it.

Written tests are enough, if you can't test it manually.

I have a few players and I'm happy to help with testing over the next week or two. Can you point me to the release or branch that needs to be tested, and give a brief description of the testing that you'd like?

That would be great. I don't have working code yet. And I have 2 things i want to do first before refactoring the media player grouping. I can tag you on the media player grouping PR once I have created it, but it will most likely be more that two weeks from now.

home-assistant bot previously requested changes Apr 8, 2024

View reviewed changes

home-assistant bot added cla-needed code-quality config-flow This integration migrates to the UI by adding a config flow has-tests integration: bluesound labels Apr 8, 2024

home-assistant bot added Quality Scale: No score cla-recheck cla-signed and removed cla-recheck cla-needed labels Apr 8, 2024

LouisChrist mentioned this pull request Apr 8, 2024

Replace bluesound configuration with config_flow home-assistant/home-assistant.io#32218

Merged

8 tasks

LouisChrist marked this pull request as ready for review April 8, 2024 16:48

joostlek requested changes Apr 8, 2024

View reviewed changes

home-assistant bot marked this pull request as draft April 8, 2024 18:21

LouisChrist force-pushed the bluesound_config_flow branch from db6a4bc to 19d87db Compare April 27, 2024 15:28

LouisChrist marked this pull request as ready for review April 27, 2024 15:37

home-assistant bot requested a review from joostlek April 27, 2024 15:37

LouisChrist mentioned this pull request May 11, 2024

Use pyblu library in bluesound #117257

Merged

20 tasks

LouisChrist force-pushed the bluesound_config_flow branch from 19d87db to a64e8ee Compare May 11, 2024 12:44

fix test; add more tests

a5b1fa4

LouisChrist added 2 commits July 27, 2024 17:56

use AsyncMock assert functions

4027452

fix potential naming collision

6343225

LouisChrist marked this pull request as ready for review July 27, 2024 16:35

home-assistant bot requested a review from joostlek July 27, 2024 16:35

joostlek requested changes Jul 27, 2024

View reviewed changes

home-assistant bot marked this pull request as draft July 27, 2024 17:36

move setup_services back to media_player.py

80a9112

LouisChrist added 2 commits July 28, 2024 00:25

implement requested changes

d64713a

add port to zeroconf flow

63bb483

LouisChrist marked this pull request as ready for review July 27, 2024 22:50

home-assistant bot requested a review from joostlek July 27, 2024 22:50

Fix comments

e35199b

joostlek approved these changes Jul 28, 2024

View reviewed changes

joostlek merged commit f98487e into home-assistant:dev Jul 28, 2024
40 checks passed

joostlek added new-feature and removed code-quality labels Jul 28, 2024

github-actions bot locked and limited conversation to collaborators Jul 29, 2024

LouisChrist deleted the bluesound_config_flow branch August 3, 2024 09:44

MartinHjelmare reviewed Aug 25, 2024

View reviewed changes

home-assistant unlocked this conversation Aug 25, 2024

github-actions bot locked and limited conversation to collaborators Aug 27, 2024

		host = config_entry.data.get(CONF_HOST)
		port = config_entry.data.get(CONF_PORT)

	platform = entity_platform.async_get_current_platform()
	platform.async_register_entity_service(
	SERVICE_GET_COMMAND,
	{vol.Required(ATTR_COMMAND): cv.string},
	f"async_{SERVICE_GET_COMMAND}",
	)

Add config_flow to bluesound integration #115207

Add config_flow to bluesound integration #115207

Conversation

LouisChrist commented Apr 8, 2024 • edited Loading

Proposed change

Type of change

Additional information

Checklist

home-assistant bot left a comment

Choose a reason for hiding this comment

home-assistant bot commented Apr 8, 2024

joostlek left a comment

Choose a reason for hiding this comment

home-assistant bot commented Apr 8, 2024

LouisChrist commented Apr 8, 2024 • edited Loading

LouisChrist commented Apr 27, 2024 • edited Loading

joostlek commented May 11, 2024

LouisChrist commented May 11, 2024

fredrike commented May 29, 2024

LouisChrist commented May 29, 2024 • edited Loading

fredrike commented May 29, 2024

joostlek commented Jul 27, 2024

LouisChrist commented Jul 27, 2024

joostlek commented Jul 27, 2024

LouisChrist commented Jul 27, 2024

joostlek commented Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostlek commented Jul 27, 2024

LouisChrist commented Jul 27, 2024

joostlek commented Jul 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouisChrist commented Apr 8, 2024 •

edited

Loading

LouisChrist commented Apr 8, 2024 •

edited

Loading

LouisChrist commented Apr 27, 2024 •

edited

Loading

LouisChrist commented May 29, 2024 •

edited

Loading

joostlek commented Jul 27, 2024 •

edited

Loading