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

Removed invalid options for the sendmail transport #8576

Merged
merged 2 commits into from Aug 26, 2020

Conversation

kylefarris
Copy link
Contributor

Removed invalid options for the sendmail transport that were causing Nodemailer to return the SMTPTransport instead.

Fixes #8575

…Nodemailer to return the SMTPTransport instead.
@CLAassistant
Copy link

CLAassistant commented Aug 23, 2020

CLA assistant check
All committers have signed the CLA.

@pitaj
Copy link
Contributor

pitaj commented Aug 24, 2020

I think these should be moved to apply to the SMTP transport instead, not removed entirely.

Also, just removing them from the code doesn't remove the options from the admin menu.

@kylefarris
Copy link
Contributor Author

Before we add those options to the SMTP transport, we should consider that Nodemailer has deprecated the options and will be removing them in future updates: https://nodemailer.com/smtp/pooled/#deprecated-options

I can still make the swap if you'd like, but, we could also just remove the options from the interface entirely. At the very least, a note should be made in the code about the deprecation.

@pitaj
Copy link
Contributor

pitaj commented Aug 24, 2020

Ah, in that case, remove rateLimit and rateDelta entirely, and add pool to the SMTP options. Maybe even make it an nconf option.

Regardless, you'll need to make changes to the admin templates and translations to remove the sendmail options.

… option for toggling whether to use pooled connections.
@kylefarris
Copy link
Contributor Author

No problem. I removed the deprecated options from the admin interface and added a new one for toggling pooled connections for the SMTP Transport. Tested on my dev machine and it all looks good there. Tests passed on there as well.

@barisusakli
Copy link
Member

LGTM

@barisusakli barisusakli added this to the 1.15.0 milestone Aug 26, 2020
@julianlam julianlam merged commit 2b78562 into NodeBB:master Aug 26, 2020
@julianlam julianlam self-assigned this Aug 26, 2020
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.

Sendmail is Basically an Impossibility Right Now
5 participants