Fix for issues #38 and #126

[davidkarlsen]

This simple change provides a pluggable way to change the encoding - including none at all which is a workaround for #38 and #126

---

This change is 

[davidkarlsen]

@rampatra ping?

[rampatra]

Hi @davidkarlsen, thanks a lot for contributing. Before reviewing/merging I would like to understand a bit more about encoding in Slack. From the slack docs, it says:

3 Characters you must encode as HTML entities

There are three characters you must convert to HTML entities and only three: &, <, and >. Slack will take care of the rest.

- Replace the ampersand, &, with &amp;
- Replace the less-than sign, < with &lt;
- Replace the greater-than sign, > with &gt;

That's it. Don't HTML entity-encode the entire message. Of course, when you send your message

So, exactly when do we need to NOT encode the message?

[davidkarlsen]

See: https://github.com/rampatra/jbot/issues/38#issuecomment-453230137 and the link there - <> have special meaning (linking) in slack - and thus you want to not encode them into &lt etc

[rampatra]

So, Slack at first asks to convert < to < in the payload and then says that there are some control sequences having special meaning and ergo, don't convert them. So, how will the bot know which one to convert and which one not to? For example, in one particular payload, there can be some mentions (user or channel) which shouldn't be converted and there can be also some other normal messages where it should be converted. So, how will JBot know which one to convert and which one not to? From your PR, I understand that I can either skip or do encoding entirely on the whole message which is not what we may need here. I hope I am making sense?

[davidkarlsen]

If you want to encode or not depends on the outcome you want to achieve. I would say that the current implementation is a bit naive and falls short (and hence #38 and #126 were registered), with my fix this is avoidable by leaving it to the use-case on how to encode.

[davidkarlsen]

@rampatra did you follow? do you need more info?

[davidkarlsen]

@rampatra ping?

[rampatra]

@davidkarlsen what I am trying to say is that we can make the encrypt function intelligent enough to encrypt just < or > signs but when it's in the form of control sequence, i.e, <@UYGUIGK>, then it shouldn't encrypt these. Hope I am making sense?

In other words, your PR is fine as it allows the user to override the encrypt function. I was just suggesting to make the default encrypt function intelligent enough so that the user doesn't even feel the need to override.

[davidkarlsen]

Maybe that can be done later - if you merge it now we can get back to that. "Release early - release often" ?

[davidkarlsen]

Ping

[dwursteisen]

👋 May I interrupt in your discussion?

Pluggable encoder is a nice workaround but in term of API, I'm not a big fan of it.

In my mind, I would see this kind of API instead:

// ...
reply(session, "<@345F> hello! <3", "<@345F>");

That will produce:

"<@345F> hello! &lt;3"

It exclude the reference to the user but it change the last <. What do you think about it?

You can find a Poc here: https://github.com/dwursteisen/jbot/blob/replay_api/jbot/src/main/java/me/ramswaroop/jbot/core/slack/Bot.java#L308

(the better would be to detect directly "<@345F>" as a slack user id, but if the developer doesn't to, you can badly interpret the string)

Regards

[rampatra]

hey @dwursteisen, I got what your method is doing, however, I did not completely understand how one would pass the user mentions (the user ids) to exclude to this method?

[dwursteisen]

If you're replying to someone, you do have his ID somewhere. For example, I use a slack bot to pick a random user from a list. So, what i'm doing is:

@Controller(events = EventType.DIRECT_MENTION)
public void pickUser(WebSocketSession session, Event event) {
           String pickedUser = randomUser();
          // did you notice that I have also updated the reply method?
           reply(session, event, "I choose…" + pickedUser + " as the winner!", pickedUser);
}

[rampatra]

@dwursteisen yes, this would only work for this case where you have the receiver id and pass it to your reply method. But what if a message has multiple mentions? For example, user A can target a message to user B, user C, etc.

Consider this message from user A on a slack channel, @userB @userC you both are late for this. Now, how will you detect and pass @userB and @userC to escape?

[dwursteisen]

// the last argument is a varargs. 
reply(session, event, "<@userB> <@userC> you both are late for this", "<@userB>", "<@userC>");