Fixing IAM displaying twice on cold start #949
Conversation
If we display IAMs prior to receiving the response to on session, we could be displaying cached IAMs that have been paused, or edited. Additionally this is causing an issue where we display IAMs twice because of the self.messages = newMessages line in updateInAppMessagesFromOnSession. We overwrite the seenInSession variable of the old object with the new one. By waiting to display IAMs until onSession has returned we fix this issue. To avoid a case where cached IAMs aren't shown when onSession fails I am now calling updateInAppMessagesFromCache in the failure block of registerUserInternal.
There was a problem hiding this comment.
Changes themselves look good to me. Would be good to have test for this however.
Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @Jeasmine)
emawby
force-pushed
the
fix/iam_display_twice
branch
from
2efdd90
to
f53be87
Compare
There was a problem hiding this comment.
Added a unit test validating that we don't try to display an IAM before init is called. Some customers have reported crashes in IAMs and it could be because we were previously trying to send impressions for cached IAMs before init is called (no appid or playerid) if they weren't always calling initWithLaunchOptions. The stack trace I got from this unit test without my changes looks similar to the customer stack traces I have seen
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @Jeasmine)
| @@ -241,8 +240,9 @@ - (void)presentInAppMessage:(OSInAppMessage *)message { | |||
| [self.messageDisplayQueue addObject:message]; | |||
|
|
|||
| // Return early if an IAM is already showing | |||
| if (self.isInAppMessageShowing) | |||
| if (self.isInAppMessageShowing || !OneSignal.isRegisterUserFinished) { | |||
There was a problem hiding this comment.
Maybe we can add a comment regarding this conditional addition. I imagine you are not putting it on [self evaluateMessages]; because even if the user is not registered, some triggers might have happened that make an IAM appear, but we need to wait for the user registration. Is that correct?
Can it happen that after the session call, the IAM added to the queue is deleted from the Server? Might messageDisplayQueue end in a weird consistency situation? because messageDisplayQueue is not used until evaluateMessageDisplayQueue is called
There was a problem hiding this comment.
Ya thats a good point I think I should check earlier so that the message doesn't enter the queue. I don't want the message in the queue and not in the messages array
This avoids a potential issue where an IAM is added to the queue, but then removed from the messages list after receiving the new messages from the server.
| @@ -1928,6 +1928,8 @@ + (void)registerUserInternal { | |||
| callbackSet.failureBlock(error); | |||
| } | |||
| } | |||
|
|
|||
| [OSMessagingController.sharedInstance updateInAppMessagesFromCache]; | |||
There was a problem hiding this comment.
I'm confused with one thing since you are checking for user registration to show IAM. At this point, OneSignal.isRegisterUserFinished; will return true? because it will immediateOnSessionRetry, then both _registerUserFinished || isOnSessionSuccessfulForCurrentState will be false right? Am I missing something? Are the cached IAM being displayed? Thanks in advance for clarifying this
There was a problem hiding this comment.
Ya you are right this isn't working we would need a registerUserFailed boolean or settings registerUserFinished to true if it fails (it gets reset when it is called again).
There was a problem hiding this comment.
I added a registerUserSuccessful boolean to be used in place of registerUserFinished in some places. Added comments to make it clear that registerUserFinished is also set to true when the call fails
If we display IAMs prior to receiving the response to on session, we could be displaying cached IAMs that have been paused, or edited. Additionally this is causing an issue where we display IAMs twice because of the
self.messages = newMessagesline inupdateInAppMessagesFromOnSession. We overwrite theseenInSessionvariable of the old object with the new one. By waiting to display IAMs until onSession has returned we fix this issue. To avoid a case where cached IAMs aren't shown when onSession fails I am now callingupdateInAppMessagesFromCachein the failure block ofregisterUserInternal.When trying to display a message before onSession has completed we will add it to the queue. We will then evaluate the message queue again once onSession finishes. If onSession fails we will still show cached IAMs because
updateInAppMessagesFromCachecallsevaluateMessagesThis change is