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

External user id not being sent fix #903

Merged
merged 2 commits into from
Mar 17, 2021
Merged

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Mar 16, 2021

This PR fixes two scenarios where we were not sending the external user id when we should be.

  1. When we have already set the external push user id, and are now trying to set the external email user id to the same value. For this we had a comment "If we are making a request to email user, we need validate both external user ids", but I am not sure why that is the case.
  2. When you have a cached external user id, but the player has been deleted from the dashboard and you are trying to set the external user id to the cached value

This change is Reviewable

@@ -148,6 +148,13 @@ - (void)registerUserWithState:(OSUserState *)registrationState withSuccess:(OSMu
//update push player id
if (results.count > 0 && results[OS_PUSH][@"id"]) {
self.currentSubscriptionState.userId = results[OS_PUSH][@"id"];

// Player record was deleted so we should reset external user id
let cachedPlayerId = [OneSignalUserDefaults.initStandard getSavedStringForKey:OSUD_PLAYER_ID_TO defaultValue:nil];
Copy link
Contributor

@Jeasmine Jeasmine Mar 17, 2021

Choose a reason for hiding this comment

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

When the player is deleted from the server on Android, we clean userId and all cached data for that player. Finally, re-trigger the player's creation. Maybe we want the same behavior here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We probably do, but I would like to release this fix quickly so I don't want to increase the scope too much. We are already clearing the external user id and player id (by setting it to the new value). Why do we need to re-trigger player creation?

@emawby emawby requested a review from Jeasmine March 17, 2021 18:09
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.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Jeasmine)

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.

None yet

3 participants