Adds "this-is-scam" message context command#1505
Conversation
|
| @SuppressWarnings("squid:S3457") // %n is wrong, markdown must use \n | ||
| private static String createDescription(Message target) { |
There was a problem hiding this comment.
Couldn't you easily use multiline strings here to make it cleaner and get rid of the suppress warnings?
| private boolean handleCanQuarantineAndBan(Guild guild, Member target, | ||
| ButtonInteractionEvent event) { | ||
| Member bot = guild.getSelfMember(); | ||
| Member moderator = Objects.requireNonNull(event.getMember()); | ||
| Role quarantinedRole = ModerationUtils.getQuarantinedRole(guild, config).orElse(null); | ||
|
|
||
| return ModerationUtils.handleRoleChangeChecks(quarantinedRole, "quarantine", target, bot, | ||
| moderator, guild, ACTION_REASON, event) | ||
| && ModerationUtils.handleHasBotPermissions("ban", Permission.BAN_MEMBERS, bot, | ||
| guild, event); | ||
| } | ||
|
|
||
| private RestAction<Boolean> sendQuarantineDm(User target, Guild guild) { | ||
| String description = | ||
| """ | ||
| Hey there, sorry to tell you but unfortunately you have been put under quarantine. | ||
| This means you can no longer interact with anyone in the server until you have been unquarantined again."""; | ||
|
|
||
| return ModerationUtils.sendModActionDm(ModerationUtils.getModActionEmbed(guild, | ||
| ACTION_TITLE, description, ACTION_REASON, true), target); | ||
| } | ||
|
|
||
| private RestAction<Void> quarantineUser(Guild guild, Member target, Member moderator) { | ||
| logger.info(LogMarkers.SENSITIVE, | ||
| "'{}' ({}) quarantined the user '{}' ({}) in guild '{}' for reason '{}'.", | ||
| moderator.getUser().getName(), moderator.getId(), target.getUser().getName(), | ||
| target.getId(), guild.getName(), ACTION_REASON); | ||
|
|
||
| actionsStore.addAction(guild.getIdLong(), moderator.getIdLong(), target.getIdLong(), | ||
| ModerationAction.QUARANTINE, null, ACTION_REASON); | ||
|
|
||
| return guild | ||
| .addRoleToMember(target, | ||
| ModerationUtils.getQuarantinedRole(guild, config).orElseThrow()) | ||
| .reason(ACTION_REASON); | ||
| } | ||
|
|
||
| private RestAction<Void> deleteMessagesByBanAndUnban(Guild guild, User target) { | ||
| return guild.ban(target, 1, TimeUnit.DAYS) | ||
| .reason(ACTION_REASON) | ||
| .flatMap(_ -> guild.unban(target).reason(ACTION_REASON)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can't all of this be reused from the existing quarantine/moderation logic?
There was a problem hiding this comment.
not easily. but yeah, some of it can probably be moved around.
the main issue is that we only want part of the behavior but not everything.
so moving the code around could possibly result in making it too complex to understand.
but its a good comment, ill have a second look :)
firasrg
left a comment
There was a problem hiding this comment.
Thank you @Zabuzard for the great work you've put into this. I'm genuinely impressed by the scope of the changes and the amount of effort behind them.
I've left comments, mostly focused on long-term maintainability and readability. I didn't spend much time reviewing the logic itself, as I trust you've given that aspect the attention it deserves.
| * Allows users to report a message as potential scam. Moderators can confirm the report from the | ||
| * audit log, causing the author to be quarantined plus message history getting deleted. | ||
| */ | ||
| public final class ThisIsScamCommand extends BotCommandAdapter implements MessageContextCommand { |
There was a problem hiding this comment.
I'd use a name like ScamReportCommand for the class, and for the command scam-report
There was a problem hiding this comment.
imo all that does is making it more confusing for the user and the devs. a context command needs to be a verb, especially bc they dont have any description in the discord ui
There was a problem hiding this comment.
Both work fine, agree it's NIT
|
|
||
| private static final String COMMAND_NAME = "this-is-scam"; | ||
|
|
||
| private static final String ACTION_TITLE = "Quarantine"; |
There was a problem hiding this comment.
Since this constant is used once at line 277, I think it's better to put its value directly there where without declaring this
There was a problem hiding this comment.
kinda NIT. ill keep it as is bc this is the style also used in the other mod commands
There was a problem hiding this comment.
This is fine as-is (good how it's done).
The ACTION_TITLE gives the string some meaning otherwise you wouldn't clearly know what the text "Quarantine" is being used for in the method it's used:
return ModerationUtils.sendModActionDm(ModerationUtils.getModActionEmbed(guild, ACTION_TITLE, description, ACTION_REASON, true), target);
vs.
return ModerationUtils.sendModActionDm(ModerationUtils.getModActionEmbed(guild, "Quarantine", description, ACTION_REASON, true), target);
same with the other params, if Java had named parameters or the function was using a builder pattern, it would have been fine to inline however since we're not, it's best to keep the context clear.
| private static final String FAILED_MESSAGE = | ||
| "Sorry, there was an issue forwarding your scam report to the moderators. We are investigating."; | ||
| private static final Duration USER_COMMAND_COOLDOWN = Duration.ofMinutes(1); | ||
| private static final Color AMBIENT_COLOR = Color.decode("#CFBFF5"); |
There was a problem hiding this comment.
- Same as above, about removing the constant, since the value is used only in line 171.
- I'd make an enum for colors (e,g:
AppColors), rather than declaring colors literally like it's happening here and in other classes (e.gCodeMessageHandler), or simply use thejava.awt.Color, like you did already at line 212
There was a problem hiding this comment.
those are the constants that someone might want to tweak, so better to have them here.
not sure what u mean referring the color, that is an awt color already and this is the color used for mod commands
|
|
||
| private final Cache<Long, Instant> reportedMessageToTimestamp = | ||
| Caffeine.newBuilder().maximumSize(10_000).expireAfterWrite(Duration.ofDays(1)).build(); | ||
| private final Cache<Long, Instant> userToLastCommandUse = Caffeine.newBuilder() |
There was a problem hiding this comment.
This other constant has similar builder chain as reportedMessageToTimestamp , why not making a helper function to avoid redundency:
private static Cache<Long, Instant> createTempCache(){
// you can make the values 10_000 and duration of days = 1 as constants at the top so anybody can see that when they reach this class
return Caffeine.newBuilder().maximumSize(10_000).expireAfterWrite(Duration.ofDays(1)).build();
}There was a problem hiding this comment.
makes it needlessly complicated bc:
- they are not related so they also shouldn't use the same construction mechanism. they might differ in the future on purpose
- that makes u jump up and down through the code just to understand the details of the cache, interrupts code flow reading. not an improvement
There was a problem hiding this comment.
// you can make the values 10_000 and duration of days = 1 as constants at the top so anybody can see that when they reach this class
I also don't get how you argue about removing constants because they are used once and now say he should do it here
| .build(); | ||
|
|
||
| private final Config config; | ||
| private final ModerationActionsStore actionsStore; |
There was a problem hiding this comment.
I'd put these 2 fields config and actionsStore above reportedMessageToTimestamp and userToLastCommandUse, because they represent a higher level
|
|
||
| return auditChannel.sendMessageEmbeds(reportEmbed) | ||
| .addActionRow(Button.success(generateComponentId(args), "Yes"), | ||
| Button.danger(generateComponentId(args), "No")); |
There was a problem hiding this comment.
No and Yes can be defined as Enum values in something cool like the code below, im seeing them duplicates across multiple classes, maybe this is the opportunity
enum YesNo {
YES("Y", "Yes" true),
NO("N", "No", false);
private final String code;
private final String label;
private final boolean boolValue;
// getters and some utility methods ..
}There was a problem hiding this comment.
don't put together unrelated stuff. the yes/no here has nothing to do with yes/no choices elsewhere. putting such stuff in a common shared place will make stuff worse bc then u have to rip things apart again if one of them had different needs suddenly than the other. only put together related stuff
| MessageEmbed resultEmbed = new EmbedBuilder() | ||
| .setDescription( | ||
| isScam ? "This is scam. The user was quarantined and messages were deleted." | ||
| : "This is not scam, no action executed.") |
There was a problem hiding this comment.
I'd rather say "This is NOT Scam. The user has NOT been quarantined and NO messages have been deleted."
There was a problem hiding this comment.
Would this not be the same as what he has now? Feel that would be unnecessarily verbose to add that.
There was a problem hiding this comment.
if anything ud use markdown highlighting by making it bold.
but the point of this message is to not draw the readers attention. so highlighting it in any way would miss the goal
|
|
||
| event.editMessageEmbeds(embeds).setComponents().queue(); | ||
|
|
||
| if (!isScam) { |
There was a problem hiding this comment.
add log.debug for better future debugging
There was a problem hiding this comment.
jda covers that already
| long targetId = Long.parseLong(args.get(1)); | ||
|
|
||
| ButtonStyle clickedStyle = event.getButton().getStyle(); | ||
| boolean isScam = clickedStyle == ButtonStyle.SUCCESS; |
There was a problem hiding this comment.
You can simplify do like this:
boolean isScam = event.getButton().getStyle() == ButtonStyle.SUCCESS;There was a problem hiding this comment.
but then we don't have to opportunity anymore to give this otherwise confusing line a good variable name that helps give context
| @SuppressWarnings("squid:S3457") // %n is wrong, markdown must use \n | ||
| private static String createDescription(Message target) { | ||
| String content = target.getContentStripped(); | ||
| String description = content.isBlank() ? "(empty message)" : content; |
There was a problem hiding this comment.
i'd rather say "empty description"
There was a problem hiding this comment.
that would be confusing as "description" is right in the context of the embed but not in the context of the reading user in discord.
the scan message that was reported is empty, so that is what we need to tell the user.
empty message.
i might improve the wording slightly though:
(message had no text content)



Overview
This adds a new message context command (right click message) called
this-is-scam. It acts as the primary way for users to report scam to mods.Mods then get presented a quick way to handle the scam properly by automatically deleting all messages from the user and putting them under quarantine through a single button press!
Internally this works by quarantine, ban (delete messages from one day), unban in that order. The user ends up being kicked out of the guild (due to the ban) but can immediately rejoin (due to the unban), but remains quarantined.
Other
The following extra situations are handled gracefully via in-memory caches:
Also, an appropriate entry in the audit-log of the user (
/audit) is created.Attachments are spelled out so it becomes easier for devs later on to edit
ScamBlockeraccordingly.Code
The integration is minimal and simple, just a straightforward single class is added
ThisIsScamCommand.java, thats it. No config changes, everything easy.Impressions
Context Menu
Response
What mods see
No scam
Yes scam
DM
Quarantined
Audit Log
Edge cases
Error