Puts more emphasis on the commit title in published messages
- Alters the message formatting to put more emphasis on the commit
title.
- Adds a new title field on the MessageTemplate Class.
- Uses StringUtils to determine the title and body of a commit
message.
- Renames escape to clean to reflect additional functionality.
- Misc. refactoring.
Change-Id: I07dc5cb430c714f8a357f031f7ae754a530649cf
diff --git a/src/main/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGenerator.java b/src/main/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGenerator.java
index 85d75bb..9b6c052 100644
--- a/src/main/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGenerator.java
+++ b/src/main/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGenerator.java
@@ -22,6 +22,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.commons.lang.StringUtils.substringBefore;
+
/**
* A specific MessageGenerator implementation that can generate a message for
* a change merged event.
@@ -81,7 +83,7 @@
template.setProject(event.change.get().project);
template.setBranch(event.change.get().branch);
template.setUrl(event.change.get().url);
- template.setMessage(event.change.get().commitMessage.split("\n")[0]);
+ template.setTitle(substringBefore(event.change.get().commitMessage, "\n"));
message = template.render();
}
diff --git a/src/main/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGenerator.java b/src/main/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGenerator.java
index 663d154..6dff8c4 100644
--- a/src/main/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGenerator.java
+++ b/src/main/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGenerator.java
@@ -18,11 +18,12 @@
package com.cisco.gerrit.plugins.slack.message;
import com.cisco.gerrit.plugins.slack.config.ProjectConfig;
-import com.google.common.base.Ascii;
import com.google.gerrit.server.events.CommentAddedEvent;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.commons.lang.StringUtils.substringBefore;
+
/**
* A specific MessageGenerator implementation that can generate a message for
* a comment added event.
@@ -71,6 +72,9 @@
String message;
message = "";
+ LOGGER.info(substringBefore(event.change.get().commitMessage, "\n"));
+ LOGGER.info(event.comment);
+
try
{
MessageTemplate template;
@@ -79,11 +83,12 @@
template.setChannel(config.getChannel());
template.setName(event.author.get().name);
template.setAction("commented on");
- template.setNumber(event.change.get().number);
template.setProject(event.change.get().project);
template.setBranch(event.change.get().branch);
template.setUrl(event.change.get().url);
- template.setMessage(Ascii.truncate(event.comment, 200, "..."));
+ template.setNumber(event.change.get().number);
+ template.setTitle(substringBefore(event.change.get().commitMessage, "\n"));
+ template.setMessage(event.comment);
message = template.render();
}
diff --git a/src/main/java/com/cisco/gerrit/plugins/slack/message/MessageTemplate.java b/src/main/java/com/cisco/gerrit/plugins/slack/message/MessageTemplate.java
index f73517b..993a37a 100644
--- a/src/main/java/com/cisco/gerrit/plugins/slack/message/MessageTemplate.java
+++ b/src/main/java/com/cisco/gerrit/plugins/slack/message/MessageTemplate.java
@@ -38,16 +38,17 @@
private String channel;
private String name;
private String action;
- private int number;
private String project;
private String branch;
private String url;
+ private int number;
+ private String title;
private String message;
public String getChannel()
{
- return escape(channel);
+ return clean(channel);
}
public void setChannel(String channel)
@@ -57,7 +58,7 @@
public String getName()
{
- return escape(name);
+ return clean(name);
}
public void setName(String name)
@@ -67,7 +68,7 @@
public String getAction()
{
- return escape(action);
+ return clean(action);
}
public void setAction(String action)
@@ -75,6 +76,36 @@
this.action = action;
}
+ public String getProject()
+ {
+ return clean(project);
+ }
+
+ public void setProject(String project)
+ {
+ this.project = project;
+ }
+
+ public String getBranch()
+ {
+ return clean(branch);
+ }
+
+ public void setBranch(String branch)
+ {
+ this.branch = branch;
+ }
+
+ public String getUrl()
+ {
+ return clean(url);
+ }
+
+ public void setUrl(String url)
+ {
+ this.url = url;
+ }
+
public int getNumber()
{
return number;
@@ -85,29 +116,19 @@
this.number = number;
}
- public String getProject()
+ public String getTitle()
{
- return escape(project);
+ return clean(title);
}
- public void setProject(String project)
+ public void setTitle(String title)
{
- this.project = project;
- }
-
- public String getBranch()
- {
- return escape(branch);
- }
-
- public void setBranch(String branch)
- {
- this.branch = branch;
+ this.title = title;
}
public String getMessage()
{
- return escape(message);
+ return clean(message);
}
public void setMessage(String message)
@@ -115,16 +136,6 @@
this.message = message;
}
- public String getUrl()
- {
- return escape(url);
- }
-
- public void setUrl(String url)
- {
- this.url = url;
- }
-
/**
* Renders the message template into a String.
@@ -143,8 +154,8 @@
"message-template.json");
result = String.format(template, getChannel(), getName(),
- getAction(), getProject(), getBranch(), getNumber(),
- getUrl(), getMessage(), "good");
+ getAction(), getProject(), getBranch(), getUrl(),
+ getNumber(), getTitle(), getMessage(), "good");
}
catch (IOException e)
{
@@ -155,19 +166,25 @@
}
/**
- * Escapes the double quote character.
+ * Cleans up the provided string to make it acceptable for using in a
+ * Slack message template. It escapes any double quote characters,
+ * trims all leading/trailing whitespace and returns an empty string if
+ * the provided string was null.
*
- * @param str The message in which to search escape double quote
- * characters
+ * @param str The string to process.
*
* @return The message with all occurrences of the double quote character
- * escaped.
+ * escaped and leading/trailing whitespace trimmed
*/
- private String escape(String str)
+ private String clean(String str)
{
if (str != null)
{
- str = str.replace("\"", "\\\"");
+ str = str.replace("\"", "\\\"").trim();
+ }
+ else
+ {
+ str = "";
}
return str;
diff --git a/src/main/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGenerator.java b/src/main/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGenerator.java
index b3d119b..d7b010f 100644
--- a/src/main/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGenerator.java
+++ b/src/main/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGenerator.java
@@ -25,6 +25,8 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import static org.apache.commons.lang.StringUtils.substringBefore;
+
/**
* A specific MessageGenerator implementation that can generate a message for a
* patchset created event.
@@ -106,7 +108,7 @@
template.setProject(event.change.get().project);
template.setBranch(event.change.get().branch);
template.setUrl(event.change.get().url);
- template.setMessage(event.change.get().commitMessage.split("\n")[0]);
+ template.setTitle(substringBefore(event.change.get().commitMessage, "\n"));
message = template.render();
}
diff --git a/src/main/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGenerator.java b/src/main/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGenerator.java
index ef9413d..d1c7870 100644
--- a/src/main/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGenerator.java
+++ b/src/main/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGenerator.java
@@ -22,6 +22,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.commons.lang.StringUtils.substringBefore;
+
/**
* A specific MessageGenerator implementation that can generate a message for
* a reviewer added event.
@@ -81,7 +83,7 @@
template.setProject(event.change.get().project);
template.setBranch(event.change.get().branch);
template.setUrl(event.change.get().url);
- template.setMessage(event.change.get().commitMessage.split("\n")[0]);
+ template.setTitle(substringBefore(event.change.get().commitMessage, "\n"));
message = template.render();
}
diff --git a/src/main/resources/message-template.json b/src/main/resources/message-template.json
index 8cddda1..5dd8038 100644
--- a/src/main/resources/message-template.json
+++ b/src/main/resources/message-template.json
@@ -2,12 +2,12 @@
"channel": "#%1$s",
"attachments": [
{
- "fallback": "%2$s %3$s - %4$s (%5$s) - %7$s - %8$s",
- "pretext": "%2$s %3$s",
- "title": "%4$s (%5$s) - change %6$s",
- "title_link": "%7$s",
- "text": "%8$s",
- "color": "%9$s"
+ "fallback": "%2$s %3$s %4$s (%5$s) %6$s: %8$s",
+ "pretext": "%2$s %3$s <%6$s|%4$s (%5$s) change %7$s>",
+ "title": "%8$s",
+ "title_link": "%6$s",
+ "text": "%9$s",
+ "color": "%10$s"
}
]
}
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/PublishEventListenerTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/PublishEventListenerTest.java
index 50b3f16..2afaf0e 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/PublishEventListenerTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/PublishEventListenerTest.java
@@ -43,7 +43,7 @@
@Test
public void handlesPatchSetCreatedEvents() throws Exception
{
- // Wat? Placeholder, as I need to think about out how to test...
+ // TODO: Add actual tests here
assertTrue(true);
}
}
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/client/WebhookClientIntegrationTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/client/WebhookClientIntegrationTest.java
index 2b2b284..29fa2af 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/client/WebhookClientIntegrationTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/client/WebhookClientIntegrationTest.java
@@ -50,11 +50,50 @@
template.setChannel("general");
template.setName("Integration Tester");
template.setAction("proposed");
- template.setNumber(1234);
template.setProject("project");
template.setBranch("master");
template.setUrl("http://gerrit/1234");
- template.setMessage("This is a really great commit.");
+ template.setNumber(1234);
+ template.setTitle("Adds a test commit message");
+
+ String webhookUrl;
+ webhookUrl = properties.getProperty("webhook-url");
+
+ assertTrue(client.publish(template.render(), webhookUrl));
+ }
+
+ @Test
+ public void canPublishMessageWithLongMessage() throws Exception
+ {
+ WebhookClient client;
+ client = new WebhookClient();
+
+ InputStream testProperties;
+ testProperties = ResourceHelper.loadNamedResourceAsStream(
+ "test.properties");
+
+ Properties properties;
+ properties = new Properties();
+ properties.load(testProperties);
+
+ testProperties.close();
+
+ MessageTemplate template;
+ template = new MessageTemplate();
+
+ template.setChannel("general");
+ template.setName("Integration Tester");
+ template.setAction("commented on");
+ template.setProject("project");
+ template.setBranch("master");
+ template.setUrl("http://gerrit/1234");
+ template.setNumber(1234);
+ template.setTitle("Adds a test commit message");
+ template.setMessage("It provides a bunch of really great things. " +
+ "I am mostly trying to fill out a really long comment to " +
+ "test message rendering. Slack should do the right thing " +
+ "but this will be on multiple lines in IRC.\n\n\n\n\n" +
+ "This is hidden.");
String webhookUrl;
webhookUrl = properties.getProperty("webhook-url");
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGeneratorTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGeneratorTest.java
index 78f3ad7..7ccbe73 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGeneratorTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/message/ChangeMergedMessageGeneratorTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.data.AccountAttribute;
import com.google.gerrit.server.data.ChangeAttribute;
-import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.events.ChangeMergedEvent;
import org.junit.Before;
import org.junit.Test;
@@ -187,7 +186,7 @@
mockChange.project = "testproject";
mockChange.branch = "master";
mockChange.url = "https://change/";
- mockChange.commitMessage = "This is a title\nand a the body.";
+ mockChange.commitMessage = "This is the title\nThis is the message body.";
mockAccount.name = "Unit Tester";
@@ -201,11 +200,11 @@
" \"channel\": \"#testchannel\",\n" +
" \"attachments\": [\n" +
" {\n" +
- " \"fallback\": \"Unit Tester merged - testproject (master) - https://change/ - This is a title\",\n" +
- " \"pretext\": \"Unit Tester merged\",\n" +
- " \"title\": \"testproject (master) - change 1234\",\n" +
+ " \"fallback\": \"Unit Tester merged testproject (master) https://change/: This is the title\",\n" +
+ " \"pretext\": \"Unit Tester merged <https://change/|testproject (master) change 1234>\",\n" +
+ " \"title\": \"This is the title\",\n" +
" \"title_link\": \"https://change/\",\n" +
- " \"text\": \"This is a title\",\n" +
+ " \"text\": \"\",\n" +
" \"color\": \"good\"\n" +
" }\n" +
" ]\n" +
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGeneratorTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGeneratorTest.java
index a2fc138..371373e 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGeneratorTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/message/CommentAddedMessageGeneratorTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.data.AccountAttribute;
import com.google.gerrit.server.data.ChangeAttribute;
-import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.events.CommentAddedEvent;
import org.junit.Before;
import org.junit.Test;
@@ -182,7 +181,7 @@
mockEvent.change = Suppliers.ofInstance(mockChange);
mockEvent.author = Suppliers.ofInstance(mockAccount);
- mockEvent.comment = "This is the first line\nAnd the second line.";
+ mockEvent.comment = "This is the comment body.";
mockChange.number = 1234;
mockChange.project = "testproject";
@@ -191,6 +190,8 @@
mockChange.owner = mockOwner;
mockOwner.name = "Owner";
+ mockChange.commitMessage = "This is the title\nThis is the message body.";
+
mockAccount.name = "Unit Tester";
// Test
@@ -203,60 +204,11 @@
" \"channel\": \"#testchannel\",\n" +
" \"attachments\": [\n" +
" {\n" +
- " \"fallback\": \"Unit Tester commented on - testproject (master) - https://change/ - This is the first line\nAnd the second line.\",\n" +
- " \"pretext\": \"Unit Tester commented on\",\n" +
- " \"title\": \"testproject (master) - change 1234\",\n" +
+ " \"fallback\": \"Unit Tester commented on testproject (master) https://change/: This is the title\",\n" +
+ " \"pretext\": \"Unit Tester commented on <https://change/|testproject (master) change 1234>\",\n" +
+ " \"title\": \"This is the title\",\n" +
" \"title_link\": \"https://change/\",\n" +
- " \"text\": \"This is the first line\n" +
- "And the second line.\",\n" +
- " \"color\": \"good\"\n" +
- " }\n" +
- " ]\n" +
- "}\n";
-
- String actualResult;
- actualResult = messageGenerator.generate();
-
- assertThat(actualResult, is(equalTo(expectedResult)));
- }
-
- @Test
- public void generatesExpectedMessageForLongComment() throws Exception
- {
- // Setup mocks
- ProjectConfig config = getConfig();
- mockEvent.change = Suppliers.ofInstance(mockChange);
- mockEvent.author = Suppliers.ofInstance(mockAccount);
-
- mockEvent.comment = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. " +
- "Integer tristique ligula nec dapibus lobortis. Nulla venenatis, lacus quis vulputate volutpat, " +
- "sem neque ornare eros, vel sodales magna risus et diam. Maecenas ultricies justo dictum orci " +
- "scelerisque consequat a vel purus.";
-
- mockChange.number = 1234;
- mockChange.project = "testproject";
- mockChange.branch = "master";
- mockChange.url = "https://change/";
- mockChange.owner = mockOwner;
- mockOwner.name = "Owner";
-
- mockAccount.name = "Unit Tester";
-
- // Test
- MessageGenerator messageGenerator;
- messageGenerator = MessageGeneratorFactory.newInstance(
- mockEvent, config);
-
- String expectedResult;
- expectedResult = "{\n" +
- " \"channel\": \"#testchannel\",\n" +
- " \"attachments\": [\n" +
- " {\n" +
- " \"fallback\": \"Unit Tester commented on - testproject (master) - https://change/ - " + mockEvent.comment.substring(0, 197) + "...\",\n" +
- " \"pretext\": \"Unit Tester commented on\",\n" +
- " \"title\": \"testproject (master) - change 1234\",\n" +
- " \"title_link\": \"https://change/\",\n" +
- " \"text\": \"" + mockEvent.comment.substring(0, 197) + "...\",\n" +
+ " \"text\": \"This is the comment body.\",\n" +
" \"color\": \"good\"\n" +
" }\n" +
" ]\n" +
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/message/MessageTemplateTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/message/MessageTemplateTest.java
index 0e30b93..3ad1733 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/message/MessageTemplateTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/message/MessageTemplateTest.java
@@ -27,15 +27,13 @@
MessageTemplate template;
template = new MessageTemplate();
- template.setChannel("general");
- template.setName("Mr. Developer");
+ template.setChannel("testchannel");
+ template.setName("Unit Tester");
template.setAction("proposed");
- template.setNumber(1234);
template.setProject("project");
template.setBranch("master");
- template.setUrl("https://gerrit-review.googlesource.com/#/admin/projects/plugins/slack-integration");
+ template.setUrl("https://change/");
+ template.setNumber(1234);
template.setMessage("This is a really great commit.");
-
- System.out.println(template.render());
}
}
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGeneratorTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGeneratorTest.java
index caf2301..7e0bf48 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGeneratorTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/message/PatchSetCreatedMessageGeneratorTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.data.AccountAttribute;
import com.google.gerrit.server.data.ChangeAttribute;
-import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.events.PatchSetCreatedEvent;
import org.junit.Before;
import org.junit.Test;
@@ -197,7 +196,7 @@
mockChange.project = "testproject";
mockChange.branch = "master";
mockChange.url = "https://change/";
- mockChange.commitMessage = "This is a title\nand a the body.";
+ mockChange.commitMessage = "This is the title\nThis is the message body.";
mockAccount.name = "Unit Tester";
@@ -211,11 +210,11 @@
" \"channel\": \"#testchannel\",\n" +
" \"attachments\": [\n" +
" {\n" +
- " \"fallback\": \"Unit Tester proposed - testproject (master) - https://change/ - This is a title\",\n" +
- " \"pretext\": \"Unit Tester proposed\",\n" +
- " \"title\": \"testproject (master) - change 1234\",\n" +
+ " \"fallback\": \"Unit Tester proposed testproject (master) https://change/: This is the title\",\n" +
+ " \"pretext\": \"Unit Tester proposed <https://change/|testproject (master) change 1234>\",\n" +
+ " \"title\": \"This is the title\",\n" +
" \"title_link\": \"https://change/\",\n" +
- " \"text\": \"This is a title\",\n" +
+ " \"text\": \"\",\n" +
" \"color\": \"good\"\n" +
" }\n" +
" ]\n" +
diff --git a/src/test/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGeneratorTest.java b/src/test/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGeneratorTest.java
index 30358f6..74f97a5 100644
--- a/src/test/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGeneratorTest.java
+++ b/src/test/java/com/cisco/gerrit/plugins/slack/message/ReviewerAddedMessageGeneratorTest.java
@@ -24,7 +24,6 @@
import com.google.gerrit.server.config.PluginConfigFactory;
import com.google.gerrit.server.data.AccountAttribute;
import com.google.gerrit.server.data.ChangeAttribute;
-import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.events.ReviewerAddedEvent;
import org.junit.Before;
import org.junit.Test;
@@ -167,7 +166,7 @@
mockChange.number = 1234;
mockChange.project = "testproject";
mockChange.branch = "master";
- mockChange.commitMessage = "This is the first line\nAnd the second line.";
+ mockChange.commitMessage = "This is the title\nThis is the message body.";
mockChange.url = "https://change/";
mockAccount.name = "Unit Tester";
@@ -182,11 +181,11 @@
" \"channel\": \"#testchannel\",\n" +
" \"attachments\": [\n" +
" {\n" +
- " \"fallback\": \"Unit Tester was added to review - testproject (master) - https://change/ - This is the first line\",\n" +
- " \"pretext\": \"Unit Tester was added to review\",\n" +
- " \"title\": \"testproject (master) - change 1234\",\n" +
+ " \"fallback\": \"Unit Tester was added to review testproject (master) https://change/: This is the title\",\n" +
+ " \"pretext\": \"Unit Tester was added to review <https://change/|testproject (master) change 1234>\",\n" +
+ " \"title\": \"This is the title\",\n" +
" \"title_link\": \"https://change/\",\n" +
- " \"text\": \"This is the first line\",\n" +
+ " \"text\": \"\",\n" +
" \"color\": \"good\"\n" +
" }\n" +
" ]\n" +
