-
Notifications
You must be signed in to change notification settings - Fork 263
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
IAM lifecycle events #977
Merged
Merged
IAM lifecycle events #977
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
OSInAppMessageDelegate previously just had the handleMessageAction method, but there was no way to set the delegate and was never called.
Using this new class in our IAM Lifecycle Handler Methods
Previously migrate was only in init. This will still allow crashes due to not migrating if customers call other public methods prior to calling OneSignal initWithLaunchOptions. We should avoid this as best as possible by migrating immediately. This implementation can be changed later to queue all methods until after init has been called so that we don't unnecessarily migrate.
jkasten2
approved these changes
Aug 27, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @emawby and @nan-li)
iOS_SDK/OneSignalSDK/UnitTests/InAppMessagingIntegrationTests.m, line 1599 at r1 (raw file):
let firstTrigger = [OSTrigger customTriggerWithProperty:@"testProp" withOperator:OSTriggerOperatorTypeExists withValue:nil]; OSInAppMessageInternal * message = [OSInAppMessageTestHelper testMessageWithTriggers:@[@[firstTrigger]]];
nit, extra space around *
.
The IAM Lifecycle Methods will now match the Android naming of OSInAppMessageLifecycleHandler. I am leaving the unused OSInAppMessageDelegate to avoid build failures in customer systems that might erroneously be using the protocol.
emawby
force-pushed
the
feature/IAM_lifecycle_events
branch
from
August 30, 2021 16:27
2800265
to
97f0ef6
Compare
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.
This PR adds the
OSInAppMessageLifecycleHandler
protocol with the following lifecycle methods:And adds the public setter method
setInAppMessageLifecycleHandler
These methods should each be called once per display of an In App Message.
The InAppMessageDelegate was exposed in the public header with the method
- (void)handleMessageAction:(OSInAppMessageAction * _Nonnull)action
But it appears to never be called. I am leaving it in case it is erroneously being used by a consumer application.
This change is