Assessment: Keep long feedback after saving an assessment (#10090)

ls1intum · Jan 5, 2025 · ec1f0df · ec1f0df
1 parent e12d70e
commit ec1f0df
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 12 deletions.
diff --git a/src/main/java/de/tum/cit/aet/artemis/assessment/repository/LongFeedbackTextRepository.java b/src/main/java/de/tum/cit/aet/artemis/assessment/repository/LongFeedbackTextRepository.java
@@ -52,6 +52,10 @@ public interface LongFeedbackTextRepository extends ArtemisJpaRepository<LongFee
             """)
     void deleteByFeedbackIds(@Param("feedbackIds") List<Long> feedbackIds);
 
+    @Modifying
+    @Transactional
+    void deleteByFeedbackId(final Long feedbackId);
+
     default LongFeedbackText findByFeedbackIdWithFeedbackAndResultAndParticipationElseThrow(final Long feedbackId) {
         return getValueElseThrow(findWithFeedbackAndResultAndParticipationByFeedbackId(feedbackId), feedbackId);
     }

diff --git a/src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java b/src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java
@@ -522,23 +522,26 @@ private void handleFeedbackPersistence(Feedback feedback, Result result, Map<Lon
         // Temporarily detach feedback from the parent result to avoid Hibernate issues
         feedback.setResult(null);
 
-        // Clear the long feedback if it exists in the map
+        // Connect old long feedback text to the feedback before saving, otherwise it would be deleted
         if (feedback.getId() != null && feedback.getHasLongFeedbackText()) {
-            LongFeedbackText longFeedback = longFeedbackTextMap.get(feedback.getId());
-            if (longFeedback != null) {
-                feedback.clearLongFeedback();
+
+            // If the long feedback is not empty, it means that changes have been made on the client, so we do not want
+            // to override the new long feedback with its previous version
+            if (feedback.getLongFeedback().isPresent()) {
+                // Delete the old long feedback so we don't get a duplicate key error
+                longFeedbackTextRepository.deleteByFeedbackId(feedback.getId());
+            }
+            else {
+                LongFeedbackText longFeedback = longFeedbackTextMap.get(feedback.getId());
+                feedback.setLongFeedbackText(Set.of(longFeedback));
             }
         }
 
         // Persist the feedback entity without the parent association
         feedback = feedbackRepository.saveAndFlush(feedback);
 
-        // Restore associations to the result and long feedback after persistence
+        // Restore associations to the result
         feedback.setResult(result);
-        LongFeedbackText longFeedback = longFeedbackTextMap.get(feedback.getId());
-        if (longFeedback != null) {
-            feedback.setDetailText(longFeedback.getText());
-        }
     }
 
     @NotNull

diff --git a/src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java b/src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java
@@ -90,8 +90,6 @@ private Result saveManualAssessment(Result result, User assessor) {
      * @return the new saved result
      */
     public Result saveAndSubmitManualAssessment(StudentParticipation participation, Result newManualResult, Result existingManualResult, User assessor, boolean submit) {
-        // long feedback text is deleted as it otherwise causes duplicate entries errors and will be saved again with {@link resultRepository.save}
-        resultService.deleteLongFeedback(newManualResult.getFeedbacks(), newManualResult);
         // make sure that the submission cannot be manipulated on the client side
         var submission = (ProgrammingSubmission) existingManualResult.getSubmission();
         ProgrammingExercise exercise = (ProgrammingExercise) participation.getExercise();

diff --git a/...ava/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationIndependentTest.java b/...ava/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationIndependentTest.java
@@ -3,6 +3,7 @@
 import org.springframework.beans.factory.annotation.Autowired;
 
 import de.tum.cit.aet.artemis.assessment.repository.ComplaintRepository;
+import de.tum.cit.aet.artemis.assessment.repository.LongFeedbackTextRepository;
 import de.tum.cit.aet.artemis.assessment.util.ComplaintUtilService;
 import de.tum.cit.aet.artemis.core.test_repository.UserTestRepository;
 import de.tum.cit.aet.artemis.core.user.util.UserUtilService;
@@ -94,6 +95,9 @@ public abstract class AbstractProgrammingIntegrationIndependentTest extends Abst
     @Autowired
     protected UserTestRepository userRepository;
 
+    @Autowired
+    protected LongFeedbackTextRepository longFeedbackTextRepository;
+
     // Services
     @Autowired
     protected AuxiliaryRepositoryService auxiliaryRepositoryService;

diff --git a/src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingAssessmentIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingAssessmentIntegrationTest.java
@@ -19,6 +19,8 @@
 import org.eclipse.jgit.lib.ObjectId;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.ArgumentMatchers;
 import org.springframework.http.HttpStatus;
 import org.springframework.security.test.context.support.WithMockUser;
@@ -526,7 +528,6 @@ void updateManualProgrammingExerciseResult_newResult() throws Exception {
     @Test
     @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
     void updateManualProgrammingExerciseResult_addFeedbackAfterManualLongFeedback() throws Exception {
-
         List<Feedback> feedbacks = new ArrayList<>();
         var manualLongFeedback = new Feedback().credits(1.00).type(FeedbackType.MANUAL_UNREFERENCED);
         manualLongFeedback.setDetailText("abc".repeat(5000));
@@ -553,6 +554,54 @@ void updateManualProgrammingExerciseResult_addFeedbackAfterManualLongFeedback()
         assertThat(savedAutomaticLongFeedback.getDetailText()).isEqualTo(manualLongFeedback.getDetailText());
     }
 
+    @ParameterizedTest
+    @ValueSource(booleans = { true, false })
+    @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
+    void shouldKeepExistingLongFeedbackWhenSavingAnAssessment(boolean submit) throws Exception {
+        var manualLongFeedback = new Feedback().credits(0.0);
+        var longText = "abc".repeat(5000);
+        manualLongFeedback.setDetailText(longText);
+        var result = new Result().feedbacks(List.of(manualLongFeedback)).score(0.0);
+        result = resultRepository.save(result);
+
+        LinkedMultiValueMap<String, String> params = new LinkedMultiValueMap<>();
+        params.add("submit", String.valueOf(submit));
+        result = request.putWithResponseBodyAndParams("/api/participations/" + programmingExerciseStudentParticipation.getId() + "/manual-results", result, Result.class,
+                HttpStatus.OK, params);
+
+        var longFeedbackText = longFeedbackTextRepository.findByFeedbackId(result.getFeedbacks().getFirst().getId());
+        assertThat(longFeedbackText).isPresent();
+        assertThat(longFeedbackText.get().getText()).isEqualTo(longText);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = { true, false })
+    @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
+    void shouldUpdateUnreferencedLongFeedbackWhenSavingAnAssessment(boolean submit) throws Exception {
+        var manualLongFeedback = new Feedback().credits(0.0).type(FeedbackType.MANUAL_UNREFERENCED);
+        var longText = "abc".repeat(5000);
+        manualLongFeedback.setDetailText(longText);
+        var result = new Result().feedbacks(List.of(manualLongFeedback)).score(0.0);
+        result = resultRepository.save(result);
+
+        var newLongText = "def".repeat(5000);
+        manualLongFeedback = result.getFeedbacks().getFirst();
+
+        // The actual complete longtext is still stored in the detailText field when the result is sent from the client
+        var detailText = Feedback.class.getDeclaredField("detailText");
+        detailText.setAccessible(true);
+        detailText.set(manualLongFeedback, newLongText);
+
+        LinkedMultiValueMap<String, String> params = new LinkedMultiValueMap<>();
+        params.add("submit", String.valueOf(submit));
+        result = request.putWithResponseBodyAndParams("/api/participations/" + programmingExerciseStudentParticipation.getId() + "/manual-results", result, Result.class,
+                HttpStatus.OK, params);
+
+        var longFeedbackText = longFeedbackTextRepository.findByFeedbackId(result.getFeedbacks().getFirst().getId());
+        assertThat(longFeedbackText).isPresent();
+        assertThat(longFeedbackText.get().getText()).isEqualTo(newLongText);
+    }
+
     @Test
     @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
     void updateManualProgrammingExerciseResult_addFeedbackAfterAutomaticLongFeedback() throws Exception {