Replace asynctask uploader with viewmodel

[grzesiek2010]

Closes #6827

Why is this the best possible solution? Were any other approaches considered?

The issue itself was not significant and could be fixed without major changes. However, it was based on a long-deprecated AsyncTask, so it was a good opportunity to replace it with modern architecture components, which I did while also ensuring the issue was properly addressed.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

We need to carefully test the entire process of manually uploading finalized forms (from the list of forms to send), as this functionality has been refactored.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• added any new strings with date formatting to DateFormatsTest
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[seadowg]

Just posting another structural change I think would be good here!

[seadowg]

I think we should use this to make a decision about the future when it comes to using coroutines directly vs Scheduler. Currently, Scheduler's main advantage is that it can be used from Java and Kotlin meaning the async approach being used is consistent. As far as I can see, testing seems pretty similar between the two at this point. There are obviously many advantages to just using coroutines directly.

I'm going to assume from you using coroutines directly here that you think we should ditch using scheduler for "immediate" async work in Kotlin?

[grzesiek2010]

I wouldn’t say I’m strongly leaning toward one option over the other. Right now, Scheduler is mainly used for starting background tasks, and in this case I needed more control, especially the ability to cancel the work.

Of course, we could probably extend Scheduler to support that as well. But in this PR we already introduced quite a bit of refactoring beyond just fixing the issue, so I think it makes sense to draw the line somewhere.

What's your opinion?

[seadowg]

What's your opinion?

The other advantage to Scheduler is that it protects our code from async framework changes. Here you're using a mixture of Kotin language features (basically suspend functions) and an async framework (CoroutineScope, CoroutineDispatcher and Job from kotlinx.coroutines).

If we want to continue with that advantage but also use suspend functions more directly and get good cancellation features, we could add suspend support to Scheduler and return the Cancellable abstraction we already have. Something like:

fun <T> immediate(work: (isCancelled: Boolean) -> Unit): Cancellable

We'd need to add even more abstraction to deal with viewModelScope though. I'm not sure it's worth it: it seems pretty overprotective to try and avoid kotlinx.coroutines at this point. I'm leaning towards saying that we deprecate the "foreground" parts of Scheduler and just embrace kotlinx.coroutines. How does that sound?

[grzesiek2010]

One thing I think is worth calling out explicitly is what we lose on the testing side. With Scheduler (and especially FakeScheduler) we currently have very fine-grained, deterministic control over execution — we can decide exactly when background and foreground work runs (e.g. flush(), runFirstBackground(), etc.) and assert intermediate states very precisely.

With coroutines, we’d be moving to TestDispatcher/runTest/advanceUntilIdle(), which works well but gives us less direct control over execution order and scheduling. In most cases this is probably an acceptable tradeoff, but it does change how we write and reason about tests.

[seadowg]

One thing I think is worth calling out explicitly is what we lose on the testing side. With Scheduler (and especially FakeScheduler) we currently have very fine-grained, deterministic control over execution — we can decide exactly when background and foreground work runs (e.g. flush(), runFirstBackground(), etc.) and assert intermediate states very precisely.

Interesting. I'd thought we had the same level of control from looking at your test code here. Is there an example test somewhere that we would have a hard time writing with the test dispatcher setup?

[grzesiek2010]

With FakeScheduler, we can control execution step by step, separating background and foreground work. For example:

class MyViewModel(
    private val scheduler: Scheduler,
    private val repo: MyRepository
) : ViewModel() {

    val state = MutableLiveData<String>("Idle")

    init {
        scheduler.immediate(
            background = Supplier {
                state.postValue("Loading")         // background step
                repo.getData()                     // fetch data
            },
            foreground = Consumer { data ->
                state.postValue("Loaded: $data")  // foreground step
            }
        )
    }
}

// ViewModel using FakeScheduler
val vm = MyViewModel(FakeScheduler(), repo)
assertEquals("Idle", vm.state.value)           // before any task runs

fakeScheduler.runFirstBackground()             // only background task
assertEquals("Loading", vm.state.value)        // intermediate state

fakeScheduler.runForeground()                  // run everything on foreground
assertEquals("Loaded: hello", vm.state.value)  // final state

This lets us assert intermediate states very precisely.

With coroutines and TestDispatcher / advanceUntilIdle(), we can still test the final outcome:

class MyViewModel2(
    private val repo: MyRepository,
    private val dispatcher: CoroutineDispatcher
) : ViewModel() {

    val state = MutableLiveData<String>("Idle")

    init {
        viewModelScope.launch(dispatcher) {
            state.value = "Loading"           
            val data = repo.getData()        
            state.value = "Loaded: $data"     
        }
    }
}

val vm = MyViewModel2(StandardTestDispatcher(testScheduler), repo)
assertEquals("Idle", vm.state.value)

advanceUntilIdle()                             // runs everything
assertEquals("Loaded: hello", vm.state.value)  // final state

…but it’s harder to observe or control partial execution steps, because background and foreground updates run together when the dispatcher is advanced.

For most cases, this is probably fine and coroutines give us simpler, more native async handling. But for scenarios where we want fine-grained control over intermediate states (like tasks that update state in multiple steps), FakeScheduler still offers advantages.

[grzesiek2010]

So I just wanted to point out that in some cases, testing might be harder, but that should be fine.

[seadowg]

Right I see what you're getting at. I don't think your examples are quite the same though - the concurrency models/behaviour are different. We're only able to test the intermediate state with FakeScheduler because the code under test splits the work into "background" and "foreground" work whereas the coroutine based implementation is just running everything as a single suspend block that is executed as a unit by the passed dispatcher. The coroutine code should look more like this for it to be equivalent to the Scheduler example:

class MyViewModel2(
    private val repo: MyRepository,
    private val foregroundDispatcher: CoroutineDispatcher,
    private val backgroundDispatcher: CoroutineDispatcher
) : ViewModel() {

    val state = MutableLiveData<String>("Idle")

    init {
        viewModelScope.launch(foregroundDispatcher) {
            state.value = "Loading"
            val data = withContext(backgroundDispatcher) {
                repo.getData()
            }
        
            state.value = "Loaded: $data"     
        }
    }
}

This is essentially the same as how we implement Scheduler with coroutines (in CoroutineTaskRunner).

As far as I can tell, we'd be able to write the equivalent test for this code by passing separate test dispatchers that we can control independently as foregroundDispatcher and backgroundDispatcher. Does that work? Or, is there still things that would make the testing more awkward that I'm missing?

[grzesiek2010]

Ahh, ok that would work. Cool! But I’m not entirely sure we shouldn’t extend our scheduler to support provided scopes. It’s not that complex, and it still might be nice to inject a single scheduler instead of two dispatchers when testing. What do you think? See 6bf0b96

[seadowg]

But I’m not entirely sure we shouldn’t extend our scheduler to support provided scopes.

This goes back to my earlier comment:

The other advantage to Scheduler is that it protects our code from async framework changes. Here you're using a mixture of Kotin language features (basically suspend functions) and an async framework (CoroutineScope, CoroutineDispatcher and Job from kotlinx.coroutines).

Adding support for CoroutineScope (and returning Job) to Schedulermeans that the interface now depends onkotlinx.coroutines. Given that, I'm not sure it's worth wrapping really? To make injection easier we could just add our own DispatcherProviderinterface that provides a foreground and background dispatcher, and then we can just use all thekotlinx.coroutinesstuff directly. That also lets us do things like collect aFlow` without "breaking" our encapsulation rules.

[seadowg]

#7008 (comment)