Skip to content

Commit 3910548

Browse files
jkasten2jkasten2
committed
Notification display issues with in-app alerts and groups
* Fixed issue when pressing 'ok' on an in-app alert dialog would display a notification - Only happened if there was 2 or more notifications part of the same group key that haven't been interacted with yet. * Fixed issue with multiple summary notifications showing for the same group key in the shade. - Issue would occur if the first notification of the group was an in-app alert and user did not close it. - And the following notifications were shown in the shade when the app is out of focus.
1 parent 307b8c0 commit 3910548

File tree

3 files changed

+169
-54
lines changed

3 files changed

+169
-54
lines changed

‎OneSignalSDK/onesignal/src/main/java/com/onesignal/GenerateNotification.java

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ static void createSummaryNotification(NotificationGenerationJob notifJob, OneSig
392392
PendingIntent summaryDeleteIntent = getNewActionPendingIntent(random.nextInt(), getNewBaseDeleteIntent(0).putExtra("summary", group));
393393

394394
Notification summaryNotification;
395-
int summaryNotificationId = random.nextInt();
395+
Integer summaryNotificationId = null;
396396

397397
String firstFullData = null;
398398
Collection<SpannableString> summaryList = null;
@@ -470,7 +470,12 @@ static void createSummaryNotification(NotificationGenerationJob notifJob, OneSig
470470
cursor.close();
471471
}
472472

473-
PendingIntent summaryContentIntent = getNewActionPendingIntent(random.nextInt(),createBaseSummaryIntent(summaryNotificationId, gcmBundle, group));
473+
if (summaryNotificationId == null) {
474+
summaryNotificationId = random.nextInt();
475+
createSummaryIdDatabaseEntry(dbHelper, group, summaryNotificationId);
476+
}
477+
478+
PendingIntent summaryContentIntent = getNewActionPendingIntent(random.nextInt(), createBaseSummaryIntent(summaryNotificationId, gcmBundle, group));
474479

475480

476481
// 2 or more notifications with a group received, group them together as a single notification.
@@ -531,33 +536,14 @@ static void createSummaryNotification(NotificationGenerationJob notifJob, OneSig
531536
summaryNotification = summaryBuilder.build();
532537
}
533538
else {
534-
// There currently isn't a visible notification from for this groupid.
535-
// save the group summary notification id and post it so it looks like a normal notification.
536-
SQLiteDatabase writableDb = null;
537-
try {
538-
writableDb = dbHelper.getWritableDbWithRetries();
539-
writableDb.beginTransaction();
540-
541-
ContentValues values = new ContentValues();
542-
values.put(NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID, summaryNotificationId);
543-
values.put(NotificationTable.COLUMN_NAME_GROUP_ID, group);
544-
values.put(NotificationTable.COLUMN_NAME_IS_SUMMARY, 1);
545-
writableDb.insertOrThrow(NotificationTable.TABLE_NAME, null, values);
546-
writableDb.setTransactionSuccessful();
547-
} catch (Throwable t) {
548-
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error adding summary notification record! ", t);
549-
} finally {
550-
if (writableDb != null)
551-
writableDb.endTransaction();
552-
}
553-
539+
// First notification with this group key, post like a normal notification.
554540
NotificationCompat.Builder summaryBuilder = notifBuilder.compatBuilder;
555541

556542
// We are re-using the notifBuilder from the normal notification so if a developer as an
557543
// extender setup all the settings will carry over.
558544
// Note: However their buttons will not carry over as we need to be setup with this new summaryNotificationId.
559545
addNotificationActionButtons(gcmBundle, summaryBuilder, summaryNotificationId, group);
560-
546+
561547
summaryBuilder.setContentIntent(summaryContentIntent)
562548
.setDeleteIntent(summaryDeleteIntent)
563549
.setOnlyAlertOnce(updateSummary)
@@ -574,6 +560,28 @@ static void createSummaryNotification(NotificationGenerationJob notifJob, OneSig
574560
private static Intent createBaseSummaryIntent(int summaryNotificationId, JSONObject gcmBundle, String group) {
575561
return getNewBaseIntent(summaryNotificationId).putExtra("onesignal_data", gcmBundle.toString()).putExtra("summary", group);
576562
}
563+
564+
private static void createSummaryIdDatabaseEntry(OneSignalDbHelper dbHelper, String group, int id) {
565+
// There currently isn't a visible notification from for this groupid.
566+
// Save the group summary notification id so it can be updated later.
567+
SQLiteDatabase writableDb = null;
568+
try {
569+
writableDb = dbHelper.getWritableDbWithRetries();
570+
writableDb.beginTransaction();
571+
572+
ContentValues values = new ContentValues();
573+
values.put(NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID, id);
574+
values.put(NotificationTable.COLUMN_NAME_GROUP_ID, group);
575+
values.put(NotificationTable.COLUMN_NAME_IS_SUMMARY, 1);
576+
writableDb.insertOrThrow(NotificationTable.TABLE_NAME, null, values);
577+
writableDb.setTransactionSuccessful();
578+
} catch (Throwable t) {
579+
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error adding summary notification record! ", t);
580+
} finally {
581+
if (writableDb != null)
582+
writableDb.endTransaction();
583+
}
584+
}
577585

578586
// Keep 'throws Throwable' as 'onesignal_bgimage_notif_layout' may not be available
579587
// This maybe the case if a jar is used instead of an aar.

‎OneSignalSDK/onesignal/src/main/java/com/onesignal/NotificationSummaryManager.java

Lines changed: 70 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import android.app.NotificationManager;
44
import android.content.ContentValues;
55
import android.content.Context;
6+
import android.content.Intent;
67
import android.database.Cursor;
78
import android.database.sqlite.SQLiteDatabase;
89

@@ -34,64 +35,66 @@ static void updatePossibleDependentSummaryOnDismiss(Context context, SQLiteDatab
3435

3536
// Called from an opened / dismissed / cancel event of a single notification to update it's parent the summary notification.
3637
static void updateSummaryNotificationAfterChildRemoved(Context context, SQLiteDatabase writableDb, String group, boolean dismissed) {
38+
Cursor cursor = null;
39+
try {
40+
cursor = internalUpdateSummaryNotificationAfterChildRemoved(context, writableDb, group, dismissed);
41+
} catch (Throwable t) {
42+
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error running updateSummaryNotificationAfterChildRemoved!", t);
43+
} finally {
44+
if (cursor != null && !cursor.isClosed())
45+
cursor.close();
46+
}
47+
}
48+
49+
private static Cursor internalUpdateSummaryNotificationAfterChildRemoved(Context context, SQLiteDatabase writableDb, String group, boolean dismissed) {
3750
Cursor cursor = writableDb.query(
3851
NotificationTable.TABLE_NAME,
3952
new String[] { NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID, // return columns
40-
NotificationTable.COLUMN_NAME_CREATED_TIME },
53+
NotificationTable.COLUMN_NAME_CREATED_TIME },
4154
NotificationTable.COLUMN_NAME_GROUP_ID + " = ? AND " + // Where String
4255
NotificationTable.COLUMN_NAME_DISMISSED + " = 0 AND " +
4356
NotificationTable.COLUMN_NAME_OPENED + " = 0 AND " +
4457
NotificationTable.COLUMN_NAME_IS_SUMMARY + " = 0" ,
4558
new String[] { group }, // whereArgs
4659
null, null,
4760
NotificationTable._ID + " DESC"); // sort order, new to old);
48-
61+
4962
int notifsInGroup = cursor.getCount();
5063

5164
// If all individual notifications consumed
5265
// - Remove summary notification from the shade.
5366
// - Mark summary notification as consumed.
5467
if (notifsInGroup == 0) {
5568
cursor.close();
56-
57-
// Get the Android Notification ID of the summary notification
58-
cursor = writableDb.query(
59-
NotificationTable.TABLE_NAME,
60-
new String[] { NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID }, // retColumn
61-
NotificationTable.COLUMN_NAME_GROUP_ID + " = ? AND " + // Where String
62-
NotificationTable.COLUMN_NAME_DISMISSED + " = 0 AND " +
63-
NotificationTable.COLUMN_NAME_OPENED + " = 0 AND " +
64-
NotificationTable.COLUMN_NAME_IS_SUMMARY + " = 1" ,
65-
new String[] { group }, // whereArgs
66-
null, null, null);
67-
68-
boolean hasRecord = cursor.moveToFirst();
69-
if (!hasRecord)
70-
return;
71-
int androidNotifId = cursor.getInt(cursor.getColumnIndex(NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID));
72-
cursor.close();
73-
69+
70+
Integer androidNotifId = getSummaryNotificationId(writableDb, group);
71+
if (androidNotifId == null)
72+
return cursor;
73+
7474
// Remove the summary notification from the shade.
7575
NotificationManager notificationManager = (NotificationManager)context.getSystemService(Context.NOTIFICATION_SERVICE);
7676
notificationManager.cancel(androidNotifId);
77-
77+
7878
// Mark the summary notification as opened or dismissed.
7979
ContentValues values = new ContentValues();
8080
values.put(dismissed ? NotificationTable.COLUMN_NAME_DISMISSED : NotificationTable.COLUMN_NAME_OPENED, 1);
8181
writableDb.update(NotificationTable.TABLE_NAME,
82-
values,
83-
NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID + " = " + androidNotifId,
84-
null);
85-
return;
82+
values,
83+
NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID + " = " + androidNotifId,
84+
null);
85+
return cursor;
8686
}
87-
87+
8888
// Only a single notification now in the group
8989
// - Need to recreate a summary notification so it looks like a normal notifications since we
9090
// only have one notification now.
9191
if (notifsInGroup == 1) {
9292
cursor.close();
93+
Integer androidNotifId = getSummaryNotificationId(writableDb, group);
94+
if (androidNotifId == null)
95+
return cursor;
9396
restoreSummary(context, group);
94-
return;
97+
return cursor;
9598
}
9699

97100
// 2 or more still left in the group
@@ -102,17 +105,23 @@ static void updateSummaryNotificationAfterChildRemoved(Context context, SQLiteDa
102105
cursor.moveToFirst();
103106
Long datetime = cursor.getLong(cursor.getColumnIndex(NotificationTable.COLUMN_NAME_CREATED_TIME));
104107
cursor.close();
108+
109+
Integer androidNotifId = getSummaryNotificationId(writableDb, group);
110+
if (androidNotifId == null)
111+
return cursor;
105112

106113
NotificationGenerationJob notifJob = new NotificationGenerationJob(context);
107114
notifJob.restoring = true;
108115
notifJob.shownTimeStamp = datetime;
109-
116+
110117
JSONObject payload = new JSONObject();
111118
payload.put("grp", group);
112119
notifJob.jsonPayload = payload;
113-
120+
114121
GenerateNotification.updateSummaryNotification(notifJob);
115122
} catch (JSONException e) {}
123+
124+
return cursor;
116125
}
117126

118127
private static void restoreSummary(Context context, String group) {
@@ -145,4 +154,36 @@ private static void restoreSummary(Context context, String group) {
145154
cursor.close();
146155
}
147156
}
157+
158+
private static Integer getSummaryNotificationId(SQLiteDatabase writableDb, String group) {
159+
Integer androidNotifId = null;
160+
Cursor cursor = null;
161+
try {
162+
// Get the Android Notification ID of the summary notification
163+
cursor = writableDb.query(
164+
NotificationTable.TABLE_NAME,
165+
new String[] { NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID }, // retColumn
166+
NotificationTable.COLUMN_NAME_GROUP_ID + " = ? AND " + // Where String
167+
NotificationTable.COLUMN_NAME_DISMISSED + " = 0 AND " +
168+
NotificationTable.COLUMN_NAME_OPENED + " = 0 AND " +
169+
NotificationTable.COLUMN_NAME_IS_SUMMARY + " = 1" ,
170+
new String[] { group }, // whereArgs
171+
null, null, null);
172+
173+
boolean hasRecord = cursor.moveToFirst();
174+
if (!hasRecord) {
175+
cursor.close();
176+
return null;
177+
}
178+
androidNotifId = cursor.getInt(cursor.getColumnIndex(NotificationTable.COLUMN_NAME_ANDROID_NOTIFICATION_ID));
179+
cursor.close();
180+
} catch (Throwable t) {
181+
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error getting android notification id for summary notification group: " + group, t);
182+
} finally {
183+
if (cursor != null && !cursor.isClosed())
184+
cursor.close();
185+
}
186+
187+
return androidNotifId;
188+
}
148189
}

‎OneSignalSDK/unittest/src/test/java/com/test/onesignal/GenerateNotificationRunner.java

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@
7676
import org.robolectric.annotation.Config;
7777
import org.robolectric.shadows.ShadowLog;
7878
import org.robolectric.shadows.ShadowSystemClock;
79+
import org.robolectric.util.ActivityController;
7980
import org.robolectric.util.ServiceController;
8081

8182
import java.util.Iterator;
@@ -101,6 +102,7 @@
101102
public class GenerateNotificationRunner {
102103

103104
private Activity blankActivity;
105+
private static ActivityController<BlankActivity> blankActivityController;
104106

105107
private static final String notifMessage = "Robo test message";
106108

@@ -114,8 +116,9 @@ public static void setUpClass() throws Exception {
114116
public void beforeEachTest() throws Exception {
115117
// Robolectric mocks System.currentTimeMillis() to 0, we need the current real time to match our SQL records.
116118
ShadowSystemClock.setCurrentTimeMillis(System.currentTimeMillis());
117-
118-
blankActivity = Robolectric.buildActivity(BlankActivity.class).create().get();
119+
120+
blankActivityController = Robolectric.buildActivity(BlankActivity.class).create();
121+
blankActivity = blankActivityController.get();
119122
blankActivity.getApplicationInfo().name = "UnitTestApp";
120123

121124
overrideNotificationId = -1;
@@ -532,6 +535,69 @@ public void shouldHandleOpeningInAppAlertWithGroupKeySet() throws Exception {
532535
SQLiteDatabase writableDb = OneSignalDbHelper.getInstance(RuntimeEnvironment.application).getWritableDatabase();
533536
NotificationSummaryManager_updateSummaryNotificationAfterChildRemoved(blankActivity, writableDb, "some_group", false);
534537
}
538+
539+
@Test
540+
public void shouldNotDisplaySummaryWhenDismissingAnInAppAlertIfOneDidntAlreadyExist() throws Exception {
541+
// Setup - init
542+
OneSignal.setInFocusDisplaying(OneSignal.OSInFocusDisplayOption.InAppAlert);
543+
OneSignal.init(blankActivity, "123456789", "b2f7f966-d8cc-11e4-bed1-df8f05be55ba");
544+
threadAndTaskWait();
545+
546+
// Setup1 - Display a notification with a group set
547+
Bundle bundle = getBaseNotifBundle("UUID1");
548+
bundle.putString("grp", "test1");
549+
NotificationBundleProcessor_ProcessFromGCMIntentService(blankActivity, bundle, null);
550+
551+
// Test1 - Manually trigger a refresh on grouped notification.
552+
SQLiteDatabase writableDb = OneSignalDbHelper.getInstance(RuntimeEnvironment.application).getWritableDatabase();
553+
NotificationSummaryManager_updateSummaryNotificationAfterChildRemoved(blankActivity, writableDb, "test1", false);
554+
Assert.assertEquals(0, ShadowRoboNotificationManager.notifications.size());
555+
556+
557+
// Setup2 - Display a 2nd notification with the same group key
558+
bundle = getBaseNotifBundle("UUID2");
559+
bundle.putString("grp", "test1");
560+
NotificationBundleProcessor_ProcessFromGCMIntentService(blankActivity, bundle, null);
561+
562+
// Test2 - Manually trigger a refresh on grouped notification.
563+
writableDb = OneSignalDbHelper.getInstance(RuntimeEnvironment.application).getWritableDatabase();
564+
NotificationSummaryManager_updateSummaryNotificationAfterChildRemoved(blankActivity, writableDb, "test1", false);
565+
Assert.assertEquals(0, ShadowRoboNotificationManager.notifications.size());
566+
}
567+
568+
569+
@Test
570+
public void shouldCorrectlyDisplaySummaryWithMixedInAppAlertsAndNotifications() throws Exception {
571+
// Setup - init
572+
OneSignal.setInFocusDisplaying(OneSignal.OSInFocusDisplayOption.InAppAlert);
573+
OneSignal.init(blankActivity, "123456789", "b2f7f966-d8cc-11e4-bed1-df8f05be55ba");
574+
threadAndTaskWait();
575+
576+
// Setup - Display a notification with a group set
577+
Bundle bundle = getBaseNotifBundle("UUID1");
578+
bundle.putString("grp", "test1");
579+
NotificationBundleProcessor_ProcessFromGCMIntentService(blankActivity, bundle, null);
580+
581+
Assert.assertEquals(0, ShadowRoboNotificationManager.notifications.size());
582+
583+
// Setup - Background app
584+
blankActivityController.pause();
585+
threadAndTaskWait();
586+
587+
// Setup - Send 2 more notifications with the same group
588+
bundle = getBaseNotifBundle("UUID2");
589+
bundle.putString("grp", "test1");
590+
NotificationBundleProcessor_ProcessFromGCMIntentService(blankActivity, bundle, null);
591+
bundle = getBaseNotifBundle("UUID3");
592+
bundle.putString("grp", "test1");
593+
NotificationBundleProcessor_ProcessFromGCMIntentService(blankActivity, bundle, null);
594+
595+
// Test - equals 3 - Should be 2 notifications + 1 summary.
596+
// Alert should stay as an in-app alert.
597+
Assert.assertEquals(3, ShadowRoboNotificationManager.notifications.size());
598+
}
599+
600+
535601

536602
@Test
537603
public void shouldSetButtonsCorrectly() throws Exception {

0 commit comments

Comments
 (0)