Red-DiscordBot icon indicating copy to clipboard operation
Red-DiscordBot copied to clipboard

[tempban] Allow tempbans by `UserID` instead of `Member`

Open cool-aid-man opened this issue 3 years ago • 14 comments

Type of feature request

Command

Description of the feature you're suggesting

  • Command: tempban
  • Cog: Mod

Hi! By default, the tempban command checks for the user to be present in the server before banning them temporarily. That means you cannot ban any user who's not present in the server. Which you should be able to.

I don't think it's discord limitation since the you can do the same with other bots and even you can ban users with the ban from the same cog, regardless of their presence in the server.

So it'd be great to have that functionality on red, allowing tempban to check for UserID instead of member so that we can ban people after they leave the server temporarily. Thank you!

Anything else?

No response

cool-aid-man avatar Nov 23 '22 06:11 cool-aid-man

Seems reasonable, I can't see a reason not to allow this.

One thing we need to consider is the behavior when a user is already banned - should we send a failure message or should we downgrade a ban to a tempban?

Jackenmen avatar Nov 23 '22 11:11 Jackenmen

One thing we need to consider is the behavior when a user is already banned - should we send a failure message or should we downgrade a ban to a tempban?

I think downgrading the ban to a tempban would be more appropriate.

japandotorg avatar Nov 23 '22 11:11 japandotorg

I think sending a failure message when user already banned is a better choose tbh.

ltzmax avatar Nov 23 '22 12:11 ltzmax

May as well just split the difference and have a confirmation menu before downgrading the ban to a tempban.

Zephyrkul avatar Nov 23 '22 12:11 Zephyrkul

Sending a failure msg, saying "User is already banned" would be a better option (when anyone tries to ban/tempban the same user who's banned already) and having two separate cmds, (ban and tempban) also gives you more room to play with, plus it doesn't make the ban command complicated.

cool-aid-man avatar Nov 23 '22 12:11 cool-aid-man

At least for me the main reason for using a bot like red is to automate tedious administrative tasks. Wanting to downgrade a ban to a tempban is one of those. You would have to either remember to unban the user or unban and then re-(temp-)ban them. Both sound pretty annoying if we're being honest.

I think it is fairly reasonable to say that a typical order of events would be:

  1. A user is being naughty
  2. A mod quickly comes to the rescue and bans through discord or with the quicker to type ban command
  3. The mod wants to issue a tempban instead because while the user was naughty, they were not very naughty

TLDR: Tempban should probably downgrade a ban.

That being said, I also like the Zeph way of life. More choices more better.

Dav-Git avatar Nov 23 '22 13:11 Dav-Git

Yeah, from usability standpoint it would probably make more sense to allow a downgrade. I like the prompt idea, though I'll wait with the decision until another org member or maybe few more community members chime in to make the choice easier.

For potential contributors, I suggest going with the prompt idea if another decision isn't made by tomorrow.

Jackenmen avatar Nov 23 '22 13:11 Jackenmen

A prompt would be much better for usability and not adding additional commands.

DariusStClair avatar Nov 23 '22 13:11 DariusStClair

If the point is to support automation, I'd suggest a [override_ban=False] argument, where when False it just displays

That user is already banned! Use <command> to override that ban with a temporary ban.

This allows aliass to be configured for bot owners who always want to override, and allow cogs that invoke other commands to more easily hook into this without it being a required part of the command. If the prompt idea is chosen, it should at least support ctx.assume_yes.

Flame442 avatar Nov 23 '22 17:11 Flame442

I think that's overly complicated and trying to appease everyone for the sake of it. We should definitely support ctx.assume_yes though.

Jackenmen avatar Nov 23 '22 18:11 Jackenmen

That's already what [p]bankset global does. If a user does not want to override temp bans, it will fail for them and they can ignore the command recommendation. If they want to be prompted to override temp bans, that's a built in prompting system for them to confirm. If they know they will always want to override, they can either remember to pass in the arg (like I do for bankset global) or add an alias with that pre-filled in.

I don't know how that's any more complicated than a similar prompt that always happens, with the one difference between the two options being that in a prompt-only approach, the only way to always get it to override is to make a separate command that calls it with assume_yes. Otherwise, the UX and code complexity is essentially the same unless I'm missing something.

Flame442 avatar Nov 23 '22 18:11 Flame442

  1. UX of having to copy the command, paste it and edit it to add a bool value before sending again is worse than being able to just accept the yes/no prompt.
  2. [p]bankset global is otherwise argumentless which makes it less odd to put argument like that there. In case of tempban, we would have to put it at the beginning or somewhere else before the reason argument which is IMO going to be harder to use and understand in comparison to the same case in bankset. It also could introduce ambiguity between someone passing number to the days argument vs passing it to the new bool. This will be even more confusing to existing users of the command.

Jackenmen avatar Nov 23 '22 19:11 Jackenmen

I hadn't considered the existing arguments of the commands, that's a valid concern. ~~If only arg parse wasn't confusing to users...~~

Flame442 avatar Nov 23 '22 19:11 Flame442

Alright, seems like we agree on this after more concerns about the [override_ban=False] approach have been listed here.

To summarize:

  • [p]tempban should allow banning by a user ID using Union[discord.Member, RawUserIdConverter]
  • if using the [p]tempban command would cause a "downgrade" from a permanent ban to a temporary ban, the command should show a prompt asking if the command invoker wants to proceed
  • the prompt should be skipped (and assumed to be agreed to) if ctx.assume_yes evaluates to True

PRs welcome!

Jackenmen avatar Nov 25 '22 15:11 Jackenmen