FEATURE: Implement support for IMAP and SMTP email protocols. (PR #8301)

Co-authored-by: Joffrey JAFFEUX j.jaffeux@gmail.com


You’ve signed the CLA, udan11. Thank you! This pull request is ready for review.

Can you import @ember/component please? It’s our new standard way of doing this.

Also super minor but you can Component.extend() without the empty object.

This is a huge one, but overall is looking great. I would like some changes and for @SamSaffron to review it too.

This can also be imported from discourse/routes/discourse

Did we mean to leave this in? Usually debugging information like this behind some kind of flag or setting.

obj here is fairly generic. I would prefer group: or target: to be more consistent with the rest of our codebase.

I dislike boolean parameters like this, because when you read the code at the calling point it looks like this:

open_mailbox('my_mailbox', true) and it’s totally unclear what true means.

Instead we should use a keyword argument like open_mailbox('my_mailbox', write: true) which is self documenting.

Personally I am not a fan of using threads, as historically they have been a source of hard to track down bugs and are very hard to get right. Having said that, if this was discussed outside of this PR and this was the conclusion that is OK, but I would like someone else to review it who has more experience such as @SamSaffron.

Might be able to simplify this: @imap ||= super.tap { |imap| apply_gmail_patch(imap) }

The logic is a little hard to follow so can you clarify something for me here? Does this patch the Imap class, so that every instance of the imap will now be patched for gmail? Or does it only patch the specific instance?

I believe that production instances usually have logging level set to >= warn so it is not an issue and is super handy in development.

Yes, it opens the metaclass (/ eigenclass / singleton class) of the parser instance to add support for Gmail’s IMAP extensions (X-GM-* attributes).

I agree that obj is generic, but I believe that is how it is supposed to be, because type specifies what obj is.

I think I can just pass @group and perform a type check with is_a?, but will have to do a quick benchmark first because I have a feeling it will be a lot slower.

Sure, that’s fine with me.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

Is this being used? I don’t see a controller action for it :thinking:


Just checking… is it safe to run multiple Demon::EmailSync at the same time? It looks like there is one Demon per host, which means we will have multiple instances running against the same database.

1 Like

Should we check SiteSetting.imap_enabled here?

1 Like

Can you instead add a method admin_attributes which would be similar to staff_attributes in order to reduce all these lines into

admin_attributes smtp_server, smtp_port, ....