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

Don't wait for push prompt to register user #990

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Sep 2, 2021

Previously we would call registerUserAfterDelay if the user has not completed the push prompt or they don't have provisional auth. We want to register right away so that we can get IAMs for the session (which might include a push prompt IAM).

I believe this was intended to be an optimization to wait for an answer to the push prompt if the prompt is currently being display but that was not the behavior, and I don't think we need to try to achieve that optimization.


This change is Reviewable

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.

Changes look good to me. In the future make sure to mix unrelated changes in the same PR. Especially in the same commit.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine and @nan-li)

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.

make sure not to mix*

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

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @nan-li)


iOS_SDK/OneSignalSDK/Source/OneSignal.m, line 1634 at r1 (raw file):

    self.currentSubscriptionState.pushToken = deviceToken;

    [self.osNotificationSettings getNotificationPermissionState:^(OSPermissionState *status) {

We are no longer using status so we don't need to call getNotificationPermissionState here.

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.

One scenario I am not sure what will happen is if app project has "background modes" off for notifications. In this scenario we don't get an APNs response since it won't give us a token. I had a similar WIP PR to this one where it was an open question to come back too:
https://github.com/OneSignal/OneSignal-iOS-SDK/pull/891/files#diff-1188e625278a26833ef333f891da137dfbf384cde4f6c5f97ba8298e4eda5afaR772-R774

However we should investigate if this change will cause the player to never be created. If so we need to address it in this PR.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @nan-li)

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.

Ignore the "background modes" off comment for this PR. Since the logic change is in updateDeviceToken this PR could not possibly change this. Something however we can look into later as a follow up.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby, @Jeasmine, and @nan-li)

@emawby emawby force-pushed the fix/register_user_immediately_after_apns branch 2 times, most recently from cdd9c14 to 8e009f9 Compare September 3, 2021 21:37
Previously we would wait for the notification permission prompt to be completed before we registered the user. This is not a correct optimization so we are removing that behavior.
This includes other unit test fixes for broken in app messaging tests that was caused by the IAM lifecycle project
@emawby emawby force-pushed the fix/register_user_immediately_after_apns branch from 8e009f9 to 66b3d4f Compare September 3, 2021 21:37
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 7 files reviewed, 1 unresolved discussion (waiting on @Jeasmine, @jkasten2, and @nan-li)


iOS_SDK/OneSignalSDK/Source/OneSignal.m, line 1634 at r1 (raw file):

Previously, jkasten2 (Josh Kasten) wrote…

We are no longer using status so we don't need to call getNotificationPermissionState here.

Done.

@emawby emawby requested a review from jkasten2 September 3, 2021 21:38
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:

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Jeasmine and @nan-li)

@emawby emawby merged commit 3672cc2 into main Sep 13, 2021
@emawby emawby deleted the fix/register_user_immediately_after_apns branch September 13, 2021 18:05
@emawby emawby mentioned this pull request Sep 13, 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