Add support for a default alert template

[eradman]

Description

Add support for a default alert template

• Escape all variables by default since Mustache only has a syntax for raw values
• Generate a generic HTML result table to allow an alert template to display any query
• Optionally allow alert template to be defined to be set using REDASH_ALERTS_DEFAULT_MAIL_BODY_TEMPLATE

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[guidopetri]

@eradman thanks for your contribution! Would you mind rebasing on master and pushing to rerun CI checks?

[guidopetri]

This LGTM! Would you mind also opening a PR for docs for REDASH_ALERTS_DEFAULT_MAIL_BODY_TEMPLATE in this repo/file?

cc @justinclift

[eradman]

Certainly! Proposed an update to the docs in getredash/website#673

[junnplus]

We should have test coverage for them.

[eradman]

I will write some test cases update this PR

[justinclift]

Awesome. 😄

[eradman]

I added tests to verify that mustache variables are rendered correctly in the context of an alert. Early next week I'll review this change again and see if there are other tests that should be included.

[justinclift]

Thanks @eradman, sounds like a plan. 😄

[eradman]

There does not seem to be an existing set of test cases for redash/destinations/*, so I did not write a test for the change to the email destination itself.

I would say that the mustache template rendering is covered reasonably.

[eradman]

We're getting closer: I was able to refactor this so that the query contents are rendered in the template.

Also access to MailDev was broken: we need to map port 1025 and make sure Redash uses that port to send mail. I can push this as a separate commit.

[justinclift]

Also access to MailDev was broken: we need to map port 1025 and make sure Redash uses that port to send mail. I can push this as a separate commit.

As a thought for this, please make sure it's not something too hard coded. Not everyone needs to use the docker container for sending emails.

eg in the production instances I manage, we just have things going directly via SMTP2GO, no docker container needed

[eradman]

As a thought for this, please make sure it's not something too hard coded. Not everyone needs to use the docker container for sending emails.

In this case, docker-compose.yml is only used for testing

# This configuration file is for the **development** setup.
# For a production example please refer to getredash/setup repository on GitHub.

[justinclift]

Cool. 😄

[eradman]

Fixed flake8 and rebased

[eradman]

Fixed black, isort warnings

[justinclift]

@eradman black still seems unhappy?

[eradman]

Oh because I was using black-21 (from Ubuntu 22). Upgraded to black-23 ran format again

[codecov]

Codecov Report

Merging #5996 (b02d31c) into master (1e33eee) will increase coverage by 0.06%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #5996      +/-   ##
==========================================
+ Coverage   59.91%   59.97%   +0.06%     
==========================================
  Files         153      153              
  Lines       12462    12470       +8     
  Branches     1684     1686       +2     
==========================================
+ Hits         7466     7479      +13     
+ Misses       4790     4783       -7     
- Partials      206      208       +2     

Impacted Files	Coverage Δ
redash/destinations/email.py	41.93% <0.00%> (-1.40%)	⬇️
redash/models/__init__.py	91.95% <100.00%> (+0.81%)	⬆️
redash/settings/__init__.py	98.65% <100.00%> (+<0.01%)	⬆️
redash/utils/__init__.py	71.75% <100.00%> (+0.66%)	⬆️

[justinclift]

@eradman It looks like this is pretty much ready to merge yeah?

Also, is there a clear way forward for this?

Also access to MailDev was broken: we need to map port 1025 and make sure Redash uses that port to send mail. I can push this as a separate commit.

Personally speaking, I'm not bothered either way if the port mapping is a separate commitPR or in this one. There are advantages (integrated testing of both at once) and disadvantages (all commits get squashed into one) either way, so your choice I reckon. 😄

[eradman]

This is ready to to go. I manually confirmed that email alerts are formatted properly and I'd say the implementation is in good shape.

I created PR #6186 with the changes to the docker port mapping. I believe if we merge that first this change will apply cleanly.

[justinclift]

Cool. Just merged the other one, so I'll merge this after the CI tests all pass again. 😄

[justinclift]

Cool, all done. This seems like it'll be a useful addition for people @eradman. 😄

[eradman]

Indeed; I'm sure others will find this change useful. Thanks for mentoring this along @justinclift @guidopetri @junnplus