Looks good to me, and I've heard it's been successfully running on pixie.town for a while.
This is fine, but this isn't ban syncing. This needs to handle the case of unbanning too, or at the very least some technical documentation to explain why we don't do that.
And actually I'd love some documentation on our docs site on what users can expect from kicks / bans.
176 | 176 | }); | |
177 | 177 | ||
178 | 178 | ||
179 | describe("Banning", () => { |
file says kicking
test says banning
new test file time?
How about renaming kicking.spec.js
to kicking-banning.spec.js
? I think they go well in one file.
792 | if (ircRooms[i].server.domain !== server.domain) { | ||
793 | return; | ||
794 | } | ||
795 | senderClient.ban(bannedNick, ircRooms[i].channel); |
Do we need to await either of these?
Why?
Because you're doing two operations here, and I'm not sure if one should proceed if the other fails? At the very least, run them both concurrently but await the result so we can throw an error. Node typically crashes the process these days if a unawaited promise throws.
I think if +b
fails (not sure why it would) and /kick
works that's better than if it fails and /kick
isn't ran because of that.
but sure I'll look into that
527 | this.log.debug("Banning %s from channel %s", nick, channel); | ||
528 | |||
529 | // best effort ban | ||
530 | await c.send("MODE", channel, "+b", nick + "!*@*"); |
I'm always a bit skeptical about these. I presume we're sure this is reasonably implementation independent.
We don't want to offer a ban command config option or anything like that?
Why would this be implementation dependent?
I'd love to know how we could offer ban config when banning from Matrix.
1266 | 1265 | await this.matrixHandler.onLeave(request, memberEvent, target); | |
1267 | 1266 | } | |
1268 | 1267 | } | |
1268 | else if (event.content.membership === "ban") { |
How do unban?
Not implemented because I didn't wrap my head on how unbanning works on Matrix and how to check that.
I think this could nicely be supplemented with an !irc ban <mask>
command which sets +b modes, to allow for more flexible matching on nick/user/host parts.
Afaik removing a ban on the matrix side consists of the user's m.room.member
state event changing to anything that isn't ban
, so you could check the prev_content
key in state update events to unset the mode
I think this could nicely be supplemented with an
!irc ban <mask>
command which sets +b modes, to allow for more flexible matching on nick/user/host parts.Afaik removing a ban on the matrix side consists of the user's
m.room.member
state event changing to anything that isn'tban
, so you could check theprev_content
key in state update events to unset the mode
@f0x52 I'll look into that, thanks
Login to write a write a comment.
Split off from #1807, credit to @funderscore1