Development: Improve error handling when sending emails fails (#8555)

build.gradle

@@ -190,7 +190,7 @@ jacocoTestCoverageVerification {
                 counter = "CLASS"
                 value = "MISSEDCOUNT"
                 // TODO: in the future the following value should become less than 10
-                maximum = 26
+                maximum = 27
             }
         }
     }

src/main/java/de/tum/in/www1/artemis/service/notifications/MailService.java

@@ -35,7 +35,6 @@
 import de.tum.in.www1.artemis.domain.notification.NotificationConstants;
 import de.tum.in.www1.artemis.domain.participation.StudentParticipation;
 import de.tum.in.www1.artemis.domain.plagiarism.PlagiarismCase;
-import de.tum.in.www1.artemis.exception.ArtemisMailException;
 import de.tum.in.www1.artemis.service.TimeService;
 import tech.jhipster.config.JHipsterProperties;
 
@@ -133,7 +132,7 @@ public void sendEmail(User recipient, String subject, String content, boolean is
         }
         catch (MailException | MessagingException e) {
             log.error("Email could not be sent to user '{}'", recipient, e);
-            throw new ArtemisMailException("Email could not be sent to user", e);
+            // Note: we should not rethrow the exception here, as this would prevent sending out other emails in case multiple users are affected
         }
     }
 
@@ -246,7 +245,16 @@ private String createAnnouncementText(Object notificationSubject, Locale locale)
     @Override
     @Async
     public void sendNotification(Notification notification, Set<User> users, Object notificationSubject) {
-        users.forEach(user -> sendNotification(notification, user, notificationSubject));
+        // TODO: we should track how many emails could not be sent and notify the instructors in case of announcements or other important notifications
+        users.forEach(user -> {
+            try {
+                sendNotification(notification, user, notificationSubject);
+            }
+            catch (Exception ex) {
+                // Note: we should not rethrow the exception here, as this would prevent sending out other emails in case multiple users are affected
+                log.error("Error while sending notification email to user '{}'", user.getLogin(), ex);
+            }
+        });
     }
 
     /**
@@ -259,12 +267,14 @@ public void sendNotification(Notification notification, Set<User> users, Object
     @Override
     public void sendNotification(Notification notification, User user, Object notificationSubject) {
         NotificationType notificationType = NotificationConstants.findCorrespondingNotificationType(notification.getTitle());
-        log.debug("Sending \"{}\" notification email to '{}'", notificationType.name(), user.getEmail());
+        log.debug("Sending '{}' notification email to '{}'", notificationType.name(), user.getEmail());
 
         String localeKey = user.getLangKey();
         if (localeKey == null) {
-            throw new IllegalArgumentException(
-                    "The user object has no language key defined. This can happen if you do not load the user object from the database but take it straight from the client");
+            log.error("The user '{}' object has no language key defined. This can happen if you do not load the user object from the database but take it straight from the client",
+                    user.getLogin());
+            // use the default locale
+            localeKey = "en";
         }
 
         Locale locale = Locale.forLanguageTag(localeKey);

src/test/java/de/tum/in/www1/artemis/metis/PostingServiceUnitTest.java

@@ -13,6 +13,7 @@
 import java.lang.reflect.Method;
 import java.util.Set;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.InjectMocks;
@@ -40,14 +41,23 @@ class PostingServiceUnitTest {
 
     private Method parseUserMentions;
 
+    private AutoCloseable closeable;
+
     @BeforeEach
     void initTestCase() throws NoSuchMethodException {
-        MockitoAnnotations.openMocks(this);
+        closeable = MockitoAnnotations.openMocks(this);
 
         parseUserMentions = PostingService.class.getDeclaredMethod("parseUserMentions", Course.class, String.class);
         parseUserMentions.setAccessible(true);
     }
 
+    @AfterEach
+    void tearDown() throws Exception {
+        if (closeable != null) {
+            closeable.close();
+        }
+    }
+
     @Test
     void testParseUserMentionsEmptyContent() throws InvocationTargetException, IllegalAccessException {
         Course course = new Course();

src/test/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationServiceTest.java

@@ -8,6 +8,7 @@
 import java.util.HashSet;
 import java.util.Set;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mock;
@@ -44,9 +45,11 @@ class GeneralInstantNotificationServiceTest {
 
     private Notification notification;
 
+    private AutoCloseable closeable;
+
     @BeforeEach
     void setUp() {
-        MockitoAnnotations.openMocks(this);
+        closeable = MockitoAnnotations.openMocks(this);
         student1 = new User();
         student1.setId(1L);
         student1.setLogin("1");
@@ -63,6 +66,13 @@ void setUp() {
         when(notificationSettingsService.checkNotificationTypeForEmailSupport(any(NotificationType.class))).thenCallRealMethod();
     }
 
+    @AfterEach
+    void tearDown() throws Exception {
+        if (closeable != null) {
+            closeable.close();
+        }
+    }
+
     /**
      * Very basic test that checks if emails and pushes are send for one user
      */

src/test/java/de/tum/in/www1/artemis/service/notifications/MailServiceTest.java

@@ -1,23 +1,36 @@
 package de.tum.in.www1.artemis.service.notifications;
 
-import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+import static org.assertj.core.api.Assertions.assertThatNoException;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.time.ZonedDateTime;
+import java.util.Set;
+
 import jakarta.mail.internet.MimeMessage;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mock;
 import org.springframework.context.MessageSource;
+import org.springframework.mail.MailSendException;
 import org.springframework.mail.javamail.JavaMailSender;
+import org.springframework.test.util.ReflectionTestUtils;
 import org.thymeleaf.spring6.SpringTemplateEngine;
 
+import de.tum.in.www1.artemis.domain.Course;
 import de.tum.in.www1.artemis.domain.User;
-import de.tum.in.www1.artemis.exception.ArtemisMailException;
+import de.tum.in.www1.artemis.domain.enumeration.GroupNotificationType;
+import de.tum.in.www1.artemis.domain.metis.Post;
+import de.tum.in.www1.artemis.domain.metis.conversation.Channel;
+import de.tum.in.www1.artemis.domain.notification.GroupNotification;
+import de.tum.in.www1.artemis.domain.notification.NotificationConstants;
 import de.tum.in.www1.artemis.service.TimeService;
 import tech.jhipster.config.JHipsterProperties;
 
@@ -53,6 +66,8 @@ class MailServiceTest {
 
     private User student1;
 
+    private User student2;
+
     private String subject;
 
     private String content;
@@ -61,11 +76,17 @@ class MailServiceTest {
      * Prepares the needed values and objects for testing
      */
     @BeforeEach
-    void setUp() {
+    void setUp() throws MalformedURLException, URISyntaxException {
         student1 = new User();
+        student1.setLogin("student1");
         student1.setId(555L);
-        String EMAIL_ADDRESS_A = "[email protected]";
-        student1.setEmail(EMAIL_ADDRESS_A);
+        student1.setEmail("[email protected]");
+        student1.setLangKey("de");
+
+        student2 = new User();
+        student2.setLogin("student2");
+        student2.setId(556L);
+        student2.setEmail("[email protected]");
 
         subject = "subject";
         content = "content";
@@ -82,7 +103,14 @@ void setUp() {
         jHipsterProperties = mock(JHipsterProperties.class);
         when(jHipsterProperties.getMail()).thenReturn(mail);
 
+        messageSource = mock(MessageSource.class);
+        when(messageSource.getMessage(any(String.class), any(), any())).thenReturn("test");
+
+        templateEngine = mock(SpringTemplateEngine.class);
+        when(templateEngine.process(any(String.class), any())).thenReturn("test");
+
         mailService = new MailService(jHipsterProperties, javaMailSender, messageSource, templateEngine, timeService);
+        ReflectionTestUtils.setField(mailService, "artemisServerUrl", new URI("http://localhost:8080").toURL());
     }
 
     /**
@@ -95,11 +123,34 @@ void testSendEmail() {
     }
 
     /**
-     * When the javaMailSender returns an exception, that exception should be caught and an ArtemisMailException should be thrown instead.
+     * When the javaMailSender returns an exception, that exception should be caught and should not be thrown instead.
+     */
+    @Test
+    void testNoMailSendExceptionThrown() {
+        doThrow(new MailSendException("Some error occurred during mail send")).when(javaMailSender).send(any(MimeMessage.class));
+        assertThatNoException().isThrownBy(() -> mailService.sendEmail(student1, subject, content, false, true));
+    }
+
+    /**
+     * When the javaMailSender returns an exception, that exception should be caught and should not be thrown instead.
      */
     @Test
-    void testThrowException() {
-        doThrow(new org.springframework.mail.MailSendException("Some error occurred")).when(javaMailSender).send(any(MimeMessage.class));
-        assertThatExceptionOfType(ArtemisMailException.class).isThrownBy(() -> mailService.sendEmail(student1, subject, content, false, true));
+    void testNoExceptionThrown() {
+        doThrow(new RuntimeException("Some random error occurred")).when(javaMailSender).send(any(MimeMessage.class));
+        var notification = new GroupNotification(null, NotificationConstants.NEW_ANNOUNCEMENT_POST_TITLE, NotificationConstants.NEW_ANNOUNCEMENT_POST_TEXT, false, new String[0],
+                student1, GroupNotificationType.STUDENT);
+        Post post = new Post();
+        post.setAuthor(student1);
+        post.setCreationDate(ZonedDateTime.now());
+        post.setVisibleForStudents(true);
+        post.setContent("hi test");
+        post.setTitle("announcement");
+
+        Course course = new Course();
+        course.setId(141L);
+        Channel channel = new Channel();
+        channel.setCourse(course);
+        post.setConversation(channel);
+        assertThatNoException().isThrownBy(() -> mailService.sendNotification(notification, Set.of(student1, student2), post));
     }
 }

src/test/java/de/tum/in/www1/artemis/service/notifications/push_notifications/AppleFirebasePushNotificationServiceTest.java

@@ -14,6 +14,7 @@
 import java.util.HexFormat;
 import java.util.Optional;
 
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mock;
@@ -51,9 +52,11 @@ class AppleFirebasePushNotificationServiceTest {
 
     private User student;
 
+    private AutoCloseable closeable;
+
     @BeforeEach
     void setUp() {
-        MockitoAnnotations.openMocks(this);
+        closeable = MockitoAnnotations.openMocks(this);
 
         student = new User();
         student.setId(1L);
@@ -79,6 +82,13 @@ void setUp() {
         ReflectionTestUtils.setField(firebasePushNotificationService, "relayServerBaseUrl", Optional.of("test"));
     }
 
+    @AfterEach
+    void tearDown() throws Exception {
+        if (closeable != null) {
+            closeable.close();
+        }
+    }
+
     @Test
     void sendNotificationRequestsToEndpoint_shouldSendNotifications() throws InterruptedException {
         // Given