Allow to customize the message that gets printed in the chat room

[retoo]

Simpel but powerful way to customize the notification messages. For example the default 'complete' message is: $JOB_NAME #$BUILD_NUMBER $STATUS ($CHANGES_OR_CAUSE) (<a href="$BUILD_URL">Open</a>)

All normal Jenkins and environment variables are available and allow countless combinations of configurations.

closes jenkinsci/hipchat-plugin#25

[aldaris]

Quite unfortunately my notification refactor stayed in my local repo for a long while, which means that most likely this PR no longer applies.

[retoo]

I can update to the latest version, if you like my current approach in the pull request.

[aldaris]

Providing var replacement for notification messages is a good first step on the way. I assume that the "completeJobMessage" is meant to be used for all notifications but started (i.e. unstable/failure/backtonormal/etc) in which case this approach looks fine to me.

[retoo]

Yes, completeJobMessage is for everything but start. This can easily be extended to all states should somebody really need it. We have been pretty happy with one field the past few weeks.

I like the refactoring, much nicer structure now. :) I'll port my changes and let you know when I'm done

Thanks for the comments!

[retoo]

Hmm I'm thinking about how to integrate my changes with the new properties file

I suggest the following:

1) We use the properties file to generate the full message 2) Instead of {0} we use the variable notation for example $DURATION 3) The user can override any NotificationType, not just started and completed.

What do you think?

(sorry for the edits)

[aldaris]

1) that was the original intent, though it really looks like the "Open" link in the messages got non-localized. 2) you mean in the .properties file right? Should be ok then 3) I don't want to overcomplicate the settings, 7 new config item sounds unreasonable to me.

In NotificationType the messages aren't too different from each other, the complete status message would fit onto most state except not_built (which doesn't have duration string). The way these new settings should work probably is that they should override the original message coming from getMessage() method.

The problem with having more than one "template" setting is that suddenly you have two choices:

• either modify NotificationType#fromResults to also receive the started and completed messages
• or modify NotificationType#getMessage to receive the message templates from HipChatNotifier depending on the type of the notification. Something that I wanted to prevent originally when I created NotificationType itself. -> well maybe this could be resolved by introducing a new getTemplate(NotificationType) method..

[retoo]

Originally I planned these two templates:

• $JOB_NAME #$BUILD_NUMBER $STATUS ($CHANGES_OR_CAUSE) (<a href="$BUILD_URL">Open</a>

And this the complete jobs:

• $JOB_NAME #$BUILD_NUMBER $STATUS after $DURATION (<a href="$BUILD_URL">Open</a>)

This means you can completely rearrange the string.

I see two options:

• we keep $STATUS and resolve that via properties file but everything else comes from the templates that are kept in the code.
• Or: we define this template for every NotificationType in the properties file. Instead of $STATUS we would the corresponding sentences. like: ... $job, $number, ABORTED after $duration <a href=$link... When the user has configured a template for a NotificationType, that template is used, otherwise the system uses the template from the properties file

I know, the second way is a bit less 'elegant', but it's dead simple. Instead of composing the message from multiple sources/combination it either uses the plugins template, or the template defined by the user. You want to link to the console? No problem, its your <a/> tag. You don't want to print the full job name, but a internal reference? No worries, just remove $JOB_NAME and configure it statically (for this job!).

What do you think?

[retoo]

Just an idea, to resolve the template we could do something like in NotificationType:

import static com.google.common.base.Objects.firstNonNull;
import static com.google.common.base.Strings.emptyToNull;

    STARTED("green") {
        @Override
        protected String getMessageString(HipChatNotifier notifier) {
            return firstNonNull(emptyToNull(notifier.getStartJobMessage()), Messages.Starting());
        }

[aldaris]

The second option it is then. I would suggest to hide the new settings in an advanced section on the job config page, so at least the UI won't be too cluttered by default.

[retoo]

I've (force) updated the branch

After I was finished implementing the second option and tried to configure it on our systems it became obvious that it is completely unpractical. I removed the fields again and now solved it with the original two fields 'completed' and 'started. The defaults and the value for $STATUS are all from the properties file.

What do you think?

[retoo]

Please let me know if you have any input to the general coding style! And I did some changes to the pom.xml, are you okay with these?

[aldaris]

These changes are looking really great. I had a look through the changes, and only had a few follow-up changes: 3639a74 Thanks for looking into this (and sorry about the slow response).