-
Notifications
You must be signed in to change notification settings - Fork 233
Add in app mesage lifecycle handler #531
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
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
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 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
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 all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @emawby, @jkasten2, and @nan-li)
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.
I tried this out on Android and LGTM
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.
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
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.
@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 inAppMessage
has been removed, however removal of this var was missed.
* Remove unused IAM varialbes and `has set IAM Lifecycle handler` variables
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 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
,onDidDismissInAppMessage
handlers to Android and iOSTesting
Manual Testing
This change is