chore: merge 5.3.0 beta into main and resolve conflict #2525
Conversation
chore: Kotlin 1.9 update
chore(tests): add SessionListenerTests class
ci: create release prs for wrappers for new android sdk release
Co-authored-by: AR Abdul Azeez <abdul@onesignal.com>
- removed CustomEvent and immediately save custom event properties into a JSONObject - renamed TrackEventOperation to TrackCustomEventOperation - removed app_id in body - fix comparison key to unique custom event key - fix some test units to prevent them from failing
jinliu9508
force-pushed
the
5.3.0_beta_branch
branch
from
629ab0b to
db81966
Compare
jinliu9508
changed the title
| @@ -1,15 +1,11 @@ | |||
| name: Build and Test SDK | |||
|
|
|||
| concurrency: | |||
There was a problem hiding this comment.
i wonder why this file would get impacted @jinliu9508 ?
| return when (value) { | ||
| null, | ||
| is Boolean, | ||
| is Number, |
There was a problem hiding this comment.
how about finite checks?
| is JSONObject, | ||
| is JSONArray, | ||
| -> true | ||
| is Map<*, *> -> value.keys.all { it is String } && value.values.all { isValidJsonObject(it) } |
There was a problem hiding this comment.
can you please write a unit test for this?
| body.put( | ||
| "timestamp", | ||
| DateUtils.iso8601Format().apply { | ||
| timeZone = TimeZone.getTimeZone("UTC") |
There was a problem hiding this comment.
we did something around timezone earlier, do we have to keep anything in mind here @nan-li ?
| * Recursively convert a JSON-serializable map into a JSON-compatible format, handling | ||
| * nested Maps and Lists appropriately. | ||
| */ | ||
| private fun mapToJson(map: Map<String, Any>): JSONObject { |
There was a problem hiding this comment.
should the two methods be placed in json utils class? and we should write unit tests for these
| } | ||
|
|
||
| companion object { | ||
| private const val DEVICE_TYPE = "device_type" |
There was a problem hiding this comment.
can we move all these into a central location?
i see a bunch of keys used in several places, all of them hardcoded.
| import com.onesignal.user.internal.operations.TrackCustomEventOperation | ||
|
|
||
| internal class CustomEventOperationExecutor( | ||
| private val _customEventBackendService: ICustomEventBackendService, |
There was a problem hiding this comment.
did the _ pass lint? this not a standard kotlin way IMHO
| NetworkUtils.ResponseStatusType.RETRYABLE -> | ||
| ExecutionResponse(ExecutionResult.FAIL_RETRY, retryAfterSeconds = ex.retryAfterSeconds) | ||
| else -> | ||
| // TODO: will not retry all other error until we finalize how to handle |
There was a problem hiding this comment.
why todo? do we need to make any other decision here?
| } | ||
|
|
||
| companion object { | ||
| const val CUSTOM_EVENT = "custom-event" |
There was a problem hiding this comment.
should go the centralized constants class/interface as well
| setOptStringProperty(::eventProperties.name, value) | ||
| } | ||
|
|
||
| override val createComparisonKey: String get() = "$appId.User.$onesignalId.CustomEvent.$eventName" |
There was a problem hiding this comment.
both are same? is that expected?
Rebase and resolve conflicts from main.
This change is