Skip to content

Migrate /warn command #249

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

Merged
merged 53 commits into from
Dec 17, 2021
Merged

Migrate /warn command #249

merged 53 commits into from
Dec 17, 2021

Conversation

RealYusufIsmail
Copy link
Contributor

@RealYusufIsmail RealYusufIsmail commented Nov 6, 2021

Overview

This adds the /warn user reason command, which lets moderators warn users. Warnings can then be later retrieved using /audit (#312 ).

As usual, the command will try to DM the target to inform them about the action, have the usual permission checks (heavy moderation role in this case) and similar. Nothing really special to see here.

Part of #63 .

  • The command saves the warns
  • Users without ban perms can not use the warn command

@RealYusufIsmail RealYusufIsmail requested review from a team as code owners November 6, 2021 14:44
Copy link
Member

@Zabuzard Zabuzard left a comment

Choose a reason for hiding this comment

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

Before looking at the code, I am proposing to change the UX to:

/warn user reason

and then having a separate

/audit user

command which shows:

  • all warnings
  • all bans
  • all kicks
  • all mutes
    (with reason)

(it is okay if the command can only capture actions that have been executed with this bot - no need to check the actual audit log)

@RealYusufIsmail
Copy link
Contributor Author

So are you saying i just delete the retrieve part.

@Zabuzard
Copy link
Member

Zabuzard commented Nov 6, 2021

Dont request a new review from me unless you think you fixed all my mentioned issues.

You know the story of the boy who cried for the wolf and then nobody believed him and came anymore when the real wolf was there? This is kinda how I feel like when you do this. It has the opposite effect of what you want to achieve, namely that I will delay my review and ignore your "requests".

@RealYusufIsmail
Copy link
Contributor Author

Ok I will improve on that note

@Zabuzard
Copy link
Member

Zabuzard commented Nov 8, 2021

Please make the command role instead of permission based (see #244 , use the heavy moderation role pattern). Best would be if you rebase ur branch on top of the branch from #244 or just wait until it is merged.

@Zabuzard Zabuzard added new command Add a new command or group of commands to the bot priority: major blocked This issue is currently blocked by another issue (see comments) labels Nov 8, 2021
@Zabuzard
Copy link
Member

Zabuzard commented Nov 8, 2021

Blocked by #244

@Zabuzard Zabuzard added this to the Migrate existing commands milestone Nov 8, 2021
@RealYusufIsmail
Copy link
Contributor Author

Please make the command role instead of permission based (see #244 , use the heavy moderation role pattern). Best would be if you rebase ur branch on top of the branch from #244 or just wait until it is merged.

Yeah I understand. I was waiting for the ban/kick pr to go through originally

The issue with ActionRecord was resolved but does not appear on Github which is a problem for their side. @Zabuzard just letting you know
@Zabuzard Zabuzard removed a link to an issue Dec 14, 2021
@Zabuzard Zabuzard changed the title Migrated warn command Migrate /warn command Dec 14, 2021
Zabuzard
Zabuzard previously approved these changes Dec 14, 2021
@Zabuzard Zabuzard disabled auto-merge December 14, 2021 08:59
@Zabuzard
Copy link
Member

Zabuzard commented Dec 14, 2021

The commit history of this PR is absolutely terrible. It must be squashed or rewritten.

DO NOT REBASE THIS, IT MUST BE SQUASHED (or rewritten first)

@Zabuzard Zabuzard enabled auto-merge (squash) December 14, 2021 09:01
@Zabuzard Zabuzard mentioned this pull request Dec 14, 2021
2 tasks
Added the space back and added Why you want to warn the user
@RealYusufIsmail
Copy link
Contributor Author

Screenshot 2021-12-16 at 21 40 46

?

@Zabuzard
Copy link
Member

Screenshot 2021-12-16 at 21 40 46 ?

The changes requested by Tais have been implemented by you, hence I dismissed his change-request.

@Zabuzard Zabuzard disabled auto-merge December 17, 2021 09:16
@Zabuzard Zabuzard merged commit 744c29d into Together-Java:develop Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Add a new command or group of commands to the bot priority: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants