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.

Sign up for GitHub

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

Jump to bottom

Fix timezone_id not being set #1842

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Fix timezone_id not being set #1842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fe6a47
Send timezone_id on createuser
emawby Sep 15, 2023
9a1bee3
Update unit tests to account for properties in createUser
emawby Sep 18, 2023
7bd784f
make getTimeZoneId non-optional
emawby Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/TimeUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ object TimeUtils {
return offset / 1000
}

fun getTimeZoneId(): String? {
fun getTimeZoneId(): String {
return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
ZoneId.systemDefault().id
} else {
Expand Down
3 changes: 2 additions & 1 deletion ...K/onesignal/core/src/main/java/com/onesignal/user/internal/backend/IUserBackendService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,11 @@ interface IUserBackendService {
* the application.
* @param subscriptions The subscriptions that should also be created and associated with the user. If subscriptions are already owned by a different user
* they will be transferred to this user.
* @param properties The properties for this user. For new users this should include the timezone_id property.
*
* @return The backend response
*/
suspend fun createUser(appId: String, identities: Map<String, String>, subscriptions: List<SubscriptionObject>): CreateUserResponse
suspend fun createUser(appId: String, identities: Map<String, String>, subscriptions: List<SubscriptionObject>, properties: Map<String, String>): CreateUserResponse
// TODO: Change to send only the push subscription, optimally

/**
Expand Down
6 changes: 5 additions & 1 deletion ...esignal/core/src/main/java/com/onesignal/user/internal/backend/impl/UserBackendService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ internal class UserBackendService(
private val _httpClient: IHttpClient,
) : IUserBackendService {

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

if (identities.isNotEmpty()) {
Expand All @@ -26,6 +26,10 @@ internal class UserBackendService(
.put("subscriptions", JSONConverter.convertToJSON(subscriptions))
}

if (properties.isNotEmpty()) {
requestJSON.put("properties", JSONObject().putMap(properties))
}

val response = _httpClient.post("apps/$appId/users", requestJSON)

if (!response.isSuccess) {
Expand Down
14 changes: 5 additions & 9 deletions .../java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
package com.onesignal.user.internal.operations.impl.executors

import android.os.Build
import com.onesignal.common.AndroidUtils
import com.onesignal.common.DeviceUtils
import com.onesignal.common.NetworkUtils
import com.onesignal.common.OneSignalUtils
import com.onesignal.common.RootToolsInternalMethods
import com.onesignal.common.*
import com.onesignal.common.exceptions.BackendException
import com.onesignal.common.modeling.ModelChangeTags
import com.onesignal.core.internal.application.IApplicationService
Expand All @@ -16,10 +12,8 @@ import com.onesignal.core.internal.operations.ExecutionResult
import com.onesignal.core.internal.operations.IOperationExecutor
import com.onesignal.core.internal.operations.Operation
import com.onesignal.debug.internal.logging.Logging
import com.onesignal.user.internal.backend.IUserBackendService
import com.onesignal.user.internal.backend.*
import com.onesignal.user.internal.backend.IdentityConstants
import com.onesignal.user.internal.backend.SubscriptionObject
import com.onesignal.user.internal.backend.SubscriptionObjectType
import com.onesignal.user.internal.identity.IdentityModelStore
import com.onesignal.user.internal.operations.CreateSubscriptionOperation
import com.onesignal.user.internal.operations.DeleteSubscriptionOperation
Expand Down Expand Up @@ -99,6 +93,8 @@ internal class LoginUserOperationExecutor(
private suspend fun createUser(createUserOperation: LoginUserOperation, operations: List<Operation>): ExecutionResponse {
var identities = mapOf<String, String>()
var subscriptions = mapOf<String, SubscriptionObject>()
val properties = mutableMapOf<String, String>()
properties["timezone_id"] = TimeUtils.getTimeZoneId()

if (createUserOperation.externalId != null) {
val mutableIdentities = identities.toMutableMap()
Expand All @@ -118,7 +114,7 @@ internal class LoginUserOperationExecutor(

try {
val subscriptionList = subscriptions.toList()
val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second })
val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second }, properties)
val idTranslations = mutableMapOf<String, String>()
// Add the "local-to-backend" ID translation to the IdentifierTranslator for any operations that were
// *not* executed but still reference the locally-generated IDs.
Expand Down
19 changes: 12 additions & 7 deletions ...esignal/core/src/test/java/com/onesignal/user/internal/backend/UserBackendServiceTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,12 @@ class UserBackendServiceTests : FunSpec({
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(403, "FORBIDDEN")
val userBackendService = UserBackendService(spyHttpClient)
val identities = mapOf<String, String>()
val properties = mapOf<String, String>()
val subscriptions = listOf<SubscriptionObject>()

/* When */
val exception = shouldThrowUnit<BackendException> {
userBackendService.createUser("appId", identities, subscriptions)
userBackendService.createUser("appId", identities, subscriptions, properties)
}

/* Then */
Expand All @@ -44,17 +45,19 @@ class UserBackendServiceTests : FunSpec({
/* Given */
val osId = "11111111-1111-1111-1111-111111111111"
val spyHttpClient = mockk<IHttpClient>()
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\", aliasLabel1: \"aliasValue1\"}}")
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\", aliasLabel1: \"aliasValue1\"}, properties:{timezone_id: \"testTimeZone\"}}")
val userBackendService = UserBackendService(spyHttpClient)
val identities = mapOf("aliasLabel1" to "aliasValue1")
val properties = mapOf("timzone_id" to "testTimeZone")
val subscriptions = listOf<SubscriptionObject>()

/* When */
val response = userBackendService.createUser("appId", identities, subscriptions)
val response = userBackendService.createUser("appId", identities, subscriptions, properties)

/* Then */
response.identities["onesignal_id"] shouldBe osId
response.identities["aliasLabel1"] shouldBe "aliasValue1"
response.properties.timezoneId shouldBe "testTimeZone"
response.subscriptions.count() shouldBe 0
coVerify {
spyHttpClient.post(
Expand All @@ -63,7 +66,7 @@ class UserBackendServiceTests : FunSpec({
it.has("identity") shouldBe true
it.getJSONObject("identity").has("aliasLabel1") shouldBe true
it.getJSONObject("identity").getString("aliasLabel1") shouldBe "aliasValue1"
it.has("properties") shouldBe false
it.has("properties") shouldBe true
it.has("subscriptions") shouldBe false
},
)
Expand All @@ -74,17 +77,19 @@ class UserBackendServiceTests : FunSpec({
/* Given */
val osId = "11111111-1111-1111-1111-111111111111"
val spyHttpClient = mockk<IHttpClient>()
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\"}, subscriptions:[{id:\"subscriptionId1\", type:\"AndroidPush\"}]}")
coEvery { spyHttpClient.post(any(), any()) } returns HttpResponse(202, "{identity:{onesignal_id: \"$osId\"}, subscriptions:[{id:\"subscriptionId1\", type:\"AndroidPush\"}], properties:{timezone_id: \"testTimeZone\"}}")
val userBackendService = UserBackendService(spyHttpClient)
val identities = mapOf<String, String>()
val subscriptions = mutableListOf<SubscriptionObject>()
val properties = mapOf("timzone_id" to "testTimeZone")
subscriptions.add(SubscriptionObject("SHOULDNOTUSE", SubscriptionObjectType.ANDROID_PUSH))

/* When */
val response = userBackendService.createUser("appId", identities, subscriptions)
val response = userBackendService.createUser("appId", identities, subscriptions, properties)

/* Then */
response.identities["onesignal_id"] shouldBe osId
response.properties.timezoneId shouldBe "testTimeZone"
response.subscriptions.count() shouldBe 1
response.subscriptions[0].id shouldBe "subscriptionId1"
response.subscriptions[0].type shouldBe SubscriptionObjectType.ANDROID_PUSH
Expand All @@ -94,7 +99,7 @@ class UserBackendServiceTests : FunSpec({
"apps/appId/users",
withArg {
it.has("identity") shouldBe false
it.has("properties") shouldBe false
it.has("properties") shouldBe true
it.has("subscriptions") shouldBe true
it.getJSONArray("subscriptions").length() shouldBe 1
it.getJSONArray("subscriptions").getJSONObject(0).has("type") shouldBe true
Expand Down
37 changes: 21 additions & 16 deletions ...e/src/test/java/com/onesignal/user/internal/operations/LoginUserOperationExecutorTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class LoginUserOperationExecutorTests : FunSpec({
test("login anonymous user successfully creates user") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
PropertiesObject(),
Expand Down Expand Up @@ -80,14 +80,15 @@ class LoginUserOperationExecutorTests : FunSpec({
appId,
mapOf(),
any(),
any(),
)
}
}

test("login anonymous user fails with retry when network condition exists") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } throws BackendException(408, "TIMEOUT")
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } throws BackendException(408, "TIMEOUT")

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()

Expand All @@ -103,13 +104,13 @@ class LoginUserOperationExecutorTests : FunSpec({

/* Then */
response.result shouldBe ExecutionResult.FAIL_RETRY
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any()) }
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any(), any()) }
}

test("login anonymous user fails with no retry when backend error condition exists") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } throws BackendException(404, "NOT FOUND")
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } throws BackendException(404, "NOT FOUND")

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()

Expand All @@ -125,13 +126,13 @@ class LoginUserOperationExecutorTests : FunSpec({

/* Then */
response.result shouldBe ExecutionResult.FAIL_NORETRY
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any()) }
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(), any(), any()) }
}

test("login identified user without association successfully creates user") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId), PropertiesObject(), listOf())

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
Expand All @@ -148,7 +149,7 @@ class LoginUserOperationExecutorTests : FunSpec({

/* Then */
response.result shouldBe ExecutionResult.SUCCESS
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any()) }
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any(), any()) }
}

test("login identified user with association succeeds when association is successful") {
Expand Down Expand Up @@ -187,7 +188,7 @@ class LoginUserOperationExecutorTests : FunSpec({
test("login identified user with association fails with retry when association fails with retry") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId), PropertiesObject(), listOf())

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
Expand Down Expand Up @@ -222,7 +223,7 @@ class LoginUserOperationExecutorTests : FunSpec({
test("login identified user with association successfully creates user when association fails with no retry") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId), PropertiesObject(), listOf())

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
Expand Down Expand Up @@ -252,13 +253,13 @@ class LoginUserOperationExecutorTests : FunSpec({
},
)
}
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any()) }
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any(), any()) }
}

test("login identified user with association fails with retry when association fails with no retry and network condition exists") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } throws BackendException(408, "TIMEOUT")
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } throws BackendException(408, "TIMEOUT")

val mockIdentityOperationExecutor = mockk<IdentityOperationExecutor>()
coEvery { mockIdentityOperationExecutor.execute(any()) } returns ExecutionResponse(ExecutionResult.FAIL_NORETRY)
Expand Down Expand Up @@ -287,13 +288,13 @@ class LoginUserOperationExecutorTests : FunSpec({
},
)
}
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any()) }
coVerify(exactly = 1) { mockUserBackendService.createUser(appId, mapOf(IdentityConstants.EXTERNAL_ID to "externalId"), any(), any()) }
}

test("creating user will merge operations into one backend call") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
PropertiesObject(),
Expand Down Expand Up @@ -357,14 +358,15 @@ class LoginUserOperationExecutorTests : FunSpec({
it[0].token shouldBe "pushToken2"
it[0].notificationTypes shouldBe SubscriptionStatus.SUBSCRIBED
},
any(),
)
}
}

test("creating user will hydrate when the user hasn't changed") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
PropertiesObject(),
Expand Down Expand Up @@ -423,14 +425,15 @@ class LoginUserOperationExecutorTests : FunSpec({
appId,
mapOf(),
any(),
any(),
)
}
}

test("creating user will not hydrate when the user has changed") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
PropertiesObject(),
Expand Down Expand Up @@ -489,14 +492,15 @@ class LoginUserOperationExecutorTests : FunSpec({
appId,
mapOf(),
any(),
any(),
)
}
}

test("creating user will provide local to remote translations") {
/* Given */
val mockUserBackendService = mockk<IUserBackendService>()
coEvery { mockUserBackendService.createUser(any(), any(), any()) } returns
coEvery { mockUserBackendService.createUser(any(), any(), any(), any()) } returns
CreateUserResponse(
mapOf(IdentityConstants.ONESIGNAL_ID to remoteOneSignalId),
PropertiesObject(),
Expand Down Expand Up @@ -536,6 +540,7 @@ class LoginUserOperationExecutorTests : FunSpec({
appId,
mapOf(),
any(),
any(),
)
}
}
Expand Down
Loading