diff options
Diffstat (limited to 'app/src')
8 files changed, 239 insertions, 14 deletions
diff --git a/app/src/androidTest/java/org/terst/listen/data/PreferencesTest.kt b/app/src/androidTest/java/org/terst/listen/data/PreferencesTest.kt index 42c8f4a..cbfd2b0 100644 --- a/app/src/androidTest/java/org/terst/listen/data/PreferencesTest.kt +++ b/app/src/androidTest/java/org/terst/listen/data/PreferencesTest.kt @@ -3,11 +3,14 @@ package org.terst.listen.data import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import org.terst.listen.server.SkipServerConfig /** * Instrumented, not a JVM unit test: [Preferences] takes a [android.content.Context] and reads @@ -38,4 +41,56 @@ class PreferencesTest { // with itself on a second run. prefs.setSkipShowFurniture(true) } + + /** + * A refused token is the one failure that cannot fix itself, and it used to be remembered + * only in memory -- so after the process died the Settings screen said nothing was wrong + * until the next refresh went and got refused all over again. Written down, it is there at + * launch. + */ + @Test + fun aRefusedServerIsRememberedAcrossRestarts() = runBlocking { + val prefs = Preferences(context) + prefs.setServer("https://listen.example", "wrong-token") + + prefs.setRejectedServer(SkipServerConfig("https://listen.example", "wrong-token")) + + val again = Preferences(context).rejectedServer.first() + assertEquals("https://listen.example", again?.baseUrl) + assertEquals("wrong-token", again?.token) + } + + @Test + fun nothingRefusedIsTheNormalState() = runBlocking { + val prefs = Preferences(context) + prefs.setRejectedServer(null) + + assertEquals(null, prefs.rejectedServer.first()) + } + + /** + * Saving different credentials is the one act that answers the complaint, so the notice has + * to stop applying at once rather than waiting for a request to prove it. + */ + @Test + fun savingDifferentCredentialsClearsTheRefusal() = runBlocking { + val prefs = Preferences(context) + prefs.setServer("https://listen.example", "wrong-token") + prefs.setRejectedServer(SkipServerConfig("https://listen.example", "wrong-token")) + + prefs.setServer("https://listen.example", "a-better-token") + + assertEquals(null, prefs.rejectedServer.first()) + } + + @Test + fun savingTheSameCredentialsAgainDoesNotPretendTheyWork() = runBlocking { + val prefs = Preferences(context) + prefs.setServer("https://listen.example", "wrong-token") + prefs.setRejectedServer(SkipServerConfig("https://listen.example", "wrong-token")) + + prefs.setServer("https://listen.example", "wrong-token") + + assertEquals("wrong-token", prefs.rejectedServer.first()?.token) + } } diff --git a/app/src/main/java/org/terst/listen/data/Preferences.kt b/app/src/main/java/org/terst/listen/data/Preferences.kt index 8cbc583..58de63e 100644 --- a/app/src/main/java/org/terst/listen/data/Preferences.kt +++ b/app/src/main/java/org/terst/listen/data/Preferences.kt @@ -7,6 +7,7 @@ import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.preferencesDataStore import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map +import org.terst.listen.server.SkipServerConfig private val Context.store by preferencesDataStore(name = "listen-preferences") @@ -73,6 +74,42 @@ class Preferences(private val context: Context) { context.store.edit { it[SERVER_URL] = url.trim() it[SERVER_TOKEN] = token.trim() + // Saving different credentials answers the complaint, so the refusal stops applying + // at once rather than waiting for a request to go and prove it. Saving the *same* + // ones again proves nothing and leaves it standing. + if (it[REJECTED_URL] != url.trim() || it[REJECTED_TOKEN] != token.trim()) { + it.remove(REJECTED_URL) + it.remove(REJECTED_TOKEN) + } + } + } + + /** + * The configuration the server last refused, or null. + * + * Written down rather than held in memory, which is what it used to be. A refused token is + * the one failure that cannot fix itself -- 401 and 403 are permanent, by decision -- and in + * memory it meant the Settings screen said nothing was wrong until the next refresh went and + * got refused all over again. Now it is there at launch. + * + * Stored as the refused configuration rather than a flag so it stops applying the moment a + * different one is saved. + */ + val rejectedServer: Flow<SkipServerConfig?> = context.store.data.map { prefs -> + val url = prefs[REJECTED_URL] + val token = prefs[REJECTED_TOKEN] + if (url.isNullOrBlank()) null else SkipServerConfig(url, token.orEmpty()) + } + + suspend fun setRejectedServer(config: SkipServerConfig?) { + context.store.edit { + if (config == null) { + it.remove(REJECTED_URL) + it.remove(REJECTED_TOKEN) + } else { + it[REJECTED_URL] = config.baseUrl + it[REJECTED_TOKEN] = config.token + } } } @@ -82,5 +119,7 @@ class Preferences(private val context: Context) { val SERVER_TOKEN = stringPreferencesKey("server_token") val SKIP_SHOW_FURNITURE = booleanPreferencesKey("skip_show_furniture") val FAST_PLAY_BREAKS = booleanPreferencesKey("fast_play_breaks") + val REJECTED_URL = stringPreferencesKey("rejected_server_url") + val REJECTED_TOKEN = stringPreferencesKey("rejected_server_token") } } diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index 35d96c0..f56fdc6 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -13,6 +13,7 @@ import org.terst.listen.server.ServerSkips import org.terst.listen.server.SkipRefresher import org.terst.listen.server.SkipServer import org.terst.listen.server.SkipServerConfig +import org.terst.listen.server.StoredRefusals import org.terst.listen.download.ChaquopyAudioSource import org.terst.listen.download.EpisodeDownloader import org.terst.listen.feed.ChapterFetcher @@ -185,6 +186,10 @@ class AppGraph(private val context: Context) { if (url.isBlank() || token.isBlank()) null else SkipServerConfig(url, token) }, transport = OkHttpSkipTransport(client), + // Written down, not held in memory: a refused token cannot fix itself, so the + // notice has to be there at launch rather than after the next refresh has gone and + // been refused again. + refusals = StoredRefusals(preferences.rejectedServer, preferences::setRejectedServer), ) } diff --git a/app/src/main/java/org/terst/listen/server/Refusals.kt b/app/src/main/java/org/terst/listen/server/Refusals.kt new file mode 100644 index 0000000..c6a51dc --- /dev/null +++ b/app/src/main/java/org/terst/listen/server/Refusals.kt @@ -0,0 +1,49 @@ +package org.terst.listen.server + +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.firstOrNull + +/** + * Where a refused configuration is remembered. + * + * Behind an interface so [SkipServer] does not have to know whether that is a preferences file + * or nothing at all: the app writes it down, and every test wants it in memory without a + * `Context` to hang a DataStore off. + */ +interface Refusals { + val rejected: Flow<SkipServerConfig?> + + /** Records the configuration the server refused, or null once one of them worked. */ + suspend fun record(config: SkipServerConfig?) + + /** Remembers for as long as the process lives, which is what a test needs. */ + class InMemory : Refusals { + private val state = MutableStateFlow<SkipServerConfig?>(null) + override val rejected = state.asStateFlow() + override suspend fun record(config: SkipServerConfig?) { + state.value = config + } + } +} + +/** + * A refusal kept wherever the caller keeps things. + * + * Takes the reading and the writing as two narrow seams rather than a `Preferences`, which would + * drag a `Context` in and make this the one piece of the server client that could only be + * exercised on a device. What it actually decides -- when *not* to write -- is then plain Kotlin. + */ +class StoredRefusals( + override val rejected: Flow<SkipServerConfig?>, + private val save: suspend (SkipServerConfig?) -> Unit, +) : Refusals { + + override suspend fun record(config: SkipServerConfig?) { + // Only on a change. Clearing runs after every successful reply, and a refresh sweep + // makes one request per downloaded episode, so writing each time would be hundreds of + // writes to store the same nothing. + if (rejected.firstOrNull() != config) save(config) + } +} diff --git a/app/src/main/java/org/terst/listen/server/SkipServer.kt b/app/src/main/java/org/terst/listen/server/SkipServer.kt index 69aaef0..5237461 100644 --- a/app/src/main/java/org/terst/listen/server/SkipServer.kt +++ b/app/src/main/java/org/terst/listen/server/SkipServer.kt @@ -1,5 +1,6 @@ package org.terst.listen.server +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.serialization.SerialName @@ -124,6 +125,11 @@ data class ServerEpisode( class SkipServer( private val config: suspend () -> SkipServerConfig?, private val transport: SkipTransport, + /** + * Where a refusal is written down, so it survives the process. Defaults to holding it in + * memory, which is what every test wants and what the app deliberately no longer does. + */ + private val refusals: Refusals = Refusals.InMemory(), ) { /** @@ -133,18 +139,19 @@ class SkipServer( * and the download path must treat the two oppositely: no server means download from the * publisher as the app always did, while a server that merely did not answer means wait. */ - private val refused = MutableStateFlow<SkipServerConfig?>(null) - /** * The configuration the server last refused, or null. * * Remembered so the settings screen can say so: a refused token otherwise changes nothing * visible, and episodes quietly come from publishers. Kept as the refused configuration * rather than a flag so it stops applying the moment a different one is saved, and cleared - * by the next successful reply. Held in memory: it is rediscovered by the first request of - * the next refresh, which is soon enough for something that cannot fix itself. + * by the next successful reply. + * + * Written down rather than held in memory. 401 and 403 are permanent by decision -- they + * cannot fix themselves -- and in memory the notice vanished with the process and only came + * back once the next refresh had gone and been refused all over again. */ - val rejected: StateFlow<SkipServerConfig?> = refused + val rejected: Flow<SkipServerConfig?> get() = refusals.rejected suspend fun isConfigured(): Boolean = config() != null @@ -245,10 +252,10 @@ class SkipServer( * Only a refusal sets it and only a success clears it. No answer says nothing about the * token, so it must not hide a refusal already seen, nor invent one. */ - private fun observe(target: SkipServerConfig, reply: SkipReply): String? { + private suspend fun observe(target: SkipServerConfig, reply: SkipReply): String? { when (reply) { - is SkipReply.Rejected -> refused.value = target - is SkipReply.Body -> refused.value = null + is SkipReply.Rejected -> refusals.record(target) + is SkipReply.Body -> refusals.record(null) is SkipReply.NoAnswer -> Unit } return (reply as? SkipReply.Body)?.text diff --git a/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt b/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt index 5aba893..879205a 100644 --- a/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt +++ b/app/src/main/java/org/terst/listen/ui/SkipServerSettingsState.kt @@ -27,7 +27,7 @@ internal fun rememberSkipServerSettings(): SkipServerSettings { val url by preferences.serverUrl.collectAsState(initial = "") val token by preferences.serverToken.collectAsState(initial = "") - val rejected by Graph.get(context).skipServer.rejected.collectAsState() + val rejected by Graph.get(context).skipServer.rejected.collectAsState(initial = null) val configured = if (url.isBlank() || token.isBlank()) null else SkipServerConfig(url, token) return SkipServerSettings( diff --git a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt index aa0fd06..650c992 100644 --- a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt +++ b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt @@ -1,5 +1,6 @@ package org.terst.listen.server +import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertNull @@ -212,7 +213,7 @@ class SkipServerTest { subject.askAboutOne() - assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.value) + assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.first()) } @Test @@ -221,7 +222,7 @@ class SkipServerTest { assertEquals(false, subject.pushSubscriptions(listOf("https://a"))) - assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.value) + assertEquals(SkipServerConfig("https://listen.terst.org", "stale"), subject.rejected.first()) } @Test @@ -234,7 +235,7 @@ class SkipServerTest { transport.response = ready subject.askAboutOne() - assertNull(subject.rejected.value) + assertNull(subject.rejected.first()) } @Test @@ -246,11 +247,11 @@ class SkipServerTest { transport.reply = SkipReply.NoAnswer subject.askAboutOne() - assertEquals(SkipServerConfig("https://listen.terst.org", "tok"), subject.rejected.value) + assertEquals(SkipServerConfig("https://listen.terst.org", "tok"), subject.rejected.first()) val fresh = server(FakeTransport().apply { reply = SkipReply.NoAnswer }) fresh.askAboutOne() - assertNull(fresh.rejected.value) + assertNull(fresh.rejected.first()) } /** A trailing slash on the configured URL must not produce a double slash in the path. */ diff --git a/app/src/test/java/org/terst/listen/server/StoredRefusalsTest.kt b/app/src/test/java/org/terst/listen/server/StoredRefusalsTest.kt new file mode 100644 index 0000000..b095454 --- /dev/null +++ b/app/src/test/java/org/terst/listen/server/StoredRefusalsTest.kt @@ -0,0 +1,69 @@ +package org.terst.listen.server + +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Writing a refusal down, and -- mostly -- not writing it down. + * + * Clearing happens after every successful reply, and a refresh sweep makes one request per + * downloaded episode. Writing on each of those would be hundreds of writes to the preferences + * file to store the same nothing. + */ +class StoredRefusalsTest { + + private val stored = MutableStateFlow<SkipServerConfig?>(null) + private var writes = 0 + private val refusals = StoredRefusals(stored) { config -> + writes++ + stored.value = config + } + + private val server = SkipServerConfig("https://listen.example", "wrong") + + @Test + fun `a refusal is written down`() = runTest { + refusals.record(server) + + assertEquals(server, stored.value) + assertEquals(1, writes) + } + + @Test + fun `the same refusal arriving again is not written again`() = runTest { + refusals.record(server) + + refusals.record(server) + + assertEquals("one refused token, one write", 1, writes) + } + + @Test + fun `a success clears what was refused`() = runTest { + refusals.record(server) + + refusals.record(null) + + assertEquals(null, stored.value) + assertEquals(2, writes) + } + + @Test + fun `a sweep of successes with nothing refused writes nothing at all`() = runTest { + repeat(200) { refusals.record(null) } + + assertEquals(0, writes) + } + + @Test + fun `a different server being refused replaces the old one`() = runTest { + refusals.record(server) + + val other = SkipServerConfig("https://elsewhere.example", "also-wrong") + refusals.record(other) + + assertEquals(other, stored.value) + } +} |