[Fix] optIn() not prompting if called before push subscription is created on backend #2037
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jkasten2
requested review from
emawby,
jinliu9508,
shepherd-l,
jennantilla and
brismithers
jinliu9508
approved these changes
Mar 28, 2024
nan-li
approved these changes
Mar 28, 2024
| @@ -157,7 +157,10 @@ internal class SubscriptionManager( | |||
| args: ModelChangedArgs, | |||
| tag: String, | |||
| ) { | |||
| val subscription = subscriptions.collection.firstOrNull { it.id == args.model.id } | |||
There was a problem hiding this comment.
I see, to use id as a getter, it would have to compare it.model.id == args.model.id
There was a problem hiding this comment.
Ya, since the Susbcription.id returns "" if it starts with local- it was not finding it, using model always gives us the id value
jkasten2
force-pushed
the
fix/limit_some_rest_calls_to_foreground
branch
from
94fe2a8
to
3b85296
Compare
jkasten2
force-pushed
the
fix/optIn-not-prompting-if-called-before-sub-created
branch
from
3c10617
to
57e8025
Compare
This is a common case where updates (such as optedIn) should still propagate even if we haven't sent the POST /users create call yet. Motivation for this test was a bug was discovered where calling OneSignal.User.pushSubscription.optIn() was not prompting for notification permission if it was called before the create User network call finished.
This is a common case where updates (such as optedIn) should still propagate even if we haven't sent the POST /users create call yet. Motivation for this test was a bug was discovered where calling OneSignal.User.pushSubscription.optIn() was not prompting for notification permission if it was called before the create User network call finished.
jkasten2
force-pushed
the
fix/optIn-not-prompting-if-called-before-sub-created
branch
from
57e8025
to
a92bcee
Compare
Base automatically changed from
fix/limit_some_rest_calls_to_foreground
to
main
March 29, 2024 17:27
jkasten2
deleted the
fix/optIn-not-prompting-if-called-before-sub-created
branch
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Fix
optIn()not prompting if called before push subscription is created on backend .Details
Motivation
It's common for developers to call
OneSignal.User.pushSubscription.optIn()and it should prompt even if the device is offline or has connection issues.Scope
Effects internal push subscription property change events.
Testing
Unit testing
Created a new unit test to cover this case.
Manual testing
Tested on a real Samsung S21 with Android 14 (security patch level Feb 1, 2024). Clean installed example app while the device was in airplane mode, ensuring calling
optIn()now prompts. Also tested adding an email and sms when the device was offline and then when I turn back on network connect it successfully created all the subscriptions as expected.Affected code checklist
Checklist
Overview
Testing
Final pass
This change is