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

Add footer to all emails sent by Kibana email connector with a link to open Kibana or to the alert details page #84371

Merged
merged 24 commits into from
Dec 15, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Nov 25, 2020

Resolves #71243

In this PR, I'm making the following changes to allow alert emails to have a link back to the alert details page:

  • Email action type to always have a footer with a link back to Kibana
  • Email action type has a new parameter viewInKibanaPath which allows executors to pass a path within Kibana to use instead of the default root
  • Footer displays Click here to open Kibana. when the path is the default and Click here to view in Kibana. when a specific path is provided.
  • Alert transformActionParams function now injects viewInKibanaPath for email action types with a path to the alert details page

TODO:

Examples

Email sent by testing the connector Screen Shot 2020-11-30 at 2 40 41 PM
Email sent by an alert Screen Shot 2020-11-30 at 2 37 31 PM

Notes for testing

I've been using this app (https://nodemailer.com/app/) to test outbound emails.

Different scenarios

  • Not configuring server.publicBaseUrl and you will only see This message was sent by Kibana. footer when testing the connector.
  • Configuring server.publicBaseUrl to the URL you access Kibana from and you will see This message was sent by Kibana. Go to Kibana. footer when testing the connector and This message was sent by Kibana. View alert in Kibana. footer when an alert is sending an email.

Note for docs

  • Update documentation for email action params to mention the new property kibanaFooterLink with { path, text } as customization options.
  • A mention to the user they need to configure server.publicBaseUrl in order to get a link back to Kibana.

@mikecote mikecote added release_note:enhancement Feature:Alerting v8.0.0 Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 labels Nov 25, 2020
@mikecote mikecote self-assigned this Nov 25, 2020
@mikecote
Copy link
Contributor Author

@gchaps I'm working on adding a footer to all emails sent by our connectors. I'm thinking it would be good to link the user to which Kibana deployment sent the email and where possible have a deep link to a specific page.

I'm working on the messaging for each scenario, looking for some of your recommendations 🙏

When we have a link to a specific page:

  • This message was sent by a Kibana connector. Click here to view in Kibana.

When we don't have a link to a specific page but rather the Kibana URL:

  • This message was sent by a Kibana connector. Click here to open Kibana.

I'm using view when we have a link to a specific page (ex: from alerts) and open when we don't (ex: testing the connector). You can see examples w/ screenshots in the PR description.

@gchaps
Copy link
Contributor

gchaps commented Nov 25, 2020

@mikecote We avoid using "click here" for accessibility reasons. So I'd remove that wording and use this text as the link:

View in Kibana
Open Kibana

For the first link, is it possible to be more specific: View alert in Kibana

@mikecote
Copy link
Contributor Author

Thanks @gchaps! Absolutely, we can make the text more specific for alerts.

@mikecote
Copy link
Contributor Author

@gchaps I've made the changes, let me know what you think. I've updated the screenshots in the "Examples" section of the PR description.

@gchaps
Copy link
Contributor

gchaps commented Nov 30, 2020

@mikecote "Open Kibana" seems a little abrupt. Would this work "Open Kibana and view alert".

@mikecote
Copy link
Contributor Author

@gchaps I've updated the examples in the PR description based on our offline discussion:

  • This message was sent by Kibana.
  • Go to Kibana

Let me know what you think

Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text LGTM

@mikecote mikecote marked this pull request as ready for review December 14, 2020 19:17
@mikecote mikecote requested a review from a team as a code owner December 14, 2020 19:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested sending emails for alerts with and without the publicBaseUrl specified.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; posted a few notes/questions, none blockers.

tested all the varieties locally, works great!

@@ -30,6 +30,8 @@ export type EmailActionTypeExecutorOptions = ActionTypeExecutorOptions<
// config definition
export type ActionTypeConfigType = TypeOf<typeof ConfigSchema>;

const EMAIL_FOOTER_DIVIDER = '\n\n--\n\n';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way this ends up getting rendered is with an empty line, a line with --, and empty line. I'm wondering if the intent is to use markdown --- which renders a horizontal rule, instead of a line with --. Seems like this would look nicer. But also thinking two dashes by themselves is also some kind of default thing in email used before a "sig"? Anyhoo, example below of using the --- hr ...


This message was written by pmuellr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmuellr yeah, I was looking at a common way to do email footers and it seemed the "empty line, --, empty line" was fairly common (mostly the --) by GitHub and a lot personal email signatures. I'm good changing it to whatever is preferred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the hr-flavored one looks a little more "modern", but it doesn't matter that much to me.

@@ -101,6 +103,14 @@ const ParamsSchema = schema.object(
bcc: schema.arrayOf(schema.string(), { defaultValue: [] }),
subject: schema.string(),
message: schema.string(),
kibanaFooterLink: schema.object({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is slightly weird - action params that aren't really intended to be updated by the user.

I'm guessing this was the easiest way to get the value passed in, without making more widespread changes, like adding some additional "bag" of variables/strings all over the place.

Seems fine for now, maybe we'll figure out a cleaner way to do this later. I think it's probably worth a comment above kibanaFooterLink though, indicating we don't intend customers to be setting this value explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmuellr ++ I'll add a comment. I agree these values won't be set by end users. Though I can still see them as parameters set programatically elsewhere like "share to" where it's done browser side and want to put a link to the visualization.

I was thinking of waiting and seeing further patterns to know what is better than params for this. Right now it's almost like hidden-from-user-while-still-being-params.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I suspect there will be other patterns to deal with, and agree we can live with the design used in this PR till we need more. Some things already considered:

  • sending the results of a previous action into a subsequent action
  • allowing customer-specified messaging "prefixes" (legal statements, etc - this kind of thing always seems to come up)

At one point I was thinking that "message" parameters should maybe be considered a list of messages, independently settable, that we'd serialize into a single message. Something like Slack's BlockKit blocks.

text: 'email-message',
html: `<p>email-message</p>\n<p>--</p>\n<p>This message was sent by Kibana. <a href=\"https://localhost:5601\">Go to Kibana</a>.</p>\n`,
text:
'email-message\n\n--\n\nThis message was sent by Kibana. [Go to Kibana](https://localhost:5601).',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we can't build a FT that provides a link into the alerting app, since we have no way of validating what the emails that get sent actually contain, since we don't have an email emulator, yet. Good to see the link to Kibana though!

Or could/should we cheat, by passing in an explicit kibanaFooterLink param, in a new test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah since there's no emulator yet I went ahead and tested the params portion instead (commit: 99b5cad).

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested and it works as expected

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47241 48002 +761

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mikecote mikecote merged commit ccfb4b6 into elastic:master Dec 15, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Dec 15, 2020
…o open Kibana or to the alert details page (elastic#84371)

* Initial work

* Change messaging from copy

* Fix jest tests for email connector

* Fix jest tests for alerts plugin

* Update copy

* Use server.publicBaseUrl

* Fix jest tests

* Update tests

* Cleanup jest test

* Code cleanup

* Improve email parameter names for kibana footer url

* Cleanup

* Add test for kibana footer link

* Fix type check

* Fix jest test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@pmuellr
Copy link
Member

pmuellr commented Dec 16, 2020

so nice to see this merged! \o/

mikecote added a commit that referenced this pull request Dec 16, 2020
…o open Kibana or to the alert details page (#84371) (#86003)

* Initial work

* Change messaging from copy

* Fix jest tests for email connector

* Fix jest tests for alerts plugin

* Update copy

* Use server.publicBaseUrl

* Fix jest tests

* Update tests

* Cleanup jest test

* Code cleanup

* Improve email parameter names for kibana footer url

* Cleanup

* Add test for kibana footer link

* Fix type check

* Fix jest test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Actions Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emails sent from Kibana Alerting to have a footer with a link back to the alert details
7 participants