JAMES-4164 Refactor VacationMailet

 - Use record instead of unnamed pair
 - Avoid double computation of RecipientId, AccountId
 - Undo unneeded extract, but re-extract relevant portion of the code

Author: chibenwa

server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/VacationMailet.java

@@ -30,7 +30,6 @@
 import jakarta.mail.internet.AddressException;
 import jakarta.mail.internet.InternetAddress;
 
-import org.apache.commons.lang3.tuple.Pair;
 import org.apache.james.core.MailAddress;
 import org.apache.james.mime4j.dom.address.Mailbox;
 import org.apache.james.mime4j.field.address.LenientAddressParser;
@@ -131,26 +130,24 @@ private void manageVacation(MailAddress recipient, Mail processedMail, ZonedDate
             return;
         }
 
-        AccountId accountId = AccountId.fromString(recipient.toString());
-
-        Mono<Vacation> vacation = vacationService.retrieveVacation(accountId);
-        Mono<Boolean> alreadySent = vacationService.isNotificationRegistered(
-                AccountId.fromString(recipient.toString()),
-                RecipientId.fromMailAddress(processedMail.getMaybeSender().get()));
-        Pair<Vacation, Boolean> pair = Flux.combineLatest(vacation, alreadySent, Pair::of)
-            .blockFirst();
+        RecipientId replyRecipient = RecipientId.fromMailAddress(processedMail.getMaybeSender().get());
+        VacationInformation vacationInformation = retrieveVacationInformation(recipient, replyRecipient);
 
-        sendNotificationIfRequired(recipient, processedMail, processingDate, pair.getKey(), pair.getValue());
+        boolean shouldSendNotification = vacationInformation.vacation.isActiveAtDate(processingDate) && !vacationInformation.alreadySent;
+        if (shouldSendNotification) {
+            sendNotification(processedMail, vacationInformation);
+        }
     }
 
-    private void sendNotificationIfRequired(MailAddress recipient, Mail processedMail, ZonedDateTime processingDate, Vacation vacation, Boolean alreadySent) {
-        if (shouldSendNotification(vacation, processingDate, alreadySent)) {
-            sendNotification(recipient, processedMail, vacation);
-        }
+    record VacationInformation(Vacation vacation, MailAddress recipient, AccountId accountId, RecipientId replyRecipient, Boolean alreadySent) {
+
     }
 
-    private boolean shouldSendNotification(Vacation vacation, ZonedDateTime processingDate, boolean alreadySent) {
-        return vacation.isActiveAtDate(processingDate) && !alreadySent;
+    private VacationInformation retrieveVacationInformation(MailAddress recipient, RecipientId replyRecipient) {
+        AccountId accountId = AccountId.fromString(recipient.toString());
+        Mono<Vacation> vacation = vacationService.retrieveVacation(accountId);
+        Mono<Boolean> alreadySent = vacationService.isNotificationRegistered(accountId, replyRecipient);
+        return Flux.combineLatest(vacation, alreadySent, (a, b) -> new VacationInformation(a, recipient, accountId, replyRecipient, b)).blockFirst();
     }
 
     private boolean isNoReplySender(Mail processedMail) {
@@ -168,30 +165,27 @@ private boolean isSentToSelf(Optional<MailAddress> maybeSender, MailAddress reci
             .orElse(false);
     }
 
-    private void sendNotification(MailAddress recipient, Mail processedMail, Vacation vacation) {
+    private void sendNotification(Mail processedMail, VacationInformation vacationInformation) {
         try {
             VacationReply vacationReply = VacationReply.builder(processedMail)
-                .receivedMailRecipient(recipient)
-                .vacation(vacation)
+                .replyRecipient(vacationInformation.replyRecipient.getMailAddress())
+                .receivedMailRecipient(vacationInformation.recipient())
+                .vacation(vacationInformation.vacation)
                 .build(mimeMessageBodyGenerator);
 
-            sendNotification(vacationReply, recipient);
+            getMailetContext().sendMail(getSender(vacationInformation.recipient),
+                vacationReply.getRecipients(),
+                vacationReply.getMimeMessage());
 
-            vacationService.registerNotification(AccountId.fromString(recipient.toString()),
-                RecipientId.fromMailAddress(processedMail.getMaybeSender().get()),
-                vacation.getToDate())
+            vacationService.registerNotification(vacationInformation.accountId(),
+                vacationInformation.replyRecipient(),
+                vacationInformation.vacation().getToDate())
                 .block();
         } catch (MessagingException e) {
-            LOGGER.warn("Failed to send JMAP vacation notification from {} to {}", recipient, processedMail.getMaybeSender(), e);
+            LOGGER.warn("Failed to send JMAP vacation notification from {} to {}", vacationInformation.recipient(), vacationInformation.replyRecipient().getAsString(), e);
         }
     }
 
-    private void sendNotification(VacationReply vacationReply, MailAddress recipient) throws MessagingException {
-        getMailetContext().sendMail(getSender(recipient),
-            vacationReply.getRecipients(),
-            vacationReply.getMimeMessage());
-    }
-
     private MailAddress getSender(MailAddress recipient) {
         if (!useUserAsMailFrom) {
             return MailAddress.nullSender();

server/mailet/mailets/src/main/java/org/apache/james/transport/mailets/VacationReply.java

@@ -32,6 +32,7 @@
 
 import com.github.fge.lambdas.Throwing;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 
 public class VacationReply {
 
@@ -47,6 +48,7 @@ public static class Builder {
         public static final boolean NOT_REPLY_TO_ALL = false;
         private final Mail originalMail;
         private MailAddress mailRecipient;
+        private MailAddress replyRecipient;
         private Vacation vacation;
 
         private Builder(Mail originalMail) {
@@ -65,18 +67,24 @@ public Builder vacation(Vacation vacation) {
             return this;
         }
 
+        public Builder replyRecipient(MailAddress replyRecipient) {
+            this.replyRecipient = replyRecipient;
+            return this;
+        }
+
         public VacationReply build(MimeMessageBodyGenerator mimeMessageBodyGenerator) throws MessagingException {
             Preconditions.checkState(mailRecipient != null, "Original recipient address should not be null");
+            Preconditions.checkState(replyRecipient != null, "Reply recipient address should not be null");
             Preconditions.checkState(originalMail.hasSender(), "Original sender address should not be null");
 
-            return new VacationReply(mailRecipient, originalMail.getMaybeSender().asList(), generateMimeMessage(mimeMessageBodyGenerator));
+            return new VacationReply(mailRecipient, ImmutableList.of(replyRecipient), generateMimeMessage(mimeMessageBodyGenerator));
         }
 
         private MimeMessage generateMimeMessage(MimeMessageBodyGenerator mimeMessageBodyGenerator) throws MessagingException {
             MimeMessage reply = (MimeMessage) originalMail.getMessage().reply(NOT_REPLY_TO_ALL);
-            vacation.getSubject().ifPresent(Throwing.consumer(subjectString -> reply.setSubject(subjectString)));
+            vacation.getSubject().ifPresent(Throwing.consumer(reply::setSubject));
             reply.setHeader(FROM_HEADER, mailRecipient.toString());
-            reply.setHeader(TO_HEADER, originalMail.getMaybeSender().get().asString());
+            reply.setHeader(TO_HEADER, replyRecipient.asString());
             reply.setHeader(AutomaticallySentMailDetector.AUTO_SUBMITTED_HEADER, AutomaticallySentMailDetector.AUTO_REPLIED_VALUE);
 
             return mimeMessageBodyGenerator.from(reply, vacation.getTextBody(), vacation.getHtmlBody());

server/mailet/mailets/src/test/java/org/apache/james/transport/mailets/VacationReplyTest.java

@@ -82,6 +82,7 @@ public void vacationReplyShouldGenerateASuitableAnswer() throws Exception {
                 .htmlBody(HTML_REASON)
                 .build())
             .receivedMailRecipient(originalRecipient)
+            .replyRecipient(originalSender)
             .build(mimeMessageBodyGenerator);
 
         assertThat(vacationReply.getRecipients()).containsExactly(originalSender);
@@ -96,6 +97,7 @@ public void vacationReplyShouldAddReSuffixToSubjectByDefault() throws Exception
                 .textBody(REASON)
                 .build())
             .receivedMailRecipient(originalRecipient)
+            .replyRecipient(originalSender)
             .build(mimeMessageBodyGenerator);
 
         verify(mimeMessageBodyGenerator).from(argThat(createSubjectMatcher("Re: Original subject")), any(), any());
@@ -111,6 +113,7 @@ public void subjectShouldBeQEncodedWhenSpecialCharacters() throws Exception {
                 .subject(Optional.of("Nghiêm Thị Tuyết Nhung"))
                 .textBody(REASON)
                 .build())
+            .replyRecipient(originalRecipient)
             .receivedMailRecipient(originalRecipient)
             .build(mimeMessageBodyGenerator);
 
@@ -126,6 +129,7 @@ public void aUserShouldBeAbleToSetTheSubjectOfTheGeneratedMimeMessage() throws E
                 .subject(Optional.of(SUBJECT))
                 .build())
             .receivedMailRecipient(originalRecipient)
+            .replyRecipient(originalSender)
             .build(mimeMessageBodyGenerator);
 
         verify(mimeMessageBodyGenerator).from(argThat(createSubjectMatcher(SUBJECT)), any(), any());