From 399a5fab4cff63c5fc4f1c3a6bbbbf76e8efab07 Mon Sep 17 00:00:00 2001 From: Marques Tye Jia Jun <97437396+marquestye@users.noreply.github.com> Date: Tue, 9 Jul 2024 00:05:22 +0800 Subject: [PATCH] [#12779] Fix notifications update logic (#13083) * Fix notifications update logic * Add required mocks --------- Co-authored-by: Ching Ming Yuan --- .../sqllogic/core/NotificationsLogicIT.java | 77 +++++++++++++++++++ .../teammates/common/util/HibernateUtil.java | 9 +++ .../sqllogic/core/NotificationsLogic.java | 11 +++ .../teammates/storage/sqlapi/EntitiesDb.java | 2 +- .../storage/sqlapi/NotificationsDb.java | 33 ++++++-- .../sqllogic/core/NotificationsLogicTest.java | 45 +++++++++++ .../storage/sqlapi/AccountRequestsDbTest.java | 1 + .../storage/sqlapi/AccountsDbTest.java | 1 + .../storage/sqlapi/CoursesDbTest.java | 1 + .../FeedbackResponseCommentsDbTest.java | 1 + .../sqlapi/FeedbackSessionsDbTest.java | 3 + .../storage/sqlapi/NotificationsDbTest.java | 34 ++------ .../teammates/storage/sqlapi/UsersDbTest.java | 1 + 13 files changed, 187 insertions(+), 32 deletions(-) diff --git a/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java b/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java index b96f6091cd5..f73b5b3e499 100644 --- a/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java +++ b/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java @@ -10,6 +10,7 @@ import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; +import teammates.common.util.HibernateUtil; import teammates.it.test.BaseTestCaseWithSqlDatabaseAccess; import teammates.sqllogic.core.NotificationsLogic; import teammates.storage.sqlentity.Notification; @@ -21,6 +22,48 @@ public class NotificationsLogicIT extends BaseTestCaseWithSqlDatabaseAccess { private NotificationsLogic notificationsLogic = NotificationsLogic.inst(); + @Test + public void testCreateNotification_endTimeIsBeforeStartTime_throwsInvalidParametersException() { + Notification invalidNotification = new Notification( + Instant.parse("2011-02-01T00:00:00Z"), + Instant.parse("2011-01-01T00:00:00Z"), + NotificationStyle.DANGER, + NotificationTargetUser.GENERAL, + "A deprecation note", + "

Deprecation happens in three minutes

"); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + assertNull(notificationsLogic.getNotification(invalidNotification.getId())); + } + + @Test + public void testCreateNotification_emptyTitle_throwsInvalidParametersException() { + Notification invalidNotification = new Notification( + Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), + NotificationStyle.DANGER, + NotificationTargetUser.GENERAL, + "", + "

Deprecation happens in three minutes

"); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + assertNull(notificationsLogic.getNotification(invalidNotification.getId())); + } + + @Test + public void testCreateNotification_emptyMessage_throwsInvalidParametersException() { + Notification invalidNotification = new Notification( + Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), + NotificationStyle.DANGER, + NotificationTargetUser.GENERAL, + "A deprecation note", + ""); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + assertNull(notificationsLogic.getNotification(invalidNotification.getId())); + } + @Test public void testUpdateNotification() throws EntityAlreadyExistsException, InvalidParametersException, EntityDoesNotExistException { @@ -59,4 +102,38 @@ public void testUpdateNotification() newStartTime, newEndTime, newStyle, newTargetUser, newTitle, newMessage)); } + @Test + public void testUpdateNotification_invalidParameters_originalUnchanged() throws Exception { + + String originalTitle = "The original title"; + String originalMessage = "

Deprecation happens in three minutes

"; + Notification notif = new Notification( + Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), + NotificationStyle.DANGER, + NotificationTargetUser.GENERAL, + originalTitle, + originalMessage); + notificationsLogic.createNotification(notif); + + String invalidLongTitle = "1234567890".repeat(10); + String validNewMessage = "This should not be reflected."; + assertThrows( + InvalidParametersException.class, + () -> notificationsLogic.updateNotification( + notif.getId(), + Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), + NotificationStyle.DANGER, + NotificationTargetUser.GENERAL, + invalidLongTitle, + validNewMessage)); + + HibernateUtil.flushSession(); + HibernateUtil.clearSession(); + Notification actual = notificationsLogic.getNotification(notif.getId()); + assertEquals(originalTitle, actual.getTitle()); + assertEquals(originalMessage, actual.getMessage()); + } + } diff --git a/src/main/java/teammates/common/util/HibernateUtil.java b/src/main/java/teammates/common/util/HibernateUtil.java index 7c91330bcc1..99525427e38 100644 --- a/src/main/java/teammates/common/util/HibernateUtil.java +++ b/src/main/java/teammates/common/util/HibernateUtil.java @@ -294,4 +294,13 @@ public static T getReference(Class entityType, Object id) { return getCurrentSession().getReference(entityType, id); } + /** + * Flush the current session and evict the given entity from the session. + * @see Session#evict(Object) + */ + public static void flushAndEvict(T entity) { + flushSession(); + getCurrentSession().evict(entity); + } + } diff --git a/src/main/java/teammates/sqllogic/core/NotificationsLogic.java b/src/main/java/teammates/sqllogic/core/NotificationsLogic.java index 3b8b5df8750..5e2acfb4b97 100644 --- a/src/main/java/teammates/sqllogic/core/NotificationsLogic.java +++ b/src/main/java/teammates/sqllogic/core/NotificationsLogic.java @@ -11,6 +11,7 @@ import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; +import teammates.common.util.HibernateUtil; import teammates.storage.sqlapi.NotificationsDb; import teammates.storage.sqlentity.Notification; @@ -47,6 +48,11 @@ public void initLogicDependencies(NotificationsDb notificationsDb) { */ public Notification createNotification(Notification notification) throws InvalidParametersException, EntityAlreadyExistsException { + assert notification != null; + + if (!notification.isValid()) { + throw new InvalidParametersException(notification.getInvalidityInfo()); + } return notificationsDb.createNotification(notification); } @@ -78,6 +84,9 @@ public Notification updateNotification(UUID notificationId, Instant startTime, I throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + Notification.class); } + // evict managed entity to avoid auto-persist + HibernateUtil.flushAndEvict(notification); + notification.setStartTime(startTime); notification.setEndTime(endTime); notification.setStyle(style); @@ -89,6 +98,8 @@ public Notification updateNotification(UUID notificationId, Instant startTime, I throw new InvalidParametersException(notification.getInvalidityInfo()); } + notificationsDb.updateNotification(notification); + return notification; } diff --git a/src/main/java/teammates/storage/sqlapi/EntitiesDb.java b/src/main/java/teammates/storage/sqlapi/EntitiesDb.java index 8ad58987ab2..7d428bb4a56 100644 --- a/src/main/java/teammates/storage/sqlapi/EntitiesDb.java +++ b/src/main/java/teammates/storage/sqlapi/EntitiesDb.java @@ -20,7 +20,7 @@ protected T merge(T entity) { assert entity != null; T newEntity = HibernateUtil.merge(entity); - log.info("Entity saved: " + entity.toString()); + log.info("Entity saved: " + newEntity.toString()); return newEntity; } diff --git a/src/main/java/teammates/storage/sqlapi/NotificationsDb.java b/src/main/java/teammates/storage/sqlapi/NotificationsDb.java index 3a3c114e42f..78449685778 100644 --- a/src/main/java/teammates/storage/sqlapi/NotificationsDb.java +++ b/src/main/java/teammates/storage/sqlapi/NotificationsDb.java @@ -1,5 +1,8 @@ package teammates.storage.sqlapi; +import static teammates.common.util.Const.ERROR_CREATE_ENTITY_ALREADY_EXISTS; +import static teammates.common.util.Const.ERROR_UPDATE_NON_EXISTENT; + import java.time.Instant; import java.util.List; import java.util.UUID; @@ -11,7 +14,7 @@ import teammates.common.datatransfer.NotificationTargetUser; import teammates.common.exception.EntityAlreadyExistsException; -import teammates.common.exception.InvalidParametersException; +import teammates.common.exception.EntityDoesNotExistException; import teammates.common.util.HibernateUtil; import teammates.storage.sqlentity.Notification; @@ -34,13 +37,16 @@ public static NotificationsDb inst() { /** * Creates a notification. + * + *

Preconditions:

+ * * Notification fields are valid. */ - public Notification createNotification(Notification notification) - throws InvalidParametersException, EntityAlreadyExistsException { + public Notification createNotification(Notification notification) throws EntityAlreadyExistsException { assert notification != null; - if (!notification.isValid()) { - throw new InvalidParametersException(notification.getInvalidityInfo()); + if (getNotification(notification.getId()) != null) { + throw new EntityAlreadyExistsException( + String.format(ERROR_CREATE_ENTITY_ALREADY_EXISTS, notification.toString())); } persist(notification); @@ -98,4 +104,21 @@ public List getActiveNotificationsByTargetUser(NotificationTargetU TypedQuery query = HibernateUtil.createQuery(cq); return query.getResultList(); } + + /** + * Updates a notification. + * + *

Preconditions:

+ * * Notification fields are valid. + */ + public Notification updateNotification(Notification notification) throws EntityDoesNotExistException { + assert notification != null; + + if (getNotification(notification.getId()) == null) { + throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + Notification.class); + } + + return merge(notification); + } + } diff --git a/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java b/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java index f54c1899ac6..f7d6fde8c0b 100644 --- a/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java +++ b/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java @@ -1,6 +1,8 @@ package teammates.sqllogic.core; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -8,13 +10,17 @@ import java.time.Instant; import java.util.UUID; +import org.mockito.MockedStatic; +import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import teammates.common.datatransfer.NotificationStyle; import teammates.common.datatransfer.NotificationTargetUser; +import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; import teammates.common.exception.InvalidParametersException; +import teammates.common.util.HibernateUtil; import teammates.storage.sqlapi.NotificationsDb; import teammates.storage.sqlentity.Notification; import teammates.test.BaseTestCase; @@ -28,10 +34,49 @@ public class NotificationsLogicTest extends BaseTestCase { private NotificationsDb notificationsDb; + private MockedStatic mockHibernateUtil; + @BeforeMethod public void setUpMethod() { notificationsDb = mock(NotificationsDb.class); notificationsLogic.initLogicDependencies(notificationsDb); + mockHibernateUtil = mockStatic(HibernateUtil.class); + } + + @AfterMethod + public void tearDownMethod() { + mockHibernateUtil.close(); + } + + @Test + public void testCreateNotification_endTimeIsBeforeStartTime_throwsInvalidParametersException() + throws EntityAlreadyExistsException { + Notification invalidNotification = new Notification(Instant.parse("2011-02-01T00:00:00Z"), + Instant.parse("2011-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, + "A deprecation note", "

Deprecation happens in three minutes

"); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + verify(notificationsDb, never()).createNotification(invalidNotification); + } + + @Test + public void testCreateNotification_emptyTitle_throwsInvalidParametersException() throws EntityAlreadyExistsException { + Notification invalidNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, + "", "

Deprecation happens in three minutes

"); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + verify(notificationsDb, never()).createNotification(invalidNotification); + } + + @Test + public void testCreateNotification_emptyMessage_throwsInvalidParametersException() throws EntityAlreadyExistsException { + Notification invalidNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), + Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, + "A deprecation note", ""); + + assertThrows(InvalidParametersException.class, () -> notificationsLogic.createNotification(invalidNotification)); + verify(notificationsDb, never()).createNotification(invalidNotification); } @Test diff --git a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java index 1044cb034c4..e6a4c94a27c 100644 --- a/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/AccountRequestsDbTest.java @@ -105,6 +105,7 @@ public void testUpdateAccountRequest_success() throws InvalidParametersException AccountRequest accountRequest = new AccountRequest("test@gmail.com", "name", "institute", AccountRequestStatus.PENDING, "comments"); doReturn(accountRequest).when(accountRequestDb).getAccountRequest(accountRequest.getId()); + mockHibernateUtil.when(() -> HibernateUtil.merge(accountRequest)).thenReturn(accountRequest); accountRequestDb.updateAccountRequest(accountRequest); diff --git a/src/test/java/teammates/storage/sqlapi/AccountsDbTest.java b/src/test/java/teammates/storage/sqlapi/AccountsDbTest.java index c9b7b0b9724..817b92deb98 100644 --- a/src/test/java/teammates/storage/sqlapi/AccountsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/AccountsDbTest.java @@ -114,6 +114,7 @@ public void testUpdateAccount_accountAlreadyExists_success() Account account = getTypicalAccount(); mockHibernateUtil.when(() -> HibernateUtil.get(Account.class, account.getId())) .thenReturn(account); + mockHibernateUtil.when(() -> HibernateUtil.merge(account)).thenReturn(account); account.setName("new name"); accountsDb.updateAccount(account); diff --git a/src/test/java/teammates/storage/sqlapi/CoursesDbTest.java b/src/test/java/teammates/storage/sqlapi/CoursesDbTest.java index 916233cf556..5379d23aa16 100644 --- a/src/test/java/teammates/storage/sqlapi/CoursesDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/CoursesDbTest.java @@ -117,6 +117,7 @@ public void testUpdateCourse_courseDoesNotExist_throwsEntityDoesNotExistExceptio public void testUpdateCourse_success() throws InvalidParametersException, EntityDoesNotExistException { Course c = new Course("course-id", "new-course-name", null, "institute"); doReturn(c).when(coursesDb).getCourse(anyString()); + mockHibernateUtil.when(() -> HibernateUtil.merge(c)).thenReturn(c); coursesDb.updateCourse(c); diff --git a/src/test/java/teammates/storage/sqlapi/FeedbackResponseCommentsDbTest.java b/src/test/java/teammates/storage/sqlapi/FeedbackResponseCommentsDbTest.java index 591cd297c10..b4a34693675 100644 --- a/src/test/java/teammates/storage/sqlapi/FeedbackResponseCommentsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/FeedbackResponseCommentsDbTest.java @@ -126,6 +126,7 @@ public void testUpdateCourse_success() throws InvalidParametersException, Entity comment.setCommentText("Placeholder Text"); doReturn(comment).when(feedbackResponseCommentsDb).getFeedbackResponseComment(anyLong()); + mockHibernateUtil.when(() -> HibernateUtil.merge(comment)).thenReturn(comment); feedbackResponseCommentsDb.updateFeedbackResponseComment(comment); mockHibernateUtil.verify(() -> HibernateUtil.merge(comment)); diff --git a/src/test/java/teammates/storage/sqlapi/FeedbackSessionsDbTest.java b/src/test/java/teammates/storage/sqlapi/FeedbackSessionsDbTest.java index 9a5fceedccf..9f669c72ff3 100644 --- a/src/test/java/teammates/storage/sqlapi/FeedbackSessionsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/FeedbackSessionsDbTest.java @@ -106,6 +106,7 @@ public void testGetFeedbackSession_sessionDoesNotExists_returnNull() { public void testUpdateFeedbackSession_success() throws InvalidParametersException, EntityDoesNotExistException { FeedbackSession feedbackSession = getTypicalFeedbackSessionForCourse(getTypicalCourse()); doReturn(feedbackSession).when(feedbackSessionsDb).getFeedbackSession(any(UUID.class)); + mockHibernateUtil.when(() -> HibernateUtil.merge(feedbackSession)).thenReturn(feedbackSession); feedbackSessionsDb.updateFeedbackSession(feedbackSession); @@ -189,6 +190,7 @@ public void testRestoreDeletedFeedbackSession_success() throws EntityDoesNotExis String courseId = feedbackSession.getCourse().getId(); feedbackSession.setDeletedAt(TimeHelperExtension.getInstantDaysOffsetFromNow(2)); doReturn(feedbackSession).when(feedbackSessionsDb).getFeedbackSession(sessionName, courseId); + mockHibernateUtil.when(() -> HibernateUtil.merge(feedbackSession)).thenReturn(feedbackSession); feedbackSessionsDb.restoreDeletedFeedbackSession(sessionName, courseId); @@ -215,6 +217,7 @@ public void testSoftDeleteFeedbackSession_success() throws EntityDoesNotExistExc String sessionName = feedbackSession.getName(); String courseId = feedbackSession.getCourse().getId(); doReturn(feedbackSession).when(feedbackSessionsDb).getFeedbackSession(sessionName, courseId); + mockHibernateUtil.when(() -> HibernateUtil.merge(feedbackSession)).thenReturn(feedbackSession); feedbackSessionsDb.softDeleteFeedbackSession(sessionName, courseId); diff --git a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java index d1489f99c73..e23b6681609 100644 --- a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java @@ -15,7 +15,6 @@ import teammates.common.datatransfer.NotificationStyle; import teammates.common.datatransfer.NotificationTargetUser; import teammates.common.exception.EntityAlreadyExistsException; -import teammates.common.exception.InvalidParametersException; import teammates.common.util.HibernateUtil; import teammates.storage.sqlentity.Notification; import teammates.test.BaseTestCase; @@ -40,8 +39,7 @@ public void teardownMethod() { } @Test - public void testCreateNotification_notificationDoesNotExist_success() - throws EntityAlreadyExistsException, InvalidParametersException { + public void testCreateNotification_notificationDoesNotExist_success() throws EntityAlreadyExistsException { Notification newNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, "A deprecation note", "

Deprecation happens in three minutes

"); @@ -52,33 +50,17 @@ public void testCreateNotification_notificationDoesNotExist_success() } @Test - public void testCreateNotification_endTimeIsBeforeStartTime_throwsInvalidParametersException() { - Notification invalidNotification = new Notification(Instant.parse("2011-02-01T00:00:00Z"), - Instant.parse("2011-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "A deprecation note", "

Deprecation happens in three minutes

"); - - assertThrows(InvalidParametersException.class, () -> notificationsDb.createNotification(invalidNotification)); - mockHibernateUtil.verify(() -> HibernateUtil.persist(invalidNotification), never()); - } - - @Test - public void testCreateNotification_emptyTitle_throwsInvalidParametersException() { - Notification invalidNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), + public void testCreateNotification_notificationExists_throwsEntityAlreadyExistsException() { + Notification existingNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "", "

Deprecation happens in three minutes

"); + "A deprecation note", "

Deprecation happens in three minutes

"); - assertThrows(InvalidParametersException.class, () -> notificationsDb.createNotification(invalidNotification)); - mockHibernateUtil.verify(() -> HibernateUtil.persist(invalidNotification), never()); - } + mockHibernateUtil.when(() -> HibernateUtil.get(Notification.class, existingNotification.getId())) + .thenReturn(existingNotification); - @Test - public void testCreateNotification_emptyMessage_throwsInvalidParametersException() { - Notification invalidNotification = new Notification(Instant.parse("2011-01-01T00:00:00Z"), - Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "A deprecation note", ""); + assertThrows(EntityAlreadyExistsException.class, () -> notificationsDb.createNotification(existingNotification)); - assertThrows(InvalidParametersException.class, () -> notificationsDb.createNotification(invalidNotification)); - mockHibernateUtil.verify(() -> HibernateUtil.persist(invalidNotification), never()); + mockHibernateUtil.verify(() -> HibernateUtil.persist(existingNotification), never()); } @Test diff --git a/src/test/java/teammates/storage/sqlapi/UsersDbTest.java b/src/test/java/teammates/storage/sqlapi/UsersDbTest.java index 7a5cfd7b78f..3a0b984b459 100644 --- a/src/test/java/teammates/storage/sqlapi/UsersDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/UsersDbTest.java @@ -144,6 +144,7 @@ public void testUpdateStudent_success() Student existingStudent = getTypicalStudent(); doReturn(existingStudent).when(usersDb).getStudent(any()); + mockHibernateUtil.when(() -> HibernateUtil.merge(existingStudent)).thenReturn(existingStudent); usersDb.updateStudent(existingStudent);