Review updates to customizable icons

jenkinsci · Jul 20, 2017 · 69cd0a5 · 69cd0a5
1 parent f60e9df
commit 69cd0a5
Show file tree

Hide file tree

Showing 10 changed files with 39 additions and 18 deletions.
diff --git a/src/main/java/jenkins/plugins/hipchat/CardProvider.java b/src/main/java/jenkins/plugins/hipchat/CardProvider.java
@@ -26,7 +26,22 @@ public CardProviderDescriptor getDescriptor() {
      *
      * @param run The build run.
      * @param taskListener The taskListener associated with the current build.
-     * @param icon The icon to include in the message.
+     * @param message The fully resolved notification message.
+     * @return The card that has been constructed for this notification. May be null if no card should be displayed.
+     * @deprecated Implement {@link #getCard(hudson.model.Run, hudson.model.TaskListener,
+     * jenkins.plugins.hipchat.model.notifications.Icon, java.lang.String)} instead.
+     */
+    @Deprecated
+    public Card getCard(Run<?, ?> run, TaskListener taskListener, String message) {
+        return getCard(run, taskListener, null, message);
+    }
+
+    /**
+     * Returns a card corresponding to the build notification.
+     *
+     * @param run The build run.
+     * @param taskListener The taskListener associated with the current build.
+     * @param icon The icon to include in the message. May be null.
      * @param message The fully resolved notification message.
      * @return The card that has been constructed for this notification. May be null if no card should be displayed.
      */

diff --git a/src/main/java/jenkins/plugins/hipchat/impl/DefaultCardProvider.java b/src/main/java/jenkins/plugins/hipchat/impl/DefaultCardProvider.java
@@ -33,10 +33,10 @@ public class DefaultCardProvider extends CardProvider {
 
     @Override
     public Card getCard(Run<?, ?> run, TaskListener taskListener, Icon icon, String message) {
-
         if (icon == null) {
-            icon = new Icon().withUrl("https://bit.ly/2ctIstd");
+            icon = new Icon().withUrl(Constants.DEFAULT_ICON_URL);
         }
+
         try {
             return new Card()
                     .withStyle(Style.APPLICATION)

diff --git a/src/main/java/jenkins/plugins/hipchat/model/Constants.java b/src/main/java/jenkins/plugins/hipchat/model/Constants.java
@@ -2,6 +2,7 @@
 
 public class Constants {
 
+    public static final String DEFAULT_ICON_URL = "https://bit.ly/2ctIstd";
     public static final String STATUS = "STATUS";
     public static final String HIPCHAT_MESSAGE_TEMPLATE = "HIPCHAT_MESSAGE_TEMPLATE";
 
@@ -41,5 +42,4 @@ public class Constants {
     public static final String ESCAPED_COMMIT_MESSAGE_MACRO = "${" + COMMIT_MESSAGE + ",escape=false}";
     public static final String HIPCHAT_CHANGES_MACRO = "${" + HIPCHAT_CHANGES + "}";
     public static final String HIPCHAT_CHANGES_OR_CAUSE_MACRO = "${" + HIPCHAT_CHANGES_OR_CAUSE + "}";
-
 }
diff --git a/src/main/java/jenkins/plugins/hipchat/model/NotificationConfig.java b/src/main/java/jenkins/plugins/hipchat/model/NotificationConfig.java
@@ -4,7 +4,9 @@
 import hudson.model.Describable;
 import hudson.model.Descriptor;
 import jenkins.model.Jenkins;
+import jenkins.plugins.hipchat.model.notifications.Icon;
 import jenkins.plugins.hipchat.model.notifications.Notification.Color;
+import org.apache.commons.lang.StringUtils;
 import org.kohsuke.stapler.DataBoundConstructor;
 
 public class NotificationConfig implements Describable<NotificationConfig> {
@@ -47,6 +49,10 @@ public String getIcon() {
         return icon;
     }
 
+    public Icon getIconObject() {
+        return StringUtils.isEmpty(icon) ? null : new Icon().withUrl(icon);
+    }
+
     public String getMessageTemplate() {
         return messageTemplate;
     }

diff --git a/src/main/java/jenkins/plugins/hipchat/model/NotificationType.java b/src/main/java/jenkins/plugins/hipchat/model/NotificationType.java
@@ -17,7 +17,6 @@
 import jenkins.plugins.hipchat.Messages;
 import jenkins.plugins.hipchat.exceptions.NotificationException;
 import jenkins.plugins.hipchat.impl.NoopCardProvider;
-import jenkins.plugins.hipchat.model.notifications.Icon;
 import jenkins.plugins.hipchat.model.notifications.Notification;
 import jenkins.plugins.hipchat.model.notifications.Notification.MessageFormat;
 import jenkins.plugins.hipchat.utils.BuildUtils;
@@ -114,9 +113,7 @@ Notification getNotification(NotificationConfig config, AbstractBuild<?, ?> buil
                     .withMessageFormat(config.isTextFormat() ? MessageFormat.TEXT : MessageFormat.HTML)
                     .withNotify(config.isNotifyEnabled())
                     .withMessage(message)
-                    .withCard(cardProvider.getCard(build, buildListener,
-                            (config.getIcon() != null && !config.getIcon().isEmpty() ?
-                                    new Icon().withUrl(config.getIcon()) : null), message));
+                    .withCard(cardProvider.getCard(build, buildListener, config.getIconObject(), message));
         } catch (MacroEvaluationException | IOException ex) {
             buildListener.getLogger().println(Messages.MacroEvaluationFailed(ex.toString()));
             throw new NotificationException(Messages.MacroEvaluationFailed(ex.getMessage()), ex);

diff --git a/src/main/java/jenkins/plugins/hipchat/workflow/HipChatSendStep.java b/src/main/java/jenkins/plugins/hipchat/workflow/HipChatSendStep.java
@@ -197,8 +197,7 @@ protected Void run() throws Exception {
                         .withColor(color)
                         .withMessage(message)
                         .withCard(cardProvider.getCard(run, listener,
-                                (step.icon != null && !step.icon.isEmpty() ? new Icon().withUrl(step.icon) : null),
-                                message))
+                                StringUtils.isEmpty(step.icon) ? null : new Icon().withUrl(step.icon), message))
                         .withNotify(step.notify)
                         .withMessageFormat(step.textFormat ? MessageFormat.TEXT : MessageFormat.HTML));
                 listener.getLogger().println(Messages.NotificationSuccessful(room));

diff --git a/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/help-defaultNotifications.jelly b/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/help-defaultNotifications.jelly
@@ -15,7 +15,10 @@
         If the message template is left empty, the project's default message template will be used. If that's empty as
         well, then the plugin falls back to the default message templates.
     </p>
-    <p>The icon URL used with card provider notifications. If it's left empty, a default is provided.</p>
+    <p>
+        The card icon controls what icon to display when card notifications are enabled. When left empty, the default
+        Jenkins logo will be displayed.
+    </p>
     <p>
         The message template may contain any normal build variable (using the <em>$VAR_NAME</em> notation).
         Additionally the following variables are provided for convenience:

diff --git a/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/help-notifications.jelly b/src/main/resources/jenkins/plugins/hipchat/HipChatNotifier/help-notifications.jelly
@@ -8,7 +8,10 @@
         If the message template is left empty, the project's default message template will be used. If that's empty as
         well, then the plugin falls back to the default message templates.
     </p>
-    <p>The icon URL used with card provider notifications. If it's left empty, a default is provided.</p>
+    <p>
+        The card icon controls what icon to display when card notifications are enabled. When left empty, the default
+        Jenkins logo will be displayed.
+    </p>
     <p>
         The message template may contain any normal build variable (using the <em>$VAR_NAME</em> notation).
         Additionally the following variables are provided for convenience:

diff --git a/src/main/resources/jenkins/plugins/hipchat/workflow/HipChatSendStep/help-icon.html b/src/main/resources/jenkins/plugins/hipchat/workflow/HipChatSendStep/help-icon.html
@@ -1,6 +1,3 @@
 <div>
-    <b>OPTIONAL</b> Icon URL for card provider notifications.<br>
-    Any valid image URL. Should be less than 128x128 in size.<br>
-    Defaults to Jenkins logo.<br>
-    <code>hipchatSend icon: "https://bit.ly/2ctIstd", color: "YELLOW", message: "Build Started: ${env.JOB_NAME} ${env.BUILD_NUMBER}"</code>
-</div>
+    Link to an icon that should be displayed in the notification, when cards are enabled. Defaults to Jenkins logo.
+</div>
diff --git a/src/test/java/jenkins/plugins/hipchat/workflow/HipChatSendStepTest.java b/src/test/java/jenkins/plugins/hipchat/workflow/HipChatSendStepTest.java
@@ -2,6 +2,7 @@
 
 import hudson.model.Result;
 import jenkins.plugins.hipchat.Messages;
+import jenkins.plugins.hipchat.model.Constants;
 import jenkins.plugins.hipchat.model.notifications.Notification.Color;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.steps.StepConfigTester;
@@ -25,7 +26,7 @@ public void evaluate() throws Throwable {
                 HipChatSendStep step1 = new HipChatSendStep("message");
                 step1.color = Color.GREEN;
                 step1.room = "room";
-                step1.icon = "https://bit.ly/2ctIstd";
+                step1.icon = Constants.DEFAULT_ICON_URL;
                 step1.v2enabled = true;
                 step1.notify = false;