Skip to content

Commit 67c2c52

Browse files
emawbyemawby
authored
Merge pull request #1794 from OneSignal/user_model/improve_create_user_erroring
[5.0.0] Combine less operations with the create user operation
2 parents 9cbbf70 + da91c6a commit 67c2c52

File tree

10 files changed

+28
-104
lines changed

10 files changed

+28
-104
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/backend/IUserBackendService.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ interface IUserBackendService {
1212
* @param appId The ID of the OneSignal application this user will be created under.
1313
* @param identities The identities to retrieve/modify this user in subsequent requests. Each identity key/value pair must be unique within
1414
* the application.
15-
* @param properties The properties for this user.
1615
* @param subscriptions The subscriptions that should also be created and associated with the user. If subscriptions are already owned by a different user
1716
* they will be transferred to this user.
1817
*
1918
* @return The backend response
2019
*/
21-
suspend fun createUser(appId: String, identities: Map<String, String>, properties: PropertiesObject, subscriptions: List<SubscriptionObject>): CreateUserResponse
20+
suspend fun createUser(appId: String, identities: Map<String, String>, subscriptions: List<SubscriptionObject>): CreateUserResponse
21+
// TODO: Change to send only the push subscription, optimally
2222

2323
/**
2424
* Update the user identified by the [appId]/[aliasId]/[aliasLabel] on the backend.

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/backend/impl/UserBackendService.kt

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,17 +14,13 @@ internal class UserBackendService(
1414
private val _httpClient: IHttpClient,
1515
) : IUserBackendService {
1616

17-
override suspend fun createUser(appId: String, identities: Map<String, String>, properties: PropertiesObject, subscriptions: List<SubscriptionObject>): CreateUserResponse {
17+
override suspend fun createUser(appId: String, identities: Map<String, String>, subscriptions: List<SubscriptionObject>): CreateUserResponse {
1818
val requestJSON = JSONObject()
1919

2020
if (identities.isNotEmpty()) {
2121
requestJSON.put("identity", JSONObject().putMap(identities))
2222
}
2323

24-
if (properties.hasAtLeastOnePropertySet) {
25-
requestJSON.put("properties", JSONConverter.convertToJSON(properties))
26-
}
27-
2824
if (subscriptions.isNotEmpty()) {
2925
requestJSON
3026
.put("subscriptions", JSONConverter.convertToJSON(subscriptions))

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteAliasOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ class DeleteAliasOperation() : Operation(IdentityOperationExecutor.DELETE_ALIAS)
3131
get() = getStringProperty(::label.name)
3232
private set(value) { setStringProperty(::label.name, value) }
3333

34-
override val createComparisonKey: String get() = "$appId.User.$onesignalId"
34+
override val createComparisonKey: String get() = ""
3535
override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Alias.$label"
3636
override val groupComparisonType: GroupComparisonType = GroupComparisonType.NONE
3737
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/DeleteTagOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ class DeleteTagOperation() : Operation(UpdateUserOperationExecutor.DELETE_TAG) {
3232
get() = getStringProperty(::key.name)
3333
private set(value) { setStringProperty(::key.name, value) }
3434

35-
override val createComparisonKey: String get() = "$appId.User.$onesignalId"
35+
override val createComparisonKey: String get() = ""
3636
override val modifyComparisonKey: String get() = createComparisonKey
3737
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
3838
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetAliasOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class SetAliasOperation() : Operation(IdentityOperationExecutor.SET_ALIAS) {
3939
get() = getStringProperty(::value.name)
4040
private set(value) { setStringProperty(::value.name, value) }
4141

42-
override val createComparisonKey: String get() = "$appId.User.$onesignalId"
42+
override val createComparisonKey: String get() = ""
4343
override val modifyComparisonKey: String get() = "$appId.User.$onesignalId.Identity.$label"
4444
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
4545
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetPropertyOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ class SetPropertyOperation() : Operation(UpdateUserOperationExecutor.SET_PROPERT
3838
get() = getOptAnyProperty(::value.name)
3939
private set(value) { setOptAnyProperty(::value.name, value) }
4040

41-
override val createComparisonKey: String get() = "$appId.User.$onesignalId"
41+
override val createComparisonKey: String get() = ""
4242
override val modifyComparisonKey: String get() = createComparisonKey
4343
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
4444
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/SetTagOperation.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class SetTagOperation() : Operation(UpdateUserOperationExecutor.SET_TAG) {
3939
get() = getStringProperty(::value.name)
4040
private set(value) { setStringProperty(::value.name, value) }
4141

42-
override val createComparisonKey: String get() = "$appId.User.$onesignalId"
42+
override val createComparisonKey: String get() = ""
4343
override val modifyComparisonKey: String get() = createComparisonKey
4444
override val groupComparisonType: GroupComparisonType = GroupComparisonType.ALTER
4545
override val canStartExecute: Boolean get() = !IDManager.isLocalId(onesignalId)

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt

Lines changed: 1 addition & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,13 @@ import com.onesignal.core.internal.operations.Operation
1818
import com.onesignal.debug.internal.logging.Logging
1919
import com.onesignal.user.internal.backend.IUserBackendService
2020
import com.onesignal.user.internal.backend.IdentityConstants
21-
import com.onesignal.user.internal.backend.PropertiesObject
2221
import com.onesignal.user.internal.backend.SubscriptionObject
2322
import com.onesignal.user.internal.backend.SubscriptionObjectType
2423
import com.onesignal.user.internal.identity.IdentityModelStore
2524
import com.onesignal.user.internal.operations.CreateSubscriptionOperation
26-
import com.onesignal.user.internal.operations.DeleteAliasOperation
2725
import com.onesignal.user.internal.operations.DeleteSubscriptionOperation
28-
import com.onesignal.user.internal.operations.DeleteTagOperation
2926
import com.onesignal.user.internal.operations.LoginUserOperation
3027
import com.onesignal.user.internal.operations.SetAliasOperation
31-
import com.onesignal.user.internal.operations.SetPropertyOperation
32-
import com.onesignal.user.internal.operations.SetTagOperation
3328
import com.onesignal.user.internal.operations.TransferSubscriptionOperation
3429
import com.onesignal.user.internal.operations.UpdateSubscriptionOperation
3530
import com.onesignal.user.internal.properties.PropertiesModel
@@ -103,7 +98,6 @@ internal class LoginUserOperationExecutor(
10398

10499
private suspend fun createUser(createUserOperation: LoginUserOperation, operations: List<Operation>): ExecutionResponse {
105100
var identities = mapOf<String, String>()
106-
var propertiesObject = PropertiesObject()
107101
var subscriptions = mapOf<String, SubscriptionObject>()
108102

109103
if (createUserOperation.externalId != null) {
@@ -115,21 +109,16 @@ internal class LoginUserOperationExecutor(
115109
// go through the operations grouped with this create user and apply them to the appropriate objects.
116110
for (operation in operations) {
117111
when (operation) {
118-
is SetAliasOperation -> identities = createIdentityFromOperation(operation, identities)
119-
is DeleteAliasOperation -> identities = createIdentityFromOperation(operation, identities)
120112
is CreateSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
121113
is TransferSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
122114
is UpdateSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
123115
is DeleteSubscriptionOperation -> subscriptions = createSubscriptionsFromOperation(operation, subscriptions)
124-
is SetTagOperation -> propertiesObject = PropertyOperationHelper.createPropertiesFromOperation(operation, propertiesObject)
125-
is DeleteTagOperation -> propertiesObject = PropertyOperationHelper.createPropertiesFromOperation(operation, propertiesObject)
126-
is SetPropertyOperation -> propertiesObject = PropertyOperationHelper.createPropertiesFromOperation(operation, propertiesObject)
127116
}
128117
}
129118

130119
try {
131120
val subscriptionList = subscriptions.toList()
132-
val response = _userBackend.createUser(createUserOperation.appId, identities, propertiesObject, subscriptionList.map { it.second })
121+
val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second })
133122
val idTranslations = mutableMapOf<String, String>()
134123
// Add the "local-to-backend" ID translation to the IdentifierTranslator for any operations that were
135124
// *not* executed but still reference the locally-generated IDs.
@@ -181,18 +170,6 @@ internal class LoginUserOperationExecutor(
181170
}
182171
}
183172

184-
private fun createIdentityFromOperation(operation: SetAliasOperation, identity: Map<String, String>): Map<String, String> {
185-
val mutableIdentity = identity.toMutableMap()
186-
mutableIdentity[operation.label] = operation.value
187-
return mutableIdentity
188-
}
189-
190-
private fun createIdentityFromOperation(operation: DeleteAliasOperation, identity: Map<String, String>): Map<String, String> {
191-
val mutableIdentity = identity.toMutableMap()
192-
mutableIdentity.remove(operation.label)
193-
return mutableIdentity
194-
}
195-
196173
private fun createSubscriptionsFromOperation(operation: TransferSubscriptionOperation, subscriptions: Map<String, SubscriptionObject>): Map<String, SubscriptionObject> {
197174
val mutableSubscriptions = subscriptions.toMutableMap()
198175
if (mutableSubscriptions.containsKey(operation.subscriptionId)) {

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/user/internal/backend/UserBackendServiceTests.kt

Lines changed: 3 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,11 @@ class UserBackendServiceTests : FunSpec({
2828
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(403, "FORBIDDEN")
2929
val userBackendService = UserBackendService(spyHttpClient)
3030
val identities = mapOf<String, String>()
31-
val properties = PropertiesObject()
3231
val subscriptions = listOf<SubscriptionObject>()
3332

3433
/* When */
3534
val exception = shouldThrowUnit<BackendException> {
36-
userBackendService.createUser("appId", identities, properties, subscriptions)
35+
userBackendService.createUser("appId", identities, subscriptions)
3736
}
3837

3938
/* Then */
@@ -48,11 +47,10 @@ class UserBackendServiceTests : FunSpec({
4847
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\", aliasLabel1: \"aliasValue1\"}}")
4948
val userBackendService = UserBackendService(spyHttpClient)
5049
val identities = mapOf("aliasLabel1" to "aliasValue1")
51-
val properties = PropertiesObject()
5250
val subscriptions = listOf<SubscriptionObject>()
5351

5452
/* When */
55-
val response = userBackendService.createUser("appId", identities, properties, subscriptions)
53+
val response = userBackendService.createUser("appId", identities, subscriptions)
5654

5755
/* Then */
5856
response.identities["onesignal_id"] shouldBe osId
@@ -79,12 +77,11 @@ class UserBackendServiceTests : FunSpec({
7977
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\"}, subscriptions:[{id:\"subscriptionId1\", type:\"AndroidPush\"}]}")
8078
val userBackendService = UserBackendService(spyHttpClient)
8179
val identities = mapOf<String, String>()
82-
val properties = PropertiesObject()
8380
val subscriptions = mutableListOf<SubscriptionObject>()
8481
subscriptions.add(SubscriptionObject("SHOULDNOTUSE", SubscriptionObjectType.ANDROID_PUSH))
8582

8683
/* When */
87-
val response = userBackendService.createUser("appId", identities, properties, subscriptions)
84+
val response = userBackendService.createUser("appId", identities, subscriptions)
8885

8986
/* Then */
9087
response.identities["onesignal_id"] shouldBe osId
@@ -107,40 +104,6 @@ class UserBackendServiceTests : FunSpec({
107104
}
108105
}
109106

110-
test("create user with an alias and properties creates a new user") {
111-
/* Given */
112-
val osId = "11111111-1111-1111-1111-111111111111"
113-
val spyHttpClient = mockk<IHttpClient>()
114-
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\", aliasLabel1: \"aliasValue1\"}, properties: { tags: {tagKey1: tagValue1}}}")
115-
val userBackendService = UserBackendService(spyHttpClient)
116-
val identities = mapOf("aliasLabel1" to "aliasValue1")
117-
val properties = PropertiesObject(tags = mapOf("tagkey1" to "tagValue1"))
118-
val subscriptions = listOf<SubscriptionObject>()
119-
120-
/* When */
121-
val response = userBackendService.createUser("appId", identities, properties, subscriptions)
122-
123-
/* Then */
124-
response.identities["onesignal_id"] shouldBe osId
125-
response.identities["aliasLabel1"] shouldBe "aliasValue1"
126-
response.subscriptions.count() shouldBe 0
127-
coVerify {
128-
spyHttpClient.post(
129-
"apps/appId/users",
130-
withArg {
131-
it.has("identity") shouldBe true
132-
it.getJSONObject("identity").has("aliasLabel1") shouldBe true
133-
it.getJSONObject("identity").getString("aliasLabel1") shouldBe "aliasValue1"
134-
it.has("properties") shouldBe true
135-
it.getJSONObject("properties").has("tags") shouldBe true
136-
it.getJSONObject("properties").getJSONObject("tags").has("tagkey1") shouldBe true
137-
it.getJSONObject("properties").getJSONObject("tags").getString("tagkey1") shouldBe "tagValue1"
138-
it.has("subscriptions") shouldBe false
139-
},
140-
)
141-
}
142-
}
143-
144107
test("update user tags") {
145108
/* Given */
146109
val aliasLabel = "onesignal_id"

0 commit comments

Comments
 (0)