Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

makes sense, thanks

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member

@Zabuzard Zabuzard Feb 25, 2026

Choose a reason for hiding this comment

The 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()));
Copy link
Contributor

@firasrg firasrg Feb 24, 2026

Choose a reason for hiding this comment

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

if you make hasMedia() has one argument rather than 3, then you can simplify this part to noneMatch(this::hasMedia)

Copy link
Member

Choose a reason for hiding this comment

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

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.

}

/**
* 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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) ?

Copy link
Member

Choose a reason for hiding this comment

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

(javadoc isnt needed but its also not bad to have)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

String content) {
return !attachments.isEmpty() || !embeds.isEmpty() || content.contains("http");
}

private MessageCreateData createNotificationMessage(Message message) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import net.dv8tion.jda.api.entities.Message;
import net.dv8tion.jda.api.entities.MessageEmbed;
import net.dv8tion.jda.api.entities.channel.ChannelType;
import net.dv8tion.jda.api.entities.messages.MessageSnapshot;
import net.dv8tion.jda.api.events.message.MessageReceivedEvent;
import net.dv8tion.jda.api.utils.messages.MessageCreateBuilder;
import net.dv8tion.jda.api.utils.messages.MessageCreateData;
Expand Down Expand Up @@ -112,4 +113,48 @@ private MessageReceivedEvent sendMessage(MessageCreateData message,
mediaOnlyChannelListener.onMessageReceived(event);
return event;
}


@Test
void keepsForwardedMessageWithAttachment() {
// GIVEN a forwarded message that contains an attachment inside the snapshot
MessageCreateData message = new MessageCreateBuilder().setContent("any").build();

MessageSnapshot snapshot = mock(MessageSnapshot.class);
when(snapshot.getAttachments()).thenReturn(List.of(mock(Message.Attachment.class)));
when(snapshot.getEmbeds()).thenReturn(List.of());
when(snapshot.getContentRaw()).thenReturn("");

// WHEN sending the forwarded message
MessageReceivedEvent event = sendMessageWithSnapshots(message, List.of(snapshot));

// THEN it does not get deleted
verify(event.getMessage(), never()).delete();
}

@Test
void deletesForwardedMessageWithoutMedia() {
// GIVEN a forwarded message that contains no media inside the snapshot
MessageCreateData message = new MessageCreateBuilder().setContent("any").build();

MessageSnapshot snapshot = mock(MessageSnapshot.class);
when(snapshot.getAttachments()).thenReturn(List.of());
when(snapshot.getEmbeds()).thenReturn(List.of());
when(snapshot.getContentRaw()).thenReturn("just some text, no media");

// WHEN sending the forwarded message
MessageReceivedEvent event = sendMessageWithSnapshots(message, List.of(snapshot));

// THEN it gets deleted
verify(event.getMessage()).delete();
}

private MessageReceivedEvent sendMessageWithSnapshots(MessageCreateData message,
List<MessageSnapshot> snapshots) {
MessageReceivedEvent event =
jdaTester.createMessageReceiveEvent(message, List.of(), ChannelType.TEXT);
when(event.getMessage().getMessageSnapshots()).thenReturn(snapshots);
mediaOnlyChannelListener.onMessageReceived(event);
return event;
}
}
Loading