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.

Sign up for GitHub

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

Jump to bottom

Iam carousel page impressions #735

Merged
merged 11 commits into from Dec 23, 2020
Merged

Iam carousel page impressions #735

merged 11 commits into from Dec 23, 2020

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Sep 2, 2020 •
edited

This PR adds SDK support for individual IAM Carousel page impressions.

It adds a new model OSInAppMessagePage and bridge event type OSInAppMessageBridgeEventTypePageChange that reads the pageId from the new page_change event that is received from the carousel's javascript.

To send the page impression we use the new pageImpression endpoint using the new request OSRequestInAppMessagePageViewed
Page impressions are only sent once per Impression of a Carousel. The ids are tracked in a new set viewedPageIds on the OSInAppMessage object

Additionally we receive the pageId for click events, but do not currently use them.

This change is Reviewable

@emawby emawby changed the base branch from major_release_3.0.0 to master December 9, 2020 22:51
@emawby emawby changed the title WiP iam carousel page impressions Iam carousel page impressions Dec 9, 2020
jkasten2
jkasten2 approved these changes Dec 11, 2020
View reviewed changes
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.

Looks good, just one nit on a comment clean up

iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m Outdated Show resolved Hide resolved
Jeasmine
Jeasmine reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

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

LGTM, nit comments

iOS_SDK/OneSignalSDK/Source/OSInAppMessageAction.m Outdated
@@ -72,6 +75,8 @@ + (instancetype _Nullable)instanceWithJson:(NSDictionary *)json {
action.closesMessage = [json[@"close"] boolValue];
else
action.closesMessage = true; // Default behavior

[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"%@",json]];
Copy link
Contributor

Choose a reason for hiding this comment

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

nit indentation comment, space before the json variable.
Is it clear that this log is from the OSInAppMessageAction" If not maybe add @"OSInAppMessageAction json: %@" introduction string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

iOS_SDK/OneSignalSDK/Source/OSInAppMessageBridgeEvent.h Outdated
@property (strong, nonatomic, nullable) OSInAppMessageAction *userAction;
@end



Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra enters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

iOS_SDK/OneSignalSDK/Source/OSInAppMessagePage.m Outdated
NSDictionary *json = [NSJSONSerialization JSONObjectWithData:data options:0 error:&error];

if (error || !json) {
[OneSignal onesignalLog:ONE_S_LL_ERROR message:[NSString stringWithFormat:@"Unable to decode in-app message JSON: %@", error.description ?: @"No Data"]];
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be in-app message page? just to know the error is from this log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@emawby emawby requested a review from Jeasmine December 14, 2020 18:48
Jeasmine
Jeasmine reviewed Dec 14, 2020
View reviewed changes
iOS_SDK/OneSignalSDK/Source/OSMessagingController.m Show resolved Hide resolved
iOS_SDK/OneSignalSDK/Source/OSMessagingController.m
@@ -372,7 +426,10 @@ - (void)setDataForRedisplay:(OSInAppMessage *)message {

[self.seenInAppMessages removeObject:message.messageId];
[self.impressionedInAppMessages removeObject:message.messageId];
[self.viewedPageIDs removeAllObjects];
Copy link
Contributor

Choose a reason for hiding this comment

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

this line shouldn't instead remove all objects from the message page ids?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed tracking from the message itself

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of removing all objects, should only remove the ids that have this IAM id prefix, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually doesn't matter since at this point we have dismissed and are showing a different IAM, at which point we can resend the impressions so either way it will always be empty when a new IAM is showing. I would rather have some weird edge case that results in sending too many impressions than an edge case that results in not sending impressions.

iOS_SDK/OneSignalSDK/Source/OSMessagingController.m Outdated

NSString *messagePrefixedPageId = [message.messageId stringByAppendingString:pageId];

if ([[message getViewedPageIds] containsObject:pageId] || [self.viewedPageIDs containsObject:messagePrefixedPageId]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

sry is not clear to me why we have pageIds under the message and a global list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it from the message!

@Jeasmine Jeasmine mentioned this pull request Dec 14, 2020
Merged
@emawby emawby requested a review from Jeasmine December 14, 2020 21:37
Jeasmine
Jeasmine approved these changes Dec 15, 2020
View reviewed changes
Copy link
Contributor

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

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

LGTM, just reset the URL back to normal before merging

@emawby emawby merged commit 77cb220 into master Dec 23, 2020
@emawby emawby deleted the feature/iam_carousel branch December 23, 2020 22:20
@emawby emawby mentioned this pull request Dec 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

@Jeasmine Jeasmine Jeasmine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@emawby @jkasten2 @Jeasmine