Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ban user by id command /banid #4411

Merged
merged 7 commits into from Feb 26, 2023

Conversation

iProdigy
Copy link
Contributor

@iProdigy iProdigy commented Feb 25, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

With the /ban command migrated to Helix, we are no longer able to ban suspended or deactivated accounts because helix getUsers is not able to resolve the user_id associated with the login name. However, if a moderator already knows the user_id, helix banUser is able to ban the account, even if suspended or deactivated. Thus, this PR adds a /banid command to allow banning users by id.

Note: similar concerns apply for timeout/unban, but are not covered by this PR (and are not as important imo)

chatterino_W6yakM3S6Z

@@ -2829,6 +2829,54 @@ void CommandController::initialize(Settings &, Paths &paths)
return "";
});

this->registerCommand("/banid", [formatBanTimeoutError](
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to other suggestions for the command name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is fine for me

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link
Collaborator

@Felanbird Felanbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality seems to work as I would expect it to 👍

CHANGELOG.md Show resolved Hide resolved
src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
@@ -2829,6 +2829,54 @@ void CommandController::initialize(Settings &, Paths &paths)
return "";
});

this->registerCommand("/banid", [formatBanTimeoutError](
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this name is fine for me

Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com>
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpicks, haven't tested functionality yet but it looks simple enough 👍

CHANGELOG.md Outdated Show resolved Hide resolved
src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
src/controllers/commands/CommandController.cpp Outdated Show resolved Hide resolved
@Mm2PL
Copy link
Collaborator

Mm2PL commented Feb 26, 2023

Note: similar concerns apply for timeout/unban, but are not covered by this PR (and are not as important imo)

Maybe special syntax could be made so all /(un)timeout, /(un)ban commands simply work from the user's point of view, for example: /ban ["id"] (name) (reason) or /ban #(id) (reason).

iProdigy and others added 2 commits February 26, 2023 11:07
Co-authored-by: pajlada <rasmus.karlsson@pajlada.com>
@pajlada
Copy link
Member

pajlada commented Feb 26, 2023

Note: similar concerns apply for timeout/unban, but are not covered by this PR (and are not as important imo)

Maybe special syntax could be made so all /(un)timeout, /(un)ban commands simply work from the user's point of view, for example: /ban ["id"] (name) (reason) or /ban #(id) (reason).

If relevant use-cases come up in the future, we'll figure out a uniform solution for this that can be applied to all commands but for now we'll go with this simple one that has an actively request use-case.

@pajlada pajlada changed the title feat: add ban user by id command Add ban user by id command /banid Feb 26, 2023
@pajlada pajlada enabled auto-merge (squash) February 26, 2023 19:39
@pajlada pajlada merged commit b5b8550 into Chatterino:master Feb 26, 2023
@iProdigy iProdigy deleted the feature/command-ban-by-id branch February 26, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants