FEATURE: Use group SMTP job and mailer instead of UserNotifications change (PR #13489)

This PR backtracks a fair bit on this one FEATURE: Use group SMTP settings for sending user notification emails (initial) by martin-brennan · Pull Request #13220 · discourse/discourse · GitHub.

Instead of sending the group SMTP email for each user via UserNotifications, we are changing to send only one email with the existing Jobs::GroupSmtpEmail job and GroupSmtpMailer. We are changing this job and mailer along with PostAlerter to make the first topic allowed user the to_address for the email and any other topic_allowed_users to be the CC address on the email. This is to cut down on emails sent via SMTP, which is subject to daily limits from providers such as Gmail. We log these details in the EmailLog table now.

In addition to this, we have changed PostAlerter to no longer rely on incoming email email addresses for sending the GroupSmtpEmail job. This was unreliable as a user’s email could have changed in the meantime. Also it was a little overcomplicated to use the incoming email records – it is far simpler to reason about to just use topic allowed users.

Most importantly this PR contains breaking changes for IMAP functionality. IMAP will no longer work after this change. This is because we are no longer making outbound IncomingEmail records in the GroupSmtpMailer. This IMAP functionality needs to be revisited at a later date. I think the best thing to do would be to have a table called ExternalEmailMetadata or something similar, that will store imap UIDs, message IDs, and possibly even provider specific data like gmail thread IDs. This breaking change has been blessed by @SamSaffron.


Ember CLI failures are completely unrelated

@udan11 can you review this change?

The EmailLog record created by Email::Sender will not work very well because we are deleting logs that are over 3 months old (configurable, see delete_email_logs_after_days).

Maybe I am having a wrong perspective but, to my eyes, IMAP pairs with IncomingEmail and SMTP pairs with EmailLog. Following this logic, any email fetched by IMAP is an incoming email and we should have a record for it.

If you want to continue with this, does it make sense to have a migration that deletes IncomingEmail records that were created as “placeholders”? This is just for the consistency and it might avoid future headaches.

I think this will do the trick:

DELETE FROM incoming_emails
USING posts
WHERE incoming_emails.post_id = posts.id AND posts.via_email = FALSE

This looks good to me, but to be honest I do not understand why breaking IMAP is necessary and I am not comfortable leaving broken code in the codebase. If @SamSaffron is ok with that, then it’s fine.

I think if we continue creating IncomingEmail records then IMAP will continue to work normally, right? Is there a reason why creating IncomingEmail does not let you to send an email to multiple recipients?

I could swear that this is the way I originally implemented this.

There is chance this will cause issues to email clients. I think many email clients use the subject to group multiple emails in a thread. If you use the topic title and that is edited, it will create a new email thread because subjects will no longer match.

This is interesting. Out of curiosity, why did it need IMAP to be enabled?

I wonder, cannot you just put all emails in cc and leave to field empty? I think that is valid.

Also, is there a reason why you prefer cc over bcc? I think we should keep users email addresses private.

I think we (or just me) had enabled it because this was IMAP only functionality before but it is not now.

I think it makes more sense to send a to email to the person that opened it and cc everyone else, while it might be valid to just cc everyone. Also I don’t think there is any issue showing the emails of everyone else. This feature is intended to be used for group inboxes; everyone cc’d on that topic should know each other and be comfortable with emails showing. We have discussed this internally, Zendesk does the same thing.

Hmm good point, this may be (possibly?) causing some issues with the sent emails not showing email. Will put this back, and just use the incoming email from the OP.

@udan11 After considering what you have said, I am going to add the IncomingEmail creation back to the group SMTP job and remove the broken IMAP notices. I think I just got quite confused being in this code so much lately. You’re right that even though the group SMTP email is outgoing, it is correct to create the incoming email record because all emails synced into the system from IMAP are technically “incoming”.

Thanks for pushing back on this. We all need external eyes and brains sometimes!

@udan11 can you please take one more look now?

Looks good to me, great job! :+1: