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.

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

Fix autosend scheduling #6121

Merged
merged 24 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0e3ae3f
Isolate deprecated network info code
seadowg Apr 30, 2024
dde08ad
Make TestScheduler network aware to reveal failing autosend tests
seadowg Apr 30, 2024
f1bca3e
Fix form level auto send
seadowg May 1, 2024
8ae1ec2
Make sure form level auto send works for multiple forms
seadowg May 1, 2024
77b9edb
Simplify auto send logic
seadowg May 1, 2024
80a35a5
Remove extra send method
seadowg May 1, 2024
4cd6530
Use one task spec for sending forms
seadowg May 1, 2024
4889cd7
Support auto send forms in bulk finalization
seadowg May 1, 2024
7e90221
Rename method
seadowg May 1, 2024
c862c75
Simplify method signature
seadowg May 1, 2024
e2a81c6
Don't queue multiple form level auto send jobs
seadowg May 2, 2024
b4d5fde
Make sure form level auto send doesn't resend forms
seadowg May 2, 2024
6c8398a
Simplify TaskSpec and make sure jobs are rescheduled to account for n…
seadowg May 3, 2024
4d8c841
Simplify NetworkStateProvider API
seadowg May 3, 2024
1d88a3b
Retry cellular tasks if connected to a metered non-cellular connection
seadowg May 3, 2024
8b1f171
Rework test
seadowg May 3, 2024
8ca2b49
Remove test mirroring implementation
seadowg May 3, 2024
57c459c
Make sure network state provider is shared properly
seadowg May 7, 2024
78a1bda
Set user property when user runs into metered wifi
seadowg May 7, 2024
8dd91aa
Remove unneeded line from test
seadowg Jun 10, 2024
5a90099
Fix import
seadowg Jun 10, 2024
f08bf48
Update test names
seadowg Jun 10, 2024
52cc9b1
Remove impossible when clause
seadowg Jun 10, 2024
64355c8
Make test helper private
seadowg Jun 10, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,9 @@ interface Analytics {
fun setParam(key: String, value: String) {
params[key] = value
}

fun setUserProperty(name: String, value: String) {
instance.setUserProperty(name, value)
}
}
}

This file was deleted.

This file was deleted.

3 changes: 3 additions & 0 deletions async/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ dependencies {
implementation(Dependencies.androidx_core_ktx)
implementation(Dependencies.kotlinx_coroutines_android)
implementation(Dependencies.androidx_work_runtime)
implementation(project(":analytics")) {
exclude("com.google.firebase")
grzesiek2010 marked this conversation as resolved.
Show resolved Hide resolved
}

testImplementation(Dependencies.hamcrest)
testImplementation(Dependencies.robolectric)
Expand Down
3 changes: 2 additions & 1 deletion async/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android">
</manifest>
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
</manifest>
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,44 @@ import kotlinx.coroutines.Dispatchers
import java.util.concurrent.TimeUnit
import kotlin.coroutines.CoroutineContext

class CoroutineAndWorkManagerScheduler(foregroundContext: CoroutineContext, backgroundContext: CoroutineContext, private val workManager: WorkManager) : CoroutineScheduler(foregroundContext, backgroundContext) {

constructor(workManager: WorkManager) : this(Dispatchers.Main, Dispatchers.IO, workManager) // Needed for Java construction

override fun networkDeferred(tag: String, spec: TaskSpec, inputData: Map<String, String>, networkConstraint: Scheduler.NetworkType?) {
class CoroutineAndWorkManagerScheduler(
foregroundContext: CoroutineContext,
backgroundContext: CoroutineContext,
private val workManager: WorkManager
) : CoroutineScheduler(foregroundContext, backgroundContext) {

constructor(workManager: WorkManager) : this(
Dispatchers.Main,
Dispatchers.IO,
workManager
) // Needed for Java construction

override fun networkDeferred(
tag: String,
spec: TaskSpec,
inputData: Map<String, String>,
networkConstraint: Scheduler.NetworkType?
) {
val constraintNetworkType = when (networkConstraint) {
Scheduler.NetworkType.WIFI -> NetworkType.UNMETERED
Scheduler.NetworkType.CELLULAR -> NetworkType.METERED
null -> NetworkType.CONNECTED
else -> NetworkType.CONNECTED
}

val constraints = Constraints.Builder()
.setRequiredNetworkType(constraintNetworkType)
.build()

val workManagerInputData = Data.Builder().putAll(inputData).build()
val workManagerInputData = Data.Builder()
.putString(TaskSpecWorker.DATA_TASK_SPEC_CLASS, spec.javaClass.name)
.putBoolean(
TaskSpecWorker.DATA_CELLULAR_ONLY,
networkConstraint == Scheduler.NetworkType.CELLULAR
)
.putAll(inputData)
.build()

val worker = spec.getWorkManagerAdapter()
val workRequest = OneTimeWorkRequest.Builder(worker)
val workRequest = OneTimeWorkRequest.Builder(TaskSpecWorker::class.java)
.addTag(tag)
.setConstraints(constraints)
.setInputData(workManagerInputData)
Expand All @@ -40,15 +59,26 @@ class CoroutineAndWorkManagerScheduler(foregroundContext: CoroutineContext, back
workManager.beginUniqueWork(tag, ExistingWorkPolicy.REPLACE, workRequest).enqueue()
}

override fun networkDeferredRepeat(tag: String, spec: TaskSpec, repeatPeriod: Long, inputData: Map<String, String>) {
override fun networkDeferredRepeat(
tag: String,
spec: TaskSpec,
repeatPeriod: Long,
inputData: Map<String, String>
) {
val constraints = Constraints.Builder()
.setRequiredNetworkType(NetworkType.CONNECTED)
.build()

val workManagerInputData = Data.Builder().putAll(inputData).build()
val workManagerInputData = Data.Builder()
.putString(TaskSpecWorker.DATA_TASK_SPEC_CLASS, spec.javaClass.name)
.putAll(inputData)
.build()

val worker = spec.getWorkManagerAdapter()
val builder = PeriodicWorkRequest.Builder(worker, repeatPeriod, TimeUnit.MILLISECONDS)
val builder = PeriodicWorkRequest.Builder(
TaskSpecWorker::class.java,
repeatPeriod,
TimeUnit.MILLISECONDS
)
.addTag(tag)
.setInputData(workManagerInputData)
.setConstraints(constraints)
Expand All @@ -59,7 +89,11 @@ class CoroutineAndWorkManagerScheduler(foregroundContext: CoroutineContext, back
}
}

workManager.enqueueUniquePeriodicWork(tag, ExistingPeriodicWorkPolicy.REPLACE, builder.build())
workManager.enqueueUniquePeriodicWork(
tag,
ExistingPeriodicWorkPolicy.REPLACE,
builder.build()
)
}

override fun cancelDeferred(tag: String) {
Expand Down
3 changes: 2 additions & 1 deletion async/src/main/java/org/odk/collect/async/Scheduler.kt
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ interface Scheduler {

enum class NetworkType {
WIFI,
CELLULAR
CELLULAR,
OTHER
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTHER would just be any other connected network. Android devices can theoretically have network delivered via Ethernet or Bluetooth for example. These kinds of connections would still satisfy WorkManager's (and our won) "connected" constraint.

}
}

Expand Down
6 changes: 0 additions & 6 deletions async/src/main/java/org/odk/collect/async/TaskSpec.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,4 @@ interface TaskSpec {
* once instead of doing that after every single execution.
*/
fun getTask(context: Context, inputData: Map<String, String>, isLastUniqueExecution: Boolean): Supplier<Boolean>
grzesiek2010 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns class that can be used to schedule this task using Android's
* WorkManager framework
*/
fun getWorkManagerAdapter(): Class<out WorkerAdapter>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realised we didn't need all this and could just use strings (and no arg constructors) to integrate with WorkManager. It will eventually have to store our class names as strings anyway.

}
47 changes: 47 additions & 0 deletions async/src/main/java/org/odk/collect/async/TaskSpecWorker.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package org.odk.collect.async

import android.content.Context
import androidx.work.Worker
import androidx.work.WorkerParameters
import org.odk.collect.analytics.Analytics
import org.odk.collect.async.network.ConnectivityProvider

class TaskSpecWorker(
context: Context,
workerParams: WorkerParameters
) : Worker(context, workerParams) {

private val connectivityProvider: ConnectivityProvider = ConnectivityProvider(context)

override fun doWork(): Result {
val cellularOnly = inputData.getBoolean(DATA_CELLULAR_ONLY, false)
if (cellularOnly && connectivityProvider.currentNetwork != Scheduler.NetworkType.CELLULAR) {
Analytics.setUserProperty("EncounteredMeteredNonCellularInTasks", "true")
return Result.retry()
}

val specClass = inputData.getString(DATA_TASK_SPEC_CLASS)!!
val spec = Class.forName(specClass).getConstructor().newInstance() as TaskSpec

val stringInputData = inputData.keyValueMap.mapValues { it.value.toString() }
val completed =
spec.getTask(applicationContext, stringInputData, isLastUniqueExecution(spec)).get()
val maxRetries = spec.maxRetries
grzesiek2010 marked this conversation as resolved.
Show resolved Hide resolved

return if (completed) {
Result.success()
} else if (maxRetries == null || runAttemptCount < maxRetries) {
Result.retry()
} else {
Result.failure()
}
}

private fun isLastUniqueExecution(spec: TaskSpec) =
spec.maxRetries?.let { runAttemptCount >= it } ?: true

companion object {
const val DATA_TASK_SPEC_CLASS = "taskSpecClass"
const val DATA_CELLULAR_ONLY = "cellularOnly"
}
}
28 changes: 0 additions & 28 deletions async/src/main/java/org/odk/collect/async/WorkerAdapter.kt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.odk.collect.async.network

import android.content.Context
import android.net.ConnectivityManager
import org.odk.collect.async.Scheduler

class ConnectivityProvider(private val context: Context) : NetworkStateProvider {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to replace this implementation at some point, as it's pretty much all deprecated.

override val currentNetwork: Scheduler.NetworkType?
get() {
return if (connectivityManager.activeNetworkInfo?.isConnected == true) {
when (connectivityManager.activeNetworkInfo?.type) {
ConnectivityManager.TYPE_WIFI -> Scheduler.NetworkType.WIFI
ConnectivityManager.TYPE_MOBILE -> Scheduler.NetworkType.CELLULAR
else -> Scheduler.NetworkType.OTHER
}
} else {
null
}
}

private val connectivityManager: ConnectivityManager
get() = context.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.odk.collect.async.network

import org.odk.collect.async.Scheduler

interface NetworkStateProvider {
seadowg marked this conversation as resolved.
Show resolved Hide resolved
val currentNetwork: Scheduler.NetworkType?

val isDeviceOnline: Boolean
get() {
return currentNetwork != null
}
}