From 32ebc2a8d0ae2ea3ec6aea02fdd38215fcc5654a Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Wed, 16 Sep 2026 13:47:22 -0700 Subject: [PATCH] feat(settings): size the notification tray by notificationTrayLimit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bell tray's size was configurable on the web and unreachable on Android, where two independent constants decided it instead: the notifications list paged by a hard-coded 20 and the push-poll collapsed anything over five items into a single tray summary. Expose the preference and have both consumers respect it: - Settings gains a "Notification tray limit" number row under View preferences, which is where the web files it — /help/settings lists it there alongside Messages per page, and that page's Notifications section points back with "up to your Notification tray limit (see View Preferences above)". The range is the documented one, 10 to 40 with a default of 20 (/help/settings, echoed by /help/api/notifications' "clamped to 10-40"), enforced client-side so an out-of-range entry never costs a request. - :feature:notifications sizes both its list pages and the poll's fetch by the preference, and the poll passes the same number to the tray raiser as its group cap, replacing SystemNotificationPoster's fixed five. The value reaches :feature:notifications through NotificationTrayLimitStore in :core:network, following the ViewingPreferenceStore pattern #19 established for exactly this problem: no feature module here depends on another, and :feature:profile owns SettingsRepository. The store caches for the process and DefaultSettingsRepository publishes into it on every read and write, so a limit changed in Settings takes effect on the next refresh rather than after a restart. That leaves two narrow accessors in :core:network beside the full SettingsRepository, which adds a reader to the ownership tangle issue #104 already tracks. Closes #35 --- .../android/core/network/dto/UserDto.kt | 8 + .../preferences/NotificationTrayLimitStore.kt | 87 +++++++++ .../NotificationTrayLimitStoreTest.kt | 129 +++++++++++++ .../data/DefaultNotificationsRepository.kt | 25 ++- .../push/NotificationPollRunner.kt | 10 +- .../push/SystemNotificationPoster.kt | 36 +++- .../DefaultNotificationsRepositoryTest.kt | 12 +- .../data/NotificationListTrayLimitTest.kt | 140 ++++++++++++++ .../push/NotificationPollRunnerTest.kt | 14 +- .../push/RecordingSystemNotificationRaiser.kt | 14 +- .../push/SystemTrayGroupLimitTest.kt | 168 +++++++++++++++++ .../feature/profile/ui/SettingsScreenTest.kt | 76 ++++++++ .../profile/data/DefaultSettingsRepository.kt | 9 + .../feature/profile/domain/SettingsBounds.kt | 21 +++ .../profile/ui/settings/SettingsScreen.kt | 30 ++- .../profile/ui/settings/SettingsViewModel.kt | 29 +++ .../data/DefaultSettingsRepositoryTest.kt | 63 ++++++- .../profile/ui/FakeSettingsRepository.kt | 2 + .../ui/SettingsNotificationTrayLimitTest.kt | 178 ++++++++++++++++++ 19 files changed, 1027 insertions(+), 24 deletions(-) create mode 100644 core/network/src/main/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStore.kt create mode 100644 core/network/src/test/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStoreTest.kt create mode 100644 feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/NotificationListTrayLimitTest.kt create mode 100644 feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/SystemTrayGroupLimitTest.kt create mode 100644 feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsNotificationTrayLimitTest.kt diff --git a/core/network/src/main/kotlin/com/interlinedlist/android/core/network/dto/UserDto.kt b/core/network/src/main/kotlin/com/interlinedlist/android/core/network/dto/UserDto.kt index e160003..a165b8e 100644 --- a/core/network/src/main/kotlin/com/interlinedlist/android/core/network/dto/UserDto.kt +++ b/core/network/src/main/kotlin/com/interlinedlist/android/core/network/dto/UserDto.kt @@ -25,6 +25,14 @@ data class UserDto( * resolves it. */ val viewingPreference: String? = null, + /** + * How many notifications the bell tray holds before older ones drop off + * (`/help/settings`: "default is 20 and you can set any value from 10 to 40"). + * Nullable because public/partial user payloads omit it; + * [com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore] + * resolves the absent case. + */ + val notificationTrayLimit: Int? = null, ) /** Maps the wire model into the domain [User]. */ diff --git a/core/network/src/main/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStore.kt b/core/network/src/main/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStore.kt new file mode 100644 index 0000000..d5cdeef --- /dev/null +++ b/core/network/src/main/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStore.kt @@ -0,0 +1,87 @@ +package com.interlinedlist.android.core.network.preferences + +import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.error.safeApiCall +import kotlinx.serialization.json.Json +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Reads the account's `notificationTrayLimit` — how many notifications the tray + * holds — for the consumers that have to size themselves by it. + * + * It lives in `:core:network` for the same reason [ViewingPreferenceStore] does: + * **no feature module in this repo depends on another feature module**, yet the + * preference is written by `:feature:profile`'s Settings screen and read by + * `:feature:notifications` (the in-app list's page size and the system-tray group + * cap). A narrow accessor on the shared `GET /api/user` is the seam they can both + * reach. + * + * The value is cached for the process so a poll or a pull-to-refresh does not pay + * for an extra `GET /api/user` every time; `:feature:profile` calls [publish] after + * every settings read or write, so a change made in Settings takes effect at once + * instead of waiting for a restart. + * + * Consequence to be aware of: this is the **second** narrow account-preference + * accessor in `:core:network`, sitting alongside `:feature:profile`'s full + * `SettingsRepository`. Issue #104 ("Consolidate the two writers of account + * preferences behind one owner") already tracks untangling that ownership, and this + * store adds a reader to the same pile — so extend it rather than introducing a third + * mechanism. + */ +@Singleton +class NotificationTrayLimitStore @Inject constructor( + private val api: InterlinedListApi, + private val json: Json, +) { + + /** Last known limit, or null until something reads or publishes one. */ + @Volatile + private var cached: Int? = null + + /** + * The limit to size the tray by: the cached value, otherwise a fresh read of + * `GET /api/user`. + * + * A failed read answers [DEFAULT] and is deliberately **not** cached, so the next + * caller retries — notifications must still load when the user endpoint is down, + * but one blip must not pin the limit for the whole process. + */ + suspend fun current(): Int { + cached?.let { return it } + return when ( + val result = safeApiCall(json) { api.getCurrentUser().user.notificationTrayLimit } + ) { + is ApiResult.Success -> publish(result.data) + is ApiResult.Failure -> DEFAULT + } + } + + /** + * Records the limit the server reports, clamped to [RANGE], and returns the value + * that took effect. A null [limit] (the account has no stored value) resolves to + * [DEFAULT]. + * + * Clamping matters because the server's own range is the contract the tray is + * sized by: a stored value from elsewhere must never make the list ask for a page + * the endpoint would refuse. + */ + fun publish(limit: Int?): Int = + (limit?.coerceIn(RANGE) ?: DEFAULT).also { cached = it } + + companion object { + /** + * The limit a fresh account gets. `/help/settings`: "The default is 20 and you + * can set any value from 10 to 40" — matching the live `GET /api/user` value. + */ + const val DEFAULT: Int = 20 + + /** + * The values the server accepts, published in two places: `/help/settings` + * ("any value from 10 to 40") and `/help/api/notifications` ("the user's + * configured tray limit (default 20, clamped to 10-40)"). + */ + val RANGE: IntRange = 10..40 + } +} diff --git a/core/network/src/test/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStoreTest.kt b/core/network/src/test/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStoreTest.kt new file mode 100644 index 0000000..253e19f --- /dev/null +++ b/core/network/src/test/kotlin/com/interlinedlist/android/core/network/preferences/NotificationTrayLimitStoreTest.kt @@ -0,0 +1,129 @@ +package com.interlinedlist.android.core.network.preferences + +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import kotlinx.coroutines.runBlocking +import kotlinx.serialization.json.Json +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test +import retrofit2.Retrofit + +/** + * The shared `notificationTrayLimit` accessor: the one number both the in-app + * notifications list and the system-tray group are sized by. + * + * The bounds come from the help centre, which publishes them twice — `/help/settings` + * ("The default is 20 and you can set any value from 10 to 40") and + * `/help/api/notifications` ("up to the user's configured tray limit (default 20, + * clamped to 10-40)") — and the default matches the live `GET /api/user`. + */ +class NotificationTrayLimitStoreTest { + + private lateinit var server: MockWebServer + private lateinit var store: NotificationTrayLimitStore + + // Mirrors the production Json (see NetworkModule). + private val json = Json { + ignoreUnknownKeys = true + explicitNulls = false + coerceInputValues = true + } + + @Before + fun setUp() { + server = MockWebServer() + server.start() + store = newStore() + } + + @After + fun tearDown() = server.shutdown() + + /** A store with an empty cache, pointed at the test server. */ + private fun newStore(): NotificationTrayLimitStore { + val api = Retrofit.Builder() + .baseUrl(server.url("/")) + .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) + .build() + .create(InterlinedListApi::class.java) + return NotificationTrayLimitStore(api, json) + } + + private fun enqueueUser(trayLimit: Int?) { + val field = trayLimit?.let { """, "notificationTrayLimit": $it""" } ?: "" + server.enqueue( + MockResponse().setBody("""{ "user": { "id": "u1", "username": "me"$field } }"""), + ) + } + + @Test + fun `current reads the limit from GET api user`() = runBlocking { + enqueueUser(40) + + assertThat(store.current()).isEqualTo(40) + assertThat(server.takeRequest().path).isEqualTo("/api/user") + } + + @Test + fun `an absent limit falls back to the documented default`() = runBlocking { + enqueueUser(null) + + assertThat(store.current()).isEqualTo(20) + } + + @Test + fun `the limit is read once and then served from the cache`() = runBlocking { + enqueueUser(30) + + assertThat(store.current()).isEqualTo(30) + assertThat(store.current()).isEqualTo(30) + + // A second GET would have no queued response; only one request was made. + assertThat(server.requestCount).isEqualTo(1) + } + + @Test + fun `a failed read falls back to the default without caching it`() = runBlocking { + server.enqueue(MockResponse().setResponseCode(500).setBody("""{ "error": "boom" }""")) + enqueueUser(35) + + // The tray still has a size to work with... + assertThat(store.current()).isEqualTo(20) + // ...and the blip is not pinned for the rest of the process. + assertThat(store.current()).isEqualTo(35) + } + + @Test + fun `a stored value outside the server's range is clamped`() = runBlocking { + enqueueUser(400) + assertThat(store.current()).isEqualTo(40) + + // A fresh store, so the clamp is exercised on the way in rather than from cache. + enqueueUser(1) + assertThat(newStore().current()).isEqualTo(10) + } + + @Test + fun `a published limit takes effect without a request`() = runBlocking { + assertThat(store.publish(25)).isEqualTo(25) + + assertThat(store.current()).isEqualTo(25) + assertThat(server.requestCount).isEqualTo(0) + } + + @Test + fun `publishing an absent limit resolves to the default`() { + assertThat(store.publish(null)).isEqualTo(20) + } + + @Test + fun `the documented bounds match the help centre`() { + assertThat(NotificationTrayLimitStore.DEFAULT).isEqualTo(20) + assertThat(NotificationTrayLimitStore.RANGE).isEqualTo(10..40) + } +} diff --git a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepository.kt b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepository.kt index c8e8d7d..0961933 100644 --- a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepository.kt +++ b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepository.kt @@ -3,12 +3,12 @@ package com.interlinedlist.android.feature.notifications.data import com.interlinedlist.android.core.common.dispatcher.DispatcherProvider import com.interlinedlist.android.core.common.result.ApiResult import com.interlinedlist.android.core.network.error.safeApiCall +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.notifications.data.local.NotificationDao import com.interlinedlist.android.feature.notifications.data.local.NotificationEntity import com.interlinedlist.android.feature.notifications.data.local.toDomain import com.interlinedlist.android.feature.notifications.data.local.toEntity import com.interlinedlist.android.feature.notifications.data.remote.NotificationsApi -import com.interlinedlist.android.feature.notifications.data.remote.dto.PaginationDto import com.interlinedlist.android.feature.notifications.data.remote.dto.toDomain import com.interlinedlist.android.feature.notifications.domain.Notification import kotlinx.coroutines.flow.Flow @@ -18,9 +18,19 @@ import kotlinx.coroutines.withContext import kotlinx.serialization.json.Json import javax.inject.Inject +/** + * Notifications backed by Room, refreshed from the API. + * + * Every page is sized by the account's `notificationTrayLimit` rather than by a + * constant, so the app's list holds what the web's bell tray holds. The limit comes + * from [NotificationTrayLimitStore] in `:core:network` because the preference is + * written by `:feature:profile`'s Settings screen and no feature module here may + * depend on another. + */ class DefaultNotificationsRepository @Inject constructor( private val api: NotificationsApi, private val notificationDao: NotificationDao, + private val trayLimitStore: NotificationTrayLimitStore, private val json: Json, private val dispatchers: DispatcherProvider, ) : NotificationsRepository { @@ -31,14 +41,14 @@ class DefaultNotificationsRepository @Inject constructor( override fun observeUnreadCount(): Flow = notificationDao.observeUnreadCount() override suspend fun fetchLatest(): ApiResult> = withContext(dispatchers.io) { - when (val result = safeCall { api.getNotifications(limit = PaginationDto.DEFAULT_LIMIT, offset = 0) }) { + when (val result = safeCall { api.getNotifications(limit = pageSize(), offset = 0) }) { is ApiResult.Success -> ApiResult.Success(result.data.items.map { it.toDomain() }) is ApiResult.Failure -> result } } override suspend fun refresh(): ApiResult = withContext(dispatchers.io) { - when (val result = safeCall { api.getNotifications(limit = PaginationDto.DEFAULT_LIMIT, offset = 0) }) { + when (val result = safeCall { api.getNotifications(limit = pageSize(), offset = 0) }) { is ApiResult.Success -> { val page = result.data val entities = page.items.mapIndexed { index, dto -> @@ -54,7 +64,7 @@ class DefaultNotificationsRepository @Inject constructor( override suspend fun loadMore(currentCount: Int): ApiResult = withContext(dispatchers.io) { when (val result = safeCall { - api.getNotifications(limit = PaginationDto.DEFAULT_LIMIT, offset = currentCount) + api.getNotifications(limit = pageSize(), offset = currentCount) }) { is ApiResult.Success -> { val page = result.data @@ -106,6 +116,13 @@ class DefaultNotificationsRepository @Inject constructor( // --- helpers ----------------------------------------------------------- + /** + * How many notifications one page holds: the account's tray limit. The endpoint's + * own `limit` accepts 1-50 (`/help/api/notifications`), which comfortably contains + * the 10-40 the tray limit is clamped to, so the preference can be sent verbatim. + */ + private suspend fun pageSize(): Int = trayLimitStore.current() + private suspend fun safeCall(block: suspend () -> T): ApiResult = safeApiCall(json, block) } diff --git a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunner.kt b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunner.kt index 654b2c1..9ca2e53 100644 --- a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunner.kt +++ b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunner.kt @@ -1,6 +1,7 @@ package com.interlinedlist.android.feature.notifications.push import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.notifications.data.NotificationPreferencesRepository import com.interlinedlist.android.feature.notifications.data.NotificationsRepository import javax.inject.Inject @@ -10,6 +11,12 @@ import javax.inject.Inject * preferences, decide what is NEW and push-enabled (via the pure [NotificationPollProcessor]), * raise the tray notifications, and advance the persisted last-seen marker. * + * The account's `notificationTrayLimit` is what the system tray is sized by: the + * repository already fetches a page of that size, and this runner passes the same + * number to the raiser so the shade never shows more separate notifications than the + * user asked the tray to hold. The value is cached by [NotificationTrayLimitStore], + * so reading it here costs no second request. + * * Holding this outside the `@HiltWorker` keeps it free of the Android `CoroutineWorker` * superclass, so it is fully unit-testable; [NotificationsPollWorker] is a thin adapter * that maps the [Result] onto WorkManager's outcome. @@ -18,6 +25,7 @@ class NotificationPollRunner @Inject constructor( private val notificationsRepository: NotificationsRepository, private val preferencesRepository: NotificationPreferencesRepository, private val lastSeenStore: LastSeenNotificationStore, + private val trayLimitStore: NotificationTrayLimitStore, private val raiser: SystemNotificationRaiser, ) { @@ -51,7 +59,7 @@ class NotificationPollRunner @Inject constructor( ) if (outcome.toPost.isNotEmpty()) { - raiser.post(outcome.toPost) + raiser.post(outcome.toPost, maxIndividual = trayLimitStore.current()) } // Advance the marker after posting, so a crash before posting doesn't skip items. outcome.newLastSeenId?.let(lastSeenStore::setLastSeenId) diff --git a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/SystemNotificationPoster.kt b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/SystemNotificationPoster.kt index b52c5ae..4ec044f 100644 --- a/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/SystemNotificationPoster.kt +++ b/feature/notifications/src/main/kotlin/com/interlinedlist/android/feature/notifications/push/SystemNotificationPoster.kt @@ -14,15 +14,24 @@ import com.interlinedlist.android.feature.notifications.domain.Notification * side-effects live only in [SystemNotificationPoster]. */ interface SystemNotificationRaiser { - /** Raises tray notifications for [items] (newest-first, already push-filtered). */ - fun post(items: List) + /** + * Raises tray notifications for [items] (newest-first, already push-filtered), + * showing at most [maxIndividual] of them separately. + */ + fun post(items: List, maxIndividual: Int) } /** * Posts system-tray notifications for a batch of NEW notifications. The batch is - * capped so a large backlog can't spam the tray: at most [MAX_INDIVIDUAL] individual + * capped so a large backlog can't spam the tray: at most `maxIndividual` individual * items are shown, and when there are more, only a single summary is posted. * + * The cap is the account's `notificationTrayLimit` — the same preference that sizes + * the web's bell tray and this app's notifications list — rather than a constant of + * this class's own choosing, so "how many notifications the tray holds" means one + * thing everywhere. [NotificationPollRunner] resolves it and passes it in, keeping + * this class free of the preference lookup. + * * Each notification's tap opens the app's launcher activity (resolved via the package * manager, so this module needs no compile-time reference to `MainActivity`) carrying * the [NotificationDeepLink] extras the app reads to route. @@ -37,13 +46,16 @@ class SystemNotificationPoster( /** * Posts [items] (newest-first, already push-filtered). No-op when the list is empty * or the user has notifications disabled at the OS level. + * + * [maxIndividual] is coerced to at least 1 so a nonsensical cap still surfaces the + * activity as a summary rather than swallowing it. */ - override fun post(items: List) { + override fun post(items: List, maxIndividual: Int) { if (items.isEmpty()) return val manager = NotificationManagerCompat.from(context) if (!manager.areNotificationsEnabled()) return - if (items.size > MAX_INDIVIDUAL) { + if (collapsesToSummary(items.size, maxIndividual)) { postSummaryOnly(manager, items) return } @@ -139,8 +151,18 @@ class SystemNotificationPoster( } companion object { - /** Max individual notifications before collapsing to a single summary. */ - const val MAX_INDIVIDUAL = 5 + /** + * Whether a batch of [count] items collapses to a single summary instead of + * being posted one by one: true once it exceeds [maxIndividual], the account's + * `notificationTrayLimit`. + * + * Pure, and separate from [post], because this is the rule the tray group is + * sized by and it has to be assertable without an Android notification manager. + * A nonsensical cap is coerced to at least 1, so activity still surfaces as a + * summary rather than being swallowed. + */ + fun collapsesToSummary(count: Int, maxIndividual: Int): Boolean = + count > maxIndividual.coerceAtLeast(1) /** Shared group key so the shade collapses our notifications together. */ const val GROUP_KEY = "il.notifications.group" diff --git a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepositoryTest.kt b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepositoryTest.kt index 6d4a5ec..738413e 100644 --- a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepositoryTest.kt +++ b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/DefaultNotificationsRepositoryTest.kt @@ -3,6 +3,8 @@ package com.interlinedlist.android.feature.notifications.data import com.google.common.truth.Truth.assertThat import com.interlinedlist.android.core.common.result.ApiResult import com.interlinedlist.android.core.common.result.AppError +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.notifications.data.remote.NotificationsApi import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -27,18 +29,23 @@ class DefaultNotificationsRepositoryTest { private lateinit var server: MockWebServer private lateinit var api: NotificationsApi private lateinit var dao: FakeNotificationDao + private lateinit var trayLimitStore: NotificationTrayLimitStore @Before fun setUp() { server = MockWebServer() server.start() val contentType = "application/json".toMediaType() - api = Retrofit.Builder() + val retrofit = Retrofit.Builder() .baseUrl(server.url("/")) .addConverterFactory(json.asConverterFactory(contentType)) .build() - .create(NotificationsApi::class.java) + api = retrofit.create(NotificationsApi::class.java) dao = FakeNotificationDao() + trayLimitStore = + NotificationTrayLimitStore(retrofit.create(InterlinedListApi::class.java), json) + // Pre-seeded so each test's queued responses are spent on notifications alone. + trayLimitStore.publish(NotificationTrayLimitStore.DEFAULT) } @After @@ -47,6 +54,7 @@ class DefaultNotificationsRepositoryTest { private fun repository() = DefaultNotificationsRepository( api = api, notificationDao = dao, + trayLimitStore = trayLimitStore, json = json, dispatchers = TestDispatcherProvider(dispatcher), ) diff --git a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/NotificationListTrayLimitTest.kt b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/NotificationListTrayLimitTest.kt new file mode 100644 index 0000000..1616ddd --- /dev/null +++ b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/data/NotificationListTrayLimitTest.kt @@ -0,0 +1,140 @@ +package com.interlinedlist.android.feature.notifications.data + +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore +import com.interlinedlist.android.feature.notifications.data.remote.NotificationsApi +import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test +import retrofit2.Retrofit + +/** + * Issue #35: the in-app notifications list is sized by the account's + * `notificationTrayLimit` instead of a hard-coded page size. + * + * `/help/settings` describes the preference as "How many notifications the bell tray + * holds before older ones drop off", so the app's list has to hold the same number the + * web's tray does. `/help/api/notifications` documents the endpoint's own `limit` as + * 1-50, which comfortably contains the 10-40 the preference is clamped to, so the + * value can be sent verbatim. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class NotificationListTrayLimitTest { + + private val dispatcher = StandardTestDispatcher() + private val json = Json { ignoreUnknownKeys = true; explicitNulls = false } + + private lateinit var server: MockWebServer + private lateinit var api: NotificationsApi + private lateinit var userApi: InterlinedListApi + private lateinit var dao: FakeNotificationDao + + @Before + fun setUp() { + server = MockWebServer() + server.start() + val retrofit = Retrofit.Builder() + .baseUrl(server.url("/")) + .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) + .build() + api = retrofit.create(NotificationsApi::class.java) + userApi = retrofit.create(InterlinedListApi::class.java) + dao = FakeNotificationDao() + } + + @After + fun tearDown() = server.shutdown() + + /** A store already holding [limit], or one that still has to read it from the API. */ + private fun store(limit: Int?) = + NotificationTrayLimitStore(userApi, json).apply { limit?.let { publish(it) } } + + private fun repository(store: NotificationTrayLimitStore) = DefaultNotificationsRepository( + api = api, + notificationDao = dao, + trayLimitStore = store, + json = json, + dispatchers = TestDispatcherProvider(dispatcher), + ) + + private fun enqueuePage(hasMore: Boolean = false, ids: List = listOf("1")) { + val rows = ids.joinToString(",") { """{ "id": "$it", "type": "follow", "subject": "s$it" }""" } + server.enqueue( + MockResponse().setBody("""{ "data": [ $rows ], "pagination": { "hasMore": $hasMore } }"""), + ) + } + + @Test + fun `refresh asks for a page the size of the account's tray limit`() = runTest(dispatcher) { + enqueuePage() + + repository(store(40)).refresh() + + val url = server.takeRequest().requestUrl!! + assertThat(url.encodedPath).isEqualTo("/api/notifications") + assertThat(url.queryParameter("limit")).isEqualTo("40") + assertThat(url.queryParameter("offset")).isEqualTo("0") + } + + @Test + fun `loadMore pages by the same limit from the current offset`() = runTest(dispatcher) { + enqueuePage(hasMore = true) + enqueuePage(ids = listOf("2")) + val repo = repository(store(10)) + + repo.refresh() + repo.loadMore(currentCount = 10) + + server.takeRequest() + val url = server.takeRequest().requestUrl!! + assertThat(url.queryParameter("limit")).isEqualTo("10") + assertThat(url.queryParameter("offset")).isEqualTo("10") + } + + @Test + fun `the poll's fetch is sized by the limit too, so it never sees less than the tray holds`() = + runTest(dispatcher) { + enqueuePage() + + repository(store(35)).fetchLatest() + + assertThat(server.takeRequest().requestUrl!!.queryParameter("limit")).isEqualTo("35") + } + + @Test + fun `an account with no stored limit falls back to the documented default of 20`() = + runTest(dispatcher) { + // Nothing cached, so `GET /api/user` answers first, then the list. + server.enqueue(MockResponse().setBody("""{ "user": { "id": "u1" } }""")) + enqueuePage() + + repository(store(null)).refresh() + + assertThat(server.takeRequest().path).isEqualTo("/api/user") + assertThat(server.takeRequest().requestUrl!!.queryParameter("limit")).isEqualTo("20") + } + + @Test + fun `a change saved in Settings is honoured by the very next refresh`() = runTest(dispatcher) { + val store = store(20) + enqueuePage() + repository(store).refresh() + assertThat(server.takeRequest().requestUrl!!.queryParameter("limit")).isEqualTo("20") + + // Settings PATCHed a new value and published it to the shared store. + store.publish(40) + enqueuePage() + repository(store).refresh() + + assertThat(server.takeRequest().requestUrl!!.queryParameter("limit")).isEqualTo("40") + } +} diff --git a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunnerTest.kt b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunnerTest.kt index f43c65c..1ca64a7 100644 --- a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunnerTest.kt +++ b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/NotificationPollRunnerTest.kt @@ -2,6 +2,8 @@ package com.interlinedlist.android.feature.notifications.push import com.google.common.truth.Truth.assertThat import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.notifications.data.DefaultNotificationsRepository import com.interlinedlist.android.feature.notifications.data.FakeNotificationDao import com.interlinedlist.android.feature.notifications.data.NotificationPreferencesRepository @@ -31,18 +33,24 @@ class NotificationPollRunnerTest { private lateinit var server: MockWebServer private lateinit var api: NotificationsApi private lateinit var dao: FakeNotificationDao + private lateinit var trayLimitStore: NotificationTrayLimitStore @Before fun setUp() { server = MockWebServer() server.start() val contentType = "application/json".toMediaType() - api = Retrofit.Builder() + val retrofit = Retrofit.Builder() .baseUrl(server.url("/")) .addConverterFactory(json.asConverterFactory(contentType)) .build() - .create(NotificationsApi::class.java) + api = retrofit.create(NotificationsApi::class.java) dao = FakeNotificationDao() + trayLimitStore = + NotificationTrayLimitStore(retrofit.create(InterlinedListApi::class.java), json) + // Pre-seeded so the poll spends its queued responses on notifications, not on + // `GET /api/user`; the fetch itself is covered by NotificationTrayLimitStoreTest. + trayLimitStore.publish(NotificationTrayLimitStore.DEFAULT) } @After @@ -51,6 +59,7 @@ class NotificationPollRunnerTest { private fun notificationsRepo() = DefaultNotificationsRepository( api = api, notificationDao = dao, + trayLimitStore = trayLimitStore, json = json, dispatchers = TestDispatcherProvider(dispatcher), ) @@ -72,6 +81,7 @@ class NotificationPollRunnerTest { notificationsRepository = notificationsRepo(), preferencesRepository = prefs, lastSeenStore = store, + trayLimitStore = trayLimitStore, raiser = raiser, ) diff --git a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/RecordingSystemNotificationRaiser.kt b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/RecordingSystemNotificationRaiser.kt index af9166e..18cc79e 100644 --- a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/RecordingSystemNotificationRaiser.kt +++ b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/RecordingSystemNotificationRaiser.kt @@ -5,13 +5,21 @@ import com.interlinedlist.android.feature.notifications.domain.Notification /** Records every [post] call so tests can assert which notifications were raised. */ class RecordingSystemNotificationRaiser : SystemNotificationRaiser { + /** Each element is one [post] call: the batch and the cap it was given. */ + data class Posted(val items: List, val maxIndividual: Int) + + val posts = mutableListOf() + /** Each element is the batch handed to one [post] call. */ - val batches = mutableListOf>() + val batches: List> get() = posts.map { it.items } /** Flattened ids across all batches, in order. */ val postedIds: List get() = batches.flatten().map { it.id } - override fun post(items: List) { - batches += items + /** The cap handed to the single [post] call a test made. */ + val maxIndividual: Int get() = posts.single().maxIndividual + + override fun post(items: List, maxIndividual: Int) { + posts += Posted(items, maxIndividual) } } diff --git a/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/SystemTrayGroupLimitTest.kt b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/SystemTrayGroupLimitTest.kt new file mode 100644 index 0000000..232063b --- /dev/null +++ b/feature/notifications/src/test/kotlin/com/interlinedlist/android/feature/notifications/push/SystemTrayGroupLimitTest.kt @@ -0,0 +1,168 @@ +package com.interlinedlist.android.feature.notifications.push + +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore +import com.interlinedlist.android.feature.notifications.data.DefaultNotificationsRepository +import com.interlinedlist.android.feature.notifications.data.FakeNotificationDao +import com.interlinedlist.android.feature.notifications.data.NotificationPreferencesRepository +import com.interlinedlist.android.feature.notifications.data.TestDispatcherProvider +import com.interlinedlist.android.feature.notifications.data.remote.NotificationsApi +import com.interlinedlist.android.feature.notifications.domain.NotificationPreference +import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test +import retrofit2.Retrofit + +/** + * Issue #35: the system-tray group is capped by the account's `notificationTrayLimit` + * rather than by a constant this module chose for itself. + * + * Before, [SystemNotificationPoster] collapsed anything over five items into a single + * summary regardless of what the user had configured. Now the poll resolves the same + * preference the in-app list uses and hands it to the raiser, so "how many + * notifications the tray holds" (`/help/settings`) means one thing everywhere. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class SystemTrayGroupLimitTest { + + private val dispatcher = StandardTestDispatcher() + private val json = Json { ignoreUnknownKeys = true; explicitNulls = false } + + private lateinit var server: MockWebServer + private lateinit var api: NotificationsApi + private lateinit var userApi: InterlinedListApi + private lateinit var dao: FakeNotificationDao + + @Before + fun setUp() { + server = MockWebServer() + server.start() + val retrofit = Retrofit.Builder() + .baseUrl(server.url("/")) + .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) + .build() + api = retrofit.create(NotificationsApi::class.java) + userApi = retrofit.create(InterlinedListApi::class.java) + dao = FakeNotificationDao() + } + + @After + fun tearDown() = server.shutdown() + + // --- the cap the poll hands to the tray ---------------------------------- + + @Test + fun `the poll caps the tray group at the account's limit`() = runTest(dispatcher) { + enqueuePage("3", "2") + val raiser = RecordingSystemNotificationRaiser() + + runner(store(12), raiser).run() + + assertThat(raiser.postedIds).containsExactly("3", "2").inOrder() + assertThat(raiser.maxIndividual).isEqualTo(12) + } + + @Test + fun `a different account limit produces a different cap`() = runTest(dispatcher) { + enqueuePage("3", "2") + val raiser = RecordingSystemNotificationRaiser() + + runner(store(40), raiser).run() + + assertThat(raiser.maxIndividual).isEqualTo(40) + } + + @Test + fun `resolving the cap costs no extra request once the page has been fetched`() = + runTest(dispatcher) { + // Only `GET /api/user` and the notifications page are queued: if the runner + // read the preference twice over the network, the second read would hang. + server.enqueue(MockResponse().setBody("""{ "user": { "id": "u1", "notificationTrayLimit": 30 } }""")) + enqueuePage("3", "2") + val raiser = RecordingSystemNotificationRaiser() + + runner(store(null), raiser).run() + + assertThat(raiser.maxIndividual).isEqualTo(30) + assertThat(server.requestCount).isEqualTo(2) + } + + // --- the grouping rule itself -------------------------------------------- + + @Test + fun `a batch within the limit is posted item by item`() { + assertThat(SystemNotificationPoster.collapsesToSummary(count = 10, maxIndividual = 10)) + .isFalse() + assertThat(SystemNotificationPoster.collapsesToSummary(count = 9, maxIndividual = 10)) + .isFalse() + } + + @Test + fun `a batch over the limit collapses to a single summary`() { + assertThat(SystemNotificationPoster.collapsesToSummary(count = 11, maxIndividual = 10)) + .isTrue() + } + + @Test + fun `the old fixed cap of five no longer decides anything`() { + // Six items used to collapse; with the account's limit they no longer do. + assertThat(SystemNotificationPoster.collapsesToSummary(count = 6, maxIndividual = 20)) + .isFalse() + // ...and a deliberately small tray collapses well before five. + assertThat(SystemNotificationPoster.collapsesToSummary(count = 3, maxIndividual = 2)) + .isTrue() + } + + @Test + fun `a nonsensical cap still surfaces the activity as a summary`() { + assertThat(SystemNotificationPoster.collapsesToSummary(count = 1, maxIndividual = 0)) + .isFalse() + assertThat(SystemNotificationPoster.collapsesToSummary(count = 2, maxIndividual = -5)) + .isTrue() + } + + // --- helpers ------------------------------------------------------------- + + private fun store(limit: Int?) = + NotificationTrayLimitStore(userApi, json).apply { limit?.let { publish(it) } } + + private fun enqueuePage(vararg ids: String) { + val rows = ids.joinToString(",") { """{ "id": "$it", "type": "follow", "subject": "s$it" }""" } + server.enqueue( + MockResponse().setBody("""{ "data": [ $rows ], "pagination": { "hasMore": false } }"""), + ) + } + + private fun runner( + trayLimitStore: NotificationTrayLimitStore, + raiser: RecordingSystemNotificationRaiser, + ) = NotificationPollRunner( + notificationsRepository = DefaultNotificationsRepository( + api = api, + notificationDao = dao, + trayLimitStore = trayLimitStore, + json = json, + dispatchers = TestDispatcherProvider(dispatcher), + ), + preferencesRepository = object : NotificationPreferencesRepository { + override suspend fun getPreferences(): ApiResult> = + ApiResult.Success(emptyList()) + + override suspend fun updatePreference(preference: NotificationPreference) = + ApiResult.Success(Unit) + }, + lastSeenStore = FakeLastSeenNotificationStore(initial = "1"), + trayLimitStore = trayLimitStore, + raiser = raiser, + ) +} diff --git a/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt b/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt index 90bab43..12361e1 100644 --- a/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt +++ b/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt @@ -41,6 +41,7 @@ class SettingsScreenTest { onSelectViewingPreference: (ViewingPreference) -> Unit = {}, onToggleShowPreviews: (Boolean) -> Unit = {}, onSetMessagesPerPage: (Int) -> Unit = {}, + onSetNotificationTrayLimit: (Int) -> Unit = {}, onSetMaxMessageLength: (Int) -> Unit = {}, onToggleDefaultPubliclyVisible: (Boolean) -> Unit = {}, onToggleShowAdvancedPostSettings: (Boolean) -> Unit = {}, @@ -57,6 +58,7 @@ class SettingsScreenTest { onSelectViewingPreference = onSelectViewingPreference, onToggleShowPreviews = onToggleShowPreviews, onSetMessagesPerPage = onSetMessagesPerPage, + onSetNotificationTrayLimit = onSetNotificationTrayLimit, onSetMaxMessageLength = onSetMaxMessageLength, onToggleDefaultPubliclyVisible = onToggleDefaultPubliclyVisible, onToggleShowAdvancedPostSettings = onToggleShowAdvancedPostSettings, @@ -282,6 +284,78 @@ class SettingsScreenTest { assert(saved == 25) } + // --- Notification tray limit (issue #35) --------------------------------- + // The web files this under View preferences (/help/settings lists it there, and + // its Notifications section points back with "see View Preferences above"), so it + // sits in the same group here. + + @Test + fun notificationTrayLimit_isOfferedUnderViewPreferences() { + setContent( + state = SettingsUiState(settings = UserSettings(notificationTrayLimit = 30)), + ) + + composeRule.onNodeWithTag(SettingsTestTags.VIEW_PREFERENCES).assertIsDisplayed() + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT) + .assertTextEquals("30") + } + + @Test + fun notificationTrayLimit_showsTheServerDefaultWhenTheAccountHasNone() { + setContent( + state = SettingsUiState(settings = UserSettings(notificationTrayLimit = null)), + ) + + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT) + .assertTextEquals("20") + } + + @Test + fun notificationTrayLimit_cannotStepAboveTheDocumentedMaximum() { + var saved: Int? = null + setContent( + state = SettingsUiState(settings = UserSettings(notificationTrayLimit = 40)), + onSetNotificationTrayLimit = { saved = it }, + ) + + composeRule.onNodeWithTag(settingsIncrementTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT)) + .assertIsNotEnabled() + + assert(saved == null) + } + + @Test + fun notificationTrayLimit_outOfRangeEntryIsRejectedWithoutReportingAValue() { + var saved: Int? = null + setContent( + state = SettingsUiState(settings = UserSettings(notificationTrayLimit = 20)), + onSetNotificationTrayLimit = { saved = it }, + ) + + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performTextClearance() + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performTextInput("41") + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performImeAction() + + composeRule.onNodeWithTag(settingsNumberErrorTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT)) + .assertIsDisplayed() + assert(saved == null) { "an out-of-range entry must not be saved, got $saved" } + } + + @Test + fun notificationTrayLimit_inRangeEntryIsReported() { + var saved: Int? = null + setContent( + state = SettingsUiState(settings = UserSettings(notificationTrayLimit = 20)), + onSetNotificationTrayLimit = { saved = it }, + ) + + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performTextClearance() + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performTextInput("35") + composeRule.onNodeWithTag(SettingsTestTags.NOTIFICATION_TRAY_LIMIT).performImeAction() + + assert(saved == 35) + } + @Test fun characterLimit_rolledBackSaveRestoresTheFieldToTheStoredValue() { // The screen is recomposed with the previous value after a rejected save; @@ -296,6 +370,7 @@ class SettingsScreenTest { onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, + onSetNotificationTrayLimit = {}, // Optimistic apply, then the server refuses and it rolls back. onSetMaxMessageLength = { settings = settings.copy(maxMessageLength = it) }, onToggleDefaultPubliclyVisible = {}, @@ -381,6 +456,7 @@ class SettingsScreenTest { onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, + onSetNotificationTrayLimit = {}, onSetMaxMessageLength = {}, onToggleDefaultPubliclyVisible = {}, onToggleShowAdvancedPostSettings = {}, diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt index 699f715..c039cde 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt @@ -4,6 +4,7 @@ import com.interlinedlist.android.core.common.dispatcher.DispatcherProvider import com.interlinedlist.android.core.common.result.ApiResult import com.interlinedlist.android.core.common.result.AppError import com.interlinedlist.android.core.network.error.safeApiCall +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.profile.data.mapper.toRequest import com.interlinedlist.android.feature.profile.data.mapper.toUserSettings import com.interlinedlist.android.feature.profile.data.remote.ProfileApi @@ -21,10 +22,17 @@ import javax.inject.Singleton * Network-backed settings with a process-scoped in-memory cache: every successful * read or write publishes the new value to [observeSettings], so the Settings screen * and the feed see the same preferences without either re-fetching. + * + * `notificationTrayLimit` is additionally forwarded to [NotificationTrayLimitStore] in + * `:core:network`, because `:feature:notifications` sizes its list and its system-tray + * group by that preference and no feature module here may depend on another. Without + * the forward, a limit changed in Settings would not take effect until the process + * restarted. */ @Singleton class DefaultSettingsRepository @Inject constructor( private val api: ProfileApi, + private val trayLimitStore: NotificationTrayLimitStore, private val json: Json, private val dispatchers: DispatcherProvider, ) : SettingsRepository { @@ -59,6 +67,7 @@ class DefaultSettingsRepository @Inject constructor( private fun publish(settings: UserSettings): UserSettings { cached.value = settings + trayLimitStore.publish(settings.notificationTrayLimit) return settings } } diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/SettingsBounds.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/SettingsBounds.kt index baeb714..937601f 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/SettingsBounds.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/SettingsBounds.kt @@ -1,5 +1,7 @@ package com.interlinedlist.android.feature.profile.domain +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore + /** * Bounds and server defaults for the numeric message preferences, plus the fallbacks * the Settings UI shows when `GET /api/user` omits a preference. @@ -36,6 +38,21 @@ object SettingsBounds { /** The page size a fresh account gets (observed live on a real account). */ const val DEFAULT_MESSAGES_PER_PAGE: Int = 20 + /** + * How many notifications the tray holds. The help centre publishes this range + * outright — `/help/settings`: "The default is 20 and you can set any value from + * 10 to 40", corroborated by `/help/api/notifications` ("clamped to 10-40"). + * + * The bounds are taken from [NotificationTrayLimitStore] rather than restated + * here, because that store is what `:feature:notifications` sizes its list and its + * system-tray group by: one source of truth, so Settings can never offer a value + * the readers would clamp away. + */ + val NOTIFICATION_TRAY_LIMIT: IntRange = NotificationTrayLimitStore.RANGE + + /** The tray limit a fresh account gets (help centre: "The default is 20"). */ + const val DEFAULT_NOTIFICATION_TRAY_LIMIT: Int = NotificationTrayLimitStore.DEFAULT + /** New messages start public unless the account says otherwise. */ const val DEFAULT_PUBLICLY_VISIBLE: Boolean = true @@ -75,3 +92,7 @@ val UserSettings.showAdvancedPostSettingsOrDefault: Boolean /** Whether the account is private, falling back to public when the API omits it. */ val UserSettings.isPrivateAccountOrDefault: Boolean get() = isPrivateAccount ?: SettingsBounds.DEFAULT_PRIVATE_ACCOUNT + +/** The notification tray limit to show, falling back to the server default. */ +val UserSettings.notificationTrayLimitOrDefault: Int + get() = notificationTrayLimit ?: SettingsBounds.DEFAULT_NOTIFICATION_TRAY_LIMIT diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt index bb9dae6..e89efd1 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt @@ -41,6 +41,7 @@ import com.interlinedlist.android.feature.profile.domain.defaultPubliclyVisibleO import com.interlinedlist.android.feature.profile.domain.isPrivateAccountOrDefault import com.interlinedlist.android.feature.profile.domain.maxMessageLengthOrDefault import com.interlinedlist.android.feature.profile.domain.messagesPerPageOrDefault +import com.interlinedlist.android.feature.profile.domain.notificationTrayLimitOrDefault import com.interlinedlist.android.feature.profile.domain.showAdvancedPostSettingsOrDefault /** Stable test tags for the Settings screen. */ @@ -57,6 +58,7 @@ object SettingsTestTags { const val SHOW_PREVIEWS = "settingsShowPreviews" const val MAX_MESSAGE_LENGTH = "settingsMaxMessageLength" const val MESSAGES_PER_PAGE = "settingsMessagesPerPage" + const val NOTIFICATION_TRAY_LIMIT = "settingsNotificationTrayLimit" const val DEFAULT_PUBLICLY_VISIBLE = "settingsDefaultPubliclyVisible" const val SHOW_ADVANCED_POST_SETTINGS = "settingsShowAdvancedPostSettings" const val PRIVATE_ACCOUNT = "settingsPrivateAccount" @@ -104,6 +106,7 @@ fun SettingsRoute( onSelectViewingPreference = viewModel::setViewingPreference, onToggleShowPreviews = viewModel::setShowPreviews, onSetMessagesPerPage = viewModel::setMessagesPerPage, + onSetNotificationTrayLimit = viewModel::setNotificationTrayLimit, onSetMaxMessageLength = viewModel::setMaxMessageLength, onToggleDefaultPubliclyVisible = viewModel::setDefaultPubliclyVisible, onToggleShowAdvancedPostSettings = viewModel::setShowAdvancedPostSettings, @@ -123,6 +126,7 @@ fun SettingsScreen( onSelectViewingPreference: (ViewingPreference) -> Unit, onToggleShowPreviews: (Boolean) -> Unit, onSetMessagesPerPage: (Int) -> Unit, + onSetNotificationTrayLimit: (Int) -> Unit, onSetMaxMessageLength: (Int) -> Unit, onToggleDefaultPubliclyVisible: (Boolean) -> Unit, onToggleShowAdvancedPostSettings: (Boolean) -> Unit, @@ -163,6 +167,7 @@ fun SettingsScreen( onSelectViewingPreference = onSelectViewingPreference, onToggleShowPreviews = onToggleShowPreviews, onSetMessagesPerPage = onSetMessagesPerPage, + onSetNotificationTrayLimit = onSetNotificationTrayLimit, ) MessageSettingsGroup( settings = settings, @@ -206,8 +211,14 @@ fun SettingsScreen( } /** - * "View preferences": which messages the Home feed shows, and whether link-preview - * cards render at all. + * "View preferences": which messages the Home feed shows, whether link-preview cards + * render at all, and how much the feed and the notification tray hold. + * + * The notification tray limit is filed here rather than under a notifications group + * because that is where the web keeps it: `/help/settings` lists it under **View + * preferences** alongside Messages per page, and the same page's Notifications section + * points back at it ("up to your Notification tray limit (see View Preferences + * above)"). */ @Composable private fun ViewPreferencesGroup( @@ -215,10 +226,11 @@ private fun ViewPreferencesGroup( onSelectViewingPreference: (ViewingPreference) -> Unit, onToggleShowPreviews: (Boolean) -> Unit, onSetMessagesPerPage: (Int) -> Unit, + onSetNotificationTrayLimit: (Int) -> Unit, ) { SettingsGroup( title = "View preferences", - description = "Control what appears in your Home feed.", + description = "Control what appears in your Home feed and your notification tray.", modifier = Modifier.testTag(SettingsTestTags.VIEW_PREFERENCES), ) { ViewingPreference.entries.forEach { option -> @@ -248,6 +260,16 @@ private fun ViewPreferencesGroup( onValueChange = onSetMessagesPerPage, tag = SettingsTestTags.MESSAGES_PER_PAGE, ) + HorizontalDivider(color = MaterialTheme.colorScheme.outlineVariant) + SettingsNumberRow( + label = "Notification tray limit", + description = "How many notifications the tray holds before older ones " + + "drop off (10 to 40).", + value = settings.notificationTrayLimitOrDefault, + range = SettingsBounds.NOTIFICATION_TRAY_LIMIT, + onValueChange = onSetNotificationTrayLimit, + tag = SettingsTestTags.NOTIFICATION_TRAY_LIMIT, + ) } } @@ -403,6 +425,7 @@ private fun SettingsScreenPreview() { maxMessageLength = 666, defaultPubliclyVisible = true, messagesPerPage = 20, + notificationTrayLimit = 20, viewingPreference = ViewingPreference.FOLLOWING, showPreviews = true, showAdvancedPostSettings = false, @@ -414,6 +437,7 @@ private fun SettingsScreenPreview() { onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, + onSetNotificationTrayLimit = {}, onSetMaxMessageLength = {}, onToggleDefaultPubliclyVisible = {}, onToggleShowAdvancedPostSettings = {}, diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt index 5fce0c2..4fa483c 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt @@ -12,6 +12,7 @@ import com.interlinedlist.android.feature.profile.domain.defaultPubliclyVisibleO import com.interlinedlist.android.feature.profile.domain.isPrivateAccountOrDefault import com.interlinedlist.android.feature.profile.domain.maxMessageLengthOrDefault import com.interlinedlist.android.feature.profile.domain.messagesPerPageOrDefault +import com.interlinedlist.android.feature.profile.domain.notificationTrayLimitOrDefault import com.interlinedlist.android.feature.profile.domain.showAdvancedPostSettingsOrDefault import com.interlinedlist.android.feature.profile.ui.common.toUserMessage import dagger.hilt.android.lifecycle.HiltViewModel @@ -170,6 +171,34 @@ class SettingsViewModel @Inject constructor( ) } + /** + * Sets how many notifications the tray holds before older ones drop off. The help + * centre documents the supported range as 10 to 40 + * ([SettingsBounds.NOTIFICATION_TRAY_LIMIT]); anything else is refused without a + * request. + * + * The saved value is not cosmetic: `:feature:notifications` sizes both its list + * and its system-tray group by it, so a rejected save must roll back rather than + * leave the two disagreeing about how much the tray holds. + */ + fun setNotificationTrayLimit(notifications: Int) { + val current = _uiState.value.settings ?: return + if (!withinRange( + notifications, + SettingsBounds.NOTIFICATION_TRAY_LIMIT, + "Notification tray limit", + ) + ) { + return + } + if (current.notificationTrayLimitOrDefault == notifications) return + save( + optimistic = current.copy(notificationTrayLimit = notifications), + previous = current, + update = UserSettingsUpdate(notificationTrayLimit = notifications), + ) + } + fun dismissError() = _uiState.update { it.copy(errorMessage = null) } /** diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt index 238a4fe..41b237d 100644 --- a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt @@ -4,6 +4,8 @@ import com.google.common.truth.Truth.assertThat import com.interlinedlist.android.core.common.dispatcher.DispatcherProvider import com.interlinedlist.android.core.common.result.ApiResult import com.interlinedlist.android.core.common.result.AppError +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.profile.data.remote.ProfileApi import com.interlinedlist.android.feature.profile.domain.UserSettingsUpdate import com.interlinedlist.android.feature.profile.domain.ViewingPreference @@ -33,6 +35,7 @@ class DefaultSettingsRepositoryTest { private lateinit var server: MockWebServer private lateinit var api: ProfileApi private lateinit var repository: DefaultSettingsRepository + private lateinit var trayLimitStore: NotificationTrayLimitStore private val json = Json { ignoreUnknownKeys = true; explicitNulls = false } private val testDispatcher = StandardTestDispatcher() @@ -52,7 +55,9 @@ class DefaultSettingsRepositoryTest { .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) .build() api = retrofit.create(ProfileApi::class.java) - repository = DefaultSettingsRepository(api, json, dispatchers) + trayLimitStore = + NotificationTrayLimitStore(retrofit.create(InterlinedListApi::class.java), json) + repository = DefaultSettingsRepository(api, trayLimitStore, json, dispatchers) } @After @@ -66,6 +71,7 @@ class DefaultSettingsRepositoryTest { messagesPerPage: Int = 20, showAdvancedPostSettings: Boolean = false, isPrivateAccount: Boolean = false, + notificationTrayLimit: Int = 25, ) = server.enqueue( MockResponse().setResponseCode(200).setBody( """ @@ -85,7 +91,7 @@ class DefaultSettingsRepositoryTest { "longitude": -122.68, "isPrivateAccount": $isPrivateAccount, "githubDefaultRepo": "adron/notes", - "notificationTrayLimit": 25 + "notificationTrayLimit": $notificationTrayLimit } } """.trimIndent(), @@ -346,4 +352,57 @@ class DefaultSettingsRepositoryTest { assertThat(result).isInstanceOf(ApiResult.Failure::class.java) assertThat(repository.observeSettings().first()?.isPrivateAccount).isFalse() } + + // --- Notification tray limit (issue #35) --------------------------------- + // The one preference a *different* feature module reads, so it has to reach the + // shared :core:network accessor as well as the settings cache. + + @Test + fun `notificationTrayLimit PATCHes alone as a JSON number`() = runTest(testDispatcher) { + enqueueUser(notificationTrayLimit = 40) + + val result = repository.update(UserSettingsUpdate(notificationTrayLimit = 40)) + + val body = server.takeJsonBody() + assertThat(body.keys).containsExactly("notificationTrayLimit") + val sent = body.getValue("notificationTrayLimit").jsonPrimitive + assertThat(sent.isString).isFalse() + assertThat(sent.intOrNull).isEqualTo(40) + assertThat((result as ApiResult.Success).data.notificationTrayLimit).isEqualTo(40) + assertThat(repository.observeSettings().first()?.notificationTrayLimit).isEqualTo(40) + } + + @Test + fun `a saved tray limit reaches the shared store the notifications feature reads`() = + runTest(testDispatcher) { + enqueueUser(notificationTrayLimit = 40) + + repository.update(UserSettingsUpdate(notificationTrayLimit = 40)) + + // No further response is queued: the store answers from what was published, + // so :feature:notifications sees the change without another GET /api/user. + assertThat(trayLimitStore.current()).isEqualTo(40) + } + + @Test + fun `a refresh republishes the tray limit to the shared store`() = runTest(testDispatcher) { + enqueueUser(notificationTrayLimit = 10) + + repository.refresh() + + assertThat(trayLimitStore.current()).isEqualTo(10) + } + + @Test + fun `an account without a stored tray limit publishes the documented default`() = + runTest(testDispatcher) { + server.enqueue( + MockResponse().setResponseCode(200) + .setBody("""{ "user": { "id": "u1", "username": "adron" } }"""), + ) + + repository.refresh() + + assertThat(trayLimitStore.current()).isEqualTo(20) + } } diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt index 64ed648..edd4003 100644 --- a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt @@ -50,6 +50,8 @@ class FakeSettingsRepository : SettingsRepository { showAdvancedPostSettings = update.showAdvancedPostSettings ?: current.showAdvancedPostSettings, isPrivateAccount = update.isPrivateAccount ?: current.isPrivateAccount, + notificationTrayLimit = + update.notificationTrayLimit ?: current.notificationTrayLimit, ) } } diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsNotificationTrayLimitTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsNotificationTrayLimitTest.kt new file mode 100644 index 0000000..e099138 --- /dev/null +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsNotificationTrayLimitTest.kt @@ -0,0 +1,178 @@ +package com.interlinedlist.android.feature.profile.ui + +import app.cash.turbine.test +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.common.result.AppError +import com.interlinedlist.android.feature.profile.domain.SettingsBounds +import com.interlinedlist.android.feature.profile.domain.UserSettings +import com.interlinedlist.android.feature.profile.ui.settings.SettingsViewModel +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test + +/** + * Issue #35: the notification tray limit, saved from Settings. + * + * The help centre publishes the range outright — `/help/settings`: "Notification tray + * limit: How many notifications the bell tray holds before older ones drop off. The + * default is 20 and you can set any value from 10 to 40" — and `/help/api/notifications` + * corroborates it ("default 20, clamped to 10-40"), so out-of-range input is refused + * here rather than spent on a request the server would reject. + * + * Like every other preference it PATCHes alone, applies optimistically and rolls back + * when the save fails — which matters more here than for a cosmetic toggle, because + * `:feature:notifications` sizes both its list and its system-tray group by the value. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class SettingsNotificationTrayLimitTest { + + private val dispatcher = StandardTestDispatcher() + private lateinit var repo: FakeSettingsRepository + + @Before + fun setUp() { + Dispatchers.setMain(dispatcher) + repo = FakeSettingsRepository() + } + + @After + fun tearDown() = Dispatchers.resetMain() + + private fun loadedViewModel(settings: UserSettings): SettingsViewModel { + repo.refreshResult = ApiResult.Success(settings) + return SettingsViewModel(repo) + } + + private val serverErrorMessage = "InterlinedList is having trouble right now. Try again shortly." + + private val outOfRangeMessage = + "Notification tray limit must be between " + + "${SettingsBounds.NOTIFICATION_TRAY_LIMIT.first} and " + + "${SettingsBounds.NOTIFICATION_TRAY_LIMIT.last}." + + @Test + fun `setting the tray limit PATCHes only that field`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 20)) + + vm.uiState.test { + advanceUntilIdle() + vm.setNotificationTrayLimit(40) + + // Applied optimistically, before the request comes back. + assertThat(vm.uiState.value.settings?.notificationTrayLimit).isEqualTo(40) + assertThat(vm.uiState.value.isSaving).isTrue() + advanceUntilIdle() + + val sent = repo.updates.single() + assertThat(sent.notificationTrayLimit).isEqualTo(40) + assertThat(sent.touchedFieldNames()).containsExactly("notificationTrayLimit") + val state = expectMostRecentItem() + assertThat(state.settings?.notificationTrayLimit).isEqualTo(40) + assertThat(state.isSaving).isFalse() + assertThat(state.errorMessage).isNull() + } + } + + @Test + fun `a limit above the documented maximum is refused without a request`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 20)) + + vm.uiState.test { + advanceUntilIdle() + vm.setNotificationTrayLimit(SettingsBounds.NOTIFICATION_TRAY_LIMIT.last + 1) + advanceUntilIdle() + + assertThat(repo.updates).isEmpty() + val state = expectMostRecentItem() + assertThat(state.settings?.notificationTrayLimit).isEqualTo(20) + assertThat(state.isSaving).isFalse() + assertThat(state.errorMessage).isEqualTo(outOfRangeMessage) + } + } + + @Test + fun `a limit below the documented minimum is refused without a request`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 20)) + + vm.uiState.test { + advanceUntilIdle() + vm.setNotificationTrayLimit(SettingsBounds.NOTIFICATION_TRAY_LIMIT.first - 1) + advanceUntilIdle() + + assertThat(repo.updates).isEmpty() + val state = expectMostRecentItem() + assertThat(state.settings?.notificationTrayLimit).isEqualTo(20) + assertThat(state.errorMessage).isEqualTo(outOfRangeMessage) + } + } + + @Test + fun `both ends of the documented range are accepted`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 20)) + advanceUntilIdle() + + vm.setNotificationTrayLimit(SettingsBounds.NOTIFICATION_TRAY_LIMIT.first) + advanceUntilIdle() + vm.setNotificationTrayLimit(SettingsBounds.NOTIFICATION_TRAY_LIMIT.last) + advanceUntilIdle() + + assertThat(repo.updates.map { it.notificationTrayLimit }).containsExactly(10, 40).inOrder() + assertThat(vm.uiState.value.errorMessage).isNull() + } + + @Test + fun `a failed save rolls back to the stored limit and surfaces the error`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 20)) + repo.updateResult = { ApiResult.Failure(AppError.Server("boom")) } + + vm.uiState.test { + advanceUntilIdle() + vm.setNotificationTrayLimit(40) + assertThat(vm.uiState.value.settings?.notificationTrayLimit).isEqualTo(40) + advanceUntilIdle() + + val state = expectMostRecentItem() + assertThat(state.settings?.notificationTrayLimit).isEqualTo(20) + assertThat(state.isSaving).isFalse() + assertThat(state.errorMessage).isEqualTo(serverErrorMessage) + } + } + + @Test + fun `re-entering the current limit does not call the API`() = runTest(dispatcher) { + val vm = loadedViewModel(UserSettings(notificationTrayLimit = 30)) + advanceUntilIdle() + + vm.setNotificationTrayLimit(30) + advanceUntilIdle() + + assertThat(repo.updates).isEmpty() + } + + @Test + fun `entering the server default when the account has no stored limit does not call the API`() = + runTest(dispatcher) { + // The row shows 20 for a null stored value, so "setting" 20 changes nothing. + val vm = loadedViewModel(UserSettings(notificationTrayLimit = null)) + advanceUntilIdle() + + vm.setNotificationTrayLimit(SettingsBounds.DEFAULT_NOTIFICATION_TRAY_LIMIT) + advanceUntilIdle() + + assertThat(repo.updates).isEmpty() + } + + @Test + fun `the documented bounds match the help centre`() { + assertThat(SettingsBounds.NOTIFICATION_TRAY_LIMIT).isEqualTo(10..40) + assertThat(SettingsBounds.DEFAULT_NOTIFICATION_TRAY_LIMIT).isEqualTo(20) + } +}