Add ignore #225

Learath2 · 2017-04-22T10:33:33Z

Small feature I added to use with our development channel. We have github hooks on both sides.

coveralls · 2017-04-22T10:34:53Z

Coverage increased (+0.03%) to 98.319% when pulling 0e0d1a0 on Learath2:pr_ignore into 4d238d9 on reactiflux:master.

Throne3d · 2017-04-22T12:03:14Z

I don't suppose you can add tests for this? (It doesn't seem to have any at present, and while it seems very straightforward, it would be useful to have tests to make sure this functionality works as expected and doesn't break in the future.)

Edit: Also it doesn't look like it'll ignore these nicks for the ircStatusNotices – is that intentional? It seems like people might expect it to.

Learath2 · 2017-04-22T12:29:32Z

I'm not much of a js person nor can I see any tests to add. I'll gladly add them if you give me a hand :P
In our case the bots don't join or leave but that sounds like a nice idea. I'll add it in.

Throne3d · 2017-04-22T14:32:04Z

Sinon chai seems to be the test library in use (which I hadn't touched before I submitted PRs to here, like 4 weeks ago?). Suggested tests would be to add example configs into the bot, trigger the event (or the method) with an author who should be ignored (and maybe an author who should not be ignored), and ensure that only the one who shouldn't be ignored gets through.

In test/bot.test.js, for the Discord → IRC side of things:

it('should skip discord bots when sending to IRC if config enabled', function () {
  const bot = new Bot({ ...config, ignoreBots: true });
  bot.connect();

  const guild = createGuildStub();
  const botMessage = {
    content: 'bot msg',
    mentions: { users: [] },
    channel: { name: 'discord' },
    author: {
      username: 'examplebot',
      id: 'examplebotid',
      bot: true
    },
    guild
  };
  const otherMessage = {
    content: 'not bot msg',
    mentions: { users: [] },
    channel: { name: 'discord' },
    author: {
      username: 'exampleuser',
      id: 'exampleuserid'
    },
    guild
  };
  const expected = '<\u000309exampleuser\u000f> not bot msg';
  bot.sendToIRC(botMessage);
  bot.sendToIRC(otherMessage);
  ClientStub.prototype.say.should.have.been.calledOnce;
  ClientStub.prototype.say.getCall(0).args.should.deep.equal(['#irc', expected]);
});

and for the IRC → Discord side of things:

it('should skip IRC nicks specified in config', function () {
  const bot = new Bot({ ...config, ignoreNicks: ['botuser'] });
  bot.connect();

  bot.sendToDiscord('botuser', '#irc', 'test bot message');
  bot.sendToDiscord('testuser', '#irc', 'other test message');
  this.sendMessageStub.should.have.been.calledOnce;
  this.sendMessageStub.getCall(0).args.should.deep.equal(['**<testuser>** other test message']);
});

(I pulled your branch and tried these on it, and they pass and I think should work as expected.)

Your code won't, I don't think, ignore Discord messages from ignored nicks, which may be confusing from the name of the config – I'd suggest either adding that (and adding a test for it, and in that case the ignoreBots thing might be unnecessary) or maybe making it clearer with the config name? (ignoreIrcNicks maybe?) It should also be added to the documentation in README.md.

Once you've added the feature for ignoring join/parts, tests could be added for that in test/bot-events.test.js.

Throne3d

Bump about tests.

mjgreer · 2017-06-19T20:32:10Z

Not sure on the protocol here but I'd like to see this feature make it in. Does anyone mind if i make the requested changes and resubmit this? Don't know what the protocol is here.

Learath2 · 2017-06-19T22:42:14Z

Sorry, completely forgot about this, way too much work... I'll try to get it done tomorrow. It is a relatively basic feature so tests do seem unnecessary IMHO but I'll get them in :)

Learath2 · 2017-06-19T23:37:48Z

Or I'll get them in before I go to sleep :P

coveralls · 2017-06-19T23:40:17Z

Coverage increased (+0.05%) to 98.502% when pulling 8b13a2f on Learath2:pr_ignore into 80c3752 on reactiflux:master.

Throne3d · 2017-06-20T00:14:37Z

The tests are mostly so that if the whole thing gets restructured later in some other pull request we don't somehow accidentally remove this functionality. Instead of just ensuring the basic code works as it's intended to at the time of implementation (which it seems it should, on inspection, which is why I otherwise accept the PR), it's ensuring future code doesn't screw anything up.

I did suggest doing messages from non-bot users and ensuring those come through properly despite ignoring bots (that is, ensuring at least cursorily that non-bot users aren't filtered out when ignoring bots), but it doesn't seem all that necessary.

Thanks for the pull request! @ekmartin if this looks okay with you I think it's good to go.

mjgreer · 2017-06-20T00:22:22Z

hey no worries guys, was just offering my help. glad to see it it got worked out.

Learath2 · 2017-06-20T07:49:33Z

Added tests for the other case but maybe should check the exact message getting through too :/ Feel free to merge with or without the last commit.

coveralls · 2017-06-20T07:49:56Z

Coverage decreased (-1.6%) to 96.875% when pulling 5fddae9 on Learath2:pr_ignore into 80c3752 on reactiflux:master.

Throne3d · 2017-07-03T22:25:29Z

Could you rebase this onto master? There are some conflicts which need to be resolved and I also expect it'll fix the problem with the coverage (presumably it's complaining because this is a slightly outdated branch).

coveralls · 2017-07-04T00:38:29Z

Coverage increased (+0.04%) to 98.551% when pulling 1a9cf07 on Learath2:pr_ignore into 85a55e6 on reactiflux:master.

Helianthella · 2017-07-31T21:09:12Z

anyone still working on this? I could fork and apply the required changes on top, if any is needed

Learath2 · 2017-07-31T23:43:10Z

Well this is done, I even rebased it :P

ekmartin

Thanks for adding tests - just a few small comments.

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

lib/bot.js

@@ -36,6 +36,8 @@ class Bot {
    this.channels = _.values(options.channelMapping);
    this.ircStatusNotices = options.ircStatusNotices;
    this.announceSelfJoin = options.announceSelfJoin;
+    this.ignoreBots = options.ignoreBots;
+    this.ignoreIrcNicks = options.ignoreIrcNicks || [];


Should this support both IRC and Discord? I.e. ignore: { discord: [], irc: [] }

Well I needed it for one way. Thought about adding it to Discord->IRC as well but Discord has many ways of identifying a user, I just couldn't choose which one to use.

For the Discord ignore I'd expect to use something that's unlikely to change – either username#discriminator or username. Maybe match on those?

Userid would be the most watertight ignore method, no?

Oh, yeah, it could be a better idea since it doesn't change. It's less readily accessible, though, which might make username#discriminator better, especially since usernames and discriminators don't, I think, change much? (I'd go about getting the ID by either putting Discord in dev mode or using \@Username to grab the ID from the escaped mention, both of which seem more awkward than username#discriminator.)

Well it depends for what the ignore would be used the most, if it's used to prevent some bots to go through username#separator would be fine. But if it's to prevent some user to flood and kicking/banning/mute isn't option for some reason, he can just switch discord username to circumvent the ignore.

In both cases I don't think the getting userid part would be too complicated, most people running servers and bots already have dev mode on all the time.

I'm unclear where such a circumstance would arise? I agree that if such a situation did occur then user ID would be better, but don't expect it'd occur enough that it'd outweigh the added (minor) ease from using username#discriminator.

I mean, not that there's much difference between the two, so I'm mostly of the opinion "pick at least one of these" here.

I think a plain snowflake id would be best. The bot owner can right click a user and choose "copy ID" to get the ID (if they can spend the time to set this up and have a Discord app, then they can enable dev mode in the client)

ekmartin · 2017-08-01T05:41:02Z

lib/bot.js

@@ -131,6 +133,7 @@ class Bot {

    this.ircClient.on('nick', (oldNick, newNick, channels) => {
      if (!this.ircStatusNotices) return;
+      if (this.ignoreIrcNicks.indexOf(oldNick) !== -1) return;


this.ignoreIrcNicks.includes(oldNick)

ekmartin · 2017-08-01T05:41:08Z

lib/bot.js

@@ -148,6 +151,7 @@ class Bot {
    this.ircClient.on('join', (channelName, nick) => {
      logger.debug('Received join:', channelName, nick);
      if (!this.ircStatusNotices) return;
+      if (this.ignoreIrcNicks.indexOf(nick) !== -1) return;


ekmartin · 2017-08-01T05:41:14Z

lib/bot.js

@@ -159,6 +163,7 @@ class Bot {
    this.ircClient.on('part', (channelName, nick, reason) => {
      logger.debug('Received part:', channelName, nick, reason);
      if (!this.ircStatusNotices) return;
+      if (this.ignoreIrcNicks.indexOf(nick) !== -1) return;


ekmartin · 2017-08-01T05:41:17Z

lib/bot.js

@@ -177,6 +182,7 @@ class Bot {
    this.ircClient.on('quit', (nick, reason, channels) => {
      logger.debug('Received quit:', nick, channels);
      if (!this.ircStatusNotices || nick === this.ircClient.nick) return;
+      if (this.ignoreIrcNicks.indexOf(nick) !== -1) return;


ekmartin · 2017-08-01T05:44:33Z

test/bot-events.test.js

+    bot.ircClient.emit('names', channel, { [bot.nickname]: '' });
+
+    const nick = 'ignored';
+    const altnick = 'notignored';


ekmartin · 2017-08-01T05:44:39Z

test/bot.test.js

+    this.bot.connect();
+
+    const guild = createGuildStub();
+    const botmsg = {


ekmartin · 2017-08-01T05:44:45Z

test/bot.test.js

+      guild
+    };
+
+    const normalmsg = {


normalMessage

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

Sorry about the slow answer here - was skeptical of adding yet another couple config options, but sounds like people find it helpful so let's get it in.

mjgreer · 2017-09-05T20:49:26Z

Any chance of this making it into master any time soon? I've applied the changes here locally to get this functionality but i'd like to see it make it into a release

mjgreer · 2017-09-05T20:49:43Z

I'm happy to help but i'm not sure where things are at with this PR

Learath2 · 2017-09-08T18:18:43Z

Just needs a config option to do filtering for Discord->IRC as well. I don't have time to do this right now.

ekmartin · 2017-10-04T22:19:35Z

The ignore users part got fixed in #322 so I'm gonna close this. Sorry that we didn't get this merged, and thank you for your work @Learath2! If anyone wants to take a shot at creating a separate PR for the ignoreBots functionality I'd be open to merging that.

ssiyad approved these changes Apr 30, 2017

View reviewed changes

Throne3d requested changes Apr 30, 2017

View reviewed changes

Learath2 force-pushed the pr_ignore branch from 0e0d1a0 to 8b13a2f Compare June 19, 2017 23:37

Throne3d approved these changes Jun 20, 2017

View reviewed changes

Learath2 added 2 commits July 4, 2017 02:31

Add ignore

f395c2a

Add tests for ignore

1a9cf07

Learath2 force-pushed the pr_ignore branch from 5fddae9 to 1a9cf07 Compare July 4, 2017 00:35

ekmartin requested changes Aug 1, 2017

View reviewed changes

Use includes(), follow naming conventions

9bf7df2

ekmartin closed this Oct 4, 2017

Add ignore #225

Add ignore #225

Conversation

Learath2 commented Apr 22, 2017

coveralls commented Apr 22, 2017 • edited Loading

Throne3d commented Apr 22, 2017 • edited Loading

Learath2 commented Apr 22, 2017

Throne3d commented Apr 22, 2017 • edited Loading

Throne3d left a comment

Choose a reason for hiding this comment

mjgreer commented Jun 19, 2017

Learath2 commented Jun 19, 2017

Learath2 commented Jun 19, 2017

coveralls commented Jun 19, 2017 • edited Loading

Throne3d commented Jun 20, 2017 • edited Loading

mjgreer commented Jun 20, 2017

Learath2 commented Jun 20, 2017

coveralls commented Jun 20, 2017 • edited Loading

Throne3d commented Jul 3, 2017

coveralls commented Jul 4, 2017 • edited Loading

Helianthella commented Jul 31, 2017

Learath2 commented Jul 31, 2017

ekmartin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Learath2 Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Throne3d Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Throne3d Aug 2, 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

Choose a reason for hiding this comment

ekmartin commented Aug 1, 2017

mjgreer commented Sep 5, 2017

mjgreer commented Sep 5, 2017

Learath2 commented Sep 8, 2017

ekmartin commented Oct 4, 2017

coveralls commented Apr 22, 2017 •

edited

Loading

Throne3d commented Apr 22, 2017 •

edited

Loading

Throne3d commented Apr 22, 2017 •

edited

Loading

coveralls commented Jun 19, 2017 •

edited

Loading

Throne3d commented Jun 20, 2017 •

edited

Loading

coveralls commented Jun 20, 2017 •

edited

Loading

coveralls commented Jul 4, 2017 •

edited

Loading

Learath2 Aug 1, 2017 •

edited

Loading

Throne3d Aug 1, 2017 •

edited

Loading

Throne3d Aug 2, 2017 •

edited

Loading