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

Moderation: Support appeal cooldowns #435

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion TPP.Core/Commands/Definitions/ModerationCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,17 @@ public class ModerationCommands : ICommandCollection
private readonly IBanLogRepo _banLogRepo;
private readonly ITimeoutLogRepo _timeoutLogRepo;
private readonly IUserRepo _userRepo;
private readonly IAppealCooldownLogRepo _appealCooldownRepo;
private readonly IClock _clock;

public ModerationCommands(
ModerationService moderationService, IBanLogRepo banLogRepo, ITimeoutLogRepo timeoutLogRepo, IUserRepo userRepo,
ModerationService moderationService, IBanLogRepo banLogRepo, ITimeoutLogRepo timeoutLogRepo, IAppealCooldownLogRepo appealCooldownRepo, IUserRepo userRepo,
IClock clock)
{
_moderationService = moderationService;
_banLogRepo = banLogRepo;
_timeoutLogRepo = timeoutLogRepo;
_appealCooldownRepo = appealCooldownRepo;
_userRepo = userRepo;
_clock = clock;
}
Expand All @@ -40,6 +42,9 @@ public ModerationCommands(
new Command("timeout", TimeoutCmd),
new Command("untimeout", UntimeoutCmd),
new Command("checktimeout", CheckTimeout),
new Command("setappealcooldown", SetAppealCooldown),
new Command("setunappealable", SetUnappealable),
new Command("checkappealcooldown", CheckAppealCooldown),
}.Select(cmd => cmd.WithModeratorsOnly());

private static string ParseReasonArgs(ManyOf<string> reasonParts)
Expand Down Expand Up @@ -162,4 +167,59 @@ private async Task<CommandResult> CheckTimeout(CommandContext context)
: $"{targetUser.Name} is not timed out. {infoText}"
};
}

private async Task<CommandResult> SetAppealCooldown(CommandContext context)
{
(User targetUser, TimeSpan timeSpan) =
await context.ParseArgs<User, TimeSpan>();
Duration duration = Duration.FromTimeSpan(timeSpan);
return new CommandResult
{
Response = await _moderationService.SetAppealCooldown(context.Message.User, targetUser, duration) switch
{
SetAppealCooldownResult.Ok => $"{targetUser.Name} is now eligible to appeal at {_clock.GetCurrentInstant() + duration}.",
SetAppealCooldownResult.UserNotBanned => $"{targetUser.Name} is not banned.",
SetAppealCooldownResult.AlreadyPermanent => throw new InvalidOperationException("Unexpected AlreadyPermanent repsonse"),
}
};
}

private async Task<CommandResult> SetUnappealable(CommandContext context)
{
User targetUser = await context.ParseArgs<User>();
return new CommandResult
{
Response = await _moderationService.SetUnappealable(context.Message.User, targetUser) switch
{
SetAppealCooldownResult.Ok => $"{targetUser.Name} is now ineligible to appeal their ban.",
SetAppealCooldownResult.UserNotBanned => $"{targetUser.Name} is not banned.",
SetAppealCooldownResult.AlreadyPermanent => $"{targetUser.Name} is already ineligible to appeal.",
}
};
}

private async Task<CommandResult> CheckAppealCooldown(CommandContext context)
{
User targetUser = await context.ParseArgs<User>();
if (!targetUser.Banned)
return new CommandResult { Response = $"{targetUser.Name} is not banned." };

AppealCooldownLog? recentLog = await _appealCooldownRepo.FindMostRecent(targetUser.Id);
string? issuerName = recentLog?.IssuerUserId == null
? "<automated>"
: (await _userRepo.FindById(recentLog.IssuerUserId))?.Name;
Comment on lines +208 to +210
Copy link
Member

Choose a reason for hiding this comment

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

Might be impossible since we never delete anyone from the users repo as far as I know, but as a safeguard against failing to look up a user, maybe sth. like this?

Suggested change
string? issuerName = recentLog?.IssuerUserId == null
? "<automated>"
: (await _userRepo.FindById(recentLog.IssuerUserId))?.Name;
string issuerName = recentLog?.IssuerUserId == null
? "<automated>"
: (await _userRepo.FindById(recentLog.IssuerUserId))?.Name
?? $"<unknown user id ${recentLog.IssuerUserId}>";

string infoText = recentLog == null
? "No logs available."
: $"Last action was {recentLog.Type} by {issuerName} " +
$"at {recentLog.Timestamp}";
if (recentLog?.Duration != null) infoText += $" for {recentLog.Duration.Value.ToTimeSpan().ToHumanReadable()}";

return new CommandResult { Response =
targetUser.AppealDate is null
? $"{targetUser.Name} is not eligible to appeal. {infoText}"
: targetUser.AppealDate < _clock.GetCurrentInstant()
? $"{targetUser.Name} is eligible to appeal now. {infoText}"
: $"{targetUser.Name} will be eligible to appeal on {targetUser.AppealDate}. {infoText}"
};
Comment on lines +217 to +223
Copy link
Member

Choose a reason for hiding this comment

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

not sure if dotnet format would like it, but making the ternary nesting a bit more visible with indentation might help a bit:

Suggested change
return new CommandResult { Response =
targetUser.AppealDate is null
? $"{targetUser.Name} is not eligible to appeal. {infoText}"
: targetUser.AppealDate < _clock.GetCurrentInstant()
? $"{targetUser.Name} is eligible to appeal now. {infoText}"
: $"{targetUser.Name} will be eligible to appeal on {targetUser.AppealDate}. {infoText}"
};
return new CommandResult { Response =
targetUser.AppealDate is null
? $"{targetUser.Name} is not eligible to appeal. {infoText}"
: targetUser.AppealDate < _clock.GetCurrentInstant()
? $"{targetUser.Name} is eligible to appeal now. {infoText}"
: $"{targetUser.Name} will be eligible to appeal on {targetUser.AppealDate}. {infoText}"
};

Or, given the infoText gets appended anyway, maybe:

Suggested change
return new CommandResult { Response =
targetUser.AppealDate is null
? $"{targetUser.Name} is not eligible to appeal. {infoText}"
: targetUser.AppealDate < _clock.GetCurrentInstant()
? $"{targetUser.Name} is eligible to appeal now. {infoText}"
: $"{targetUser.Name} will be eligible to appeal on {targetUser.AppealDate}. {infoText}"
};
return new CommandResult { Response = (
targetUser.AppealDate is null
? $"{targetUser.Name} is not eligible to appeal."
: targetUser.AppealDate < _clock.GetCurrentInstant()
? $"{targetUser.Name} is eligible to appeal now."
: $"{targetUser.Name} will be eligible to appeal on {targetUser.AppealDate}."
) + " " + infoText
};

}
}
43 changes: 42 additions & 1 deletion TPP.Core/Moderation/ModerationService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace TPP.Core.Moderation;

public enum TimeoutResult { Ok, MustBe2WeeksOrLess, UserIsBanned, UserIsModOrOp, NotSupportedInChannel }
public enum BanResult { Ok, UserIsModOrOp, NotSupportedInChannel }
public enum SetAppealCooldownResult { Ok, UserNotBanned, AlreadyPermanent }
public enum ModerationActionType { Ban, Unban, Timeout, Untimeout }
public class ModerationActionPerformedEventArgs : EventArgs
{
Expand All @@ -28,17 +29,19 @@ public class ModerationService
private readonly IExecutor? _executor;
private readonly ITimeoutLogRepo _timeoutLogRepo;
private readonly IBanLogRepo _banLogRepo;
private readonly IAppealCooldownLogRepo _appealCooldownLogRepo;
private readonly IUserRepo _userRepo;

public event EventHandler<ModerationActionPerformedEventArgs>? ModerationActionPerformed;

public ModerationService(
IClock clock, IExecutor? executor, ITimeoutLogRepo timeoutLogRepo, IBanLogRepo banLogRepo, IUserRepo userRepo)
IClock clock, IExecutor? executor, ITimeoutLogRepo timeoutLogRepo, IBanLogRepo banLogRepo, IAppealCooldownLogRepo appealCooldownLogRepo, IUserRepo userRepo)
{
_clock = clock;
_executor = executor;
_timeoutLogRepo = timeoutLogRepo;
_banLogRepo = banLogRepo;
_appealCooldownLogRepo = appealCooldownLogRepo;
_userRepo = userRepo;
}

Expand Down Expand Up @@ -70,6 +73,14 @@ await _timeoutLogRepo.LogTimeout( // bans/unbans automatically lift timeouts
issuerUser.Id, now, null);
await _userRepo.SetBanned(targetUser, isBan);

// First ban can be appealed after 1 month by default.
// I don't want to automatically calculate how many bans the user has had, because some might be joke/mistakes
// A mod should manually set the next appeal cooldown based on the rules.
var DEFAULT_APPEAL_TIME = Duration.FromDays(30);
Instant expiration = now + DEFAULT_APPEAL_TIME;
await _userRepo.SetAppealCooldown(targetUser, expiration);
await _appealCooldownLogRepo.LogAppealCooldownChange(targetUser.Id, "auto_appeal_cooldown", issuerUser.Id, now, DEFAULT_APPEAL_TIME);
Comment on lines +81 to +82
Copy link
Member

Choose a reason for hiding this comment

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

to make it impossible to change the appeal cooldown without logging, would it be better to put both into one call somehow? Not sure how example though


Comment on lines +76 to +83
Copy link
Member

Choose a reason for hiding this comment

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

should this entire section only run if isBan?

ModerationActionPerformed?.Invoke(this, new ModerationActionPerformedEventArgs(
issuerUser, targetUser, isBan ? ModerationActionType.Ban : ModerationActionType.Unban));

Expand Down Expand Up @@ -112,4 +123,34 @@ await _timeoutLogRepo.LogTimeout(

return TimeoutResult.Ok;
}

public Task<SetAppealCooldownResult> SetAppealCooldown(User issueruser, User targetuser, Duration duration) =>
_SetAppealCooldown(issueruser, targetuser, duration);
public Task<SetAppealCooldownResult> SetUnappealable(User issueruser, User targetuser) =>
_SetAppealCooldown(issueruser, targetuser, null);

private async Task<SetAppealCooldownResult> _SetAppealCooldown(
User issueruser, User targetUser, Duration? duration)
{
if (!targetUser.Banned)
return SetAppealCooldownResult.UserNotBanned;

Instant now = _clock.GetCurrentInstant();

if (duration.HasValue)
{
Instant expiration = now + duration.Value;
await _userRepo.SetAppealCooldown(targetUser, expiration);
await _appealCooldownLogRepo.LogAppealCooldownChange(targetUser.Id, "manual_cooldown_change", issueruser.Id, now, duration);
}
else
{
if (targetUser.AppealDate is null)
return SetAppealCooldownResult.AlreadyPermanent;
await _userRepo.SetAppealCooldown(targetUser, null);
await _appealCooldownLogRepo.LogAppealCooldownChange(targetUser.Id, "manual_perma", issueruser.Id, now, null);
}

return SetAppealCooldownResult.Ok;
}
}
7 changes: 5 additions & 2 deletions TPP.Core/Setups.cs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ public static CommandProcessor SetUpCommandProcessor(
databases.CommandLogger, argsParser);

var moderationService = new ModerationService(
SystemClock.Instance, executor, databases.TimeoutLogRepo, databases.BanLogRepo, databases.UserRepo);
SystemClock.Instance, executor, databases.TimeoutLogRepo, databases.BanLogRepo,
databases.AppealCooldownLogRepo, databases.UserRepo);
ILogger<ModerationService> logger = loggerFactory.CreateLogger<ModerationService>();
moderationService.ModerationActionPerformed += (_, args) => TaskToVoidSafely(logger, () =>
{
Expand All @@ -139,7 +140,7 @@ public static CommandProcessor SetUpCommandProcessor(
chatModeChanger, databases.LinkedAccountRepo, databases.ResponseCommandRepo
).Commands,
new ModerationCommands(
moderationService, databases.BanLogRepo, databases.TimeoutLogRepo, databases.UserRepo,
moderationService, databases.BanLogRepo, databases.TimeoutLogRepo, databases.AppealCooldownLogRepo, databases.UserRepo,
SystemClock.Instance
).Commands
}.SelectMany(cmds => cmds).ToList();
Expand Down Expand Up @@ -189,6 +190,7 @@ public record Databases(
IModbotLogRepo ModbotLogRepo,
IBanLogRepo BanLogRepo,
ITimeoutLogRepo TimeoutLogRepo,
IAppealCooldownLogRepo AppealCooldownLogRepo,
IResponseCommandRepo ResponseCommandRepo,
IRunCounterRepo RunCounterRepo,
IInputLogRepo InputLogRepo,
Expand Down Expand Up @@ -252,6 +254,7 @@ public static Databases SetUpRepositories(ILoggerFactory loggerFactory, ILogger
ModbotLogRepo: new ModbotLogRepo(mongoDatabase),
BanLogRepo: new BanLogRepo(mongoDatabase),
TimeoutLogRepo: new TimeoutLogRepo(mongoDatabase),
AppealCooldownLogRepo: new AppealCooldownLogRepo(mongoDatabase),
ResponseCommandRepo: new ResponseCommandRepo(mongoDatabase),
RunCounterRepo: new RunCounterRepo(mongoDatabase),
InputLogRepo: new InputLogRepo(mongoDatabase),
Expand Down
2 changes: 2 additions & 0 deletions TPP.Model/Logs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ public record BanLog(
public record TimeoutLog(
string Id, string Type, string UserId, string Reason, string? IssuerUserId, Instant Timestamp,
Duration? Duration);
public record AppealCooldownLog(
string Id, string Type, string UserId, string IssuerUserId, Instant Timestamp, Duration? Duration);

public record SubscriptionLog(
string Id,
Expand Down
4 changes: 4 additions & 0 deletions TPP.Model/User.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ public class User : PropertyEquatable<User>

public Instant? TimeoutExpiration { get; init; }
public bool Banned { get; init; }
/// <summary>
/// When a user can appeal. If null, the user is never allowed to appeal.
/// </summary>
public Instant? AppealDate { get; init; }
Comment on lines +79 to +82
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a clarification how this value should get interpreted in the case the user isn't banned? Maybe it can just be any value, but that arbitrary value should bear no meaning in that case?


public User(
string id,
Expand Down
53 changes: 53 additions & 0 deletions TPP.Persistence.MongoDB/Repos/ModerationRelatedRepos.cs
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,56 @@ public async Task<TimeoutLog> LogTimeout(
.SortByDescending(log => log.Timestamp)
.FirstOrDefaultAsync();
}

public class AppealCooldownLogRepo : IAppealCooldownLogRepo
{
private const string CollectionName = "appealcooldownlog";

public readonly IMongoCollection<AppealCooldownLog> Collection;

static AppealCooldownLogRepo()
{
BsonClassMap.RegisterClassMap<AppealCooldownLog>(cm =>
{
cm.MapIdProperty(b => b.Id)
.SetIdGenerator(StringObjectIdGenerator.Instance)
.SetSerializer(ObjectIdAsStringSerializer.Instance);
cm.MapProperty(b => b.Type).SetElementName("type");
cm.MapProperty(b => b.UserId).SetElementName("user");
cm.MapProperty(b => b.IssuerUserId).SetElementName("issuer");
cm.MapProperty(b => b.Timestamp).SetElementName("timestamp");
cm.MapProperty(b => b.Duration).SetElementName("duration")
.SetSerializer(NullableDurationAsSecondsSerializer.Instance);
});
}

public AppealCooldownLogRepo(IMongoDatabase database)
{
database.CreateCollectionIfNotExists(CollectionName).Wait();
Collection = database.GetCollection<AppealCooldownLog>(CollectionName);
InitIndexes();
}

private void InitIndexes()
{
Collection.Indexes.CreateMany(new[]
{
new CreateIndexModel<AppealCooldownLog>(Builders<AppealCooldownLog>.IndexKeys.Ascending(u => u.UserId)),
new CreateIndexModel<AppealCooldownLog>(Builders<AppealCooldownLog>.IndexKeys.Descending(u => u.Timestamp)),
});
}

public async Task<AppealCooldownLog> LogAppealCooldownChange(
string userId, string type, string issuerUserId, Instant timestamp, Duration? duration)
{
var log = new AppealCooldownLog(string.Empty, type, userId, issuerUserId, timestamp, duration);
await Collection.InsertOneAsync(log);
Debug.Assert(log.Id.Length > 0, "The MongoDB driver injected a generated ID");
return log;
}

public async Task<AppealCooldownLog?> FindMostRecent(string userId) => await Collection
.Find(log => log.UserId == userId)
.SortByDescending(log => log.Timestamp)
.FirstOrDefaultAsync();
}
7 changes: 7 additions & 0 deletions TPP.Persistence.MongoDB/Repos/UserRepo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ static UserRepo()
cm.MapProperty(u => u.TimeoutExpiration).SetElementName("timeout_expiration");
cm.MapProperty(u => u.Roles).SetElementName("roles")
.SetDefaultValue(new HashSet<Role>());
cm.MapProperty(u => u.AppealDate).SetElementName("appeal_date").SetDefaultValue(Instant.MinValue);
});
}

Expand Down Expand Up @@ -274,5 +275,11 @@ await Collection.FindOneAndUpdateAsync<User>(
.Set(u => u.Banned, false)
.Set(u => u.TimeoutExpiration, timeoutExpiration),
new FindOneAndUpdateOptions<User> { ReturnDocument = ReturnDocument.After });

public async Task<User> SetAppealCooldown(User user, Instant? canAppeal) =>
await Collection.FindOneAndUpdateAsync<User>(
u => u.Id == user.Id,
Builders<User>.Update.Set(u => u.AppealDate, canAppeal),
new FindOneAndUpdateOptions<User> { ReturnDocument = ReturnDocument.After });
}
}
4 changes: 4 additions & 0 deletions TPP.Persistence/IUserRepo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ public Task<User> SetSubscriptionInfo(

public Task<User> SetBanned(User user, bool banned);
public Task<User> SetTimedOut(User user, Instant? timeoutExpiration);
/// <summary>
/// Sets the time a user can appeal a ban. Null = can't appeal.
/// </summary>
public Task<User> SetAppealCooldown(User user, Instant? canAppeal);

/// Unselects the specified species as the presented badge if it is the currently equipped species.
/// Used for resetting the equipped badge after a user lost all of that species' badges.
Expand Down
6 changes: 6 additions & 0 deletions TPP.Persistence/ModerationRelatedRepos.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,10 @@ Task<TimeoutLog> LogTimeout(
string userId, string type, string reason, string? issuerUserId, Instant timestamp, Duration? duration);
Task<TimeoutLog?> FindMostRecent(string userId);
}
public interface IAppealCooldownLogRepo
{
Task<AppealCooldownLog> LogAppealCooldownChange(
string userId, string type, string issuerUserId, Instant timestamp, Duration? duration);
Task<AppealCooldownLog?> FindMostRecent(string userId);
}
}
Loading