fix: allow forwarded messages with media in media-only channels#1419
fix: allow forwarded messages with media in media-only channels#1419VlaM5 wants to merge 4 commits intoTogether-Java:developfrom
Conversation
|
|
...cation/src/main/java/org/togetherjava/tjbot/features/mediaonly/MediaOnlyChannelListener.java
Outdated
Show resolved
Hide resolved
...cation/src/main/java/org/togetherjava/tjbot/features/mediaonly/MediaOnlyChannelListener.java
Outdated
Show resolved
Hide resolved
...cation/src/main/java/org/togetherjava/tjbot/features/mediaonly/MediaOnlyChannelListener.java
Outdated
Show resolved
Hide resolved
...on/src/test/java/org/togetherjava/tjbot/features/mediaonly/MediaOnlyChannelListenerTest.java
Outdated
Show resolved
Hide resolved
|
nice stuff, thanks. just make the CI/CD happy and we can get it merged soon :) |
|
poggers, all green 👍 |
...cation/src/main/java/org/togetherjava/tjbot/features/mediaonly/MediaOnlyChannelListener.java
Show resolved
Hide resolved
| return message.getAttachments().isEmpty() && message.getEmbeds().isEmpty() | ||
| && !message.getContentRaw().contains("http"); | ||
| if (hasMedia(message.getAttachments(), message.getEmbeds(), message.getContentRaw())) { | ||
| return false; |
There was a problem hiding this comment.
it would be nice to add a log.debug here, just before the return
There was a problem hiding this comment.
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?
| * @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, |
There was a problem hiding this comment.
- There is no need for javadoc here, since the method is private;
- I think this method should be marked
static; - based on the use above of this method
hasMedia(message.getAttachments(), message.getEmbeds(), message.getContentRaw())why not simply put one single argument hasMedia(message) ?
There was a problem hiding this comment.
(javadoc isnt needed but its also not bad to have)
There was a problem hiding this comment.
It doesn't matter if a method is private, we still have to maintain it and documentation is always going to help.
There was a problem hiding this comment.
private static boolean hasMedia makes sense indeed, but lets decide where to place debugging (see discussion above) and I'll address both in a single commit.
| return message.getMessageSnapshots() | ||
| .stream() | ||
| .noneMatch(snapshot -> hasMedia(snapshot.getAttachments(), snapshot.getEmbeds(), | ||
| snapshot.getContentRaw())); |
There was a problem hiding this comment.
if you make hasMedia() has one argument rather than 3, then you can simplify this part to noneMatch(this::hasMedia)
There was a problem hiding this comment.
u cant because the method is shared across the two types Message and MessageSnapshot and bc JDA is JDA they dont share a common interface.
5 participants
Fixes #1243
Forwarded messages that contain media (attachments, embeds, or links)
were incorrectly deleted in media-only channels, because the bot only
checked the message itself, not its MessageSnapshots.
Changes: