diff --git a/AGENTS.md b/AGENTS.md index 0b88963d90..cfa2bca394 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -194,7 +194,7 @@ After finishing code changes, run `./gradlew detekt ktlintCheck` and fix any new ## Static Analysis -- **detekt**: config in `detekt.yml` (maxIssues: 80) +- **detekt**: config in `detekt.yml` - **ktlint**: via `org.jlleitschuh.gradle.ktlint` plugin - **SpotBugs**: filter in `spotbugs-filter.xml`; FindSecBugs and fb-contrib active - **lint**: HTML report at `app/build/reports/lint/lint.html` @@ -204,6 +204,8 @@ After finishing code changes, run `./gradlew detekt ktlintCheck` and fix any new - **Unit tests**: `app/src/test/` — JUnit 4/5, Mockito, Robolectric, MockWebServer. Uses `useJUnitPlatform()`. - **Instrumented tests**: `app/src/androidTest/` — Espresso. Integration tests need real server credentials in `gradle.properties` (`NC_TEST_SERVER_BASEURL`, etc.). - **Room migrations**: if you change the schema, add or update migration tests under `androidTest/data/`. See `data/source/local/TalkDatabase.kt` for migration declarations. +- **A new test must fail without its fix**: a passing test is not evidence that it reached the code it names. Revert the change under test, confirm the test fails, then restore it. Tests have been merged here asserting behaviour they never exercised, because an earlier guard returned before the code under test ran — and running the suite cannot tell you that. +- **Assertions come from the requirement, fixtures come from the code**: derive what the behaviour *should* be from the spec, but derive the setup from the code path, since only the code says which preconditions reach the branch. A fixture written from the description alone often puts the system in a state where the behaviour under test cannot occur at all. - **App startup workers**: `NextcloudTalkApplication.kt` schedules periodic workers (`CapabilitiesWorker`, signaling/WebSocket workers) at startup. Worker scheduling changes can cause subtle startup regressions. ## Commits diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt index bb3bc03ad6..5a9b74eeb4 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncer.kt @@ -21,7 +21,11 @@ import com.nextcloud.talk.data.database.model.ChatMessageEntity import com.nextcloud.talk.data.network.NetworkMonitor import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.models.json.chat.ChatMessageDto +import com.nextcloud.talk.utils.CapabilitiesUtil import com.nextcloud.talk.utils.SpreedFeatures +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicBoolean +import javax.inject.Inject import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow @@ -30,9 +34,6 @@ import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.sync.Mutex import retrofit2.HttpException -import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.atomic.AtomicBoolean -import javax.inject.Inject /** * The chat message fetch-and-persist core, shared between the chat screen @@ -132,10 +133,15 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, threadId: Long? = null, lastCommonRead: Int? = null, - markNotificationsAsRead: Boolean = true + markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false ): HashMap { val fieldMap = HashMap() + if (prefetch) { + fieldMap["prefetch"] = 1 + } + fieldMap["includeLastKnown"] = if (includeLastKnown) 1 else 0 if (lastKnown != null) { @@ -225,7 +231,8 @@ class ChatMessageSyncer @Inject constructor( * * Requires the chat-keep-notifications capability: without it, a background fetch would * dismiss the user's push notifications for the fetched messages, so the catch-up is skipped - * entirely (same guard as on iOS). + * entirely (same guard as on iOS). It is also skipped when the server turns preloading off + * for mobile clients. * * Reachability is deliberately not pre-checked. Callers are gated by a WorkManager * `NetworkType.CONNECTED` constraint, and a request that fails because the device is offline @@ -241,18 +248,24 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, lastReadMessage: Int? = null, unreadMessages: Int = 0 - ): SyncOutcome { - if (!target.user.hasSpreedFeatureCapability(SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value)) { - Log.d( - TAG, - "Server lacks ${SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value}, " + - "skipping catch-up for ${target.internalConversationId}" - ) - return NOTHING_SYNCED - } + ): SyncOutcome = + when { + !target.user.hasSpreedFeatureCapability(SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value) -> { + Log.d( + TAG, + "Server lacks ${SpreedFeatures.CHAT_KEEP_NOTIFICATIONS.value}, " + + "skipping catch-up for ${target.internalConversationId}" + ) + NOTHING_SYNCED + } - return coalescedRoomCatchUp(target, limit, lastReadMessage, unreadMessages) - } + !CapabilitiesUtil.isChatPreloadAllowed(target.user.capabilities?.spreedCapability) -> { + Log.d(TAG, "Server turned off preloading, skipping catch-up for ${target.internalConversationId}") + NOTHING_SYNCED + } + + else -> coalescedRoomCatchUp(target, limit, lastReadMessage, unreadMessages) + } /** * Runs at most one catch-up per room at a time. A request arriving while one is running only @@ -350,7 +363,8 @@ class ChatMessageSyncer @Inject constructor( target = target, fromMessageId = newestMessageIdFromDb, limit = limit, - markNotificationsAsRead = false + markNotificationsAsRead = false, + prefetch = true ) } else { initialCatchUp( @@ -358,7 +372,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, lastReadMessage = lastReadMessage, unreadMessages = unreadMessages, - markNotificationsAsRead = false + markNotificationsAsRead = false, + prefetch = true ) } @@ -399,6 +414,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages: Int = 0, lastCommonRead: Int? = null, markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false, events: Events = NO_EVENTS ): SyncOutcome { val closableBacklogAnchor = lastReadMessage?.takeIf { @@ -413,6 +429,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages = unreadMessages, lastCommonRead = lastCommonRead, markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch, events = events ) } else { @@ -426,7 +443,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) @@ -445,6 +463,7 @@ class ChatMessageSyncer @Inject constructor( unreadMessages: Int, lastCommonRead: Int?, markNotificationsAsRead: Boolean, + prefetch: Boolean, events: Events ): SyncOutcome { Log.d( @@ -462,7 +481,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) @@ -478,6 +498,7 @@ class ChatMessageSyncer @Inject constructor( limit = limit, lastCommonRead = lastCommonRead, markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch, events = events ) return SyncOutcome( @@ -510,6 +531,7 @@ class ChatMessageSyncer @Inject constructor( limit: Int = DEFAULT_MESSAGES_LIMIT, lastCommonRead: Int? = null, markNotificationsAsRead: Boolean = true, + prefetch: Boolean = false, events: Events = NO_EVENTS ): SyncOutcome { var anchor = fromMessageId @@ -527,7 +549,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ) val roundOutcome = pullAndPersistMessages(target, fieldMap, events) @@ -569,7 +592,8 @@ class ChatMessageSyncer @Inject constructor( limit = limit, threadId = target.threadId, lastCommonRead = lastCommonRead, - markNotificationsAsRead = markNotificationsAsRead + markNotificationsAsRead = markNotificationsAsRead, + prefetch = prefetch ), events ) diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt index dd47778279..b919a17190 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepository.kt @@ -26,6 +26,7 @@ import com.nextcloud.talk.extensions.isPowerSaveMode import com.nextcloud.talk.logger.Logger import com.nextcloud.talk.models.domain.ConversationModel import com.nextcloud.talk.utils.ApiUtils +import com.nextcloud.talk.utils.CapabilitiesUtil import com.nextcloud.talk.utils.CapabilitiesUtil.isUserStatusAvailable import com.nextcloud.talk.utils.SpreedFeatures import com.nextcloud.talk.utils.withRetry @@ -431,6 +432,11 @@ class OfflineFirstConversationsRepository @Inject constructor( false } + !CapabilitiesUtil.isChatPreloadAllowed(user.capabilities?.spreedCapability) -> { + Log.d(TAG, "Server turned off preloading, skipping message catch-up") + false + } + context.isPowerSaveMode() -> { Log.d(TAG, "Battery saver is active, skipping message catch-up") false diff --git a/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt b/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt index e013e828d2..628e4fa940 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/CapabilitiesUtil.kt @@ -301,6 +301,17 @@ object CapabilitiesUtil { return false } + /** + * Whether the server lets mobile clients preload chat messages before the user opens a + * conversation. + * + * True unless the server says `mobile-preload-chat` is false, so a server that does not know + * the setting, does not report it, or has not been asked for its capabilities yet, allows + * preloading. + */ + fun isChatPreloadAllowed(spreedCapabilities: SpreedCapabilityDto?): Boolean = + spreedCapabilities?.config?.get("chat")?.get(MOBILE_PRELOAD_CHAT)?.toString()?.lowercase() != "false" + fun isTranslationsSupported(spreedCapabilities: SpreedCapabilityDto): Boolean = spreedCapabilities.config?.containsKey("chat") == true && spreedCapabilities.config!!["chat"] != null && @@ -418,4 +429,5 @@ object CapabilitiesUtil { private const val SERVER_VERSION_MIN_SUPPORTED = 17 private const val SERVER_VERSION_SUPPORT_WARNING = 26 private const val CONVERSATION_DESCRIPTION_LENGTH_FOR_OLD_SERVER = 500 + private const val MOBILE_PRELOAD_CHAT = "mobile-preload-chat" } diff --git a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt index 4503d582f9..7df743cdcc 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/data/network/ChatMessageSyncerTest.kt @@ -21,6 +21,8 @@ import com.nextcloud.talk.models.json.capabilities.SpreedCapabilityDto import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.models.json.chat.ChatOCS import com.nextcloud.talk.models.json.chat.ChatOverall +import com.nextcloud.talk.utils.CapabilitiesUtil +import java.io.IOException import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf @@ -48,7 +50,6 @@ import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever import org.mockito.kotlin.wheneverBlocking import retrofit2.Response -import java.io.IOException @Suppress("TooManyFunctions", "LargeClass") class ChatMessageSyncerTest { @@ -95,11 +96,109 @@ class ChatMessageSyncerTest { ) assertFalse(fieldMap.containsKey("markNotificationsAsRead")) + assertFalse(fieldMap.containsKey("prefetch")) assertFalse(fieldMap.containsKey("lastKnownMessageId")) assertEquals(0, fieldMap["setReadMarker"]) assertEquals(1, fieldMap["includeLastKnown"]) } + @Test + fun `buildFieldMap marks the request as a prefetch when asked`() { + val fieldMap = syncer.buildFieldMap( + lookIntoFuture = true, + timeout = 0, + includeLastKnown = false, + lastKnown = 42, + prefetch = true + ) + + assertEquals(1, fieldMap["prefetch"]) + } + + @Test + fun `catchUpRoom marks its request as a prefetch`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + syncer.catchUpRoom(target()) + + val fieldMapCaptor = argumentCaptor>() + verifyBlocking(network) { pullChatMessages(eq(CREDENTIALS), eq(CHAT_URL), fieldMapCaptor.capture()) } + assertEquals(1, fieldMapCaptor.firstValue["prefetch"]) + } + + @Test + fun `a fetch for a chat the user is reading is not marked as a prefetch`() = + runTest { + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + syncer.tryCloseBacklog(target(), fromMessageId = 42L) + + val fieldMapCaptor = argumentCaptor>() + verifyBlocking(network) { pullChatMessages(eq(CREDENTIALS), eq(CHAT_URL), fieldMapCaptor.capture()) } + assertFalse(fieldMapCaptor.firstValue.containsKey("prefetch")) + } + + @Test + fun `catchUpRoom skips when the server turned preloading off`() = + runTest { + val outcome = syncer.catchUpRoom(target(user(preloadChat = false))) + + assertFalse(outcome.persistedNewMessages) + verifyNoInteractions(network) + } + + @Test + fun `catchUpRoom preloads when the server does not report the setting`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + val outcome = syncer.catchUpRoom(target(user(preloadChat = null))) + + assertTrue(outcome.persistedNewMessages) + } + + /** + * Only an explicit false turns preloading off, so a server that says nothing about it allows + * it. Asserted on the capability itself rather than through [ChatMessageSyncer.catchUpRoom], + * which skips a user with no capabilities at the earlier `chat-keep-notifications` guard and + * so never reaches this one. + */ + @Test + fun `a server that says nothing about preloading still allows it`() { + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(null)) + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(SpreedCapabilityDto())) + assertTrue(CapabilitiesUtil.isChatPreloadAllowed(SpreedCapabilityDto().apply { config = hashMapOf() })) + } + + @Test + fun `catchUpRoom preloads when the server allows it explicitly`() = + runTest { + whenever(chatBlocksDao.getNewestMessageIdFromChatBlocks(INTERNAL_CONVERSATION_ID, null)) + .thenReturn(42L) + whenever(chatBlocksDao.getChatBlocksContainingMessageId(INTERNAL_CONVERSATION_ID, null, 42L)) + .thenReturn(flowOf(listOf(block(oldest = 10, newest = 42)))) + wheneverBlocking { network.pullChatMessages(any(), any(), any()) } + .thenReturn(Response.success(overall(message(43)))) + + val outcome = syncer.catchUpRoom(target(user(preloadChat = true))) + + assertTrue(outcome.persistedNewMessages) + } + @Test fun `catchUpRoom skips without chat-keep-notifications capability`() = runTest { @@ -778,19 +877,23 @@ class ChatMessageSyncerTest { verifyNoInteractions(chatBlocksDao) } - private fun user(withKeepNotificationsCapability: Boolean = true): User { + private fun user(withKeepNotificationsCapability: Boolean = true, preloadChat: Boolean? = null): User { val features = if (withKeepNotificationsCapability) { listOf("chat-keep-notifications") } else { emptyList() } + val chatConfig = preloadChat?.let { hashMapOf("mobile-preload-chat" to it) } return User( id = ACCOUNT_ID, userId = "me", username = "me", baseUrl = "https://server.example.com", capabilities = CapabilitiesDto().apply { - spreedCapability = SpreedCapabilityDto().apply { this.features = features } + spreedCapability = SpreedCapabilityDto().apply { + this.features = features + this.config = chatConfig?.let { hashMapOf("chat" to it) } + } } ) }