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

Better bans #230

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Better bans #230

wants to merge 7 commits into from

Conversation

Arkaeriit
Copy link
Collaborator

This PR adds the following improvement related to bans:

  • Temporary bans are robust against server restart.
  • User banned temporary can see how long they have to wait.
  • Cleaner in-code ban API.
  • Devbot banning and anti-spam banning are respectively one minute and one month instead of being forever.

Instead of using a fire-and-forget goroutine,
time-based banning work with additional ban data.
main.go Outdated
Log.Println("Rejected " + u.Name + " [" + host + "] (banned)")
u.writeln(Devbot, "**You are banned**. If you feel this was a mistake, please reach out to the server admin. Include the following information: [ID "+u.id+"]")
if banInfo.UseTime {
u.writeln(Devbot, "You will be unbaned on "+banInfo.UnbanTime.Format(time.RFC3339))
Copy link
Owner

Choose a reason for hiding this comment

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

use a duration here instead of an absolute time

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like that:

devbot: You will be unbaned in 41 days, 15 hours, 56 minutes, and 8 seconds.

or like that:

devbot: You will be unbaned in 13 seconds.

Copy link
Owner

Choose a reason for hiding this comment

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

It's quite verbose. Which could be fine. But we do also have a pretty duration printer here: https://github.com/Arkaeriit/devzat/blob/c98c58e03fd3703f5d500424e886e5f06fe0768d/util.go#L420

what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ho right, I didn't remember this function. printPrettyDuration is not very suitable for duration longer than a day, so not great for bans.
But maybe we can merge printPrettyDuration and formatDuration to prevent code duplication.
I'll make a proposition of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with full unit names. So it changes the way the relative time display is. If you don't like it, I will go back to units symbols.
image

Copy link
Owner

Choose a reason for hiding this comment

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

I see what you mean. maybe it's better to keep it short but add ability to display days using the unit 'd'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I reverted back to using 1 letter units with no space between the numeric value and the unit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like that now:
image

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 9, 2024
main.go Show resolved Hide resolved
util.go Show resolved Hide resolved
It's now in the format '9d 4h 3m'.
This makes it easier to remember which key is
which.
@Arkaeriit
Copy link
Collaborator Author

I added an additional feature of storing names in the DB and displaying them when doing lsbans. This should make it easier to clean erroneous bans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants