diff --git a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt index 25632759e3..3fe0b2b732 100644 --- a/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt +++ b/apps/flipcash/features/messenger/src/main/kotlin/com/flipcash/app/messenger/internal/ChatViewModel.kt @@ -74,6 +74,7 @@ import com.flipcash.shared.chat.ActiveTypist import com.flipcash.shared.chat.ChatCoordinator import com.flipcash.shared.chat.ChatDraftSnapshot import com.flipcash.shared.chat.ChatDraftStore +import com.flipcash.shared.chat.ChatHydration import com.flipcash.shared.chat.ChatMembership import com.flipcash.shared.chat.GroupAccess import com.flipcash.shared.chat.MessageCapability @@ -1367,8 +1368,12 @@ internal class ChatViewModel @Inject constructor( * and the roster stream all only ever carry chats the viewer is already in — so without the * hydration the screen has no title, no info card and no gate, and nothing that would later * supply them. A chat the device already holds costs no round trip: [ChatCoordinator.hydrateChat] - * answers null for it and the Room observation stays the only source, so opening a synced chat - * behaves exactly as it did. + * answers [ChatHydration.Stored] for it and the Room observation stays the only source, so + * opening a synced chat behaves exactly as it did. + * + * A chat the server has no record of is left unloaded. A DM opened on its derived id before + * anyone has written in it — from a scanned tip card or a profile — has no messages, and asking + * for them comes back DENIED. The first tip loads the chat once it exists. * * The transcript is fetched only for a viewer the gate lets through. A withheld group is drawn * as a placeholder rather than fetched, so asking for its messages would pull the very ones the @@ -1379,17 +1384,19 @@ internal class ChatViewModel @Inject constructor( * decides it has not arrived yet. */ private suspend fun openTranscript(chatId: ChatId) { - val hydrated = chatCoordinator.hydrateChat(chatId) ?: run { - chatCoordinator.loadMessages(chatId) - return - } - - if (hydrated.metadata.type != ChatType.GROUP) { - chatCoordinator.loadMessages(chatId) - return + when (val hydration = chatCoordinator.hydrateChat(chatId)) { + ChatHydration.Stored, + ChatHydration.Unavailable -> chatCoordinator.loadMessages(chatId) + ChatHydration.Absent -> Unit + is ChatHydration.Fetched -> { + val membership = hydration.membership + if (membership.metadata.type == ChatType.GROUP) { + dispatchEvent(Event.OnGroupResolved(membership)) + } else { + chatCoordinator.loadMessages(chatId) + } + } } - - dispatchEvent(Event.OnGroupResolved(hydrated)) } private fun initChatHandlers() { diff --git a/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatOpenTranscriptTest.kt b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatOpenTranscriptTest.kt new file mode 100644 index 0000000000..7ecf0da66e --- /dev/null +++ b/apps/flipcash/features/messenger/src/test/kotlin/com/flipcash/app/messenger/internal/ChatOpenTranscriptTest.kt @@ -0,0 +1,155 @@ +package com.flipcash.app.messenger.internal + +import android.content.ClipboardManager +import androidx.arch.core.executor.testing.InstantTaskExecutorRule +import com.flipcash.app.analytics.RecordingAnalytics +import com.flipcash.app.core.MainCoroutineRule +import com.flipcash.app.core.chat.ChatIdentifier +import com.flipcash.libs.coroutines.TestDispatcherProvider +import com.flipcash.services.models.UserProfile +import com.flipcash.services.models.chat.ChatId +import com.flipcash.services.models.chat.ChatMetadata +import com.flipcash.services.models.chat.ChatType +import com.flipcash.services.user.UserManager +import com.flipcash.shared.chat.ChatCoordinator +import com.flipcash.shared.chat.ChatHydration +import com.flipcash.shared.chat.ChatMembership +import com.flipcash.shared.payments.TipPaymentDelegate +import com.getcode.manager.BottomBarManager +import com.getcode.opencode.controllers.TransactionController +import com.getcode.opencode.exchange.Exchange +import com.getcode.opencode.model.accounts.AccountCluster +import com.getcode.opencode.model.core.bytes +import com.getcode.opencode.model.financial.Rate +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flowOf +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import java.util.UUID + +/** + * Which chat opens fetch a transcript. A DM opened on its derived id before either side has written + * in it does not exist on the server, and asking for its messages comes back DENIED, so that open + * must not ask. Every other outcome still loads as it did. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class ChatOpenTranscriptTest { + + @get:Rule + var instantExecutorRule = InstantTaskExecutorRule() + + @get:Rule + var mainCoroutineRule = MainCoroutineRule(UnconfinedTestDispatcher()) + + private val chatCoordinator = mockk(relaxed = true) + private val tipPaymentDelegate = mockk(relaxed = true) + private val transactionController = mockk(relaxed = true) + private val exchange = mockk(relaxed = true) + private val userManager = mockk(relaxed = true) + + private val chatId = ChatId(UUID.randomUUID().bytes) + private val userId = UUID.randomUUID().bytes + private val openByUser = ChatViewModel.Event.OnChatOpened( + ChatIdentifier.ByUser(userId = userId, profile = mockk(relaxed = true)), + ) + + @Before + fun setUp() { + BottomBarManager.clear() + + every { userManager.accountCluster } returns mockk(relaxed = true) + every { exchange.preferredRate } returns Rate.oneToOne + every { transactionController.limits } returns MutableStateFlow(null) + every { tipPaymentDelegate.minimumToOpenDmWith(any()) } returns flowOf(null) + coEvery { chatCoordinator.generateChatId(userId) } returns Result.success(chatId) + } + + @After + fun tearDown() { + BottomBarManager.clear() + } + + private fun createViewModel(): ChatViewModel = ChatViewModel( + chatCoordinator = chatCoordinator, + contactCoordinator = mockk(relaxed = true), + contactPaymentDelegate = mockk(relaxed = true), + tipPaymentDelegate = tipPaymentDelegate, + transactionController = transactionController, + tokenCoordinator = mockk(relaxed = true), + exchange = exchange, + verifiedFiatCalculator = mockk(relaxed = true), + purchaseMethodController = mockk(relaxed = true), + userManager = userManager, + resources = mockk(relaxed = true), + analytics = RecordingAnalytics(), + clipboardManager = mockk(relaxed = true), + userFlags = mockk(relaxed = true), + linkCardClassifier = mockk(relaxed = true), + linkCardResolver = mockk(relaxed = true), + cashLinkClaims = mockk(relaxed = true), + chatCashLinks = mockk(relaxed = true), + chatDraftStore = mockk(relaxed = true), + recentReactionsStore = mockk(relaxed = true), + toastController = mockk(relaxed = true), + emojiCatalogLoader = mockk(relaxed = true), + userProfileDataSource = mockk(relaxed = true), + dispatchers = TestDispatcherProvider(mainCoroutineRule.dispatcher), + ) + + private fun fetched(type: ChatType) = ChatHydration.Fetched( + ChatMembership( + metadata = mockk(relaxed = true) { every { this@mockk.type } returns type }, + isMember = null, + ), + ) + + @Test + fun `a DM the server has no record of is not fetched`() = runTest { + coEvery { chatCoordinator.hydrateChat(chatId) } returns ChatHydration.Absent + + createViewModel().dispatchEvent(openByUser) + advanceUntilIdle() + + coVerify(exactly = 0) { chatCoordinator.loadMessages(any()) } + } + + @Test + fun `a chat already on the device is fetched`() = runTest { + coEvery { chatCoordinator.hydrateChat(chatId) } returns ChatHydration.Stored + + createViewModel().dispatchEvent(openByUser) + advanceUntilIdle() + + coVerify(exactly = 1) { chatCoordinator.loadMessages(chatId) } + } + + @Test + fun `a failed lookup still fetches, since it says nothing about the chat`() = runTest { + coEvery { chatCoordinator.hydrateChat(chatId) } returns ChatHydration.Unavailable + + createViewModel().dispatchEvent(openByUser) + advanceUntilIdle() + + coVerify(exactly = 1) { chatCoordinator.loadMessages(chatId) } + } + + @Test + fun `a fetched DM is fetched`() = runTest { + coEvery { chatCoordinator.hydrateChat(chatId) } returns fetched(ChatType.TIP_DM) + + createViewModel().dispatchEvent(openByUser) + advanceUntilIdle() + + coVerify(exactly = 1) { chatCoordinator.loadMessages(chatId) } + } +} diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt index c0656b43a2..658a6fdb05 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatCoordinator.kt @@ -330,8 +330,10 @@ interface MessagingOperations { * Fetches [chatId] from the server for a chat this device holds no row for, so a conversation * reached by invite link or push tap can render before it has ever been synced. * - * Returns `null` when the row is already stored — [observeMetadata] is answering in that case, - * and a second copy would only race it — and when the fetch fails. + * Returns [ChatHydration.Stored] without a round trip when the row is already stored — + * [observeMetadata] is answering in that case, and a second copy would only race it. A fetch + * the server answers NOT_FOUND is [ChatHydration.Absent], told apart from any other failure + * because a chat that does not exist has no transcript to load. * * Deliberately not persisted. `GetChat` returns the chat, not the caller's relationship to it, * and the row's membership column has to be written with something, so storing this would @@ -339,7 +341,7 @@ interface MessagingOperations { * feed has not synced yet. The result carries membership as `null` instead, and the join path * writes the row once the server has confirmed it. */ - suspend fun hydrateChat(chatId: ChatId): ChatMembership? + suspend fun hydrateChat(chatId: ChatId): ChatHydration /** * Every profile this device holds, keyed by user-id hex. diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatHydration.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatHydration.kt new file mode 100644 index 0000000000..c39136f9ca --- /dev/null +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/ChatHydration.kt @@ -0,0 +1,26 @@ +package com.flipcash.shared.chat + +/** + * What [MessagingOperations.hydrateChat] found for a chat on the way onto the screen. + * + * The cases stay apart because the transcript fetch that follows depends on which one it was: a + * chat the server has never heard of has no messages to ask for, and asking anyway comes back + * DENIED, while a fetch that merely failed says nothing about the chat and the fetch should still + * be tried. + */ +sealed interface ChatHydration { + /** The device already holds the row; [ChatCoordinator.observeMetadata] is answering for it. */ + data object Stored : ChatHydration + + /** Fetched from the server and deliberately not persisted; see [ChatMembership.isMember]. */ + data class Fetched(val membership: ChatMembership) : ChatHydration + + /** + * The server has no such chat. A DM opened on its derived id before either side has written + * in it, from a scanned tip card or a profile, is the usual way here. + */ + data object Absent : ChatHydration + + /** The fetch failed for some other reason, so whether the chat exists is still unknown. */ + data object Unavailable : ChatHydration +} diff --git a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt index 708e89d062..5dc55342c8 100644 --- a/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt +++ b/apps/flipcash/shared/chat/src/main/kotlin/com/flipcash/shared/chat/internal/delegates/MessagingDelegate.kt @@ -29,7 +29,9 @@ import com.flipcash.services.models.chat.TypingState import com.flipcash.services.models.chat.ViewerState import com.flipcash.services.models.DeleteMessageError import com.flipcash.services.models.EditMessageError +import com.flipcash.services.models.GetChatError import com.flipcash.services.models.UserProfile +import com.flipcash.shared.chat.ChatHydration import com.flipcash.shared.chat.ChatHydrationState import com.flipcash.shared.chat.internal.SenderResolver import com.flipcash.shared.chat.ChatMembership @@ -127,13 +129,14 @@ class MessagingDelegate @Inject constructor( notificationManager.cancel(chatId.hashCode()) } - override suspend fun hydrateChat(chatId: ChatId): ChatMembership? { - if (metadataDataSource.exists(chatId)) return null - val metadata = chatController.getChat(chatId).getOrElse { + override suspend fun hydrateChat(chatId: ChatId): ChatHydration { + if (metadataDataSource.exists(chatId)) return ChatHydration.Stored + val metadata = chatController.getChat(chatId).getOrElse { cause -> + if (cause is GetChatError.NotFound) return ChatHydration.Absent trace(tag = TAG, message = "Hydrate failed for $chatId", type = TraceType.Error) - return null + return ChatHydration.Unavailable } - return ChatMembership(metadata = metadata, isMember = null) + return ChatHydration.Fetched(ChatMembership(metadata = metadata, isMember = null)) } override fun observeMessages(chatId: ChatId): Flow> { diff --git a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessagingMetadataObservationTest.kt b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessagingMetadataObservationTest.kt index 6881cd8c6f..ab2caef08c 100644 --- a/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessagingMetadataObservationTest.kt +++ b/apps/flipcash/shared/chat/src/test/kotlin/com/flipcash/shared/chat/MessagingMetadataObservationTest.kt @@ -7,6 +7,7 @@ import com.flipcash.app.persistence.sources.ChatMemberDataSource import com.flipcash.app.persistence.sources.ChatMessageDataSource import com.flipcash.app.persistence.sources.ChatMetadataDataSource import com.flipcash.services.controllers.ChatController +import com.flipcash.services.models.GetChatError import com.flipcash.services.models.chat.ChatId import com.flipcash.services.models.chat.ChatMetadata import com.flipcash.services.models.chat.ChatType @@ -22,6 +23,7 @@ import kotlinx.coroutines.test.TestCoroutineScheduler import kotlinx.coroutines.test.runTest import org.junit.Test import kotlin.test.assertEquals +import kotlin.test.assertIs import kotlin.test.assertNull import kotlin.time.Instant @@ -134,10 +136,10 @@ class MessagingMetadataObservationTest { chatController = chatController, ) - val hydrated = subject.hydrateChat(chatId) + val hydrated = assertIs(subject.hydrateChat(chatId)).membership - assertEquals(4L, hydrated?.metadata?.rosterSummary?.memberCount) - assertNull(hydrated?.isMember) + assertEquals(4L, hydrated.metadata.rosterSummary?.memberCount) + assertNull(hydrated.isMember) coVerify(exactly = 0) { metadataDataSource.upsert(any()) } coVerify(exactly = 0) { metadataDataSource.upsert(any>()) } coVerify(exactly = 0) { @@ -164,10 +166,56 @@ class MessagingMetadataObservationTest { chatController = chatController, ) - assertNull(subject.hydrateChat(chatId)) + assertEquals(ChatHydration.Stored, subject.hydrateChat(chatId)) coVerify(exactly = 0) { chatController.getChat(any()) } } + /** + * A DM opened on its derived id before anyone has written in it does not exist on the server + * yet. That answer is kept apart from a failed fetch, because the screen skips the transcript + * fetch for it: the server answers GetMessages on a chat it has never heard of with DENIED. + */ + @Test + fun `hydrateChat reports a chat the server has no record of as absent`() = + runTest(dispatchers.dispatcher) { + val metadataDataSource = mockk(relaxed = true) { + coEvery { exists(chatId) } returns false + } + val chatController = mockk(relaxed = true) { + coEvery { getChat(chatId) } returns Result.failure(GetChatError.NotFound()) + } + + val subject = delegate( + metadataDataSource = metadataDataSource, + memberDataSource = mockk(relaxed = true), + messageDataSource = mockk(relaxed = true), + chatController = chatController, + ) + + assertEquals(ChatHydration.Absent, subject.hydrateChat(chatId)) + } + + /** Any other failure says nothing about whether the chat exists. */ + @Test + fun `hydrateChat reports any other failure as unavailable`() = + runTest(dispatchers.dispatcher) { + val metadataDataSource = mockk(relaxed = true) { + coEvery { exists(chatId) } returns false + } + val chatController = mockk(relaxed = true) { + coEvery { getChat(chatId) } returns Result.failure(GetChatError.Other()) + } + + val subject = delegate( + metadataDataSource = metadataDataSource, + memberDataSource = mockk(relaxed = true), + messageDataSource = mockk(relaxed = true), + chatController = chatController, + ) + + assertEquals(ChatHydration.Unavailable, subject.hydrateChat(chatId)) + } + private fun delegate( metadataDataSource: ChatMetadataDataSource, memberDataSource: ChatMemberDataSource, diff --git a/services/flipcash/src/main/kotlin/com/flipcash/services/models/Errors.kt b/services/flipcash/src/main/kotlin/com/flipcash/services/models/Errors.kt index 9d4e2df4cd..d9a91dcdfe 100644 --- a/services/flipcash/src/main/kotlin/com/flipcash/services/models/Errors.kt +++ b/services/flipcash/src/main/kotlin/com/flipcash/services/models/Errors.kt @@ -345,7 +345,12 @@ sealed class GetChatError( override val cause: Throwable? = null ): CodeServerError(message, cause) { class Denied : GetChatError("Denied") - class NotFound : GetChatError("Not found") + + /** + * The server has no such chat. Every caller handles it — a DM opened before anyone has written + * in it, a deleted group behind a link card — so it is not reported. + */ + class NotFound : GetChatError("Not found"), UnreportedError class Unrecognized : GetChatError("Unrecognized"), NotifiableError data class Other(override val cause: Throwable? = null) : GetChatError(message = cause?.message, cause = cause), NotifiableError } diff --git a/services/flipcash/src/test/kotlin/com/flipcash/services/models/ErrorsTest.kt b/services/flipcash/src/test/kotlin/com/flipcash/services/models/ErrorsTest.kt index 910c23d484..332734b316 100644 --- a/services/flipcash/src/test/kotlin/com/flipcash/services/models/ErrorsTest.kt +++ b/services/flipcash/src/test/kotlin/com/flipcash/services/models/ErrorsTest.kt @@ -196,6 +196,11 @@ class ErrorsTest { assertEquals("Not found", GetChatError.NotFound().message) } + @Test + fun `GetChatError NotFound is not reported`() { + assertIs(GetChatError.NotFound()) + } + // -- GetDmChatFeedError -- @Test