Skip to content

Commit

Permalink
Outcome callback cleanup (#871)
Browse files Browse the repository at this point in the history
* Changing outcome callbacks and removing cache settings
* Cache settings was removed as a decision to prevent users from turning off cache (will need to find a good way to clean cached notifications/outcomes in future though)
* Changed callback functionality so there is no failure now (failure could give users the ability to reattempt sending an outcome which we do already through caching and new init)

* Fixed example app text view for debugging
* Also cleaned unique notification id for-loops so only one is needed

* Previously changed/removed getParams
* Added getParams back and make sure it returns a String

* Added new outcome callback and cache cleaning
* Outcome callback should just return the entire OutcomeEvent instead of a name
* Also removed the failure and only allow success since we don't want people reattempting sending unique outcomes when they will be reattempted by us anyway (could result in duplicate outcome events failling and being sent later)
* Added SQL caching for the unique outcome events with notifications (ATTRIBUTED) and cleaning so they are wiped away when the cached notifications are wiped away
* Added SharedPrefs caching for UNATTRIBUTED unique outcome events that simply wipe and start over on enw sessions only

* Outcome event clean up (#873)

* Outcome event clean up
* Removed OutcomeParams class and instead only use a float weight
* Fixed all tests that broke after removing OutcomeParams
* Cleaned up requests in OutcomeEventsRepository so now there is only 1 of each session (DIRECT, INDIRECT, UNATTRIBUTED)
* Reordered expected outcome unit tests because of outcome event changes to JSON creation
* Added checking in two places for weight of 0 being sent with an OutcomeEvent, one is when we call the public method (logs an error) and the other is in the toJSON of the OutcomeEvent preventing a weight of 0 from being added into the JSON

* Minor clean up after PR
* Change equals check on weight to Float in OutcomeEvent equals override
* Add '||' check for valid outcome name and value instead of '&&'

* Updated example app for new outcomes callback logic

* Made CachedUniqueOutcomeNotification package-private
* Fixed all unit tests by adding CachedUniqueOutcomeNotification to OneSignalPackagePrivateHelper
* Made OutcomeEvent toJsonObject public for access in wrappers

* Removed TODO for clean up in OutcomeEventsRepository
  • Loading branch information
mikechoch authored Oct 22, 2019
1 parent 317107c commit 5099bcb
Show file tree
Hide file tree
Showing 25 changed files with 615 additions and 649 deletions.
12 changes: 6 additions & 6 deletions OneSignalSDK/app/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ apply plugin: 'com.android.application'
//apply plugin: com.onesignal.androidsdk.GradleProjectPlugin

android {
compileSdkVersion 27
buildToolsVersion '27.0.3'
compileSdkVersion 28
buildToolsVersion '28.0.3'
defaultConfig {
applicationId "com.onesignal.example"
manifestPlaceholders = [onesignal_app_id: "b2f7f966-d8cc-11e4-bed1-df8f05be55ba",
// Project number pulled from dashboard, local value is ignored.
onesignal_google_project_number: "REMOTE"]
minSdkVersion 15
targetSdkVersion 27
targetSdkVersion 28
versionCode 1
versionName "1.0"
}
Expand All @@ -39,7 +39,7 @@ repositories {

dependencies {
implementation fileTree(include: ['*.jar'], dir: 'libs')
implementation 'com.android.support:appcompat-v7:27.1.0'
implementation 'com.android.support:appcompat-v7:28.0.0'

// Use for SDK Development
implementation(project(':onesignal')) {
Expand All @@ -64,10 +64,10 @@ dependencies {
// Old Instructions - Use for released SDK
// compile 'com.onesignal:OneSignal:3.+@aar'

implementation 'com.google.android.gms:play-services-location:12.0.0'
implementation 'com.google.android.gms:play-services-location:16.0.0'

// For Chrome tabs
implementation 'com.android.support:customtabs:27.1.0'
implementation 'com.android.support:customtabs:28.0.0'
}

//apply plugin: 'com.google.gms.google-services'
27 changes: 6 additions & 21 deletions OneSignalSDK/app/src/main/java/com/onesignal/MainActivity.java
Original file line number Diff line number Diff line change
Expand Up @@ -330,27 +330,17 @@ public void onFailure(OneSignal.EmailUpdateError error) {
public void onSendOutcomeClicked(View view) {
OneSignal.sendOutcome(outcomeName.getText().toString(), new OneSignal.OutcomeCallback() {
@Override
public void onOutcomeSuccess(String name) {
updateTextView(name + " Outcome sent successfully");
}

@Override
public void onOutcomeFail(int statusCode, String response) {
updateTextView("Outcome fail with status code: " + statusCode);
public void onSuccess(OutcomeEvent outcomeEvent) {
updateTextView(outcomeEvent.toString());
}
});
}

public void onSendUniqueOutcomeClicked(View view) {
OneSignal.sendUniqueOutcome(outcomeUnique.getText().toString(), new OneSignal.OutcomeCallback() {
@Override
public void onOutcomeSuccess(String name) {
updateTextView(name + " Unique Outcome sent successfully");
}

@Override
public void onOutcomeFail(int statusCode, String response) {
updateTextView("Unique Outcome fail with status code: " + statusCode);
public void onSuccess(OutcomeEvent outcomeEvent) {
updateTextView(outcomeEvent.toString());
}
});
}
Expand All @@ -361,13 +351,8 @@ public void onSendOutcomeWithValueClicked(View view) {

OneSignal.sendOutcomeWithValue(outcomeValueName.getText().toString(), Float.parseFloat(outcomeValue.getText().toString()), new OneSignal.OutcomeCallback() {
@Override
public void onOutcomeSuccess(String name) {
updateTextView(name + " Outcome sent with value successfully");
}

@Override
public void onOutcomeFail(int statusCode, String response) {
updateTextView("Outcome with value fail with status code: " + statusCode);
public void onSuccess(OutcomeEvent outcomeEvent) {
updateTextView(outcomeEvent.toString());
}
});
}
Expand Down
2 changes: 1 addition & 1 deletion OneSignalSDK/app/src/main/res/layout/activity_main.xml
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@
<TextView
android:id="@+id/debugTextView"
android:layout_width="match_parent"
android:layout_height="80dp" />
android:layout_height="wrap_content" />

</LinearLayout>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package com.onesignal;

class CachedUniqueOutcomeNotification {

private String notificationId;
private String name;

CachedUniqueOutcomeNotification(String notificationId, String name) {
this.notificationId = notificationId;
this.name = name;
}

String getNotificationId() {
return notificationId;
}

String getName() {
return name;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,6 @@ private static void saveNotification(NotificationGenerationJob notifiJob, boolea

writableDb.beginTransaction();

deleteOldNotifications(writableDb);

// Count any notifications with duplicated android notification ids as dismissed.
// -1 is used to note never displayed
if (notifiJob.isNotificationToDisplay()) {
Expand Down Expand Up @@ -272,13 +270,6 @@ static void markRestoredNotificationAsDismissed(NotificationGenerationJob notifi
}
}

// Clean up old records after 1 week.
static void deleteOldNotifications(SQLiteDatabase writableDb) {
writableDb.delete(NotificationTable.TABLE_NAME,
NotificationTable.COLUMN_NAME_CREATED_TIME + " < " + ((System.currentTimeMillis() / 1_000L) - 604_800L),
null);
}

static @NonNull JSONObject bundleAsJSONObject(Bundle bundle) {
JSONObject json = new JSONObject();
Set<String> keys = bundle.keySet();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,35 +122,13 @@ public static void restore(Context context) {
OneSignal.Log(OneSignal.LOG_LEVEL.INFO, "Restoring notifications");

OneSignalDbHelper dbHelper = OneSignalDbHelper.getInstance(context);
deleteOldNotificationsFromDb(dbHelper);

StringBuilder dbQuerySelection = OneSignalDbHelper.recentUninteractedWithNotificationsWhere();
skipVisibleNotifications(context, dbQuerySelection);

queryAndRestoreNotificationsAndBadgeCount(context, dbHelper, dbQuerySelection);
}

private static void deleteOldNotificationsFromDb(OneSignalDbHelper dbHelper) {
SQLiteDatabase writableDb = null;

try {
writableDb = dbHelper.getWritableDbWithRetries();
writableDb.beginTransaction();
NotificationBundleProcessor.deleteOldNotifications(writableDb);
writableDb.setTransactionSuccessful();
} catch (Throwable t) {
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error deleting old notification records! ", t);
} finally {
if (writableDb != null) {
try {
writableDb.endTransaction(); // May throw if transaction was never opened or DB is full.
} catch (Throwable t) {
OneSignal.Log(OneSignal.LOG_LEVEL.ERROR, "Error closing transaction! ", t);
}
}
}
}

private static void queryAndRestoreNotificationsAndBadgeCount(
Context context,
OneSignalDbHelper dbHelper,
Expand Down
76 changes: 28 additions & 48 deletions OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignal.java
Original file line number Diff line number Diff line change
Expand Up @@ -595,10 +595,12 @@ public static void setAppContext(@NonNull Context context) {

if (wasAppContextNull) {
sessionManager = new OSSessionManager(getNewSessionListener());
outcomeEventsController = new OutcomeEventsController(sessionManager, OneSignalDbHelper.getInstance(appContext), outcomeSettings);
outcomeEventsController = new OutcomeEventsController(sessionManager, OneSignalDbHelper.getInstance(appContext));
// Prefs require a context to save
// If the previous state of appContext was null, kick off write in-case it was waiting
OneSignalPrefs.startDelayedWrite();
// Cleans out old cached data to prevent over using the storage on devices
OneSignalCacheCleaner.cleanOldCachedData(context);
}
}

Expand Down Expand Up @@ -3077,13 +3079,6 @@ static void fireEmailUpdateFailure() {
/*
* Start OneSignalOutcome module
*/
private static OutcomeSettings outcomeSettings = null;

static void changeOutcomeSettings(OutcomeSettings settings) {
outcomeSettings = settings;
outcomeEventsController.setOutcomeSettings(settings);
}

static OSSessionManager getSessionManager() {
return sessionManager;
}
Expand All @@ -3096,6 +3091,11 @@ public static void sendOutcome(@NonNull String name, OutcomeCallback callback) {
if (!isValidOutcomeEntry(name))
return;

if (outcomeEventsController == null) {
OneSignal.Log(LOG_LEVEL.ERROR, "Make sure OneSignal.init is called first");
return;
}

outcomeEventsController.sendOutcomeEvent(name, callback);
}

Expand All @@ -3107,6 +3107,11 @@ public static void sendUniqueOutcome(@NonNull String name, OutcomeCallback callb
if (!isValidOutcomeEntry(name))
return;

if (outcomeEventsController == null) {
OneSignal.Log(LOG_LEVEL.ERROR, "Make sure OneSignal.init is called first");
return;
}

outcomeEventsController.sendUniqueOutcomeEvent(name, callback);
}

Expand All @@ -3115,18 +3120,27 @@ public static void sendOutcomeWithValue(@NonNull String name, float value) {
}

public static void sendOutcomeWithValue(@NonNull String name, float value, OutcomeCallback callback) {
if (!isValidOutcomeEntry(name))
if (!isValidOutcomeEntry(name) || !isValidOutcomeValue(value))
return;

if (outcomeEventsController == null) {
OneSignal.Log(LOG_LEVEL.ERROR, "Make sure OneSignal.init is called first");
return;
}

outcomeEventsController.sendOutcomeEventWithValue(name, value, callback);
}

private static boolean isValidOutcomeEntry(String name) {
if (outcomeEventsController == null) {
OneSignal.Log(LOG_LEVEL.ERROR, "Make sure OneSignal.init is called first");
return false;
private static boolean isValidOutcomeValue(float value) {
if (value <= 0) {
OneSignal.Log(LOG_LEVEL.ERROR, "Outcome value must be greater than 0");
return false;
}

return true;
}

private static boolean isValidOutcomeEntry(String name) {
if (name == null || name.isEmpty()) {
OneSignal.Log(LOG_LEVEL.ERROR, "Outcome name must not be empty");
return false;
Expand All @@ -3136,41 +3150,7 @@ private static boolean isValidOutcomeEntry(String name) {
}

public interface OutcomeCallback {
void onOutcomeSuccess(String name);
void onOutcomeFail(int statusCode, String response);
}

static class OutcomeSettings {
private boolean cacheActive;

OutcomeSettings(Builder builder) {
this.cacheActive = builder.cacheActive;
}

boolean isCacheActive() {
return cacheActive;
}

public static class Builder {

private boolean cacheActive = true;

public static Builder newInstance() {
return new Builder();
}

private Builder() {
}

public Builder setCacheActive(boolean active) {
this.cacheActive = active;
return this;
}

public OutcomeSettings build() {
return new OutcomeSettings(this);
}
}
void onSuccess(OutcomeEvent outcomeEvent);
}
/*
* End OneSignalOutcome module
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.onesignal;

import com.onesignal.OneSignalDbContract.NotificationTable;
import com.onesignal.OneSignalDbContract.CachedUniqueOutcomeNotificationTable;

import android.content.Context;
import android.database.sqlite.SQLiteDatabase;
import android.os.Process;

class OneSignalCacheCleaner {

private static String OS_DELETE_OLD_CACHED_DATA = "OS_DELETE_OLD_CACHED_DATA";

/**
* We clean outdated cache from several places within the OneSignal SDK here
* 1. In App Messaging id sets (impressions, clicks, views)
* 2. Notifications after 1 week
* 3. Unique outcome events linked to notification ids (1 week)
*/
synchronized static void cleanOldCachedData(final Context context) {
new Thread(new Runnable() {
@Override
public void run() {
Thread.currentThread().setPriority(Process.THREAD_PRIORITY_BACKGROUND);
OneSignalDbHelper dbHelper = OneSignalDbHelper.getInstance(context);
SQLiteDatabase writableDb = dbHelper.getWritableDbWithRetries();

cleanInAppMessagingCache();
cleanNotificationCache(writableDb);
}
}, OS_DELETE_OLD_CACHED_DATA).start();
}

/**
* TODO: Needs to be implemented to clean out old IAM data used to track impressions, clicks, and viewed IAMs
*/
static void cleanInAppMessagingCache() {
// NOTE: Currently IAMs will pile up overtime and since IAMs can be modified, active, inactive, etc.
// we never truly know when it is the correct time to remove these ids form our cache
}

/**
* Cleans two notification tables
* 1. NotificationTable.TABLE_NAME
* 2. CachedUniqueOutcomeNotificationTable.TABLE_NAME
*/
static void cleanNotificationCache(SQLiteDatabase writableDb) {
cleanOldNotificationData(writableDb);
cleanOldUniqueOutcomeEventNotificationsCache(writableDb);
}

/**
* Deletes any notifications with created timestamps older than 7 days
*/
private static void cleanOldNotificationData(SQLiteDatabase writableDb) {
writableDb.delete(NotificationTable.TABLE_NAME,
NotificationTable.COLUMN_NAME_CREATED_TIME + " < " + ((System.currentTimeMillis() / 1_000L) - 604_800L),
null);
}

/**
* Deletes any notifications whose ids do not exist inside of the NotificationTable.TABLE_NAME
*/
static void cleanOldUniqueOutcomeEventNotificationsCache(SQLiteDatabase writableDb) {
writableDb.delete(CachedUniqueOutcomeNotificationTable.TABLE_NAME,
"NOT EXISTS(SELECT NULL FROM " + NotificationTable.TABLE_NAME +
" n WHERE" +
" n." + NotificationTable.COLUMN_NAME_NOTIFICATION_ID + " = " + CachedUniqueOutcomeNotificationTable.COLUMN_NAME_NOTIFICATION_ID + ")",
null);
}

}
Loading

0 comments on commit 5099bcb

Please sign in to comment.