Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using of additional notification keys requires duplication of 'Message' & 'Title' service data #349

Closed
mutasim opened this issue Feb 9, 2020 · 12 comments · Fixed by #356

Comments

@mutasim
Copy link

mutasim commented Feb 9, 2020

Home Assistant Android version: 1.6.0
Android version: 10
Phone model: Pixel 2XL
Home Assistant version: 0.105.2

Description of problem:
When using notification keys such as 'tag' or 'color' (I suspect this was never formally support yet..) the 'Message' & 'Title' keys aren't being passed through and thus need to be replaced in the data > android > notification part of the payload.

Previously Working

message: 'Body Text'
title: 'Title Text'
data:
  android:
    notification:
      color: '#e8e8e8'
      tag: 'my_notification'

Current Behaviour

Notification sends, but title & message are missing. I believe in using these keys within data > android > notification, it is now also looking for 'body' & 'title' keys within to provide the message and title.

Current workaround

message: 'WORKAROUND'
title: 'WORAROUND'
data:
  android:
    notification:
      color: '#e8e8e8'
      tag: 'my_notification'
      body: 'Body Text'
      title: 'Title Text'

(Note that some dummy text needs to be inputted into the 'message' & 'title' keys as the service won't work without them). Using this workaround will continue to allow support for using of Notification Tags which are a useful feature.

@TazUk
Copy link

TazUk commented Feb 9, 2020

Confirming this issue.
The app update breaks all my existing notifications that push an image.

Home Assistant Android version: 1.6.0
Android version: 10
Phone model: Pixel 3
Home Assistant version: 0.105.2 (also 0.104.3)

Description of problem:
Existing notifications with images no longer display title or message text after the app update, only the image. Text only notifications without additional data keys are unaffected.

Previously Working

title: 'Title Text'
message: 'Body Text'
data:
  android:
    notification:
      image: 'url'

Current Behaviour
Notification sends, title and message are absent and only image is displayed.
Notifications without images are unaffected.

Current workaround

title: 'Title Text'
message: 'Body Text'
data:
  android:
    notification:
      title: 'Title Text'
      body: 'Body Text'
      image: 'url'

@JBassett
Copy link
Collaborator

JBassett commented Feb 9, 2020

Does that workaround work when the application is open or only when it's closed?

@mutasim
Copy link
Author

mutasim commented Feb 9, 2020

Just tested it - works when the app is open and closed.

@JBassett
Copy link
Collaborator

JBassett commented Feb 9, 2020

So after digging into this for a bit I think we need to, for now, restrict what can be sent via push. We should only allow what we truly support. Because of the data you are sending you are bypassing our on device handling of the message. This is going to cause issues and confusion when you go to implement actionable notifications in the future. These are the options that we support currently:

title: 'Title Text'
message: 'Body Text'
data:
  actions:
    - action: test
      title: Click Me
  data:
    image: 'imageUrl'

The only required field is message, everything else is optional.

@mutasim
Copy link
Author

mutasim commented Feb 9, 2020

For what it's worth, I was able to setup actionable notifications successfully even with the workaround. Didn't do any robust testing around it, just a POC.

Make sense to limit to only what's supported though.

@andriej
Copy link

andriej commented Feb 9, 2020

It would be nice then to allow other parameters to be sent/used, like tag.
Or colours.

@TazUk
Copy link

TazUk commented Feb 9, 2020

@JBassett
Thanks for clarifying this - it looks like I trod the wrong path when first setting up my notifications. I've fixed my image-based notifications to follow the supported path.

@mutasim
Copy link
Author

mutasim commented Feb 9, 2020

It would be nice then to allow other parameters to be sent/used, like tag.
Or colours.

Sticky is another useful one - great for allowing a notification to stay after being clicked (swipe to dismiss)

@JBassett
Copy link
Collaborator

JBassett commented Feb 9, 2020

It's actually my fault, I should have put together better documentation on what the scheme for sending notifications is. I have a PR on the server side to restrict to known data fields the application can handle: home-assistant/mobile-apps-fcm-push#5
This is what we will handle after the PR is merged:

title: 'title'
message: 'message' # REQUIRED
data:
  actions: # Up to 3 actions
    - title: 'Click Me'
      action: 'clicked'
  image: 'url' # Must be publicly facing for now
  ttl: 0
  priority: high

Obviously this will reduce some functionality for now. If you have a need for additional notification functionality then we should add proper support (enhancement request) for it rather than rely on the system to handle it, because it can be inconsistent.

@andriej
Copy link

andriej commented Feb 9, 2020

Half of my notification-based actions rely on tag right now and I've moved those to app.
Is there any timeline on new features I can look for?

@acordill
Copy link

acordill commented Feb 9, 2020

+1 for sticky, color, and tag

@leranp
Copy link

leranp commented Feb 9, 2020

What about sending with icon? This is very important add-on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants