home-assistant / android

:iphone: Home Assistant Companion for Android
https://companion.home-assistant.io/
Apache License 2.0
2.15k stars 610 forks source link

Incorrect notification group handling #4453

Open miguelangel-nubla opened 1 month ago

miguelangel-nubla commented 1 month ago

The problem

Notifications to Android phone using a group in service notify.mobile_app_* gives unexpected behavior. Running these two services sequentially:

service: notify.mobile_app_phone
data:
  message: Test message
  data:
    tag: tag1
    group: group1
service: notify.mobile_app_phone
data:
  message: Test message
  data:
    tag: tag1
    group: group2

results in: Screenshot 2024-06-10 012136

Since they are using the same tag, I expected either the tag to be unique globally (so only last message is displayed) or two complete notifications, one per group, instead of one missing the message.

Also note the "last" notification at the top does not display group.

What version of Home Assistant Core has the issue?

core-2024.6.1

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

mobile_app

Link to integration documentation on our website

https://www.home-assistant.io/integrations/mobile_app

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 1 month ago

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

Code owner commands Code owners of `mobile_app` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign mobile_app` Removes the current integration label and assignees on the issue, 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 issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


mobile_app documentation mobile_app source (message by IssueLinks)

dshokouhi commented 1 month ago

I think your issue here is that you have 2 separate groups and one tag, tags will always replace each other. You are probably causing a weird bug to appear. What happens if you give the same group name or unique tags per group? Also this should be a bug with the android repo and not HA core :)

miguelangel-nubla commented 1 month ago

What happens if you give the same group name or unique tags per group?

Then it works fine. To me seems that the notification is handled correctly (deleted from one group and created in another) but the previous group "header" is kept, instead of checking if group empty and cleared automatically.

Also this should be a bug with the android repo and not HA core :)

Sorry about that, my bad. Should I close this one and open there?

dshokouhi commented 1 month ago

Then it works fine. To me seems that the notification is handled correctly (deleted from one group and created in another) but the previous group "header" is kept, instead of checking if group empty and cleared automatically.

the app wont be able to automatically clean that up based on the provided YAML. As far as the app is concerned we create each notification with how the user defines it. We dont want to prevent users from creating notifications but you should be testing them to make sure they work as you expect. Different devices also do different things with how they display notifications. I suppose in this case we can consider leaving a note in teh docs about this to use the same group with the same tag.

Sorry about that, my bad. Should I close this one and open there?

we can leave it for now thats fine