Implement !tunnel command

[Matiiss]

Fixes #3512

[Matiiss]

I do have a couple of questions though, first, so currently I have made it so that only discord.TextChannel are supported, which means you can't use this command inside those voice channel text channels or threads, but I suppose it'd be fine to allow its usage there, too.

Another thing is that if I change the command method signature to use discord.abc.MessageableChannel, then suddenly !tunnel is being interpreted as a "tag" and it's trying to find a match for it among the other tags, instead of running this command. I'd like some advice on how to make it behave like it does with discord.TextChannel or like make the tag search skip it or something.

Then also, if I use the discord.TextChannel annotation (and presumably it would be similar if I fixed it so that discord.abc.MessageableChannel would work), if I provide a wrong ID (same length, just changed some digit), it doesn't appear to complain at any level, it seems it just sets that argument to None, which, I'm not sure if that's the behavior that I'd like. That is, you either provide a correct argument for the channel or it uses the default behavior without letting you know that you may have messed up.

Should there be checks for whether the given channel is a DM channel or some such? Currently it just seems to treat it as incorrect channel ID (probably because it's not a discord.TextChannel), so it just sets it to None, but if switching to discord.abc.MessageableChannel, that should probably be explicitly checked?

Similarly, what to do about ctx.guild, should that throw some more specific error or should it realistically never be None (which is my assumption, hence the AssertionError)? I used raise AssertionError` mostly for typing purposes here (in places where it also should never be realistically raised anyway).

Is there a simple way to do overloads for this command? So that with one argument it's !tunnel [destination_channel], but then with two it's !tunnel [source_channel] [destination_channel]?

Any shortcuts/convenience functions I should use for say permission checks from the existing code base?

Does this need unit or integration tests?

[jb3]

Nice initial implementation, a few feedback comments. Will address your other comments in a second.

[jb3]

so currently I have made it so that only discord.TextChannel are supported, which means you can't use this command inside those voice channel text channels or threads, but I suppose it'd be fine to allow its usage there, too.

Sounds fine — with the necessary cooldown added as mentioned so users cannot spam this in places we have less visibility.

Another thing is that if I change the command method signature to use discord.abc.MessageableChannel, then suddenly !tunnel is being interpreted as a "tag" and it's trying to find a match for it among the other tags, instead of running this command. I'd like some advice on how to make it behave like it does with discord.TextChannel or like make the tag search skip it or something.

This sounds weird, do you get the same if you try use GuildChannel instead?

That is, you either provide a correct argument for the channel or it uses the default behavior without letting you know that you may have messed up.

I wonder if using a https://discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.ext.commands.TextChannelConverter might help?

Should there be checks for whether the given channel is a DM channel or some such? Currently it just seems to treat it as incorrect channel ID (probably because it's not a discord.TextChannel), so it just sets it to None, but if switching to discord.abc.MessageableChannel, that should probably be explicitly checked?

You can use a guild-only decorator to enforce this.

Similarly, what to do about ctx.guild, should that throw some more specific error or should it realistically never be None (which is my assumption, hence the AssertionError)? I used raise AssertionError` mostly for typing purposes here (in places where it also should never be realistically raised anyway).

Same as above.

Is there a simple way to do overloads for this command? So that with one argument it's !tunnel [destination_channel], but then with two it's !tunnel [source_channel] [destination_channel]?

Not easily without doing most of the argument handling yourself.

I was going to suggest that for this initial implementation though we ditch the source channel support. Especially since the user who did the tunneling isn't mentioned in the tunneling messages it seems ripe for abuse (e.g. someone spamming in bot commands with another source & dest).

For now, let's keep things simple and just implement an optional destination channel.

Does this need unit or integration tests?

You can if you want to (and if you remove the source stuff it should be easier), we do have some utilities for mocking Discord components here and it's not too hard to add new bits.

[jb3]

Also, on the last point about abuse: can we include the user who tunneled on both the source and destination message (username is fine, doesn't need to be a mention), so that we can easily see any problems without having to look through message logs.

[wookie184]

Thanks for working on this :)

Potential simplification:

1. Get the channels in the cog_load and store them in in a list e.g. self.channels
2. replace get_least_active_channel_id with get_least_active_channel that looks something like

   return min(self.channels, key=lambda c: thattimeutilthing(c.last_message_id)
   

3. Then the channel_id_to_timestamp logic + on_message aren't needed at all, and it's a bit simpler as you're passing the channel around instead of the channel id (don't need to fetch it).

[Matiiss]

This sounds weird, do you get the same if you try use GuildChannel instead?

Yes

I wonder if using a https://discordpy.readthedocs.io/en/latest/ext/commands/api.html#discord.ext.commands.TextChannelConverter might help?

Yes, it does, it seems I then just annotate the destination channel as str and any argument would be passed directly to it without some weird implicit magic going on. That is nice, currently the command is set up to expect a discord.TextChannel since that is the only converter used here. I think it's mostly fine, if you give it a thread id, like a help thread id, it will throw a BadArgument and that will be the response from the bot as well, so it's clear. It does mean that essentially you can't tunnel into a thread, but I don't think that's super necessary at the moment? You can tunnel out of one though.

Now, since the text uses the word "continued", should the command have an alias called "continue" as well? I quite like "tunnel", I think it's fun, but if the text now uses "continued", it might be more intuitive to have an option to call it via such an alias as well? I'm not sure, tbh, if the command is called "tunnel", then maybe it's not necessary to use the word "continued" in the text and we can use the word "tunneled" in the text as well, since it shouldn't be too confusing on what it means given the invocation of the command is probably gonna be pretty much nearby and also one could infer from the fact that there are two messages that link to each other in a "tunnel-like fashion". Not sure.

I saw there was a decorator for cooldown where some roles can be excluded from that (bot.utils.checks.cooldown_with_role_bypass), but I'm not sure that's super necessary here, besides that function had a comment saying that it might break at some point. So the cooldown is there just mainly for abuse prevention I suppose, I don't imagine this command would actually get used extensively in practice.

Otherwise, I think this is ready for another review, I might eventually maybe add those integration tests as well, but who knows, that might take another month to get there 😅