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

IAM liquid templating support #887

Merged
merged 17 commits into from
Mar 17, 2021
Merged

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Feb 19, 2021

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 OSInAppMessageView
  • This PR also adds OSPlayerTags to cache tags in NSUserDefaults on the device
  • 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 once per session.
  • currently is targeting local host for testing. This will be removed before merging

This change is Reviewable

@emawby emawby force-pushed the feature/iam_liquid_templating_support branch from 64e8c91 to 37e2371 Compare February 19, 2021 23:29
@emawby emawby force-pushed the feature/iam_liquid_templating_support branch from 37e2371 to 7f67548 Compare February 19, 2021 23:31
@emawby emawby force-pushed the feature/iam_liquid_templating_support branch 2 times, most recently from 2a18e4b to 8e5d307 Compare February 19, 2021 23:38
Loading the tags in parallel to the HTML content and only loading once per session.
@emawby emawby force-pushed the feature/iam_liquid_templating_support branch from 8e5d307 to 50341a8 Compare February 19, 2021 23:39
@emawby emawby requested a review from jkasten2 February 19, 2021 23:46
Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, 7 of 7 files at r2, 1 of 3 files at r3, 1 of 2 files at r4, 1 of 6 files at r5, 1 of 3 files at r6, 1 of 3 files at r7, 3 of 4 files at r8, 3 of 6 files at r9, 6 of 6 files at r10.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @emawby)


iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m, line 99 at r1 (raw file):

}

#define ONESIGNAL_APP_ID_KEY_FOR_TESTING @"1bcc4195-2039-4b18-83d7-2ab5b98daf76"

This is only a dev project but I recommend we don't change the app_id in a feature PR


iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m, line 82 at r10 (raw file):

    [OneSignal setLaunchURLsInApp:YES];
    [OneSignal setProvidesNotificationSettingsView:NO];
    //[OneSignal sendTag:@"player_name" value:@"cached tag works!"];

Remove commented out code


iOS_SDK/OneSignalSDK/Source/OneSignal.m, line 422 at r2 (raw file):

+ (NSDictionary *)getPlayerTags {
    NSLog(@"ECM player tags: %@", playerTags);

Remove NSLog


iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h, line 36 at r10 (raw file):

#define OS_API_VERSION @"1"
#define OS_API_ACCEPT_HEADER @"application/vnd.onesignal.v" OS_API_VERSION @"+json"
#define OS_API_SERVER_URL @"http://192.168.1.86:3000/api/v1/"

URL needs to be put back


iOS_SDK/OneSignalSDK/Source/OSInAppMessage.m, line 179 at r10 (raw file):

    
    if (self.hasLiquid) {
        json[@"has_liquid"] = @"1";

I would expect this to be a boolean in JSON.


iOS_SDK/OneSignalSDK/Source/OSMessagingController.m, line 294 at r10 (raw file):

- (void)loadTags {
    self.loadedTags = YES;

Shouldn't loadedTags be set to true in the getTags callback below instead of here?


iOS_SDK/OneSignalSDK/UnitTests/InAppMessagingTests.m, line 217 at r10 (raw file):

}

- (void)testCorrectlyParsedHasLiquid {

Not sure I understand this test. Why do we want hasLiquid to be true on the message but not the redisplay?

Copy link
Contributor Author

@emawby emawby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 9 of 16 files reviewed, 7 unresolved discussions (waiting on @emawby and @jkasten2)


iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m, line 99 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This is only a dev project but I recommend we don't change the app_id in a feature PR

I will be better about this in the future. I want to leave the commit in the history to make it easier to test and but I will revert it before asking for review next time


iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m, line 82 at r10 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Remove commented out code

Done.


iOS_SDK/OneSignalSDK/Source/OneSignal.m, line 422 at r2 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Remove NSLog

Done.


iOS_SDK/OneSignalSDK/Source/OneSignalCommonDefines.h, line 36 at r10 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

URL needs to be put back

leaving commit in history but reverting until merge


iOS_SDK/OneSignalSDK/Source/OSInAppMessage.m, line 179 at r10 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

I would expect this to be a boolean in JSON.

Done.


iOS_SDK/OneSignalSDK/UnitTests/InAppMessagingTests.m, line 217 at r10 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Not sure I understand this test. Why do we want hasLiquid to be true on the message but not the redisplay?

I made this test more clear by adding methods to InAppMessagingHelpers

@emawby emawby requested a review from jkasten2 March 5, 2021 18:33
@jkasten2
Copy link
Member


iOS_SDK/OneSignalSDK/Source/OSMessagingController.m, line 294 at r10 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Shouldn't loadedTags be set to true in the getTags callback below instead of here?

The new name calledLoadTags makes sense now

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 3 of 3 files at r15.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jkasten2)

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, other than the two commits that need need to be undone before we merge

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

Copy link
Member

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r16, 1 of 1 files at r17.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @emawby)

@emawby emawby merged commit e502609 into master Mar 17, 2021
@emawby emawby mentioned this pull request Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants