Skipping audit logs against bot message deletions (#395)

* Skipping audit logs against bot message deletions
* Bugfix with broken RestAction pipeline

Author: Zabuzard

application/src/main/java/org/togetherjava/tjbot/routines/ModAuditLogRoutine.java

@@ -25,8 +25,8 @@
 import java.time.*;
 import java.time.temporal.ChronoUnit;
 import java.time.temporal.TemporalAccessor;
-import java.util.*;
 import java.util.List;
+import java.util.*;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.function.BiFunction;
@@ -70,33 +70,15 @@ public ModAuditLogRoutine(@NotNull Database database, @NotNull Config config) {
         this.database = database;
     }
 
-    private static @NotNull RestAction<MessageEmbed> handleAction(@NotNull Action action,
+    private static @NotNull RestAction<AuditLogMessage> handleAction(@NotNull Action action,
             @NotNull AuditLogEntry entry) {
         User author = Objects.requireNonNull(entry.getUser());
-        return getTargetTagFromEntry(entry).map(targetTag -> createMessage(author, action,
-                targetTag, entry.getReason(), entry.getTimeCreated()));
+        return getTargetFromEntryOrNull(entry).map(target -> new AuditLogMessage(author, action,
+                target, entry.getReason(), entry.getTimeCreated()));
     }
 
-    private static @NotNull MessageEmbed createMessage(@NotNull User author, @NotNull Action action,
-            @NotNull String targetTag, @Nullable String reason,
-            @NotNull TemporalAccessor timestamp) {
-        String description = "%s **%s**.".formatted(action.getVerb(), targetTag);
-        if (reason != null && !reason.isBlank()) {
-            description += "\n\nReason: " + reason;
-        }
-        return new EmbedBuilder().setAuthor(author.getAsTag(), null, author.getAvatarUrl())
-            .setDescription(description)
-            .setTimestamp(timestamp)
-            .setColor(AMBIENT_COLOR)
-            .build();
-    }
-
-    private static RestAction<String> getTargetTagFromEntry(@NotNull AuditLogEntry entry) {
-        // If the target is null, the user got deleted in the meantime
-        return entry.getJDA()
-            .retrieveUserById(entry.getTargetIdLong())
-            .onErrorMap(error -> null)
-            .map(target -> target == null ? "(user unknown)" : target.getAsTag());
+    private static RestAction<User> getTargetFromEntryOrNull(@NotNull AuditLogEntry entry) {
+        return entry.getJDA().retrieveUserById(entry.getTargetIdLong()).onErrorMap(error -> null);
     }
 
     private static boolean isSnowflakeAfter(@NotNull ISnowflake snowflake,
@@ -178,34 +160,40 @@ private static boolean isSnowflakeAfter(@NotNull ISnowflake snowflake,
             @NotNull AuditLogEntry entry) {
         // NOTE Temporary bans are realized as permanent bans with automated unban,
         // hence we can not differentiate a permanent or a temporary ban here
-        return Optional.of(handleAction(Action.BAN, entry));
+        return Optional.of(handleAction(Action.BAN, entry).map(AuditLogMessage::toEmbed));
     }
 
     private static @NotNull Optional<RestAction<MessageEmbed>> handleUnbanEntry(
             @NotNull AuditLogEntry entry) {
-        return Optional.of(handleAction(Action.UNBAN, entry));
+        return Optional.of(handleAction(Action.UNBAN, entry).map(AuditLogMessage::toEmbed));
     }
 
     private static @NotNull Optional<RestAction<MessageEmbed>> handleKickEntry(
             @NotNull AuditLogEntry entry) {
-        return Optional.of(handleAction(Action.KICK, entry));
+        return Optional.of(handleAction(Action.KICK, entry).map(AuditLogMessage::toEmbed));
     }
 
     private static @NotNull Optional<RestAction<MessageEmbed>> handleMuteEntry(
             @NotNull AuditLogEntry entry) {
         // NOTE Temporary mutes are realized as permanent mutes with automated unmute,
         // hence we can not differentiate a permanent or a temporary mute here
-        return Optional.of(handleAction(Action.MUTE, entry));
+        return Optional.of(handleAction(Action.MUTE, entry).map(AuditLogMessage::toEmbed));
     }
 
     private static @NotNull Optional<RestAction<MessageEmbed>> handleUnmuteEntry(
             @NotNull AuditLogEntry entry) {
-        return Optional.of(handleAction(Action.UNMUTE, entry));
+        return Optional.of(handleAction(Action.UNMUTE, entry).map(AuditLogMessage::toEmbed));
     }
 
     private static @NotNull Optional<RestAction<MessageEmbed>> handleMessageDeleteEntry(
             @NotNull AuditLogEntry entry) {
-        return Optional.of(handleAction(Action.MESSAGE_DELETION, entry));
+        return Optional.of(handleAction(Action.MESSAGE_DELETION, entry).map(message -> {
+            if (message.target() != null && message.target().isBot()) {
+                // Message deletions against bots should be skipped. Cancel action.
+                return null;
+            }
+            return message.toEmbed();
+        }));
     }
 
     @Override
@@ -274,8 +262,7 @@ private void handleAuditLogs(@NotNull MessageChannel auditLogChannel,
             .sorted(Comparator.comparing(TimeUtil::getTimeCreated))
             .map(entry -> handleAuditLog(auditLogChannel, entry))
             .flatMap(Optional::stream)
-            .reduce((firstMessage, secondMessage) -> firstMessage.flatMap(result -> secondMessage))
-            .ifPresent(RestAction::queue);
+            .forEach(RestAction::queue);
 
         database.write(context -> {
             var entry = context.newRecord(ModAuditLogGuildProcess.MOD_AUDIT_LOG_GUILD_PROCESS);
@@ -298,7 +285,13 @@ private Optional<RestAction<Message>> handleAuditLog(@NotNull MessageChannel aud
             case MESSAGE_DELETE -> handleMessageDeleteEntry(entry);
             default -> Optional.empty();
         };
-        return maybeMessage.map(message -> message.flatMap(auditLogChannel::sendMessageEmbeds));
+        // It can have 3 states:
+        // * empty optional - entry is irrelevant and should not be logged
+        // * has RestAction but that will contain null - entry was relevant at first, but at
+        // query-time we found out that it is irrelevant
+        // * has RestAction but will contain a message - entry is relevant, log the message
+        return maybeMessage
+            .map(message -> message.flatMap(Objects::nonNull, auditLogChannel::sendMessageEmbeds));
     }
 
     private @NotNull Optional<RestAction<MessageEmbed>> handleRoleUpdateEntry(
@@ -359,4 +352,23 @@ String getVerb() {
             return verb;
         }
     }
+
+    private record AuditLogMessage(@NotNull User author, @NotNull Action action,
+            @Nullable User target, @Nullable String reason, @NotNull TemporalAccessor timestamp) {
+        @NotNull
+        MessageEmbed toEmbed() {
+            String targetTag = target == null ? "(user unknown)" : target.getAsTag();
+            String description = "%s **%s**.".formatted(action.getVerb(), targetTag);
+
+            if (reason != null && !reason.isBlank()) {
+                description += "\n\nReason: " + reason;
+            }
+
+            return new EmbedBuilder().setAuthor(author.getAsTag(), null, author.getAvatarUrl())
+                .setDescription(description)
+                .setTimestamp(timestamp)
+                .setColor(AMBIENT_COLOR)
+                .build();
+        }
+    }
 }