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] Correct IMAP configuration for email inbox #25789

Merged
merged 58 commits into from
Aug 25, 2022

Conversation

cauefcr
Copy link
Contributor

@cauefcr cauefcr commented Jun 7, 2022

Proposed changes (including videos or screenshots)

The primary change here has been to make the library try and reconnect after some time, up to a certain configured number of times, on a few different error classes.

Issue(s)

Steps to test or reproduce

Enable Omnichannel, select something as livechat routing strategy.
Administration -> Users -> click your username -> make sure your e-mail is verified.
Administration -> Email Inbox -> NEW -> fill everything up -> Save
Test the email by clicking on the arrow icon, and checking if it arrives on your email.
If it does, send an e-mail to the configured e-mail from another e-mail account.
The e-mail should arrive as a new visitor with a little envelope icon on the sidebar.
Click on the new visitor -> Reply via email -> type something and press enter
The response should arrive on the visitor's email.

Further comments

Email can be finicky to get working, so here a few specific tricks to get it to run:
gmail: enable 2-factor-authentication, enable an app password called 'Rocket.Chat', and use that as the password on the inbox.
outlook: really dislikes our SMTP SSL connection, SSL must be disabled to work with outlook/office365.

This list is compatible with our SMTP library https://nodemailer.com/smtp/well-known/ and provides working configurations for known hosts, comparing it with this list https://www.arclab.com/en/kb/email/list-of-smtp-and-imap-servers-mailserver-list.html makes it easier to test different email services.

P.S.: Other e-mail providers may need app passwords like gmail does.

@cauefcr cauefcr requested a review from a team June 7, 2022 13:38
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
@cauefcr cauefcr requested a review from a team as a code owner June 16, 2022 15:46
@cauefcr cauefcr requested a review from KevLehman June 16, 2022 15:57
Copy link
Contributor

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

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

If the configurations are actually invalid, would be good to avoid keeping the loop of retries. Is there any data passed to the close event we can use? 👀 like the reason for it to be closing, so we can decide if we want to reconnect or not

apps/meteor/server/email/IMAPInterceptor.ts Outdated Show resolved Hide resolved
apps/meteor/server/email/IMAPInterceptor.ts Show resolved Hide resolved
@cauefcr cauefcr requested a review from a team as a code owner June 20, 2022 18:17
@cauefcr cauefcr requested a review from murtaza98 June 20, 2022 18:24
Copy link
Contributor

@murtaza98 murtaza98 left a comment

Choose a reason for hiding this comment

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

@cauefcr Can you also check the builds, please? It seems to be failing because of some recent changes

Also, I created this new PR on top of your PR to migrate the logger which was getting used on the email feature. Do you mind reviewing it?

murtaza98
murtaza98 previously approved these changes Jun 23, 2022
Copy link
Contributor

@murtaza98 murtaza98 left a comment

Choose a reason for hiding this comment

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

LGTM!

KevLehman
KevLehman previously approved these changes Aug 19, 2022
gabriellsh
gabriellsh previously approved these changes Aug 19, 2022
Copy link
Member

@gabriellsh gabriellsh left a comment

Choose a reason for hiding this comment

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

Front-End LGTM!

@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Aug 22, 2022
@cauefcr cauefcr dismissed stale reviews from gabriellsh and KevLehman via 77f724c August 22, 2022 13:54
@casalsgh casalsgh requested a review from ggazzo August 23, 2022 19:25
@kodiakhq kodiakhq bot merged commit 1de809a into develop Aug 25, 2022
@kodiakhq kodiakhq bot deleted the fix/imap-breaking-configuration-change branch August 25, 2022 14:59
gabriellsh added a commit that referenced this pull request Aug 26, 2022
…hreads

* 'develop' of github.com:RocketChat/Rocket.Chat: (93 commits)
  Chore: Upgrade dependencies (#26694)
  Chore: More Omnichannel tests (#26691)
  Regression: Banner - Room not found - Omnichannel room (#26693)
  [NEW] Capability to search visitors by custom fields (#26312)
  Chore: Create tests for Omnichannel admin add a custom fields (#26609)
  [FIX] Avatars of other chats disappear when they located near chat with broken avatar (#26689)
  [IMPROVE] Added identification on calls to/from existing contacts (#26334)
  Regression: invalid statistics format  (#26684)
  Regression: "Cache size is not a function" error when booting (#26683)
  [FIX] Correct IMAP configuration for email inbox (#25789)
  [FIX] Active users count on `@all` and `@here`  (#25957)
  [FIX] Autotranslate method should respect setting (#26549)
  Chore: Remove italic/bold font-style from system messages (#26655)
  Chore: Convert AppSetting to tsx (#26625)
  Chore: Remove & Test old closeChat templates (#26631)
  [IMPROVE] General federation improvements (#26150)
  [NEW] Warn admins about running multiple instances of the monolith (#26667)
  Regression: Prevent message from being temp forever (#26668)
  Regression: Add alsoSendThreadToChannel to user settings api (#26663)
  [IMPROVE] Spotlight search user results (#26599)
  ...
csuadev pushed a commit that referenced this pull request Aug 26, 2022
Co-authored-by: Murtaza Patrawala <[email protected]>
Co-authored-by: Kevin Aleman <[email protected]>
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants