-
Notifications
You must be signed in to change notification settings - Fork 24
delete blacklisted attachments from message and replace with webhook #520
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
base: main
Are you sure you want to change the base?
delete blacklisted attachments from message and replace with webhook #520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the changes I wrote in the review comments.
Also, please use a proper/professional commit message. You can change the commit message of your existing commit using git commit --amend
(optionally with -m <message>
) and then force-pushing to your branch with git push --force-with-lease
.
src/main/java/net/discordjug/javabot/data/config/GuildConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/net/discordjug/javabot/listener/BlacklistedMessageAttachmentListener.java
Outdated
Show resolved
Hide resolved
src/main/java/net/discordjug/javabot/listener/BlacklistedMessageAttachmentListener.java
Outdated
Show resolved
Hide resolved
src/main/java/net/discordjug/javabot/listener/BlacklistedMessageAttachmentListener.java
Outdated
Show resolved
Hide resolved
src/main/java/net/discordjug/javabot/listener/BlacklistedMessageAttachmentListener.java
Outdated
Show resolved
Hide resolved
Please also fix the checkstyle errors. We use tabs for indentation. |
src/main/java/net/discordjug/javabot/listener/BlacklistedMessageAttachmentListener.java
Outdated
Show resolved
Hide resolved
699283b
to
a2b3d33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the comments.
Other things to eventually convert to a filter:
- AutoMod (please do this in this PR)
- suggestions (can be another PR)
- code formatting in help channel (can be in another PR)
It might als be a good idea to add an overload to WebhookUtil.replaceMessage
without embeds/attachments to make the code simpler instead of having to add a null
argument wherever calling it.
@@ -18,7 +18,10 @@ | |||
import java.lang.reflect.Field; | |||
import java.nio.file.Files; | |||
import java.nio.file.Path; | |||
import java.util.HashSet; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashSet
and List
shouldn't be needed (I think the CI would complain about that)
import java.util.List; | ||
|
||
public interface MessageFormatFilter { | ||
boolean processMessage(MessageReceivedEvent event, StringBuilder builder, List<Message.Attachment> attachments, List<MessageEmbed> embeds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create one data class (probably a record is the best choice) to store the content StringBuilder
, the current attachments and the embeds and pass that.
That makes it easier to add additional contents that should be changed without modifying every filter.
import java.util.List; | ||
|
||
public interface MessageFormatFilter { | ||
boolean processMessage(MessageReceivedEvent event, StringBuilder builder, List<Message.Attachment> attachments, List<MessageEmbed> embeds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A boolean
return type will probably not be sufficient as it will probably be necessary to indicate the message being removed as well for things like converting automod to a filter.
An enum or record would likely be a better choice.
@Override | ||
public boolean processMessage(MessageReceivedEvent event, StringBuilder builder, List<Message.Attachment> attachments, List<MessageEmbed> embeds) { | ||
replaceFucks(builder); | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this always return true
independently of whether the message has been modified?
} | ||
|
||
public static void replaceFucks(StringBuilder sb) { | ||
Matcher matcher = FUCKER.matcher(sb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matcher
doesn't seem to expect the CharSequence
to be modified during the matching (until the find()
s are complete). Please change this to FUCKER.matcher(sb.toString())
(or similar).
example code showing that Matcher
doesn't expect it to be modified:
StringBuilder sb = new StringBuilder("aba");
Matcher matcher = Pattern.compile("a").matcher(sb);
sb.insert(0,"xyz");//xyzaba
if(matcher.find()) { // false
// alternatively move the sb.insert(0,"xyz"); here to see to break even more
System.out.println(matcher.start());
System.out.println(matcher.group());
}
Aside from that, I think the easiest (and most robust) approach would be to keep the existing logic using Matcher#replaceAll
and replace the StringBuilder
content if the returned String
is different (or alternatively use if(matcher.find())
with matcher.replaceAll(...)
inside).
|
||
for (MessageFormatFilter filter : filters) { | ||
boolean processed = filter.processMessage(event, builder, attachments, embeds); | ||
if (processed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, this could be handled |= filter.processMessage()
but since you'll need multiple states for automod (filter execution aborted), that's probably irrelevant.
if (webhookContainer == null) { | ||
return; | ||
} | ||
replaceMessage(event, webhookContainer, builder.toString(), threadId, attachments, embeds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this works correctly with initial forum posts (i.e. the first message in a forum post/thread)?
} | ||
|
||
private static void replaceMessage(@NotNull MessageReceivedEvent event, IWebhookContainer webhookContainer, | ||
String messageContentRaw, long threadId, List<Message.Attachment> attachments, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using a record for these parameters, it would be good to pass the record until here btw.
} | ||
|
||
private static CompletableFuture<ReadonlyMessage> mirrorMessageToWebhook(@NotNull Webhook webhook, Message originalMessage, String newMessageContent, long threadId, | ||
List<LayoutComponent> components, List<MessageEmbed> embeds, Member member) { | ||
List<LayoutComponent> components, List<Message.Attachment> attachments, List<MessageEmbed> embeds, Member member) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a whitespace change here and a bit above. Is there a reason for this?
@@ -118,11 +118,12 @@ private static CompletableFuture<ReadonlyMessage> mirrorMessageToWebhook(@NotNul | |||
message.addEmbeds(embeds.stream() | |||
.map(e -> WebhookEmbedBuilder.fromJDA(e).build()) | |||
.toList()); | |||
List<Attachment> attachments = originalMessage.getAttachments(); | |||
List<Attachment> newAttachments = attachments != null ? attachments : originalMessage.getAttachments(); | |||
System.out.println(newAttachments.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this use of System.out
.
), | ||
e -> ExceptionLogger.capture( | ||
e, | ||
"Error creating webhook for BlacklistedMessageAttachmentListener" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't BlacklistedMessageAttachmentListener
any more ;)
You can use getClass().getSimpleName()
.
new crazy amazing feature where if someone sends a message with a forbidden attachment then that attachment is removed and the whole message is replaced as a webhook with only the good attachments
works server wide
config in guild config