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] Direct Reply #22588

Merged
merged 10 commits into from
Jun 29, 2022
Merged

[FIX] Direct Reply #22588

merged 10 commits into from
Jun 29, 2022

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented Jul 6, 2021

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

https://docs.rocket.chat/guides/administration/administration/settings/email/direct-reply

Further comments

@CLAassistant
Copy link

CLAassistant commented Dec 17, 2021

CLA assistant check
All committers have signed the CLA.

@ggazzo ggazzo requested a review from a team April 5, 2022 01:06
@ggazzo ggazzo self-assigned this Apr 5, 2022
@ggazzo ggazzo added this to the 4.7.0 milestone Apr 5, 2022
@d-gubert d-gubert modified the milestones: 4.7.0, 4.8.0 Apr 22, 2022
@d-gubert d-gubert modified the milestones: 4.8.0, 4.9.0 May 24, 2022
@ggazzo ggazzo marked this pull request as ready for review June 28, 2022 12:32
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.

Most of my comments are on console.xxx usage, maybe we can create a logger for that EmailInterceptor.

From Omnichannel, the changed file makes sense and seems to be working without issues.

}
}

export class POP3Intercepter {
constructor() {
this.pop3 = new POP3Lib(settings.get('Direct_Reply_Port'), settings.get('Direct_Reply_Host'), {
enabletls: !settings.get('Direct_Reply_IgnoreTLS'),
debug: settings.get('Direct_Reply_Debug') ? console.log : false,
// debug: settings.get('Direct_Reply_Debug') ? console.log : false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to keep the comment? 👀

}),
);
this.pop3.on('connect', () => {
console.log('Pop connect');
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 could be changed by an actual logger instead of console, same for debug

@ggazzo ggazzo merged commit f7519a9 into develop Jun 29, 2022
@ggazzo ggazzo deleted the fix/direct-reply branch June 29, 2022 14:04
@murtaza98 murtaza98 mentioned this pull request Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants