Adding ability to set icon_emoji and username on bot messages #578

EricBartusch · 2019-05-31T20:00:30Z

When using a bot, the iconEmoji and username fields will overwrite the emoji and name used by the bot in Slack. Either value being set will override the as_user field and turn the flag off. The parameters are optional and not including both of them results in the original behavior. It's possible to just set one or the other as well.

This works in pipeline and freestyle jobs.

Fixing issue where previous build could also be current build if multiple builds were running concurrently

jetersen · 2019-06-01T04:35:10Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -390,7 +410,7 @@ public SlackNotifier(final String baseUrl, final String teamDomain, final String
    }

    public SlackNotifier(final String baseUrl, final String teamDomain, final String authToken, final boolean botUser, final String room, final String tokenCredentialId,
-                         final String sendAs, final boolean startNotification, final boolean notifyAborted, final boolean notifyFailure,
+                         final String sendAs, final String iconEmoji, final String username, final boolean startNotification, final boolean notifyAborted, final boolean notifyFailure,


This is unwieldy, and you would need "another" deprecated SlackNotifier to avoid binary incompatibility.
Perhaps we ought to switch to a builder pattern?
Or is the only used for testing? Perhaps mark it as NoExternalAccess
@timja WDYT?

I think we need a Pojo with a builder pattern, we can't keep adding arguments here.

Help me understand a little bit: I get the builder pattern bit, but I don't know where it will actually be used. Wouldn't I need to make another constructor anyway to avoid the binary incompatibility?

There will be one new constructor, and a conversion in the old one to the builder.

Never more will there be a new constructor, there's about 6 in this file

jetersen · 2019-06-01T04:39:43Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

+        public String getIconEmoji() { return iconEmoji; }
+
+        @DataBoundSetter
+        public void setIconEmoji(String iconEmoji) {


Something is fishy when you need to add the setter/getter twice.

Surely we could use the same object/class.

Aren't these needed to get/set values on configuration pages?

This could be done as a separate improvement, but there is definitely some need to reduce the repetition 😓

jetersen · 2019-06-01T04:44:23Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

+                    }
+                    else {
+                        if (StringUtils.isNotEmpty(iconEmoji)) {
+                            url += "&icon_emoji=" + URLEncoder.encode(iconEmoji, StandardCharsets.UTF_8.name());


~~May I suggest we start using handy URI?~~
No, you may not! Use JSON post request instead of building URL

You should use the Jenkins plugin version of it: https://github.com/jenkinsci/handy-uri-templates-2-api-plugin

However is the docs: https://github.com/damnhandy/Handy-URI-Templates
See usage here: https://github.com/jenkinsci/bitbucket-branch-source-plugin/pull/80/files

See this comment: #577 (comment)

jetersen · 2019-06-01T04:51:23Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

+        if(StringUtils.isEmpty(iconEmoji)) {
+            return iconEmoji;
+        }
+        else if (!iconEmoji.startsWith(":")) {


You could shorten this to

Suggested change

else if (!iconEmoji.startsWith(":")) {

iconEmoji = StringUtils.appendIfMissing(iconEmoji, ":")

jetersen · 2019-06-01T04:51:25Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

+        else if (!iconEmoji.startsWith(":")) {
+            iconEmoji = ":" + iconEmoji;
+        }
+        if (!iconEmoji.endsWith(":")) {


Also

Suggested change

if (!iconEmoji.endsWith(":")) {

iconEmoji = StringUtils.prependIfMissing(iconEmoji, ":")

jetersen · 2019-06-03T19:59:28Z

pom.xml

+        <dependency>
+            <groupId>org.apache.commons</groupId>
+            <artifactId>commons-lang3</artifactId>
+            <version>3.7</version>


Are we really using such an old commons-lang version? 😢

… stub

EricBartusch · 2019-06-04T14:46:08Z

I'm clueless on what SpotBugs wants from me. I'm guessing I've implemented the builder pattern incorrectly.

timja · 2019-06-11T06:25:47Z

You need to update doTestConnection in SlackNotifier
So that the "Test connection" works on the global configuration page

EricBartusch · 2019-06-11T11:26:06Z

Can I also change the name of the method and button? I didn't think to add it because doTestConnection makes our dinner like it's just making sure Jenkins can talk to Slack, not actually send a message. doTestMessage or sendTestMessage makes more sense to me.

timja · 2019-06-11T12:31:14Z

it needs to start with do it makes sense to me how it is, it's testing the connection to slack...

… needing to be saved before applying to the test connection

timja · 2019-06-11T16:11:40Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

                String targetRoom = Util.fixEmpty(room) != null ? room : this.room;

-                SlackService testSlackService = getSlackService(targetUrl, targetDomain, targetTokenCredentialId, targetBotUser, targetRoom, project);
+                SlackService testSlackService = getSlackService(new StandardSlackServiceBuilder()


Suggested change

SlackService testSlackService = getSlackService(new StandardSlackServiceBuilder()

SlackService testSlackService = getSlackService(StandardSlackServiceBuilder.builder()

timja · 2019-06-11T16:11:52Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

+                        .withRoomId(targetRoom)
+                        .withIconEmoji(iconEmoji)
+                        .withUsername(username)
+                        ,tokenCredentialId, project


hmm, formatting looks weird here, maybe put the comma one line up?

timja · 2019-06-11T16:17:24Z

Test connection is working in global config now:

timja · 2019-06-11T16:21:29Z

The slack send pipeline log hasn't been updated:

but the functionality works

timja · 2019-06-11T19:35:00Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -712,12 +815,18 @@ public FormValidation doTestConnection(@QueryParameter("baseUrl") final String b
                    targetUrl = this.baseUrl;
                }
                String targetDomain = Util.fixEmpty(teamDomain) != null ? teamDomain : this.teamDomain;
-                boolean targetBotUser = botUser || this.botUser;
-                String targetTokenCredentialId = Util.fixEmpty(tokenCredentialId) != null ? tokenCredentialId :


removing these two lines break relying on global config when testing in a freestyle job

You're right, the test needs to be consistent so the behavior is the same between the test and how it actually works.

… behave

timja · 2019-06-11T21:01:33Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -471,7 +532,16 @@ public SlackService newSlackService(AbstractBuild abstractBuild, BuildListener l
        authTokenCredentialId = env.expand(authTokenCredentialId);
        room = env.expand(room);
        final String populatedToken = CredentialsObtainer.getTokenToUse(authTokenCredentialId, abstractBuild.getParent(), authToken);
-        return new StandardSlackService(baseUrl, teamDomain, botUser, room, false, populatedToken);
+        return new StandardSlackService(
+                new StandardSlackServiceBuilder()


Suggested change

new StandardSlackServiceBuilder()

StandardSlackService.builder()

timja · 2019-06-11T21:03:04Z

src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java

-            return new StandardSlackService(baseUrl, team, botUser, channel, replyBroadcast, populatedToken);
+        SlackService getSlackService(String baseUrl, String team, boolean botUser, String channel, boolean replyBroadcast, String iconEmoji, String username, String populatedToken) {
+            return new StandardSlackService(
+                    new StandardSlackServiceBuilder()


Suggested change

new StandardSlackServiceBuilder()

StandardSlackService.builder()

timja · 2019-06-11T21:03:21Z

src/test/java/jenkins/plugins/slack/SlackNotifierStub.java

-                notifyNotBuilt, notifySuccess, notifyUnstable, notifyRegression, notifyBackToNormal, notifyRepeatedFailure,
-                includeTestSummary, includeFailedTests, matrixTriggerMode, commitInfoChoice, includeCustomMessage, customMessage,
-                customMessageSuccess, customMessageAborted, customMessageNotBuilt, customMessageUnstable, customMessageFailure);
+        super(new SlackNotifierBuilder()


Suggested change

super(new SlackNotifierBuilder()

super(builder()

timja · 2019-06-11T21:06:55Z

Can you update the tests to not call the deprecated constructor please?
i.e. in:
StandardSlackServiceTest
StandardSlackServiceStub
SlackNotifierStub (this one maybe needs a suppression, haven't looked closely)

…ugin into feature/icon_emoji

timja

Thanks for this! merging when the build is green

timja · 2019-06-12T08:01:52Z

Want me to wait for your other PR before doing a release?

EricBartusch · 2019-06-12T14:18:33Z

Yeah, I should be able to do the other one pretty soon here

EricBartusch added 18 commits May 29, 2019 15:07

Update ActiveNotifier.java

2488895

Fixing issue where previous build could also be current build if multiple builds were running concurrently

adding null check

95d60ee

undoing old branch work

4c4dd70

adding emoji option

c40233a

adding check

28fd2bd

adding icon_emoji for bot users

cb30c3f

Correcting emoji formatting

1c1a358

fixing tests and adding test

260867e

username work in progress

febda3b

adding username param

232c5fe

unit tests

7d911db

adding ability to use in freestyle jobs

dd628b2

global settings and helpers

3b13703

adding fields to snippet generator

2f825b6

tidying up

3c8bb2d

fixing compile error

77ef960

fixing test

c35ce54

Correcting integration test

9f4e5c5

jetersen reviewed Jun 1, 2019

View reviewed changes

EricBartusch added 7 commits June 3, 2019 10:08

adding commons-lang3 to add StringUtils shortcuts

7e3a6e4

refactoring api call to use json

6438ccc

creating another constructor to not break API

a813f65

added POJO builder

c9d80e9

fix unit test

37f4fb7

Adding builder pattern to SlackNotifier

ef228f8

use builder in stub

fd68d6c

jetersen reviewed Jun 3, 2019

View reviewed changes

EricBartusch added 2 commits June 3, 2019 19:54

Making builder constructor private, going back to old constructor for…

af7ad61

… stub

ignore return value of constructor

8185b57

EricBartusch added 3 commits June 11, 2019 09:17

Adding iconEmoji and username to testconnection, fixing bot user flag…

26fec3d

… needing to be saved before applying to the test connection

Adding a constructor to fix SlackNotifer tests

43bc498

Pass in authTokenCredentialId to new constructor

6914f8e

timja reviewed Jun 11, 2019

View reviewed changes

EricBartusch added 2 commits June 11, 2019 11:53

Small fixes

9c56387

Adding new parameters to log output

311d811

timja reviewed Jun 11, 2019

View reviewed changes

EricBartusch added 2 commits June 11, 2019 15:18

Grab global value so the test is consistent with how it will actually…

fd50485

… behave

Apply global config for iconEmoji and username testing

b2785ea

timja added the feature label Jun 11, 2019

timja mentioned this pull request Jun 11, 2019

Ability to pass optional arguments to slack API's chat.PostMessage #367

Closed

timja reviewed Jun 11, 2019

View reviewed changes

EricBartusch and others added 4 commits June 11, 2019 18:31

Using non-deprecated constructors in tests

fa7c694

Merge branch 'master' into feature/icon_emoji

af91cea

Merge branch 'feature/icon_emoji' of github.com:EricBartusch/slack-pl…

389717b

…ugin into feature/icon_emoji

Cleanup

d716f5c

timja approved these changes Jun 12, 2019

View reviewed changes

timja merged commit 1ece202 into jenkinsci:master Jun 12, 2019

EricBartusch deleted the feature/icon_emoji branch June 12, 2019 14:18

mike-pt mentioned this pull request Aug 11, 2019

Add support for icon_emoji #477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ability to set icon_emoji and username on bot messages #578

Adding ability to set icon_emoji and username on bot messages #578

EricBartusch commented May 31, 2019

jetersen Jun 1, 2019

timja Jun 1, 2019

EricBartusch Jun 3, 2019

timja Jun 3, 2019

jetersen Jun 1, 2019

EricBartusch Jun 3, 2019

jetersen Jun 3, 2019

jetersen Jun 1, 2019 •

edited

Loading

jetersen Jun 1, 2019

jetersen Jun 1, 2019

jetersen Jun 1, 2019

jetersen Jun 3, 2019

EricBartusch commented Jun 4, 2019

timja commented Jun 11, 2019

EricBartusch commented Jun 11, 2019

timja commented Jun 11, 2019

timja Jun 11, 2019

timja Jun 11, 2019 •

edited

Loading

timja commented Jun 11, 2019

timja commented Jun 11, 2019

timja Jun 11, 2019

EricBartusch Jun 11, 2019

timja Jun 11, 2019 •

edited

Loading

timja Jun 11, 2019

timja Jun 11, 2019

timja commented Jun 11, 2019

timja left a comment

timja commented Jun 12, 2019

EricBartusch commented Jun 12, 2019

	else if (!iconEmoji.startsWith(":")) {
	iconEmoji = StringUtils.appendIfMissing(iconEmoji, ":")

	if (!iconEmoji.endsWith(":")) {
	iconEmoji = StringUtils.prependIfMissing(iconEmoji, ":")

	SlackService testSlackService = getSlackService(new StandardSlackServiceBuilder()
	SlackService testSlackService = getSlackService(StandardSlackServiceBuilder.builder()

	new StandardSlackServiceBuilder()
	StandardSlackService.builder()

Adding ability to set icon_emoji and username on bot messages #578

Adding ability to set icon_emoji and username on bot messages #578

Conversation

EricBartusch commented May 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricBartusch commented Jun 4, 2019

timja commented Jun 11, 2019

EricBartusch commented Jun 11, 2019

timja commented Jun 11, 2019

Choose a reason for hiding this comment

timja Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

timja commented Jun 11, 2019

timja commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja Jun 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Jun 11, 2019

timja left a comment

Choose a reason for hiding this comment

timja commented Jun 12, 2019

EricBartusch commented Jun 12, 2019

jetersen Jun 1, 2019 •

edited

Loading

timja Jun 11, 2019 •

edited

Loading

timja Jun 11, 2019 •

edited

Loading