Rework side effects

[Rawa]

Summary: This PR fixes some sideeffect issues we've seen. E.g when Navigation to the Account view, and OutOfTime view overriding and showing instead. It does this by reworking the OutOfTimeUseCase and rewriting how we handle side effects.

Details:
We have different ways side effects happens in our app here are some unique examples:

• User action caused effect (user does an action, which then may or may not result in side effect, e.g conditional navigation)
• Delayed effect (user does an action, but may result in a delayed effect, e.g in the Login screen, user logs in but then navigation happens after 1 seconds of us showing success screen)
• External effect (External component causes a side effect to happen, e.g user happens to become OutOfTime)

These types of side effects needs to be taken into consideration and discussions made on a case by case basis. This PR changes so we no longer buffer old side effects and then at a later time accidentally executes them. We then have to choose when executing a side effect if we want it to be blocking (send) or non-blocking as an attempt (trySend)

The biggest changes to this PR is:

Connect Screen, Welcome, OutOfTime, we now expect that, when subscribing to side effects, all side effects that can happen should emit at subscription. Previously the subscription of e.g OutOfTime happened on view model init, this is now moved to the subscription of uiSideEffects. This means if we stop subscribe (e.g because we are navigating away, the event will still be able to be consumed once we resume the view (e.g come back from account view)

LoginScreen: We no longer depend on the value being buffered but rather send blocking.

Splash Screen: We now re-evaluate on each Resume which destination we should go to. Edge case scenario, if user opens app, quickly closes it, come back it to a later stage, event though we never left the splash screen the destination was already evaluated and then invoked when user already left the app. Now we will re-evaluate when the splash screen is opened.

---

This change is 

[linear]

DROID-658 OutOfTimeScreen is navigated to, despite the ConnectScreen not showing

[Pururun]

Reviewed 39 of 40 files at r1, all commit messages.
Reviewable status: 39 of 40 files reviewed, 5 unresolved discussions (waiting on @Rawa)

---

android/app/side_effect.md line 1 at r1 (raw file):

# Side Effects

This should be removed or rewritten

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 161 at r1 (raw file):

        },
        onAccountClick = {
            navigator.navigate(AccountDestination, true) { launchSingleTop = true }

Since we probably should use navigate with single top and even only when resumed should we write a helper method for this so it is not forgotten?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt line 111 at r1 (raw file):

    single { PortRangeUseCase(get()) }
    single { RelayListUseCase(get(), get()) }
    single { OutOfTimeUseCase(get(), get(), MainScope()) }

Would this risk calls to the service/daemon being done on the main thread?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 132 at r1 (raw file):

    }

    private fun outOfTimeEffect() =

⭐

---

android/test/mockapi/src/main/kotlin/net/mullvad/mullvadvpn/test/mockapi/AccountExpiryMockApiTest.kt line 53 at r1 (raw file):

    @Test
    @Disabled(

Woho

[Rawa]

Reviewable status: 39 of 40 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/app/side_effect.md line 1 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should be removed or rewritten

Thanks, it should be removed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 161 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Since we probably should use navigate with single top and even only when resumed should we write a helper method for this so it is not forgotten?

I'm open to it. Technically though I'm not sure we will always need launchSingleTop, and in many of these cases they are a bit doing "double" safety. E.g single top prevents us from having multiple AccountScreen in our backstack, but at the same time we only navigate there from Connect, and we protect ourself from duplicate navigation with the onlyIfResumed flag. So on side I agree, it makes it easier for us to hide this away, but on the otherside we make our code a bit more complicated by not showing explicitly what we are doing. We wouldn't stand to gain much in terms of lines of code since it is all on one line, so it would be for the fact that we might miss a launchSingleTop when we want it.

What do you think @albin-mullvad?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt line 111 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Would this risk calls to the service/daemon being done on the main thread?

I don't believe so, since we use AccountRepository & MessageHandler in the OutOfTimeUseCase. It would indeed run on the main thread and I think later on it would be an improvement to looking into moving logic like this over to an IO dispatcher. From what I've read and my understanding (source: https://stackoverflow.com/a/63728529 & https://medium.com/androiddevelopers/coroutines-patterns-for-work-that-shouldnt-be-cancelled-e26c40f142ad ) this should be acceptable to do.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 132 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

⭐

❤️

---

android/test/mockapi/src/main/kotlin/net/mullvad/mullvadvpn/test/mockapi/AccountExpiryMockApiTest.kt line 53 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Woho

🎉

[Pururun]

Reviewed 1 of 40 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/di/UiModule.kt line 111 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I don't believe so, since we use AccountRepository & MessageHandler in the OutOfTimeUseCase. It would indeed run on the main thread and I think later on it would be an improvement to looking into moving logic like this over to an IO dispatcher. From what I've read and my understanding (source: https://stackoverflow.com/a/63728529 & https://medium.com/androiddevelopers/coroutines-patterns-for-work-that-shouldnt-be-cancelled-e26c40f142ad ) this should be acceptable to do.

Right let's leave it like that then. :)

[albin-mullvad]

Reviewed 7 of 40 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/app/side_effect.md line 1 at r1 (raw file):

# Side Effects

We might want keep some of this documentation in a new document android/app/docs/architecture.md

How about starting such a document?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effect.kt line 12 at r1 (raw file):

@Composable
inline fun <T> LaunchedEffectCollect(

I would like to discuss this function versus CollectSideEffectWithLifecycle. It seems to me that we might always want to use the lifecycle aware variant 🤔

Code quote:

LaunchedEffectCollect

[Rawa]

Reviewable status: 38 of 40 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/util/Effect.kt line 12 at r1 (raw file):

Previously, albin-mullvad wrote…

I would like to discuss this function versus CollectSideEffectWithLifecycle. It seems to me that we might always want to use the lifecycle aware variant 🤔

Discussed offline.

Conclusion, most of the cases can probably be bound by the lifecycle. However, we should take each case into consideration individually since events triggered by UI does not get replayed and if we don't consume and miss them we might get unexpected behavior. So to limit the scope of this PR we use LaunchedEffectCollect until we've gone through each individual collection.

---

android/app/side_effect.md line 1 at r1 (raw file):

Previously, albin-mullvad wrote…

We might want keep some of this documentation in a new document android/app/docs/architecture.md

How about starting such a document?

I agree, we should have such a document. I'd say we add it in a separate PR and after we've had internal meeting about plan for architecture in the feature. What do you think?

[albin-mullvad]

Reviewed 31 of 40 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Pururun and @Rawa)

---

-- commits line 5 at r2:
Make shorter and skip initial space

Code quote:

  Update OutOfTimeUseCase to be reactive, and adjust related view models to handle side effect correctly

---

-- commits line 13 at r2:
This message subject hurts by eyes 😆

I suggest going with just: Re-enable test

Code quote:

Re-enable testAccountTimeExpiredWhileUsingTheAppShouldShowOutOfTimeScreen

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 161 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I'm open to it. Technically though I'm not sure we will always need launchSingleTop, and in many of these cases they are a bit doing "double" safety. E.g single top prevents us from having multiple AccountScreen in our backstack, but at the same time we only navigate there from Connect, and we protect ourself from duplicate navigation with the onlyIfResumed flag. So on side I agree, it makes it easier for us to hide this away, but on the otherside we make our code a bit more complicated by not showing explicitly what we are doing. We wouldn't stand to gain much in terms of lines of code since it is all on one line, so it would be for the fact that we might miss a launchSingleTop when we want it.

What do you think @albin-mullvad?

Imo it would be nice if we could somehow transparantely use a decided default behavior for all navigate calls, however since we are still figuring out how handle it in terms of "double"/redundant safety and lifecycle (e.g. dialogs) I believe it probably makes sense to wait a bit with looking into if/how we can define a default behavior

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 49 at r2 (raw file):

        }

    // What if we already are out of time?

Remove or clarify

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ConnectViewModel.kt line 190 at r2 (raw file):

    }

    // isOutOfTime=true and we should navigate to OutOfTime view.

nit: I'm not sure this comment adds much value compared to the code.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ConnectViewModel.kt line 194 at r2 (raw file):

        outOfTimeUseCase.isOutOfTime.filter { it == true }.map { UiSideEffect.OutOfTime }

    // If DeviceState is Revoked we should navigate to the RevokedDevice screen.

nit: I'm not sure this comment adds much value compared to the code.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModel.kt line 104 at r2 (raw file):

                when (val result = loginDeferred.awaitWithTimeoutOrNull(LOGIN_TIMEOUT_MILLIS)) {
                    LoginResult.Ok -> {
                        newDeviceNotificationUseCase.newDeviceCreated()

Is this new order significant? In that case we should probably make sure to document that somehow.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 133 at r2 (raw file):

    private fun outOfTimeEffect() =
        // If the user is out of time, navigate to the out of time screen.

Incorrect comment. I suggest skippit it

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 90 at r2 (raw file):

    private fun outOfTimeEffect() =
        // If the user is out of time, navigate to the out of time screen.

It looks like this comment is incorrect after being copied from somewhere else. Since it also doesn't add much value compared to the code I suggest skipping it

---

android/app/side_effect.md line 1 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I agree, we should have such a document. I'd say we add it in a separate PR and after we've had internal meeting about plan for architecture in the feature. What do you think?

Sure 👍

[Rawa]

Reviewable status: 22 of 40 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCase.kt line 49 at r2 (raw file):

Previously, albin-mullvad wrote…

Remove or clarify

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/LoginViewModel.kt line 104 at r2 (raw file):

Previously, albin-mullvad wrote…

Is this new order significant? In that case we should probably make sure to document that somehow.

Not really significant, it was just more clear. Technically, since a launch is happening below it could finish and launch a side effect and navigate away, however in practice it would never happen. This ensures it technically can't happen either, so it is just a small matter of correctness, and doesn't really need a comment I feel.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/OutOfTimeViewModel.kt line 133 at r2 (raw file):

Previously, albin-mullvad wrote…

Incorrect comment. I suggest skippit it

Agree, I've removed comment an d changed the function name to more correctly represent what it does.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 90 at r2 (raw file):

Previously, albin-mullvad wrote…

It looks like this comment is incorrect after being copied from somewhere else. Since it also doesn't add much value compared to the code I suggest skipping it

Agree, I've removed comment an d changed the function name to more correctly represent what it does.

[albin-mullvad]

Reviewed 18 of 18 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

-- commits line 15 at r3:
Nit: typo "simply"?

[Pururun]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

[albin-mullvad]

Reviewed 1 of 40 files at r1, 7 of 7 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Rawa)

---

-- commits line 13 at r5:
nit: this is a bit hard to read. I suggest going with just Re-enable test or Re-enable expiry test

Code quote:

Re-enable testAccountTimeExpiredWhileUsingTheAppShouldShowOutOfTimeScreen

---

-- commits line 18 at r5:
I suggest rephrasing this commit message subject if that wasn't already the plan. We have a plan/idea to improve this, but atm this commit rather reverts to something similar to the previous behavior. We should probably avoid the word "temp" unless we're sure when it will be addressed.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 88 at r5 (raw file):

    }

    private fun hasAddedTime() =

Can we clarify that this is a flow of potential updates? It's easy to mistake this for a regular boolean return function

Code quote:

 hasAddedTime

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 91 at r5 (raw file):

        accountRepository.accountExpiryState
            .map { it.date() }
            .filterNotNull()

I believe mapNotNull can be used here

Code quote:

            .map { it.date() }
            .filterNotNull()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 94 at r5 (raw file):

            .filter { it.minusHours(MIN_TIME_PAST_ACCOUNT_EXPIRY).isAfterNow }
            .map {
                paymentUseCase.resetPurchaseResult()

This call isn't really part of mapping the data. so this map should probably be split into:

.onEach { paymentUseCase.resetPurchaseResult() }
.map {UiSideEffect.OpenConnectScreen }

Code quote:

paymentUseCase.resetPurchaseResult()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 150 at r5 (raw file):

    companion object {
        private const val MIN_TIME_PAST_ACCOUNT_EXPIRY = 20

Clarify that it's time in hours

Code quote:

MIN_TIME_PAST_ACCOUNT_EXPIRY

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCaseTest.kt line 174 at r5 (raw file):

            AccountExpiry.Available(initialAccountExpiry.expiryDateTime.plusDays(30))
        // Act, Assert
        outOfTimeUseCase.isOutOfTime.test {

Please add some comments to this test like you've done for the other tests. It really helped quickly understanding the intended test/behavior.

[Rawa]

Reviewable status: 38 of 40 files reviewed, 8 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

-- commits line 15 at r3:

Previously, albin-mullvad wrote…

Nit: typo "simply"?

Thanks, should be simplied effects

---

-- commits line 13 at r5:

Previously, albin-mullvad wrote…

nit: this is a bit hard to read. I suggest going with just Re-enable test or Re-enable expiry test

Thought I fixed this but I believe I goofed my self with rebase and not using reword. Fixed now.

---

-- commits line 18 at r5:

Previously, albin-mullvad wrote…

I suggest rephrasing this commit message subject if that wasn't already the plan. We have a plan/idea to improve this, but atm this commit rather reverts to something similar to the previous behavior. We should probably avoid the word "temp" unless we're sure when it will be addressed.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 88 at r5 (raw file):

Previously, albin-mullvad wrote…

Can we clarify that this is a flow of potential updates? It's easy to mistake this for a regular boolean return function

I've renamed it to hasAddedTimeEffect, to be in line with other ViewModels. I think later once we reworked this logic and to look at expiry from account creation, it can be provided by a usecase as a boolean and then the viewmodel can filter and convert to side effect.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 91 at r5 (raw file):

Previously, albin-mullvad wrote…

I believe mapNotNull can be used here

Thanks, good catch! 🎯

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 94 at r5 (raw file):

Previously, albin-mullvad wrote…

This call isn't really part of mapping the data. so this map should probably be split into:

.onEach { paymentUseCase.resetPurchaseResult() }
.map {UiSideEffect.OpenConnectScreen }

I agree, that is much more clear.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/WelcomeViewModel.kt line 150 at r5 (raw file):

Previously, albin-mullvad wrote…

Clarify that it's time in hours

Clarified!

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/OutOfTimeUseCaseTest.kt line 174 at r5 (raw file):

            AccountExpiry.Available(initialAccountExpiry.expiryDateTime.plusDays(30))
        // Act, Assert
        outOfTimeUseCase.isOutOfTime.test {

Added! 💯

[albin-mullvad]

Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[albin-mullvad]

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved