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

Watcher reporting: add email warning if CSV attachment contains values that may be interperted as formulas #44460

Merged
merged 6 commits into from Aug 14, 2019

Conversation

@jakelandis
Copy link
Contributor

commented Jul 16, 2019

This commit introduces a Warning message to the emails generated by Watcher's reporting action. This change complements Kibana's CSV formula notifications (see elastic/kibana#37930). This is implemented by reading a header (kbn-csv-contains-formulas) provided by Kibana to notify to attach the Warning to the email. The wording of the warning is borrowed from Kibana's UI and may be overridden by a dynamic setting (xpack.notification.reporting.warning.kbn-csv-contains-formulas.text). This warning is enabled by default, but may be disabled via a dynamic setting xpack.notification.reporting.warning.enabled.

Note - the doc PR will need to be separate since the majority of this is documented included from the Kibana docs.

Kibana

image

image

Watcher email - HTML

image

Watcher email - Text

image

@jakelandis jakelandis added the WIP label Jul 16, 2019

@jakelandis jakelandis changed the title initial commit for watcher reporting csv email warning Watcher reporting: add email warning if CSV attachment contains values that may be interperted as formulas Jul 16, 2019

@@ -44,7 +44,7 @@ dependencies {

testCompile 'org.subethamail:subethasmtp:3.1.7'
// needed for subethasmtp, has @GuardedBy annotation
testCompile 'com.google.code.findbugs:jsr305:3.0.1'
testCompile 'com.google.code.findbugs:jsr305:3.0.2'

This comment has been minimized.

Copy link
@jakelandis

jakelandis Jul 16, 2019

Author Contributor

this is to avoid JAR hell when running tests via IntelliJ (nothing specific to the changes here)

@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2019

@jakelandis jakelandis requested review from hub-cap and joelgriffith Jul 16, 2019

@jakelandis jakelandis marked this pull request as ready for review Jul 16, 2019

@joelgriffith

This comment has been minimized.

Copy link

commented Jul 22, 2019

LGTM!

}
}

This comment has been minimized.

Copy link
@jakelandis

jakelandis Jul 25, 2019

Author Contributor

I just realized, that despite the current documenation, body is not actually required, will need to adjust this to always emit a warning even if the body is not defined.

This comment has been minimized.

Copy link
@jakelandis

jakelandis Aug 7, 2019

Author Contributor

done in 0acff82

jakelandis added some commits Aug 7, 2019

@hub-cap
Copy link
Contributor

left a comment

Super small nits. great stuff. LGTM.


import static javax.mail.Part.ATTACHMENT;
import static javax.mail.Part.INLINE;

public abstract class Attachment extends BodyPartSource {

private final boolean inline;
private final Set<String> warnings;

protected Attachment(String id, String name, String contentType, boolean inline) {

This comment has been minimized.

Copy link
@hub-cap

hub-cap Aug 13, 2019

Contributor

is this constructor used anymore? Im ok w/ removing it if we dont use it anywhere. Also, if we dont want a NPE down in the EmailTemplate we should check for null assignment in the other constructor, maybe with a Objects.requireNonNull. Otherwise the user is free to put null there and receive a nice NPE during warnings.addAll(attachment.getWarnings()); below. Ill add a note with here is the NPE im talking about.

This comment has been minimized.

Copy link
@jakelandis

jakelandis Aug 14, 2019

Author Contributor

Yeah, this constructor is still used in some cases.

I added an assert warnings != null, since this is an internal API, the variable is final, means that someone working on Watcher internals would be required to explicitly call the second constructor with null, I think the assert is sufficient to express "don't do that". ed5128f

if (attachments != null) {
for (Attachment attachment : attachments.values()) {
builder.attach(attachment);
warnings.addAll(attachment.getWarnings());

This comment has been minimized.

Copy link
@hub-cap

hub-cap Aug 13, 2019

Contributor

here is the NPE im talking about

jakelandis added some commits Aug 14, 2019

@jakelandis jakelandis merged commit b28f089 into elastic:master Aug 14, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docs Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jakelandis jakelandis deleted the jakelandis:watcher_reporting_email_warning branch Aug 14, 2019

jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Aug 14, 2019

Watcher add email warning if CSV attachment contains formulas (elasti…
…c#44460)

This commit introduces a Warning message to the emails generated by 
Watcher's reporting action. This change complements Kibana's CSV 
formula notifications (see elastic/kibana#37930). 

This is implemented by reading a header (kbn-csv-contains-formulas) 
provided by Kibana to notify to attach the Warning to the email. 
The wording of the warning is borrowed from Kibana's UI and may 
be overridden by a dynamic setting
xpack.notification.reporting.warning.kbn-csv-contains-formulas.text.
This warning is enabled by default, but may be disabled via a 
dynamic setting xpack.notification.reporting.warning.enabled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.