Programming exercises: Fix an issue in which long manual feedback is …

…not correctly displayed (#9562)

src/main/java/de/tum/cit/aet/artemis/assessment/repository/LongFeedbackTextRepository.java

@@ -6,9 +6,11 @@
 import java.util.Optional;
 
 import org.springframework.context.annotation.Profile;
+import org.springframework.data.jpa.repository.Modifying;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;
+import org.springframework.transaction.annotation.Transactional;
 
 import de.tum.cit.aet.artemis.assessment.domain.LongFeedbackText;
 import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository;
@@ -42,6 +44,14 @@ public interface LongFeedbackTextRepository extends ArtemisJpaRepository<LongFee
             """)
     Optional<LongFeedbackText> findWithFeedbackAndResultAndParticipationByFeedbackId(@Param("feedbackId") final Long feedbackId);
 
+    @Modifying
+    @Transactional
+    @Query("""
+            DELETE FROM LongFeedbackText longFeedback
+            WHERE longFeedback.feedback.id IN :feedbackIds
+            """)
+    void deleteByFeedbackIds(@Param("feedbackIds") List<Long> feedbackIds);
+
     default LongFeedbackText findByFeedbackIdWithFeedbackAndResultAndParticipationElseThrow(final Long feedbackId) {
         return getValueElseThrow(findWithFeedbackAndResultAndParticipationByFeedbackId(feedbackId), feedbackId);
     }

src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.java

@@ -17,6 +17,7 @@
 import de.tum.cit.aet.artemis.assessment.dto.AssessmentUpdateBaseDTO;
 import de.tum.cit.aet.artemis.assessment.repository.ComplaintRepository;
 import de.tum.cit.aet.artemis.assessment.repository.FeedbackRepository;
+import de.tum.cit.aet.artemis.assessment.repository.LongFeedbackTextRepository;
 import de.tum.cit.aet.artemis.assessment.repository.ResultRepository;
 import de.tum.cit.aet.artemis.assessment.web.ResultWebsocketService;
 import de.tum.cit.aet.artemis.communication.service.notifications.SingleUserNotificationService;
@@ -67,10 +68,12 @@ public class AssessmentService {
 
     protected final ResultWebsocketService resultWebsocketService;
 
+    private final LongFeedbackTextRepository longFeedbackTextRepository;
+
     public AssessmentService(ComplaintResponseService complaintResponseService, ComplaintRepository complaintRepository, FeedbackRepository feedbackRepository,
             ResultRepository resultRepository, StudentParticipationRepository studentParticipationRepository, ResultService resultService, SubmissionService submissionService,
             SubmissionRepository submissionRepository, ExamDateService examDateService, UserRepository userRepository, Optional<LtiNewResultService> ltiNewResultService,
-            SingleUserNotificationService singleUserNotificationService, ResultWebsocketService resultWebsocketService) {
+            SingleUserNotificationService singleUserNotificationService, ResultWebsocketService resultWebsocketService, LongFeedbackTextRepository longFeedbackTextRepository) {
         this.complaintResponseService = complaintResponseService;
         this.complaintRepository = complaintRepository;
         this.feedbackRepository = feedbackRepository;
@@ -84,6 +87,7 @@ public AssessmentService(ComplaintResponseService complaintResponseService, Comp
         this.ltiNewResultService = ltiNewResultService;
         this.singleUserNotificationService = singleUserNotificationService;
         this.resultWebsocketService = resultWebsocketService;
+        this.longFeedbackTextRepository = longFeedbackTextRepository;
     }
 
     /**
@@ -283,6 +287,9 @@ public Result saveManualAssessment(final Submission submission, final List<Feedb
         User user = userRepository.getUser();
         result.setAssessor(user);
 
+        // long feedback text is deleted as it otherwise causes duplicate entries errors and will be saved again with {@link resultRepository.save}
+        resultService.deleteLongFeedback(feedbackList, result);
+
         result.updateAllFeedbackItems(feedbackList, false);
         result.determineAssessmentType();
 

src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java

@@ -12,6 +12,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 import jakarta.annotation.Nullable;
@@ -494,45 +495,49 @@ public Map<Long, String> getLogsAvailabilityForResults(List<Result> results, Par
 
     @NotNull
     private List<Feedback> saveFeedbackWithHibernateWorkaround(@NotNull Result result, List<Feedback> feedbackList) {
-        // Avoid hibernate exception
         List<Feedback> savedFeedbacks = new ArrayList<>();
-        // Collect ids of feedbacks that have long feedback.
-        List<Long> feedbackIdsWithLongFeedback = feedbackList.stream().filter(feedback -> feedback.getId() != null && feedback.getHasLongFeedbackText()).map(Feedback::getId)
-                .toList();
-        // Get long feedback list from the database.
-        List<LongFeedbackText> longFeedbackTextList = longFeedbackTextRepository.findByFeedbackIds(feedbackIdsWithLongFeedback);
 
-        // Convert list to map for accessing later.
-        Map<Long, LongFeedbackText> longLongFeedbackTextMap = longFeedbackTextList.stream()
-                .collect(Collectors.toMap(longFeedbackText -> longFeedbackText.getFeedback().getId(), longFeedbackText -> longFeedbackText));
-        feedbackList.forEach(feedback -> {
-            // cut association to parent object
-            feedback.setResult(null);
-            LongFeedbackText longFeedback = null;
-            // look for long feedback that parent feedback has and cut the association between them.
-            if (feedback.getId() != null && feedback.getHasLongFeedbackText()) {
-                longFeedback = longLongFeedbackTextMap.get(feedback.getId());
-                if (longFeedback != null) {
-                    feedback.clearLongFeedback();
-                }
-            }
-            // persist the child object without an association to the parent object.
-            feedback = feedbackRepository.saveAndFlush(feedback);
-            // restore the association to the parent object
-            feedback.setResult(result);
+        // Fetch long feedback texts associated with the provided feedback list
+        Map<Long, LongFeedbackText> longFeedbackTextMap = longFeedbackTextRepository
+                .findByFeedbackIds(feedbackList.stream().filter(feedback -> feedback.getId() != null && feedback.getHasLongFeedbackText()).map(Feedback::getId).toList()).stream()
+                .collect(Collectors.toMap(longFeedbackText -> longFeedbackText.getFeedback().getId(), Function.identity()));
 
-            // restore the association of the long feedback to the parent feedback
-            if (longFeedback != null) {
-                feedback.setDetailText(longFeedback.getText());
-            }
+        feedbackList.forEach(feedback -> {
+            handleFeedbackPersistence(feedback, result, longFeedbackTextMap);
             savedFeedbacks.add(feedback);
         });
+
         return savedFeedbacks;
     }
 
+    private void handleFeedbackPersistence(Feedback feedback, Result result, Map<Long, LongFeedbackText> longFeedbackTextMap) {
+        // Temporarily detach feedback from the parent result to avoid Hibernate issues
+        feedback.setResult(null);
+
+        // Clear the long feedback if it exists in the map
+        if (feedback.getId() != null && feedback.getHasLongFeedbackText()) {
+            LongFeedbackText longFeedback = longFeedbackTextMap.get(feedback.getId());
+            if (longFeedback != null) {
+                feedback.clearLongFeedback();
+            }
+        }
+
+        // Persist the feedback entity without the parent association
+        feedback = feedbackRepository.saveAndFlush(feedback);
+
+        // Restore associations to the result and long feedback after persistence
+        feedback.setResult(result);
+        LongFeedbackText longFeedback = longFeedbackTextMap.get(feedback.getId());
+        if (longFeedback != null) {
+            feedback.setDetailText(longFeedback.getText());
+        }
+    }
+
     @NotNull
     private Result shouldSaveResult(@NotNull Result result, boolean shouldSave) {
         if (shouldSave) {
+            // long feedback text is deleted as it otherwise causes duplicate entries errors and will be saved again with {@link resultRepository.save}
+            deleteLongFeedback(result.getFeedbacks(), result);
             // Note: This also saves the feedback objects in the database because of the 'cascade = CascadeType.ALL' option.
             return resultRepository.save(result);
         }
@@ -623,4 +628,32 @@ public FeedbackAnalysisResponseDTO getFeedbackDetailsOnPage(long exerciseId, Fee
     public long getMaxCountForExercise(long exerciseId) {
         return studentParticipationRepository.findMaxCountForExercise(exerciseId);
     }
+
+    /**
+     * Deletes long feedback texts for the provided list of feedback items to prevent duplicate entries in the {@link LongFeedbackTextRepository}.
+     * <br>
+     * This method processes the provided list of feedback items, identifies those with associated long feedback texts, and removes them in bulk
+     * from the repository to avoid potential duplicate entry errors when saving new feedback entries.
+     * <p>
+     * Primarily used to ensure data consistency in the {@link LongFeedbackTextRepository}, especially during operations where feedback entries are
+     * overridden or updated. The deletion is performed only for feedback items with a non-null ID and an associated long feedback text.
+     * <p>
+     * This approach reduces the need for individual deletion calls and performs batch deletion in a single database operation.
+     *
+     * @param feedbackList The list of {@link Feedback} objects for which the long feedback texts are to be deleted. Only feedback items that have long feedback texts and a
+     *                         non-null ID will be processed.
+     * @param result       The {@link Result} object associated with the feedback items, used to update feedback list before processing.
+     */
+    public void deleteLongFeedback(List<Feedback> feedbackList, Result result) {
+        if (feedbackList == null) {
+            return;
+        }
+
+        List<Long> feedbackIdsWithLongText = feedbackList.stream().filter(feedback -> feedback.getHasLongFeedbackText() && feedback.getId() != null).map(Feedback::getId).toList();
+
+        longFeedbackTextRepository.deleteByFeedbackIds(feedbackIdsWithLongText);
+
+        List<Feedback> feedbacks = new ArrayList<>(feedbackList);
+        result.updateAllFeedbackItems(feedbacks, false);
+    }
 }

src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java

@@ -16,6 +16,7 @@
 import de.tum.cit.aet.artemis.assessment.repository.ComplaintRepository;
 import de.tum.cit.aet.artemis.assessment.repository.FeedbackRepository;
 import de.tum.cit.aet.artemis.assessment.repository.GradingCriterionRepository;
+import de.tum.cit.aet.artemis.assessment.repository.LongFeedbackTextRepository;
 import de.tum.cit.aet.artemis.assessment.repository.ResultRepository;
 import de.tum.cit.aet.artemis.assessment.service.AssessmentService;
 import de.tum.cit.aet.artemis.assessment.service.ComplaintResponseService;
@@ -48,9 +49,10 @@ public ProgrammingAssessmentService(ComplaintResponseService complaintResponseSe
             ResultRepository resultRepository, StudentParticipationRepository studentParticipationRepository, ResultService resultService, SubmissionService submissionService,
             SubmissionRepository submissionRepository, ExamDateService examDateService, UserRepository userRepository, GradingCriterionRepository gradingCriterionRepository,
             Optional<LtiNewResultService> ltiNewResultService, SingleUserNotificationService singleUserNotificationService, ResultWebsocketService resultWebsocketService,
-            ProgrammingExerciseParticipationService programmingExerciseParticipationService, Optional<AthenaFeedbackSendingService> athenaFeedbackSendingService) {
+            ProgrammingExerciseParticipationService programmingExerciseParticipationService, Optional<AthenaFeedbackSendingService> athenaFeedbackSendingService,
+            LongFeedbackTextRepository longFeedbackTextRepository) {
         super(complaintResponseService, complaintRepository, feedbackRepository, resultRepository, studentParticipationRepository, resultService, submissionService,
-                submissionRepository, examDateService, userRepository, ltiNewResultService, singleUserNotificationService, resultWebsocketService);
+                submissionRepository, examDateService, userRepository, ltiNewResultService, singleUserNotificationService, resultWebsocketService, longFeedbackTextRepository);
         this.programmingExerciseParticipationService = programmingExerciseParticipationService;
         this.athenaFeedbackSendingService = athenaFeedbackSendingService;
     }
@@ -88,6 +90,8 @@ 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();

src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java

@@ -16,6 +16,7 @@
 import de.tum.cit.aet.artemis.assessment.repository.ComplaintRepository;
 import de.tum.cit.aet.artemis.assessment.repository.FeedbackRepository;
 import de.tum.cit.aet.artemis.assessment.repository.GradingCriterionRepository;
+import de.tum.cit.aet.artemis.assessment.repository.LongFeedbackTextRepository;
 import de.tum.cit.aet.artemis.assessment.repository.ResultRepository;
 import de.tum.cit.aet.artemis.assessment.service.AssessmentService;
 import de.tum.cit.aet.artemis.assessment.service.ComplaintResponseService;
@@ -41,9 +42,9 @@ public TextAssessmentService(UserRepository userRepository, ComplaintResponseSer
             FeedbackRepository feedbackRepository, ResultRepository resultRepository, StudentParticipationRepository studentParticipationRepository, ResultService resultService,
             SubmissionRepository submissionRepository, TextBlockService textBlockService, ExamDateService examDateService, GradingCriterionRepository gradingCriterionRepository,
             SubmissionService submissionService, Optional<LtiNewResultService> ltiNewResultService, SingleUserNotificationService singleUserNotificationService,
-            ResultWebsocketService resultWebsocketService) {
+            ResultWebsocketService resultWebsocketService, LongFeedbackTextRepository longFeedbackTextRepository) {
         super(complaintResponseService, complaintRepository, feedbackRepository, resultRepository, studentParticipationRepository, resultService, submissionService,
-                submissionRepository, examDateService, userRepository, ltiNewResultService, singleUserNotificationService, resultWebsocketService);
+                submissionRepository, examDateService, userRepository, ltiNewResultService, singleUserNotificationService, resultWebsocketService, longFeedbackTextRepository);
         this.textBlockService = textBlockService;
     }
 

src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts

@@ -1,17 +1,19 @@
-import { Component, EventEmitter, Input, Output } from '@angular/core';
+import { Component, EventEmitter, Input, InputSignal, OnInit, Output, inject, input } from '@angular/core';
 import { faCheck, faExclamation, faExclamationTriangle, faQuestionCircle, faTrash, faTrashAlt } from '@fortawesome/free-solid-svg-icons';
 import { Feedback, FeedbackType } from 'app/entities/feedback.model';
 import { StructuredGradingCriterionService } from 'app/exercises/shared/structured-grading-criterion/structured-grading-criterion.service';
 import { ButtonSize } from 'app/shared/components/button.component';
 import { Subject } from 'rxjs';
+import { FeedbackService } from 'app/exercises/shared/feedback/feedback.service';
 
 @Component({
     selector: 'jhi-unreferenced-feedback-detail',
     templateUrl: './unreferenced-feedback-detail.component.html',
     styleUrls: ['./unreferenced-feedback-detail.component.scss'],
 })
-export class UnreferencedFeedbackDetailComponent {
+export class UnreferencedFeedbackDetailComponent implements OnInit {
     @Input() public feedback: Feedback;
+    resultId: InputSignal<number> = input.required<number>();
     @Input() isSuggestion: boolean;
     @Input() public readOnly: boolean;
     @Input() highlightDifferences: boolean;
@@ -21,6 +23,7 @@ export class UnreferencedFeedbackDetailComponent {
     @Output() public onFeedbackDelete = new EventEmitter<Feedback>();
     @Output() onAcceptSuggestion = new EventEmitter<Feedback>();
     @Output() onDiscardSuggestion = new EventEmitter<Feedback>();
+    private feedbackService = inject(FeedbackService);
 
     // Icons
     faTrashAlt = faTrashAlt;
@@ -39,6 +42,20 @@ export class UnreferencedFeedbackDetailComponent {
 
     constructor(public structuredGradingCriterionService: StructuredGradingCriterionService) {}
 
+    ngOnInit() {
+        this.loadLongFeedback();
+    }
+
+    /**
+     * Call this method to load long feedback if needed
+     */
+    public async loadLongFeedback() {
+        if (this.feedback.hasLongFeedbackText) {
+            this.feedback.detailText = await this.feedbackService.getLongFeedbackText(this.resultId, this.feedback.id!);
+            this.onFeedbackChange.emit(this.feedback);
+        }
+    }
+
     /**
      * Emits assessment changes to parent component
      */