Add in app mesage lifecycle handler#531
Conversation
* Add OSInAppMessage to OSFlutterCategories
* Add IAM Lifecycle methods to OneSignalPlugin.m * Add OSInAppMessageLifeCycleHandler to OneSignalPlugin declaration in OneSignalPlugin.h
* Update OS XCFramework version to 3.10.0 * Update onesignal_flutter.podspec
* Add IAM Lifecycle implementation to OneSignalPlugin.java * Add convertInAppMessageToMap to OneSignalSerializer
* Add IAM Lifecycle implementation to onesignal dart * Add OSInAppMessage to JSON representation in in_app_message dart file
* Add IAM Lifecycle methods to Flutter app
emawby
left a comment
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed (commit messages unreviewed), 3 unresolved discussions (waiting on @jkasten2, @nan-li, and @tanaynigam)
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 174 at r1 (raw file):
this.OnWillDisplayInAppMessageHandlerParams(); else if (call.method.contentEquals("OneSignal#onDidDisplayInAppMessageHandlerParams")) this.OnWillDisplayInAppMessageHandlerParams();
this is calling OnWillDisplat instead of onDidDisplay
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 176 at r1 (raw file):
this.OnWillDisplayInAppMessageHandlerParams(); else if (call.method.contentEquals("OneSignal#onWillDismissInAppMessageHandlerParams")) this.OnWillDisplayInAppMessageHandlerParams();
willdisplay instead of willdismiss
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 178 at r1 (raw file):
this.OnWillDisplayInAppMessageHandlerParams(); else if (call.method.contentEquals("OneSignal#onDidDismissInAppMessageHandlerParams")) this.OnWillDisplayInAppMessageHandlerParams();
willdisplay instead of diddismiss
* Fix IAM lifecycle handler params call on OneSignalPlugin.java
emawby
left a comment
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @emawby, @jkasten2, and @nan-li)
nan-li
left a comment
There was a problem hiding this comment.
I tried this out on Android and LGTM
jkasten2
left a comment
There was a problem hiding this comment.
I have main point comment around removing code that is trying to handle missed events. See details on the specific lines below.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tanaynigam)
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 62 at r2 (raw file):
private boolean waitingForUserPrivacyConsent = false; private OSInAppMessage inAppMessage;
I see this variable is to store the last IAM that had one of these newly added display / dismiss events. It is then read and reset if we added a new handler so it can fire a missed event if the handler was setup late. However it the way it is setup now it will only fire one event instead of all of them. The biggest thing is though these are more real time events where knowing about these after that fact would not be every useful.
Given the above I believe we not include the variable and this logic using it below.
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 466 at r2 (raw file):
@Override public void onWillDisplayInAppMessage(OSInAppMessage message) { onWillDisplayInAppMessageFlutter(message);
Given my comment above where inAppMessage is defined we can simplify this to do the this.hasSetOnWillDisplayInAppMessageHandler if check and call invokeMethodOnUiThread("OneSignal#onWillDisplayInAppMessage" directly here to.
Same goes for the 3 other methods below. And same for iOS as well.
ios/Classes/OneSignalPlugin.m, line 64 at r2 (raw file):
@property (atomic) BOOL hasSetOnDidDismissInAppMessageHandler; @property (strong, nonatomic) OSInAppMessage *inAppMessage;
Apply the same to iOS as my Android comment above.
* Remove OSInAppMessage * Remove InAppMessage Lifecycle HandlerParams methods * Remove InAppMessage Lifecycle Flutter methods * Move invokeMethodonUiThread for InAppMessageLifecycle methods in setInAppMessageLifecycleHandler
* Remove OSInAppMessage * Remove InAppMessage lifecycle handler params methods
* Remove IAM Lifecycle handler params in onesignal_flutter dart file
jkasten2
left a comment
There was a problem hiding this comment.
@tanaynigam PR looks good, just one nit on removing and extra unused variable. Feel free to merge without another approval after fixing.
Reviewed 7 of 10 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tanaynigam)
android/src/main/java/com/onesignal/flutter/OneSignalPlugin.java, line 62 at r2 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
I see this variable is to store the last IAM that had one of these newly added display / dismiss events. It is then read and reset if we added a new handler so it can fire a missed event if the handler was setup late. However it the way it is setup now it will only fire one event instead of all of them. The biggest thing is though these are more real time events where knowing about these after that fact would not be every useful.
Given the above I believe we not include the variable and this logic using it below.
All logic that was using this instanced inAppMessagehas been removed, however removal of this var was missed.
* Remove unused IAM varialbes and `has set IAM Lifecycle handler` variables
jkasten2
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tanaynigam)
Description
Line Summary
Adds the In-App Message Lifecycle Handler which includes up to 4 callbacks for the displaying and dismissing of In-App Messages.
Details
Motivation
This handler was added to give customers visibility into the display lifecycle of IAMs: when an IAM will be displayed, and also when it has been dismissed.
Scope
Extra fields will show up if a user logs the OSInAppMessage instead of just its messageId property, which is fine.
In-App Messages are only read and not modified.
Public API Changes
The documentation will need to be updated for react native to provide these new methods.
onWillDisplayInAppMessage,onDidDisplayInAppMessage,onWillDismissInAppMessage,onDidDismissInAppMessagehandlers to Android and iOSTesting
Manual Testing
This change is