Using FCM notifications with data payloads leads to potentially confusing results

bolds07 commented 4 years ago

[READ] Step 1: Are you in the right place?

Yes, your people might consider it is a feature request but to me this is clearly a bug due a bad architecture.

[REQUIRED] Step 2: Describe your environment

Android Studio version: any
Firebase Component: FCM
Component version: any

[REQUIRED] Step 3: Describe the problem

Steps to reproduce:

When using FCM to push notifications to android devices, developpers need to deal with a 6 posible scenarios described in this table https://firebase.google.com/docs/cloud-messaging/android/receive#handling_messages

the simple fact of having a 6 scenario table for something simple like "push notification" should alread trig an 'alert' in any architect mind, but the thing gets worse when we think about the messages with data. they might be delivered to onMessageReceived and might be to the launcher intent.

so it automatically creates a scenario where the code will be duplicated, two classes will need to deal with same business logic. but this is not the end

follow my logic: 1- app is in background 2- fcm pushes a data+notification message 3- user sees the notification in system tray... and simple ignores it, or even has that kind of notification set to NEVER SHOW on android settings

following this scenario the app WILL NEVER RECEIVE THE DATA FROM THAT NOTIFICATION. since the launcher intent from the notification tray will never be clicked

i dont know what kind of android restrictions lead the engineers decide for this such frankstein behavior but at first glance it could be easily fix

if the android-fcm SDK is able to send the data messages to onMessageReceived even during background time... why the mixed data+notification could not send the data part also to onMessageReceived?

imho everything could be sent to that method and this class could implement a very simple method allowing any developper to foward the notifications to system tray from the onMessageReceived method

this would avoid so much code replication and create a less franksteins behavior

google-oss-bot commented 4 years ago

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

carlonzo commented 4 years ago

Hi, I would like to add a few points to your post:

reading a post with the title "TERRIBLE ARCHITECTURE" is totally starting in the wrong way, accusing the engineers who are supposed to come here and help you to have taken terrible architecture decisions.
if I was one of them I would have stop reading at the title and close the issue. but I was in the mood of reading something to make a good laugh, so I kept going.
you start the post saying there is a "bug" due to bad architecture. sadly there is no bug described.
you complain that there are 6 (6!!) differen scenarios to handle, referring to a table where it shows you 2 scenarios (background and foreground) describing what happens when a notification message is received.
FYI https://firebase.google.com/docs/cloud-messaging/concept-options#notifications_and_data_messages notification messages are "display messages" carrying an in intent to open the application. bringing the data (if present) within the intent makes perfect sense to me instead.
if you want to only receive data from the push, dont opt for a display message, but for a data message. if you read the docs they can explain to you what they are, and that they are all being handle by a single place.
be nice with people, please.

bolds07 commented 4 years ago

are you on firebase or google team? if i was wrong for stating a terrible designed architecture what to say about you sticking the finger somewhere YOU ARENT ADDING NOTHING TO THE DISCUSSION.

everybody makes mistakes and googles for sures lots of them... this isnt the first stupid architecture decision i find in android and wont be the last

https://github.com/firebase/firebase-admin-java/issues/137 https://github.com/firebase/firebase-android-sdk/issues/1418 https://github.com/firebase/firebase-android-sdk/issues/1791

this to point only the ones that are in my inbox this week

FYI https://firebase.google.com/docs/cloud-messaging/concept-options#notifications_and_data_messages notification messages are "display messages" carrying an in intent to open the application. bringing the data (if present) within the intent makes perfect sense to me instead

if the user decides to ignore the notification... if the user has set the notification mute forever in android settings or instead clicking the notification decides to click at the launcher icon

THE DATA ATTACHED TO DATA INTENT WILL BE LOST FOREVER

if this isnt a bug or a terrible design idea i want to tore up my diploma

the current architecture scenario makes basically useless the "notification+data" message. so instead server must always send 2 messages to assure both parts (data and notification) will be delivered and processed at best effort.

ashwinraghav commented 4 years ago

@bolds07 Thanks for the feedback. I recognize that this is confusing. I'll try to explain how we got here.

When using FCM to push notifications to android devices, developpers need to deal with a 6 posible scenarios described in this table

Can you share how you handle these scenarios so we can understand what the complexities better ?

so it automatically creates a scenario where the code will be duplicated, two classes will need to deal with same business logic.

Fair. Have you considered abstracting these into a form that can be shared between these code paths ?

following this scenario the app WILL NEVER RECEIVE THE DATA FROM THAT NOTIFICATION. since the launcher intent from the notification tray will never be clicked

I'm not sure what motivates this decision. @ciarand may be able to help here.

imho everything could be sent to that method and this class could implement a very simple method allowing any developper to foward the notifications to system tray from the onMessageReceived method

There's a design goal for this use case to be automatically handled by the SDK. https://firebase.google.com/docs/cloud-messaging/concept-options#notifications_and_data_messages. This allows most developers to onboard trivially. We'll consider allowing an override with a default in the future.

bolds07 commented 4 years ago

from your link:

Notification messages, sometimes thought of as "display messages." These are handled by the FCM SDK automatically. this is not true, these messages will be handled automatically only if app is in background. if app is foreground onMessageReceived will be called and i myself need to implement how i want to display the message.

understand me, if you think of notification only as display messages, there is no problem at all. the design is bad, but is possible to handle it.

if you think of push data notification the problem starts. 1- it need to be handled either in onMessageReceived and onCreate of launcher activity 2- there is ABSOLUTELY NO GARANTEE DATA WILL BE DELIVERED because of the reasons i mentioned before

so FCM cannot be considered a reliable service to delived data to clients, since the deliver rely on A- client do not block notification in android setting, B- client chooses to click on the notification message instead of anything else

i cant count how many times i got my users to write me saying: "i received a notification but accidentally i clicked somewhere else and when i opened the app to read it wasnt there anymore"

ciarand commented 4 years ago

Hi @bolds07,

The onMessageReceived logic is a little confusing at the moment. I think for the use case you're describing, you'd be better served by just using data messages and deciding when and how to show a notification in your app code.

This is really good feedback though, and we've been looking into ways that we can clarify the relationship and semantics behind data vs notification messages. Hopefully we'll have something to share soon.

bolds07 commented 4 years ago

you'd be better served by just using data messages

i think you got my point. what i currently do is only use data messages and include 2 text fields to use as display notification when is needed

the only con of my way is that the whole firebase dashboard is designed the other way around so manage and send notifications gets less friendly

samtstern commented 4 years ago

@bolds07 as I mentioned in https://github.com/firebase/firebase-android-sdk/issues/1818 it's not acceptable to abuse anyone in the Firebase community (Googlers or other developers like @carlonzo) on GitHub. We're all here to make Firebase better but that has to start from a place of respect.

firebase / firebase-android-sdk