diff --git a/mail-conversation/data/src/main/kotlin/ch/protonmail/android/mailconversation/data/repository/ConversationRepositoryImpl.kt b/mail-conversation/data/src/main/kotlin/ch/protonmail/android/mailconversation/data/repository/ConversationRepositoryImpl.kt index 48862571f8..e2c76d9b11 100644 --- a/mail-conversation/data/src/main/kotlin/ch/protonmail/android/mailconversation/data/repository/ConversationRepositoryImpl.kt +++ b/mail-conversation/data/src/main/kotlin/ch/protonmail/android/mailconversation/data/repository/ConversationRepositoryImpl.kt @@ -31,7 +31,7 @@ import ch.protonmail.android.mailconversation.domain.repository.ConversationRemo import ch.protonmail.android.mailconversation.domain.repository.ConversationRepository import ch.protonmail.android.maillabel.domain.extension.isSpam import ch.protonmail.android.maillabel.domain.extension.isTrash -import ch.protonmail.android.maillabel.domain.model.SystemLabelId +import ch.protonmail.android.maillabel.domain.model.filterUnmodifiableLabels import ch.protonmail.android.mailmessage.data.local.MessageLocalDataSource import ch.protonmail.android.mailmessage.data.usecase.ExcludeDraftMessagesAlreadyInOutbox import ch.protonmail.android.mailpagination.domain.model.PageKey @@ -146,13 +146,14 @@ class ConversationRepositoryImpl @Inject constructor( conversationIds: List, labelIds: List ): Either> { - return conversationLocalDataSource.addLabels(userId, conversationIds, labelIds).onRight { + val filteredLabelIdsToAdd = labelIds.filterUnmodifiableLabels() + return conversationLocalDataSource.addLabels(userId, conversationIds, filteredLabelIdsToAdd).onRight { messageLocalDataSource.relabelMessagesInConversations( userId = userId, conversationIds = conversationIds, - labelIdsToAdd = labelIds.toSet() + labelIdsToAdd = filteredLabelIdsToAdd.toSet() ) - conversationRemoteDataSource.addLabels(userId, conversationIds, labelIds) + conversationRemoteDataSource.addLabels(userId, conversationIds, filteredLabelIdsToAdd) } } @@ -173,13 +174,14 @@ class ConversationRepositoryImpl @Inject constructor( conversationIds: List, labelIds: List ): Either> { - return conversationLocalDataSource.removeLabels(userId, conversationIds, labelIds).onRight { + val filteredLabelIdsToRemove = labelIds.filterUnmodifiableLabels() + return conversationLocalDataSource.removeLabels(userId, conversationIds, filteredLabelIdsToRemove).onRight { messageLocalDataSource.relabelMessagesInConversations( userId = userId, conversationIds = conversationIds, - labelIdsToRemove = labelIds.toSet() + labelIdsToRemove = filteredLabelIdsToRemove.toSet() ) - conversationRemoteDataSource.removeLabels(userId, conversationIds, labelIds) + conversationRemoteDataSource.removeLabels(userId, conversationIds, filteredLabelIdsToRemove) } } @@ -203,7 +205,6 @@ class ConversationRepositoryImpl @Inject constructor( return moveToTrashOrSpam(userId, conversationIds, allLabelIds, toLabelId) } - return conversationLocalDataSource.relabel( userId = userId, conversationIds = conversationIds, @@ -303,23 +304,18 @@ class ConversationRepositoryImpl @Inject constructor( ): Either> { require(labelId.isTrash() || labelId.isSpam()) { "Invalid system label id: $labelId" } - val persistentLabels = setOf( - SystemLabelId.AllDrafts.labelId, - SystemLabelId.AllMail.labelId, - SystemLabelId.AllSent.labelId - ) - val labelsToBeRemoved = allLabelIds - persistentLabels + val filteredLabelIdsToRemove = allLabelIds.filterUnmodifiableLabels() return conversationLocalDataSource.relabel( userId = userId, conversationIds = conversationIds, labelIdsToAdd = listOf(labelId), - labelIdsToRemove = labelsToBeRemoved + labelIdsToRemove = filteredLabelIdsToRemove ).onRight { messageLocalDataSource.relabelMessagesInConversations( userId = userId, conversationIds = conversationIds, - labelIdsToRemove = labelsToBeRemoved.toSet(), + labelIdsToRemove = filteredLabelIdsToRemove.toSet(), labelIdsToAdd = setOf(labelId) ) conversationRemoteDataSource.addLabel(userId, conversationIds, labelId) diff --git a/mail-conversation/data/src/test/kotlin/ch/protonmail/android/mailconversation/data/ConversationRepositoryImplTest.kt b/mail-conversation/data/src/test/kotlin/ch/protonmail/android/mailconversation/data/ConversationRepositoryImplTest.kt index 2b40cf28cc..20e5128a68 100644 --- a/mail-conversation/data/src/test/kotlin/ch/protonmail/android/mailconversation/data/ConversationRepositoryImplTest.kt +++ b/mail-conversation/data/src/test/kotlin/ch/protonmail/android/mailconversation/data/ConversationRepositoryImplTest.kt @@ -45,6 +45,7 @@ import ch.protonmail.android.mailpagination.domain.model.PageKey import ch.protonmail.android.testdata.conversation.ConversationTestData import ch.protonmail.android.testdata.conversation.ConversationWithContextTestData import ch.protonmail.android.testdata.message.MessageTestData +import ch.protonmail.android.testdata.message.MessageTestData.unmodifiableLabels import io.mockk.Called import io.mockk.Ordering import io.mockk.coEvery @@ -777,7 +778,7 @@ class ConversationRepositoryImplTest { // given val conversations = listOf(ConversationTestData.conversationWithConversationLabels) val conversationIds = listOf(ConversationId(ConversationTestData.RAW_CONVERSATION_ID)) - val labelsToBeRemoved = listOf(LabelId("1")) + val labelsToBeRemoved = listOf(LabelId("3")) val labelsToBeAdded = listOf(LabelId("10"), LabelId("11")) coEvery { conversationLocalDataSource.removeLabels(userId, conversationIds, labelsToBeRemoved) @@ -810,6 +811,82 @@ class ConversationRepositoryImplTest { } } + @Test + fun `relabel conversations does not remove unmodifiable labels`() = runTest { + // given + val conversations = listOf(ConversationTestData.conversationWithConversationLabels) + val conversationIds = listOf(ConversationId(ConversationTestData.RAW_CONVERSATION_ID)) + val labelsToBeAdded = listOf(SystemLabelId.Starred.labelId) + val labelsToBeRemoved = unmodifiableLabels.map { it.labelId } + coEvery { + conversationLocalDataSource.removeLabels(userId, conversationIds, emptyList()) + } returns conversations.right() + coEvery { + conversationLocalDataSource.addLabels(userId, conversationIds, labelsToBeAdded) + } returns conversations.right() + coEvery { + messageLocalDataSource.relabelMessagesInConversations( + userId = userId, + conversationIds = conversationIds, + labelIdsToRemove = emptySet() + ) + } returns MessageTestData.unStarredMsgByConversationWithStarredMsg.right() + coEvery { + messageLocalDataSource.relabelMessagesInConversations( + userId = userId, + conversationIds = conversationIds, + labelIdsToAdd = labelsToBeAdded.toSet() + ) + } returns MessageTestData.unStarredMsgByConversationWithStarredMsg.right() + + // when + conversationRepository.relabel(userId, conversationIds, labelsToBeRemoved, labelsToBeAdded) + + // then + coVerifyOrder { + conversationLocalDataSource.removeLabels(userId, conversationIds, emptyList()) + conversationLocalDataSource.addLabels(userId, conversationIds, labelsToBeAdded) + } + } + + @Test + fun `relabel conversations does not add unmodifiable labels`() = runTest { + // given + val conversations = listOf(ConversationTestData.conversationWithConversationLabels) + val conversationIds = listOf(ConversationId(ConversationTestData.RAW_CONVERSATION_ID)) + val labelsToBeAdded = unmodifiableLabels.map { it.labelId } + val labelsToBeRemoved = listOf(SystemLabelId.Starred.labelId) + coEvery { + conversationLocalDataSource.removeLabels(userId, conversationIds, labelsToBeRemoved) + } returns conversations.right() + coEvery { + conversationLocalDataSource.addLabels(userId, conversationIds, emptyList()) + } returns conversations.right() + coEvery { + messageLocalDataSource.relabelMessagesInConversations( + userId = userId, + conversationIds = conversationIds, + labelIdsToRemove = labelsToBeRemoved.toSet() + ) + } returns MessageTestData.unStarredMsgByConversationWithStarredMsg.right() + coEvery { + messageLocalDataSource.relabelMessagesInConversations( + userId = userId, + conversationIds = conversationIds, + labelIdsToAdd = emptySet() + ) + } returns MessageTestData.unStarredMsgByConversationWithStarredMsg.right() + + // when + conversationRepository.relabel(userId, conversationIds, labelsToBeRemoved, labelsToBeAdded) + + // then + coVerifyOrder { + conversationLocalDataSource.removeLabels(userId, conversationIds, labelsToBeRemoved) + conversationLocalDataSource.addLabels(userId, conversationIds, emptyList()) + } + } + @Test fun `mark read returns error when local data source fails`() = runTest { // given diff --git a/mail-label/domain/src/main/kotlin/ch/protonmail/android/maillabel/domain/model/SystemLabelId.kt b/mail-label/domain/src/main/kotlin/ch/protonmail/android/maillabel/domain/model/SystemLabelId.kt index 69f8a6f274..0f61af51b9 100644 --- a/mail-label/domain/src/main/kotlin/ch/protonmail/android/maillabel/domain/model/SystemLabelId.kt +++ b/mail-label/domain/src/main/kotlin/ch/protonmail/android/maillabel/domain/model/SystemLabelId.kt @@ -32,6 +32,7 @@ import ch.protonmail.android.maillabel.domain.model.MailLabelId.System.Snoozed import ch.protonmail.android.maillabel.domain.model.MailLabelId.System.Spam import ch.protonmail.android.maillabel.domain.model.MailLabelId.System.Starred import ch.protonmail.android.maillabel.domain.model.MailLabelId.System.Trash +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.Companion.unmodifiableByUserList import me.proton.core.label.domain.entity.LabelId enum class SystemLabelId(val labelId: LabelId) { @@ -87,7 +88,7 @@ enum class SystemLabelId(val labelId: LabelId) { companion object { - private val map = values().associateBy { stringOf(it) } + private val map = entries.associateBy { stringOf(it) } val displayedList = listOf(Inbox, Drafts, Sent, Starred, Archive, Spam, Trash, AllMail) @@ -95,7 +96,9 @@ enum class SystemLabelId(val labelId: LabelId) { val exclusiveList = exclusiveDestinationList + Drafts + Sent - fun stringOf(value: SystemLabelId): String = value.labelId.id + val unmodifiableByUserList = listOf(AllMail, AlmostAllMail, AllDrafts, AllSent, AllScheduled, Outbox, Snoozed) + + private fun stringOf(value: SystemLabelId): String = value.labelId.id fun enumOf(value: String?): SystemLabelId = map[value] ?: Inbox } } @@ -118,4 +121,6 @@ fun SystemLabelId.toMailLabelSystem(): MailLabel.System = when (this) { SystemLabelId.Snoozed -> MailLabel.System(Snoozed) } -fun LabelId.isReservedSystemLabelId() = id in SystemLabelId.values().map { it.labelId.id } +fun LabelId.isReservedSystemLabelId() = id in SystemLabelId.entries.map { it.labelId.id } + +fun List.filterUnmodifiableLabels(): List = this - unmodifiableByUserList.map { it.labelId }.toSet() diff --git a/test/test-data/src/main/kotlin/ch/protonmail/android/testdata/message/MessageTestData.kt b/test/test-data/src/main/kotlin/ch/protonmail/android/testdata/message/MessageTestData.kt index 82a7beeafc..a379229757 100644 --- a/test/test-data/src/main/kotlin/ch/protonmail/android/testdata/message/MessageTestData.kt +++ b/test/test-data/src/main/kotlin/ch/protonmail/android/testdata/message/MessageTestData.kt @@ -23,6 +23,13 @@ import java.time.temporal.ChronoUnit import ch.protonmail.android.mailcommon.domain.model.ConversationId import ch.protonmail.android.maillabel.domain.model.MailLabelId import ch.protonmail.android.maillabel.domain.model.SystemLabelId +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.AllDrafts +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.AllMail +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.AllScheduled +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.AllSent +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.AlmostAllMail +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.Outbox +import ch.protonmail.android.maillabel.domain.model.SystemLabelId.Snoozed import ch.protonmail.android.mailmessage.domain.model.AttachmentCount import ch.protonmail.android.mailmessage.domain.model.Message import ch.protonmail.android.mailmessage.domain.model.MessageId @@ -44,6 +51,9 @@ object MessageTestData { val recipient2 = Recipient("recipient2@pm.me", "Recipient2") val recipient3 = Recipient("recipient3@pm.me", "Recipient3") + // Do not reference production code directly for this, otherwise tests will never fail in case of changes. + val unmodifiableLabels = listOf(AllMail, AlmostAllMail, AllDrafts, AllSent, AllScheduled, Outbox, Snoozed) + val message = buildMessage( userId = userId, id = RAW_MESSAGE_ID, subject = RAW_SUBJECT, labelIds = listOf(SystemLabelId.Inbox.labelId.id) )