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.
2024-05-30 19:14:43 +02:00 · 2024-05-30 19:14:43 +02:00 · 4520a29c74
parent 51c4469e85
commit 4520a29c74
14 changed files with 70 additions and 53 deletions
--- a/app/lint-baseline.xml
+++ b/app/lint-baseline.xml
@ -1481,6 +1481,28 @@
            column="13"/>
    </issue>

+    <issue
+        id="UnusedResources"
+        message="The resource `R.string.failed_to_pin` appears to be unused"
+        errorLine1="    &lt;string name=&quot;failed_to_pin&quot;>Failed to Pin&lt;/string>"
+        errorLine2="            ~~~~~~~~~~~~~~~~~~~~">
+        <location
+            file="src/main/res/values/strings.xml"
+            line="444"
+            column="13"/>
+    </issue>
+
+    <issue
+        id="UnusedResources"
+        message="The resource `R.string.failed_to_unpin` appears to be unused"
+        errorLine1="    &lt;string name=&quot;failed_to_unpin&quot;>Failed to Unpin&lt;/string>"
+        errorLine2="            ~~~~~~~~~~~~~~~~~~~~~~">
+        <location
+            file="src/main/res/values/strings.xml"
+            line="445"
+            column="13"/>
+    </issue>
+
    <issue
        id="UnusedResources"
        message="The resource `R.string.description_login` appears to be unused"
@ -1591,6 +1613,17 @@
            column="13"/>
    </issue>

+    <issue
+        id="UnusedResources"
+        message="The resource `R.string.ui_error_unknown` appears to be unused"
+        errorLine1="    &lt;string name=&quot;ui_error_unknown&quot;>unknown reason&lt;/string>"
+        errorLine2="            ~~~~~~~~~~~~~~~~~~~~~~~">
+        <location
+            file="src/main/res/values/strings.xml"
+            line="640"
+            column="13"/>
+    </issue>
+
    <issue
        id="UnusedResources"
        message="The resource `R.string.hint_filter_title` appears to be unused"
--- a/app/src/main/java/app/pachli/EditProfileActivity.kt
+++ b/app/src/main/java/app/pachli/EditProfileActivity.kt
@ -41,6 +41,7 @@ import app.pachli.core.common.extensions.viewBinding
 import app.pachli.core.data.model.InstanceInfo.Companion.DEFAULT_MAX_ACCOUNT_FIELDS
 import app.pachli.core.designsystem.R as DR
 import app.pachli.core.ui.extensions.await
+import app.pachli.core.ui.extensions.getErrorString
 import app.pachli.databinding.ActivityEditProfileBinding
 import app.pachli.util.Error
 import app.pachli.util.Loading
@ -221,7 +222,7 @@ class EditProfileActivity : BaseActivity() {
                    binding.saveProgressBar.visibility = View.VISIBLE
                }
                is Error -> {
-                    onSaveFailure(it.errorMessage)
+                    onSaveFailure(it.cause?.getErrorString(this))
                }
            }
        }
--- 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)),
                    )
                }
            }
--- 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()
--- 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(
--- 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(
--- 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)
--- 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<T : IStatusViewData> : 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()
                        }
                    }
--- 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)
--- 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))
                },
            )
        }
--- 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(),
            )
        }
    }
--- 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()
--- 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)
--- 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")