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(core): add method to put device token #170

Closed
wants to merge 1 commit into from
Closed

feat(core): add method to put device token #170

wants to merge 1 commit into from

Conversation

wkunert
Copy link
Contributor

@wkunert wkunert commented Apr 29, 2020

What does this PR do?
This PR adds a method to put device tokens which are utilized by some destinations (e.g. Customer.io). Setting device tokens (obtained from Firebase Cloud Messaging SDK) enables Customer.io to track devices via Segment which enables their push notification feature. I created two other pull requests (for the Android and iOS SDK) which needs to land before this could be merged: segmentio/analytics-android#655 segmentio/analytics-ios#881

How should this be manually tested?
Call analytics.putDeviceToken('i-am-a-device-token') and check in Segment’s event debugger if new events contain the token key within their context/device dictionary.

Any background context you want to provide?
We are using Firebase Cloud Messaging (via React Native Firebase). We want to use Customer.io to send out push notifications and they require the device type and token to be set in order to recognize devices.

What are the relevant tickets?
#144

Questions:

  • Does the docs need an update?
    I added comments, but the Segment website (Analytics for React Native) might also need one.
  • Are there any security concerns?
    I don't think so.
  • Do we need to update engineering / success?
    Maybe?

Before this lands the dependencies to the native SDKs need to be updated [1][2].

@bsneed
Copy link
Contributor

bsneed commented Apr 30, 2020

@nutlike I'm about to push a new release out. I've updated the dependencies to the latest release versions as you note in the PR. Is 4.5.0 and 3.8.0 sufficient? Is this PR ready other than that?

@wkunert
Copy link
Contributor Author

wkunert commented Apr 30, 2020

The versions for the dependencies depend on the version number you will be giving to the respective SDKs after my other pull requests are merged and released ([1], [2]). So 4.5.0 for Android and 3.8.0 for iOS will not suffice since they are lacking needed functionality – other than that this PR is ready.

@bsneed
Copy link
Contributor

bsneed commented Apr 30, 2020

@nutlike can you tell me which versions are sufficient then? Those are both the latest release versions for both platforms, the only thing newer is 4.0 for iOS but I know that wouldn't be blocking this.

@bsneed
Copy link
Contributor

bsneed commented Apr 30, 2020

@nutlike OH! I see. You need those other PRs merged first. Makes sense now.

@wkunert
Copy link
Contributor Author

wkunert commented Apr 30, 2020

@bsneed That's correct, I was already in the process drafting a reply. 😄

@bsneed
Copy link
Contributor

bsneed commented Apr 30, 2020

@nutlike looked at those two PRs in the context of this one. I'm not sure I can merge those. My concern is that the nomenclature feels a little like the device tokens one would use on those platforms re push notifications, but it's a separate thing. If the goal is to insert fields into the context object, we do have a mechanism for this, which is middleware. We have a simpler way coming on the roadmap, but it's not ready quite yet.

@wkunert
Copy link
Contributor Author

wkunert commented Apr 30, 2020

@bsneed Thanks for taking the time. I do not fully comprehend what you are trying to say so I will try to explain my reasoning:

The Android SDK pull request is mirroring a functionality which already exists in the iOS SDK (adding the device type to the device context) – so that should be fine to merge (I don’t see a reason why Android/iOS should behave different).

The iOS SDK pull request is mirroring a functionality which already exists in the Android SDK (putting a device token) – I do understand that the iOS SDK does have a similar functionality already (registeredForRemoteNotificationsWithDeviceToken) but it only works when people are using APNs directly. When people are using Firebase Cloud Messaging (which is imho quite reasonable when using React Native) this approach does not work – the token has a different format. But it is still a token used for push notifications – but messages will be send via FCM not plain APNs. I am successfully sending push notifications from Customer.io with the FCM tokens I sent over through these modifications. So the pull request (and its nomenclature) is definitely related to push notifications: It adds the ability to use Firebase Cloud Messaging with the iOS SDK.

I hope that makes it a bit more clear. I am convinced that this is a good feature to have and that others will benefit from it (for example the people opening the tickets I linked in the PRs). Let me know what you think – and if (and how) there is a way to bring this functionality to upstream: would be a shame if I would have to maintain a fork for these changes for the foreseeable future. 😉

@wkunert
Copy link
Contributor Author

wkunert commented Apr 30, 2020

@bsneed Thanks for pointing out middlewares – I see how the same could be achieved using them (and I might do that instead of maintaining a fork if this PR will be rejected). But I think it is a good thing when the Android and iOS SDKs behave the same (if possible) and their functionality is made accessible in the React Native SDK; in my opinion this lowers the entry barrier for your customers (out-of-the-box solution vs. self-made middleware).

@bsneed
Copy link
Contributor

bsneed commented Apr 30, 2020

@nutlike No problem! I agree with your point, 100%. The feature I mentioned that isn't there yet will allow you to insert user-defined data into Context, such that you could do something like this ...

myContext = { "myData" : { "key1": "value1" } }
analytics.setUserData(myContext)

And that myData value would appear in every event thereafter. That seems like it would satisfy your use case even better than middleware. When we do this, it will be across all 3 platforms. Middleware is a bit high in terms of lift required to implement, given that the above is typically what most customers want to do.

@Brendobrien
Copy link

Hello @bsneed, the application I'm working is exactly as @nutlike described. I need to forward my FCM token to Customer IO through Segment. However, I can't do what's recommended in the docs (attach the device token to a Application Installed, Application Uninstalled, or Application Opened event) since I hand off that functionality using the trackAppLifecycleEvents: true option within the setup config (https://segment.com/docs/connections/destinations/catalog/customer-io/#device-token-set-up). Additionally, my device token logic would occur later in my application's lifecycle.

It would be great to use @nutlike's suggestion in a way like this:

messaging()
    .getToken()
    .then(fcmToken => {
      if (fcmToken) {
        if (isDev || isTestFlight) {
          console.log('[debug] fcmToken: ', fcmToken);
        }
        // user has a device token
        analytics.putDeviceToken(fcmToken)
      } else {
        // user doesn't have a device token yet
      }
    })

Thanks! Let me know your thoughts. I'm hoping to implement something ASAP.

@SMJ93
Copy link

SMJ93 commented May 26, 2020

Hey @Brendobrien, did you manage to send your FCM token to customer.io? I need to do something similar

@SMJ93
Copy link

SMJ93 commented May 26, 2020

Hey @nutlike, any update on this?

@wkunert
Copy link
Contributor Author

wkunert commented May 26, 2020

Hey @SMJ93, no news from my side. My implementation works for me but it seems unlikely to me that it will get merged – you can either wait for the new feature @bsneed mentioned above, use my forks (but I probably won't merge upstream regularly) or figure out how to achieve the same with middlewares. I haven't dug into using middlewares yet (other things came in between) but I would like to check them out in the near future, this comment should be a good starting point.

@harrigee
Copy link

@bsneed Hello. I am exactly at the same point. I am using fcm and want to forward the fcm token towards customer.io. Currently there is no good official way in doing so. Also I can not get it working when using the middleware. It would be awesome if we could find a way to make that happen.

@harrigee
Copy link

harrigee commented Aug 18, 2020

In order to send the token / device to customer.io I am now using a combination of the Middlware and replicating the lifecycle events. Though this feels not very good - it is at least working for now without changing the Library. Also I am able to anonymize the ip address for my custom lifecycle events now.

This ist the middleware:

  private readonly getDeviceInfoForSegment = (): JsonValue => {
    return {
      token: FCM_TOKEN,
      type: Platform.OS,
      ...
    }
  }

  segmentAnalytics.middleware((payload) => {
      const { next, context } = payload;
      context.ip = '0.0.0.0';
      context.device = this.getDeviceInfoForSegment();
      next(context);
    });

And these are the lifecylce events I implemented to make this happen:

  // This event is to send the push token to customer.io via segment, because segment`s native lifecycle events can not do so
  public segmentTrackApplicationOpened = () => {
    return segmentAnalytics.track('Application Opened');
  }

  // This event is to send the push token to customer.io via segment, because segment`s native lifecycle events can not do so
  public segmentTrackApplicationBackgrounded = () => {
    return segmentAnalytics.track('Application Backgrounded');
  }

  // This event is to send the push token to customer.io via segment, because segment`s native lifecycle events can not do so
  public segmentTrackApplicationInstalled = (versionNumber: string, buildNumber: string): Promise<void> => {
    return segmentAnalytics.track('Application Installed', {
      version: versionNumber,
      build: buildNumber
    });
  }

  // This event is to send the push token to customer.io via segment, because segment`s native lifecycle events can not do so
  public segmentTrackApplicationUpdated = (
    versionNumber: string,
    buildNumber: string,
    previousVersionNumber: string,
    previousBuildNumber: string
  ): Promise<void> => {
    return segmentAnalytics.track('Application Updated', {
      previous_version: previousVersionNumber,
      previous_build: previousBuildNumber,
      version: versionNumber,
      build: buildNumber
    });
  }

Though I am currently missing the uninstall lifecycle event, finally my fcm tokens are appearing as devices in customer.io. I hope there will be a better official solution soon.

@kelvinmontini
Copy link

@harrigee I implemented some like your code using middleware and it works, but I have a question about the correct moments to call the lifecycle events.
As I understood you are doing some overrides on segment lifecycle events, but I didn't find how I can do it.
Do you have any recommendations, references, or examples?

@phuochau
Copy link

phuochau commented Jan 9, 2021

I can confirm with the version 1.3.2. If we use the middleware approach, it will lost all "context.device" from native.

Middleware

analytics.middleware((payload) => {
      const { next, context } = payload;
      if (!context.device) { // always goes there because in JS, we don't see the device information in the context
           context.device = {
                token: "{{your_device_token}}"
           }
      }
      next(context);
    });

Orignal context.device

"device": {
      "adTrackingEnabled": true,
      "advertisingId": "44ae7535-0099-4c8c-8dcd-dffd2654fd0b",
      "id": "42e0fd3a67ab7b8e",
      "manufacturer": "Xiaomi",
      "model": "Redmi Note 3",
      "name": "kenzo",
      "type": "android"
    },

After used the middleware


"device": {
      "token": "{{your_device_token}}",
      "type": "android"
    },

So without the advertisingId, the event will not be tracked in Adjust. Are there any else solution? Thank you

@bsneed
Copy link
Contributor

bsneed commented Mar 18, 2021

Closing this. Further investigation shows neither this solution or middleware are necessary.

https://segment.com/docs/connections/sources/catalog/libraries/mobile/react-native/#react-native-push-notifications-on-ios

Following the steps shown there will get a value set to device.token. Simply calling registeredForRemoteNotificationsWithDeviceToken: will do the trick and device.token will be set even if you don't need all the other things mentioned there. Segment doesn't care what the value is, we just throw it in the payload as requested.

I would also recommend disabling lifecycle events and responding to them on your own since they happen out of sync on the iOS side and might miss the token addition.

@bsneed bsneed closed this Mar 18, 2021
@xseignard
Copy link

Hello @bsneed
Indeed your solution works for iOS but we're left with no working solution for Android.
What do you recommend for Android x react-native?

@xseignard
Copy link

Hi @bsneed any updates?

@cdedreuille
Copy link

We are using React Native with Expo SDK 45. As we build our app with EAS we don't have access to the ios or android folder. The link you shared above @bsneed doesn't seem to give any indication on how to set up registeredForRemoteNotificationsWithDeviceToken. In our case I don't think it works neither for android.

Any update on how to solve this?

@bsneed
Copy link
Contributor

bsneed commented May 4, 2022

@alanjcharles @oscb ^

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.

9 participants