Fix notification disabled by channel being restored

[Jeasmine]

Fix notification disabled by channel being restored

• If the user disables notifications while the application is closed or in the background, then gets notifications, this notification won't display. If the user then enables notification channel previously disable, then open the application, notifications were being displayed because of restoring
• Save as dismissed a notification disable by channel

Fix notification received

• Mark as received all notification even if they are silent

This fix solves #1265

Testing steps used:

Started the app so user is subscribed.
Disabled notification channel in system settings
Sent notification message to disabled channel
Enabled notification channel
Killed the app
Started the app
OneSignal SDK restored notification which was delivered when the channel was disabled

Solution

Check if the notification channel id is disabled, if so, save notification as dismissed to avoid being restore

This change is 

[jkasten2]

This test is failing on CI that could be related
https://travis-ci.com/github/OneSignal/OneSignal-Android-SDK/builds/219967374#L3672-L3678

com.test.onesignal.GenerateNotificationRunner > shouldHandleBasicNotifications FAILED
    junit.framework.AssertionFailedError
        at junit.framework.Assert.fail(Assert.java:55)
        at junit.framework.Assert.assertTrue(Assert.java:22)
        at junit.framework.Assert.assertTrue(Assert.java:31)
        at com.test.onesignal.GenerateNotificationRunner.shouldHandleBasicNotifications(GenerateNotificationRunner.java:842)

Reviewable status: 0 of 5 files reviewed, all discussions resolved

[jkasten2]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Jeasmine)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 331 at r1 (raw file):

      if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)
         return OneSignalNotificationManager.notificationChannelEnabled(currentContext, notification.getChannelId());

It would be better if we put this at the top of the method to early return. No point in it trying to display notification if the channel is off were it will never be shown.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 331 at r1 (raw file):

      if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O)
         return OneSignalNotificationManager.notificationChannelEnabled(currentContext, notification.getChannelId());

It doesn't look like we are account for notifications being completely disabled either. This can be done in this PR or a follow up, just wanted to make sure we don't forget.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java, line 147 at r1 (raw file):

            } else {
                // Notification might end not displaying because the channel for that notification has notification disable
                notificationDisabledByChannel = !GenerateNotification.displayNotificationFromJsonPayload(notificationJob);

notificationDisabledByChannel seems overly specific, this code only needs to know if it was displayed or not.
Suggested name notificationDisplayed, that way we don't deal with the ! here either.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java, line 181 at r1 (raw file):

        String notificationId = notificationJob.getApiNotificationId();
        OSReceiveReceiptController.getInstance().sendReceiveReceipt(notificationId);

We actually only want to send received receipts if it was displayed. It is really a confirmed display so we should keep this at the bottom where it was. Could also add comment so this is caught need time. Or even better a unit test.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalNotificationManager.java, line 169 at r1 (raw file):

    static boolean notificationChannelEnabled(Context context, String channelId) {
        boolean isEnabled = true;
        if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.O) {

nit, might be a bit easier to read as an early return. I'll let you decide though.

[Jeasmine]

shouldHandleBasicNotifications
This is still a flaky test 😞

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jkasten2)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 331 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

It would be better if we put this at the top of the method to early return. No point in it trying to display notification if the channel is off were it will never be shown.

but the channel is being set on the notification on different methods

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 331 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

It doesn't look like we are account for notifications being completely disabled either. This can be done in this PR or a follow up, just wanted to make sure we don't forget.

gotcha

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java, line 147 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

notificationDisabledByChannel seems overly specific, this code only needs to know if it was displayed or not.
Suggested name notificationDisplayed, that way we don't deal with the ! here either.

Done.

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationBundleProcessor.java, line 181 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

We actually only want to send received receipts if it was displayed. It is really a confirmed display so we should keep this at the bottom where it was. Could also add comment so this is caught need time. Or even better a unit test.

gotcha

[jkasten2]

Reviewed 6 of 6 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)

---

OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java, line 331 at r1 (raw file):

Previously, Jeasmine (JNahui) wrote…

but the channel is being set on notification on different methods

Thinking about this again, I don't think we can move anyway. As then applyNotificationExtender would not fire if the user turned off the notification channel.
So the way you have it here is good.

[jkasten2]

Reviewed 5 of 5 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)