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

Fix/direct outcomes #823

Merged
merged 5 commits into from
Dec 4, 2020
Merged

Fix/direct outcomes #823

merged 5 commits into from
Dec 4, 2020

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Dec 3, 2020

The primary goal of this PR is to fix Direct Sessions in the major release.

The problem was that we were relying on the foreground variable to decide if we should upgrade the session,

if (!foreground) {
        OneSignal.appEntryState = NOTIFICATION_CLICK;
        [OneSignal.sessionManager onDirectInfluenceFromNotificationOpen:_appEntryState withNotificationId:messageId];
}

and in the major release I had changed the setting of the foreground variable to:
let isAppForeground = [[UIApplication sharedApplication] applicationState] != UIApplicationStateBackground;
From:
let isAppForeground = [[UIApplication sharedApplication] applicationState] == UIApplicationStateActive;

This was to account for the inactive state which is technically in the foreground.

The first commit on this branch just flips it back to [[UIApplication sharedApplication] applicationState] == UIApplicationStateActive; to fix direct outcomes.

However, I don't think we need foreground or isActive to be passed around at all anymore. Since we are using the native iOS call for willShowInForeground we don't need to track the states on notification received. We were only logging those variables and (improperly) using them for direct outcomes. So I removed the foreground and isActive variables and am checking the state directly when deciding if we need to upgrade the session for direct outcomes.

I also added unit tests for outcomes while in the inactive state

This change is Reviewable

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.

Awesome! This is way more clear

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.

Test failing, this is ok but @emawby please confirm tests pass locally?
We can fix the log limit in another PR as it is unrelated to this one.

@emawby
Copy link
Contributor Author

emawby commented Dec 4, 2020

The unit tests pass locally

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.

@emawby 👍 Merge it in!

@emawby emawby merged commit ccabea1 into major_release_3.0.0 Dec 4, 2020
@emawby emawby deleted the fix/direct_outcomes branch December 4, 2020 23:26
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.

3 participants