Skip to content
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.

Sign up for GitHub

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

Jump to bottom

User model modularize Location and InAppMessages #1273

Merged
merged 25 commits into from
Jul 17, 2023
Merged

User model modularize Location and InAppMessages #1273

merged 25 commits into from
Jul 17, 2023

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Jun 21, 2023 •
edited
Loading

Description

One Line Summary

Move location and InAppMessages to their own framework

Details

  • Create separate frameworks for location and InAppMessages
  • OneSignalFramework will not directly depend on these relying on the App to include them.
  • The public api protocols for these have been moved to OneSignalCore along with stub implementations that will be used if the frameworks are not included.
  • This is currently using NSClassForString along with performSelector/NSInvocation to call into the libraries from the OneSignalFramework. This could be cleaned up in the future to use dependency injection instead.

Distribution

  • For Swift Package Manager we will create InAppMessaging and Location products
  • For Cocoapods we will create InAppMessaging and Location subspecs and not have the OneSignal spec depend on those subspecs. Then we should update our cocoapods instructions to NOT use Cocoapods for your NotificationServiceExtension target. This is due to the duplicate output issue Cocoapods has. Instead customers can use OneSignalExtension by either adding the XCFramework directly in the libraries and frameworks project section, or have their runpaths look for the framework in the app target. I need to test this strategy fully with an app store upload, but it has been successful through the archive step.

Motivation

Modularize IAMs and Location so that customers can exclude them from their apps. This will help with app store rejections due to these features.

Scope

  • InAppMessages: This code has moved to its own framework. It also has an indirect dependency on OneSignalLocation and uses NSInvocation to prompt for location.
  • Location: This code has moved to its own framework.
  • Migrations: In App Message migrations have been moved to the OneSignalInAppMessages framework.
  • Lifecycle controller: This class calls location onFocus and InAppMessages appDidBecomeActive
  • Sessions: Moved session time to OSSessionManager from OneSignal

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Unit testing

OPTIONAL - Explain unit tests added, if not clear in the code.

Manual testing

RECOMMEND - OPTIONAL - Explain what scenarios were tested and the environment.
Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

@emawby emawby requested a review from jkasten2 June 21, 2023 19:24
@emawby emawby changed the base branch from main to major_release_5.0.0 June 21, 2023 19:24
@emawby emawby force-pushed the user_model/modularize_location branch from 4cd20ab to b3f2f9e Compare June 30, 2023 19:27
@emawby emawby changed the title WiP User model modularize location and InAppMessages WiP User model modularize Location and InAppMessages Jul 10, 2023
@emawby emawby force-pushed the user_model/modularize_location branch 2 times, most recently from f3e075a to 41ef681 Compare July 12, 2023 20:00
emawby added 15 commits July 12, 2023 13:02
@emawby
Updating OneSignalInAppMessages imports
e2053bc
@emawby
Moving code out of OneSignalFramework for IAM module dependencies
9fd635e 
OneSignalInAppMessages now builds.
Removed dependencies on code in OneSignalHelper and OneSignalViewHelper.
Moved SessionTime to OSSessionManager
@emawby
Updating OneSignalFramework to use OneSignalInAppMessages framework
aed7c9a
@emawby
Build OneSignalFramework without dependency on location and IAM
73fee6f 
The public protocols for OSInAppMessages and OSLocation have been moved to OneSignalCore along with a stub implementation.
This is a PoC implementation using ClassFromString but we could use injection instead to get the concrete implementation of OneSignalLocation and OneSignalInAppMessages without using classfromstring
@emawby
Adding a helper method for NSInvocation to OneSignalCoreHelper and us…
4dba9dc 
…ing it for location
@emawby
Forward onApplicationDidBecomeActive to MessagingController through O…
a708527 
…neSignalInAppMessages
@emawby
Migrate IAMs in the IAM framework
9460834 
The Migration controller will now tell the OneSignalInAppMessages framework to do its migration. This way the MigrationController doesn't need to reference internal IAM classes.
@emawby
Add OneSignalInAppMessages to the dev app
781fff9
@emawby
Rename OneSignalLocation classes -> OneSignalLocationManager
c20a28c
@emawby
Updated podspec and build script for new modules
d128864
@emawby
test modularized binaries
9a4d7d8
@emawby
mark OSCore as extension safe
761f70d
@emawby
Fixing IAM location prompting
dd8bffc
@emawby
new test binaries after fixes
eeb1758
@emawby
Adding Location and InAppMessages SwiftPM products
a162f49
@emawby emawby force-pushed the user_model/modularize_location branch from 41ef681 to 441e361 Compare July 12, 2023 20:03
@emawby emawby changed the title WiP User model modularize Location and InAppMessages User model modularize Location and InAppMessages Jul 12, 2023
jkasten2
jkasten2 requested changes Jul 13, 2023
View reviewed changes
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.

Code changes themselves look good, however there are a number of wrong copyrights with "Hiptic" on new files that need to be corrected.
Branch also contained rebuilt frameworks which made it harder to read what changed.

iOS_SDK/OneSignalSDK/OneSignalCore/Source/OSStubLocation.m Outdated Show resolved Hide resolved
iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@emawby emawby requested a review from jkasten2 July 13, 2023 19:49
@emawby emawby force-pushed the user_model/modularize_location branch from 9947b17 to 62af317 Compare July 13, 2023 20:24
@emawby
Removing launchURLsInApp and webkit code from Notifications
8350017 
We are removing the launch push notification urls in a webview feature. This feature was not consistently effective, and added a webkit dependency to notifications that has gotten apps rejected from the app store. If we later want to add this feature back we should consider it as an optional module.
@emawby
Merge pull request #1275 from OneSignal/user_model/remove_launch_urls…
0dc4f15 
…_in_app

[User model] Remove launch urls in app
@emawby emawby merged commit 7e03af4 into major_release_5.0.0 Jul 17, 2023
@emawby emawby deleted the user_model/modularize_location branch July 17, 2023 16:43
This was referenced Jul 20, 2023
Merged
Merged
nan-li pushed a commit that referenced this pull request Oct 30, 2023
@emawby @nan-li
Merge pull request #1273 from OneSignal/user_model/modularize_location
e1100e0 
User model modularize Location and InAppMessages
nan-li pushed a commit that referenced this pull request Oct 30, 2023
@emawby @nan-li
Merge pull request #1273 from OneSignal/user_model/modularize_location
4d0780a 
User model modularize Location and InAppMessages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkasten2 jkasten2 jkasten2 approved these changes

@nan-li nan-li Awaiting requested review from nan-li

@shepherd-l shepherd-l Awaiting requested review from shepherd-l

@jennantilla jennantilla Awaiting requested review from jennantilla

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emawby @jkasten2