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

Notifications omitted in OSRemoteNotificationReceivedHandler and from disabled channels are restored and shown as silent notifications. #1265

Closed
krzysztof-drobek opened this issue Jan 26, 2021 · 10 comments
Assignees
Labels
Medium Priority WIP Work In Progress

Comments

@krzysztof-drobek
Copy link

krzysztof-drobek commented Jan 26, 2021

Description:
Notifications omitted in OSRemoteNotificationReceivedHandler and from disabled channels are restored and shown as silent notifications.

Environment
OneSignal SDK: 4.1.0
Android version: 11.0

Steps to Reproduce Issue:
Omitted notifications:

  1. Implement OSRemoteNotificationReceivedHandler
  2. Omit notification with notificationReceivedEvent?.complete(null) method
  3. Send push notification
  4. Notification is not shown (correct behaviour)
  5. Change omit logic to show all notifications
  6. Kill the app
  7. Start the app
  8. Previously omitted notifications are restored and shown

Channels:

  1. Disable notification channel from system app settings
  2. Kill the app
  3. Send push notification
  4. Start the app
  5. Notifications from disabled channels are shown

Expectations
Omitted notifications are not restored upon app restart
Notifications from disabled channels are not restored upon app restart

@krzysztof-drobek krzysztof-drobek changed the title Notifications from disabled channels are restored and shown as silent notifications Notifications omitted in OSRemoteNotificationReceivedHandler and from disabled channels are restored and shown as silent notifications. Jan 28, 2021
@jfishman1
Copy link
Contributor

Hi there

For this step:

Omit notification with notificationReceivedEvent?.complete(null) method
The notification should still be in the notification center, even if you don't show it while the app is open.

What you are showing is expected behavior, if you do not want to have the message show upon re-opening the app, you can set a TTL

Notifications past the TTL will not show again.

​Is there a specific use case you don't want them to restore at all?

@krzysztof-drobek
Copy link
Author

krzysztof-drobek commented Jan 30, 2021

The notification should still be in the notification center, even if you don't show it while the app is open.

Then what for is this method if not for omitting notifications? It's just delaying the delivery.

What you are showing is expected behavior, if you do not want to have the message show upon re-opening the app, you can set a TTL

TTL is not meant to be used like that and it will not solve this problem.

I don't want to restore notification if:

  • user disabled notification channel for this type of notifications (obvious bug)
  • for whatever business logic I was not interested in showing this notification to user at the time.
    For example if user is in the chat window, I don't want to show him notification at all (even silent one). Once user leaves chat window he should receive new notifications. This is not possible with notificationReceivedEvent?.complete(null) method because omitted notification will come back after restarting the app.
  • user disabled notifications with system settings and turned them back on later. Right now all notifications received during disabled time are restored.

Is there any method in this SDK that will ignore notification completely? I don't want to show it now or never.

@Jeasmine Jeasmine self-assigned this Feb 9, 2021
@Jeasmine Jeasmine added the Possible Bug Maintainers need to confirm or reproduce label Feb 9, 2021
@Jeasmine
Copy link
Contributor

Hey @krzysztof-drobek, calling notificationReceivedEvent?.complete(null) for the received notification should not restore it and show it after starting the app again. I tried to reproduce the bug but could not do so either on Android 10 and 11.
Could you please provide the log? When the notification arrived and when starting the app again (when the notification is being restored and show again).

Also, channel notification disable is something handle by the Android OS, not OneSignal can you provide more information on this scenario? Maybe the notification configuration you are using on the dashboard.

Thanks!

@jkasten2
Copy link
Member

@krzysztof-drobek Thanks for the details. I believe there are 3 different issues being described here. Let me know if I am summarying correctly.

Issue 1

Notification being restored after being omitted with notificationReceivedEvent?.complete(null).

Omitted notifications:

  1. Implement OSRemoteNotificationReceivedHandler
  2. Omit notification with notificationReceivedEvent?.complete(null) method
  3. Send push notification
  4. Notification is not shown (correct behaviour)
  5. Change omit logic to show all notifications
  6. Kill the app
  7. Start the app
  8. Previously omitted notifications are restored and shown

Status

It seems @Jeasmine could not reproduce this and would need more information. Could you provide the imformation requested from #1265 (comment) ?

Issue 2

Notifications being restored for a disabled Notification channel after restarting the app.

  1. Disable notification channel from system app settings
  2. Kill the app
  3. Send push notification
  4. Start the app
  5. Notifications from disabled channels are shown

Status

I don't think we tried to reproduce this one yet. It seems like this could be a possible bug with this SDK.

Issue 3

User disabled notifications with system settings and turned them back on later. Right now all notifications received during disabled time are restored.

Status

I don't think we tried to reproduce this one yet either, similar to issue 2 but the SDK will need to check a different setting.

@krzysztof-drobek
Copy link
Author

krzysztof-drobek commented Mar 11, 2021

I can't reproduce the first issue as well. It must have been conected with the other two.
The second and the third issue seems to be the same thing. Your demo app has in app messages paused on start which may block displaying all restored notifications (issue 2 and 3).

@Jeasmine
Copy link
Contributor

Sorry If it was not clear. Channel and application notification disabling was tested too, and could not reproduce the bug.

In-app message handling is separate from notification, so there should not be a conflict there.

@krzysztof-drobek
Copy link
Author

krzysztof-drobek commented Mar 11, 2021

The demo app has foreground notifications disabled with OneSignal.setNotificationWillShowInForegroundHandler which prevents restored notifications from being shown on app start, please remove it and redo the test.

I've run your demo app without OneSignal.setNotificationWillShowInForegroundHandler in MainApplication class and then did following:

  1. Started the app so user is subscribed.
  2. Disabled notification channel in system settings
  3. Sent notification message to disabled channel
  4. Enabled notification channel
  5. Killed the app
  6. Started the app
  7. OneSignal SDK restored notification which was delivered when channel was disabled
screencapture-1615481233418_HupmidMS.mp4

On a side note your demo app never uses app id stored in R.string.onesignal_app_id because SharedPreferenceUtil.getOneSignalAppId(this) has default value of 77e32082-ea27-42e3-a898-c72e141824ef. This may be confusing for anyone trying it.

@Jeasmine
Copy link
Contributor

@krzysztof-drobek Thanks for the video. That is very helpful! I will try again to reproduce it again.

@Jeasmine
Copy link
Contributor

I was able to reproduce with those steps; I see the problem. We will be working on fixing this for the next release. Thanks again!

@Jeasmine Jeasmine added Medium Priority WIP Work In Progress and removed Needs More Information Possible Bug Maintainers need to confirm or reproduce labels Mar 11, 2021
@Jeasmine
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Medium Priority WIP Work In Progress
Projects
None yet
Development

No branches or pull requests

4 participants