-
-
Notifications
You must be signed in to change notification settings - Fork 107
fix: allow forwarded messages with media in media-only channels #1419
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: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| import org.togetherjava.tjbot.features.MessageReceiverAdapter; | ||
|
|
||
| import java.awt.Color; | ||
| import java.util.List; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.regex.Pattern; | ||
|
|
||
|
|
@@ -51,9 +52,39 @@ public void onMessageReceived(MessageReceivedEvent event) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether the given message has no media attached. | ||
| * <p> | ||
| * A message is considered to have media if it contains attachments, embeds, or a URL in its | ||
| * text content. For forwarded messages, the snapshots are also checked for media. | ||
| * | ||
| * @param message the message to check | ||
| * @return {@code true} if the message has no media, {@code false} otherwise | ||
| */ | ||
| private boolean messageHasNoMediaAttached(Message message) { | ||
| return message.getAttachments().isEmpty() && message.getEmbeds().isEmpty() | ||
| && !message.getContentRaw().contains("http"); | ||
| if (hasMedia(message.getAttachments(), message.getEmbeds(), message.getContentRaw())) { | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would be nice to add a log.debug here, just before the return
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looked at logging patterns in the codebase — actions are logged where decisions are made, not inside helper methods. For example, AutoPruneHelperRoutine logs in pruneRoleIfFull() rather than isRoleFull(). Following the same principle, I'd place the debug log in onMessageReceived() rather than messageHasNoMediaAttached(), since the latter is just a predicate. Does that make sense?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, thanks
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, including some meaningful logs within methods/functions, whatever they may be, can be helpful, especially in parts where errors/bugs are likely to occur. It enhances testing and encapsulation
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, but not in the helper method used as "tester". it needs to be put into the actual logic-method. personally, i dont think a log is really useful in this particular case because in which case would anyone consume that log? if we suspect a bug in this logic we would just run it locally where we have a debugger anyways. this isnt really something that would be debugged on the life-system. but debug logs dont hurt, so whatever |
||
| } | ||
|
|
||
| return message.getMessageSnapshots() | ||
| .stream() | ||
| .noneMatch(snapshot -> hasMedia(snapshot.getAttachments(), snapshot.getEmbeds(), | ||
| snapshot.getContentRaw())); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you make
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. u cant because the method is shared across the two types |
||
| } | ||
|
|
||
| /** | ||
| * Checks whether the given content contains any media. | ||
| * <p> | ||
| * Media is considered present if there are attachments, embeds, or a URL (identified by | ||
| * {@code "http"}) in the text content. | ||
| * | ||
| * @param attachments the attachments of the message or snapshot | ||
| * @param embeds the embeds of the message or snapshot | ||
| * @param content the raw text content of the message or snapshot | ||
| */ | ||
| private boolean hasMedia(List<Message.Attachment> attachments, List<MessageEmbed> embeds, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
hasMedia(message.getAttachments(), message.getEmbeds(), message.getContentRaw())why not simply put one single argument
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (javadoc isnt needed but its also not bad to have)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't matter if a method is private, we still have to maintain it and documentation is always going to help.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| String content) { | ||
| return !attachments.isEmpty() || !embeds.isEmpty() || content.contains("http"); | ||
| } | ||
|
|
||
| private MessageCreateData createNotificationMessage(Message message) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.