Skip to content

Conversation

mikechoch
Copy link
Contributor

@mikechoch mikechoch commented Sep 6, 2019

This change is Reviewable

• Adds in-app messaging core classes
• The SDK currently only displays https://www.google.com as the content of in-app messages
• TODO: Build out triggers. Finalize the API spec
• Adds tests to ensure that OSInAppMessage objects can be parsed from JSON correctly using our new OSJSONDecodable protocol
• Adds WebKit listener code to handle actions from the HTML content of messages
• When a user taps an action, the SDK will alert delegates with the action ID and any arbitrary data associated with the action
• Decided that having a data parameter associated with actions would not provide much benefit and likely won't get implemented in 1.0
• Improves the constraints for the message view. Fixed an issue where the message view, on some devices, could be cut off.
• Improved the dummy message views
• Adds buttons to the demo app to display the different types of messages
• Adds support for local triggers to display in-app messages.
• The triggers are implemented as a 2D array, where the first level signifies OR conditions and the second level signifies AND conditions
• Adds a test to ensure that attempting to initialize a trigger with an invalid operator will not crash but will fail to initialize and will return nil
• Renames OSMessagingTriggerController to OSTriggerController
• Adds a new class (OSDynamicTrigger) to represent non-key/value triggers, such as session_duration
• Renames the initWithData() and initWithJson() methods of the OSJSONDecodable protocol that represents classes that are decodable from JSON
• Renamed these methods to class methods like instanceWithData(). This is cleaner, so that instead of having to use [[OSTrigger alloc] initWithData:data] you can instead use [OSTrigger instanceWithData:data]
• Adds a "contains" operator to allow developers to check if a message' trigger value exists in a local trigger array
• Adds macros to clean up type conversion code a bit.
• The compiler optimization level for the Tests target was previously defaulting to -O1
• The problem is that when any level of compiler optimization is enabled, the LLDB debugger usually loses the ability to resolve symbols/values at runtime
• Fixed by completely disabling all compiler optimization for the Tests target.
• We want the SDK to be able to support dynamic triggers, so that developers can specify that a message should be shown at a specific time or prevent messages from being shown at certain times
• Created an OSDynamicTriggerController class that can schedule timers when needed
• The logic looks a bit more complicated than one would expect, but there are reasons for this. For example, an in-app message may potentially use multiple time-based trigger conditions. We also want to make sure time-based triggers aren't even evaluated unless they are the only triggers that still apply (ie. if "gameScore" is required to be 30 for a message but is currently 20, there is no point in scheduling a timer for any additional time-based triggers until gameScore is set to 30+)
• In previous commits I hadn't considered that operators could be used with dynamic triggers. This commit adds support for using operators with dynamic triggers.
• Fixes a logic error with the dynamic trigger controller where the session duration was being calculated incorrectly (negative instead of positive)
• Adds a new parameter (max display time) to inapp messages so they are automatically dismissed after being shown for a certain amount of time.
• Implements a queue so that if multiple in-app messages are eligible to be shown, they are shown sequentially.
• The session duration test needs the session launch time to be reset between tests so that it can assume the session launch time is now (just launched).
• Checking SDK version is a supported trigger - added test to cover this scenario
• To prevent causing unintended issues with other tests, I've added logic to stop NSTimers from being created in the in-app message unit tests
• Adds a few integration tests for in-app messaging.
• The SDK mocks the server response for registration and then checks to make sure that the response (which has an in-app message with a dynamic duration-based trigger) correctly sets up a timer
• Adds an overrider class to mock the Message Controller's method that actually displays message UI.
• Adds a check to send in-app message data to the message controller if found in the response to registration
• Added a method to the OneSignalClientOverrider that allows us to mock API responses. This is currently implemented in a very simplistic way but I plan in the future to have a more comprehensive API mocking solution
• Fixes a mistake made when swizzling a method on OSMessagingController
• Adds additional tests to verify that messages actually get displayed, and that messages won't overlap if two messages are eligible to be displayed at the same time
• Makes our helper methods a bit clearer/easier to understand by moving them to their own class instead of being category methods implemented on OSInAppMessage
• Adds a test helper category on OSMessagingController to let tests set triggers and reset state (clear queue of displayed messages)
• Adds a new test that checks the full in-app message flow from registration JSON to making sure the message actually gets shown after a session_duration delay
• We already had an == operator. This commit adds an != operator and a test
• Adds support for filtering in-app messages using a device type trigger (for "phone" vs. "tablet"), added some tests as well
• Forgot to stage some files in the previous commit. Adds a PBXProject include reference for the Tablet/Phone detection trigger, adds support for the Not Equal To trigger
• Adds a new "not_exists" trigger operator and test
• Fixes an issue where the timer wasn't being properly initialized & added to the runloop in tests
• Adds a category method to OSMessagingController to make sure that state is reset between tests
Jeasmine and others added 26 commits August 9, 2019 10:38
* Banners now allow interaction with app behind them (don't take up full screen)
* Banners also do not dismiss on outside click and do not have a dark background
* Center modals and full screens take up entire screen and now have a scale intro animation instead of sliding
* Orientation changes are handled now, so the IAM only responds to landscape and portrait changes to and from
* Orientation change now hides the IAM and then reanimates the intro animation once the orientation is completely settled. This fixes the issue of seeing a background behind an image url while it is readjusting the messageView
* All IAMs now have corners and drop shadows for a better look
* Banners have a limited pan gesture Y and min/max dismiss time
* Other changes are constraint modifications, code cleanup, and commenting
* This is necessary so that during orientation change no strange UI issues are shown (blackouts)
* Interface changes to IAMs from previous work done already
* Banners now allow interaction with app behind them (don't take up full screen)
* Banners also do not dismiss on outside click and do not have a dark background
* Center modals and full screens take up entire screen and now have a scale intro animation instead of sliding
* Orientation changes are handled now, so the IAM only responds to landscape and portrait changes to and from
* Orientation change now hides the IAM and then reanimates the intro animation once the orientation is completely settled. This fixes the issue of seeing a background behind an image url while it is readjusting the messageView
* All IAMs now have corners and drop shadows for a better look
* Banners have a limited pan gesture Y and min/max dismiss time
* Other changes are constraint modifications, code cleanup, and commenting

* Removed clip bounds on IAM view on accident
* This is necessary so that during orientation change no strange UI issues are shown (blackouts)
* Add IAM Preview

* Fix click on image or button URL
…ivate into js_bridge

# Conflicts:
#	iOS_SDK/OneSignalSDK/Source/OSInAppMessageViewController.m
#	iOS_SDK/OneSignalSDK/Source/OSMessagingController.m
* Code cleanup and handling for dismissing current showing IAM
* When a preview IAM is incoming and a current IAM is showing, it will now be dismissed with its default animation off the screen and show the incoming preview IAM

* Added handling for Active and Resign Active state for IAM
* Some cases where this matters regard leaving the app from a button, body, or image click directing you to a browser. When dismissal is active, the current IAM will be dismissed and as you transition to the browser a new one (if queued) will try to show. When returning to the app from the browser in some cases, the IAM will be hidden and only show up when changing orientation of the phone (or other interface changes trigger UI refresh).
* Another case is panning the IAM and as it is panned somewhere other than the finalYConstraint and the user leaves the app, on returning to the app the IAM will stay in the previous panned location.
* Last case is leaving the app for a while and returning to the with an IAM on the screen. The URL for the IAM will reload in the WebView and cause the background to be shown.
* For all of these cases, the reanimation on reentry into the app solves these interface problems by basically resetting the IAM to what it would be if it just showed up into the app.

* Changed ResignActive observer to EnterBackground observer
* Found an issue where ResignActive was being triggered while pulling down notification tray
* EnterBackground is triggered when leaving the app, which is better scenario for resetting constraints for reentry animation
…ew (#15)

* Modified OSInAppMessage and add extra handling when showing IAM preview
* Removed previewUUID and replaced it with isPreview BOOL
* Added handling for when a new IAM preview is sent, which dismisses the current IAM if there is one and shows the preview

* Cleanup for dismissing current IAM
* Added 2 new methods for dismissing current IAM so that direction and velocity are now defaulted in some cases
* JS method getPageMetaData() crashes between JS and Objective C when trying to send over a DOMRect in the JSON. Changes were changed in backend and a PR has been created, once it is approved and merged the height can be adjusted for orientation changes.
* Methods created for webView adjustments during runtime so when orientation change occurs webView is removed from superview and fills screen and new height can be calculated using JS getPageMetaData(). Then the webView will be placed back inside the messageView with correct constraints set and height.
* Currently the method for handling new height calculation  during orientation change is commented out with a TODO until the JS code is updated to prevent crash.
* Add apns preview parse
… update (#18)

* Two main concerns in this commit
1) First thing is the height update by calling the JS getPageMetaData() method
* JS method is called to get new height based on resized WebView
* During orientation change all constraints are removed from the messageView and it is hidden from the view
* Orientation change could cause the text in the IAM to not wrap as many lines, therefore the WebView is resized to height of the device and width (with margins accounted for)
* Then the new height can be obtained and passed back to the messageVie where a completion callback is used to set new constraints

2) Second thing is the dash stats and only showing IAMs once (after dismissed)
* Similar to Android we have 3 sets (impression, clicks, and seen)
* These sets are cached in user defaults after any successful events regarding impressions (displayed IAM), clicks (button, body, image), or seen (dismissed IAM)
* Impressions and clicks use onFailure and onSuccess completion blocks to determine whether or not the event was successfully logged as a dashboard stat with a POST
* Seen is tracked by simply caching any IAMs that have been dismissed and is handled in the dismiss callback of the IAM controller delegate

* Now moving on to creating unit tests for impressions, clicks, and seen events for IAMs

* Some comments and code cleanup

* Added broken out method for impressions on a IAM
* This will allow for easier unit tests in future since it keeps the logic away from any UI issues
* Deleted cache logic for setting the _messagingEnabled
* Fix triggers custom

  * OS-2408

* Codereview comment
…o be merged into js_bridge (#22)

* Two main concerns in this commit
1) First thing is the height update by calling the JS getPageMetaData() method
* JS method is called to get new height based on resized WebView
* During orientation change all constraints are removed from the messageView and it is hidden from the view
* Orientation change could cause the text in the IAM to not wrap as many lines, therefore the WebView is resized to height of the device and width (with margins accounted for)
* Then the new height can be obtained and passed back to the messageVie where a completion callback is used to set new constraints

2) Second thing is the dash stats and only showing IAMs once (after dismissed)
* Similar to Android we have 3 sets (impression, clicks, and seen)
* These sets are cached in user defaults after any successful events regarding impressions (displayed IAM), clicks (button, body, image), or seen (dismissed IAM)
* Impressions and clicks use onFailure and onSuccess completion blocks to determine whether or not the event was successfully logged as a dashboard stat with a POST
* Seen is tracked by simply caching any IAMs that have been dismissed and is handled in the dismiss callback of the IAM controller delegate

* Now moving on to creating unit tests for impressions, clicks, and seen events for IAMs

* Some comments and code cleanup

* Added broken out method for impressions on a IAM
* This will allow for easier unit tests in future since it keeps the logic away from any UI issues
* Deleted cache logic for setting the _messagingEnabled

* Work done on the TriggerController and DynamicTriggerController (#20)

* Work done on the TriggerController and DynamicTriggerController
* A lot of clean up on the TriggerController for inconsistent behavior surrounding caching triggers
* IAM originally planned to be shown X number of times and this logic was being implemented, counted, and cached. This is now removed also.
* Changed parsing behavior for DynamicTriggerController bridge to get the "property" key's value, which is incorrect and should be grabbing the "kind" key's value
* Added proper handling for dynamic trigger kinds "session_duration" and "min_time_since" for IAMs

Misc code clean up, commenting, within all of the IAM files. Just creates more consistency and removes old outdated logic to match Android behavior.

* Added property field back into Trigger class
* Making sure pausing IAM and click action handler are implemented
* Previously pausing of IAM was called messageEnabled, this was inconsistent to Android and documentation. Also previous code was caching state of paused IAM, this is removed now and can only be set locally during app session defaulting back to false on app cold start up.
* Now click action handler has a setter which takes in a block similar to notification received and notification opened blocks
* Refactored code related to "messagingEnabled" now being refactored to "isInAppMessagingPaused"

* Unit test clean up for changes

* _isInAppMessagingPaused is synchronized
* Changed default setting by using the actual setter method "setInAppMessagingPaused()"

* Update OSMessagingController.h
* Fixing merging issues and inconsistencies after trigger work

* Merge caused property to be replaced by kind
* This is fixed now
* Field for pausing IAMs, removing trigger, getting trigger

Other code cleanup
* Merge caused trigger "kind" to replace "property". Fixed now and property value is working properly to show IAMs when triggers are satisfied
* Merge caused deletion of a few methods, but these issues are now fixed after looking at other commits before the merge
* Refactored pause in app messaging methods
* Changed color for demo app to red
* Added dismiss duration handling for IAMs
* Payload comes along with max display time and this will be used to set a timer and hide the IAM after that timer reaches the time set

Excluding iOS 9 and older from seeing IAMs and disabled in focus notification for IAM preview
* When the app is in background notification will be shown by the OS so this will need to be fixed by excluding specific versions from being valid IAM preview test devices on dashboard UI

Also added better system version checking and updated all methods using old check

* Two minor fixes for IAM
* Fixed duplicate message issue when leaving the app while 2 or more IAMs are in the message display queue. Trigger an on_session and they will be added twice because of improper contains check.
* Notifications showing for previews while in focus in app fixed

* Fixes from comments
* Fixed DummyOSMessagingController selectors removal
* Fixed iOS version checking

* Fixes from comments
* Fixed iOS version checking

* UI issue slipped through the cracks
* When locking the device or leaving the app and changing orientation and then reopening the app the IAM would have unexpected erros

In cases of banners the WebView would be gone and sometimes the IAM would be hidden
* This was due to constraints being set when backgrounding the app and the WebView is now never taken out of its superview but instead constraints are removed and re-added when necessary

In cases of center modals and full screens the WebView would retain its previous size and not chnage to the correct orientation change constraints

Most of this was fixed by modifying what constraints we adjust when backgrounding or coming into active states in the app
* Merge from master version 021001

* Update OneSignal.h

* A lot of clean up to get all tests passing with this merge
* A lot of conflicts around version checking, where the test method was not overriding the version check method because it was refactored. This is now fixed though and fixed a lot of tests.
* iOS runs tests at random, so sometimes tests fail when run all together, but once run individually they pass. This will most likely involve something being reset in beforeEachTest method in future.
* Removed feature for runtime push notification payload modification allowing somebody to create a block and modify the payload if they want before showing it to the user receiving it.

* Removed trigger.jsonRepresentation
* Added this when I was testing something, but should not be here

* Changed pauseInAppMessaging to pauseInAppMessages

* Few minor updates (#30)

* OneSignalHelperOverrider only needs to swizzle greater than or equal method and removed unnecessary methods from .h and .m
* Changed NSLog to OneSignal log in update external id request
* Also updated action click handler to print all variables of action out
# Conflicts:
#	OneSignal.podspec
#	OneSignalDynamic.podspec
#	iOS_SDK/OneSignalDevApp/OneSignalDevApp.xcodeproj/project.pbxproj
#	iOS_SDK/OneSignalDevApp/OneSignalDevApp/AppDelegate.m
#	iOS_SDK/OneSignalDevApp/OneSignalDevApp/Base.lproj/Main.storyboard
#	iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.h
#	iOS_SDK/OneSignalDevApp/OneSignalDevApp/ViewController.m
#	iOS_SDK/OneSignalSDK/Framework/Dynamic/OneSignal.framework/Versions/A/OneSignal
#	iOS_SDK/OneSignalSDK/Framework/OneSignal.framework/Versions/A/OneSignal
#	iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj
#	iOS_SDK/OneSignalSDK/Source/OneSignal.h
#	iOS_SDK/OneSignalSDK/Source/OneSignal.m
#	iOS_SDK/OneSignalSDK/Source/OneSignalHelper.m
#	iOS_SDK/OneSignalSDK/Source/OneSignalWebView.m
#	iOS_SDK/OneSignalSDK/Source/Requests.h
#	iOS_SDK/OneSignalSDK/Source/Requests.m
#	iOS_SDK/OneSignalSDK/Source/UNUserNotificationCenter+OneSignal.m
#	iOS_SDK/OneSignalSDK/UnitTests/RequestTests.m
#	iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.h
#	iOS_SDK/OneSignalSDK/UnitTests/Shadows/OneSignalClientOverrider.m
#	iOS_SDK/OneSignalSDK/UnitTests/UnitTests.m

Also added a new check for messageDisplayQueue so that no IAM is added twice
* NSString did not like the "==" comparison, isEqual built in method worked better
@mikechoch mikechoch requested a review from Jeasmine September 6, 2019 03:13
Copy link
Contributor

@Jeasmine Jeasmine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikechoch mikechoch merged commit 6a70518 into master Sep 9, 2019
@Jeasmine Jeasmine deleted the merge_021100_into_master branch May 12, 2020 22:01
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