[#12779] Fix auto persistence during database updates

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",
+                "<p>Deprecation happens in three minutes</p>");
+        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,
+                        "<p>Deprecation happens in three minutes</p>"
+                )
+        );
+
+        Notification actual = notificationsLogic.getNotification(notif.getId());
+        assert actual.getTitle().equals(notif.getTitle()) : actual.getTitle();
+    }
+
 }

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());
+    }
+
 }

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 {

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;
     }
 
     /**

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;

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;
     }
 
     /**

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<String> 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;
     }
 
     /**

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;
     }
 
     /**

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<FeedbackResponse> responsesFromUser =
@@ -199,7 +202,7 @@ public Instructor updateInstructorCascade(String courseId, InstructorCreateReque
             feedbackResponseCommentsLogic.updateFeedbackResponseCommentsEmails(courseId, originalEmail, newEmail);
         }
 
-        return instructor;
+        return instructorCopy;
     }
 
     /**

src/main/java/teammates/storage/sqlapi/AccountRequestsDb.java

@@ -106,7 +106,7 @@ public List<AccountRequest> 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 {
@@ -117,8 +117,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);

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);

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);