-
Notifications
You must be signed in to change notification settings - Fork 368
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 carousel page impressions #1239
Conversation
@Override | ||
void onSuccess(String response) { | ||
printHttpSuccessForInAppMessageRequest("page impression", response); | ||
/*OneSignalPrefs.saveStringSet( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is a TODO yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this was my question at the end of the PR do we need to save the ids to the prefs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only scenario that I think needs this.
1 - Opening app
2 - Display IAM
3 - Impression sent
4 - Close application (swipe away) without dismiss IAM
5 - Open app
6 - IAM should display again
7 - Impression shouldn't be sent again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool confirmed that this is an issue so I will store them in the prefs
@@ -4,6 +4,6 @@ | |||
# Location of the SDK. This is only used by Gradle. | |||
# For customization when using a Version Control System, please read the | |||
# header note. | |||
#Tue Jul 23 19:22:28 PDT 2019 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a ignored file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this file from the PR?
@@ -125,6 +133,11 @@ public String getClickName() { | |||
return clickName; | |||
} | |||
|
|||
@NonNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is marked as notNull but
pageId = json.optString(PAGE_ID, null);
it could end being null, maybe set a default value? tr change to Nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
OneSignalSDK/onesignal/src/main/java/com/onesignal/OSInAppMessageController.java
Show resolved
Hide resolved
|
||
|
||
// Never send multiple page impressions for the same message UUID unless that page change is from an IAM with redisplay | ||
if (message.getViewedPageIds().contains(pageId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is checked under onPageChanged too, is that ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now only checking in the rest method call for consistency with iOS and better separation of concerns
pageIndex = jsonObject.optString(PAGE_INDEX, null) | ||
} | ||
|
||
fun toJSONObject(): JSONObject? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the optional needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope fixed
|
||
@Test | ||
public void testOnPageChanged() throws Exception { | ||
threadAndTaskWait(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it crashes without it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, weird 🤔 test setup should not break tests, can you point out which error it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry the crash can be fixed, but you need to initialize to have the player id etc for the request
public void testOnPageChanged() throws Exception { | ||
threadAndTaskWait(); | ||
|
||
OneSignalPackagePrivateHelper.onPageChanged(message, InAppMessagingHelpers.buildTestPageJson()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is ok to test without mocking the IAM display?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we test without mocking the iam display for clicks as well and they should be entirely independent
@@ -4,6 +4,6 @@ | |||
# Location of the SDK. This is only used by Gradle. | |||
# For customization when using a Version Control System, please read the | |||
# header note. | |||
#Tue Jul 23 19:22:28 PDT 2019 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this file from the PR?
final String messagePrefixedPageId = message.messageId + pageId; | ||
|
||
// Never send multiple page impressions for the same message UUID unless that page change is from an IAM with redisplay | ||
if (message.getViewedPageIds().contains(pageId) || viewedPageIds.contains(messagePrefixedPageId)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as on iOS, see OneSignal/OneSignal-iOS-SDK#735 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same fix as ios. only tracking globally now, but it is still per inapp since we don't want them to affect each other (in local testing I was getting duplicate page ids for different inapps which is why I prefixed the page id with the message id)
@@ -494,6 +568,8 @@ private void setDataForRedisplay(OSInAppMessage message) { | |||
|
|||
dismissedMessages.remove(message.messageId); | |||
impressionedMessages.remove(message.messageId); | |||
viewedPageIds.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put some comment over this desition of cleaning impressions, so we know in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
9fd9540
to
1a8c6a9
Compare
1a8c6a9
to
38ef376
Compare
This PR adds SDK support for individual IAM Carousel page impressions.
It adds a new model
OSInAppMessagePage.kt
and event type pageChange 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
fireRESTCallForPageChange
Page impressions are only sent once per Impression of a Carousel. The ids are tracked in a new set
viewedPageIds
on theOSInAppMessage
objectAdditionally we receive the pageId for click events, but do not currently use them.
Question: Do we need to save impressioned pages in OneSignalPrefs like we do for normal impression? (commented out code right now)
This change is