Temp action bugfix and code improvement

* database returns multiple results, forgot limit(1)
* logic mistake of using last expired action instead of last temp action (implicitly given by last action that passes the "not permanent" check)
* can use `fetchOptional` with database
* added note regarding startup race condition issue

Author: Zabuzard

application/src/main/java/org/togetherjava/tjbot/commands/moderation/ModerationActionsStore.java

@@ -95,13 +95,15 @@ public ModerationActionsStore(@NotNull Database database) {
     // FIXME javadoc
     public @NotNull Optional<ActionRecord> findLastActionAgainstTargetByType(long guildId,
             long targetId, @NotNull ModerationAction actionType) {
-        return Optional.of(database.read(context -> context
-            .selectFrom(ModerationActions.MODERATION_ACTIONS)
-            .where(ModerationActions.MODERATION_ACTIONS.GUILD_ID.eq(guildId)
-                .and(ModerationActions.MODERATION_ACTIONS.TARGET_ID.eq(targetId)
-                    .and(ModerationActions.MODERATION_ACTIONS.ACTION_TYPE.eq(actionType.name()))))
-            .orderBy(ModerationActions.MODERATION_ACTIONS.ISSUED_AT.desc())
-            .fetchOne())).map(ActionRecord::of);
+        return database
+            .read(context -> context.selectFrom(ModerationActions.MODERATION_ACTIONS)
+                .where(ModerationActions.MODERATION_ACTIONS.GUILD_ID.eq(guildId)
+                    .and(ModerationActions.MODERATION_ACTIONS.TARGET_ID.eq(targetId))
+                    .and(ModerationActions.MODERATION_ACTIONS.ACTION_TYPE.eq(actionType.name())))
+                .orderBy(ModerationActions.MODERATION_ACTIONS.ISSUED_AT.desc())
+                .limit(1)
+                .fetchOptional())
+            .map(ActionRecord::of);
     }
 
     /**
@@ -111,10 +113,10 @@ public ModerationActionsStore(@NotNull Database database) {
      * @return the action with the given case id, if present
      */
     public @NotNull Optional<ActionRecord> findActionByCaseId(int caseId) {
-        return Optional
-            .of(database.read(context -> context.selectFrom(ModerationActions.MODERATION_ACTIONS)
+        return database
+            .read(context -> context.selectFrom(ModerationActions.MODERATION_ACTIONS)
                 .where(ModerationActions.MODERATION_ACTIONS.CASE_ID.eq(caseId))
-                .fetchOne()))
+                .fetchOptional())
             .map(ActionRecord::of);
     }
 

application/src/main/java/org/togetherjava/tjbot/commands/moderation/temp/TemporaryModerationRoutine.java

@@ -55,27 +55,22 @@ private void checkExpiredActions() {
         actionsStore.getExpiredActionsAscending()
             .stream()
             .filter(action -> typeToRevocableAction.containsKey(action.actionType()))
-            .collect(Collectors.groupingBy(RevocationGroupIdentifier::of))
+            .map(RevocationGroupIdentifier::of)
+            .collect(Collectors.toSet())
             .forEach(this::processGroupedActions);
 
         logger.debug("Finished checking expired temporary moderation actions to revoke.");
     }
 
-    private void processGroupedActions(@NotNull RevocationGroupIdentifier groupIdentifier,
-            @NotNull Collection<ActionRecord> expiredActions) {
-        // Last issued temporary action takes priority
-        ActionRecord actionToRevoke = expiredActions.stream()
-            .max(Comparator.comparing(ActionRecord::issuedAt))
-            .orElseThrow();
-
+    private void processGroupedActions(@NotNull RevocationGroupIdentifier groupIdentifier) {
         // Do not revoke an action which was overwritten by a permanent action that was issued
         // afterwards
         // For example if a user was perm-banned after being temp-banned
-        ActionRecord lastAction = actionsStore
+        ActionRecord lastApplyAction = actionsStore
             .findLastActionAgainstTargetByType(groupIdentifier.guildId, groupIdentifier.targetId,
                     groupIdentifier.type)
             .orElseThrow();
-        if (lastAction.actionExpiresAt() == null) {
+        if (lastApplyAction.actionExpiresAt() == null) {
             return;
         }
 
@@ -88,7 +83,7 @@ private void processGroupedActions(@NotNull RevocationGroupIdentifier groupIdent
             .findLastActionAgainstTargetByType(groupIdentifier.guildId, groupIdentifier.targetId,
                     revokeActionType)
             .orElseThrow();
-        if (lastRevokeAction.issuedAt().isAfter(actionToRevoke.issuedAt())
+        if (lastRevokeAction.issuedAt().isAfter(lastApplyAction.issuedAt())
                 && (lastRevokeAction.actionExpiresAt() == null
                         || lastRevokeAction.actionExpiresAt().isAfter(Instant.now()))) {
             return;
@@ -149,7 +144,9 @@ private void handleFailure(@NotNull Throwable failure,
     public void start() {
         // TODO This should be registered at some sort of routine system instead (see GH issue #235
         // which adds support for routines)
-        checkExpiredActionsService.scheduleWithFixedDelay(this::checkExpiredActions, 0, 5,
+        // NOTE The initial run has to be delayed until after the guild cache has been updated
+        // (during CommandSystem startup)
+        checkExpiredActionsService.scheduleWithFixedDelay(this::checkExpiredActions, 5, 5,
                 TimeUnit.MINUTES);
     }