IAM Tag Substitution #1283

emawby · 2021-02-23T01:41:26Z

This PR adds support for IAM liquid templating tag substitution
The liquid rendering is handled by a javascript library added to the IAM by the dashboard. The SDKs just need to inject the tags for the player into the IAM html.

Injects tags into the IAM via the iamInfo.tags object in the IAM. I had to add it into the HTML before it was loaded into the webview to get it to render properly. This takes place in OSInAppMessageController after the HTML is loaded and tags are retrieved.
Tags are retrieved in parallel with the IAM HTML if a new property has_liquid is true in the IAM json. Tags will only be retrieved from the server once per app run (no change in behavior from existing getTags logic)
currently is targeting local host for testing. This will be removed before merging

This change is

hasLiquid does not exist yet on the IAM object so I am using a placeholder

Parsed from "has_liquid" this property indicates if an IAM contains liquid templating. It is currently used to ask for tags for tag substitution when loading HTML content for an IAM

Jeasmine · 2021-03-01T23:41:40Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/application/MainApplication.java

@@ -45,8 +45,8 @@ public void onCreate() {
            notificationReceivedEvent.complete(null);
        });

-        OneSignal.unsubscribeWhenNotificationsAreDisabled(true);
-        OneSignal.pauseInAppMessages(true);
+        OneSignal.unsubscribeWhenNotificationsAreDisabled(false);


demo application changes should be done separately, but in this case, it seems like they are not needed for this change

reverting the local testing commit but leaving in the history so I can add it back easier. I will remove this before merging. For the bug bash I will make this target staging instead

Jeasmine · 2021-03-01T23:48:23Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/util/SharedPreferenceUtil.java

@@ -25,7 +25,7 @@ public static boolean exists(Context context, String key) {
    }

    public static String getOneSignalAppId(Context context) {
-        return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "77e32082-ea27-42e3-a898-c72e141824ef");
+        return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "1bcc4195-2039-4b18-83d7-2ab5b98daf76");


reverting the local testing commit but leaving in the history so I can add it back easier. I will remove this before merging. For the bug bash I will make this target staging instead

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessage.java

Jeasmine · 2021-03-01T23:59:48Z

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java

@@ -750,6 +762,24 @@ private void displayMessage(@NonNull final OSInAppMessage message) {
        }

        inAppMessageShowing = true;
+        waitForTags = false;
+        if (message.getHasLiquid()) {


Why not wait for get tags to call display message and avoid the parallel flag check?

What about checking on the evaluateInAppMessages method if the IAM hasLiquid? If so, check if it already has tags set. If not, download tags and put them to that IAM, then call evaluateInAppMessages again

We definitely could do that and it would be cleaner code but this is theoretically more performant since we are kicking off the rest requests in parallel we don't need to wait for 2 round trips.

I am open to not doing them in parallel if the performance improvement is less important than the simplicity and reliability of the sequential calls.

Agree that this will be less performant, but I'm worried about future extensibility and maintainability with other functionalities. Maybe @jkasten2 can give his opinion on this one

Left a follow up comment here: https://reviewable.io/reviews/onesignal/onesignal-android-sdk/1283/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java#-MV-jin1cLek6URzCeI5-r1-766

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java

Jeasmine · 2021-03-02T00:01:13Z

OneSignalSDK/onesignal/src/main/java/com/onesignal/OneSignalRestClient.java

@@ -52,7 +52,7 @@ void onFailure(int statusCode, String response, Throwable throwable) {}

   private static final String OS_API_VERSION = "1";
   private static final String OS_ACCEPT_HEADER = "application/vnd.onesignal.v" + OS_API_VERSION + "+json";
-   private static final String BASE_URL = "https://api.onesignal.com/";
+   private static final String BASE_URL = "http://192.168.1.86:3000/api/v1/";


reverting the local testing commit but leaving in the history so I can add it back easier. I will remove this before merging. For the bug bash I will make this target staging instead

OneSignalSDK/unittest/src/test/java/com/onesignal/InAppMessagingHelpers.java

This reverts commit 684d221.

Jeasmine

LGTM, I want to know about @jkasten2 point of view of the logic performance vs. maintainability

jkasten2

Reviewed 1 of 8 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @emawby and @Jeasmine)

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

Previously, Jeasmine (JNahui) wrote…

Agree that this will be less performant, but I'm worried about future extensibility and maintainability with other functionalities. Maybe @jkasten2 can give his opinion on this one

I am concerned about performance if we don't make these calls in parallel. It depends on the IAM but if someone calls OneSignal.setTrigger in their app from a button press and wants an IAM to show up in response we want to make sure that happens as fast as possible.

To make this code cleaner we could use an async / await strategy for this instead. This is a Kotlin feature and would require us to switch some of this logic to Kotlin. Which we could do now but would recommend doing so in a follow up PR. Seems like a interesting trail to blaze for this code base, if someone is interested in digging.
https://kotlinlang.org/docs/composing-suspending-functions.html#concurrent-using-async
@Jeasmine @emawby Thoughts?

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java, line 831 at r5 (raw file):

    }

    String taggedHTMLString(@NonNull String untaggedString) {

Add @NonNull to return type too.

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java, line 833 at r5 (raw file):

    String taggedHTMLString(@NonNull String untaggedString) {
        String tagsDict = userTagsString;
        String tagScript =  LIQUID_TAG_SCRIPT;

nit, extra space after =.

Jeasmine

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @emawby, @Jeasmine, and @jkasten2)

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

Previously, jkasten2 (Josh Kasten) wrote…

Left a follow up comment here: https://reviewable.io/reviews/onesignal/onesignal-android-sdk/1283/OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java#-MV-jin1cLek6URzCeI5-r1-766

Fair point. in this case, we do want performance better than maintainability. Introducing suspending functions will mean introducing coroutines. I do like the idea of introducing coroutines and was part of the reactor, but that will mean changing a lot of stuff if we want to do it correctly, first making a new package for IAM and transform it to kotlin, after that define a layer for coroutine and scope managing, then use coroutines. But will love to do that 🔥

emawby

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Jeasmine and @jkasten2)

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

Previously, Jeasmine (JNahui) wrote…

Fair point. in this case, we do want performance better than maintainability. Introducing suspending functions will mean introducing coroutines. I do like the idea of introducing coroutines and was part of the reactor, but that will mean changing a lot of stuff if we want to do it correctly, first making a new package for IAM and transform it to kotlin, after that define a layer for coroutine and scope managing, then use coroutines. But will love to do that 🔥

Ya I am definitely interested in improving this area. Keeping it maintainable and performant is ideal so I am onboard with using something like async/await in the future

OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java, line 831 at r5 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Add @NonNull to return type too.

Done.

Also removing extra space

jkasten2

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Jeasmine and @jkasten2)

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

Previously, emawby (Elliot Mawby) wrote…

Ya I am definitely interested in improving this area. Keeping it maintainable and performant is ideal so I am onboard with using something like async/await in the future

Not a requirement for this PR but wanted to bring it up. I am in favor of refactoring in another PR to keep this PR simple.

jkasten2

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Jeasmine)

We will always get tags for the preview since we don't have a real InAppMessage object to check for hasLiquid.

Jeasmine · 2021-03-12T20:58:07Z

I retrigger the Travis with no intention, but I saw there were 2 new test failing, its that correct?

Jeasmine · 2021-03-12T21:34:38Z

Looks like shouldShowInAppPreviewWhenOpeningPreviewNotification and osIAMPreview_showsPreview are always failing

emawby added 3 commits February 22, 2021 12:41

local testing

684d221

Adding script for injecting tags into IAM

40fb5a2

getting tags in parallel with IAM html content if hasLiquid

a0263fc

hasLiquid does not exist yet on the IAM object so I am using a placeholder

emawby requested review from jkasten2 and Jeasmine February 23, 2021 01:41

emawby added 2 commits February 22, 2021 18:02

adding hasLiquid to OSInAppMessage

8295c0a

Parsed from "has_liquid" this property indicates if an IAM contains liquid templating. It is currently used to ask for tags for tag substitution when loading HTML content for an IAM

unit test for has_liquid and getTags behavior

5e7805b

emawby force-pushed the feature/iam_tab_sub branch from dc379f4 to 5e7805b Compare February 23, 2021 02:02

Jeasmine reviewed Mar 2, 2021

View reviewed changes

emawby added 4 commits March 2, 2021 15:14

Revert "local testing"

942d552

This reverts commit 684d221.

reformatting hasLiquid accessors

51a4ddb

defining "has_liquid" as a constant

1cc3c1f

removing extra semicolon

b7ca4ff

emawby requested a review from Jeasmine March 2, 2021 23:22

Jeasmine reviewed Mar 4, 2021

View reviewed changes

jkasten2 requested changes Mar 5, 2021

View reviewed changes

Jeasmine approved these changes Mar 5, 2021

View reviewed changes

emawby commented Mar 5, 2021

View reviewed changes

emawby added 2 commits March 5, 2021 10:44

adding nonnull to taggedHTMLString

b4eaf71

Also removing extra space

rename of javascript variable for player's tags

8892627

jkasten2 approved these changes Mar 11, 2021

View reviewed changes

using setPlayerTags function instead of setting tag variable directly

1db43af

jkasten2 approved these changes Mar 11, 2021

View reviewed changes

Fixing tag substitution for IAM previews

989224b

We will always get tags for the preview since we don't have a real InAppMessage object to check for hasLiquid.

Jeasmine approved these changes Mar 12, 2021

View reviewed changes

fixing iam preview unit tests

be363ae

emawby merged commit 97df02d into master Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IAM Tag Substitution #1283

IAM Tag Substitution #1283

emawby commented Feb 23, 2021 •

edited by jkasten2

Loading

Jeasmine Mar 1, 2021

emawby Mar 2, 2021

Jeasmine Mar 1, 2021

emawby Mar 2, 2021

Jeasmine Mar 1, 2021

emawby Mar 2, 2021

Jeasmine Mar 3, 2021

jkasten2 Mar 5, 2021

Jeasmine Mar 2, 2021

emawby Mar 2, 2021

Jeasmine left a comment

jkasten2 left a comment

Jeasmine left a comment

emawby left a comment

jkasten2 left a comment

jkasten2 left a comment

Jeasmine commented Mar 12, 2021

Jeasmine commented Mar 12, 2021

IAM Tag Substitution #1283

IAM Tag Substitution #1283

Conversation

emawby commented Feb 23, 2021 • edited by jkasten2 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jeasmine left a comment

Choose a reason for hiding this comment

jkasten2 left a comment

Choose a reason for hiding this comment

Jeasmine left a comment

Choose a reason for hiding this comment

emawby left a comment

Choose a reason for hiding this comment

jkasten2 left a comment

Choose a reason for hiding this comment

jkasten2 left a comment

Choose a reason for hiding this comment

Jeasmine commented Mar 12, 2021

Jeasmine commented Mar 12, 2021

emawby commented Feb 23, 2021 •

edited by jkasten2

Loading