From 81687959f57267f0cb3a1b172f3e9969509e48de Mon Sep 17 00:00:00 2001 From: marquestye Date: Tue, 27 Feb 2024 14:49:42 +0800 Subject: [PATCH 1/3] Fix auto persistence --- .../sqllogic/core/NotificationsLogicIT.java | 31 +++++++ .../it/sqllogic/core/UsersLogicIT.java | 84 +++++++++++++++++++ ...edbackSessionOpeningRemindersActionIT.java | 4 +- .../GetSessionResponseStatsActionIT.java | 2 +- .../UpdateFeedbackQuestionActionIT.java | 28 +++++++ .../sqllogic/core/AccountRequestsLogic.java | 2 +- .../sqllogic/core/AccountsLogic.java | 1 - .../teammates/sqllogic/core/CoursesLogic.java | 11 ++- .../sqllogic/core/FeedbackQuestionsLogic.java | 33 ++++---- .../sqllogic/core/NotificationsLogic.java | 21 +++-- .../teammates/sqllogic/core/UsersLogic.java | 23 ++--- .../storage/sqlapi/AccountRequestsDb.java | 5 +- .../teammates/storage/sqlapi/CoursesDb.java | 2 +- .../storage/sqlapi/DeadlineExtensionsDb.java | 2 +- .../storage/sqlapi/FeedbackQuestionsDb.java | 23 +++++ .../storage/sqlapi/NotificationsDb.java | 20 +++++ .../storage/sqlentity/AccountRequest.java | 15 ++++ .../teammates/storage/sqlentity/Course.java | 15 ++++ .../storage/sqlentity/FeedbackQuestion.java | 13 +++ .../storage/sqlentity/FeedbackSession.java | 6 +- .../storage/sqlentity/Instructor.java | 17 ++++ .../storage/sqlentity/Notification.java | 16 ++++ .../ui/webapi/CreateAccountAction.java | 11 +-- .../webapi/UpdateFeedbackSessionAction.java | 1 + .../sqllogic/core/CoursesLogicTest.java | 7 ++ .../UpdateFeedbackSessionActionTest.java | 29 +++++++ .../storage/sqlapi/NotificationsDbTest.java | 29 ++++++- 27 files changed, 392 insertions(+), 59 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..f1e20de3941 100644 --- a/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java +++ b/src/it/java/teammates/it/sqllogic/core/NotificationsLogicIT.java @@ -59,4 +59,35 @@ public void testUpdateNotification() newStartTime, newEndTime, newStyle, newTargetUser, newTitle, newMessage)); } + @Test + public void testUpdateNotification_invalidParameters_originalUnchanged() throws Exception { + + Notification notif = 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

"); + notificationsLogic.createNotification(notif); + + String invalidLongTitle = "1234567890".repeat(10); + + 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, + "

Deprecation happens in three minutes

" + ) + ); + + Notification actual = notificationsLogic.getNotification(notif.getId()); + assert actual.getTitle().equals(notif.getTitle()) : actual.getTitle(); + } + } diff --git a/src/it/java/teammates/it/sqllogic/core/UsersLogicIT.java b/src/it/java/teammates/it/sqllogic/core/UsersLogicIT.java index 1caf2c2235c..584e3bdb074 100644 --- a/src/it/java/teammates/it/sqllogic/core/UsersLogicIT.java +++ b/src/it/java/teammates/it/sqllogic/core/UsersLogicIT.java @@ -6,6 +6,7 @@ import teammates.common.datatransfer.InstructorPrivileges; import teammates.common.exception.EntityAlreadyExistsException; import teammates.common.exception.EntityDoesNotExistException; +import teammates.common.exception.InstructorUpdateException; import teammates.common.exception.InvalidParametersException; import teammates.common.util.Const; import teammates.common.util.Const.InstructorPermissions; @@ -16,7 +17,10 @@ import teammates.storage.sqlentity.Account; import teammates.storage.sqlentity.Course; import teammates.storage.sqlentity.Instructor; +import teammates.storage.sqlentity.Section; import teammates.storage.sqlentity.Student; +import teammates.storage.sqlentity.Team; +import teammates.ui.request.InstructorCreateRequest; /** * SUT: {@link UsersLogic}. @@ -140,4 +144,84 @@ public void testUpdateToEnsureValidityOfInstructorsForTheCourse() { assertFalse(instructor.getPrivileges().isAllowedForPrivilege( Const.InstructorPermissions.CAN_MODIFY_INSTRUCTOR)); } + + @Test + public void testUpdateInstructorCascade() throws InvalidParametersException, InstructorUpdateException, + EntityAlreadyExistsException, EntityDoesNotExistException { + Instructor instructor = getTypicalInstructor(); + instructor.setCourse(course); + instructor.setAccount(account); + + ______TS("success: typical case"); + usersLogic.createInstructor(instructor); + + String newName = "new name"; + String newEmail = "new_inst_email@newmail.com"; + String newRoleName = "Manager"; + String newDisplayName = "new display name"; + boolean newIsDisplayedToStudent = true; + InstructorCreateRequest request = new InstructorCreateRequest( + instructor.getGoogleId(), newName, newEmail, newRoleName, newDisplayName, newIsDisplayedToStudent); + + Instructor updatedInstructor = usersLogic.updateInstructorCascade(instructor.getCourseId(), request); + assertEquals(newName, updatedInstructor.getName()); + assertEquals(newEmail, updatedInstructor.getEmail()); + assertEquals(newRoleName, updatedInstructor.getRole().getRoleName()); + assertEquals(newDisplayName, updatedInstructor.getDisplayName()); + assertEquals(newIsDisplayedToStudent, updatedInstructor.isDisplayedToStudents()); + + ______TS("failure: invalid parameter, original unchanged"); + String originalName = instructor.getName(); + + String invalidLongName = "somelongname".repeat(10); + InstructorCreateRequest requestWithInvalidName = new InstructorCreateRequest( + instructor.getGoogleId(), invalidLongName, instructor.getEmail(), + instructor.getRole().getRoleName(), instructor.getDisplayName(), true); + + assertThrows(InvalidParametersException.class, + () -> usersLogic.updateInstructorCascade(instructor.getCourseId(), requestWithInvalidName)); + assertEquals(originalName, usersLogic.getInstructor(instructor.getId()).getName()); + } + + @Test + public void testUpdateStudentCascade() + throws InvalidParametersException, EntityAlreadyExistsException, EntityDoesNotExistException { + Student student = getTypicalStudent(); + student.setCourse(course); + student.setAccount(account); + Section originalSection = usersLogic.getSectionOrCreate(course.getId(), "section name"); + student.setTeam(usersLogic.getTeamOrCreate(originalSection, "team name")); + + ______TS("success: typical case"); + usersLogic.createStudent(student); + + String newName = "new name"; + String newEmail = "new_stu_email@newmail.com"; + String newComments = "new comments"; + Section newSection = usersLogic.getSectionOrCreate(course.getId(), "new section name"); + Team newTeam = usersLogic.getTeamOrCreate(newSection, "new team name"); + Student studentData = new Student(course, newName, newEmail, newComments, newTeam); + studentData.setId(student.getId()); + + Student updatedStudent = usersLogic.updateStudentCascade(studentData); + + assertEquals(newName, updatedStudent.getName()); + assertEquals(newEmail, updatedStudent.getEmail()); + assertEquals(newComments, updatedStudent.getComments()); + assertEquals(newSection.getId(), updatedStudent.getSection().getId()); + assertEquals(newTeam.getId(), updatedStudent.getTeam().getId()); + + ______TS("failure: invalid parameter, original unchanged"); + String originalName = student.getName(); + + String invalidLongName = "somelongname".repeat(10); + Student studentDataWithInvalidName = + new Student(course, invalidLongName, student.getEmail(), student.getComments(), student.getTeam()); + studentDataWithInvalidName.setId(student.getId()); + + assertThrows(InvalidParametersException.class, + () -> usersLogic.updateStudentCascade(studentDataWithInvalidName)); + assertEquals(originalName, usersLogic.getStudent(student.getId()).getName()); + } + } diff --git a/src/it/java/teammates/it/ui/webapi/FeedbackSessionOpeningRemindersActionIT.java b/src/it/java/teammates/it/ui/webapi/FeedbackSessionOpeningRemindersActionIT.java index 74c3cae648c..cbaba138d06 100644 --- a/src/it/java/teammates/it/ui/webapi/FeedbackSessionOpeningRemindersActionIT.java +++ b/src/it/java/teammates/it/ui/webapi/FeedbackSessionOpeningRemindersActionIT.java @@ -109,9 +109,9 @@ private void testExecute_typicalSuccess1() { // # of email to send = // # emails sent to instructorsToNotify (ie co-owner), 1 + - // # emails sent to students, 4 + + // # emails sent to students, 5 + // # emails sent to instructors, 3 (including instructorsToNotify) - verifySpecifiedTasksAdded(Const.TaskQueue.SEND_EMAIL_QUEUE_NAME, 8); + verifySpecifiedTasksAdded(Const.TaskQueue.SEND_EMAIL_QUEUE_NAME, 9); List tasksAdded = mockTaskQueuer.getTasksAdded(); for (TaskWrapper task : tasksAdded) { diff --git a/src/it/java/teammates/it/ui/webapi/GetSessionResponseStatsActionIT.java b/src/it/java/teammates/it/ui/webapi/GetSessionResponseStatsActionIT.java index f7a2bf24082..837c5d75c08 100644 --- a/src/it/java/teammates/it/ui/webapi/GetSessionResponseStatsActionIT.java +++ b/src/it/java/teammates/it/ui/webapi/GetSessionResponseStatsActionIT.java @@ -53,7 +53,7 @@ protected void testExecute() { JsonResult r = getJsonResult(a); FeedbackSessionStatsData output = (FeedbackSessionStatsData) r.getOutput(); - assertEquals(7, output.getExpectedTotal()); + assertEquals(8, output.getExpectedTotal()); assertEquals(3, output.getSubmittedTotal()); ______TS("fail: instructor accesses stats of non-existent feedback session"); diff --git a/src/it/java/teammates/it/ui/webapi/UpdateFeedbackQuestionActionIT.java b/src/it/java/teammates/it/ui/webapi/UpdateFeedbackQuestionActionIT.java index 289876b0566..a0333478519 100644 --- a/src/it/java/teammates/it/ui/webapi/UpdateFeedbackQuestionActionIT.java +++ b/src/it/java/teammates/it/ui/webapi/UpdateFeedbackQuestionActionIT.java @@ -17,6 +17,7 @@ import teammates.ui.output.FeedbackQuestionData; import teammates.ui.output.NumberOfEntitiesToGiveFeedbackToSetting; import teammates.ui.request.FeedbackQuestionUpdateRequest; +import teammates.ui.request.InvalidHttpRequestBodyException; import teammates.ui.webapi.JsonResult; import teammates.ui.webapi.UpdateFeedbackQuestionAction; @@ -107,6 +108,33 @@ protected void testExecute() throws Exception { assertTrue(typicalQuestion.getShowRecipientNameTo().isEmpty()); } + @Test + protected void testExecute_invalidDetails_originalUnchanged() { + Instructor instructor1ofCourse1 = typicalBundle.instructors.get("instructor1OfCourse1"); + loginAsInstructor(instructor1ofCourse1.getGoogleId()); + + FeedbackQuestion fq1 = typicalBundle.feedbackQuestions.get("qn1InSession1InCourse1"); + String originalDescription = fq1.getDescription(); + + FeedbackQuestionUpdateRequest updateRequest = getTypicalTextQuestionUpdateRequest(); + updateRequest.setQuestionDescription("new question description"); + assertNotEquals(originalDescription, updateRequest.getQuestionDescription()); + + String[] param = new String[] { + Const.ParamsNames.FEEDBACK_QUESTION_ID, fq1.getId().toString(), + }; + + // set invalid question detail + FeedbackTextQuestionDetails ftqd = (FeedbackTextQuestionDetails) updateRequest.getQuestionDetails(); + ftqd.setRecommendedLength(0); + updateRequest.setQuestionDetails(ftqd); + + InvalidHttpRequestBodyException ihrbe = verifyHttpRequestBodyFailure(updateRequest, param); + + assertEquals("[Recommended length must be 1 or greater]", ihrbe.getMessage()); + assertEquals(originalDescription, logic.getFeedbackQuestion(fq1.getId()).getDescription()); + } + @Override @Test protected void testAccessControl() throws Exception { diff --git a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java index fd7742b3a73..d0f19ebae66 100644 --- a/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java +++ b/src/main/java/teammates/sqllogic/core/AccountRequestsLogic.java @@ -102,7 +102,7 @@ public AccountRequest resetAccountRequest(String email, String institute) } accountRequest.setRegisteredAt(null); - return accountRequestDb.updateAccountRequest(accountRequest); + return accountRequest; } /** diff --git a/src/main/java/teammates/sqllogic/core/AccountsLogic.java b/src/main/java/teammates/sqllogic/core/AccountsLogic.java index 74bc4af732b..843b12dc887 100644 --- a/src/main/java/teammates/sqllogic/core/AccountsLogic.java +++ b/src/main/java/teammates/sqllogic/core/AccountsLogic.java @@ -208,7 +208,6 @@ public Instructor joinCourseForInstructor(String key, String googleId) Student student = usersLogic.getStudentForEmail(instructor.getCourseId(), instructor.getEmail()); if (student != null) { student.setAccount(account); - usersLogic.updateStudentCascade(student); } return instructor; diff --git a/src/main/java/teammates/sqllogic/core/CoursesLogic.java b/src/main/java/teammates/sqllogic/core/CoursesLogic.java index 5153c03baba..3175e0f0c06 100644 --- a/src/main/java/teammates/sqllogic/core/CoursesLogic.java +++ b/src/main/java/teammates/sqllogic/core/CoursesLogic.java @@ -177,14 +177,13 @@ public Course updateCourse(String courseId, String name, String timezone) if (course == null) { throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + Course.class); } - course.setName(name); - course.setTimeZone(timezone); + Course courseCopy = course.getCopy(); // prevent auto persistence + courseCopy.setName(name); + courseCopy.setTimeZone(timezone); - if (!course.isValid()) { - throw new InvalidParametersException(course.getInvalidityInfo()); - } + coursesDb.updateCourse(courseCopy); - return course; + return courseCopy; } /** diff --git a/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java b/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java index 208f0efab4b..eb953035d76 100644 --- a/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java +++ b/src/main/java/teammates/sqllogic/core/FeedbackQuestionsLogic.java @@ -190,42 +190,45 @@ public FeedbackQuestion updateFeedbackQuestionCascade(UUID questionId, FeedbackQ previousQuestionsInSession = getFeedbackQuestionsForSession(question.getFeedbackSession()); } - // update question - question.setQuestionNumber(updateRequest.getQuestionNumber()); - question.setDescription(updateRequest.getQuestionDescription()); - question.setQuestionDetails(updateRequest.getQuestionDetails()); - question.setGiverType(updateRequest.getGiverType()); - question.setRecipientType(updateRequest.getRecipientType()); - question.setNumOfEntitiesToGiveFeedbackTo(updateRequest.getNumberOfEntitiesToGiveFeedbackTo()); - question.setShowResponsesTo(updateRequest.getShowResponsesTo()); - question.setShowGiverNameTo(updateRequest.getShowGiverNameTo()); - question.setShowRecipientNameTo(updateRequest.getShowRecipientNameTo()); + FeedbackQuestion questionCopy = question.getCopy(); // prevent auto persistence + questionCopy.setQuestionNumber(updateRequest.getQuestionNumber()); + questionCopy.setDescription(updateRequest.getQuestionDescription()); + questionCopy.setQuestionDetails(updateRequest.getQuestionDetails()); + questionCopy.setGiverType(updateRequest.getGiverType()); + questionCopy.setRecipientType(updateRequest.getRecipientType()); + questionCopy.setNumOfEntitiesToGiveFeedbackTo(updateRequest.getNumberOfEntitiesToGiveFeedbackTo()); + questionCopy.setShowResponsesTo(updateRequest.getShowResponsesTo()); + questionCopy.setShowGiverNameTo(updateRequest.getShowGiverNameTo()); + questionCopy.setShowRecipientNameTo(updateRequest.getShowRecipientNameTo()); // validate questions (giver & recipient) - String err = question.getQuestionDetailsCopy().validateGiverRecipientVisibility(question); + String err = questionCopy.getQuestionDetailsCopy().validateGiverRecipientVisibility(questionCopy); if (!err.isEmpty()) { throw new InvalidParametersException(err); } // validate questions (question details) - FeedbackQuestionDetails questionDetails = question.getQuestionDetailsCopy(); + FeedbackQuestionDetails questionDetails = questionCopy.getQuestionDetailsCopy(); List questionDetailsErrors = questionDetails.validateQuestionDetails(); if (!questionDetailsErrors.isEmpty()) { throw new InvalidParametersException(questionDetailsErrors.toString()); } + // update question + fqDb.updateFeedbackQuestion(questionCopy); + if (oldQuestionNumber != newQuestionNumber) { // shift other feedback questions (generate an empty "slot") adjustQuestionNumbers(oldQuestionNumber, newQuestionNumber, previousQuestionsInSession); } // adjust responses - if (question.areResponseDeletionsRequiredForChanges(updateRequest.getGiverType(), + if (questionCopy.areResponseDeletionsRequiredForChanges(updateRequest.getGiverType(), updateRequest.getRecipientType(), updateRequest.getQuestionDetails())) { - frLogic.deleteFeedbackResponsesForQuestionCascade(question.getId()); + frLogic.deleteFeedbackResponsesForQuestionCascade(questionCopy.getId()); } - return question; + return questionCopy; } /** diff --git a/src/main/java/teammates/sqllogic/core/NotificationsLogic.java b/src/main/java/teammates/sqllogic/core/NotificationsLogic.java index 3b8b5df8750..3c4fea1a0eb 100644 --- a/src/main/java/teammates/sqllogic/core/NotificationsLogic.java +++ b/src/main/java/teammates/sqllogic/core/NotificationsLogic.java @@ -78,18 +78,17 @@ public Notification updateNotification(UUID notificationId, Instant startTime, I throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + Notification.class); } - notification.setStartTime(startTime); - notification.setEndTime(endTime); - notification.setStyle(style); - notification.setTargetUser(targetUser); - notification.setTitle(title); - notification.setMessage(message); - - if (!notification.isValid()) { - throw new InvalidParametersException(notification.getInvalidityInfo()); - } + Notification notificationCopy = notification.getCopy(); // prevent auto persistence + notificationCopy.setStartTime(startTime); + notificationCopy.setEndTime(endTime); + notificationCopy.setStyle(style); + notificationCopy.setTargetUser(targetUser); + notificationCopy.setTitle(title); + notificationCopy.setMessage(message); + + notificationsDb.updateNotification(notificationCopy); - return notification; + return notificationCopy; } /** diff --git a/src/main/java/teammates/sqllogic/core/UsersLogic.java b/src/main/java/teammates/sqllogic/core/UsersLogic.java index e0486616a69..dfb2f657ed4 100644 --- a/src/main/java/teammates/sqllogic/core/UsersLogic.java +++ b/src/main/java/teammates/sqllogic/core/UsersLogic.java @@ -157,23 +157,26 @@ public Instructor updateInstructorCascade(String courseId, InstructorCreateReque newDisplayName = Const.DEFAULT_DISPLAY_NAME_FOR_INSTRUCTOR; } - instructor.setName(SanitizationHelper.sanitizeName(instructorRequest.getName())); - instructor.setEmail(SanitizationHelper.sanitizeEmail(instructorRequest.getEmail())); - instructor.setRole(InstructorPermissionRole.getEnum(instructorRequest.getRoleName())); - instructor.setPrivileges(new InstructorPrivileges(instructorRequest.getRoleName())); - instructor.setDisplayName(SanitizationHelper.sanitizeName(newDisplayName)); - instructor.setDisplayedToStudents(instructorRequest.getIsDisplayedToStudent()); + Instructor instructorCopy = instructor.getCopy(); + instructorCopy.setName(SanitizationHelper.sanitizeName(instructorRequest.getName())); + instructorCopy.setEmail(SanitizationHelper.sanitizeEmail(instructorRequest.getEmail())); + instructorCopy.setRole(InstructorPermissionRole.getEnum(instructorRequest.getRoleName())); + instructorCopy.setPrivileges(new InstructorPrivileges(instructorRequest.getRoleName())); + instructorCopy.setDisplayName(SanitizationHelper.sanitizeName(newDisplayName)); + instructorCopy.setDisplayedToStudents(instructorRequest.getIsDisplayedToStudent()); - String newEmail = instructor.getEmail(); + String newEmail = instructorCopy.getEmail(); if (!originalEmail.equals(newEmail)) { needsCascade = true; } - if (!instructor.isValid()) { - throw new InvalidParametersException(instructor.getInvalidityInfo()); + if (!instructorCopy.isValid()) { + throw new InvalidParametersException(instructorCopy.getInvalidityInfo()); } + usersDb.updateUser(instructorCopy); + if (needsCascade) { // cascade responses List responsesFromUser = @@ -199,7 +202,7 @@ public Instructor updateInstructorCascade(String courseId, InstructorCreateReque feedbackResponseCommentsLogic.updateFeedbackResponseCommentsEmails(courseId, originalEmail, newEmail); } - return instructor; + return instructorCopy; } /** diff --git a/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java b/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java index f78f0f3026b..8497fb7a317 100644 --- a/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java +++ b/src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java @@ -104,7 +104,7 @@ public List getAccountRequests(Instant startTime, Instant endTim } /** - * Updates or creates (if does not exist) the AccountRequest in the database. + * Updates the AccountRequest in the database. */ public AccountRequest updateAccountRequest(AccountRequest accountRequest) throws InvalidParametersException, EntityDoesNotExistException { @@ -115,8 +115,7 @@ public AccountRequest updateAccountRequest(AccountRequest accountRequest) } if (getAccountRequest(accountRequest.getEmail(), accountRequest.getInstitute()) == null) { - throw new EntityDoesNotExistException( - String.format(ERROR_UPDATE_NON_EXISTENT, accountRequest.toString())); + throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + AccountRequest.class); } merge(accountRequest); diff --git a/src/main/java/teammates/storage/sqlapi/CoursesDb.java b/src/main/java/teammates/storage/sqlapi/CoursesDb.java index 710eeb04bd9..7e4cb17f601 100644 --- a/src/main/java/teammates/storage/sqlapi/CoursesDb.java +++ b/src/main/java/teammates/storage/sqlapi/CoursesDb.java @@ -76,7 +76,7 @@ public Course updateCourse(Course course) throws InvalidParametersException, Ent } if (getCourse(course.getId()) == null) { - throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT); + throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + Course.class); } return merge(course); diff --git a/src/main/java/teammates/storage/sqlapi/DeadlineExtensionsDb.java b/src/main/java/teammates/storage/sqlapi/DeadlineExtensionsDb.java index c1042ea2abf..09b7722319e 100644 --- a/src/main/java/teammates/storage/sqlapi/DeadlineExtensionsDb.java +++ b/src/main/java/teammates/storage/sqlapi/DeadlineExtensionsDb.java @@ -105,7 +105,7 @@ public DeadlineExtension updateDeadlineExtension(DeadlineExtension deadlineExten } if (getDeadlineExtension(deadlineExtension.getId()) == null) { - throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT); + throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + DeadlineExtension.class); } return merge(deadlineExtension); diff --git a/src/main/java/teammates/storage/sqlapi/FeedbackQuestionsDb.java b/src/main/java/teammates/storage/sqlapi/FeedbackQuestionsDb.java index 6d1fadabb77..51e2b46f794 100644 --- a/src/main/java/teammates/storage/sqlapi/FeedbackQuestionsDb.java +++ b/src/main/java/teammates/storage/sqlapi/FeedbackQuestionsDb.java @@ -1,9 +1,13 @@ package teammates.storage.sqlapi; +import static teammates.common.util.Const.ERROR_UPDATE_NON_EXISTENT; + import java.util.List; import java.util.UUID; import teammates.common.datatransfer.FeedbackParticipantType; +import teammates.common.exception.EntityDoesNotExistException; +import teammates.common.exception.InvalidParametersException; import teammates.common.util.HibernateUtil; import teammates.storage.sqlentity.Course; import teammates.storage.sqlentity.FeedbackQuestion; @@ -115,4 +119,23 @@ public boolean hasFeedbackQuestionsForGiverType( cb.equal(root.get("giverType"), giverType))); return !HibernateUtil.createQuery(cq).getResultList().isEmpty(); } + + /** + * Updates a feedback question. + */ + public FeedbackQuestion updateFeedbackQuestion(FeedbackQuestion feedbackQuestion) + throws InvalidParametersException, EntityDoesNotExistException { + assert feedbackQuestion != null; + + if (!feedbackQuestion.isValid()) { + throw new InvalidParametersException(feedbackQuestion.getInvalidityInfo()); + } + + if (feedbackQuestion.getId() == null) { + throw new EntityDoesNotExistException(ERROR_UPDATE_NON_EXISTENT + FeedbackQuestion.class); + } + + return merge(feedbackQuestion); + } + } diff --git a/src/main/java/teammates/storage/sqlapi/NotificationsDb.java b/src/main/java/teammates/storage/sqlapi/NotificationsDb.java index 85e00e48b47..33630ca344a 100644 --- a/src/main/java/teammates/storage/sqlapi/NotificationsDb.java +++ b/src/main/java/teammates/storage/sqlapi/NotificationsDb.java @@ -6,6 +6,7 @@ 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.sqlentity.Notification; @@ -98,4 +99,23 @@ public List getActiveNotificationsByTargetUser(NotificationTargetU TypedQuery query = HibernateUtil.createQuery(cq); return query.getResultList(); } + + /** + * Updates a notification. + * + *

Preconditions:

+ * * Notification id exists in the database. + */ + public Notification updateNotification(Notification notification) + throws InvalidParametersException, EntityDoesNotExistException { + assert notification != null; + + if (!notification.isValid()) { + throw new InvalidParametersException(notification.getInvalidityInfo()); + } + + merge(notification); + return notification; + } + } diff --git a/src/main/java/teammates/storage/sqlentity/AccountRequest.java b/src/main/java/teammates/storage/sqlentity/AccountRequest.java index 2389fbf352d..e5fcffef073 100644 --- a/src/main/java/teammates/storage/sqlentity/AccountRequest.java +++ b/src/main/java/teammates/storage/sqlentity/AccountRequest.java @@ -60,6 +60,21 @@ public AccountRequest(String email, String name, String institute) { this.setRegisteredAt(null); } + /** + * Gets a copy of the AccountRequest. + */ + public AccountRequest getCopy() { + AccountRequest copy = new AccountRequest(email, name, institute); + + copy.setId(this.getId()); + copy.setRegistrationKey(this.getRegistrationKey()); + copy.setRegisteredAt(this.getRegisteredAt()); + copy.setCreatedAt(this.getCreatedAt()); + copy.setUpdatedAt(this.getUpdatedAt()); + + return copy; + } + @Override public List getInvalidityInfo() { List errors = new ArrayList<>(); diff --git a/src/main/java/teammates/storage/sqlentity/Course.java b/src/main/java/teammates/storage/sqlentity/Course.java index 2b0fac83767..6365457330d 100644 --- a/src/main/java/teammates/storage/sqlentity/Course.java +++ b/src/main/java/teammates/storage/sqlentity/Course.java @@ -59,6 +59,21 @@ public Course(String id, String name, String timeZone, String institute) { this.setInstitute(institute); } + /** + * Gets a copy of the Course. + */ + public Course getCopy() { + Course copy = new Course(id, name, timeZone, institute); + + copy.setFeedbackSessions(feedbackSessions); + copy.setSections(sections); + copy.setCreatedAt(getCreatedAt()); + copy.setUpdatedAt(updatedAt); + copy.setDeletedAt(deletedAt); + + return copy; + } + @Override public List getInvalidityInfo() { List errors = new ArrayList<>(); diff --git a/src/main/java/teammates/storage/sqlentity/FeedbackQuestion.java b/src/main/java/teammates/storage/sqlentity/FeedbackQuestion.java index f672ea6b3d0..8398fb9cba3 100644 --- a/src/main/java/teammates/storage/sqlentity/FeedbackQuestion.java +++ b/src/main/java/teammates/storage/sqlentity/FeedbackQuestion.java @@ -107,6 +107,19 @@ public FeedbackQuestion( this.setShowRecipientNameTo(showRecipientNameTo); } + /** + * Gets a copy of the FeedbackQuestion. + */ + public FeedbackQuestion getCopy() { + FeedbackQuestion copy = makeDeepCopy(this.getFeedbackSession()); + + copy.setId(this.getId()); + copy.setCreatedAt(this.getCreatedAt()); + copy.setUpdatedAt(this.getUpdatedAt()); + + return copy; + } + /** * Gets a copy of the question details of the feedback question. */ diff --git a/src/main/java/teammates/storage/sqlentity/FeedbackSession.java b/src/main/java/teammates/storage/sqlentity/FeedbackSession.java index b5633033d92..e05a36b8d1a 100644 --- a/src/main/java/teammates/storage/sqlentity/FeedbackSession.java +++ b/src/main/java/teammates/storage/sqlentity/FeedbackSession.java @@ -144,13 +144,17 @@ public FeedbackSession getCopyForUser(String userEmail) { return copy; } - private FeedbackSession getCopy() { + /** + * Gets a copy of the FeedbackSession. + */ + public FeedbackSession getCopy() { FeedbackSession fs = new FeedbackSession( name, course, creatorEmail, instructions, startTime, endTime, sessionVisibleFromTime, resultsVisibleFromTime, gracePeriod, isOpeningEmailEnabled, isClosingEmailEnabled, isPublishedEmailEnabled ); + fs.setId(getId()); fs.setCreatedAt(getCreatedAt()); fs.setUpdatedAt(getUpdatedAt()); fs.setDeletedAt(getDeletedAt()); diff --git a/src/main/java/teammates/storage/sqlentity/Instructor.java b/src/main/java/teammates/storage/sqlentity/Instructor.java index 6848d80ac4b..7dbf737e224 100644 --- a/src/main/java/teammates/storage/sqlentity/Instructor.java +++ b/src/main/java/teammates/storage/sqlentity/Instructor.java @@ -52,6 +52,23 @@ public Instructor(Course course, String name, String email, boolean isDisplayedT this.setPrivileges(privileges); } + /** + * Gets a copy of the instructor. + */ + public Instructor getCopy() { + Instructor copy = new Instructor(getCourse(), getName(), getEmail(), + isDisplayedToStudents, displayName, role, privileges); + + copy.setId(getId()); + copy.setAccount(getAccount()); + copy.setTeam(getTeam()); + copy.setRegKey(getRegKey()); + copy.setCreatedAt(getCreatedAt()); + copy.setUpdatedAt(getUpdatedAt()); + + return copy; + } + public boolean isDisplayedToStudents() { return isDisplayedToStudents; } diff --git a/src/main/java/teammates/storage/sqlentity/Notification.java b/src/main/java/teammates/storage/sqlentity/Notification.java index 77436146757..43a812ae824 100644 --- a/src/main/java/teammates/storage/sqlentity/Notification.java +++ b/src/main/java/teammates/storage/sqlentity/Notification.java @@ -74,6 +74,22 @@ protected Notification() { // required by Hibernate } + /** + * Gets the copy of the notification. + */ + public Notification getCopy() { + Notification copy = new Notification(startTime, endTime, style, targetUser, title, message); + + copy.setId(this.id); + copy.setCreatedAt(this.getCreatedAt()); + copy.setUpdatedAt(this.updatedAt); + if (this.isShown()) { + copy.setShown(); + } + + return copy; + } + @Override public List getInvalidityInfo() { List errors = new ArrayList<>(); diff --git a/src/main/java/teammates/ui/webapi/CreateAccountAction.java b/src/main/java/teammates/ui/webapi/CreateAccountAction.java index 63bde3a7f9d..1a8b66a1f31 100644 --- a/src/main/java/teammates/ui/webapi/CreateAccountAction.java +++ b/src/main/java/teammates/ui/webapi/CreateAccountAction.java @@ -117,12 +117,13 @@ public JsonResult execute() throws InvalidHttpRequestBodyException, InvalidOpera private AccountRequest setAccountRequestAsRegistered(AccountRequest accountRequest, String instructorEmail, String instructorInstitution) throws InvalidParametersException, EntityDoesNotExistException { - accountRequest.setEmail(instructorEmail); - accountRequest.setInstitute(instructorInstitution); - accountRequest.setRegisteredAt(Instant.now()); + AccountRequest accountRequestCopy = accountRequest.getCopy(); // prevent auto persistence + accountRequestCopy.setEmail(instructorEmail); + accountRequestCopy.setInstitute(instructorInstitution); + accountRequestCopy.setRegisteredAt(Instant.now()); - sqlLogic.updateAccountRequest(accountRequest); - return accountRequest; + sqlLogic.updateAccountRequest(accountRequestCopy); + return accountRequestCopy; } private static String getDateString(Instant instant) { diff --git a/src/main/java/teammates/ui/webapi/UpdateFeedbackSessionAction.java b/src/main/java/teammates/ui/webapi/UpdateFeedbackSessionAction.java index e4352433ec8..1bc96dd8cf5 100644 --- a/src/main/java/teammates/ui/webapi/UpdateFeedbackSessionAction.java +++ b/src/main/java/teammates/ui/webapi/UpdateFeedbackSessionAction.java @@ -147,6 +147,7 @@ public JsonResult execute() throws InvalidHttpRequestBodyException { .collect(Collectors.toMap(Map.Entry::getKey, entry -> TimeHelper.getMidnightAdjustedInstantBasedOnZone( entry.getValue(), timeZone, true))); + feedbackSession = feedbackSession.getCopy(); // prevent auto persistence feedbackSession.setInstructions(updateRequest.getInstructions()); feedbackSession.setStartTime(startTime); feedbackSession.setEndTime(endTime); diff --git a/src/test/java/teammates/sqllogic/core/CoursesLogicTest.java b/src/test/java/teammates/sqllogic/core/CoursesLogicTest.java index f6e1b478915..cebe9fe8a39 100644 --- a/src/test/java/teammates/sqllogic/core/CoursesLogicTest.java +++ b/src/test/java/teammates/sqllogic/core/CoursesLogicTest.java @@ -244,8 +244,14 @@ public void testUpdateCourse_throwInvalidParametersException() throws InvalidParametersException, EntityDoesNotExistException { Course course = getTypicalCourse(); String courseId = course.getId(); + String originalName = course.getName(); + + Course courseWithEmptyName = getTypicalCourse(); + courseWithEmptyName.setName(""); when(coursesDb.getCourse(courseId)).thenReturn(course); + when(coursesDb.updateCourse(course)).thenThrow( + new InvalidParametersException(courseWithEmptyName.getInvalidityInfo())); InvalidParametersException ex = assertThrows(InvalidParametersException.class, () -> coursesLogic.updateCourse(courseId, "", "Asia/Singapore")); @@ -255,6 +261,7 @@ public void testUpdateCourse_throwInvalidParametersException() + " It should not be empty."; assertEquals(expectedMessage, ex.getMessage()); + assertEquals(originalName, course.getName()); } @Test diff --git a/src/test/java/teammates/sqlui/webapi/UpdateFeedbackSessionActionTest.java b/src/test/java/teammates/sqlui/webapi/UpdateFeedbackSessionActionTest.java index 388e78bbce0..1fcf9441202 100644 --- a/src/test/java/teammates/sqlui/webapi/UpdateFeedbackSessionActionTest.java +++ b/src/test/java/teammates/sqlui/webapi/UpdateFeedbackSessionActionTest.java @@ -138,6 +138,35 @@ void testExecute_createDeadlineExtensionEndTime_success() verify(mockLogic).createDeadlineExtension(argThat((DeadlineExtension de) -> de.getEndTime().equals(nearestHour))); } + @Test + void testExecute_invalidParameters_originalUnchanged() + throws InvalidParametersException, EntityDoesNotExistException, EntityAlreadyExistsException { + loginAsInstructor(instructor.getGoogleId()); + FeedbackSession originalFeedbackSession = generateSession1InCourse(course, instructor); + + String[] param = new String[] { + Const.ParamsNames.COURSE_ID, originalFeedbackSession.getCourse().getId(), + Const.ParamsNames.FEEDBACK_SESSION_NAME, originalFeedbackSession.getName(), + Const.ParamsNames.NOTIFY_ABOUT_DEADLINES, String.valueOf(false), + }; + + String originalInstructions = originalFeedbackSession.getInstructions(); + + when(mockLogic.getFeedbackSession(any(), any())).thenReturn(originalFeedbackSession); + + FeedbackSession invalidFeedbackSession = generateSession1InCourse(course, instructor); + invalidFeedbackSession.setInstructions(null); + + when(mockLogic.updateFeedbackSession(originalFeedbackSession)) + .thenThrow(new InvalidParametersException(invalidFeedbackSession.getInvalidityInfo())); + + FeedbackSessionUpdateRequest updateRequest = + getTypicalFeedbackSessionUpdateRequest(invalidFeedbackSession); + + verifyHttpRequestBodyFailure(updateRequest, param); + assertEquals(originalInstructions, originalFeedbackSession.getInstructions()); + } + private FeedbackSessionUpdateRequest getTypicalFeedbackSessionUpdateRequest(FeedbackSession feedbackSession) { FeedbackSessionUpdateRequest updateRequest = new FeedbackSessionUpdateRequest(); updateRequest.setInstructions("instructions"); diff --git a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java index d1489f99c73..325009cf13f 100644 --- a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java @@ -1,8 +1,11 @@ package teammates.storage.sqlapi; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; import java.time.Instant; import java.util.UUID; @@ -15,6 +18,7 @@ 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.sqlentity.Notification; @@ -25,13 +29,14 @@ */ public class NotificationsDbTest extends BaseTestCase { - private NotificationsDb notificationsDb = NotificationsDb.inst(); + private NotificationsDb notificationsDb; private MockedStatic mockHibernateUtil; @BeforeMethod public void setUpMethod() { mockHibernateUtil = mockStatic(HibernateUtil.class); + notificationsDb = spy(NotificationsDb.class); } @AfterMethod @@ -117,4 +122,26 @@ public void testDeleteNotification_entityDoesNotExists_success() { mockHibernateUtil.verify(() -> HibernateUtil.remove(any()), never()); } + @Test + public void testUpdateNotification_entityExists_success() + throws EntityDoesNotExistException, InvalidParametersException { + Notification notification = getTypicalNotificationWithId(); + doReturn(notification).when(notificationsDb).getNotification(notification.getId()); + + notificationsDb.updateNotification(notification); + + mockHibernateUtil.verify(() -> HibernateUtil.merge(notification), times(1)); + } + + @Test + public void testUpdateNotification_invalidParameters_throwsInvalidParametersException() + throws EntityDoesNotExistException, InvalidParametersException { + Notification notification = getTypicalNotificationWithId(); + notification.setTitle(""); + + assertThrows(InvalidParametersException.class, () -> notificationsDb.updateNotification(notification)); + + mockHibernateUtil.verify(() -> HibernateUtil.merge(notification), never()); + } + } From 49ee6422a2b6a9b3b95f89d3b22a3381003f4579 Mon Sep 17 00:00:00 2001 From: marquestye Date: Tue, 27 Feb 2024 23:38:35 +0800 Subject: [PATCH 2/3] Move tests from NotificationsLogicTest to NotificationsDbTest --- .../sqllogic/core/NotificationsLogicTest.java | 46 ------------------- .../storage/sqlapi/NotificationsDbTest.java | 34 ++++++++++++-- 2 files changed, 31 insertions(+), 49 deletions(-) diff --git a/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java b/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java index f54c1899ac6..d966cd9cfb9 100644 --- a/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java +++ b/src/test/java/teammates/sqllogic/core/NotificationsLogicTest.java @@ -63,52 +63,6 @@ public void testUpdateNotification_entityAlreadyExists_success() assertEquals(newMessage, updatedNotification.getMessage()); } - @Test - public void testUpdateNotification_invalidNonNullParameter_endTimeBeforeStartTime() { - Notification notification = getTypicalNotificationWithId(); - UUID notificationId = notification.getId(); - - when(notificationsDb.getNotification(notificationId)).thenReturn(notification); - - InvalidParametersException ex = assertThrows(InvalidParametersException.class, - () -> notificationsLogic.updateNotification(notificationId, Instant.parse("2011-01-01T00:00:01Z"), - Instant.parse("2011-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "A deprecation note", "

Deprecation happens in three minutes

")); - - assertEquals("The time when the notification will expire for this notification cannot be earlier than " - + "the time when the notification will be visible.", ex.getMessage()); - } - - @Test - public void testUpdateNotification_invalidNonNullParameter_emptyTitle() { - Notification notification = getTypicalNotificationWithId(); - UUID notificationId = notification.getId(); - - when(notificationsDb.getNotification(notificationId)).thenReturn(notification); - - InvalidParametersException ex = assertThrows(InvalidParametersException.class, - () -> notificationsLogic.updateNotification(notificationId, Instant.parse("2011-01-01T00:00:00Z"), - Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "", "

Deprecation happens in three minutes

")); - - assertEquals("The field 'notification title' is empty.", ex.getMessage()); - } - - @Test - public void testUpdateNotification_invalidNonNullParameter_emptyMessage() { - Notification notification = getTypicalNotificationWithId(); - UUID notificationId = notification.getId(); - - when(notificationsDb.getNotification(notificationId)).thenReturn(notification); - - InvalidParametersException ex = assertThrows(InvalidParametersException.class, - () -> notificationsLogic.updateNotification(notificationId, Instant.parse("2011-01-01T00:00:00Z"), - Instant.parse("2099-01-01T00:00:00Z"), NotificationStyle.DANGER, NotificationTargetUser.GENERAL, - "An updated deprecation note", "")); - - assertEquals("The field 'notification message' is empty.", ex.getMessage()); - } - @Test public void testUpdateNotification_entityDoesNotExist() { Notification notification = getTypicalNotificationWithId(); diff --git a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java index 325009cf13f..e3ab330ee59 100644 --- a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java @@ -6,6 +6,7 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; +import static org.mockito.Mockito.when; import java.time.Instant; import java.util.UUID; @@ -134,13 +135,40 @@ public void testUpdateNotification_entityExists_success() } @Test - public void testUpdateNotification_invalidParameters_throwsInvalidParametersException() - throws EntityDoesNotExistException, InvalidParametersException { + public void testUpdateNotification_invalidNonNullParameter_endTimeBeforeStartTime() { + Notification notification = getTypicalNotificationWithId(); + notification.setEndTime(Instant.parse("2010-01-01T00:00:00Z")); + assertEquals(notification.getEndTime().isBefore(notification.getStartTime()), true); + + InvalidParametersException ex = assertThrows(InvalidParametersException.class, + () -> notificationsDb.updateNotification(notification)); + + assertEquals("The time when the notification will expire for this notification cannot be earlier than " + + "the time when the notification will be visible.", ex.getMessage()); + mockHibernateUtil.verify(() -> HibernateUtil.merge(notification), never()); + } + + @Test + public void testUpdateNotification_invalidNonNullParameter_emptyTitle() { Notification notification = getTypicalNotificationWithId(); notification.setTitle(""); - assertThrows(InvalidParametersException.class, () -> notificationsDb.updateNotification(notification)); + InvalidParametersException ex = assertThrows(InvalidParametersException.class, + () -> notificationsDb.updateNotification(notification)); + + assertEquals("The field 'notification title' is empty.", ex.getMessage()); + mockHibernateUtil.verify(() -> HibernateUtil.merge(notification), never()); + } + + @Test + public void testUpdateNotification_invalidNonNullParameter_emptyMessage() { + Notification notification = getTypicalNotificationWithId(); + notification.setMessage(""); + + InvalidParametersException ex = assertThrows(InvalidParametersException.class, + () -> notificationsDb.updateNotification(notification)); + assertEquals("The field 'notification message' is empty.", ex.getMessage()); mockHibernateUtil.verify(() -> HibernateUtil.merge(notification), never()); } From f6fc90a54e78a2b682db683c7882da1a6091a3e6 Mon Sep 17 00:00:00 2001 From: marquestye Date: Tue, 27 Feb 2024 23:55:08 +0800 Subject: [PATCH 3/3] Fix lint --- src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java index e3ab330ee59..1896b00460e 100644 --- a/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java +++ b/src/test/java/teammates/storage/sqlapi/NotificationsDbTest.java @@ -6,7 +6,6 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; -import static org.mockito.Mockito.when; import java.time.Instant; import java.util.UUID;