Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: read receipts are not sent when the app is in background (WPB-8756) #3054

Open
wants to merge 1 commit into
base: release/candidate
Choose a base branch
from
Open

fix: read receipts are not sent when the app is in background (WPB-8756) #3054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions ...in/kotlin/com/wire/kalium/logic/feature/conversation/UpdateConversationReadDateUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,18 @@ class UpdateConversationReadDateUseCase internal constructor(
* @param conversationId The conversation id to update the last read date.
* @param time The last read date to update.
*/
operator fun invoke(conversationId: QualifiedID, time: Instant) {
workQueue.enqueue(ConversationTimeEventInput(conversationId, time), worker)
operator fun invoke(
conversationId: QualifiedID,
time: Instant,
shouldWaitUntilLive: Boolean = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is the right way to solve it, since this use case will send an encrypted message, and messages should only be sent if the app is live, to avoid breaking the session

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the better approach is to handle it the same way we do when FCM message is received, start incremental sync and send the message when we are live then drop after ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reason to redo the sync for nothing because the app is already up to date, all messages are synced after getting the event through FCM.
The user in this case just need to reply from notification without waiting for app to complete sync.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but user might reply from notification with some delay, and during that delay some changes (new messages) might happen. For example: user receive notification -> lose connection for 5 minutes -> some messages comes to that conversation, but there is no connection, so the client doesn't know about it -> client goes online and sends reply instantly without sync

) {
workQueue.enqueue(
ConversationTimeEventInput(conversationId, time, shouldWaitUntilLive),
worker
)
}

private val worker = ConversationTimeEventWorker { (conversationId, time) ->
private val worker = ConversationTimeEventWorker { (conversationId, time, shouldWaitUntilLive) ->
coroutineScope {
conversationRepository.observeConversationById(conversationId).first().onFailure {
logger.w("Failed to update conversation read date; StorageFailure $it")
Expand All @@ -82,7 +89,7 @@ class UpdateConversationReadDateUseCase internal constructor(
return@onSuccess
}
launch {
sendConfirmation(conversationId, conversation.lastReadDate, time)
sendConfirmation(conversationId, conversation.lastReadDate, time, shouldWaitUntilLive)
}
launch {
conversationRepository.updateConversationReadDate(conversationId, time)
Expand Down
3 changes: 2 additions & 1 deletion ...monMain/kotlin/com/wire/kalium/logic/feature/message/receipt/ConversationTimeEventWork.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import kotlinx.datetime.Instant
*/
internal data class ConversationTimeEventInput(
val conversationId: ConversationId,
val eventTime: Instant
val eventTime: Instant,
val shouldWaitUntilLive: Boolean
)

/**
Expand Down
10 changes: 7 additions & 3 deletions ...ommonMain/kotlin/com/wire/kalium/logic/feature/message/receipt/SendConfirmationUseCase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ internal interface SendConfirmationUseCase {
suspend operator fun invoke(
conversationId: ConversationId,
afterDateTime: Instant,
untilDateTime: Instant
untilDateTime: Instant,
shouldWaitUntilLive: Boolean = true
): Either<CoreFailure, Unit>
}

Expand All @@ -81,9 +82,12 @@ internal fun SendConfirmationUseCase(
override suspend operator fun invoke(
conversationId: ConversationId,
afterDateTime: Instant,
untilDateTime: Instant
untilDateTime: Instant,
shouldWaitUntilLive: Boolean
): Either<CoreFailure, Unit> {
syncManager.waitUntilLive()
if (shouldWaitUntilLive) {
syncManager.waitUntilLive()
}

val messageIds = getPendingUnreadMessagesIds(
conversationId,
Expand Down
2 changes: 1 addition & 1 deletion ...kotlin/com/wire/kalium/logic/feature/message/receipt/ParallelConversationWorkQueueTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,6 @@ class ParallelConversationWorkQueueTest {
private fun workInput(
convIdValue: String = "abc",
time: Instant = Instant.DISTANT_PAST
) = ConversationTimeEventInput(ConversationId(convIdValue, "domain"), time)
) = ConversationTimeEventInput(ConversationId(convIdValue, "domain"), time, true)

}
33 changes: 32 additions & 1 deletion ...nTest/kotlin/com/wire/kalium/logic/feature/message/receipt/SendConfirmationUseCaseTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,37 @@ class SendConfirmationUseCaseTest {
val result = sendConfirmation(TestConversation.ID, after, until)

result.shouldSucceed()

coVerify {
arrangement.syncManager.waitUntilLive()
}.wasInvoked(exactly = once)

coVerify {
arrangement.messageSender.sendMessage(any(), any())
}.wasInvoked(exactly = once)
}

@Test
fun givenAShouldNotWaitUntilLive_whenSendingReadReceipts_theDoNotWaitForSyncAndSendConfirmation() = runTest {
val (arrangement, sendConfirmation) = Arrangement()
.withCurrentClientIdProvider()
.withGetConversationByIdSuccessful()
.withToggleReadReceiptsStatus(true)
.withPendingMessagesResponse()
.withSendMessageSuccess()
.arrange()

val after = Instant.DISTANT_PAST
val until = after + 10.seconds

val result = sendConfirmation(TestConversation.ID, after, until, false)

result.shouldSucceed()

coVerify {
arrangement.syncManager.waitUntilLive()
}.wasNotInvoked()

coVerify {
arrangement.messageSender.sendMessage(any(), any())
}.wasInvoked(exactly = once)
Expand Down Expand Up @@ -101,7 +132,7 @@ class SendConfirmationUseCaseTest {
private val currentClientIdProvider = mock(CurrentClientIdProvider::class)

@Mock
private val syncManager = mock(SyncManager::class)
val syncManager = mock(SyncManager::class)

@Mock
val messageSender = mock(MessageSender::class)
Expand Down
Loading