Review changes for message templating

closes jlewallen#25
Palmr · Mar 9, 2015 · 3639a74 · 3639a74
1 parent c4f0a1d
commit 3639a74
Show file tree

Hide file tree

Showing 11 changed files with 64 additions and 80 deletions.
diff --git a/pom.xml b/pom.xml
@@ -17,6 +17,8 @@
     <url>http://wiki.jenkins-ci.org/display/JENKINS/HipChat+Plugin</url>
 
     <properties>
+        <maven-compiler-plugin.version>3.2</maven-compiler-plugin.version>
+        <powermock.version>1.6.1</powermock.version>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
     </properties>
@@ -57,28 +59,20 @@
             <dependency>
                 <groupId>junit</groupId>
                 <artifactId>junit</artifactId>
-                <version>4.11</version>
+                <version>4.12</version>
                 <scope>test</scope>
             </dependency>
-            <dependency>
-                <groupId>org.mockito</groupId>
-                <artifactId>mockito-core</artifactId>
-                <version>1.9.5</version>
-            </dependency>
             <dependency>
                 <groupId>org.powermock</groupId>
                 <artifactId>powermock-module-junit4</artifactId>
-                <version>1.5.6</version>
+                <version>${powermock.version}</version>
+                <scope>test</scope>
             </dependency>
             <dependency>
                 <groupId>org.powermock</groupId>
                 <artifactId>powermock-api-mockito</artifactId>
-                <version>1.5.6</version>
-            </dependency>
-            <dependency>
-                <groupId>org.hamcrest</groupId>
-                <artifactId>hamcrest-all</artifactId>
-                <version>1.3</version>
+                <version>${powermock.version}</version>
+                <scope>test</scope>
             </dependency>
         </dependencies>
     </dependencyManagement>
@@ -91,25 +85,13 @@
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
         </dependency>
-        <dependency>
-            <groupId>org.mockito</groupId>
-            <artifactId>mockito-core</artifactId>
-            <scope>test</scope>
-        </dependency>
         <dependency>
             <groupId>org.powermock</groupId>
             <artifactId>powermock-module-junit4</artifactId>
-            <scope>test</scope>
         </dependency>
         <dependency>
             <groupId>org.powermock</groupId>
             <artifactId>powermock-api-mockito</artifactId>
-            <scope>test</scope>
-        </dependency>
-        <dependency>
-            <groupId>org.hamcrest</groupId>
-            <artifactId>hamcrest-all</artifactId>
-            <scope>test</scope>
         </dependency>
     </dependencies>
 

diff --git a/src/main/java/jenkins/plugins/hipchat/HipChatNotifier.java b/src/main/java/jenkins/plugins/hipchat/HipChatNotifier.java
@@ -65,7 +65,7 @@ public HipChatNotifier(String token, String room, boolean startNotification, boo
         this.completeJobMessage = completeJobMessage;
     }
 
-    /* notification enabled disabled setter/getter */
+    /* notification enabled disabled setters/getters */
 
     public boolean isStartNotification() {
         return startNotification;
@@ -123,7 +123,16 @@ public void setNotifyBackToNormal(boolean notifyBackToNormal) {
         this.notifyBackToNormal = notifyBackToNormal;
     }
 
-    /* notification message configuration*/
+    /* notification message configurations */
+
+    public String getStartJobMessage() {
+        return startJobMessage;
+    }
+
+    public void setStartJobMessage(String startJobMessage) {
+        this.startJobMessage = startJobMessage;
+    }
+
 
     public String getCompleteJobMessage() {
         return completeJobMessage;
@@ -133,22 +142,16 @@ public void setCompleteJobMessage(String completeJobMessage) {
         this.completeJobMessage = completeJobMessage;
     }
 
-    public String getStartJobMessage() {
-        return startJobMessage;
-    }
+    /* Default notification messages for UI */
 
-    public void setStartJobMessage(String startJobMessage) {
-        this.startJobMessage = startJobMessage;
+    public String getStartJobMessageDefault() {
+        return Messages.JobStarted();
     }
 
     public String getCompleteJobMessageDefault() {
         return Messages.JobCompleted();
     }
 
-    public String getStartJobMessageDefault() {
-        return Messages.JobStarted();
-    }
-
     public String getRoom() {
         return StringUtils.isBlank(room) ? getDescriptor().getRoom() : room;
     }

diff --git a/src/main/java/jenkins/plugins/hipchat/NotificationType.java b/src/main/java/jenkins/plugins/hipchat/NotificationType.java
@@ -4,6 +4,7 @@
 import hudson.EnvVars;
 import hudson.model.AbstractBuild;
 import hudson.model.Result;
+import hudson.Util;
 import hudson.util.LogTaskListener;
 import hudson.util.VariableResolver;
 
@@ -67,7 +68,7 @@ protected String getStatus() {
         }
     };
 
-    private static final Logger logger = Logger.getLogger(NotificationType.class.getName());
+    private static final Logger LOGGER = Logger.getLogger(NotificationType.class.getName());
     private final String color;
     private final boolean isStartType;
 
@@ -94,32 +95,33 @@ public final String getMessage(AbstractBuild<?, ?> build, HipChatNotifier notifi
     }
 
     private String getTemplateFor(HipChatNotifier notifier) {
-        String userConfig, defaultConfig;
-        if (this.isStartType) {
+        String userConfig;
+        String defaultConfig;
+        if (isStartType) {
             userConfig = notifier.getStartJobMessage();
             defaultConfig = Messages.JobStarted();
         } else {
             userConfig = notifier.getCompleteJobMessage();
             defaultConfig = Messages.JobCompleted();
         }
 
-        if (userConfig == null || userConfig.trim().isEmpty()) {
-            Preconditions.checkState(defaultConfig != null, "default config not set for %s", this);
+        if (Util.fixEmptyAndTrim(userConfig) == null) {
+            Preconditions.checkNotNull(defaultConfig, "Default template not set for %s", this);
             return defaultConfig;
         } else {
             return userConfig;
         }
     }
 
-    private Map<String, String> collectParametersFor(AbstractBuild build) {
+    private Map<String, String> collectParametersFor(AbstractBuild<?, ?> build) {
         Map<String, String> merged = newHashMap();
         merged.putAll(build.getBuildVariables());
         merged.putAll(getEnvVars(build));
 
         String cause = NotificationTypeUtils.getCause(build);
         String changes = NotificationTypeUtils.getChanges(build);
 
-        merged.put("STATUS", this.getStatus());
+        merged.put("STATUS", getStatus());
         merged.put("DURATION", build.getDurationString());
         merged.put("URL", NotificationTypeUtils.getUrl(build));
         merged.put("CAUSE", cause);
@@ -129,9 +131,9 @@ private Map<String, String> collectParametersFor(AbstractBuild build) {
         return merged;
     }
 
-    private EnvVars getEnvVars(AbstractBuild build) {
+    private EnvVars getEnvVars(AbstractBuild<?, ?> build) {
         try {
-            return build.getEnvironment(new LogTaskListener(logger, INFO));
+            return build.getEnvironment(new LogTaskListener(LOGGER, INFO));
         } catch (IOException e) {
             throw propagate(e);
         } catch (InterruptedException e) {

diff --git a/src/main/java/jenkins/plugins/hipchat/NotificationTypeUtils.java b/src/main/java/jenkins/plugins/hipchat/NotificationTypeUtils.java
@@ -10,17 +10,16 @@
 import java.util.Set;
 import java.util.logging.Logger;
 
-import static java.util.logging.Level.INFO;
-
+import static java.util.logging.Level.*;
 
 public final class NotificationTypeUtils {
-    private static final Logger logger = Logger.getLogger(NotificationTypeUtils.class.getName());
+
+    private static final Logger LOGGER = Logger.getLogger(NotificationTypeUtils.class.getName());
 
     private NotificationTypeUtils() {
-        /* utility method */
     }
 
-    public static String getCause(AbstractBuild build) {
+    public static String getCause(AbstractBuild<?, ?> build) {
         CauseAction cause = build.getAction(CauseAction.class);
         if (cause != null) {
             return cause.getShortDescription();
@@ -29,28 +28,31 @@ public static String getCause(AbstractBuild build) {
         }
     }
 
-    public static String getUrl(AbstractBuild build) {
+    public static String getUrl(AbstractBuild<?, ?> build) {
         return Jenkins.getInstance().getRootUrl() + build.getUrl();
     }
 
-    public static String getChanges(AbstractBuild build) {
+    public static String getChanges(AbstractBuild<?, ?> build) {
         if (!build.hasChangeSetComputed()) {
+            LOGGER.log(FINE, "No changeset computed for job {0}", build.getProject().getFullDisplayName());
             return null;
         }
         Set<String> authors = Sets.newHashSet();
         int changedFiles = 0;
         for (Object o : build.getChangeSet().getItems()) {
             ChangeLogSet.Entry entry = (ChangeLogSet.Entry) o;
+            LOGGER.log(FINEST, "Entry {0}", entry);
             authors.add(entry.getAuthor().getDisplayName());
             try {
                 changedFiles += entry.getAffectedFiles().size();
             } catch (UnsupportedOperationException e) {
-                logger.log(INFO, "Unable to collect the affected files for job {0}",
+                LOGGER.log(INFO, "Unable to collect the affected files for job {0}",
                         build.getProject().getFullDisplayName());
                 return null;
             }
         }
         if (changedFiles == 0) {
+            LOGGER.log(FINE, "No changes detected");
             return null;
         }
 

diff --git a/src/main/java/jenkins/plugins/hipchat/impl/HipChatV1Service.java b/src/main/java/jenkins/plugins/hipchat/impl/HipChatV1Service.java
@@ -1,14 +1,12 @@
 package jenkins.plugins.hipchat.impl;
 
-import hudson.model.AbstractBuild;
 import org.apache.commons.httpclient.HttpClient;
 import org.apache.commons.httpclient.HttpStatus;
 import org.apache.commons.httpclient.methods.PostMethod;
 
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.plugins.hipchat.HipChatService;
-import jenkins.plugins.hipchat.NotificationType;
 
 public class HipChatV1Service extends HipChatService {
 

diff --git a/src/main/java/jenkins/plugins/hipchat/impl/HipChatV2Service.java b/src/main/java/jenkins/plugins/hipchat/impl/HipChatV2Service.java
@@ -1,13 +1,11 @@
 package jenkins.plugins.hipchat.impl;
 
-import hudson.model.AbstractBuild;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import jenkins.plugins.hipchat.HipChatService;
-import jenkins.plugins.hipchat.NotificationType;
 import net.sf.json.JSONObject;
 import org.apache.commons.httpclient.HttpClient;
 import org.apache.commons.httpclient.HttpStatus;

diff --git a/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/config.jelly b/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/config.jelly
@@ -44,7 +44,7 @@
             Default: '${instance.getStartJobMessageDefault()}'
         </f:entry>
 
-        <f:entry title="${%Job completed}">
+        <f:entry title="${%Job completed}" help="/plugin/hipchat/help-projectConfig-hipChatMessages.html">
             <f:textbox name="hipchat.completeJobMessage" value="${instance.completeJobMessage}"/>
             Default: '${instance.getCompleteJobMessageDefault()}'
         </f:entry>

diff --git a/src/main/resources/jenkins/plugins/hipchat/Messages.properties b/src/main/resources/jenkins/plugins/hipchat/Messages.properties
@@ -15,4 +15,4 @@ Unstable=UNSTABLE
 JobStarted=$JOB_NAME #$BUILD_NUMBER $STATUS ($CHANGES_OR_CAUSE) (<a href="$URL">Open</a>)
 JobCompleted=$JOB_NAME #$BUILD_NUMBER $STATUS after $DURATION (<a href="$URL">Open</a>)
 
-Unknown=Unknown
+Unknown=Unknown
diff --git a/src/main/webapp/help-projectConfig-hipChatMessages.html b/src/main/webapp/help-projectConfig-hipChatMessages.html
@@ -1,19 +1,19 @@
 <div>
     <p>
         Configures the message that will be displayed in the room. If left blank a default value will be used.
-        All normal build variables can be used with the notation <em>$VAR_NAME</em>. Additional the following
+        All normal build variables can be used with the notation <em>$VAR_NAME</em>. Additionally the following
         variables are provided:
     </p>
     <ul>
         <li><strong>$DURATION</strong> - duration of the build in human readable form. Example: 13 min</li>
         <li><strong>$URL</strong> - URL to the job page on jenkins. You can use normal HTML to create a link
-            i.e: &lt;a href="$URL"&gt;More Info&lt;a&gt; </li>
-        <li><strong>$CAUSE</strong> - cause of this change. i.e. 'starte by user foo'</li>
+            i.e: &lt;a href="$URL"&gt;More Info&lt;/a&gt; </li>
+        <li><strong>$CAUSE</strong> - cause of this change. Example: 'Started by user foo'</li>
         <li><strong>$CHANGES</strong> - a short overview of the changes that caused that job, if available</li>
         <li><strong>$CHANGES_OR_CAUSE</strong> - either $CHANGES or $CAUSE, see above</li>
-        <li><strong>$STATUS</strong> - the status of the build, i.e. 'Success'</li>
+        <li><strong>$STATUS</strong> - the status of the build. Example: 'Success'</li>
         <li><strong>$PRINT_FULL_ENV</strong> - dump of all variables, use this only for debugging purposes.
-            Be aware that it also contains the full host environment and which might contain sensitive information.
+            Be aware that it also contains the full host environment which might contain sensitive information.
         </li>
     </ul>
 </div>
diff --git a/src/test/java/jenkins/plugins/hipchat/HipChatNotifierBuilder.java b/src/test/java/jenkins/plugins/hipchat/HipChatNotifierBuilder.java
@@ -13,7 +13,7 @@ public class HipChatNotifierBuilder {
     private String messageJobStarted;
     private String messageJobCompleted;
 
-    public static HipChatNotifierBuilder notifier() {
+    public static HipChatNotifierBuilder builder() {
         return new HipChatNotifierBuilder();
     }
 
@@ -62,7 +62,7 @@ public HipChatNotifierBuilder setNotifyBackToNormal(boolean notifyBackToNormal)
         return this;
     }
 
-    public HipChatNotifier createHipChatNotifier() {
+    public HipChatNotifier build() {
         return new HipChatNotifier(token, room, startNotification, notifySuccess, notifyAborted, notifyNotBuilt,
                 notifyUnstable, notifyFailure, notifyBackToNormal, messageJobStarted, messageJobCompleted);
     }