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 328685638d..3b4d3f4233 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 @@ -172,6 +172,11 @@ class OfflineFirstConversationsRepository @Inject constructor( val includeStatus = isUserStatusAvailable(user) try { + // Taken before the request: the response describes the server as it was at this moment, + // so a conversation that arrives locally while the request is in flight is not one the + // response can be read as having dropped. + val knownBeforeRequest = dao.getConversationIdsForUser(user.id!!).toSet() + val conversationsList = withRetry( retries = NETWORK_FETCH_RETRIES, initialDelayMillis = NETWORK_FETCH_RETRY_INITIAL_DELAY_MS, @@ -197,6 +202,7 @@ class OfflineFirstConversationsRepository @Inject constructor( conversationsFromSync ), conversationIdsToDelete = determineLeftConversationIds(previousConversations, conversationsFromSync) + .filter { it in knownBeforeRequest } ) val roomsWithNewMessages = getRoomsWithNewMessages(conversationsFromSync, previousConversations) diff --git a/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt b/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt index 1008111556..e590e5a2f4 100644 --- a/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt +++ b/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt @@ -25,6 +25,10 @@ interface ConversationsDao { @Query("SELECT * FROM Conversations where accountId = :accountId AND token = :token") fun getConversationForUser(accountId: Long, token: String): Flow + /** The internal ids of [accountId]'s conversations, without loading the conversations themselves. */ + @Query("SELECT internalId FROM Conversations where accountId = :accountId") + suspend fun getConversationIdsForUser(accountId: Long): List + /** * Applies a full room list sync atomically: left conversations are deleted and the server * items are upserted in one transaction, so observers of the conversations table see a single diff --git a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt index 6e3ad48553..6cb9a72983 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt @@ -258,6 +258,8 @@ class DummyUserDaoImpl : UsersDao() { } class DummyConversationDaoImpl : ConversationsDao { + override suspend fun getConversationIdsForUser(accountId: Long): List = emptyList() + override fun getConversationsForUser(accountId: Long): Flow> = flowOf() override fun getConversationForUser(accountId: Long, token: String): Flow = flowOf() diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt index cb413f7d31..e3202b87c6 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/OfflineFirstConversationsRepositoryTest.kt @@ -92,7 +92,7 @@ class OfflineFirstConversationsRepositoryTest { whenever(connectivityManager.restrictBackgroundStatus) .thenReturn(ConnectivityManager.RESTRICT_BACKGROUND_STATUS_DISABLED) - whenever(dao.getConversationsForUser(ACCOUNT_ID)).thenReturn(flowOf(emptyList())) + stubStoredConversations() whenever(conversationListUpdater.preservePendingLocalState(any(), any())) .thenAnswer { invocation -> invocation.getArgument>(1) } @@ -140,7 +140,7 @@ class OfflineFirstConversationsRepositoryTest { fun `getRooms skips deleting local conversations when the server returns an empty list`() = runBlocking { val previous = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) - whenever(dao.getConversationsForUser(ACCOUNT_ID)).thenReturn(flowOf(listOf(previous))) + stubStoredConversations(previous) whenever(network.getRooms(any(), any(), any())).thenReturn(Observable.just(emptyList())) repository.getRooms(user()).join() @@ -155,7 +155,7 @@ class OfflineFirstConversationsRepositoryTest { runBlocking { val staying = conversation(token = "roomA", lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) val leaving = conversation(token = "roomB", lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) - whenever(dao.getConversationsForUser(ACCOUNT_ID)).thenReturn(flowOf(listOf(staying, leaving))) + stubStoredConversations(staying, leaving) val stayingRoom = conversation(token = "roomA", lastActivity = 5, unreadMessages = 0) whenever(network.getRooms(any(), any(), any())).thenReturn(Observable.just(listOf(stayingRoom))) @@ -170,7 +170,7 @@ class OfflineFirstConversationsRepositoryTest { fun `getRooms merges the server response through the conversation list updater`() = runBlocking { val previous = conversation(token = ROOM_TOKEN, lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) - whenever(dao.getConversationsForUser(ACCOUNT_ID)).thenReturn(flowOf(listOf(previous))) + stubStoredConversations(previous) val serverRoom = conversation(token = ROOM_TOKEN, lastActivity = 6, unreadMessages = 1) whenever(network.getRooms(any(), any(), any())).thenReturn(Observable.just(listOf(serverRoom))) @@ -250,8 +250,7 @@ class OfflineFirstConversationsRepositoryTest { conversation(token = "unchanged", lastActivity = 10, unreadMessages = 0).asEntity(ACCOUNT_ID) val previousNoBlock = conversation(token = "noBlockUnread", lastActivity = 10, unreadMessages = 3).asEntity(ACCOUNT_ID) - whenever(dao.getConversationsForUser(ACCOUNT_ID)) - .thenReturn(flowOf(listOf(previousUnchanged, previousNoBlock))) + stubStoredConversations(previousUnchanged, previousNoBlock) val unchangedRoom = conversation(token = "unchanged", lastActivity = 10, unreadMessages = 0) val noBlockRoom = conversation(token = "noBlockUnread", lastActivity = 10, unreadMessages = 3) @@ -442,6 +441,33 @@ class OfflineFirstConversationsRepositoryTest { lastReadMessage = lastReadMessage ) + @Test + fun `a conversation that arrives while the request is in flight is not treated as left`() = + runBlocking { + val known = conversation(token = "known", lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) + val arrivedDuringRequest = + conversation(token = "arrived", lastActivity = 5, unreadMessages = 0).asEntity(ACCOUNT_ID) + // the ids were taken before the request; the rows are read after it, by which time the + // second conversation had been written by something else + wheneverBlocking { dao.getConversationIdsForUser(ACCOUNT_ID) }.thenReturn(listOf(known.internalId)) + whenever(dao.getConversationsForUser(ACCOUNT_ID)) + .thenReturn(flowOf(listOf(known, arrivedDuringRequest))) + whenever(network.getRooms(any(), any(), any())).thenReturn(Observable.just(listOf())) + + repository.getRooms(user()).join() + + verifyBlocking(dao) { syncConversationsForUser(eq(ACCOUNT_ID), any(), eq(listOf(known.internalId))) } + } + + /** + * Stubs both reads the sync makes of the conversations table: the rows it merges against, and + * the ids it takes before the request to know which conversations the response can speak about. + */ + private fun stubStoredConversations(vararg stored: ConversationEntity) { + whenever(dao.getConversationsForUser(ACCOUNT_ID)).thenReturn(flowOf(stored.toList())) + wheneverBlocking { dao.getConversationIdsForUser(ACCOUNT_ID) }.thenReturn(stored.map { it.internalId }) + } + companion object { private const val ACCOUNT_ID = 1L private const val BASE_URL = "https://server.example.com"