Skip to content

Allow null message in SlackMessage #31288

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Allow null message in SlackMessage #31288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c8e69c
Make text message not required in constructor for slack
albendz Jun 13, 2018
b6876b4
Remove unnecessary comments in test file
albendz Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions ...rc/main/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,16 @@ public class SlackMessage implements MessageElement {
final String from;
final String[] to;
final String icon;
final String text;
final Attachment[] attachments;
String text;
Attachment[] attachments;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for making these non final? Our objects are typically final. And assigning null to these still counts as making sure they are final. It is very atypical for us to assign setters to objects like this to set these values after construction of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can correct this. There's no reason for them not to be final.


public SlackMessage(String from, String[] to, String icon) {
this(from, to, icon, null, null); // Empty text and empty attachments
}

public SlackMessage(String from, String[] to, String icon, String text) {
this(from, to, icon, text, null); // Empty attachments
}

public SlackMessage(String from, String[] to, String icon, String text, Attachment[] attachments) {
this.from = from;
Expand Down Expand Up @@ -57,6 +65,10 @@ public Attachment[] getAttachments() {
return attachments;
}

public void setText(String text) { this.text = text;}

public void setAttachments(Attachment[] attachments) { this.attachments = attachments;}

Copy link
Contributor

Choose a reason for hiding this comment

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

see previous comment about final.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@Override
public boolean equals(Object o) {
if (this == o) return true;
Expand Down Expand Up @@ -202,7 +214,7 @@ public SlackMessage render(String watchId, String actionId, TextTemplateEngine e
attachments.addAll(dynamicAttachments.render(engine, model, defaults.attachment));
}
if (attachments == null) {
return new SlackMessage(from, to, icon, text, null);
return new SlackMessage(from, to, icon, text);
}
return new SlackMessage(from, to, icon, text, attachments.toArray(new Attachment[attachments.size()]));
}
Expand Down
7 changes: 6 additions & 1 deletion ...st/java/org/elasticsearch/xpack/watcher/notification/slack/message/SlackMessageTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ public void testUrlPathIsFiltered() throws Exception {
HttpResponse response = new HttpResponse(500);
String path = randomAlphaOfLength(20);
HttpRequest request = HttpRequest.builder("localhost", 1234).path(path).build();
SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text", null);
SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon", "text");
SentMessages sentMessages = new SentMessages("foo",
Arrays.asList(SentMessages.SentMessage.responded("recipient", slackMessage, request, response)));

Expand All @@ -602,6 +602,11 @@ public void testUrlPathIsFiltered() throws Exception {
}
}

public void canHaveNullTextAndAttachment() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will not get executed as a test if the method does not begin with test

SlackMessage slackMessage = new SlackMessage("from", new String[] {"to"}, "icon");
assertThat(slackMessage.getText(), is(null));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is causing a compile error for me, pls use assertNull(object) so it compiles clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

assertThat(slackMessage.getAttachments(), is(null));
}
private static void writeFieldIfNotNull(XContentBuilder builder, String field, Object value) throws IOException {
if (value != null) {
builder.field(field, value);
Expand Down