From 2349f970a8444c105bf200bf009a9d3f690d1727 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn <matthias.urhahn@sap.com> Date: Wed, 28 Apr 2021 19:33:23 +0200 Subject: [PATCH] Improve error handling and popups (DEV, EXPOSUREAPP-6518) (#2995) * Show errors when trying to gain ENS system permission. * Handle missing ENS components gracefully by defaulting to longer polling delays on temporary errors, and aborting without error on permanent errors (i.e. not installed). * Introduce "KnownError" concept, to remove some of the error dialog chaos. * Add wiring to display corona test errors via HomeFragment. * Fix lints * Move files into main module that were accidentally placed in tester only module. * Adjust error title for tests. * Address PR comments. Co-authored-by: harambasicluka <64483219+harambasicluka@users.noreply.github.com> --- .../exceptions/MappedException.kt | 29 +++++++++ .../bugreporting/ui/ErrorDialog.kt | 61 +++++++++++++++++++ .../CoronaTestRepositoryExtensions.kt | 27 ++++++++ .../modules/tracing/DefaultTracingStatus.kt | 33 +++++++--- .../ui/settings/SettingsTracingFragment.kt | 5 ++ .../SettingsTracingFragmentViewModel.kt | 5 +- .../ui/main/home/HomeFragment.kt | 13 ++++ .../ui/main/home/HomeFragmentViewModel.kt | 4 ++ .../onboarding/OnboardingTracingFragment.kt | 4 ++ .../OnboardingTracingFragmentViewModel.kt | 2 + .../util/HasHumanReadableError.kt | 3 +- .../util/errors/ExceptionExtensions.kt | 6 ++ .../tracing/DefaultTracingStatusTest.kt | 29 +++++++++ 13 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/exceptions/MappedException.kt create mode 100644 Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/ui/ErrorDialog.kt diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/exceptions/MappedException.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/exceptions/MappedException.kt new file mode 100644 index 000000000..dcff207f6 --- /dev/null +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/exceptions/MappedException.kt @@ -0,0 +1,29 @@ +package de.rki.coronawarnapp.bugreporting.exceptions + +import android.content.Context +import com.google.android.gms.common.api.ApiException +import de.rki.coronawarnapp.R +import de.rki.coronawarnapp.util.HumanReadableError + +interface KnownThrowable { + fun matches(throwable: Throwable): Boolean + + fun getReadableError(context: Context): HumanReadableError +} + +enum class MappedException : KnownThrowable { + // ApiException with StatusCode 17 + ENS_NOT_INSTALLED { + override fun matches(throwable: Throwable): Boolean = throwable is ApiException && throwable.statusCode == 17 + + override fun getReadableError(context: Context): HumanReadableError = HumanReadableError( + title = "${context.getString(R.string.errors_generic_details_headline)}: 3\n" + + context.getString(R.string.errors_generic_headline), + description = context.getString(R.string.errors_google_update_needed) + ) + }, +} + +fun Throwable.findKnownError(context: Context): HumanReadableError? { + return MappedException.values().find { it.matches(this@findKnownError) }?.getReadableError(context) +} diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/ui/ErrorDialog.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/ui/ErrorDialog.kt new file mode 100644 index 000000000..49e16cef4 --- /dev/null +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/bugreporting/ui/ErrorDialog.kt @@ -0,0 +1,61 @@ +package de.rki.coronawarnapp.bugreporting.ui + +import android.content.Context +import android.text.SpannableString +import android.text.method.LinkMovementMethod +import android.text.util.Linkify +import android.widget.TextView +import androidx.appcompat.app.AlertDialog +import com.google.android.material.dialog.MaterialAlertDialogBuilder +import de.rki.coronawarnapp.R +import de.rki.coronawarnapp.util.ContextExtensions.getColorStateListCompat +import de.rki.coronawarnapp.util.tryHumanReadableError +import java.util.regex.Pattern + +private fun MaterialAlertDialogBuilder.setMessageView( + message: String, + textHasLinks: Boolean, +) { + // create spannable and add links, removed stack trace links into nowhere + val spannable = SpannableString(message) + val httpPattern: Pattern = Pattern.compile("[a-z]+://[^ \\n]*") + Linkify.addLinks(spannable, httpPattern, "") + + val paddingStartEnd = context.resources.getDimension(R.dimen.spacing_normal).toInt() + val paddingLeftRight = context.resources.getDimension(R.dimen.spacing_small).toInt() + + val textView = TextView(context).apply { + text = spannable + linksClickable = true + movementMethod = LinkMovementMethod.getInstance() + setPadding( + paddingStartEnd, + paddingLeftRight, + paddingStartEnd, + paddingLeftRight + ) + setTextAppearance(R.style.body1) + setLinkTextColor(context.getColorStateListCompat(R.color.button_primary)) + setTextIsSelectable(!textHasLinks) + } + setView(textView) +} + +fun Throwable.toErrorDialogBuilder(context: Context) = MaterialAlertDialogBuilder(context).apply { + val error = this@toErrorDialogBuilder + val humanReadable = error.tryHumanReadableError(context) + + setTitle(humanReadable.title ?: context.getString(R.string.errors_generic_headline_short)) + setMessageView(humanReadable.description, textHasLinks = true) + + setPositiveButton(R.string.errors_generic_button_positive) { _, _ -> } + + setNeutralButton(R.string.errors_generic_button_negative) { _, _ -> + AlertDialog.Builder(context).apply { + setMessageView( + error.toString() + "\n\n" + error.stackTraceToString(), + textHasLinks = false + ) + }.show() + } +} diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/coronatest/CoronaTestRepositoryExtensions.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/coronatest/CoronaTestRepositoryExtensions.kt index b7a7d6ae3..e84382983 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/coronatest/CoronaTestRepositoryExtensions.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/coronatest/CoronaTestRepositoryExtensions.kt @@ -5,7 +5,10 @@ import de.rki.coronawarnapp.coronatest.type.pcr.PCRCoronaTest import de.rki.coronawarnapp.coronatest.type.rapidantigen.RACoronaTest import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.flatMapMerge +import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map +import timber.log.Timber val CoronaTestRepository.latestPCRT: Flow<PCRCoronaTest?> get() = this.coronaTests @@ -32,3 +35,27 @@ val CoronaTestRepository.isRiskCalculationNecessary: Flow<Boolean> get() = coronaTests.map { tests -> tests.none { it.isPositive } } + +// This in memory to not show duplicate errors +// CoronaTest.lastError is also only in memory +private val consumedErrors = mutableMapOf<String, Throwable?>() + +val CoronaTestRepository.testErrorsSingleEvent: Flow<List<CoronaTest>> + get() = coronaTests + .map { tests -> + tests + .filter { + val consumedClass = consumedErrors[it.identifier]?.javaClass + consumedClass != it.lastError?.javaClass + } + .onEach { + Timber.v("Unconsumed error for %s: %s", it.identifier, it.lastError?.toString()) + consumedErrors[it.identifier] = it.lastError + } + .filter { it.lastError != null } + } + .flatMapMerge { tests -> + // First we emit the tests with errors + // then an empty list because the errors should only be displayed once + flowOf(tests, emptyList()) + } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatus.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatus.kt index e8c1e9f7d..025a3ff9f 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatus.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatus.kt @@ -20,6 +20,7 @@ import kotlinx.coroutines.flow.onCompletion import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.launch +import kotlinx.coroutines.tasks.await import timber.log.Timber import javax.inject.Inject import javax.inject.Singleton @@ -84,23 +85,34 @@ class DefaultTracingStatus @Inject constructor( .addOnFailureListener { cont.resumeWithException(it) } } + @Suppress("LoopWithTooManyJumpStatements") override val isTracingEnabled: Flow<Boolean> = flow { while (true) { try { emit(isEnabled()) delay(POLLING_DELAY_MS) } catch (e: CancellationException) { - Timber.d("isBackgroundRestricted was cancelled") + Timber.tag(TAG).d("isBackgroundRestricted was cancelled") break + } catch (e: ApiException) { + emit(false) + if (e.statusCode == 17) { + // No ENS installed, no need to keep polling. + Timber.tag(TAG).v("No ENS available, aborting polling, assuming permanent.") + break + } else { + Timber.tag(TAG).v("Polling failed, will retry with backoff.") + delay(POLLING_DELAY_MS * 5) + } } } } .distinctUntilChanged() - .onStart { Timber.v("isTracingEnabled FLOW start") } - .onEach { Timber.v("isTracingEnabled FLOW emission: %b", it) } - .onCompletion { if (it == null) Timber.v("isTracingEnabled FLOW completed.") } + .onStart { Timber.tag(TAG).v("isTracingEnabled FLOW start") } + .onEach { Timber.tag(TAG).v("isTracingEnabled FLOW emission: %b", it) } + .onCompletion { if (it == null) Timber.tag(TAG).v("isTracingEnabled FLOW completed.") } .catch { - Timber.w(it, "ENF isEnabled failed.") + Timber.tag(TAG).w(it, "ENF isEnabled failed.") it.report(ExceptionCategory.EXPOSURENOTIFICATION, TAG, null) emit(false) } @@ -109,10 +121,13 @@ class DefaultTracingStatus @Inject constructor( scope = scope ) - private suspend fun isEnabled(): Boolean = suspendCoroutine { cont -> - client.isEnabled - .addOnSuccessListener { cont.resume(it) } - .addOnFailureListener { cont.resumeWithException(it) } + private suspend fun isEnabled(): Boolean = try { + client.isEnabled.await().also { + Timber.tag(TAG).v("Tracing isEnabled=$it") + } + } catch (e: Throwable) { + Timber.tag(TAG).w(e, "Failed to determine tracing status.") + throw e } companion object { diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragment.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragment.kt index a3d333a23..bc08e9d60 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragment.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragment.kt @@ -6,6 +6,7 @@ import android.view.View import android.view.accessibility.AccessibilityEvent import androidx.fragment.app.Fragment import de.rki.coronawarnapp.R +import de.rki.coronawarnapp.bugreporting.ui.toErrorDialogBuilder import de.rki.coronawarnapp.databinding.FragmentSettingsTracingBinding import de.rki.coronawarnapp.nearby.InternalExposureNotificationClient import de.rki.coronawarnapp.tracing.ui.TracingConsentDialog @@ -75,6 +76,10 @@ class SettingsTracingFragment : Fragment(R.layout.fragment_settings_tracing), Au binding.settingsTracingSwitchRow.settingsSwitchRowSwitch.isChecked = checked } + vm.ensErrorEvents.observe2(this) { error -> + error.toErrorDialogBuilder(requireContext()).show() + } + setButtonOnClickListener() } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragmentViewModel.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragmentViewModel.kt index 420e2179b..d8eedf1c3 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragmentViewModel.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/tracing/ui/settings/SettingsTracingFragmentViewModel.kt @@ -65,6 +65,8 @@ class SettingsTracingFragmentViewModel @AssistedInject constructor( } } + val ensErrorEvents = SingleLiveEvent<Throwable>() + private val tracingPermissionHelper = tracingPermissionHelperFactory.create( object : TracingPermissionHelper.Callback { @@ -96,7 +98,8 @@ class SettingsTracingFragmentViewModel @AssistedInject constructor( } override fun onError(error: Throwable) { - Timber.w(error, "Failed to start tracing") + Timber.w(error, "Failed to start tracing from settings screen.") + ensErrorEvents.postValue(error) } } ) diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragment.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragment.kt index 59df149b3..6341cf5e8 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragment.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragment.kt @@ -10,6 +10,7 @@ import androidx.navigation.fragment.findNavController import androidx.recyclerview.widget.DefaultItemAnimator import androidx.recyclerview.widget.LinearLayoutManager import de.rki.coronawarnapp.R +import de.rki.coronawarnapp.bugreporting.ui.toErrorDialogBuilder import de.rki.coronawarnapp.coronatest.type.CoronaTest import de.rki.coronawarnapp.databinding.HomeFragmentLayoutBinding import de.rki.coronawarnapp.tracing.ui.TracingExplanationDialog @@ -119,6 +120,18 @@ class HomeFragment : Fragment(R.layout.home_fragment_layout), AutoInject { if (!showDialog) return@observe2 deviceTimeIncorrectDialog.show { viewModel.userHasAcknowledgedIncorrectDeviceTime() } } + + viewModel.coronaTestErrors.observe2(this) { tests -> + tests.forEach { test -> + test.lastError?.toErrorDialogBuilder(requireContext())?.apply { + val testName = when (test.type) { + CoronaTest.Type.PCR -> R.string.ag_homescreen_card_pcr_title + CoronaTest.Type.RAPID_ANTIGEN -> R.string.ag_homescreen_card_rapidtest_title + } + setTitle(getString(testName) + " " + getString(R.string.errors_generic_headline_short)) + }?.show() + } + } } override fun onResume() { diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragmentViewModel.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragmentViewModel.kt index bdbd3098d..8a96a5c5c 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragmentViewModel.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/home/HomeFragmentViewModel.kt @@ -10,6 +10,7 @@ import de.rki.coronawarnapp.appconfig.CoronaTestConfig import de.rki.coronawarnapp.coronatest.CoronaTestRepository import de.rki.coronawarnapp.coronatest.latestPCRT import de.rki.coronawarnapp.coronatest.latestRAT +import de.rki.coronawarnapp.coronatest.testErrorsSingleEvent import de.rki.coronawarnapp.coronatest.type.CoronaTest import de.rki.coronawarnapp.coronatest.type.pcr.PCRCoronaTest import de.rki.coronawarnapp.coronatest.type.pcr.SubmissionStatePCR @@ -102,6 +103,9 @@ class HomeFragmentViewModel @AssistedInject constructor( val popupEvents = SingleLiveEvent<HomeFragmentEvents>() + val coronaTestErrors = coronaTestRepository.testErrorsSingleEvent + .asLiveData(context = dispatcherProvider.Default) + fun showPopUps() { launch { if (errorResetTool.isResetNoticeToBeShown) { diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragment.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragment.kt index 55776f95b..9c657fa36 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragment.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragment.kt @@ -7,6 +7,7 @@ import android.view.accessibility.AccessibilityEvent import androidx.fragment.app.Fragment import androidx.navigation.fragment.findNavController import de.rki.coronawarnapp.R +import de.rki.coronawarnapp.bugreporting.ui.toErrorDialogBuilder import de.rki.coronawarnapp.databinding.FragmentOnboardingTracingBinding import de.rki.coronawarnapp.ui.doNavigate import de.rki.coronawarnapp.util.DialogHelper @@ -62,6 +63,9 @@ class OnboardingTracingFragment : Fragment(R.layout.fragment_onboarding_tracing) vm.permissionRequestEvent.observe2(this) { permissionRequest -> permissionRequest.invoke(requireActivity()) } + vm.ensErrorEvents.observe2(this) { error -> + error.toErrorDialogBuilder(requireContext()).show() + } } override fun onResume() { diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragmentViewModel.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragmentViewModel.kt index dca5b110f..94f0dadf7 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragmentViewModel.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/onboarding/OnboardingTracingFragmentViewModel.kt @@ -28,6 +28,7 @@ class OnboardingTracingFragmentViewModel @AssistedInject constructor( .asLiveData(context = dispatcherProvider.Default) val routeToScreen: SingleLiveEvent<OnboardingNavigationEvents> = SingleLiveEvent() val permissionRequestEvent = SingleLiveEvent<(Activity) -> Unit>() + val ensErrorEvents = SingleLiveEvent<Throwable>() private val tracingPermissionHelper = tracingPermissionHelperFactory.create( @@ -49,6 +50,7 @@ class OnboardingTracingFragmentViewModel @AssistedInject constructor( override fun onError(error: Throwable) { Timber.e(error, "Failed to activate tracing during onboarding.") + ensErrorEvents.postValue(error) } } ) diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/HasHumanReadableError.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/HasHumanReadableError.kt index 37eebfda6..87de83108 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/HasHumanReadableError.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/HasHumanReadableError.kt @@ -1,6 +1,7 @@ package de.rki.coronawarnapp.util import android.content.Context +import de.rki.coronawarnapp.bugreporting.exceptions.findKnownError import de.rki.coronawarnapp.util.ui.LazyString interface HasHumanReadableError { @@ -15,7 +16,7 @@ data class HumanReadableError( fun Throwable.tryHumanReadableError(context: Context): HumanReadableError = when (this) { is HasHumanReadableError -> this.toHumanReadableError(context) else -> { - HumanReadableError( + findKnownError(context) ?: HumanReadableError( description = (localizedMessage ?: this.message) ?: this.toString() ) } diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/ExceptionExtensions.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/ExceptionExtensions.kt index f2d986507..50407b262 100644 --- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/ExceptionExtensions.kt +++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/ExceptionExtensions.kt @@ -9,3 +9,9 @@ fun Throwable.causes(): Sequence<Throwable> = sequence { error = error.cause ?: break } } + +fun Throwable.stackTraceToStringList(): List<String> { + return stackTrace.toList().map { traceElement -> + traceElement.toString() + } +} diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatusTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatusTest.kt index a9a402b39..b8d6def71 100644 --- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatusTest.kt +++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/nearby/modules/tracing/DefaultTracingStatusTest.kt @@ -1,5 +1,7 @@ package de.rki.coronawarnapp.nearby.modules.tracing +import com.google.android.gms.common.api.ApiException +import com.google.android.gms.common.api.Status import com.google.android.gms.nearby.exposurenotification.ExposureNotificationClient import de.rki.coronawarnapp.storage.TracingSettings import io.kotest.matchers.shouldBe @@ -100,4 +102,31 @@ class DefaultTracingStatusTest : BaseTest() { every { client.isEnabled } answers { MockGMSTask.forValue(false) } instance.isTracingEnabled.first() shouldBe false } + + @Test + fun `api errors during state polling are mapped to false`() = runBlockingTest2(ignoreActive = true) { + every { client.isEnabled } answers { MockGMSTask.forError(ApiException(Status.RESULT_INTERNAL_ERROR)) } + + val instance = createInstance(scope = this) + + instance.isTracingEnabled.first() shouldBe false + } + + @Test + fun `api errors during state setting are rethrown`() = runBlockingTest2(ignoreActive = true) { + val ourError = ApiException(Status.RESULT_INTERNAL_ERROR) + every { client.isEnabled } answers { MockGMSTask.forError(ourError) } + + val instance = createInstance(scope = this) + + var thrownError: Throwable? = null + instance.setTracing( + enable = true, + onSuccess = {}, + onError = { thrownError = it }, + onPermissionRequired = {} + ) + + thrownError shouldBe ourError + } } -- GitLab