Wrap all management service calls in Either

[Pururun]

This is so the app will not crash if an error occured.

Added konsist test so that all calls needs to be wrapped in Either.

---

This change is 

[linear]

DROID-1170 Ensure all potential gRPC errors are wrapped

[Rawa]

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

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 268 at r2 (raw file):

            .mapLeft(ConnectError::Unknown)

    suspend fun disconnect(): Either<ConnectError, Boolean> =

If we believe these can result in errors we should also handle them downstream, right now the new are kind of fail silent. We catch the error but we don't do anything with it. See code below, same goes for some of the others:

lib/shared/src/main/kotlin/net/mullvad/mullvadvpn/lib/shared/ConnectionProxy.kt
23:    suspend fun disconnect() = managementService.disconnect()

---

android/test/arch/src/test/kotlin/net/mullvad/mullvadvpn/test/arch/ManagementServiceTest.kt line 10 at r2 (raw file):

class ManagementServiceTest {

    @Test

Nice test ⭐

[Pururun]

Reviewable status: 4 of 19 files reviewed, 2 unresolved discussions (waiting on @Rawa)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 268 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

If we believe these can result in errors we should also handle them downstream, right now the new are kind of fail silent. We catch the error but we don't do anything with it. See code below, same goes for some of the others:

lib/shared/src/main/kotlin/net/mullvad/mullvadvpn/lib/shared/ConnectionProxy.kt
23:    suspend fun disconnect() = managementService.disconnect()

Added toast messages for those that made sense and added logs for all calls.

---

android/test/arch/src/test/kotlin/net/mullvad/mullvadvpn/test/arch/ManagementServiceTest.kt line 10 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice test ⭐

Done.

[Rawa]

Reviewed 3 of 4 files at r1, 14 of 14 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 268 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Added toast messages for those that made sense and added logs for all calls.

Good, the logs will be critical because it will be the only way for us to debug from errors logs. Can we add error logs to every single case were we map to a Unknown error? This will help us expand the Unknown cases and introduce new well-typed errors in the future.

[Pururun]

Reviewable status: 19 of 20 files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 268 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Good, the logs will be critical because it will be the only way for us to debug from errors logs. Can we add error logs to every single case were we map to a Unknown error? This will help us expand the Unknown cases and introduce new well-typed errors in the future.

Added logs for all unkown errors

[Rawa]

Reviewable status: 18 of 20 files reviewed, 1 unresolved discussion (waiting on @Pururun)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 268 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Added logs for all unkown errors

Nice 👍

[Rawa]

Reviewed 1 of 1 files at r4.
Reviewable status: 19 of 20 files reviewed, all discussions resolved

[Rawa]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[Rawa]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Rawa]

Reviewed 1 of 2 files at r6.
Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @Pururun)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 331 at r6 (raw file):

                        LoginAccountError.MaxDevicesReached(accountNumber)
                    Status.Code.UNAVAILABLE -> LoginAccountError.RpcError
                    else -> LoginAccountError.Unknown(it)

Here too

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 531 at r6 (raw file):

                when (it.status.code) {
                    Status.Code.ALREADY_EXISTS -> NameAlreadyExists(customList.name)
                    else -> UnknownCustomListError(it)

We should log on this unknown case, so we can follow up on it.

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 625 at r6 (raw file):

                    Status.Code.RESOURCE_EXHAUSTED -> RedeemVoucherError.VoucherAlreadyUsed
                    Status.Code.UNAVAILABLE -> RedeemVoucherError.RpcError
                    else -> RedeemVoucherError.Unknown(it)

Same here

[Rawa]

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

[albin-mullvad]

Reviewed 2 of 4 files at r1, 3 of 5 files at r2, 12 of 14 files at r3, 1 of 1 files at r5, 1 of 1 files at r7, 1 of 38 files at r8, 37 of 37 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 179 at r9 (raw file):

                launch { copyToClipboard(sideEffect.accountNumber, copyTextString) }
            AccountViewModel.UiSideEffect.GenericError ->
                snackbarHostState.showSnackbarImmediately(message = errorString)

Is this new toast aligned with design? Same question applies to other new toasts

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 1 at r9 (raw file):

package net.mullvad.mullvadvpn.lib.daemon.grpc

Are we sure none of the new error logs contain any sensitive information when we include it? E.g. Logger.e("Disconnect error", it)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 716 at r9 (raw file):

    ): Either<TestApiAccessMethodError, Unit> =
        Either.catch { grpc.testCustomApiAccessMethod(customProxy.fromDomain()) }
            .onLeft { Logger.e("test custom api access method error", it) }

nit: Test

Code quote:

test

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 726 at r9 (raw file):

    ): Either<TestApiAccessMethodError, Unit> =
        Either.catch { grpc.testApiAccessMethodById(apiAccessMethodId.fromDomain()) }
            .onLeft { Logger.e("test api access method error", it) }

nit: Test

Code quote:

tes

[Pururun]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 1 at r9 (raw file):

Previously, albin-mullvad wrote…

Are we sure none of the new error logs contain any sensitive information when we include it? E.g. Logger.e("Disconnect error", it)

If it is a grpc error it should not contain any sensitive information, but I guess it is possible if it is some other exception?

[albin-mullvad]

Reviewed 2 of 3 files at r10, all commit messages.
Reviewable status: 56 of 57 files reviewed, 4 unresolved discussions (waiting on @Pururun and @Rawa)

[albin-mullvad]

Reviewable status: 56 of 57 files reviewed, 4 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 1 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

If it is a grpc error it should not contain any sensitive information, but I guess it is possible if it is some other exception?

If we can't be certain we should avoid including it. Can we make it more specific or skip it? Or perhaps use duplicate log calls: one Logger.e without it and one Logger.d with it (since the debug level wouldn't be included in release builds)

[Pururun]

Reviewable status: 50 of 57 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Rawa)

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 1 at r9 (raw file):

Previously, albin-mullvad wrote…

If we can't be certain we should avoid including it. Can we make it more specific or skip it? Or perhaps use duplicate log calls: one Logger.e without it and one Logger.d with it (since the debug level wouldn't be included in release builds)

Removed all it

[albin-mullvad]

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

---

android/lib/daemon-grpc/src/main/kotlin/net/mullvad/mullvadvpn/lib/daemon/grpc/ManagementService.kt line 1 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Removed all it

👍

[Pururun]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 179 at r9 (raw file):

Previously, albin-mullvad wrote…

Is this new toast aligned with design? Same question applies to other new toasts

It has now been aligned with design. Also created an issue in linear on how we can improve our handling of grpc failures.

[albin-mullvad]

Reviewable status: complete! all files reviewed, all discussions resolved

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/AccountScreen.kt line 179 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

It has now been aligned with design. Also created an issue in linear on how we can improve our handling of grpc failures.

Perfect! 🙌