Merge pull request #2350 from element-hq/feature/bma/improveSendProblemUx

Improve send problem ux

features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportFormError.kt

@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2024 New Vector Ltd
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.element.android.features.rageshake.impl.bugreport
+
+sealed class BugReportFormError : Exception() {
+    data object DescriptionTooShort : BugReportFormError()
+}

features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt

@@ -91,7 +91,13 @@ class BugReportPresenter @Inject constructor(
 
         fun handleEvents(event: BugReportEvents) {
             when (event) {
-                BugReportEvents.SendBugReport -> appCoroutineScope.sendBugReport(formState.value, crashInfo.isNotEmpty(), uploadListener)
+                BugReportEvents.SendBugReport -> {
+                    if (formState.value.description.length < 10) {
+                        sendingAction.value = AsyncAction.Failure(BugReportFormError.DescriptionTooShort)
+                    } else {
+                        appCoroutineScope.sendBugReport(formState.value, crashInfo.isNotEmpty(), uploadListener)
+                    }
+                }
                 BugReportEvents.ResetAll -> appCoroutineScope.resetAll()
                 is BugReportEvents.SetDescription -> updateFormState(formState) {
                     copy(description = event.description)

features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportState.kt

@@ -28,8 +28,9 @@ data class BugReportState(
     val sending: AsyncAction<Unit>,
     val eventSink: (BugReportEvents) -> Unit
 ) {
-    val submitEnabled =
+    val submitEnabled = sending !is AsyncAction.Loading
-        formState.description.length > 10 && sending !is AsyncAction.Loading
+    val isDescriptionInError = sending is AsyncAction.Failure &&
+        sending.error is BugReportFormError.DescriptionTooShort
 }
 
 @Parcelize

features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportStateProvider.kt

@@ -33,6 +33,7 @@ open class BugReportStateProvider : PreviewParameterProvider<BugReportState> {
             ),
             aBugReportState().copy(sending = AsyncAction.Loading),
             aBugReportState().copy(sending = AsyncAction.Success(Unit)),
+            aBugReportState().copy(sending = AsyncAction.Failure(BugReportFormError.DescriptionTooShort)),
         )
 }
 

features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt

@@ -96,7 +96,7 @@ fun BugReportView(
                         imeAction = ImeAction.Next
                     ),
                     minLines = 3,
-                    // TODO Error text too short
+                    isError = state.isDescriptionInError,
                 )
             }
             Spacer(modifier = Modifier.height(16.dp))
@@ -167,6 +167,12 @@ fun BugReportView(
                 eventSink(BugReportEvents.ResetAll)
                 onDone()
             },
+            errorMessage = { error ->
+                when (error) {
+                    BugReportFormError.DescriptionTooShort -> stringResource(id = R.string.screen_bug_report_error_description_too_short)
+                    else -> error.message ?: error.toString()
+                }
+            },
             onErrorDismiss = { eventSink(BugReportEvents.ClearError) },
         )
     }

features/rageshake/impl/src/main/res/values/localazy.xml

@@ -7,6 +7,7 @@
   <string name="screen_bug_report_editor_description">"Please describe the problem. What did you do? What did you expect to happen? What actually happened. Please go into as much detail as you can."</string>
   <string name="screen_bug_report_editor_placeholder">"Describe the problem…"</string>
   <string name="screen_bug_report_editor_supporting">"If possible, please write the description in English."</string>
+  <string name="screen_bug_report_error_description_too_short">"The description is too short, please provide more details about what happened. Thanks!"</string>
   <string name="screen_bug_report_include_crash_logs">"Send crash logs"</string>
   <string name="screen_bug_report_include_logs">"Allow logs"</string>
   <string name="screen_bug_report_include_screenshot">"Send screenshot"</string>

features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt

@@ -20,6 +20,9 @@ import app.cash.molecule.RecompositionMode
 import app.cash.molecule.moleculeFlow
 import app.cash.turbine.test
 import com.google.common.truth.Truth.assertThat
+import io.element.android.features.rageshake.api.crash.CrashDataStore
+import io.element.android.features.rageshake.api.reporter.BugReporter
+import io.element.android.features.rageshake.api.screenshot.ScreenshotHolder
 import io.element.android.features.rageshake.test.crash.A_CRASH_DATA
 import io.element.android.features.rageshake.test.crash.FakeCrashDataStore
 import io.element.android.features.rageshake.test.screenshot.A_SCREENSHOT_URI
@@ -27,6 +30,7 @@ import io.element.android.features.rageshake.test.screenshot.FakeScreenshotHolde
 import io.element.android.libraries.architecture.AsyncAction
 import io.element.android.libraries.matrix.test.A_FAILURE_REASON
 import io.element.android.tests.testutils.WarmUpRule
+import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.runTest
 import org.junit.Rule
 import org.junit.Test
@@ -40,12 +44,7 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - initial state`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter()
-            FakeBugReporter(),
-            FakeCrashDataStore(),
-            FakeScreenshotHolder(),
-            this,
-        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
@@ -55,24 +54,19 @@ class BugReportPresenterTest {
             assertThat(initialState.sending).isEqualTo(AsyncAction.Uninitialized)
             assertThat(initialState.screenshotUri).isNull()
             assertThat(initialState.sendingProgress).isEqualTo(0f)
-            assertThat(initialState.submitEnabled).isFalse()
+            assertThat(initialState.submitEnabled).isTrue()
         }
     }
 
     @Test
     fun `present - set description`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter()
-            FakeBugReporter(),
-            FakeCrashDataStore(),
-            FakeScreenshotHolder(),
-            this,
-        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             val initialState = awaitItem()
             initialState.eventSink.invoke(BugReportEvents.SetDescription(A_SHORT_DESCRIPTION))
-            assertThat(awaitItem().submitEnabled).isFalse()
+            assertThat(awaitItem().submitEnabled).isTrue()
             initialState.eventSink.invoke(BugReportEvents.SetDescription(A_LONG_DESCRIPTION))
             assertThat(awaitItem().submitEnabled).isTrue()
         }
@@ -80,12 +74,7 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - can contact`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter()
-            FakeBugReporter(),
-            FakeCrashDataStore(),
-            FakeScreenshotHolder(),
-            this,
-        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
@@ -99,12 +88,7 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - send logs`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter()
-            FakeBugReporter(),
-            FakeCrashDataStore(),
-            FakeScreenshotHolder(),
-            this,
-        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
@@ -119,12 +103,7 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - send screenshot`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter()
-            FakeBugReporter(),
-            FakeCrashDataStore(),
-            FakeScreenshotHolder(),
-            this,
-        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
@@ -138,11 +117,9 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - reset all`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter(
-            FakeBugReporter(),
+            crashDataStore = FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
-            FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
+            screenshotHolder = FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            this,
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
@@ -160,16 +137,17 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - send success`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter(
             FakeBugReporter(mode = FakeBugReporterMode.Success),
             FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
             FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            this,
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             val initialState = awaitItem()
+            initialState.eventSink.invoke(BugReportEvents.SetDescription(A_LONG_DESCRIPTION))
+            skipItems(1)
             initialState.eventSink.invoke(BugReportEvents.SendBugReport)
             skipItems(1)
             val progressState = awaitItem()
@@ -185,16 +163,17 @@ class BugReportPresenterTest {
 
     @Test
     fun `present - send failure`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter(
             FakeBugReporter(mode = FakeBugReporterMode.Failure),
             FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
             FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            this,
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             val initialState = awaitItem()
+            initialState.eventSink.invoke(BugReportEvents.SetDescription(A_LONG_DESCRIPTION))
+            skipItems(1)
             initialState.eventSink.invoke(BugReportEvents.SendBugReport)
             skipItems(1)
             val progressState = awaitItem()
@@ -212,18 +191,38 @@ class BugReportPresenterTest {
         }
     }
 
+    @Test
+    fun `present - send failure description too short`() = runTest {
+        val presenter = createPresenter()
+        moleculeFlow(RecompositionMode.Immediate) {
+            presenter.present()
+        }.test {
+            val initialState = awaitItem()
+            initialState.eventSink.invoke(BugReportEvents.SetDescription(A_SHORT_DESCRIPTION))
+            skipItems(1)
+            initialState.eventSink.invoke(BugReportEvents.SendBugReport)
+            val errorState = awaitItem()
+            assertThat(errorState.sending).isEqualTo(AsyncAction.Failure(BugReportFormError.DescriptionTooShort))
+            // Reset failure
+            initialState.eventSink.invoke(BugReportEvents.ClearError)
+            val lastItem = awaitItem()
+            assertThat(lastItem.sending).isInstanceOf(AsyncAction.Uninitialized::class.java)
+        }
+    }
+
     @Test
     fun `present - send cancel`() = runTest {
-        val presenter = BugReportPresenter(
+        val presenter = createPresenter(
             FakeBugReporter(mode = FakeBugReporterMode.Cancel),
             FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true),
             FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI),
-            this,
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             val initialState = awaitItem()
+            initialState.eventSink.invoke(BugReportEvents.SetDescription(A_LONG_DESCRIPTION))
+            skipItems(1)
             initialState.eventSink.invoke(BugReportEvents.SendBugReport)
             skipItems(1)
             val progressState = awaitItem()
@@ -235,4 +234,15 @@ class BugReportPresenterTest {
             assertThat(awaitItem().sending).isEqualTo(AsyncAction.Uninitialized)
         }
     }
+
+    private fun TestScope.createPresenter(
+        bugReporter: BugReporter = FakeBugReporter(),
+        crashDataStore: CrashDataStore = FakeCrashDataStore(),
+        screenshotHolder: ScreenshotHolder = FakeScreenshotHolder(),
+    ) = BugReportPresenter(
+        bugReporter,
+        crashDataStore,
+        screenshotHolder,
+        this,
+    )
 }

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Day-0_1_null_0,NEXUS_5,1.0,en].png

@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:b80e48cadbb6e76fc6675eb0a2c49d6495be6a8145d649ad90c2bce10e8695a6
+oid sha256:96172e8f6a0283ae1abde6eac27d2ce5fde9e74f20ad89bfb2bb188a2f474fe3
-size 72701
+size 73080

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Day-0_1_null_3,NEXUS_5,1.0,en].png

@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:b80e48cadbb6e76fc6675eb0a2c49d6495be6a8145d649ad90c2bce10e8695a6
+oid sha256:96172e8f6a0283ae1abde6eac27d2ce5fde9e74f20ad89bfb2bb188a2f474fe3
-size 72701
+size 73080

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Day-0_1_null_4,NEXUS_5,1.0,en].png

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:53089b8e5b88e494ef1565e6b9e759f46cd26b1b3f786221dcc047af31835daf
+size 55195

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Night-0_2_null_0,NEXUS_5,1.0,en].png

@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:7696307abd3ca96ae3c036cbf3088c2be08b4e4aabb861b0c7472460ab795303
+oid sha256:2eb33de5998edbdb79ff57b39dddcc7aecdd588613c9f5706fecd7b3f668a3a2
-size 69558
+size 69971

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Night-0_2_null_3,NEXUS_5,1.0,en].png

@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:7696307abd3ca96ae3c036cbf3088c2be08b4e4aabb861b0c7472460ab795303
+oid sha256:2eb33de5998edbdb79ff57b39dddcc7aecdd588613c9f5706fecd7b3f668a3a2
-size 69558
+size 69971

tests/uitests/src/test/snapshots/images/ui_S_t[f.rageshake.impl.bugreport_BugReportView_null_BugReportView-Night-0_2_null_4,NEXUS_5,1.0,en].png

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:8fbef7e43158f49adcaf30447443deb39f5980b0970eab79874d2a7a8dbca563
+size 49958