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

[Bug]: Using OneSignal.login() does not set externalId in Subscription Records #2043

Closed
1 task done
sebastinto opened this issue Apr 4, 2024 · 9 comments
Closed
1 task done

Comments

@sebastinto
Copy link

sebastinto commented Apr 4, 2024

What happened?

Calling OneSignal.login() does not seem to be updating Subscription Records i.e. the External ID column values are unset. OneSignal.User.externalId is correctly set on device.

I tried on a fresh install, multiple device, calling OneSignal.logout() before OneSignal.login() to no avail. I'm wondering if there's anything I may be missing.

Tags are set correctly.

Steps to reproduce?

1. Initialize SDK
2. Call OneSignal.login() with a value (e.g. "1234")
3. Observe Subscription Records External ID does not update

What did you expect to happen?

External ID updates in Subscription Records.

OneSignal Android SDK version

Release 5.1.7

Android version

13

Specific Android models

* Pixel 8 Pro
* Samsung S20 5G

Relevant log output

os_log.txt

Code of Conduct

  • I agree to follow this project's Code of Conduct
@jkasten2
Copy link
Member

jkasten2 commented Apr 4, 2024

@sebastinto thanks for reporting, I have a few follow up questions.

  1. Where are you calling OneSignal.initWithContext and OneSignal.login in your code?
  2. "Observe Subscription Records External ID does not update" where are you observing this? On the OneSignal dashboard?
  3. Does the issue happen if you simply call OneSignal.initWithContext then OneSignal.login?
    • Or does the issue only happen if you call logout()?
  4. The os_log.txt you shared includes a number of other calls, logout(), clearAllNotifications(), addTags(), and requestPermission(), can you share a log with the minimum operations required to reproduce the issue?

@sebastinto
Copy link
Author

Thanks @jkasten2 !

  1. Where are you calling OneSignal.initWithContext and OneSignal.login in your code?

OneSignal.initWithContext is called in the onCreate() function of the main Application class. OneSignal.login is called after a user successfully authenticates via our API.

  1. "Observe Subscription Records External ID does not update" where are you observing this? On the OneSignal dashboard?

Correct!

  1. Does the issue happen if you simply call OneSignal.initWithContext then OneSignal.login?
    • Or does the issue only happen if you call logout()?

Removing logout() fixes the issue.

I have a few follow-up questions if you don't mind:

  1. Is this expected behavior? Why would calling logout() prevent subsequent login() calls?
  2. It seems like calling login() with a different value is enough to override external id. Is this the recommended implementation?
  3. If it is, what would be the use case for calling logout()? Explicitly de-identifying a user? How would you re-identify a user after calling logout()?

@jkasten2
Copy link
Member

jkasten2 commented Apr 4, 2024

Removing logout() fixes the issue.

Good to hear! However that is bug as logout() shouldn't cause an issue like this. Could you explain step-by-step what the order of timing of the login() and logout() calls that cause the issue? Include any delays or if they are called back-to-back.

  1. Is this expected behavior? Why would calling logout() prevent subsequent login() calls?

It seems to be a bug, the repo steps noted above will be helpful to us.

  1. It seems like calling login() with a different value is enough to override external id. Is this the recommended implementation?

Yes, this usage is something we design for, if you call login with a different id it simply switches users.

If it is, what would be the use case for calling logout()? Explicitly de-identifying a user? How would you re-identify a user after calling logout()?

The only reason to call logout is if you have that functionally in your app. What OneSignal does is create a new anonymous User and move the push subscription to it. This way the subscription has been dissociated from the user that was logged in.

@sebastinto
Copy link
Author

Dig a little bit more digging and it seems like calling logout() before login() may not be the issue after all. Sorry about that.

External IDs do update in the OneSignalDashboard if they do not already exist even when logout() is called before login() (which may happen back-to-back, a short second apart to answer your question).

However calling login() and passing the value of an existing External ID (i.e. one that can already be found in the OneSignal Dashboard), results in a new entry that has a blank External ID value.

It might be worth mentioning that we recently replaced the Service Account.json with one from a different Firebase project, but I just rolled back to the previous version of the OneSignal SDK that we were using (4.8.4) and existing External IDs do update as expected in the OneSignal Dashboard.

Yes, this usage is something we design for, if you call login with a different id it simply switches users.

Nice, good to know it is an option.

The only reason to call logout is if you have that functionally in your app. What OneSignal does is create a new anonymous User and move the push subscription to it. This way the subscription has been dissociated from the user that was logged in.

That makes sense. We do have that functionality in the app. Users can explicitly log out / delete local data / analytics id.

@jkasten2
Copy link
Member

jkasten2 commented Apr 5, 2024

Thanks for all the details, I have undercovered some cases we are not handling correctly and we will improve on this in the next release.

@sebastinto
Copy link
Author

Thanks for all the details, I have undercovered some cases we are not handling correctly and we will improve on this in the next release.

Interesting! I'll definitely keep an eye out for an update, thanks!

@jkasten2
Copy link
Member

@sebastinto we released 5.1.8 which should address this issue. Thanks for the details you provided here!

Let us know if the issue is completely gone after this update or not.

@sebastinto
Copy link
Author

@sebastinto we released 5.1.8 which should address this issue. Thanks for the details you provided here!

Let us know if the issue is completely gone after this update or not.

That was fast!💪

I'm OOF until next week but will test when I'm back and report back. Thanks!

@sebastinto
Copy link
Author

@sebastinto we released 5.1.8 which should address this issue. Thanks for the details you provided here!

Let us know if the issue is completely gone after this update or not.

Fix confirmed! Thanks!

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

No branches or pull requests

2 participants