Return specific group state if there is one #115866

jbouwh · 2024-04-19T23:43:43Z

Breaking change

The state of a group entity will now return a specific ON or OFF state, even if the entity component supports multiple ON states, if the group state is on, and there and the ON states are from entities from a single domain, and one ON state used. E.g. all switched on vacuum's are cleaning -> the on state will be cleaning. The previous behavior was that the group state was ON.

Users should check their automation's still work correctly if they are based on group states.

Proposed change

Return specific group state if there is one. This prepares the group integration to handle integrations that have entities with multiple on states, like vacuum and like lock will have.

Added test cases for vacuum to assert with multiple on states.

This also prepares the lock integration can share an OPEN state with cover.

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: Document group state changes when in cases with one specific state home-assistant.io#32443

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 · 2024-04-19T23:43:50Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (group) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of group 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 group 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.

homeassistant/components/group/entity.py

bdraco · 2024-04-22T14:48:29Z

It seems like most cases will be a single active domain. Suggestion to optimize for that bdraco@34d4f95

homeassistant/components/group/entity.py

homeassistant/components/group/registry.py

emontnemery · 2024-04-22T14:54:18Z

homeassistant/components/group/registry.py

@@ -60,11 +66,15 @@ def exclude_domain(self) -> None:

    def on_off_states(self, on_states: set, off_state: str) -> None:
        """Register on and off states for the current domain."""
+        domain = current_domain.get()


In a follow-up PR, the context variable should be removed, and domain should be passed instead. There's a single custom integration published in HACS - bodymiscale - which also implements mixed group support so a breaking change + blog post + notifying the author of bodymiscale should be fine.

home-assistant · 2024-04-22T14:54:30Z

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.

KapJI · 2024-04-23T21:12:35Z

Btw, this is not a code quality improvement, this is a breaking change because it changes how group is reporting states. This needs to be mentioned in documentation as well.

jbouwh · 2024-04-23T21:34:45Z

Btw, this is not a code quality improvement, this is a breaking change because it changes how group is reporting states. This needs to be mentioned in documentation as well.

I'd rather call this a bugfix as the docs seem clear.

KapJI · 2024-04-23T22:03:08Z

I'd rather call this a bugfix

Why you consider old behaviour a bug?

the docs seem clear

https://www.home-assistant.io/integrations/group/
"When a group contains entities from domains that have multiple on states or only use on and off, the group state will be on or off."

jbouwh · 2024-04-24T06:00:39Z

I'd rather call this a bugfix

Why you consider old behaviour a bug?

the docs seem clear

https://www.home-assistant.io/integrations/group/ "When a group contains entities from domains that have multiple on states or only use on and off, the group state will be on or off."

Well as you can see in some fixed texts, that sometimes worked. E.g. the state of a person and device_tracker are now behaving like that. But multiple vacuums, all cleaning would only return ON. That is now fixed.
Further we planned for lock to support an extra ON state. Without this PR it could break automation's with lock groups, as they would get a state ON in stead of LOCKED or UNLOCKED that is the main reason for this PR. Now the state becomes ON only if there are multiple active ON states. I will update the breaking change section though and open a docs PR.

emontnemery · 2024-04-24T09:19:48Z

When reading the breaking change note it's not clear to me what the change is. Maybe improve the examples to explain what the behavior was before the change, not just what it is after the change?

homeassistant/components/group/entity.py

Co-authored-by: Erik Montnemery <erik@montnemery.com>

jbouwh · 2024-04-24T09:35:30Z

When reading the breaking change note it's not clear to me what the change is. Maybe improve the examples to explain what the behavior was before the change, not just what it is after the change?

Updated the examples a bit to show the difference.

gjohansson-ST · 2024-04-24T09:41:51Z

... the group state is off, (all entities are in an OFF state) and all OFF states are the same. E.g. all entities are unlocked, the group state will be unlocked.

The off state for lock is locked I believe.

jbouwh · 2024-04-24T09:46:12Z

... the group state is off, (all entities are in an OFF state) and all OFF states are the same. E.g. all entities are unlocked, the group state will be unlocked.

The off state for lock is locked I believe.

https://www.home-assistant.io/integrations/group/ says:

When member entities all have a single on and off state, the group state will be calculated as follows:

Domain	on	off
device_tracker	home	not_home
cover	open	closed
lock	unlocked	locked
person	home	not_home
media_player	ok	problem

jbouwh · 2024-04-24T09:49:01Z

... the group state is off, (all entities are in an OFF state) and all OFF states are the same. E.g. all entities are unlocked, the group state will be unlocked.

The off state for lock is locked I believe.

core/homeassistant/components/lock/group.py

Lines 12 to 17 in e9e401a

    
           @callback 
        
           def async_describe_on_off_states( 
        
               hass: HomeAssistant, registry: "GroupIntegrationRegistry" 
        
           ) -> None: 
        
               """Describe group on off states.""" 
        
               registry.on_off_states({STATE_UNLOCKED}, STATE_LOCKED)

jbouwh · 2024-04-24T09:49:59Z

... the group state is off, (all entities are in an OFF state) and all OFF states are the same. E.g. all entities are unlocked, the group state will be unlocked.

The off state for lock is locked I believe.

You are right, updated the PR breaking section

emontnemery

LGTM

This reverts commit 350ca48.

Revert "Return specific group state if there is one (#115866)" This reverts commit 350ca48.

Return specific group state if there is one

f92aa41

home-assistant bot added cla-signed code-quality core has-tests integration: group labels Apr 19, 2024

home-assistant bot added the Quality Scale: internal label Apr 19, 2024

jbouwh added 2 commits April 20, 2024 10:35

Refactor

62ca890

Additional test cases

a5e92dc

jbouwh marked this pull request as ready for review April 20, 2024 11:12

jbouwh requested a review from a team as a code owner April 20, 2024 11:12

jbouwh mentioned this pull request Apr 20, 2024

Add open state to LockEntity #111968

Merged

20 tasks

bdraco reviewed Apr 21, 2024

View reviewed changes