From 31c3b47bcdd654597f71f54bb19a0019b5777762 Mon Sep 17 00:00:00 2001 From: darken Date: Sun, 2 Aug 2026 11:50:08 +0200 Subject: [PATCH] fix(upgrade): Only count a sponsor visit when the page actually opened The sponsor unlock heuristic armed itself on every tap, regardless of whether a browser ever opened the page. An unrelated later background round-trip could then hand out supporter status with no page ever shown. WebpageTool.open() now reports whether an activity was actually started and the FOSS repo passes that through synchronously, so the ViewModel can only arm after a successful launch. A second tap while a launch is still pending is ignored, and the upgraded status view's donate button gets its own unarmed entry point - an existing supporter has nothing left to unlock, and re-persisting would rewrite their "supporter since" date. --- .../common/upgrade/core/UpgradeRepoFoss.kt | 7 +- .../capod/common/upgrade/ui/UpgradeScreen.kt | 8 ++- .../common/upgrade/ui/UpgradeViewModel.kt | 19 ++++++ .../eu/darken/capod/common/WebpageTool.kt | 8 ++- .../eu/darken/capod/common/WebpageToolTest.kt | 57 ++++++++++++++++ .../upgrade/ui/FossUpgradeScreenHostTest.kt | 1 + .../upgrade/ui/FossUpgradeScreenTest.kt | 11 ++- .../upgrade/ui/FossUpgradeViewModelTest.kt | 67 +++++++++++++++++++ 8 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 app/src/test/java/eu/darken/capod/common/WebpageToolTest.kt diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt index 3546afa1..40f48184 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/core/UpgradeRepoFoss.kt @@ -13,7 +13,6 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.shareIn -import kotlinx.coroutines.launch import java.time.Instant import java.util.UUID import javax.inject.Inject @@ -49,9 +48,11 @@ class UpgradeRepoFoss @Inject constructor( .setupCommonEventHandlers(TAG) { "upgradeInfo" } .shareIn(appScope, SharingStarted.WhileSubscribed(3000L, 0L), replay = 1) - fun openGithubSponsorsPage() = appScope.launch { + // Synchronous so the caller learns whether the page actually opened: the FOSS unlock heuristic + // only arms on a successful launch, and a fire-and-forget coroutine can't report that back. + fun openGithubSponsorsPage(): Boolean { log(TAG) { "openGithubSponsorsPage()" } - webpageTool.open(upgradeSite) + return webpageTool.open(upgradeSite) } // Writes capod's RETAINED persistence schema: existing supporter records are serialized with diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeScreen.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeScreen.kt index fc5f1602..93c7bce5 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeScreen.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeScreen.kt @@ -95,6 +95,7 @@ fun UpgradeScreenHost( supporterSince = state?.supporterSince, snackbarHostState = snackbarHostState, onGithubSponsors = vm::goGithubSponsors, + onOpenSponsors = vm::openSponsors, onShowUpgradeOptions = vm::onShowUpgradeOptions, onNavigateUp = vm::navUp, ) @@ -106,6 +107,7 @@ internal fun UpgradeScreen( supporterSince: Instant? = null, snackbarHostState: SnackbarHostState = remember { SnackbarHostState() }, onGithubSponsors: () -> Unit = {}, + onOpenSponsors: () -> Unit = {}, onShowUpgradeOptions: () -> Unit = {}, onNavigateUp: () -> Unit = {}, ) { @@ -140,7 +142,7 @@ internal fun UpgradeScreen( FossUpgradeView.STATUS_UPGRADED -> UpgradeStatusUpgradedContent( paddingValues = paddingValues, supporterSince = supporterSince, - onGithubSponsors = onGithubSponsors, + onOpenSponsors = onOpenSponsors, ) } } @@ -234,7 +236,7 @@ private fun UpgradeStatusFreeContent( private fun UpgradeStatusUpgradedContent( paddingValues: PaddingValues, supporterSince: Instant? = null, - onGithubSponsors: () -> Unit, + onOpenSponsors: () -> Unit, ) { UpgradeScreenContent( paddingValues = paddingValues, @@ -273,7 +275,7 @@ private fun UpgradeStatusUpgradedContent( ) { UpgradeSectionBody(text = stringResource(R.string.upgrade_screen_recurring_body)) OutlinedButton( - onClick = onGithubSponsors, + onClick = onOpenSponsors, modifier = Modifier .fillMaxWidth() .testTag(UpgradeScreenTags.FOSS_DONATE), diff --git a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt index 47882106..880e76a7 100644 --- a/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt +++ b/app/src/foss/java/eu/darken/capod/common/upgrade/ui/UpgradeViewModel.kt @@ -106,9 +106,28 @@ class UpgradeViewModel @Inject constructor( handle[KEY_SHOW_UPGRADE_OPTIONS] = true } + /** Armed variant: the pitch's sponsor button, which starts the return-after-5s unlock heuristic. */ fun goGithubSponsors() { log(TAG) { "goGithubSponsors()" } + if (hasPendingSponsorLaunch()) { + log(TAG) { "A sponsor launch is already awaiting its return" } + return + } + // Only arm the heuristic if the page actually opened; otherwise an unrelated later + // background/foreground round-trip would grant supporter status with no page ever shown. + if (!upgradeRepo.openGithubSponsorsPage()) { + log(TAG) { "Sponsor page didn't open; not arming the unlock heuristic" } + return + } handle[KEY_SPONSOR_PRESSED_AT] = SystemClock.elapsedRealtime() + } + + /** + * Unarmed variant: the status view's donate button. An existing supporter re-visiting the page + * must not re-arm the unlock heuristic — there is nothing left to unlock. + */ + fun openSponsors() { + log(TAG) { "openSponsors()" } upgradeRepo.openGithubSponsorsPage() } diff --git a/app/src/main/java/eu/darken/capod/common/WebpageTool.kt b/app/src/main/java/eu/darken/capod/common/WebpageTool.kt index 19cf4ecb..3e993ef6 100644 --- a/app/src/main/java/eu/darken/capod/common/WebpageTool.kt +++ b/app/src/main/java/eu/darken/capod/common/WebpageTool.kt @@ -15,14 +15,18 @@ class WebpageTool @Inject constructor( @ApplicationContext private val context: Context, ) { - fun open(address: String) { + // Returns whether an activity was actually started, so callers that gate behaviour on the page + // having opened (e.g. the FOSS sponsor unlock heuristic) don't fire when no browser handled it. + fun open(address: String): Boolean { val intent = Intent(Intent.ACTION_VIEW, address.toUri()).apply { addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } - try { + return try { context.startActivity(intent) + true } catch (e: Exception) { log(ERROR) { "Failed to launch: ${e.asLog()}" } + false } } diff --git a/app/src/test/java/eu/darken/capod/common/WebpageToolTest.kt b/app/src/test/java/eu/darken/capod/common/WebpageToolTest.kt new file mode 100644 index 00000000..e47a17c7 --- /dev/null +++ b/app/src/test/java/eu/darken/capod/common/WebpageToolTest.kt @@ -0,0 +1,57 @@ +package eu.darken.capod.common + +import android.app.Application +import android.content.ActivityNotFoundException +import android.content.ContextWrapper +import android.content.Intent +import androidx.core.net.toUri +import androidx.test.core.app.ApplicationProvider +import io.kotest.matchers.shouldBe +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config +import testhelpers.BaseTest +import testhelpers.TestApplication + +/** + * The return value is a contract, not a convenience: the FOSS sponsor unlock heuristic only arms + * when the page actually opened. Every swallow point has to report the failure back. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33], application = TestApplication::class) +class WebpageToolTest : BaseTest() { + + private val application: Application + get() = ApplicationProvider.getApplicationContext() + + @Test + fun `a launched page reports success`() { + WebpageTool(application).open(URL) shouldBe true + + shadowOf(application).nextStartedActivity!!.data shouldBe URL.toUri() + } + + @Test + fun `a missing browser reports failure`() { + val context = object : ContextWrapper(application) { + override fun startActivity(intent: Intent) = throw ActivityNotFoundException("No browser") + } + + WebpageTool(context).open(URL) shouldBe false + } + + @Test + fun `a denied launch reports failure`() { + val context = object : ContextWrapper(application) { + override fun startActivity(intent: Intent) = throw SecurityException("Permission Denial") + } + + WebpageTool(context).open(URL) shouldBe false + } + + companion object { + private const val URL = "https://github.com/sponsors/d4rken" + } +} diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt index e1cb5abe..9b870d7d 100644 --- a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenHostTest.kt @@ -39,6 +39,7 @@ class FossUpgradeScreenHostTest : BaseTest() { private fun mockRepo(): UpgradeRepoFoss = mockk(relaxed = true).apply { every { upgradeInfo } returns MutableStateFlow(UpgradeRepoFoss.Info()) + every { openGithubSponsorsPage() } returns true coEvery { persistUpgrade() } answers { persisted++ } } diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenTest.kt index a5bc947d..a52093b5 100644 --- a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenTest.kt +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeScreenTest.kt @@ -12,6 +12,7 @@ import androidx.test.core.app.ApplicationProvider import androidx.compose.ui.semantics.SemanticsActions import eu.darken.capod.R import eu.darken.capod.common.compose.PreviewWrapper +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test import testhelpers.compose.BaseComposeRobolectricTest @@ -127,9 +128,16 @@ class FossUpgradeScreenTest : BaseComposeRobolectricTest() { @Test fun `recurring donation button invokes the sponsors callback`() { var clicked = false + // The armed pitch callback must stay untouched here: a supporter donating again has nothing + // left to unlock, so the donate button goes through the unarmed callback. + var armed = false composeRule.setUpgradeContent { - UpgradeScreen(view = FossUpgradeView.STATUS_UPGRADED, onGithubSponsors = { clicked = true }) + UpgradeScreen( + view = FossUpgradeView.STATUS_UPGRADED, + onGithubSponsors = { armed = true }, + onOpenSponsors = { clicked = true }, + ) } composeRule.onNodeWithTag(UpgradeScreenTags.FOSS_DONATE) @@ -137,6 +145,7 @@ class FossUpgradeScreenTest : BaseComposeRobolectricTest() { composeRule.runOnIdle { assertTrue(clicked) + assertFalse(armed) } } } diff --git a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt index e7bf6ca2..c358e658 100644 --- a/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt +++ b/app/src/testFoss/java/eu/darken/capod/common/upgrade/ui/FossUpgradeViewModelTest.kt @@ -11,6 +11,7 @@ import io.kotest.matchers.shouldBe import io.mockk.coVerify import io.mockk.every import io.mockk.mockk +import io.mockk.verify import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async @@ -61,6 +62,7 @@ class FossUpgradeViewModelTest : BaseTest() { info: MutableStateFlow = MutableStateFlow(UpgradeRepoFoss.Info()), ): UpgradeRepoFoss = mockk(relaxed = true).apply { every { upgradeInfo } returns info + every { openGithubSponsorsPage() } returns true } private fun buildVm( @@ -309,4 +311,69 @@ class FossUpgradeViewModelTest : BaseTest() { // Consumed: a later resume must not re-run the unlock. recreatedVm.hasPendingSponsorLaunch() shouldBe false } + + @Test + fun `a sponsor page that never opened arms nothing and a later retry still works`() = runTest2( + context = testDispatcher, + ) { + // A silently failed launch must not leave the heuristic armed: an unrelated later + // background round-trip would otherwise hand out supporter status for free. + val repo = mockRepo() + every { repo.openGithubSponsorsPage() } returns false + val vm = buildVm(repo = repo) + + vm.goGithubSponsors() + advanceUntilIdle() + + vm.hasPendingSponsorLaunch() shouldBe false + + // And the failure must not brick the button either — the next working attempt arms as usual. + every { repo.openGithubSponsorsPage() } returns true + vm.goGithubSponsors() + advanceUntilIdle() + + vm.hasPendingSponsorLaunch() shouldBe true + } + + @Test + fun `a second sponsor tap while a launch is pending opens the page only once`() = runTest2( + context = testDispatcher, + ) { + val repo = mockRepo() + val vm = buildVm(repo = repo) + + vm.goGithubSponsors() + vm.goGithubSponsors() + advanceUntilIdle() + + verify(exactly = 1) { repo.openGithubSponsorsPage() } + } + + @Test + fun `a long donate visit from the status view does not re-persist the upgrade`() = runTest2( + context = testDispatcher, + ) { + // The status view's donate button is unarmed on purpose: a supporter browsing the sponsors + // page for a while must not run the unlock heuristic again and rewrite their upgrade date. + val repo = mockRepo(MutableStateFlow(upgradedInfo())) + val vm = buildVm(repo = repo) + + val state = async { vm.state.first { it != null }!! } + vm.bindRoute(Nav.Main.Upgrade(manage = true)) + advanceUntilIdle() + state.await().supporterSince shouldBe Instant.EPOCH + + vm.openSponsors() + advanceUntilIdle() + + verify(exactly = 1) { repo.openGithubSponsorsPage() } + vm.hasPendingSponsorLaunch() shouldBe false + + ShadowSystemClock.advanceBy(Duration.ofSeconds(6)) + vm.checkSponsorReturn() + advanceUntilIdle() + + coVerify(exactly = 0) { repo.persistUpgrade() } + vm.state.value!!.supporterSince shouldBe Instant.EPOCH + } }