search

alexkvak / teamcity-slack

TeamCity Slack plugin
MIT License
124 stars 23 forks source link

support urlencode for placeholders #55

Closed YoungjaeKim closed 5 years ago

YoungjaeKim commented 5 years ago

Hi. First of all, thanks for your effort.

When we use a parameter within a Slack template, and a text with the parameter can be a URL. For example, https://www.sample.com/build?version={%build.version%} is shown as a link in Slack.

If we have 0.1.1+meta for %build.version%, the click result is wrongly addressed because of urlencode issue for + sign.

I thought it could be resolved when we have a method like {url(%build.number%)}. To do this, I tried to make it by copy-and-paste in this part as below; https://github.com/alexkvak/teamcity-slack/blob/56e5013ba03520da1dc424840fee74c4058f25cb/slackIntegration-server/src/main/scala/com/fpd/teamcity/slack/MessageBuilder.scala#L68

  case "reason" ⇒ reason
  // new code start
  case x if x.startsWith("url(%") && x.endsWith("%)") ⇒
        context.getBuildParameter(build, x.substring(5, x.length - 2).trim) match {
        case Some(value) ⇒ URLEncoder.encode(value, "UTF-8")
        case _ ⇒ unknownParameter
  }
  // new code end
  case x if x.startsWith("%") && x.endsWith("%") ⇒
        context.getBuildParameter(build, x.substring(1, x.length - 1).trim) match {
        case Some(value) ⇒ value
        case _ ⇒ unknownParameter
  }

But I think it is very ugly and short-sighted approach.

Any idea to improve it?

alexkvak commented 5 years ago

Hi, why do you think so?

As for me it's normal code 👍 Let's do PR

alexkvak commented 5 years ago

And please add new test case

YoungjaeKim commented 5 years ago

@alexkvak Thanks :) I thought, my proposal has a limitation that it only supports %parameters%. How can we support all placeholders with a neat code?

alexkvak commented 5 years ago

Oh, I see. You need some new placeholder type or even modifier. It's not possible. Could you try Slack url formatting syntax <https://www.sample.com/build?version={%build.version%}|caption> ?

YoungjaeKim commented 5 years ago

@alexkvak I tested <url|caption> but it has same urlencode problem :( Shall I PR to you with my proposal?

alexkvak commented 5 years ago

No, in this case all parameters will be url encoded. Could you replace plus sign with minus sign? It resolves your problem very simple.

YoungjaeKim commented 5 years ago

+ symbol is just SemVer format, so it could help some others :) And, what do you mean 'all parameters will be url encoded'? My PR would perform in {url(%parameter%)} input only.

alexkvak commented 5 years ago

Yes, now I see, sorry :) It seem that this solution is simplest. So let's look your changes with tests together. And don't forget to modify readme.