From 3754360396f843cc4da8a989bd3a65e8b4201a43 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 10:45:46 -0700 Subject: [PATCH 1/9] [nits] string descriptions of Requests --- .../OneSignalUser/Source/Requests/OSRequestFetchUser.swift | 4 ++-- .../OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift | 4 ++-- .../Source/Requests/OSRequestTransferSubscription.swift | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestFetchUser.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestFetchUser.swift index f9f1fe210..e8911ca5b 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestFetchUser.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestFetchUser.swift @@ -58,7 +58,7 @@ class OSRequestFetchUser: OneSignalRequest, OSUserRequest { self.aliasLabel = aliasLabel self.aliasId = aliasId self.onNewSession = onNewSession - self.stringDescription = "" + self.stringDescription = "" super.init() self.method = GET _ = prepareForExecution() // sets the path property @@ -88,7 +88,7 @@ class OSRequestFetchUser: OneSignalRequest, OSUserRequest { self.aliasLabel = aliasLabel self.aliasId = aliasId self.onNewSession = coder.decodeBool(forKey: "onNewSession") - self.stringDescription = "" + self.stringDescription = "" super.init() self.method = HTTPMethod(rawValue: rawMethod) self.timestamp = timestamp diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift index 90dcb0ea0..5572efe05 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift @@ -72,7 +72,7 @@ class OSRequestIdentifyUser: OneSignalRequest, OSUserRequest { self.identityModelToUpdate = identityModelToUpdate self.aliasLabel = aliasLabel self.aliasId = aliasId - self.stringDescription = "" + self.stringDescription = "" super.init() self.parameters = ["identity": [aliasLabel: aliasId]] self.method = PATCH @@ -106,7 +106,7 @@ class OSRequestIdentifyUser: OneSignalRequest, OSUserRequest { self.identityModelToUpdate = identityModelToUpdate self.aliasLabel = aliasLabel self.aliasId = aliasId - self.stringDescription = "" + self.stringDescription = "" super.init() self.timestamp = timestamp self.parameters = parameters diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestTransferSubscription.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestTransferSubscription.swift index 57bacec4e..e9040e5ae 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestTransferSubscription.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestTransferSubscription.swift @@ -66,7 +66,7 @@ class OSRequestTransferSubscription: OneSignalRequest, OSUserRequest { self.subscriptionModel = subscriptionModel self.aliasLabel = aliasLabel self.aliasId = aliasId - self.stringDescription = "OSRequestTransferSubscription" + self.stringDescription = "" super.init() self.parameters = ["identity": [aliasLabel: aliasId]] self.method = PATCH @@ -97,7 +97,7 @@ class OSRequestTransferSubscription: OneSignalRequest, OSUserRequest { self.subscriptionModel = subscriptionModel self.aliasLabel = aliasLabel self.aliasId = aliasId - self.stringDescription = "OSRequestTransferSubscription" + self.stringDescription = "" super.init() self.parameters = parameters self.method = HTTPMethod(rawValue: rawMethod) From 7684aec478e30cdd843586cef4ac846d77c84c14 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 10:56:32 -0700 Subject: [PATCH 2/9] [tests] Client will default to async request handling * This is more realistic as real requests have a delay * This change was motivated by this flow: anon -> login (identify user) -> login (create user) * When the requests ran synchronously, when the identify user request returned, the "current user" was still itself. --- .../OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift b/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift index daf1aaef3..0f8c12365 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalCoreMocks/MockOneSignalClient.swift @@ -35,7 +35,7 @@ public class MockOneSignalClient: NSObject, IOneSignalClient { public var lastHTTPRequest: OneSignalRequest? public var networkRequestCount = 0 public var executedRequests: [OneSignalRequest] = [] - public var executeInstantaneously = true + public var executeInstantaneously = false var remoteParamsResponse: [String: Any]? var shouldUseProvisionalAuthorization = false // new in iOS 12 (aka Direct to History) From a1ce32c0ac7c380e2b0dd9ee8163ef128b06f439 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 11:03:25 -0700 Subject: [PATCH 3/9] [tests] Add more mocked user responses * Create some default helper methods --- .../OneSignalUserMocks/MockUserDefines.swift | 2 + .../OneSignalUserMocks/MockUserRequests.swift | 118 ++++++++++++++++++ 2 files changed, 120 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserDefines.swift b/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserDefines.swift index 452c4c192..86ccd5823 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserDefines.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserDefines.swift @@ -3,3 +3,5 @@ public let userA_OSID = "test_user_a_onesignal_id" public let userA_EUID = "test_user_a_external_id" public let userB_OSID = "test_user_b_onesignal_id" public let userB_EUID = "test_user_b_external_id" + +public let testPushSubId = "test_push_subscription_id" diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserRequests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserRequests.swift index fbc57ee59..474fe9a51 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserRequests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserMocks/MockUserRequests.swift @@ -1,4 +1,5 @@ import OneSignalCore +import OneSignalCoreMocks public class MockUserRequests { @@ -15,4 +16,121 @@ public class MockUserRequests { "properties": properties ] } + + public static func testDefaultPushSubPayload(id: String) -> [String: Any] { + return [ + "id": testPushSubId, + "app_id": "test-app-id", + "type": "iOSPush", + "token": "", + "enabled": true, + "notification_types": 80, + "session_time": 0, + "session_count": 1, + "sdk": "test_sdk_version", + "device_model": "iPhone14,3", + "device_os": "17.4.1", + "rooted": false, + "test_type": 1, + "app_version": "1.4.4", + "net_type": 0, + "carrier": "", + "web_auth": "", + "web_p256": "" + ] + } + + public static func testDefaultFullCreateUserResponse(onesignalId: String, externalId: String?, subscriptionId: String?) -> [String: Any] { + let identity = testIdentityPayload(onesignalId: onesignalId, externalId: externalId) + let subscription = testDefaultPushSubPayload(id: testPushSubId) + let properties = [ + "language": "en", + "timezone_id": "America/Los_Angeles", + "country": "US", + "first_active": 1714860182, + "last_active": 1714860182, + "ip": "xxx.xx.xxx.xxx" + ] as [String: Any] + + return [ + "subscriptions": [subscription], + "identity": identity["identity"]!, + "properties": properties + ] + } +} + +// MARK: - Set Up Default Client Responses + +extension MockUserRequests { + private static func getOneSignalId(for externalId: String) -> String { + switch externalId { + case userA_EUID: + return userA_OSID + case userB_EUID: + return userB_OSID + default: + return UUID().uuidString + } + } + + public static func setDefaultCreateAnonUserResponses(with client: MockOneSignalClient) { + let anonCreateResponse = testDefaultFullCreateUserResponse(onesignalId: anonUserOSID, externalId: nil, subscriptionId: testPushSubId) + + client.setMockResponseForRequest( + request: "", + response: anonCreateResponse) + } + + public static func setDefaultCreateUserResponses(with client: MockOneSignalClient, externalId: String) { + let osid = getOneSignalId(for: externalId) + + let userResponse = MockUserRequests.testIdentityPayload(onesignalId: osid, externalId: externalId) + + client.setMockResponseForRequest( + request: "", + response: userResponse + ) + client.setMockResponseForRequest( + request: "", + response: userResponse + ) + } + + public static func setDefaultIdentifyUserResponses(with client: MockOneSignalClient, externalId: String, conflicted: Bool = false) { + var osid: String + var fetchResponse: [String: [String: String]] + + // 1. Set the response for the Identify User request + if conflicted { + osid = getOneSignalId(for: externalId) + fetchResponse = MockUserRequests.testIdentityPayload(onesignalId: osid, externalId: externalId) + client.setMockFailureResponseForRequest( + request: "", + error: NSError(domain: "not-important", code: 409) + ) + } else { + // The Identify User is successful, the OSID is unchanged + osid = anonUserOSID + fetchResponse = MockUserRequests.testIdentityPayload(onesignalId: osid, externalId: externalId) + client.setMockResponseForRequest( + request: "", + response: fetchResponse + ) + } + // 2. Set the response for the subsequent Fetch User request + client.setMockResponseForRequest( + request: "", + response: fetchResponse + ) + } + + public static func setAddTagsResponse(with client: MockOneSignalClient, tags: [String: String]) { + let tagsResponse = MockUserRequests.testPropertiesPayload(properties: ["tags": tags]) + + client.setMockResponseForRequest( + request: "", + response: tagsResponse + ) + } } From 6b6c0f2bdfbe5fc7212378d4f41ffd4867a7353a Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 11:07:49 -0700 Subject: [PATCH 4/9] [tests] Add test to reproduce Identify User when in middle bug * Before the fix in this PR, this test would fail because tags for user A do not get sent --- .../OneSignalUserTests.swift | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index c3b5d3ce5..e69e59dd4 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -216,4 +216,56 @@ final class OneSignalUserTests: XCTestCase { contains: ["properties": ["tags": tagsUserB]]) ) } + + /** + Motivation: We had a bug where we did not hydrate the middle user's OSID, so any pending updates got dropped. + */ + func testIdentifyUserWithConflict_whenNotCurrentUser_sendsCorrectTags() throws { + /* Setup */ + + let client = MockOneSignalClient() + OneSignalCoreImpl.setSharedClient(client) + + // 1. Set up mock responses for the anonymous user + MockUserRequests.setDefaultCreateAnonUserResponses(with: client) + + // 2. Set up mock responses for User A with 409 conflict response + let tagsUserA = ["tag_a": "value_a"] + MockUserRequests.setDefaultIdentifyUserResponses(with: client, externalId: userA_EUID, conflicted: true) + MockUserRequests.setAddTagsResponse(with: client, tags: tagsUserA) + + // 3. Set up mock responses for User B + let tagsUserB = ["tag_b": "value_b"] + MockUserRequests.setDefaultCreateUserResponses(with: client, externalId: userB_EUID) + MockUserRequests.setAddTagsResponse(with: client, tags: tagsUserB) + + /* When */ + + // 1. Login to user A and add tag + OneSignalUserManagerImpl.sharedInstance.login(externalId: userA_EUID, token: nil) + OneSignalUserManagerImpl.sharedInstance.addTag(key: "tag_a", value: "value_a") + + // 2. Login to user B and add tag + OneSignalUserManagerImpl.sharedInstance.login(externalId: userB_EUID, token: nil) + OneSignalUserManagerImpl.sharedInstance.addTag(key: "tag_b", value: "value_b") + + // 3. Run background threads + OneSignalCoreMocks.waitForBackgroundThreads(seconds: 0.5) + + /* Then */ + + // Assert that every request SDK makes has a response set, and is handled + XCTAssertTrue(client.allRequestsHandled) + + // Assert there is only one request containing these tags and they are sent to userA + XCTAssertTrue(client.onlyOneRequest( + contains: "apps/test-app-id/users/by/onesignal_id/\(userA_OSID)", + contains: ["properties": ["tags": tagsUserA]]) + ) + // Assert there is only one request containing these tags and they are sent to userB + XCTAssertTrue(client.onlyOneRequest( + contains: "apps/test-app-id/users/by/onesignal_id/\(userB_OSID)", + contains: ["properties": ["tags": tagsUserB]]) + ) + } } From 318a0a90a2d9474bc92cc4ee864b2097c892ba02 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 8 May 2024 15:11:20 -0700 Subject: [PATCH 5/9] [tests] update project files to build successfully * OneSignalUserMocks needed a dependency on XCTest apparently * Because it depends on OneSignalCoreMocks now, who uses XCTest --- iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj index 935ddc58f..23e19a7c4 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj @@ -116,6 +116,8 @@ 3CA283A92B86A30400097465 /* OneSignalCore.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = DE7D17E627026B95002D3A5D /* OneSignalCore.framework */; }; 3CA283AA2B86A30400097465 /* OneSignalCore.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = DE7D17E627026B95002D3A5D /* OneSignalCore.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; }; 3CA6CE0A28E4F19B00CA0585 /* OSUserRequest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3CA6CE0928E4F19B00CA0585 /* OSUserRequest.swift */; }; + 3CA8B8822BEC2FCB0010ADA1 /* XCTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 3C7A39D42B7C18EE0082665E /* XCTest.framework */; }; + 3CA8B8832BEC2FCB0010ADA1 /* XCTest.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = 3C7A39D42B7C18EE0082665E /* XCTest.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; }; 3CC063942B6D6B6B002BB07F /* OneSignalCore.m in Sources */ = {isa = PBXBuildFile; fileRef = 3CC063932B6D6B6B002BB07F /* OneSignalCore.m */; }; 3CC063A22B6D7A8E002BB07F /* OneSignalCoreMocks.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 3CC0639A2B6D7A8C002BB07F /* OneSignalCoreMocks.framework */; }; 3CC063A72B6D7A8E002BB07F /* OneSignalCoreTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3CC063A62B6D7A8E002BB07F /* OneSignalCoreTests.swift */; }; @@ -1022,6 +1024,7 @@ dstSubfolderSpec = 10; files = ( 3CEE93542B7C78EC008440BD /* OneSignalUser.framework in Embed Frameworks */, + 3CA8B8832BEC2FCB0010ADA1 /* XCTest.framework in Embed Frameworks */, 3CEE934F2B7C787B008440BD /* OneSignalOSCore.framework in Embed Frameworks */, ); name = "Embed Frameworks"; @@ -1569,6 +1572,7 @@ buildActionMask = 2147483647; files = ( 3CEE93532B7C78EC008440BD /* OneSignalUser.framework in Frameworks */, + 3CA8B8822BEC2FCB0010ADA1 /* XCTest.framework in Frameworks */, 3CEE934E2B7C787B008440BD /* OneSignalOSCore.framework in Frameworks */, ); runOnlyForDeploymentPostprocessing = 0; From af10704b8725aad37fe55e8b343f27a90cc02418 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 11:31:03 -0700 Subject: [PATCH 6/9] [tests] Fix existing test case: testSwitchUser_sendsCorrectTags * Fix an assert about where a set of tags is sent * This test previously succeeded because all the requests are processed synchronously, but after we introduced async client requests, this test failed because it was not the correct actual behavior * Also use the new client response setting helpers (no logic change) --- .../OneSignalUserTests.swift | 46 ++++--------------- 1 file changed, 9 insertions(+), 37 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index e69e59dd4..82f97dd1d 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -144,46 +144,17 @@ final class OneSignalUserTests: XCTestCase { let client = MockOneSignalClient() // 1. Set up mock responses for the anonymous user - let anonCreateResponse = MockUserRequests.testIdentityPayload(onesignalId: anonUserOSID, externalId: nil) - - client.setMockResponseForRequest( - request: "", - response: anonCreateResponse) + MockUserRequests.setDefaultCreateAnonUserResponses(with: client) // 2. Set up mock responses for User A let tagsUserA = ["tag_a": "value_a"] - let createUserA = MockUserRequests.testIdentityPayload(onesignalId: userA_OSID, externalId: userA_EUID) - let tagsResponseUserA = MockUserRequests.testPropertiesPayload(properties: ["tags": tagsUserA]) - - client.setMockResponseForRequest( - request: "", - response: createUserA - ) - client.setMockResponseForRequest( - request: "", - response: createUserA - ) - client.setMockResponseForRequest( - request: "", - response: tagsResponseUserA - ) + MockUserRequests.setDefaultIdentifyUserResponses(with: client, externalId: userA_EUID) + MockUserRequests.setAddTagsResponse(with: client, tags: tagsUserA) // 3. Set up mock responses for User B let tagsUserB = ["tag_b": "value_b"] - let createUserB = MockUserRequests.testIdentityPayload(onesignalId: userB_OSID, externalId: userB_EUID) - let tagsResponseUserB = MockUserRequests.testPropertiesPayload(properties: ["tags": tagsUserB]) - - client.setMockResponseForRequest( - request: "", - response: createUserB - ) - client.setMockResponseForRequest( - request: "", - response: createUserB - ) - client.setMockResponseForRequest( - request: "", - response: tagsResponseUserB) + MockUserRequests.setDefaultCreateUserResponses(with: client, externalId: userB_EUID) + MockUserRequests.setAddTagsResponse(with: client, tags: tagsUserB) OneSignalCoreImpl.setSharedClient(client) @@ -205,9 +176,10 @@ final class OneSignalUserTests: XCTestCase { // Assert that every request SDK makes has a response set, and is handled XCTAssertTrue(client.allRequestsHandled) - // Assert there is only one request containing these tags and they are sent to userA + // Assert there is only one request containing these tags and they are sent to the Anon User + // This is because the Identify User request succeeded, so the user remains the same XCTAssertTrue(client.onlyOneRequest( - contains: "apps/test-app-id/users/by/onesignal_id/\(userA_OSID)", + contains: "apps/test-app-id/users/by/onesignal_id/\(anonUserOSID)", contains: ["properties": ["tags": tagsUserA]]) ) // Assert there is only one request containing these tags and they are sent to userB @@ -216,7 +188,7 @@ final class OneSignalUserTests: XCTestCase { contains: ["properties": ["tags": tagsUserB]]) ) } - + /** Motivation: We had a bug where we did not hydrate the middle user's OSID, so any pending updates got dropped. */ From 420f78c08b55c3f8ddb4e8e28efcf8f73e55b791 Mon Sep 17 00:00:00 2001 From: Nan Date: Mon, 6 May 2024 11:42:54 -0700 Subject: [PATCH 7/9] [Bug] Fix Identify User processing if the user has changed * Motivation: consider the scenario Anon -> Login (Identify User) -> Make Updates like Tags -> Login (Create User) * When the Identify User request returns, because the user is now different, we were not hydrating the previous user's OSID, so updates made to it were dropped. Now we still fetch the user to hydrate the OSID. * In addition, transfer subscription had a bug. What would happen is after the last Create User is sent with the device's push subscription in the payload, the Transfer Subscription request would come next and move it back to the middle user. Now we only transfer if it's still the same user. Any Create User requests after would already have the push subscription in it. --- .../Source/Executors/OSUserExecutor.swift | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index a4bc005f1..513ce8503 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -329,6 +329,10 @@ class OSUserExecutor { if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { fetchUser(aliasLabel: OS_EXTERNAL_ID, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) } else { + // Need to hydrate the identity model for any pending requests + if let osid = request.identityModelToIdentify.onesignalId { + request.identityModelToUpdate.hydrate([OS_ONESIGNAL_ID: osid]) + } executePendingRequests() } } onFailure: { error in @@ -340,12 +344,13 @@ class OSUserExecutor { OneSignalLog.onesignalLog(.LL_DEBUG, message: "executeIdentifyUserRequest returned error code user-2. Now handling user-2 error response... switch to this user.") removeFromQueue(request) - // Fetch the user only if its the current user + // Transfer the push subscription only if it's the current user if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { - fetchUser(aliasLabel: OS_EXTERNAL_ID, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) - // TODO: Link ^ to the new user... what was this todo for? + transferPushSubscriptionTo(aliasLabel: request.aliasLabel, aliasId: request.aliasId) } - transferPushSubscriptionTo(aliasLabel: request.aliasLabel, aliasId: request.aliasId) + // Need to still hydrate the identity model for any pending requests + // TODO: After implementing JWT, instead of fetching just to hydrate the OSID for pending requests, maybe we can update the external_id on the Identity Model and set the alias to use for pending requests be the external_id + fetchUser(aliasLabel: OS_EXTERNAL_ID, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) } else if responseType == .invalid || responseType == .unauthorized { // Failed, no retry removeFromQueue(request) From 42a6a04e09426503414c762b503e64f374690e53 Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 7 May 2024 13:19:52 -0700 Subject: [PATCH 8/9] [nit] more clear client response logging * See the URL that the response was for --- iOS_SDK/OneSignalSDK/OneSignalCore/Source/API/OneSignalClient.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalCore/Source/API/OneSignalClient.m b/iOS_SDK/OneSignalSDK/OneSignalCore/Source/API/OneSignalClient.m index 0c83011dc..d9066e347 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalCore/Source/API/OneSignalClient.m +++ b/iOS_SDK/OneSignalSDK/OneSignalCore/Source/API/OneSignalClient.m @@ -203,7 +203,7 @@ - (void)handleJSONNSURLResponse:(NSURLResponse*)response data:(NSData*)data erro if (data != nil && [data length] > 0) { innerJson = [NSJSONSerialization JSONObjectWithData:data options:NSJSONReadingMutableContainers error:&jsonError]; innerJson[@"httpStatusCode"] = [NSNumber numberWithLong:statusCode]; - [OneSignalLog onesignalLog:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"network response (%@): %@", NSStringFromClass([request class]), innerJson]]; + [OneSignalLog onesignalLog:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"network response (%@) with URL %@: %@", NSStringFromClass([request class]), request.urlRequest.URL.absoluteString, innerJson]]; if (jsonError) { if (failureBlock != nil) failureBlock([NSError errorWithDomain:@"OneSignal Error" code:statusCode userInfo:@{@"returned" : jsonError}]); From 9f156d74853c017cdd2c60b34cf923d19acf6ced Mon Sep 17 00:00:00 2001 From: Nan Date: Tue, 7 May 2024 14:31:46 -0700 Subject: [PATCH 9/9] Avoid a fetch user after login returns 409 * When Identify User returns 409, there may be pending requests for that user. * Avoid another user fetch just to hydrate the OSID, and instead use the external_id that we have for this user to send those pending requests. * Also, the `fetchUser` is meant only for the current user, as it clears local state for hydration. We avoid refactoring that method as well. * Add new concept of "default alias" that lives on an Identity Model and indicates what alias should be used for requests. --- .../Source/Executors/OSUserExecutor.swift | 10 ++++---- .../Source/OSIdentityModel.swift | 23 +++++++++++++++++++ .../Source/Requests/OSRequestAddAliases.swift | 5 ++-- .../OSRequestCreateSubscription.swift | 5 ++-- .../Requests/OSRequestIdentifyUser.swift | 7 +++--- .../Requests/OSRequestRemoveAlias.swift | 5 ++-- .../Requests/OSRequestUpdateProperties.swift | 5 ++-- .../OneSignalUserTests.swift | 6 ++--- 8 files changed, 48 insertions(+), 18 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 513ce8503..a65ed16d0 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -344,13 +344,15 @@ class OSUserExecutor { OneSignalLog.onesignalLog(.LL_DEBUG, message: "executeIdentifyUserRequest returned error code user-2. Now handling user-2 error response... switch to this user.") removeFromQueue(request) - // Transfer the push subscription only if it's the current user + // Transfer the push subscription, and fetch only if it's the current user if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + fetchUser(aliasLabel: OS_EXTERNAL_ID, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) transferPushSubscriptionTo(aliasLabel: request.aliasLabel, aliasId: request.aliasId) + } else { + // Use external_id for any pending requests, avoiding a fetch to hydrate onesignal_id + request.identityModelToUpdate.primaryAliasLabel = .external_id + executePendingRequests() } - // Need to still hydrate the identity model for any pending requests - // TODO: After implementing JWT, instead of fetching just to hydrate the OSID for pending requests, maybe we can update the external_id on the Identity Model and set the alias to use for pending requests be the external_id - fetchUser(aliasLabel: OS_EXTERNAL_ID, aliasId: request.aliasId, identityModel: request.identityModelToUpdate) } else if responseType == .invalid || responseType == .unauthorized { // Failed, no retry removeFromQueue(request) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift index 6e70b5057..639e47c50 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSIdentityModel.swift @@ -29,7 +29,23 @@ import Foundation import OneSignalCore import OneSignalOSCore +// By matching the enum name to the raw value, it will always stringify correctly +enum OSDefaultAlias: String { + // swiftlint:disable identifier_name + case onesignal_id = "onesignal_id" + case external_id = "external_id" + // swiftlint:enable identifier_name +} + class OSIdentityModel: OSModel { + /** + Set either `onesignal_id` or `external_id`, representing the alias that will be used in requests. + */ + var primaryAliasLabel: OSDefaultAlias = .onesignal_id + var primaryAliasId: String? { + return if primaryAliasLabel == .external_id { externalId } else { onesignalId } + } + var onesignalId: String? { return internalGetAlias(OS_ONESIGNAL_ID) } @@ -57,6 +73,7 @@ class OSIdentityModel: OSModel { aliasesLock.withLock { super.encode(with: coder) coder.encode(aliases, forKey: "aliases") + coder.encode(primaryAliasLabel.rawValue, forKey: "primaryAliasLabel") // Encodes as String } } @@ -66,6 +83,12 @@ class OSIdentityModel: OSModel { // log error return nil } + if let rawType = coder.decodeObject(forKey: "primaryAliasLabel") as? String, + let label = OSDefaultAlias(rawValue: rawType) { + self.primaryAliasLabel = label + } else { + self.primaryAliasLabel = .onesignal_id + } self.aliases = aliases } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestAddAliases.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestAddAliases.swift index d70ca8a6f..3af53122e 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestAddAliases.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestAddAliases.swift @@ -39,9 +39,10 @@ class OSRequestAddAliases: OneSignalRequest, OSUserRequest { // requires a `onesignal_id` to send this request func prepareForExecution() -> Bool { - if let onesignalId = identityModel.onesignalId, let appId = OneSignalConfigManager.getAppId() { + let aliasLabel = identityModel.primaryAliasLabel + if let aliasId = identityModel.primaryAliasId, let appId = OneSignalConfigManager.getAppId() { self.addJWTHeader(identityModel: identityModel) - self.path = "apps/\(appId)/users/by/\(OS_ONESIGNAL_ID)/\(onesignalId)/identity" + self.path = "apps/\(appId)/users/by/\(aliasLabel)/\(aliasId)/identity" return true } else { // self.path is non-nil, so set to empty string diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestCreateSubscription.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestCreateSubscription.swift index 6128c475b..f75adcc31 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestCreateSubscription.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestCreateSubscription.swift @@ -44,9 +44,10 @@ class OSRequestCreateSubscription: OneSignalRequest, OSUserRequest { // Need the onesignal_id of the user func prepareForExecution() -> Bool { - if let onesignalId = identityModel.onesignalId, let appId = OneSignalConfigManager.getAppId() { + let aliasLabel = identityModel.primaryAliasLabel + if let aliasId = identityModel.primaryAliasId, let appId = OneSignalConfigManager.getAppId() { self.addJWTHeader(identityModel: identityModel) - self.path = "apps/\(appId)/users/by/\(OS_ONESIGNAL_ID)/\(onesignalId)/subscriptions" + self.path = "apps/\(appId)/users/by/\(aliasLabel)/\(aliasId)/subscriptions" return true } else { self.path = "" // self.path is non-nil, so set to empty string diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift index 5572efe05..320d78b6e 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestIdentifyUser.swift @@ -48,14 +48,15 @@ class OSRequestIdentifyUser: OneSignalRequest, OSUserRequest { // requires a onesignal_id to send this request func prepareForExecution() -> Bool { - if let onesignalId = identityModelToIdentify.onesignalId, let appId = OneSignalConfigManager.getAppId() { + let aliasLabel = identityModelToIdentify.primaryAliasLabel + if let aliasId = identityModelToIdentify.primaryAliasId, let appId = OneSignalConfigManager.getAppId() { self.addJWTHeader(identityModel: identityModelToIdentify) - self.path = "apps/\(appId)/users/by/\(OS_ONESIGNAL_ID)/\(onesignalId)/identity" + self.path = "apps/\(appId)/users/by/\(aliasLabel)/\(aliasId)/identity" return true } else { // self.path is non-nil, so set to empty string self.path = "" - OneSignalLog.onesignalLog(.LL_DEBUG, message: "Cannot generate the Identify User request due to null app ID or null OneSignal ID.") + OneSignalLog.onesignalLog(.LL_DEBUG, message: "Cannot generate the Identify User request due to null app ID or null \(aliasLabel) ID.") return false } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestRemoveAlias.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestRemoveAlias.swift index 49e6ea691..ac4343dde 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestRemoveAlias.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestRemoveAlias.swift @@ -38,9 +38,10 @@ class OSRequestRemoveAlias: OneSignalRequest, OSUserRequest { var identityModel: OSIdentityModel func prepareForExecution() -> Bool { - if let onesignalId = identityModel.onesignalId, let appId = OneSignalConfigManager.getAppId() { + let aliasLabel = identityModel.primaryAliasLabel + if let aliasId = identityModel.primaryAliasId, let appId = OneSignalConfigManager.getAppId() { self.addJWTHeader(identityModel: identityModel) - self.path = "apps/\(appId)/users/by/\(OS_ONESIGNAL_ID)/\(onesignalId)/identity/\(labelToRemove)" + self.path = "apps/\(appId)/users/by/\(aliasLabel)/\(aliasId)/identity/\(labelToRemove)" return true } else { // self.path is non-nil, so set to empty string diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateProperties.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateProperties.swift index 91290cead..5348fbbee 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateProperties.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateProperties.swift @@ -39,11 +39,12 @@ class OSRequestUpdateProperties: OneSignalRequest, OSUserRequest { // TODO: Decide if addPushSubscriptionIdToAdditionalHeadersIfNeeded should block. // Note Android adds it to requests, if the push sub ID exists func prepareForExecution() -> Bool { - if let onesignalId = identityModel.onesignalId, + let aliasLabel = identityModel.primaryAliasLabel + if let aliasId = identityModel.primaryAliasId, let appId = OneSignalConfigManager.getAppId() { _ = self.addPushSubscriptionIdToAdditionalHeaders() self.addJWTHeader(identityModel: identityModel) - self.path = "apps/\(appId)/users/by/\(OS_ONESIGNAL_ID)/\(onesignalId)" + self.path = "apps/\(appId)/users/by/\(aliasLabel)/\(aliasId)" return true } else { // self.path is non-nil, so set to empty string diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 82f97dd1d..6f7e6e3cf 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -213,7 +213,7 @@ final class OneSignalUserTests: XCTestCase { /* When */ - // 1. Login to user A and add tag + // 1. Login to user A (will result in 409 conflict) and add tag OneSignalUserManagerImpl.sharedInstance.login(externalId: userA_EUID, token: nil) OneSignalUserManagerImpl.sharedInstance.addTag(key: "tag_a", value: "value_a") @@ -229,9 +229,9 @@ final class OneSignalUserTests: XCTestCase { // Assert that every request SDK makes has a response set, and is handled XCTAssertTrue(client.allRequestsHandled) - // Assert there is only one request containing these tags and they are sent to userA + // Assert only one request containing these tags and they are sent to userA by external_id XCTAssertTrue(client.onlyOneRequest( - contains: "apps/test-app-id/users/by/onesignal_id/\(userA_OSID)", + contains: "apps/test-app-id/users/by/external_id/\(userA_EUID)", contains: ["properties": ["tags": tagsUserA]]) ) // Assert there is only one request containing these tags and they are sent to userB