Use webhooks to reflect irc usernames + avatars #230

Fiaxhs · 2017-05-02T10:11:29Z

This fixes #128

The discord bot gets renamed with the irc nickname, and use the account avatar if there's a matching username in the room.

IRC formatting

coveralls · 2017-05-02T11:05:01Z

Coverage decreased (-3.3%) to 95.076% when pulling 2c05038 on Fiaxhs:master into fe37ec4 on reactiflux:master.

coveralls · 2017-05-02T11:08:11Z

Coverage decreased (-3.3%) to 95.057% when pulling b4452dd on Fiaxhs:master into fe37ec4 on reactiflux:master.

Throne3d · 2017-05-02T13:16:06Z

Could you add tests for getDiscordAvatar and findWebhook, as well as ensuring that it uses a webhook in sendToDiscord when one is available (and then does not go on to send a regular message)? I can probably advise on how to do this if you're unsure.

Fiaxhs · 2017-05-02T13:46:10Z

Sure! I'll do this soon.

Throne3d

Bump on tests?

Fiaxhs · 2017-05-17T22:56:57Z

Yes, I'm a bit short on free time these days. I'll try to produce something this week-end :)

…

On 16 May 2017 at 20:01, Edward Jones ***@***.***> wrote: ***@***.**** requested changes on this pull request. Bump on tests? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#230 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArNkT1dnEipdy1wkc2PF_PymWZs83xFks5r6eRngaJpZM4NN89p> .

Added test

coveralls · 2017-05-30T12:40:14Z

Coverage increased (+0.1%) to 98.582% when pulling 2bf761b on Fiaxhs:master into 8c4d83a on reactiflux:master.

Fiaxhs · 2017-05-30T12:41:33Z

Finally got some time to write tests, sorry for the delay :)

Kakkela · 2017-05-30T12:49:04Z

I would like to add this as a warning if this gets pushed to master.
https://trello.com/c/6ZZTEBJw/278-webhook-incorrectly-stacks-message-authors-on-android-client

So basically currently this wont be really readable on android client because of messages merging under one author incorrectly.

NightScript370 · 2017-05-30T13:04:40Z

Aw man, you already made it work with latest commit. Oh well. I'll delete my fork now.

Throne3d · 2017-05-30T13:21:16Z

Testing this locally, it seems to work well, and the tests added should help to ensure it keeps this functionality.

Assuming @ekmartin is okay with it, I think this is good to go.

(I'm slightly worried about IRC users masquerading as authenticated Discord users by changing to their usernames, but I don't think this is readily fixable, and the messages have 'BOT' marked on them, and it presents a problem on the IRC end as is anyway, so this shouldn't actually make it much if at all worse.)

Edit: Oh, a note – maybe add to the readme that webhooks allow for this sort of display behavior? That's not immediately obvious from the term 'webhook'.

Fiaxhs · 2017-05-30T14:13:52Z

Just added a note on webhooks. However feel free to change it, even the image. I feel quite uncomfortable writing the readme for a project that isn't mine!

coveralls · 2017-05-30T14:14:18Z

Coverage increased (+0.1%) to 98.582% when pulling 8c0c11a on Fiaxhs:master into 8c4d83a on reactiflux:master.

Throne3d · 2017-05-30T16:30:29Z

That looks like an accurate and useful summary, so it should be fine! Thanks.

NightScript370 · 2017-06-07T00:45:26Z

@Fiaxhs Can you make this case-insensitive?

Noted a necessary change

Throne3d · 2017-07-01T20:40:32Z

lib/bot.js

+    }
+    return null;
+  }
+


I don't think case insensitivity should be a blocker but it does make me wonder what happens if two Discord users have the same display name, as this seems doable, or the same name ignoring case. This is a problem elsewhere already, I guess, such as when an IRC user mentions a Discord user.

This function also currently checks by username as opposed to nickname (display name), which might be unexpected. This should maybe:

Check by nickname and username

Check by the above, case-insensitively, if no match found

If more than one found, default to no fancy icon

That might be more complicated than necessary, but I think it would suffice for most or all use-cases. It should at least check nicknames?

What's the purpose of checking case-sensitively at all? Performance?

So that if you have two users with the same nickname barring case, you can pick a particular one to use. (It's also possible to have two users with the exact same nickname, case-sensitively, but then the ambiguity is basically required as opposed to mostly quite avoidable.)

Maybe it's over-much, though – if two users have the same nickname, case-sensitive or not, it's not particularly unreasonable to expect weird behavior around the ambiguity. It at least shouldn't be actively detrimental.

Throne3d · 2017-07-01T20:41:47Z

lib/bot.js

+        }).catch(logger.error);
+      return;
+    }
+


… I also failed to notice that this seems to discard the custom formatting when sending with a webhook; I think the 'Webhooks first' code should go after the const withAuthor… bit, here, and use withAuthor instead of withMentions.

Edit: Or, rather… make a new pattern for webhook messages?

Edit2: … I'm not sure. Uh. It seems plausibly useful to be able to change the formatting for a webhook message, if people were changing the withAuthor thing drastically. Maybe. But defeats the point of a webhook a little. Uh. Maybe ignore this. We can address it later if anybody complains. (Above thing should still use nicknames and maybe case insensitively check.)

Throne3d · 2017-07-01T20:42:32Z

(Sorry I didn't notice this before.)

NightScript370 · 2017-07-04T03:04:05Z

Hey @Fiaxhs, can you update this to the latest commit?

Fiaxhs · 2017-07-31T13:17:42Z

Nice! I didn't see the addUser function, this would have save me some time.

I keep finding things to be picky about

Fine by me :)

Thanks for your contribution!

You're welcome! I use it every day, for once I could bring some value to a project I use.

ekmartin

Sorry about the late answer here - this is definitely cool. Just a few minor comments :)

ekmartin · 2017-08-01T05:52:28Z

lib/bot.js


    // Remove channel passwords from the mapping and lowercase IRC channel names
    _.forOwn(options.channelMapping, (ircChan, discordChan) => {
      this.channelMapping[discordChan] = ircChan.split(' ')[0].toLowerCase();
    });

+    // Extract id and token from Webhook urls
+    _.forOwn(options.webhooks, (url, chan) => {


chan -> channel

(consistent with your block on line 80)

on second thought, might as well merge this down into the connect block and only have one forOwn(options.webhooks, ...)?

ekmartin · 2017-08-01T05:54:36Z

lib/bot.js

@@ -258,7 +268,9 @@ class Bot {
  sendToIRC(message) {
    const author = message.author;
    // Ignore messages sent by the bot itself:
-    if (author.id === this.discord.user.id) return;
+    if (author.id === this.discord.user.id ||
+        Object.keys(this.webhooks).some(chan => this.webhooks[chan].id === author.id)


ekmartin · 2017-08-01T05:56:10Z

lib/bot.js

@@ -335,6 +347,44 @@ class Bot {
    return null;
  }

+  findWebhook(ircChannel) {
+    const discordChannelName = this.invertedMapping[ircChannel.toLowerCase()];
+    if (discordChannelName) {


Might as well do: return discordChannelName && this.webhooks[discordChannelName].

ekmartin · 2017-08-01T05:58:31Z

lib/bot.js

+    const guildMembers = this.findDiscordChannel(channel).guild.members;
+    const findByNicknameOrUsername = caseSensitive =>
+      (member) => {
+        if (caseSensitive) {


Are discord nicknames case sensitive or insensitive? Can there be two users with the same username/nickname with different casing? I think we can simplify this logic as long as we know what assumptions we're making.

Insensitive and yes, which is why I suggested (I think I suggested?) matching case sensitively first and then falling back to case insensitive if there's no match found. (Since IRC users have no way to, in a dropdown, select the correct one of the various case sensitivities – Discord users have this ability since Discord translates the mention in its UI – I thought it best to allow them to force the right one by picking the right case, where relevant.)

You can even have two users with the exact same nickname (same case): http://i.imgur.com/ACuYlcS.png + http://i.imgur.com/oD3JEfK.png
(And @maorninja suggested the change :) )

(Yes but I suggested here the logic for how to implement it. :P)

( right :D )

Personally, I would blame Discord because they allow 2 users with the exact same nickname, but that's just my opinion. I wish there was a bot that could block that.

ekmartin · 2017-08-01T05:59:26Z

lib/bot.js

+          username: author,
+          text,
+          avatarURL: this.getDiscordAvatar(author, channel)
+        }).catch(logger.error);


Would the error here not bubble up to our on('error' handler? (I don't know)

Webhook clients aren't attached to the main discord client:

this.discord = new discord.Client({ autoReconnect: true });
vs.
this.webhooks[channel] = { [...], client: new discord.WebhookClient(id, token);}
So there's no way errors will bubble up to the main on('error'

I could probably define a function and use it for both error handling, though.

ekmartin · 2017-08-01T06:00:53Z

lib/bot.js

+    const webhook = this.findWebhook(channel);
+    if (webhook) {
+      logger.debug('Sending message to Discord via webhook', withMentions, channel, '->', `#${discordChannel.name}`);
+      webhook.client.sendMessage(


formatting nitpick:

webhook.client.sendMessage(withMentions, { text, username: author, avatarUrl: this.getDiscordAvatar(author, channel) })

(could even do const avatarURL = this.getDiscordAvatar(author, channel); first - always optimize for readability)

ekmartin · 2017-08-01T06:01:28Z

test/bot.test.js

@@ -807,4 +810,81 @@ describe('Bot', function () {
    this.sendStub.should.have.been.calledOnce;
    this.sendStub.getCall(0).args.should.deep.equal([msg]);
  });
+
+  it('should create webhooks clients for each webhook url in the config', function () {


Lots of tests - nice!

Variables renaming Improved code readability

coveralls · 2017-08-03T11:54:23Z

Coverage increased (+0.07%) to 99.327% when pulling 549b8fb on Fiaxhs:master into ddd9c54 on reactiflux:master.

qaisjp · 2017-08-16T05:55:34Z

What if we fall back to adorable.io avatars?

See https://github.com/ekmartin/slack-irc/ and qaisjp/go-discord-irc@8593484

qaisjp · 2017-08-16T06:06:19Z

Also, please send feedback to Discord asking them to put some focus on this! https://trello.com/c/6ZZTEBJw

@Kakkela's #230 (comment) shows a major block

Throne3d · 2017-08-16T19:35:49Z

@qiasjp

I'm not sure there's a need to fallback to specific avatars – if the users haven't got Discord accounts with avatars, then they haven't chosen an avatar, so it seems sensible to stick to what Discord does by default? (That is, the standard game controller picture.)

I agree that there should be a warning about the bug when this gets merged. That's a pretty big issue on Discord's end. I don't think it should be a block to us merging this, but instead added as a clear warning in the relevant config area, so users of the individual servers can choose whether to enable it despite this.

qaisjp · 2017-08-16T23:18:28Z

@Throne3d yes but the Discord default avatars have a bit of variety. the webhook currently has the same default avatar across usernames.

fyi the only reason I suggested it is because @ekmartin's slack bridge supports it

qaisjp · 2017-08-17T17:54:04Z

Also, something to mention to people reading this issue:

A nice workaround for the Android/web bug is to alternate the webhook per user

(This wouldn't be suitable for reactiflux/discord-irc, but my purpose build discord-irc bridge bot will do this)

qaisjp · 2017-08-24T20:10:30Z

lib/bot.js

+    const webhook = this.findWebhook(channel);
+    if (webhook) {
+      logger.debug('Sending message to Discord via webhook', withMentions, channel, '->', `#${discordChannel.name}`);
+      const avatarURL = this.getDiscordAvatar(author, channel);


During netsplits author is nil. This would usually send some message like <${username}> ${message} (or something similar, I can't recall. but the message would contain ${} sent verbatim). Now this.getDiscordAvatar crashes with TypeError: Cannot read property 'toLowerCase' of undefined

NightScript370 · 2017-09-24T23:13:25Z

EDIT: NVM.

coveralls · 2018-01-11T10:35:15Z

Coverage increased (+0.1%) to 98.48% when pulling 90c901a on Fiaxhs:master into 68d0fd8 on reactiflux:master.

Fiaxhs · 2018-01-15T10:15:56Z

@Kovaelin Here's what my config looks like:

{
  "channelMapping": {
    "#machinzone": "#machinzonefr",
    "#crypto": "#machincrypto",
    "#dev": "#machindev"
  }
  [...]
  "webhooks": {
    "#crypto": "https://discordapp.com/api/webhooks/xxxx/XXXXX",
    "#machinzone": "https://discordapp.com/api/webhooks/xxxx/XXXXX",
    "#dev": "https://discordapp.com/api/webhooks/xxxx/XXXXX"
  }
}

Kovaelin · 2018-01-15T14:31:59Z

I still can't seem to get the webhook to work. Do I have to change any of the other files? Does it work if I use the Discord channel ID rather than the channel name?

My config file looks like this:
[
{
"nickname": "ROBOBOT",
"server": "IRC.SERVER.BOT",
"discordToken": "TOkeN",
"commandCharacters": ["!"],
"ircStatusNotices": true,
"ircOptions": {
"floodProtection": false,
"encoding": "utf-8"
},
"channelMapping": {
"DISCORDIDNUMBERSA": "#IRC"
},
"webhooks": {
"NUMBERSA": "https://discordapp.com/api/webhooks/COPYPASTEA/COPYPASTEB"
}
},

{
"nickname": "ROBOBOT2",
"server": "IRC.SERVER.BOT",
"discordToken": "TOkeN",
"channelMapping": {
"DISCORDIDNUMBERSB": "#IRC2"
},
"commandCharacters": ["!"]
}
]

Fiaxhs · 2018-01-15T14:40:06Z

@Kovaelin Could you open an issue on https://github.com/Fiaxhs/discord-irc instead, please?

ryantheleach · 2018-01-23T11:47:52Z

Still looking for this even with the bug. This would be a huge improvement to quality of life

Kakkela · 2018-03-21T22:29:01Z

Webhook merging bug on Android is now moved to "Claimed fixed, needs verification"
I'm going to test on my bridge today or tomorrow. Not sure if it's fixed on just Alpha channel on Android or what.

https://trello.com/c/6ZZTEBJw/278-webhook-incorrectly-stacks-message-authors-on-android-client

qaisjp · 2018-03-21T22:40:36Z

Omg excited. Please keep us updated :)

…

On Wed, 21 Mar 2018, 10:29 pm Iku, ***@***.***> wrote: Webhook merging bug on Android is now moved to "Claimed fixed, needs verification" I'm going to test on my bridge today or tomorrow. Not sure if it's fixed on just Alpha channel on Android or what. https://trello.com/c/6ZZTEBJw/278-webhook-incorrectly-stacks-message-authors-on-android-client — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#230 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4WasiBpdYzaXZsPzjqPNtLG0Lt89sUks5tgtQwgaJpZM4NN89p> .

Kakkela · 2018-03-22T09:36:11Z

Yes this does now indeed work on Android (all channels) as intended and no unnecessary merging happens.

qaisjp · 2018-03-22T11:47:48Z

And it's fixed on web too!!

…

On Thu, 22 Mar 2018, 9:36 am Iku, ***@***.***> wrote: Yes this does now indeed work on Android (all channels) as intended and no unnecessary merging happens. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#230 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4Wahd1WFQzo45R210JnMnMbLjBiy6Zks5tg3CNgaJpZM4NN89p> .

ekmartin · 2018-03-22T14:34:53Z

Published in version 2.6.0 of discord-irc - thank you for your patience!

I wonder if the usernames on the Discord side should be suffixed with (IRC) to make what's happening clearer though? If you think that's a good idea we can publish a new version later.

qaisjp · 2018-03-22T14:39:54Z

Side note: discord webhooks have a minimum name length of 2 characters, and having a suffix of `(IRC)` would be an easy countermeasure.

…

On Thu, 22 Mar 2018 at 14:34 Martin Ek ***@***.***> wrote: Published in version 2.6.0 <https://github.com/reactiflux/discord-irc/blob/master/CHANGELOG.md#260---2018-03-22> of discord-irc - thank you for your patience! I wonder if the usernames on the Discord side should be suffixed with (IRC) to make what's happening clearer though? If you think that's a good idea we can publish a new version later. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#230 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4WalOpkW_cP004I-c5nKEfn50ToSWgks5tg7aOgaJpZM4NN89p> .

ekmartin · 2018-03-22T14:41:25Z

Opened an issue for it: #377 - PRs welcome!

Kovaelin · 2018-04-02T02:36:23Z

Thank you for this update!

Fiaxhs added 4 commits February 24, 2017 16:43

Implemented webhooks with matching avatars

2f59cbe

use discord.js 11.1.0

3ca43ae

Merge remote-tracking branch 'upstream/master'

9ea90a9

Reworked webhook flow

2c05038

IRC formatting

remove useless brackets

b4452dd

Throne3d requested changes May 16, 2017

View reviewed changes

ekmartin mentioned this pull request May 23, 2017

Enable IRC timestamps in Discord chat #244

Closed

Fiaxhs added 2 commits May 30, 2017 14:35

Merge remote-tracking branch 'upstream/master'

fa9ef71

getDiscordAvatar refactoring

2bf761b

Added test

Throne3d previously approved these changes May 30, 2017

View reviewed changes

Added webhooks section to readme

8c0c11a

Throne3d requested changes Jul 1, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master'

d62dcfb

ekmartin requested changes Aug 1, 2017

View reviewed changes

One pass for webhook options

549b8fb

Variables renaming Improved code readability

qaisjp suggested changes Aug 24, 2017

View reviewed changes

Throne3d mentioned this pull request Oct 22, 2017

Improve IRC → Discord mentions for non-word characters and partial word matches #273

Merged

Merge remote-tracking branch 'upstream/master'

90c901a

Fiaxhs closed this Mar 22, 2018

Fiaxhs reopened this Mar 22, 2018

ekmartin merged commit c7263b7 into reactiflux:master Mar 22, 2018

djtyral mentioned this pull request May 16, 2018

Cannot get the webhook functionality to work #400

Closed

Use webhooks to reflect irc usernames + avatars #230

Use webhooks to reflect irc usernames + avatars #230

Conversation

Fiaxhs commented May 2, 2017

coveralls commented May 2, 2017 • edited Loading

coveralls commented May 2, 2017 • edited Loading

Throne3d commented May 2, 2017

Fiaxhs commented May 2, 2017

Throne3d left a comment

Choose a reason for hiding this comment

Fiaxhs commented May 17, 2017 via email

coveralls commented May 30, 2017 • edited Loading

Fiaxhs commented May 30, 2017

Kakkela commented May 30, 2017 • edited Loading

NightScript370 commented May 30, 2017

Throne3d commented May 30, 2017 • edited Loading

Fiaxhs commented May 30, 2017

coveralls commented May 30, 2017 • edited Loading

Throne3d commented May 30, 2017

NightScript370 commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Throne3d Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Throne3d Jul 1, 2017 • edited Loading

Choose a reason for hiding this comment

Throne3d commented Jul 1, 2017

NightScript370 commented Jul 4, 2017

Fiaxhs commented Jul 31, 2017

ekmartin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fiaxhs Aug 3, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 3, 2017 • edited Loading

qaisjp commented Aug 16, 2017

qaisjp commented Aug 16, 2017

Throne3d commented Aug 16, 2017 • edited Loading

qaisjp commented Aug 16, 2017

qaisjp commented Aug 17, 2017

Choose a reason for hiding this comment

NightScript370 commented Sep 24, 2017 • edited Loading

coveralls commented Jan 11, 2018 • edited Loading

Fiaxhs commented Jan 15, 2018

Kovaelin commented Jan 15, 2018

Fiaxhs commented Jan 15, 2018

ryantheleach commented Jan 23, 2018

Kakkela commented Mar 21, 2018

qaisjp commented Mar 21, 2018 via email

Kakkela commented Mar 22, 2018

qaisjp commented Mar 22, 2018 via email

ekmartin commented Mar 22, 2018

qaisjp commented Mar 22, 2018 via email

ekmartin commented Mar 22, 2018

Kovaelin commented Apr 2, 2018

coveralls commented May 2, 2017 •

edited

Loading

coveralls commented May 2, 2017 •

edited

Loading

coveralls commented May 30, 2017 •

edited

Loading

Kakkela commented May 30, 2017 •

edited

Loading

Throne3d commented May 30, 2017 •

edited

Loading

coveralls commented May 30, 2017 •

edited

Loading

Throne3d Aug 16, 2017 •

edited

Loading

Throne3d Jul 1, 2017 •

edited

Loading

Fiaxhs Aug 3, 2017 •

edited

Loading

coveralls commented Aug 3, 2017 •

edited

Loading

Throne3d commented Aug 16, 2017 •

edited

Loading

NightScript370 commented Sep 24, 2017 •

edited

Loading

coveralls commented Jan 11, 2018 •

edited

Loading