From 4520a29c749460b09284a8c523160805c2dcb2c9 Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Thu, 30 May 2024 19:14:43 +0200 Subject: [PATCH] fix: Generate useful error messages for all errors (#719) Previous code was inconsistent about using getServerErrorMessage() and whether or not the case where getServerErrorMessage() returns null was handled. Switch to using getErrorString() which does handle the null case and always returns a usable string. Some string resources are rendered temporarily unused by this change. They will be used again soon, so configure lint to ignore them at the moment. --- app/lint-baseline.xml | 33 +++++++++++++++++++ .../java/app/pachli/EditProfileActivity.kt | 3 +- .../components/compose/ComposeActivity.kt | 8 ++--- .../components/compose/MediaUploader.kt | 9 ++--- .../notifications/NotificationsFragment.kt | 8 ++--- .../components/timeline/TimelineFragment.kt | 7 +--- .../viewthread/ViewThreadFragment.kt | 8 ++--- .../java/app/pachli/fragment/SFragment.kt | 4 +-- .../java/app/pachli/usecase/TimelineCases.kt | 5 +-- .../pachli/viewmodel/EditProfileViewModel.kt | 3 +- .../app/pachli/usecase/TimelineCasesTest.kt | 5 +-- .../core/ui/extensions/ThrowableExtensions.kt | 24 +++++++++----- feature/login/build.gradle.kts | 1 + .../app/pachli/feature/login/LoginActivity.kt | 5 ++- 14 files changed, 70 insertions(+), 53 deletions(-) diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index 83534adaa..7148a83bc 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -1481,6 +1481,28 @@ column="13"/> + + + + + + + + + + + + { - onSaveFailure(it.errorMessage) + onSaveFailure(it.cause?.getErrorString(this)) } } } diff --git a/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt b/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt index bc05abc56..f1083489a 100644 --- a/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt +++ b/app/src/main/java/app/pachli/components/compose/ComposeActivity.kt @@ -80,7 +80,6 @@ import app.pachli.core.common.extensions.show import app.pachli.core.common.extensions.viewBinding import app.pachli.core.common.extensions.visible import app.pachli.core.common.string.mastodonLength -import app.pachli.core.common.string.unicodeWrap import app.pachli.core.data.model.InstanceInfo.Companion.DEFAULT_CHARACTER_LIMIT import app.pachli.core.data.model.InstanceInfo.Companion.DEFAULT_MAX_MEDIA_ATTACHMENTS import app.pachli.core.database.model.AccountEntity @@ -88,13 +87,13 @@ import app.pachli.core.designsystem.R as DR import app.pachli.core.navigation.ComposeActivityIntent import app.pachli.core.navigation.ComposeActivityIntent.ComposeOptions import app.pachli.core.navigation.ComposeActivityIntent.ComposeOptions.InitialCursorPosition -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Attachment import app.pachli.core.network.model.Emoji import app.pachli.core.network.model.Status import app.pachli.core.preferences.AppTheme import app.pachli.core.preferences.PrefKeys import app.pachli.core.preferences.SharedPreferencesRepository +import app.pachli.core.ui.extensions.getErrorString import app.pachli.databinding.ActivityComposeBinding import app.pachli.util.PickMediaFiles import app.pachli.util.getInitialLanguages @@ -513,10 +512,7 @@ class ComposeActivity : displayTransientMessage(throwable.errorMessage) } else { displayTransientMessage( - getString( - R.string.error_media_upload_sending_fmt, - throwable.getServerErrorMessage().unicodeWrap(), - ), + getString(R.string.error_media_upload_sending_fmt, throwable.getErrorString(this@ComposeActivity)), ) } } diff --git a/app/src/main/java/app/pachli/components/compose/MediaUploader.kt b/app/src/main/java/app/pachli/components/compose/MediaUploader.kt index 256d5d566..920987ea6 100644 --- a/app/src/main/java/app/pachli/components/compose/MediaUploader.kt +++ b/app/src/main/java/app/pachli/components/compose/MediaUploader.kt @@ -31,8 +31,8 @@ import app.pachli.R import app.pachli.components.compose.ComposeActivity.QueuedMedia import app.pachli.core.common.string.randomAlphanumericString import app.pachli.core.data.model.InstanceInfo -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.MediaUploadApi +import app.pachli.core.ui.extensions.getErrorString import app.pachli.network.ProgressRequestBody import app.pachli.util.MEDIA_SIZE_UNKNOWN import app.pachli.util.getImageSquarePixels @@ -313,12 +313,7 @@ class MediaUploader @Inject constructor( send(UploadEvent.FinishedEvent(responseBody.id, uploadResponse.code() == 200)) } else { val error = HttpException(uploadResponse) - val errorMessage = error.getServerErrorMessage() - if (errorMessage == null) { - throw error - } else { - throw UploadServerError(errorMessage) - } + throw UploadServerError(error.getErrorString(context)) } awaitClose() diff --git a/app/src/main/java/app/pachli/components/notifications/NotificationsFragment.kt b/app/src/main/java/app/pachli/components/notifications/NotificationsFragment.kt index 2eb10226c..c87f7e2e9 100644 --- a/app/src/main/java/app/pachli/components/notifications/NotificationsFragment.kt +++ b/app/src/main/java/app/pachli/components/notifications/NotificationsFragment.kt @@ -52,15 +52,14 @@ import app.pachli.core.activity.openLink import app.pachli.core.common.extensions.hide import app.pachli.core.common.extensions.show import app.pachli.core.common.extensions.viewBinding -import app.pachli.core.common.string.unicodeWrap import app.pachli.core.navigation.AttachmentViewData.Companion.list -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Filter import app.pachli.core.network.model.Notification import app.pachli.core.network.model.Poll import app.pachli.core.network.model.Status import app.pachli.core.ui.ActionButtonScrollListener import app.pachli.core.ui.BackgroundMessage +import app.pachli.core.ui.extensions.getErrorString import app.pachli.databinding.FragmentTimelineNotificationsBinding import app.pachli.fragment.SFragment import app.pachli.interfaces.AccountActionListener @@ -235,10 +234,7 @@ class NotificationsFragment : viewModel.uiError.collect { error -> val message = getString( error.message, - ( - error.throwable.getServerErrorMessage() ?: error.throwable.localizedMessage - ?: getString(R.string.ui_error_unknown) - ).unicodeWrap(), + error.throwable.getErrorString(requireContext()), ) Timber.d(error.throwable, message) val snackbar = Snackbar.make( diff --git a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt index dcd00d4dc..27610c3b7 100644 --- a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt +++ b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt @@ -54,12 +54,10 @@ import app.pachli.core.activity.extensions.startActivityWithDefaultTransition import app.pachli.core.common.extensions.hide import app.pachli.core.common.extensions.show import app.pachli.core.common.extensions.viewBinding -import app.pachli.core.common.string.unicodeWrap import app.pachli.core.database.model.TranslationState import app.pachli.core.model.Timeline import app.pachli.core.navigation.AccountListActivityIntent import app.pachli.core.navigation.AttachmentViewData -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Poll import app.pachli.core.network.model.Status import app.pachli.core.ui.ActionButtonScrollListener @@ -237,10 +235,7 @@ class TimelineFragment : viewModel.uiError.collect { error -> val message = getString( error.message, - ( - error.throwable.getServerErrorMessage() ?: error.throwable.localizedMessage - ?: getString(R.string.ui_error_unknown) - ).unicodeWrap(), + error.throwable.getErrorString(requireContext()), ) Timber.d(error.throwable, message) snackbar = Snackbar.make( diff --git a/app/src/main/java/app/pachli/components/viewthread/ViewThreadFragment.kt b/app/src/main/java/app/pachli/components/viewthread/ViewThreadFragment.kt index 0ae9b163b..be9b09653 100644 --- a/app/src/main/java/app/pachli/components/viewthread/ViewThreadFragment.kt +++ b/app/src/main/java/app/pachli/components/viewthread/ViewThreadFragment.kt @@ -38,13 +38,12 @@ import app.pachli.core.activity.openLink import app.pachli.core.common.extensions.hide import app.pachli.core.common.extensions.show import app.pachli.core.common.extensions.viewBinding -import app.pachli.core.common.string.unicodeWrap import app.pachli.core.designsystem.R as DR import app.pachli.core.navigation.AccountListActivityIntent import app.pachli.core.navigation.AttachmentViewData.Companion.list -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Poll import app.pachli.core.network.model.Status +import app.pachli.core.ui.extensions.getErrorString import app.pachli.databinding.FragmentViewThreadBinding import app.pachli.fragment.SFragment import app.pachli.interfaces.StatusActionListener @@ -211,10 +210,7 @@ class ViewThreadFragment : lifecycleScope.launch { viewModel.errors.collect { throwable -> Timber.w(throwable, "failed to load status context") - val msg = view.context.getString( - app.pachli.core.ui.R.string.error_generic_fmt, - throwable.getServerErrorMessage().unicodeWrap(), - ) + val msg = throwable.getErrorString(view.context) Snackbar.make(binding.root, msg, Snackbar.LENGTH_INDEFINITE) .setAction(app.pachli.core.ui.R.string.action_retry) { viewModel.retry(thisThreadsStatusId) diff --git a/app/src/main/java/app/pachli/fragment/SFragment.kt b/app/src/main/java/app/pachli/fragment/SFragment.kt index 4b06b9a10..0af1634c8 100644 --- a/app/src/main/java/app/pachli/fragment/SFragment.kt +++ b/app/src/main/java/app/pachli/fragment/SFragment.kt @@ -60,6 +60,7 @@ import app.pachli.core.network.model.Attachment import app.pachli.core.network.model.Status import app.pachli.core.network.parseAsMastodonHtml import app.pachli.core.network.retrofit.MastodonApi +import app.pachli.core.ui.extensions.getErrorString import app.pachli.interfaces.StatusActionListener import app.pachli.network.ServerRepository import app.pachli.usecase.TimelineCases @@ -339,8 +340,7 @@ abstract class SFragment : Fragment(), StatusActionListener R.id.pin -> { lifecycleScope.launch { timelineCases.pin(status.id, !status.isPinned()).onFailure { e: Throwable -> - val message = e.message - ?: getString(if (status.isPinned()) R.string.failed_to_unpin else R.string.failed_to_pin) + val message = e.getErrorString(requireContext()) Snackbar.make(requireView(), message, Snackbar.LENGTH_LONG).show() } } diff --git a/app/src/main/java/app/pachli/usecase/TimelineCases.kt b/app/src/main/java/app/pachli/usecase/TimelineCases.kt index 7fab5478c..c0ac6674a 100644 --- a/app/src/main/java/app/pachli/usecase/TimelineCases.kt +++ b/app/src/main/java/app/pachli/usecase/TimelineCases.kt @@ -27,7 +27,6 @@ import app.pachli.appstore.PollVoteEvent import app.pachli.appstore.ReblogEvent import app.pachli.appstore.StatusDeletedEvent import app.pachli.components.timeline.CachedTimelineRepository -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.DeletedStatus import app.pachli.core.network.model.Poll import app.pachli.core.network.model.Relationship @@ -122,7 +121,7 @@ class TimelineCases @Inject constructor( NetworkResult.success(status) }, { e -> Timber.w(e, "Failed to change pin state") - NetworkResult.failure(TimelineError(e.getServerErrorMessage())) + NetworkResult.failure(e) }) } @@ -152,5 +151,3 @@ class TimelineCases @Inject constructor( cachedTimelineRepository.translateUndo(statusViewData) } } - -class TimelineError(message: String?) : RuntimeException(message) diff --git a/app/src/main/java/app/pachli/viewmodel/EditProfileViewModel.kt b/app/src/main/java/app/pachli/viewmodel/EditProfileViewModel.kt index e3ba22795..9afe9f56d 100644 --- a/app/src/main/java/app/pachli/viewmodel/EditProfileViewModel.kt +++ b/app/src/main/java/app/pachli/viewmodel/EditProfileViewModel.kt @@ -26,7 +26,6 @@ import app.pachli.appstore.EventHub import app.pachli.appstore.ProfileEditedEvent import app.pachli.core.common.string.randomAlphanumericString import app.pachli.core.data.repository.InstanceInfoRepository -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Account import app.pachli.core.network.model.StringField import app.pachli.core.network.retrofit.MastodonApi @@ -151,7 +150,7 @@ class EditProfileViewModel @Inject constructor( eventHub.dispatch(ProfileEditedEvent(newAccountData)) }, { throwable -> - saveData.postValue(Error(errorMessage = throwable.getServerErrorMessage())) + saveData.postValue(Error(cause = throwable)) }, ) } diff --git a/app/src/test/java/app/pachli/usecase/TimelineCasesTest.kt b/app/src/test/java/app/pachli/usecase/TimelineCasesTest.kt index 7c373b7d2..a9ecbec3e 100644 --- a/app/src/test/java/app/pachli/usecase/TimelineCasesTest.kt +++ b/app/src/test/java/app/pachli/usecase/TimelineCasesTest.kt @@ -5,6 +5,7 @@ import app.cash.turbine.test import app.pachli.appstore.EventHub import app.pachli.appstore.PinEvent import app.pachli.components.timeline.CachedTimelineRepository +import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.Status import app.pachli.core.network.retrofit.MastodonApi import at.connyduck.calladapter.networkresult.NetworkResult @@ -54,7 +55,7 @@ class TimelineCasesTest { } @Test - fun `pin failure with server error throws TimelineError with server message`() { + fun `pin failure with server error returns failure with server message`() { api.stub { onBlocking { pinStatus(statusId) } doReturn NetworkResult.failure( HttpException( @@ -68,7 +69,7 @@ class TimelineCasesTest { runBlocking { assertEquals( "Validation Failed: You have already pinned the maximum number of toots", - timelineCases.pin(statusId, true).exceptionOrNull()?.message, + timelineCases.pin(statusId, true).exceptionOrNull()?.getServerErrorMessage(), ) } } diff --git a/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt b/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt index badfdd19c..0d631bb41 100644 --- a/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt +++ b/core/ui/src/main/kotlin/app/pachli/core/ui/extensions/ThrowableExtensions.kt @@ -29,7 +29,7 @@ import retrofit2.HttpException fun Throwable.getDrawableRes(): Int = when (this) { is IOException -> R.drawable.errorphant_offline is HttpException -> { - if (this.code() == 404) { + if (code() == 404) { R.drawable.elephant_friend_empty } else { R.drawable.errorphant_offline @@ -38,10 +38,18 @@ fun Throwable.getDrawableRes(): Int = when (this) { else -> R.drawable.errorphant_error } -/** @return A string error message for this throwable */ -fun Throwable.getErrorString(context: Context): String = getServerErrorMessage() ?: when (this) { - is IOException -> String.format(context.getString(R.string.error_network_fmt), this.localizedMessage.unicodeWrap()) - is HttpException -> if (this.code() == 404) String.format(context.getString(R.string.error_404_not_found_fmt), this.message.unicodeWrap()) else String.format(context.getString(R.string.error_generic_fmt), this.message.unicodeWrap()) - is JsonDataException -> String.format(context.getString(R.string.error_json_data_fmt), this.message.unicodeWrap()) - else -> String.format(context.getString(R.string.error_generic_fmt), this.message.unicodeWrap()) -} +/** @return A unicode-wrapped string error message for this throwable */ +fun Throwable.getErrorString(context: Context): String = ( + getServerErrorMessage() ?: when (this) { + is IOException -> String.format(context.getString(R.string.error_network_fmt), localizedMessage) + is HttpException -> { + if (code() == 404) { + String.format(context.getString(R.string.error_404_not_found_fmt), localizedMessage) + } else { + String.format(context.getString(R.string.error_generic_fmt), localizedMessage) + } + } + is JsonDataException -> String.format(context.getString(R.string.error_json_data_fmt), localizedMessage) + else -> String.format(context.getString(R.string.error_generic_fmt), localizedMessage) + } + ).unicodeWrap() diff --git a/feature/login/build.gradle.kts b/feature/login/build.gradle.kts index 880f3505d..360a4fb6f 100644 --- a/feature/login/build.gradle.kts +++ b/feature/login/build.gradle.kts @@ -37,6 +37,7 @@ dependencies { implementation(projects.core.navigation) implementation(projects.core.network) implementation(projects.core.preferences) + implementation(projects.core.ui) implementation(libs.bundles.androidx) implementation(libs.androidx.webkit) diff --git a/feature/login/src/main/kotlin/app/pachli/feature/login/LoginActivity.kt b/feature/login/src/main/kotlin/app/pachli/feature/login/LoginActivity.kt index 8f424417c..dcce37db5 100644 --- a/feature/login/src/main/kotlin/app/pachli/feature/login/LoginActivity.kt +++ b/feature/login/src/main/kotlin/app/pachli/feature/login/LoginActivity.kt @@ -34,13 +34,12 @@ import app.pachli.core.activity.extensions.setCloseTransition import app.pachli.core.activity.extensions.startActivityWithTransition import app.pachli.core.activity.openLinkInCustomTab import app.pachli.core.common.extensions.viewBinding -import app.pachli.core.common.string.unicodeWrap import app.pachli.core.navigation.LoginActivityIntent import app.pachli.core.navigation.MainActivityIntent -import app.pachli.core.network.extensions.getServerErrorMessage import app.pachli.core.network.model.AccessToken import app.pachli.core.network.retrofit.MastodonApi import app.pachli.core.preferences.getNonNullString +import app.pachli.core.ui.extensions.getErrorString import app.pachli.feature.login.databinding.ActivityLoginBinding import at.connyduck.calladapter.networkresult.fold import com.bumptech.glide.Glide @@ -196,7 +195,7 @@ class LoginActivity : BaseActivity() { binding.domainTextInputLayout.error = String.format( getString(R.string.error_failed_app_registration_fmt), - (e.getServerErrorMessage() ?: e.localizedMessage).unicodeWrap(), + e.getErrorString(this@LoginActivity), ) setLoading(false) Timber.e(e, "Error when creating/registing app")