From 7564a39aa4104111436fcfb44655b55d309646a6 Mon Sep 17 00:00:00 2001 From: Yamil Medina Date: Tue, 30 Jan 2024 10:12:28 +0100 Subject: [PATCH] fix: duplicated system messages for adding/removing participants mls (WPB-6249) (#2426) * fix: handle local creation id of event with localid to avoid duplication * fix: adding participants failure message handle only in case there are, plus docs * fix: more docs --------- Co-authored-by: Mojtaba Chenani --- .../ConversationGroupRepository.kt | 9 +++-- .../conversation/MLSConversationRepository.kt | 3 +- .../logic/data/message/MessageContent.kt | 28 +++++++++++++++ .../ConversationGroupRepositoryTest.kt | 14 ++++---- .../MLSConversationRepositoryTest.kt | 9 ++--- .../persistence/dao/message/MessageEntity.kt | 34 ++++++++++++++++++- 6 files changed, 79 insertions(+), 18 deletions(-) diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepository.kt index 6e7dc614b3d..4484e9f7b59 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepository.kt @@ -207,7 +207,7 @@ internal class ConversationGroupRepositoryImpl( remainingAttempts: Int = 2 ): Either { return when (val addingMemberResult = mlsConversationRepository.addMemberToMLSGroup(GroupID(groupId), userIdList)) { - is Either.Right -> handleMLSMembersAdded(conversationId, userIdList, failedUsersList) + is Either.Right -> handleMLSMembersNotAdded(conversationId, failedUsersList) is Either.Left -> { addingMemberResult.value.handleMLSMembersFailed( conversationId = conversationId, @@ -262,13 +262,12 @@ internal class ConversationGroupRepositoryImpl( } } - private suspend fun handleMLSMembersAdded( + private suspend fun handleMLSMembersNotAdded( conversationId: ConversationId, - userIdList: List, failedUsersList: Set ): Either { - return newGroupConversationSystemMessagesCreator.value.conversationResolvedMembersAddedAndFailed( - conversationId.toDao(), userIdList, failedUsersList.toList() + return newGroupConversationSystemMessagesCreator.value.conversationFailedToAddMembers( + conversationId, failedUsersList ).flatMap { Either.Right(Unit) } diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt index c952880399c..3ab2a9f75c1 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepository.kt @@ -68,6 +68,7 @@ import com.wire.kalium.network.exceptions.isMlsCommitMissingReferences import com.wire.kalium.network.exceptions.isMlsStaleMessage import com.wire.kalium.persistence.dao.conversation.ConversationDAO import com.wire.kalium.persistence.dao.conversation.ConversationEntity +import com.wire.kalium.persistence.dao.message.LocalId import com.wire.kalium.util.DateTimeUtil import com.wire.kalium.util.KaliumDispatcher import com.wire.kalium.util.KaliumDispatcherImpl @@ -367,7 +368,7 @@ internal class MLSConversationDataSource( private suspend fun processCommitBundleEvents(events: List) { events.forEach { eventContentDTO -> - val event = MapperProvider.eventMapper(selfUserId).fromEventContentDTO("", eventContentDTO, true, false) + val event = MapperProvider.eventMapper(selfUserId).fromEventContentDTO(LocalId.generate(), eventContentDTO, true, false) if (event is Event.Conversation) { commitBundleEventReceiver.onEvent(event) } diff --git a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/MessageContent.kt b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/MessageContent.kt index b0580765486..d0904a91061 100644 --- a/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/MessageContent.kt +++ b/logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/MessageContent.kt @@ -170,6 +170,7 @@ sealed class MessageContent { @SerialName("data") val targets: Targets? = null, ) + @Serializable data class Targets( @SerialName("targets") @@ -249,11 +250,36 @@ sealed class MessageContent { // server message content types // TODO: rename members to userList sealed class MemberChange(open val members: List) : System() { + /** + * A member(s) was added to the conversation. + */ data class Added(override val members: List) : MemberChange(members) + + /** + * A member(s) was removed from the conversation. + */ data class Removed(override val members: List) : MemberChange(members) + + /** + * A member(s) was removed from the team. + */ data class RemovedFromTeam(override val members: List) : MemberChange(members) + + /** + * A member(s) was not added to the conversation. + * Note: This is only valid for the creator of the conversation, local-only. + */ data class FailedToAdd(override val members: List) : MemberChange(members) + + /** + * A member(s) was added to the conversation while the conversation was being created. + * Note: This is only valid for the creator of the conversation, local-only. + */ data class CreationAdded(override val members: List) : MemberChange(members) + + /** + * Member(s) removed from the conversation, due to some backend stopped to federate between them, or us. + */ data class FederationRemoved(override val members: List) : MemberChange(members) } @@ -339,11 +365,13 @@ sealed class MessageContent { data class Removed(val domain: String) : FederationStopped() data class ConnectionRemoved(val domainList: List) : FederationStopped() } + sealed class LegalHold : System() { sealed class ForMembers(open val members: List) : LegalHold() { data class Enabled(override val members: List) : ForMembers(members) data class Disabled(override val members: List) : ForMembers(members) } + sealed class ForConversation : LegalHold() { data object Enabled : ForConversation() data object Disabled : ForConversation() diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepositoryTest.kt index 03d7152e2a8..967dd361d3b 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/ConversationGroupRepositoryTest.kt @@ -452,7 +452,7 @@ class ConversationGroupRepositoryTest { .withProtocolInfoById(MLS_PROTOCOL_INFO) .withAddMemberAPISucceedChanged() .withSuccessfulAddMemberToMLSGroup() - .withInsertAddedAndFailedSystemMessageSuccess() + .withInsertFailedToAddSystemMessageSuccess() .arrange() conversationGroupRepository.addMembers(listOf(TestConversation.USER_1), TestConversation.ID) @@ -1201,7 +1201,7 @@ class ConversationGroupRepositoryTest { KEY_PACKAGES_NOT_AVAILABLE_FAILURE, Either.Right(Unit) ) - .withInsertAddedAndFailedSystemMessageSuccess() + .withInsertFailedToAddSystemMessageSuccess() .arrange() // when @@ -1223,8 +1223,8 @@ class ConversationGroupRepositoryTest { }).wasInvoked(exactly = once) verify(arrangement.newGroupConversationSystemMessagesCreator) - .suspendFunction(arrangement.newGroupConversationSystemMessagesCreator::conversationResolvedMembersAddedAndFailed) - .with(anything(), matching { it.size == 1 }, matching { it.size == 1 }) + .suspendFunction(arrangement.newGroupConversationSystemMessagesCreator::conversationFailedToAddMembers) + .with(anything(), matching { it.size == 1 }) .wasInvoked(exactly = once) } @@ -1241,7 +1241,7 @@ class ConversationGroupRepositoryTest { buildCommitBundleFederatedFailure("otherDomain"), Either.Right(Unit) ) - .withInsertAddedAndFailedSystemMessageSuccess() + .withInsertFailedToAddSystemMessageSuccess() .arrange() // when @@ -1263,8 +1263,8 @@ class ConversationGroupRepositoryTest { }).wasInvoked(exactly = once) verify(arrangement.newGroupConversationSystemMessagesCreator) - .suspendFunction(arrangement.newGroupConversationSystemMessagesCreator::conversationResolvedMembersAddedAndFailed) - .with(anything(), matching { it.size == 1 }, matching { it.size == 1 }) + .suspendFunction(arrangement.newGroupConversationSystemMessagesCreator::conversationFailedToAddMembers) + .with(anything(), matching { it.size == 1 }) .wasInvoked(exactly = once) } diff --git a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt index 2463e78d25f..dfd9cbd4789 100644 --- a/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt +++ b/logic/src/commonTest/kotlin/com/wire/kalium/logic/data/conversation/MLSConversationRepositoryTest.kt @@ -76,6 +76,7 @@ import com.wire.kalium.persistence.dao.UserIDEntity import com.wire.kalium.persistence.dao.conversation.ConversationDAO import com.wire.kalium.persistence.dao.conversation.ConversationEntity import com.wire.kalium.persistence.dao.conversation.E2EIConversationClientInfoEntity +import com.wire.kalium.persistence.dao.message.LocalId import com.wire.kalium.util.DateTimeUtil import io.ktor.util.decodeBase64Bytes import io.ktor.util.encodeBase64 @@ -392,7 +393,7 @@ class MLSConversationRepositoryTest { } @Test - fun givenSuccessfulResponses_whenCallingAddMemberToMLSGroup_thenMemberJoinEventIsProcessed() = runTest { + fun givenSuccessfulResponses_whenCallingAddMemberToMLSGroup_thenMemberJoinEventIsProcessedWithLocalId() = runTest { val (arrangement, mlsConversationRepository) = Arrangement() .withCommitPendingProposalsReturningNothing() .withClaimKeyPackagesSuccessful() @@ -406,7 +407,7 @@ class MLSConversationRepositoryTest { verify(arrangement.commitBundleEventReceiver) .suspendFunction(arrangement.commitBundleEventReceiver::onEvent) - .with(anyInstanceOf(Event.Conversation.MemberJoin::class)) + .with(matching { it is Event.Conversation.MemberJoin && LocalId.check(it.id) }) .wasInvoked(once) } @@ -752,7 +753,7 @@ class MLSConversationRepositoryTest { } @Test - fun givenSuccessfulResponses_whenCallingRemoveMemberFromGroup_thenMemberLeaveEventIsProcessed() = runTest { + fun givenSuccessfulResponses_whenCallingRemoveMemberFromGroup_thenMemberLeaveEventIsProcessedWithLocalId() = runTest { val (arrangement, mlsConversationRepository) = Arrangement() .withCommitPendingProposalsReturningNothing() .withGetMLSClientSuccessful() @@ -767,7 +768,7 @@ class MLSConversationRepositoryTest { verify(arrangement.commitBundleEventReceiver) .suspendFunction(arrangement.commitBundleEventReceiver::onEvent) - .with(anyInstanceOf(Event.Conversation.MemberLeave::class)) + .with(matching { it is Event.Conversation.MemberLeave && LocalId.check(it.id) }) .wasInvoked(once) } diff --git a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/message/MessageEntity.kt b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/message/MessageEntity.kt index bff78026429..7ef3ab5307e 100644 --- a/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/message/MessageEntity.kt +++ b/persistence/src/commonMain/kotlin/com/wire/kalium/persistence/dao/message/MessageEntity.kt @@ -203,7 +203,37 @@ sealed interface MessageEntity { } enum class MemberChangeType { - ADDED, REMOVED, CREATION_ADDED, FAILED_TO_ADD, FEDERATION_REMOVED, REMOVED_FROM_TEAM; + /** + * A member(s) was added to the conversation. + */ + ADDED, + + /** + * A member(s) was removed from the conversation. + */ + REMOVED, + + /** + * A member(s) was added to the conversation while the conversation was being created. + * Note: This is only valid for the creator of the conversation, local-only. + */ + CREATION_ADDED, + + /** + * A member(s) was not added to the conversation. + * Note: This is only valid for the creator of the conversation, local-only. + */ + FAILED_TO_ADD, + + /** + * Member(s) removed from the conversation, due to some backend stopped to federate between them, or us. + */ + FEDERATION_REMOVED, + + /** + * A member(s) was removed from the team. + */ + REMOVED_FROM_TEAM; } enum class FederationType { @@ -339,6 +369,7 @@ sealed class MessageEntityContent { data object MissedCall : System() data object CryptoSessionReset : System() data class ConversationRenamed(val conversationName: String) : System() + @Deprecated("not maintained and will be deleted") data class TeamMemberRemoved(val userName: String) : System() data class NewConversationReceiptMode(val receiptMode: Boolean) : System() @@ -419,6 +450,7 @@ sealed class MessagePreviewEntityContent { val otherUserIdList: List, val isContainSelfUserId: Boolean, ) : MessagePreviewEntityContent() + data class TeamMembersRemoved( val senderName: String?, val otherUserIdList: List,