Alert redesign #4 - custom notification template

[ranbena]

• Refactor
• Feature

Description

Adds the previously hidden feature of custom subject/body (#3137) for alert notifications.

Now added to Alert edit mode:

Switched to preview mode:

And in Alert page view mode:

How it looks on webmail:

How it looks on slack:

[ranbena]

@arikfr @rauchy I'd like to test the python changes to templating in this PR - for instance, get an email alert notification to mail or slack. Is that possible?

[arikfr]

I know it's a draft, but noticed a few things while reading.

[arikfr]

Is that possible?

Locally?

[ranbena]

Locally?

Yes. If not, maybe in deploy preview somehow?

[rauchy]

Is that possible?

@ranbena #4173 will help you with this.

[kravets-levko]

Could it be an issue in terms of backward compatibility (replacing template option with custom_body - and for subject as well - what if somebody already has saved alerts with template option)?

Also - shouldn't it be self.options.get('custom_body', '') as before?

[arikfr]

Could it be an issue in terms of backward compatibility

There are several backward incompatible changes in here (like the template variables). Because it's hidden behind a feature flag, I doubt many people use it.

But to be friendlier to them I suggest we keep the option name as template? As for the variables, we will document the change in the release notes.

[ranbena]

Could it be an issue in terms of backward compatibility

This feature was off since it landed, so I'm assuming that chances are slim that someone changed clientConfig.extendedAlertOptions apart for the original author @k-tomoyasu.

Maybe I can do this?:

# backwards compatibility
def template(self):
  return self.custom_body;

Also - shouldn't it be self.options.get('custom_body', '') as before?

render_template() now returns '' if custom_body us empty. Don't think it's enough?

[kravets-levko]

custom_body feels a better name for this option than template - so if you think it's not an issue - I'd prefer new names as well.

About template variables: do we introduce them (or some new ones), or rename/remove some existing?

[ranbena]

About template variables: do we introduce them (or some new ones), or rename/remove some existing?

I renamed the original 3:

redash/client/app/services/alert-template.js

Lines 6 to 10 in cbfd25f

	const view = {
	state: alert.state,
	rows: queryResult.rows,
	cols: queryResult.columns,
	};

[arikfr]

render_template() now returns '' if custom_body us empty. Don't think it's enough?

I think @kravets-levko was referring to the case when self.options doesn't have the custom_body key (every existing alert). I address this in my example below. If you access self.options['custom_body'] (or custom_subject) elsewhere in the code, you need to add similar treatment.

Maybe I can do this?:

This won't help. What might happen is that a user will have an existing alert object with the template key in options, while you look for custom_body. To make it backward compatible you can change the custom_body property method into something like:

 def custom_body(self):
        template = self.options.get('custom_body', self.options.get('template'))
        return self.render_template(template)

This will look in the custom_body key but if not found will give a try at looking in template. If not found it will return None, so you need to update render_template as well (to return empty if it's None).

Btw, shouldn't the configuration key be custom_body_template?

[ranbena]

@arikfr does this suffice? 5f8634a