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.

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

feat: register/delete device tokens #26

Merged
merged 42 commits into from
Sep 10, 2021
Merged

Conversation

hownowstephen
Copy link
Contributor

@hownowstephen hownowstephen commented Aug 31, 2021

#19

Adds support for registering device tokens with the MessagingPush base class, adds the MessagingPushAPNS class as a thin wrapper, for usage like:

func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data) {
   self.messagingPush.registerDeviceToken(deviceToken: deviceToken)
}

we will only keep the device token in-memory, per the apple docs to avoid weird issues with backups etc

Important

Never cache device tokens in local storage. APNs issues a new token when the user restores a device from a backup, when the user installs your app on a new device, and when the user reinstalls the operating system. If you ask the system to provide the token each time, you’re guaranteed to get an up-to-date token.

multiple calls to registerDeviceToken currently only overwrite the active token in memory, I need to research further if there's a reasonable way of determining when a device token is revoked

@hownowstephen hownowstephen marked this pull request as ready for review August 31, 2021 20:34
@levibostian levibostian changed the title Register/Delete device tokens feat: register/delete device tokens Aug 31, 2021
@levibostian levibostian force-pushed the identify-customer-forreal branch 2 times, most recently from dd2a0f0 to 8484485 Compare September 3, 2021 17:10
Sources/MessagingPush/MessagingPush.swift Outdated Show resolved Hide resolved
Sources/MessagingPush/MessagingPush.swift Outdated Show resolved Hide resolved
Sources/MessagingPush/Service/Request/Device.swift Outdated Show resolved Hide resolved
Sources/MessagingPushAPN/MessagingPushAPN.swift Outdated Show resolved Hide resolved
Sources/MessagingPush/MessagingPush.swift Outdated Show resolved Hide resolved
Swift code goes into this module that are common to *all* of the Messaging Push modules (APN, FCM, etc).
So, performing an HTTP request to the API with a device token goes here.
*/
open class MessagingPush {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that you linked to apple docs stating they recommend you do not persist the device token.

However, since we are storing it in memory, do we know for sure that APN will call didRegisterForRemoteNotificationsWithDeviceToken for us the next time that the app is restarted?

I am nervous that there will be a scenario where a device token is valid for a device but deviceToken is still nil in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from my reading, as long as the app calls registerForRemoteNotificationTypes, the didRegisterForRemoteNotificationsWithDeviceToken delegate will get called every time it is opened, also gets called on settings change as well. I vote we stick with the Apple best practice for the time being and then do a few tests. Actually persisting the device token is also pretty trivial, so if we have any concern after testing that the token doesn't show up or something then we can raise it at that point.

https://stackoverflow.com/questions/10983536/when-is-didregisterforremotenotificationswithdevicetoken-called

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree to follow what they mention. I have never tested this myself before (I have always used a background queue in an app that persisted this data for me) so I am not sure on the behavior.

QA will help assert this is true.

I am working with QA right now to help them test the behavior of the SDK better. I am thinking we create log statements throughout the SDK (this is a feature of the SDK for customers, not just QA so helping everyone!) and so QA can check the logs of the RH app and see "device token registered" when app starts up. Then we can confirm that this fact is true.

Tests/MessagingPush/MessagingPushTest.swift Show resolved Hide resolved
Tests/MessagingPush/MessagingPushTest.swift Outdated Show resolved Hide resolved
Tests/MessagingPush/MessagingPushTest.swift Show resolved Hide resolved
Tests/MessagingPush/MessagingPushTest.swift Show resolved Hide resolved
Base automatically changed from identify-customer-forreal to identify-customer September 9, 2021 20:49
Base automatically changed from identify-customer to alpha September 9, 2021 21:06
Comment on lines +30 to +50
private func pushSetup() -> MessagingPush{
let identifyRepository = CIOIdentifyRepository(httpClient: httpClientMock, keyValueStorage: DITracking.shared.keyValueStorage, jsonAdapter: jsonAdapter, siteId: String.random)
let cio = CustomerIO(credentialsStore: SdkCredentialsStoreMock(), sdkConfig: SdkConfig(), identifyRepository: identifyRepository, keyValueStorage: nil)

cio.credentials = SdkCredentials(siteId: String.random,
apiKey: String.random,
region: Region.EU)

httpClientMock.requestClosure = { params, onComplete in
onComplete(Result.success(Data()))
}

let identifier: String? = String.random


cio.identify(identifier: identifier!) { result in
guard case .success = result else { return XCTFail() }
XCTAssertEqual(cio.identifier, identifier)
}

return MessagingPush(customerIO: cio, httpClient: httpClientMock, jsonAdapter: jsonAdapter)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be improved when I perform the refactors that I have mentioned a couple of times. Dependency injection is not not really setup the best at the moment which makes creating mocks and instances harder. This is a good workaround for now.

@hownowstephen hownowstephen merged commit d0ddc07 into alpha Sep 10, 2021
@hownowstephen hownowstephen deleted the register-device-token-apn branch September 10, 2021 16:46
github-actions bot pushed a commit that referenced this pull request Sep 10, 2021
# [1.0.0-alpha.5](1.0.0-alpha.4...1.0.0-alpha.5) (2021-09-10)

### Features

* register/delete device tokens ([#26](#26)) ([d0ddc07](d0ddc07))
@ami-ci
Copy link
Contributor

ami-ci commented Sep 10, 2021

🎉 This PR is included in version 1.0.0-alpha.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@levibostian levibostian linked an issue Sep 10, 2021 that may be closed by this pull request
4 tasks
github-actions bot pushed a commit that referenced this pull request Dec 15, 2021
# 1.0.0-beta.1 (2021-12-15)

### Bug Fixes

* call callback on main thread APN tokens ([#40](#40)) ([982ce9d](982ce9d))
* convert APN device token to string ([#39](#39)) ([1f64a13](1f64a13))
* deep links previously being ignored ([#79](#79)) ([2041767](2041767))
* improve user-agent with more detail ([#74](#74)) ([4301034](4301034))
* logs now show up in mac console app ([#80](#80)) ([535d0be](535d0be))
* opened push metrics ([#70](#70)) ([a277378](a277378))
* remove apn device token profile request body ([#41](#41)) ([61946c4](61946c4))
* rename singletons instance to shared ([#34](#34)) ([3bf1384](3bf1384))
* rename stop identify function ([#31](#31)) ([d97e931](d97e931))

### Features

* automatic push events ([#63](#63)) ([cf81b23](cf81b23))
* create mocks for push messaging ([#35](#35)) ([b2c5d62](b2c5d62))
* event tracking ([#42](#42)) ([be768bb](be768bb))
* identify customer ([#27](#27)) ([f79d1c9](f79d1c9))
* make US default region ([#28](#28)) ([1d10a8f](1d10a8f))
* register device token with FCM ([#46](#46)) ([f6a87e0](f6a87e0))
* register/delete device tokens ([#26](#26)) ([d0ddc07](d0ddc07))
* rich push deep links ([#45](#45)) ([fe21cc8](fe21cc8))
* rich push images ([#59](#59)) ([03fc284](03fc284))
* save all siteids registered with SDK ([#30](#30)) ([95db6dc](95db6dc))
* track push events ([#47](#47)) ([a37e60e](a37e60e))
@ami-ci
Copy link
Contributor

ami-ci commented Dec 15, 2021

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

ami-ci pushed a commit that referenced this pull request Jan 19, 2022
# 1.0.0 (2022-01-19)

### Bug Fixes

* add createdAt timestamp to added queue tasks  ([#106](#106)) ([46aab62](46aab62))
* automatic screenview tracking correct siteid ([#120](#120)) ([abd3ea9](abd3ea9))
* background queue timer scheduling and running ([#114](#114)) ([6be8a74](6be8a74))
* call callback on main thread APN tokens ([#40](#40)) ([982ce9d](982ce9d))
* change hostname for CIO API ([#109](#109)) ([90e9407](90e9407))
* convert APN device token to string ([#39](#39)) ([1f64a13](1f64a13))
* deep links previously being ignored ([#79](#79)) ([2041767](2041767))
* duplicate entries for active workspace ([#124](#124)) ([c903e4a](c903e4a))
* improve user-agent with more detail ([#74](#74)) ([4301034](4301034))
* logs now show up in mac console app ([#80](#80)) ([535d0be](535d0be))
* more safely handle 5xx, 401 status codes ([#107](#107)) ([d56807b](d56807b))
* mutex locks shared across instances ([#119](#119)) ([cb169bf](cb169bf))
* opened push metrics ([#70](#70)) ([a277378](a277378))
* opened push metrics being sent to API again ([#111](#111)) ([93971bf](93971bf))
* remove apn device token profile request body ([#41](#41)) ([61946c4](61946c4))
* remove custom jsonencoder public functions ([#122](#122)) ([061a568](061a568))
* rename singletons instance to shared ([#34](#34)) ([3bf1384](3bf1384))
* rename stop identify function ([#31](#31)) ([d97e931](d97e931))
* screen view tracking ([#108](#108)) ([f836b9a](f836b9a))
* track events type in HTTP requests ([#117](#117)) ([745d4ad](745d4ad))

### Features

* add [String:Any] support to identify & track ([#94](#94)) ([5a220c4](5a220c4))
* add screen view tracking ([#82](#82)) ([c2034a6](c2034a6))
* automatic push events ([#63](#63)) ([cf81b23](cf81b23))
* create background queue  ([#99](#99)) ([80fffb8](80fffb8))
* create mocks for push messaging ([#35](#35)) ([b2c5d62](b2c5d62))
* event tracking ([#42](#42)) ([be768bb](be768bb))
* identify customer ([#27](#27)) ([f79d1c9](f79d1c9))
* make US default region ([#28](#28)) ([1d10a8f](1d10a8f))
* register device token with FCM ([#46](#46)) ([f6a87e0](f6a87e0))
* register/delete device tokens ([#26](#26)) ([d0ddc07](d0ddc07))
* rich push deep links ([#45](#45)) ([fe21cc8](fe21cc8))
* rich push images ([#59](#59)) ([03fc284](03fc284))
* save all siteids registered with SDK ([#30](#30)) ([95db6dc](95db6dc))
* subsequent identifies without passing identifier ([#96](#96)) ([47d9166](47d9166))
* track push events ([#47](#47)) ([a37e60e](a37e60e))
@ami-ci
Copy link
Contributor

ami-ci commented Jan 19, 2022

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Register APN device token for push notifications
3 participants