-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: refactoring of alerts and send correct email alerts #1788
Conversation
3d360d5
to
c26c451
Compare
5df454b
to
85f7040
Compare
I am not convinced this is a refactor for alerts. As the main issues with the initial alerts implementation (being tightly coupled to the currently supported alerts, making it very hard to support any other alerting mechanism for now) are still there. This change should be considered as a bug fix and not a refactor (ie: maybe the title and the description of this pull request should be slightly changed). That being said, I still think that this pull request should be reviewed and merged, for the purpose of fixing the mentioned bugs, and to serve as a functional starting point for a refactor. Either way, I am not a maintainer, so make of my opinion what you want. |
The code have been still refactored, not just fixed. Is it more flexible than before? No, not yet. |
@tboerger did you test it? |
I have tested the mail alerts with mailhog as a target and it contains all the mandatory fields like To, Date and so on. |
Looks like I got to resolve conflicts anyway as the teams integration have been merged. |
Previously the sent email alerts have been missing mandatory headers like `Date` and it was also missing content type, content transfer encoding and mime version. I have taken proper examples form the unmaintained gomail library to build right emails. Besides that I have refactored the calls for alerts, they git the same structure now and it should be prepared to inject custom templates for all altering methods at some later point. Generally it is prepared for a more flexible alert handling.
85f7040
to
66ec7e7
Compare
66ec7e7
to
dba0b8e
Compare
Previously the sent email alerts have been missing mandatory headers like
Date
and it was also missing content type, content transfer encoding and mime version. I have taken proper examples form the unmaintained gomail library to build right emails.Besides that I have refactored the calls for alerts, they git the same structure now and it should be prepared to inject custom templates for all altering methods at some later point. Generally it is prepared for a more flexible alert handling.
Fixes #1782
Fixes #1627
Fixes #1571
Replaces #1771
Replaces #1636