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.

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

fix no sent mailbox configured #8311

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix no sent mailbox configured #8311

wants to merge 5 commits into from

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Mar 28, 2023

Fix #4153
Repro:

  1. remove the default folder for sent ("you can do it in vuex by setting sentMailboxId to null")
  2. Try to send a message

Fix:
Ps: Other ideas are appreciated

  1. Check if a mailbox named Sent or equivalent in the local language
  • if it exists set it as Sent default folder and resend message, if not:
  1. create a new mailbox
  2. set it as send default folder
  3. resend the message
  4. Catch exceptions and inform the user with toasts accordingly

@JohannesGGE
Copy link
Contributor

Where exactly do you set sentMailboxId to null? (I can't get the error reproduced)

@hamza221
Copy link
Contributor Author

hamza221 commented Jul 19, 2023

Where exactly do you set sentMailboxId to null? (I can't get the error reproduced)

I do it in the vue.js devtools browser extension

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

The changes feel a bit too complex and messy. I think we should try to find a cleaner, less complicated spot to place the logic. I'll think about an alternative.

@ChristophWurst
Copy link
Member

The changes feel a bit too complex and messy. I think we should try to find a cleaner, less complicated spot to place the logic. I'll think about an alternative.

How about we just move the logic into the head of NewMessageModal::onSend? We have access to the accountId and can fetch the account, check for the mailbox existence and act, then proceed with the usual send logic.

The good news is that this isn't too far off the current state. The code just has to be pushed around a bit.

I also think we don't necessarily need a special loading text for this state. For the user it's sufficient if we show Sending … while creating/assigning the mailbox

@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
24 tasks
@hamza221 hamza221 marked this pull request as draft March 4, 2025 14:00
Signed-off-by: hamza221 <[email protected]>
Signed-off-by: Hamza Mahjoubi <[email protected]>
@hamza221 hamza221 force-pushed the bug/not-sent-mailbox branch from 3913ba7 to 40f8232 Compare March 7, 2025 16:37
@hamza221 hamza221 requested a review from ChristophWurst March 7, 2025 16:38
@hamza221 hamza221 marked this pull request as ready for review March 7, 2025 16:38
@hamza221 hamza221 requested a review from kesselb as a code owner March 7, 2025 16:38
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

The logical flow looks good otherwise

showWarning(t('mail', 'Setting Sent default folder'))
let newSentMailboxId = null
const mailboxes = this.mainStore.getMailboxes(data.accountId)
const sentMailboxId = mailboxes.find((mailbox) => mailbox.name === account.personalNamespace + 'Sent' || mailbox.name === account.personalNamespace + t('mail', 'Sent'))?.databaseId
Copy link
Member

Choose a reason for hiding this comment

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

nit. first look for the localized, then search for Sent. this ensures that the localized takes priority.

const mailboxes = this.mainStore.getMailboxes(data.accountId)
const sentMailboxId = mailboxes.find((mailbox) => mailbox.name === account.personalNamespace + 'Sent' || mailbox.name === account.personalNamespace + t('mail', 'Sent'))?.databaseId
if (sentMailboxId) {
await this.setSentMailboxAndResend(account, sentMailboxId, data)
Copy link
Member

Choose a reason for hiding this comment

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

I'm missing a return or anything else that leaves this method after setting the existing mailbox

}
logger.info(`creating ${t('mail', 'Sent')} mailbox`)
try {
const newSentMailbox = await this.mainStore.createMailbox({ account, name: account.personalNamespace + t('mail', 'Sent') })
Copy link
Member

Choose a reason for hiding this comment

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

if the IMAP server supports it (likely) we should also set the specialuse attribute to \sent

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.

"No sent mailbox configured" Error when sending a message
3 participants