Closed MattMofDoom closed 2 years ago
@nblumhardt After looking at 2021.3, I'm not sure that this particular PR is needed, unless my comment on https://github.com/datalust/seq-tickets/issues/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 😂
Thanks, Matt - makes sense to me 👍
(Sorry we've been so slow on getting back to these - you can tell RE the new Alerts feature that we've been busy 😅)
@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 👍
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.