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

feat: enhance email notification to send alerts to multiple recipients #174

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

bbarman4u
Copy link

@bbarman4u bbarman4u commented Sep 18, 2022

This PR enhances the email notification functionality where email alerts can now take a set of email addresses separated by comma.
This is related to this github issue #171

Email alerts can take a set of email addresses separated by comma
@jemand771
Copy link
Member

jemand771 commented Sep 20, 2022

after staring at this for a bit, I think the use of FormatAddress narrows down what you can put into statping's email field.
previously, you could put any of these:

willy@jemand771.net
<willy@jemand771.net>
Willy <willy@jemand771.net>

and it would just set the header to that. according to RFC5322, section 3.4, all of those are valid email header contents.

with FormatAddress, users may only put in an email address like willy@jemand771.net, but the "full format" is no longer supported.
not sure if that's realistically gonna be a problem, but I'd rather not make a breaking change for a simple feature.

also see FormatAddress's source - it doesn't even do anything when you pass in an empty name :D

Copy link
Member

@jemand771 jemand771 left a comment

Choose a reason for hiding this comment

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

I haven't tested this at all yet, but I like the suggestion (with some minor changes :b)

.gitignore Outdated Show resolved Hide resolved
Comment on lines 168 to 173
tempSplit := strings.Split(email.To, ",")
addresses := make([]string, len(tempSplit))
for i := range addresses {
addresses[i] = m.FormatAddress(tempSplit[i], "")
}
m.SetHeader("To", addresses...)
Copy link
Member

Choose a reason for hiding this comment

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

haven't tested this, I'm thinking of something like

Suggested change
tempSplit := strings.Split(email.To, ",")
addresses := make([]string, len(tempSplit))
for i := range addresses {
addresses[i] = m.FormatAddress(tempSplit[i], "")
}
m.SetHeader("To", addresses...)
addresses := strings.Split(email.To, ",")
m.SetHeader("To", addresses...)

(see my long comment from earlier)

Copy link
Author

Choose a reason for hiding this comment

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

@jemand771 Thanks for the suggestions. I did get to test your proposed changes as well as the earlier version, both of the cases it does work fine with various email formats, however since your proposed changes are more succinct I agree it makes more sense to use that version. Thank you again for helping to review this.

Copy link
Author

Choose a reason for hiding this comment

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

@jemand771 Please review the updated PR.

@jemand771 jemand771 linked an issue Sep 20, 2022 that may be closed by this pull request
@jemand771 jemand771 added the Improvement An Improvement of an already implemented feature label Sep 20, 2022
@jemand771 jemand771 self-requested a review October 24, 2022 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement An Improvement of an already implemented feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: SMTP Email Notification multiple email addresses
2 participants