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

Digest mode #87

Closed
wants to merge 41 commits into from
Closed

Digest mode #87

wants to merge 41 commits into from

Conversation

MattMofDoom
Copy link
Contributor

Seq.App.DigestEmail was based on Email+ but hasn't been updated since 2016.

It would be possible to update it to include improvements that have been added to Email+, but strictly speaking, this is probably not necessary and may not be desirable since it means maintaining two codebases with similar functionality. We could, instead, add digest mode as an optional configuration to Email+.

With this PR, if BatchTimeInSeconds is configured, an alternate default template will be used (the Seq.App.DigestEmail template) and the timer-based processing will kick in.

As this is a speculative PR, I haven't implemented unit tests. This is built on top of the enhancements for #83 for consistency with previous PRs.

…esharper code issues, add test case for envelope template parsing
Reflects that an alternate plain text body can be set
@MattMofDoom
Copy link
Contributor Author

@nblumhardt After looking at 2021.3, I'm not sure that this particular PR is needed, unless my comment on datalust/seq-tickets#1126 is relevant.

eg. if this is based on a dashboard alert, then you have this covered off by the 2021.3 changes for including contributing events. That might be enough, and I'm struggling to think of a reason that the digest email would need a 1:1 relationship to events on this basis - it seems like this fulfils the entirety of the old Seq.App.DigestEmail.

Hence if you agree, I can withdraw this PR. #81 and #83 remain current and I note we haven't had any more breaking changes lately that would prevent those PRs being merged once reviewed 😂

@nblumhardt
Copy link
Member

Thanks, Matt - makes sense to me 👍

@nblumhardt
Copy link
Member

(Sorry we've been so slow on getting back to these - you can tell RE the new Alerts feature that we've been busy 😅)

@MattMofDoom
Copy link
Contributor Author

@nblumhardt No problem at all - I've seen you guys are busy, and I was working in isolation for this PR so I'm really glad I had a play with 2021.3 👍

@MattMofDoom MattMofDoom closed this Oct 7, 2021
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.

2 participants