Fix invalid unique id for Transmission entities #84664

avee87 · 2022-12-27T23:06:38Z

Proposed change

Currently it's using host and entity name for unique id which is not acceptable. Changing to config entry id + key.

Follow up PR will enable new entity naming and use entity descriptions - I don't want to do all this in one PR because it would do too many things at once and get stuck. See #74438 which I'm splitting into smaller parts now.

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:

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
The code has been formatted using Black (black --fast 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 · 2022-12-27T23:06:44Z

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

Code owner commands

Code owners of transmission can trigger bot actions by commenting:

@home-assistant close Closes the issue.
@home-assistant rename Awesome new title Change the title of the issue.
@home-assistant reopen Reopen the issue.
@home-assistant unassign transmission Removes the current integration label and assignees on the issue, add the integration domain after the command.

tests/components/transmission/test_init.py

engrbm87

Few suggestions for the migration test. Also let's add type hints for new code.

tests/components/transmission/test_init.py

homeassistant/components/transmission/__init__.py

home-assistant · 2023-04-28T09:34:03Z

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.

avee87 · 2023-04-28T10:11:29Z

Coverage issue is now gone! Thank you @epenet

tests/components/transmission/test_init.py

homeassistant/components/transmission/__init__.py

epenet

One more little tweak, to account for @engrbm87 comment regarding cases that can't happen, and then I think it's good.

tests/components/transmission/test_init.py

avee87 · 2023-04-28T12:03:05Z

I had those cases as an attempt to improve code coverage. They are not relevant now so will remove.

epenet

LGTM 👍
It would be nice to have confirmation from a code owner

avee87 · 2023-05-04T15:44:49Z

@engrbm87, @JPHutchins, any thoughts on this?

homeassistant/components/transmission/sensor.py

emontnemery

The unique_id cleanup is fine as such, but we should not add deprecated composing of names.
Background in https://developers.home-assistant.io/docs/core/entity#has_entity_name-true-mandatory-for-new-integrations

homeassistant/components/transmission/switch.py

emontnemery · 2023-06-28T07:41:50Z

homeassistant/components/transmission/sensor.py

-        self._client_name = client_name
-        self._name = sensor_name
-        self._sub_type = sub_type
+        self._attr_name = f"{client_name} {sensor_name}"


Please don't add this unwanted composing of name with device name.
Instead, leave it as is and in another PR set _attr_has_entity_name = True to have the name composing happen automatically in frontend.

I'm not adding name composing, I'm keeping existing functionality just moving it from function to attribute.

I've been explicitly asked to avoid doing too many things in one PR so this one is focused on unique_id. Once it's merged I'll be happy to migrate to new entity naming.

You're right, sorry about that!
A PR setting _attr_has_entity_name = True would still be welcome 👍

emontnemery

Thanks, @avee87 👍

Update unique id for Transmission entities

6dce495

home-assistant bot added cla-signed code-quality has-tests integration: transmission Quality Scale: No score labels Dec 27, 2022

engrbm87 suggested changes Dec 28, 2022

View reviewed changes

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

Moved migration to a separate function

2d267da

engrbm87 suggested changes Dec 28, 2022

View reviewed changes

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

avee87 added 3 commits December 28, 2022 18:35

Hopefully fixed coverage

3672260

Extracted dictionary to constant

b9c0944

Merge branch 'dev' into transmission_unique_id

712d12b

This was referenced Mar 7, 2023

Transmission "idle" status is lower cased, unlike other statuses #87854

Closed

[dev] Transmission integration creates duplicate entities after upgrading to latest dev #89325

Closed

Revert Transmission entities name changes #89409

Merged

Merge branch 'dev' into transmission_unique_id

e39769a

avee87 mentioned this pull request Apr 26, 2023

Improve Transmission entity naming #92091

Closed

20 tasks

epenet requested changes Apr 28, 2023

View reviewed changes

home-assistant bot marked this pull request as draft April 28, 2023 09:34

avee87 added 4 commits April 28, 2023 10:40

Merge branch 'dev' into transmission_unique_id

20981e9

review comments

f8f2033

more comments

aa0ffd5

revert accidental name change

12fb5c4

avee87 marked this pull request as ready for review April 28, 2023 10:10

home-assistant bot requested review from engrbm87 and epenet April 28, 2023 10:10

epenet requested changes Apr 28, 2023

View reviewed changes

home-assistant bot marked this pull request as draft April 28, 2023 11:30

epenet changed the title ~~Update unique id for Transmission entities~~ Fix invalid unique id for Transmission entities Apr 28, 2023

more review comments

d6d9a74

avee87 marked this pull request as ready for review April 28, 2023 11:51

avee87 requested a review from epenet April 28, 2023 11:51

epenet requested changes Apr 28, 2023

View reviewed changes

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

tests/components/transmission/test_init.py Outdated Show resolved Hide resolved

home-assistant bot marked this pull request as draft April 28, 2023 11:52

more review comments

882d8f1

avee87 marked this pull request as ready for review April 28, 2023 12:06

avee87 requested a review from epenet April 28, 2023 12:06

epenet approved these changes Apr 28, 2023

View reviewed changes

JPHutchins reviewed May 4, 2023

View reviewed changes

homeassistant/components/transmission/sensor.py Outdated Show resolved Hide resolved

use lists instead of incorrect tuple syntax

ac28301

frenck added the smash Indicator this PR is close to finish for merging or closing label Jun 21, 2023

emontnemery requested changes Jun 28, 2023

View reviewed changes

home-assistant bot marked this pull request as draft June 28, 2023 07:43

frenck removed the smash Indicator this PR is close to finish for merging or closing label Jun 28, 2023

avee87 marked this pull request as ready for review June 28, 2023 08:16

avee87 requested a review from emontnemery June 28, 2023 08:17

emontnemery approved these changes Jun 28, 2023

View reviewed changes

emontnemery merged commit a5b91cb into home-assistant:dev Jun 28, 2023
24 checks passed

avee87 deleted the transmission_unique_id branch June 28, 2023 10:52

github-actions bot locked and limited conversation to collaborators Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid unique id for Transmission entities #84664

Fix invalid unique id for Transmission entities #84664

avee87 commented Dec 27, 2022

home-assistant bot commented Dec 27, 2022

engrbm87 left a comment

home-assistant bot commented Apr 28, 2023

avee87 commented Apr 28, 2023

epenet left a comment

avee87 commented Apr 28, 2023

epenet left a comment

avee87 commented May 4, 2023

emontnemery left a comment

emontnemery Jun 28, 2023

avee87 Jun 28, 2023

emontnemery Jun 28, 2023 •

edited

emontnemery left a comment

Fix invalid unique id for Transmission entities #84664

Fix invalid unique id for Transmission entities #84664

Conversation

avee87 commented Dec 27, 2022

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Dec 27, 2022

engrbm87 left a comment

Choose a reason for hiding this comment

home-assistant bot commented Apr 28, 2023

avee87 commented Apr 28, 2023

epenet left a comment

Choose a reason for hiding this comment

avee87 commented Apr 28, 2023

epenet left a comment

Choose a reason for hiding this comment

avee87 commented May 4, 2023

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery Jun 28, 2023

Choose a reason for hiding this comment

avee87 Jun 28, 2023

Choose a reason for hiding this comment

emontnemery Jun 28, 2023 • edited

Choose a reason for hiding this comment

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery Jun 28, 2023 •

edited