Skip to content

feat: Messages actions use case#2819

Open
solrubado wants to merge 34 commits intoprotected/refactor-mail-actionsfrom
messages-actions-use-case
Open

feat: Messages actions use case#2819
solrubado wants to merge 34 commits intoprotected/refactor-mail-actionsfrom
messages-actions-use-case

Conversation

@solrubado
Copy link
Copy Markdown
Contributor

@solrubado solrubado commented Feb 23, 2026

Depends on #2814

@solrubado solrubado changed the title Messages actions use case feat: Messages actions use case Feb 24, 2026
@github-actions github-actions Bot added the dependent This MR depends on another PR label Feb 24, 2026
@FabianDevel FabianDevel force-pushed the messages-actions-use-case branch from b80a46a to 3a7ff5e Compare March 3, 2026 14:34
@solrubado solrubado force-pushed the network-management branch from e6d2918 to cde5dde Compare March 3, 2026 15:11
Base automatically changed from network-management to protected/refactor-mail-actions March 3, 2026 15:22
@github-actions github-actions Bot removed the dependent This MR depends on another PR label Mar 3, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 3, 2026

This PR/issue depends on:

@solrubado solrubado force-pushed the messages-actions-use-case branch from 3a7ff5e to 3db2f00 Compare March 3, 2026 15:26
Comment thread app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ActionsViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/useCases/MessagesActionsUseCase.kt Outdated
}

sealed class ApiCallResult {
data class Success(val messageRes: Int) : ApiCallResult()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you put a message in success ? I don't think we need it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We show a success message when we do a phishing report, block user and when we successfully undo an action.

Comment thread app/src/main/java/com/infomaniak/mail/ui/main/thread/actions/ActionsViewModel.kt Outdated

private suspend fun moveOutThreadsLocally(messages: List<Message>, destinationFolder: Folder): List<String> {
val uidsToMove = mutableListOf<String>().apply {
messages.flatMapTo(mutableSetOf(), Message::threads).forEach { thread ->
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see that you now open the realm to get the thread, why do you need to do it now ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is safer this way, the old code could access thread.messages and it.folderId on potentially deleted objects.

Comment thread app/src/main/java/com/infomaniak/mail/ui/MainViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/useCases/MessagesActionsUseCase.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/useCases/MessagesActionsUseCase.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/useCases/MessagesActionsUseCase.kt Outdated
@solrubado solrubado force-pushed the messages-actions-use-case branch from e18fa08 to 9c6dbcf Compare March 23, 2026 14:11
Copy link
Copy Markdown
Contributor

@FabianDevel FabianDevel left a comment

Choose a reason for hiding this comment

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

It appears when testing that the messages actions inside a thread may not work

trackScheduleSendEvent(MatomoName.CustomScheduleConfirm)
localSettings.lastSelectedScheduleEpochMillis = timestamp
mainViewModel.rescheduleDraft(Date(timestamp))
actionsViewModel.rescheduleDraft(Date(timestamp), mainViewModel.currentMailbox.value!!)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You should not add new code with this nullablility check
You can manage it like elsewhere with an error snackback

threadController.updateIsLocallyMovedOutStatus(threadsUids, hasBeenMovedOut = false)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revert this

downloadThreadsStatusManager.updateState(false)
}


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Revert this

threads.flatMap { thread ->
messageController.getLastMessageAndItsDuplicatesToExecuteAction(thread, mailbox.featureFlags)
}
} else threads.flatMap { thread -> messageController.getUnseenMessages(thread) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Always add the brackets when the whole if cannot be inlined

Comment on lines +259 to +262
messages = messages,
mailbox = mailbox,
threadsUids = threadsUids,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oneline this

fun updateState(isDownloading: Boolean) {
_isDownloading.value = isDownloading
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this line


data class UndoData(
val resources: List<String>,
val resources: List<String>?,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It doesn't need to be nullable, and the code that checks this nullability will be useless

@solrubado solrubado force-pushed the messages-actions-use-case branch from 6a03b28 to c51c111 Compare April 15, 2026 14:39
@solrubado solrubado force-pushed the protected/refactor-mail-actions branch 2 times, most recently from e8077b7 to 86a961b Compare April 16, 2026 07:23
@solrubado solrubado force-pushed the messages-actions-use-case branch from c51c111 to bd50647 Compare April 16, 2026 07:29
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

2814 - Partially compliant

Compliant requirements:

  • Refactored message actions into dedicated use case (MessagesActionsUseCase)
  • Centralized API calls within the use case layer
  • Separated business logic from ViewModels into use cases
  • Removed direct ApiRepository calls from ViewModels

Non-compliant requirements:

Requires further human verification:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Auto-Advance Logic

The refactoring removed the shouldAutoAdvance check that was previously present in moveThreadsOrMessageTo. When moving threads/messages succeeds (e.g., in createNewFolderAndMove), the UI should auto-advance to the next item if applicable. This logic is missing in the new implementation at lines 722-730, potentially causing the UI to remain on the moved item instead of advancing.

    val result = messagesActionsUseCase.moveThreadsOrMessagesTo(
        destinationFolderId = newFolderId,
        threadsUids = threadsUids,
        messagesUids = messagesUids,
        mailbox = mailbox,
        currentFolderId = currentFolderId,
    ) ?: run {
        snackbarManager.postValue(appContext.getString(RCore.string.anErrorHasOccurred))
        return@launch
    }

    with(result) {
        if (apiResponses.atLeastOneSucceeded()) {
            refreshFoldersAsync(
                mailbox = currentMailbox.value!!,
                messagesFoldersIds = messages.getFoldersIds(),
                destinationFolderId = newFolderId,
                threadsUids = threadsUids,
            )
            showMoveSnackbar(threadsUids.count(), messages, apiResponses, destinationFolder)
            isMovedToNewFolder.postValue(true)
        }

        if (apiResponses.atLeastOneFailed()) {
            threadController.updateIsLocallyMovedOutStatus(threadsUids, hasBeenMovedOut = false)
        }
    }
}
Redundant Variable Shadowing

Lines 604 and 622 contain redundant variable declarations val mailbox = mailbox that shadow the function parameters. This unnecessary code should be removed to improve readability and avoid confusion.

val mailbox = mailbox

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants