diff --git a/app/src/androidTest/kotlin/org/libremail/IntentHandledMarkerTest.kt b/app/src/androidTest/kotlin/org/libremail/IntentHandledMarkerTest.kt new file mode 100644 index 0000000..d109e70 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/IntentHandledMarkerTest.kt @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail + +import android.content.Intent +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Locks in the #157 regression fix: [IntentHandledMarker] must tell a redelivered (already-parsed) + * intent apart from a genuinely new one using only the [Intent] instance itself, never + * `savedInstanceState` — which Android also sets to non-null after a process-death relaunch, where the + * delivered intent is a brand new, not-yet-handled notification tap or mailto/share, not a replay. + */ +@RunWith(AndroidJUnit4::class) +class IntentHandledMarkerTest { + + @Test + fun first_look_marks_the_intent_and_reports_it_as_unhandled() { + val intent = Intent(Intent.ACTION_MAIN) + assertTrue(IntentHandledMarker.markIfUnhandled(intent)) + } + + @Test + fun redelivering_the_same_instance_is_recognized_as_already_handled() { + // Simulates a config-change recreation: Android redelivers the very same Intent object to the + // new Activity instance's onCreate, so a second look must be recognized as a replay. + val intent = Intent(Intent.ACTION_MAIN) + IntentHandledMarker.markIfUnhandled(intent) + assertFalse(IntentHandledMarker.markIfUnhandled(intent)) + } + + @Test + fun a_freshly_constructed_equivalent_intent_is_still_unhandled() { + // Simulates a process-death relaunch (e.g. tapping a notification after the app was killed in + // the background): a brand new Intent instance arrives — content may equal one already marked + // in a prior (now-dead) process, but it was never marked in THIS one, so it must be parsed. + val first = Intent(Intent.ACTION_VIEW) + IntentHandledMarker.markIfUnhandled(first) + + val second = Intent(Intent.ACTION_VIEW) + assertTrue(IntentHandledMarker.markIfUnhandled(second)) + } +} diff --git a/app/src/main/kotlin/org/libremail/MainActivity.kt b/app/src/main/kotlin/org/libremail/MainActivity.kt index e480786..fc65324 100644 --- a/app/src/main/kotlin/org/libremail/MainActivity.kt +++ b/app/src/main/kotlin/org/libremail/MainActivity.kt @@ -66,12 +66,7 @@ class MainActivity : FragmentActivity() { } } } - // Only on a fresh launch — on a config-change recreation the NavHost restores the compose / - // reader destination itself, so re-parsing the (unchanged) intent would open a duplicate. - if (savedInstanceState == null) { - pendingCompose.value = IntentComposeParser.parse(intent) - pendingOpenMessageId.value = NotificationIntents.messageId(intent) - } + handleIntent(intent) setContent { val dynamicColor by settingsRepository.dynamicColor.collectAsStateWithLifecycle(initialValue = true) LibreMailTheme(dynamicColor = dynamicColor) { @@ -92,7 +87,54 @@ class MainActivity : FragmentActivity() { override fun onNewIntent(intent: Intent) { super.onNewIntent(intent) setIntent(intent) + handleIntent(intent) + } + + /** + * Parses [intent] for a pending compose ([IntentComposeParser]) or open-message + * ([NotificationIntents]) request — unless [IntentHandledMarker] says this exact intent instance + * was already parsed. + * + * This used to be gated on `savedInstanceState == null` in [onCreate]: a non-null value was + * assumed to mean "config-change recreation" — where Android redelivers the very same, + * already-parsed intent and the NavHost restores its own compose/reader destination itself, so + * re-parsing would only navigate to a duplicate. But Android *also* passes a restored, non-null + * savedInstanceState when it recreates this activity after the process was killed in the + * background and is then relaunched — e.g. by tapping a notification. There, `intent` is the new + * tap, not a replay, but the old guard swallowed it exactly like a rotation: `pendingOpenMessageId` + * was never set, so the tap silently landed wherever the restored back stack was, never the + * message. That regression is #157. + * + * Marking the [Intent] instance itself — rather than branching on savedInstanceState, which can't + * tell a config change and a process-death relaunch apart — is correct for both: a config-change + * recreation redelivers the very same intent this activity already marked, so it's recognized and + * skipped; a genuinely new intent — a fresh notification tap or mailto/share, whether delivered + * warm via [onNewIntent] or cold via [onCreate] after a process-death relaunch — is never marked + * yet, so it's always (re)parsed. + */ + private fun handleIntent(intent: Intent) { + if (!IntentHandledMarker.markIfUnhandled(intent)) return IntentComposeParser.parse(intent)?.let { pendingCompose.value = it } NotificationIntents.messageId(intent)?.let { pendingOpenMessageId.value = it } } } + +/** + * Marks an [Intent] as already parsed by [MainActivity.handleIntent], so a redelivery of the very same + * instance — which is what Android does when it recreates an Activity for a configuration change — is + * recognized and skipped instead of re-triggering a duplicate navigation. A freshly constructed intent + * (a new notification tap or mailto/share, however it arrives) is never marked yet, so it is always + * treated as unhandled — including right after a process-death relaunch, where `savedInstanceState` is + * restored (non-null) but the intent itself is new. Internal (not private) so androidTest can verify + * the marking contract directly. See #157. + */ +internal object IntentHandledMarker { + private const val EXTRA_HANDLED = "org.libremail.extra.INTENT_HANDLED" + + /** Marks [intent] handled and returns `true` — but only the first time this instance is seen. */ + fun markIfUnhandled(intent: Intent): Boolean { + if (intent.getBooleanExtra(EXTRA_HANDLED, false)) return false + intent.putExtra(EXTRA_HANDLED, true) + return true + } +} diff --git a/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt b/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt index 3b6d929..2c782f7 100644 --- a/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt +++ b/app/src/main/kotlin/org/libremail/reporting/DebugReport.kt @@ -29,6 +29,8 @@ data class DebugReport( val settings: Map, val logs: List, val userComment: String = "", + /** Reply-to address the user supplied when submitting (see #159); required for online submit. */ + val userEmail: String = "", ) { /** The exact text shown for review, copied, saved to a file, and POSTed on submit. */ fun toSubmissionPayload(): String = toJson().toString(JSON_INDENT) @@ -55,6 +57,7 @@ data class DebugReport( .put("app", app) .put("device", device) .put("userComment", userComment) + .put("userEmail", userEmail) .put("settings", settingsJson) .put("logs", JSONArray(logs)) if (stackTrace != null) json.put("stackTrace", stackTrace) @@ -88,6 +91,7 @@ data class DebugReport( settings = settings, logs = logs, userComment = json.optString("userComment", ""), + userEmail = json.optString("userEmail", ""), ) } } diff --git a/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewScreen.kt b/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewScreen.kt index d14c76d..a106a2f 100644 --- a/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewScreen.kt @@ -128,6 +128,36 @@ fun ReportReviewScreen(onDone: () -> Unit, viewModel: ReportReviewViewModel = hi label = { Text(stringResource(R.string.report_comment_label)) }, modifier = Modifier.fillMaxWidth(), minLines = 2, + isError = !state.isCommentLongEnough, + supportingText = { + Text( + stringResource( + R.string.report_comment_counter, + state.comment.length, + ReportSubmissionRules.MIN_COMMENT_LENGTH, + ), + ) + }, + ) + Spacer(Modifier.height(16.dp)) + OutlinedTextField( + value = state.email, + onValueChange = viewModel::updateEmail, + label = { Text(stringResource(R.string.report_email_label)) }, + modifier = Modifier.fillMaxWidth(), + singleLine = true, + isError = !state.isEmailValid, + supportingText = { + if (!state.isEmailValid) { + Text(stringResource(R.string.report_email_invalid)) + } + }, + ) + Spacer(Modifier.height(8.dp)) + Text( + stringResource(R.string.report_email_consent), + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, ) Spacer(Modifier.height(16.dp)) Text( @@ -141,7 +171,9 @@ fun ReportReviewScreen(onDone: () -> Unit, viewModel: ReportReviewViewModel = hi Spacer(Modifier.height(16.dp)) Button( onClick = viewModel::submit, - enabled = state.submit != SubmitUiState.SUBMITTING, + enabled = state.submit != SubmitUiState.SUBMITTING && + state.isCommentLongEnough && + state.isEmailValid, modifier = Modifier.fillMaxWidth(), ) { Text(stringResource(R.string.report_submit)) diff --git a/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewViewModel.kt b/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewViewModel.kt index e42a8b7..a374a26 100644 --- a/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/reporting/ReportReviewViewModel.kt @@ -20,14 +20,45 @@ import javax.inject.Inject /** UI-facing status of a submission attempt. [UNAVAILABLE] means no endpoint is configured. */ enum class SubmitUiState { IDLE, SUBMITTING, SUCCEEDED, FAILED, UNAVAILABLE } +/** + * Pure validation rules for problem-report submission (#159). Kept dependency-free — in particular, + * no `android.util.Patterns`, which is a no-op stub under plain JVM unit tests — so both the + * min-length and email-shape checks are directly unit-testable and shareable between + * [ReportReviewState] and [ReportReviewViewModel]. + */ +object ReportSubmissionRules { + /** Minimum comment length (characters) required before Submit is enabled. */ + const val MIN_COMMENT_LENGTH = 200 + + // Basic shape only: a non-blank local part, an `@`, and a domain with at least one `.` and + // non-blank labels either side of it. Deliberately not a full RFC 5322 validator. + private val EMAIL_REGEX = Regex("^[^\\s@]+@[^\\s@]+\\.[^\\s@]+$") + + /** Whether [comment] reaches [MIN_COMMENT_LENGTH]. */ + fun isCommentLongEnough(comment: String): Boolean = comment.length >= MIN_COMMENT_LENGTH + + /** Whether [email] has a plausible local-part@domain.tld shape. */ + fun isValidEmail(email: String): Boolean = EMAIL_REGEX.matches(email.trim()) +} + data class ReportReviewState( val loaded: Boolean = false, val exists: Boolean = false, val payload: String = "", val comment: String = "", + val email: String = "", val canSubmitOnline: Boolean = false, val submit: SubmitUiState = SubmitUiState.IDLE, -) +) { + /** True once [comment] reaches [ReportSubmissionRules.MIN_COMMENT_LENGTH]. */ + val isCommentLongEnough: Boolean get() = ReportSubmissionRules.isCommentLongEnough(comment) + + /** True once [email] is a plausible reply-to address. */ + val isEmailValid: Boolean get() = ReportSubmissionRules.isValidEmail(email) + + /** Submit gate: the original SUBMITTING check, plus the new length/email requirements. */ + val canSubmit: Boolean get() = submit != SubmitUiState.SUBMITTING && isCommentLongEnough && isEmailValid +} @HiltViewModel class ReportReviewViewModel @Inject constructor( @@ -38,17 +69,20 @@ class ReportReviewViewModel @Inject constructor( private val reportId: String = checkNotNull(savedStateHandle[Routes.REPORT_REVIEW_ARG_ID]) private val comment = MutableStateFlow(store.find(reportId)?.userComment.orEmpty()) + private val email = MutableStateFlow(store.find(reportId)?.userEmail.orEmpty()) private val submitState = MutableStateFlow(SubmitUiState.IDLE) val state: StateFlow = - combine(store.reports, comment, submitState) { reports, currentComment, submit -> + combine(store.reports, comment, email, submitState) { reports, currentComment, currentEmail, submit -> val report = reports.firstOrNull { it.id == reportId } ReportReviewState( loaded = true, exists = report != null, - // The comment is folded in so the preview is byte-for-byte what a submit would send. - payload = report?.copy(userComment = currentComment)?.toSubmissionPayload().orEmpty(), + // The comment/email are folded in so the preview is byte-for-byte what a submit would send. + payload = report?.copy(userComment = currentComment, userEmail = currentEmail) + ?.toSubmissionPayload().orEmpty(), comment = currentComment, + email = currentEmail, canSubmitOnline = submitter.isEnabled, submit = submit, ) @@ -58,6 +92,10 @@ class ReportReviewViewModel @Inject constructor( comment.value = value } + fun updateEmail(value: String) { + email.value = value + } + fun discard() { viewModelScope.launch { store.delete(reportId) } } @@ -66,11 +104,20 @@ class ReportReviewViewModel @Inject constructor( * The only path that can send a report off-device, and only from an explicit Submit tap. Persists * the reviewed comment first so the upload matches exactly what was shown, then enqueues the * worker (unless no endpoint is configured, in which case it steers the user to Copy/Save). + * + * Guarded by [ReportSubmissionRules] directly (rather than trusting the caller) so this can't + * succeed with an under-length comment or an invalid email even if invoked outside the + * Compose button's `enabled` gate — e.g. from an accessibility service activating a control it + * still considers actionable. */ fun submit() { viewModelScope.launch { + val currentSubmit = submitState.value + if (currentSubmit == SubmitUiState.SUBMITTING) return@launch + if (!ReportSubmissionRules.isCommentLongEnough(comment.value)) return@launch + if (!ReportSubmissionRules.isValidEmail(email.value)) return@launch val report = store.find(reportId) ?: return@launch - store.save(report.copy(userComment = comment.value)) + store.save(report.copy(userComment = comment.value, userEmail = email.value)) if (!submitter.isEnabled) { submitState.value = SubmitUiState.UNAVAILABLE return@launch @@ -82,7 +129,8 @@ class ReportReviewViewModel @Inject constructor( } /** The exact text shown for review — used for Copy and Save-to-file. */ - fun payload(): String = store.find(reportId)?.copy(userComment = comment.value)?.toSubmissionPayload().orEmpty() + fun payload(): String = store.find(reportId)?.copy(userComment = comment.value, userEmail = email.value) + ?.toSubmissionPayload().orEmpty() private fun SubmitStatus.toUi(): SubmitUiState = when (this) { SubmitStatus.IDLE, SubmitStatus.SUBMITTING -> SubmitUiState.SUBMITTING diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 529f981..ebab46e 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -331,7 +331,13 @@ Review report May contain personal information This report can include email addresses, server names, and other details from your device. Read the whole thing below before sending. Nothing is sent unless you tap Submit. - What went wrong? (optional) + What went wrong? + + %1$d/%2$d + Your email address + Enter a valid email address + + By submitting this report and supplying your email, you agree that the maintainers of LibreMail may contact you at the supplied email. Supplying an email and submitting a report does not guarantee reply or resolution to your concern. Exactly what will be sent Submit Discard report diff --git a/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt b/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt index b582078..4cf670b 100644 --- a/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt +++ b/app/src/test/kotlin/org/libremail/reporting/DebugReportTest.kt @@ -69,4 +69,28 @@ class DebugReportTest { assertTrue(edited.contains("edited note")) } + + @Test + fun `round-trips the reply-to email`() { + val original = sample().copy(userEmail = "reporter@example.com") + + val restored = DebugReport.fromStorageJson(original.toStorageJson()) + + assertEquals("reporter@example.com", restored.userEmail) + assertEquals(original, restored) + } + + @Test + fun `a report with no email round-trips to a blank one`() { + val restored = DebugReport.fromStorageJson(sample().toStorageJson()) + + assertEquals("", restored.userEmail) + } + + @Test + fun `submission payload contains the reply-to email`() { + val payload = sample().copy(userEmail = "reporter@example.com").toSubmissionPayload() + + assertTrue(payload.contains("reporter@example.com")) + } } diff --git a/app/src/test/kotlin/org/libremail/ui/reporting/ReportReviewViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/reporting/ReportReviewViewModelTest.kt index 79eb032..f592c1b 100644 --- a/app/src/test/kotlin/org/libremail/ui/reporting/ReportReviewViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/reporting/ReportReviewViewModelTest.kt @@ -24,6 +24,8 @@ import org.libremail.reporting.ReportStore import org.libremail.reporting.ReportSubmitter import org.libremail.ui.navigation.Routes import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertTrue @OptIn(ExperimentalCoroutinesApi::class) class ReportReviewViewModelTest { @@ -83,31 +85,82 @@ class ReportReviewViewModelTest { } @Test - fun `submit enqueues the upload exactly once and persists the reviewed comment`() = runTest(testDispatcher) { - every { submitter.isEnabled } returns true - every { submitter.submit("rid") } just Runs - every { submitter.status("rid") } returns emptyFlow() - every { store.save(any()) } just Runs + fun `payload shown reflects the entered email`() = runTest(testDispatcher) { val vm = viewModel() - vm.updateComment("edited before submit") - vm.submit() + vm.updateEmail("me@example.com") - verify(exactly = 1) { store.save(match { it.userComment == "edited before submit" }) } - verify(exactly = 1) { submitter.submit("rid") } + assertEquals(report.copy(userEmail = "me@example.com").toSubmissionPayload(), vm.payload()) } @Test - fun `submit with no endpoint configured never transmits`() = runTest(testDispatcher) { + fun `submit enqueues the upload exactly once and persists the reviewed comment and email`() = + runTest(testDispatcher) { + every { submitter.isEnabled } returns true + every { submitter.submit("rid") } just Runs + every { submitter.status("rid") } returns emptyFlow() + every { store.save(any()) } just Runs + val vm = viewModel() + vm.updateComment(VALID_COMMENT) + vm.updateEmail(VALID_EMAIL) + + vm.submit() + + verify(exactly = 1) { + store.save(match { it.userComment == VALID_COMMENT && it.userEmail == VALID_EMAIL }) + } + verify(exactly = 1) { submitter.submit("rid") } + } + + @Test + fun `submit with no endpoint configured never transmits but still persists`() = runTest(testDispatcher) { every { submitter.isEnabled } returns false every { store.save(any()) } just Runs val vm = viewModel() - vm.updateComment("please send") + vm.updateComment(VALID_COMMENT) + vm.updateEmail(VALID_EMAIL) vm.submit() - // The comment is still persisted for Copy/Save, but nothing is enqueued for upload. - verify(exactly = 1) { store.save(match { it.userComment == "please send" }) } + // The comment/email are still persisted for Copy/Save, but nothing is enqueued for upload. + verify(exactly = 1) { + store.save(match { it.userComment == VALID_COMMENT && it.userEmail == VALID_EMAIL }) + } + verify(exactly = 0) { submitter.submit(any()) } + } + + @Test + fun `submit does nothing when the comment is under the minimum length`() = runTest(testDispatcher) { + val vm = viewModel() + vm.updateEmail(VALID_EMAIL) + vm.updateComment("way too short") + + vm.submit() + + verify(exactly = 0) { store.save(any()) } + verify(exactly = 0) { submitter.submit(any()) } + } + + @Test + fun `submit does nothing when the email is blank`() = runTest(testDispatcher) { + val vm = viewModel() + vm.updateComment(VALID_COMMENT) + + vm.submit() + + verify(exactly = 0) { store.save(any()) } + verify(exactly = 0) { submitter.submit(any()) } + } + + @Test + fun `submit does nothing when the email is malformed`() = runTest(testDispatcher) { + val vm = viewModel() + vm.updateComment(VALID_COMMENT) + vm.updateEmail("not-an-email") + + vm.submit() + + verify(exactly = 0) { store.save(any()) } verify(exactly = 0) { submitter.submit(any()) } } @@ -121,4 +174,46 @@ class ReportReviewViewModelTest { verify(exactly = 1) { store.delete("rid") } verify(exactly = 0) { submitter.submit(any()) } } + + @Test + fun `isCommentLongEnough is false below the threshold and true at or above it`() { + val short = ReportReviewState(comment = "a".repeat(ReportSubmissionRules.MIN_COMMENT_LENGTH - 1)) + val exact = ReportReviewState(comment = "a".repeat(ReportSubmissionRules.MIN_COMMENT_LENGTH)) + + assertFalse(short.isCommentLongEnough) + assertTrue(exact.isCommentLongEnough) + } + + @Test + fun `isEmailValid accepts plausible addresses and rejects malformed or blank input`() { + assertTrue(ReportReviewState(email = "user@example.com").isEmailValid) + assertTrue(ReportReviewState(email = "first.last+tag@sub.example.co.uk").isEmailValid) + assertFalse(ReportReviewState(email = "").isEmailValid) + assertFalse(ReportReviewState(email = "no-at-sign.com").isEmailValid) + assertFalse(ReportReviewState(email = "user@").isEmailValid) + assertFalse(ReportReviewState(email = "user@nodot").isEmailValid) + assertFalse(ReportReviewState(email = "@example.com").isEmailValid) + assertFalse(ReportReviewState(email = "has space@example.com").isEmailValid) + } + + @Test + fun `canSubmit requires both a long-enough comment and a valid email`() { + val validComment = "a".repeat(ReportSubmissionRules.MIN_COMMENT_LENGTH) + + assertTrue(ReportReviewState(comment = validComment, email = "user@example.com").canSubmit) + assertFalse(ReportReviewState(comment = "short", email = "user@example.com").canSubmit) + assertFalse(ReportReviewState(comment = validComment, email = "not-an-email").canSubmit) + assertFalse( + ReportReviewState( + comment = validComment, + email = "user@example.com", + submit = SubmitUiState.SUBMITTING, + ).canSubmit, + ) + } + + private companion object { + val VALID_COMMENT = "a".repeat(ReportSubmissionRules.MIN_COMMENT_LENGTH) + const val VALID_EMAIL = "reporter@example.com" + } }