Skip to content

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

Merged
merged 11 commits into from
Feb 25, 2022
Merged

Conversation

tanaynigam
Copy link
Contributor

@tanaynigam tanaynigam commented Jan 21, 2022

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.

  • Add onWillDisplayInAppMessage, onDidDisplayInAppMessage, onWillDismissInAppMessage, onDidDismissInAppMessage handlers to Android and iOS
  • Add respective handlers to OneSignal dart file
  • Add example handlers to the example app

Testing

Manual Testing

  • Test device using IAM lifecycle handlers on Android and iOS. The IAM lifecycle handlers trigger on receiving an In-app message.

This change is Reviewable

* 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
Copy link
Contributor

@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.

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
@tanaynigam tanaynigam requested a review from emawby February 1, 2022 00:29
Copy link
Contributor

@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.

Reviewed all commit messages.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @emawby, @jkasten2, and @nan-li)

Copy link
Contributor

@nan-li nan-li left a 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

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.

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
@tanaynigam tanaynigam requested a review from jkasten2 February 22, 2022 16:22
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.

@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
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 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tanaynigam)

@tanaynigam tanaynigam merged commit 3f03b5f into main Feb 25, 2022
@tanaynigam tanaynigam deleted the feature/IAM-lifecycle-methods branch February 25, 2022 00:45
nan-li added a commit that referenced this pull request Mar 10, 2022
@nan-li nan-li mentioned this pull request Mar 10, 2022
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.

4 participants