From f40b0fb7138b6841ce808929dee5d51634ecd95a Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Tue, 7 May 2024 22:04:55 +0200 Subject: [PATCH] Development: Improve error handling when sending emails fails (#8555) --- build.gradle | 2 +- .../service/notifications/MailService.java | 22 ++++-- .../artemis/metis/PostingServiceUnitTest.java | 12 +++- ...GeneralInstantNotificationServiceTest.java | 12 +++- .../notifications/MailServiceTest.java | 69 ++++++++++++++++--- ...leFirebasePushNotificationServiceTest.java | 12 +++- 6 files changed, 110 insertions(+), 19 deletions(-) diff --git a/build.gradle b/build.gradle index 4d3f46d6cc32..07ee7acd4b1e 100644 --- a/build.gradle +++ b/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 } } } diff --git a/src/main/java/de/tum/in/www1/artemis/service/notifications/MailService.java b/src/main/java/de/tum/in/www1/artemis/service/notifications/MailService.java index ddd15f3dda06..3abe2ae70d73 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/notifications/MailService.java +++ b/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 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 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); diff --git a/src/test/java/de/tum/in/www1/artemis/metis/PostingServiceUnitTest.java b/src/test/java/de/tum/in/www1/artemis/metis/PostingServiceUnitTest.java index 539a706b1fd2..3f650e76ee09 100644 --- a/src/test/java/de/tum/in/www1/artemis/metis/PostingServiceUnitTest.java +++ b/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(); diff --git a/src/test/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationServiceTest.java b/src/test/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationServiceTest.java index 13c229e0e7a1..275c2076d630 100644 --- a/src/test/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationServiceTest.java +++ b/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 */ diff --git a/src/test/java/de/tum/in/www1/artemis/service/notifications/MailServiceTest.java b/src/test/java/de/tum/in/www1/artemis/service/notifications/MailServiceTest.java index a8afcece758e..1af71cfe2c9a 100644 --- a/src/test/java/de/tum/in/www1/artemis/service/notifications/MailServiceTest.java +++ b/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 = "benige8246@omibrown.com"; - student1.setEmail(EMAIL_ADDRESS_A); + student1.setEmail("benige8246@omibrown.com"); + student1.setLangKey("de"); + + student2 = new User(); + student2.setLogin("student2"); + student2.setId(556L); + student2.setEmail("bege123@abc.com"); 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)); } } diff --git a/src/test/java/de/tum/in/www1/artemis/service/notifications/push_notifications/AppleFirebasePushNotificationServiceTest.java b/src/test/java/de/tum/in/www1/artemis/service/notifications/push_notifications/AppleFirebasePushNotificationServiceTest.java index b0da4e33550a..c52848ac92ae 100644 --- a/src/test/java/de/tum/in/www1/artemis/service/notifications/push_notifications/AppleFirebasePushNotificationServiceTest.java +++ b/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