[Feat] Use only OneSignal ID for requests #1464

nan-li · 2024-07-24T22:56:01Z

Description

One Line Summary

Use only OneSignal ID for requests, no longer using any External ID for requests.

Details

Fetch user will only occur with OneSignal ID, no more EUID
Any user updates will only occur with OneSignal ID, no more EUID
Remove Transfer Subscription Request except to uncache on upgrade only
- Still relevant Transfer Subscription request will be converted into an equivalent Create User request
- Cached Transfer Subscription requests should be rare - in order for this to be cached, it means the Identify User request returned 409 but the immediate Transfer Subscription did not send or resulted in a retry-able error response
Identify User success and conflict responses result in different behavior now
- Both will call Create User instead of marking pending requests to use EUID or fetching and transferring the push subscription

Motivation

Move away from using External ID in request URLs, only using OneSignal ID.

Scope

Making requests

Testing

Unit testing

No new unit test are added, but existing unit tests are updated

Manual testing

Device: iPhone 13 on iOS 17.5.1

Identify User with success (external ID applied successfully)

Start with anonymous user in the SDK
Add a tag and an email
Login to a new user (brand new external ID)
The Identify User request is successful
Fetch User by OSID happens
Local state is correct and contains the tag and email

Identify User with 409 conflict

Start with anonymous user and login to an existing EUID
Receive 409 conflict response
If the user is the current user, it will make a CreateUser request with the external ID and push subscription in the payload
If the user is now different, it will make a CreateUser request with the external ID only for hydration of the OneSignal ID, for any pending requests for that previous user

SDK Upgrade: There is a cached Transfer Subscription for the current user

Reproduce this on main by login to an existing EUID
The Identify User happens and receives 409 conflict response
The subsequent Fetch User by EUID and Transfer Subscription are generated and cached, but do not get sent
Upgrade SDK to this version
During uncaching, the Fetch User is dropped and the Transfer Subscription is converted to a CreateUser containing the external ID and push subscription.
User observer fires and everything looks correct.

SDK Upgrade: There is a cached Transfer Subscription for a previous user

Reproduce this on main by login to an existing EUID userA
The Identify User happens and receives 409 conflict response
The subsequent Fetch User by EUID and Transfer Subscription are generated and cached, but do not get sent
Now login to another user userB, which enqueues a CreateUser request
Upgrade SDK to this version
During uncaching, the Fetch User is dropped and the Transfer Subscription is dropped, only the CreateUser remains
The CreateUser is sent and the state in the SDK seems correct for userB.

Affected code checklist

Checklist

Overview

I have filled out all REQUIRED sections above
PR does one thing
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.
I have reviewed this PR myself, ensuring it meets each checklist item

This change is

* The SDK will longer make Transfer Subscription calls now that requests via external ID are removed, and all requests are expected to use OneSignal ID. * The Transfer Subscription request was previously used in an Identify User 409 conflict to transfer the push subscription to that External ID. * Instead these will translate to Create User which will contain the External ID and the push subscription in the payload. * Deprecate this request class but keep a skeleton in order to uncache them

* The initializer for the Request has optional PropertiesModel and optional SubscriptionModel * This use case will send an external ID in the payload to retrieve the OneSignal ID.

* After a successful `CreateUser` or `IdentifyUser` without conflict, fetch the user for hydration via OneSignal ID instead of External ID.

* On Identify User 409 conflict response, no longer fetch user by External ID nor transfer subscription. * Instead, make a call to CreateUser with the push subscription in the payload. * If the user has changed since then, make a CreateUser call with just the Identity Model to hydrate the OneSignal ID for any pending updates that need to be sent for the past user. * Nit: remove an extraneous error log. The SDK already logs that it handles the 409 response. And the Client will already log this error as well.

* On Identify User successful, the SDK was only hydrating the OneSignal ID for past users (not the current user) and fetching the current user (which means OneSignal ID will be missing locally until that response) * Let's hydrate both OneSignal ID and External ID for all cases so that the local state can be up to date (instead of relying on the Fetch User to hydrate back the OneSignal and External IDs)

All user-update related requests will also only use OneSignal ID in the path * OSRequestUpdateProperties * OSRequestIdentifyUser * OSRequestAddAliases * OSRequestRemoveAlias * OSRequestCreateSubscription

* Now that requests will always use OneSignal ID, remove the concept of a primary alias for an Identity Model (ie: OneSignal ID or External ID)

* No more transfer subscription * All requests using EUID previously now use OSID * After a 409 conflict, there can be a follow-up CreateUser request (whose purpose is to retrieve the OneSignal ID) and a Fetch User request

emawby · 2024-07-25T21:46:10Z

iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift

+     */
+    static func createUser(identityModel: OSIdentityModel) {
+        guard identityModel.externalId != nil else {
+            OneSignalLog.onesignalLog(.LL_ERROR, message: "createUser(identityModel) called with missing external ID")


If we hit this case what happens? Will we try again in the same session?

The purpose of this version of ceateUser() is to get an OSID for an EUID. The EUID must exist because the only usage of this method is after an Identify User 409 conflict, in which case we would have the EUID. The guard and error log is more for noticing that we do something wrong such as using this method inappropriately down the line.

Thinking about this again, it may be clearer to make EUID a parameter instead of it being implicit.

No, it won't ever retry, which means any pending updates for a previous user who experienced this particular flow will get updates dropped.

Ya it should probably require it as a param. Would it retry next session?

I edited my response after to talk about retrying. This call would be made if the user has changed since, so this would be for a previous user. It won't retry since no EUID, no way to get its OSID. That means if there are pending requests for this previous user, they will effectively be dropped.

* The 2nd use of create user is passing an external ID to hydrate the OSID. Let's make it explicit by using the alias as a parameter and creating an additional initializer for the Create User Request. * Here I also removed encoding and decoding of the `path` property. This should not be decoded as it may not be encoded yet. The `prepareForExecution` will always generate the path. Other requests follow this pattern already.

* After a Create User is successful, we should fetch the user for hydration. There was a bug introduced in an earlier commit that checked the request payload for the OSID, when it should be retrieved from the server response

jkasten2 · 2024-07-26T18:36:52Z

iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift

-    /**
-     Set either `onesignal_id` or `external_id`, representing the alias that will be used in requests.
-     */
-    var primaryAliasLabel: OSDefaultAlias = .onesignal_id


We will probably want to use this concept for JWT, since we will make requests with external_id when it is enabled. I am still ok with removing this code in this PR to if you want to, just making a note in-case this didn't come up.

@jkasten2 I thought about JWT before removing this. The issue with the primaryAliasLabel as it exists here is that every Identity Model needs to manage it. Therefore, when the SDK first detects JWT is on, it needs to go and update every Identity Model. Instead, some flag can be centralized.

The reason this primaryAliasLabel existed on the Identity Models is that some models can use external ID instead of onesignal ID.

nan-li added 8 commits July 24, 2024 13:37

Use Create User to hydrate OneSignal ID

e656417

* The initializer for the Request has optional PropertiesModel and optional SubscriptionModel * This use case will send an external ID in the payload to retrieve the OneSignal ID.

Fetch User by OSID, not external ID

454909d

* After a successful `CreateUser` or `IdentifyUser` without conflict, fetch the user for hydration via OneSignal ID instead of External ID.

All requests will only use OneSignal ID

5f43d4f

All user-update related requests will also only use OneSignal ID in the path * OSRequestUpdateProperties * OSRequestIdentifyUser * OSRequestAddAliases * OSRequestRemoveAlias * OSRequestCreateSubscription

Remove concept of primary alias

4ed7a64

* Now that requests will always use OneSignal ID, remove the concept of a primary alias for an Identity Model (ie: OneSignal ID or External ID)

[tests] Update tests

589bc34

* No more transfer subscription * All requests using EUID previously now use OSID * After a 409 conflict, there can be a follow-up CreateUser request (whose purpose is to retrieve the OneSignal ID) and a Fetch User request

nan-li requested review from jkasten2, emawby, jinliu9508, shepherd-l and jennantilla July 25, 2024 16:26

emawby self-assigned this Jul 25, 2024

emawby reviewed Jul 25, 2024

View reviewed changes

nan-li added 2 commits July 25, 2024 16:53

Fix a bug to fetch user after create user is made

bc9112c

* After a Create User is successful, we should fetch the user for hydration. There was a bug introduced in an earlier commit that checked the request payload for the OSID, when it should be retrieved from the server response

nan-li requested a review from emawby July 25, 2024 23:56

emawby approved these changes Jul 25, 2024

View reviewed changes

jkasten2 reviewed Jul 26, 2024

View reviewed changes

jkasten2 approved these changes Jul 26, 2024

View reviewed changes

nan-li changed the title ~~Use OneSignal ID for requests~~ [Feat] Use only OneSignal ID for requests Aug 2, 2024

nan-li merged commit d419c8e into main Aug 2, 2024
3 of 4 checks passed

nan-li deleted the feat/no_more_external_id_based_requests branch August 2, 2024 00:21

This was referenced Aug 9, 2024

Release 5.2.3 #1469

Merged

Release 5.2.3 OneSignal/OneSignal-XCFramework#96

Merged

This was referenced Aug 15, 2024

Release 5.2.4 OneSignal/OneSignal-Cordova-SDK#1013

Merged

Release 5.2.3 OneSignal/OneSignal-Flutter-SDK#933

Merged

Release 5.2.3 OneSignal/react-native-onesignal#1731

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat] Use only OneSignal ID for requests #1464

[Feat] Use only OneSignal ID for requests #1464

nan-li commented Jul 24, 2024 •

edited

Loading

emawby Jul 25, 2024

nan-li Jul 25, 2024 •

edited

Loading

emawby Jul 25, 2024

nan-li Jul 25, 2024

jkasten2 Jul 26, 2024

nan-li Jul 31, 2024 •

edited

Loading

[Feat] Use only OneSignal ID for requests #1464

[Feat] Use only OneSignal ID for requests #1464

Conversation

nan-li commented Jul 24, 2024 • edited Loading

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

emawby Jul 25, 2024

Choose a reason for hiding this comment

nan-li Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

emawby Jul 25, 2024

Choose a reason for hiding this comment

nan-li Jul 25, 2024

Choose a reason for hiding this comment

jkasten2 Jul 26, 2024

Choose a reason for hiding this comment

nan-li Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

nan-li commented Jul 24, 2024 •

edited

Loading

nan-li Jul 25, 2024 •

edited

Loading

nan-li Jul 31, 2024 •

edited

Loading