diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportFormError.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportFormError.kt new file mode 100644 index 0000000000..39d1235ee5 --- /dev/null +++ b/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() +} diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt index fea6bc95b1..5e3b3cfeb1 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt +++ b/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) diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportState.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportState.kt index c3bfbec888..16edae46c4 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportState.kt +++ b/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, val eventSink: (BugReportEvents) -> Unit ) { - val submitEnabled = - formState.description.length > 10 && sending !is AsyncAction.Loading + val submitEnabled = sending !is AsyncAction.Loading + val isDescriptionInError = sending is AsyncAction.Failure && + sending.error is BugReportFormError.DescriptionTooShort } @Parcelize diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportStateProvider.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportStateProvider.kt index bb0d273de4..8bda648c4c 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportStateProvider.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportStateProvider.kt @@ -33,6 +33,7 @@ open class BugReportStateProvider : PreviewParameterProvider { ), aBugReportState().copy(sending = AsyncAction.Loading), aBugReportState().copy(sending = AsyncAction.Success(Unit)), + aBugReportState().copy(sending = AsyncAction.Failure(BugReportFormError.DescriptionTooShort)), ) } diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt index e10e9250e3..86f351ea00 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportView.kt +++ b/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) }, ) } diff --git a/features/rageshake/impl/src/main/res/values/localazy.xml b/features/rageshake/impl/src/main/res/values/localazy.xml index 83413c3919..1d1805a386 100644 --- a/features/rageshake/impl/src/main/res/values/localazy.xml +++ b/features/rageshake/impl/src/main/res/values/localazy.xml @@ -7,6 +7,7 @@ "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." "Describe the problem…" "If possible, please write the description in English." + "The description is too short, please provide more details about what happened. Thanks!" "Send crash logs" "Allow logs" "Send screenshot" diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt index e36cc5b841..7d6f16d412 100644 --- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenterTest.kt +++ b/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( - FakeBugReporter(), - FakeCrashDataStore(), - FakeScreenshotHolder(), - this, - ) + val presenter = createPresenter() 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( - FakeBugReporter(), - FakeCrashDataStore(), - FakeScreenshotHolder(), - this, - ) + val presenter = createPresenter() 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( - FakeBugReporter(), - FakeCrashDataStore(), - FakeScreenshotHolder(), - this, - ) + val presenter = createPresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -99,12 +88,7 @@ class BugReportPresenterTest { @Test fun `present - send logs`() = runTest { - val presenter = BugReportPresenter( - FakeBugReporter(), - FakeCrashDataStore(), - FakeScreenshotHolder(), - this, - ) + val presenter = createPresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -119,12 +103,7 @@ class BugReportPresenterTest { @Test fun `present - send screenshot`() = runTest { - val presenter = BugReportPresenter( - FakeBugReporter(), - FakeCrashDataStore(), - FakeScreenshotHolder(), - this, - ) + val presenter = createPresenter() moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -138,11 +117,9 @@ class BugReportPresenterTest { @Test fun `present - reset all`() = runTest { - val presenter = BugReportPresenter( - FakeBugReporter(), - FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true), - FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI), - this, + val presenter = createPresenter( + crashDataStore = FakeCrashDataStore(crashData = A_CRASH_DATA, appHasCrashed = true), + screenshotHolder = FakeScreenshotHolder(screenshotUri = A_SCREENSHOT_URI), ) 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, + ) } diff --git a/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 b/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 index 1c8b28b5ad..ad82505d3e 100644 --- a/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 +++ b/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 -size 72701 +oid sha256:96172e8f6a0283ae1abde6eac27d2ce5fde9e74f20ad89bfb2bb188a2f474fe3 +size 73080 diff --git a/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 b/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 index 1c8b28b5ad..ad82505d3e 100644 --- a/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 +++ b/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 -size 72701 +oid sha256:96172e8f6a0283ae1abde6eac27d2ce5fde9e74f20ad89bfb2bb188a2f474fe3 +size 73080 diff --git a/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 b/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 new file mode 100644 index 0000000000..ee76b9944b --- /dev/null +++ b/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 diff --git a/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 b/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 index 6ae1a1f2ed..ce0d1a50fb 100644 --- a/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 +++ b/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 -size 69558 +oid sha256:2eb33de5998edbdb79ff57b39dddcc7aecdd588613c9f5706fecd7b3f668a3a2 +size 69971 diff --git a/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 b/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 index 6ae1a1f2ed..ce0d1a50fb 100644 --- a/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 +++ b/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 -size 69558 +oid sha256:2eb33de5998edbdb79ff57b39dddcc7aecdd588613c9f5706fecd7b3f668a3a2 +size 69971 diff --git a/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 b/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 new file mode 100644 index 0000000000..8084317ee1 --- /dev/null +++ b/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