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

Fixing impressions being sent for in-active IAMs #931

Merged
merged 5 commits into from
May 20, 2021

Conversation

emawby
Copy link
Contributor

@emawby emawby commented May 19, 2021

Prior to this PR we would send an IAM impression before validating that we were successfully able to load the html content and display the IAM to the user. This meant that we would still send impressions for cached paused/completed IAMs that would receive a "410 IAM is in-active" error for the loadHTMLContent request. The IAM would not display to the user, but we should not be sending these impressions.

Now we will send IAM impressions in response to the webViewContentFinishedLoading callback which is triggered by the rendering_complete event from the webview's javascript. This guarantees that any errors that occur before displaying the IAM will result in an impression not being sent.

Additionally we will now delete IAMs from the cache that receive the in-active error when loading HTML content so we don't keep trying to display in-active IAMs.


This change is Reviewable

@emawby emawby requested review from jkasten2 and Jeasmine May 19, 2021 18:10
Fixes an issue with IAM content not being updated for display multiple times IAMs
After calling getTags self.playerTags was not being set with the tags unless they were cached
@emawby emawby changed the base branch from master to fix/improve_tag_sub_logging May 19, 2021 20:54
@emawby emawby changed the base branch from fix/improve_tag_sub_logging to fix/dont_cache_iam_content_request May 19, 2021 20:55
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 7 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emawby and @Jeasmine)


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 269 at r1 (raw file):

- (void)encounteredErrorLoadingMessageContent:(NSError * _Nullable)error {
    let message = [NSString stringWithFormat:@"An error occurred while attempting to load message content: %@", error.description ?: @"Unknown Error"];
    if (error.code == 410 || [error.description containsString:@"In-App Message is not active"]) {

Shouldn't code against a human readable message as it could change. I word remove the the description check.
Also we should check for 404.


iOS_SDK/OneSignalSDK/UnitTests/UnitTestCommonMethods.m, line 155 at r1 (raw file):

 This will allow more time for chained async methods to complete
 */
+ (void)runLongBackgroundThreads {

This new selector shares a lot of with runBackgroundThreads. Can the code between the be shared?

@emawby emawby changed the base branch from fix/dont_cache_iam_content_request to master May 20, 2021 01:15
Reading error message when loading HTML which checks if the IAM is active. If not then we want to delete it from the cache.
webViewContentFinishedLoading is called after the rendering_complete JS event is fired from the IAM webview. By calling the impression here we ensure that an impression is only sent if the IAM is actually displayed
@emawby emawby force-pushed the fix/stopped_iams_still_display branch from 0167aa5 to 95e5c75 Compare May 20, 2021 01:27
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: 5 of 12 files reviewed, 2 unresolved discussions (waiting on @Jeasmine and @jkasten2)


iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m, line 269 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

Shouldn't code against a human readable message as it could change. I word remove the the description check.
Also we should check for 404.

Done.


iOS_SDK/OneSignalSDK/UnitTests/UnitTestCommonMethods.m, line 155 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

This new selector shares a lot of with runBackgroundThreads. Can the code between the be shared?

Done.

@emawby emawby requested a review from jkasten2 May 20, 2021 01:28
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 7 of 7 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine and @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.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine)

@emawby emawby merged commit a4e6920 into master May 20, 2021
@emawby emawby deleted the fix/stopped_iams_still_display branch May 20, 2021 18:55
@emawby emawby mentioned this pull request May 20, 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