Development: Prevent exception in Iris for inactive exercises (#10099)

ls1intum · Jan 4, 2025 · e219599 · e219599
1 parent ec8e010
commit e219599
Show file tree

Hide file tree

Showing 12 changed files with 110 additions and 84 deletions.
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
@@ -198,8 +198,8 @@ public Result createNewManualResult(Result result, boolean ratedResult) {
         return savedResult;
     }
 
-    public Result createNewRatedManualResult(Result result) {
-        return createNewManualResult(result, true);
+    public void createNewRatedManualResult(Result result) {
+        createNewManualResult(result, true);
     }
 
     /**

diff --git a/src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultWebsocketService.java b/src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultWebsocketService.java
@@ -18,7 +18,6 @@
 import de.tum.cit.aet.artemis.communication.service.WebsocketMessagingService;
 import de.tum.cit.aet.artemis.core.service.AuthorizationCheckService;
 import de.tum.cit.aet.artemis.exam.service.ExamDateService;
-import de.tum.cit.aet.artemis.exercise.domain.Exercise;
 import de.tum.cit.aet.artemis.exercise.domain.Team;
 import de.tum.cit.aet.artemis.exercise.domain.participation.Participation;
 import de.tum.cit.aet.artemis.exercise.domain.participation.StudentParticipation;
@@ -73,7 +72,7 @@ public void broadcastNewResult(Participation participation, Result result) {
     }
 
     private void broadcastNewResultToParticipants(StudentParticipation studentParticipation, Result result) {
-        final Exercise exercise = studentParticipation.getExercise();
+        final var exercise = studentParticipation.getExercise();
         boolean isWorkingPeriodOver;
         if (exercise.isExamExercise()) {
             isWorkingPeriodOver = examDateService.isIndividualExerciseWorkingPeriodOver(exercise.getExam(), studentParticipation);

diff --git a/src/main/java/de/tum/cit/aet/artemis/core/domain/User.java b/src/main/java/de/tum/cit/aet/artemis/core/domain/User.java
@@ -534,6 +534,10 @@ public void setIrisAcceptedTimestamp(@Nullable ZonedDateTime irisAccepted) {
         this.irisAccepted = irisAccepted;
     }
 
+    public boolean hasAcceptedIris() {
+        return irisAccepted != null;
+    }
+
     /**
      * Checks if the user has accepted the Iris privacy policy.
      * If not, an {@link AccessForbiddenException} is thrown.

diff --git a/src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseChatSessionRepository.java b/src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseChatSessionRepository.java
@@ -36,7 +36,6 @@ public interface IrisExerciseChatSessionRepository extends ArtemisJpaRepository<
      * @return A list of chat sessions sorted by creation date in descending order.
      */
     @Query("""
-
             SELECT s
                 FROM IrisExerciseChatSession s
                 WHERE s.exercise.id = :exerciseId
@@ -68,11 +67,9 @@ public interface IrisExerciseChatSessionRepository extends ArtemisJpaRepository<
      */
     default List<IrisExerciseChatSession> findLatestByExerciseIdAndUserIdWithMessages(Long exerciseId, Long userId, Pageable pageable) {
         List<Long> ids = findSessionsByExerciseIdAndUserId(exerciseId, userId, pageable).stream().map(DomainObject::getId).toList();
-
         if (ids.isEmpty()) {
             return Collections.emptyList();
         }
-
         return findSessionsWithMessagesByIdIn(ids);
     }
 

diff --git a/src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseSettingsRepository.java b/src/main/java/de/tum/cit/aet/artemis/iris/repository/IrisExerciseSettingsRepository.java
@@ -0,0 +1,24 @@
+package de.tum.cit.aet.artemis.iris.repository;
+
+import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_IRIS;
+
+import org.springframework.context.annotation.Profile;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.query.Param;
+import org.springframework.stereotype.Repository;
+
+import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository;
+import de.tum.cit.aet.artemis.iris.domain.settings.IrisExerciseSettings;
+
+@Repository
+@Profile(PROFILE_IRIS)
+public interface IrisExerciseSettingsRepository extends ArtemisJpaRepository<IrisExerciseSettings, Long> {
+
+    @Query("""
+            SELECT EXISTS(s)
+            FROM IrisExerciseSettings s
+            WHERE s.exercise.id = :exerciseId
+                AND s.irisTextExerciseChatSettings.enabled = TRUE
+            """)
+    boolean isExerciseChatEnabled(@Param("exerciseId") long exerciseId);
+}
diff --git a/src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java b/src/main/java/de/tum/cit/aet/artemis/iris/service/pyris/PyrisEventService.java
@@ -5,6 +5,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.context.annotation.Profile;
+import org.springframework.scheduling.annotation.Async;
 import org.springframework.stereotype.Service;
 
 import de.tum.cit.aet.artemis.iris.domain.session.IrisChatSession;
@@ -41,17 +42,18 @@ public PyrisEventService(IrisCourseChatSessionService irisCourseChatSessionServi
      *
      * @see PyrisEvent
      */
+    @Async
     public void trigger(PyrisEvent<? extends AbstractIrisChatSessionService<? extends IrisChatSession>, ?> event) {
         log.debug("Starting to process event of type: {}", event.getClass().getSimpleName());
         try {
             switch (event) {
                 case CompetencyJolSetEvent competencyJolSetEvent -> {
-                    log.info("Processing CompetencyJolSetEvent: {}", competencyJolSetEvent);
+                    log.debug("Processing CompetencyJolSetEvent: {}", competencyJolSetEvent);
                     competencyJolSetEvent.handleEvent(irisCourseChatSessionService);
                     log.debug("Successfully processed CompetencyJolSetEvent");
                 }
                 case NewResultEvent newResultEvent -> {
-                    log.info("Processing NewResultEvent: {}", newResultEvent);
+                    log.debug("Processing NewResultEvent: {}", newResultEvent);
                     newResultEvent.handleEvent(irisExerciseChatSessionService);
                     log.debug("Successfully processed NewResultEvent");
                 }

diff --git a/src/main/java/de/tum/cit/aet/artemis/lti/service/LtiNewResultService.java b/src/main/java/de/tum/cit/aet/artemis/lti/service/LtiNewResultService.java
@@ -3,6 +3,7 @@
 import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_LTI;
 
 import org.springframework.context.annotation.Profile;
+import org.springframework.scheduling.annotation.Async;
 import org.springframework.stereotype.Service;
 
 import de.tum.cit.aet.artemis.exercise.domain.participation.StudentParticipation;
@@ -22,6 +23,7 @@ public LtiNewResultService(Lti13Service lti13Service) {
      *
      * @param participation The exercise participation for which a new build result is available
      */
+    @Async
     public void onNewResult(StudentParticipation participation) {
         if (!participation.getExercise().getCourseViaExerciseGroupOrCourseMember().isOnlineCourse()) {
             return;

diff --git a/src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java b/src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingMessagingService.java
@@ -28,6 +28,7 @@
 import de.tum.cit.aet.artemis.exercise.dto.SubmissionDTO;
 import de.tum.cit.aet.artemis.exercise.repository.ParticipationRepository;
 import de.tum.cit.aet.artemis.exercise.repository.TeamRepository;
+import de.tum.cit.aet.artemis.iris.repository.IrisExerciseSettingsRepository;
 import de.tum.cit.aet.artemis.iris.service.pyris.PyrisEventService;
 import de.tum.cit.aet.artemis.iris.service.pyris.event.NewResultEvent;
 import de.tum.cit.aet.artemis.lti.service.LtiNewResultService;
@@ -57,16 +58,20 @@ public class ProgrammingMessagingService {
 
     private final Optional<PyrisEventService> pyrisEventService;
 
+    private final Optional<IrisExerciseSettingsRepository> irisExerciseSettingsRepository;
+
     private final ParticipationRepository participationRepository;
 
     public ProgrammingMessagingService(GroupNotificationService groupNotificationService, WebsocketMessagingService websocketMessagingService,
             ResultWebsocketService resultWebsocketService, Optional<LtiNewResultService> ltiNewResultService, TeamRepository teamRepository,
-            Optional<PyrisEventService> pyrisEventService, ParticipationRepository participationRepository) {
+            Optional<PyrisEventService> pyrisEventService, Optional<IrisExerciseSettingsRepository> irisExerciseSettingsRepository,
+            ParticipationRepository participationRepository) {
         this.groupNotificationService = groupNotificationService;
         this.websocketMessagingService = websocketMessagingService;
         this.resultWebsocketService = resultWebsocketService;
         this.ltiNewResultService = ltiNewResultService;
         this.teamRepository = teamRepository;
+        this.irisExerciseSettingsRepository = irisExerciseSettingsRepository;
         this.participationRepository = participationRepository;
         this.pyrisEventService = pyrisEventService;
     }
@@ -188,13 +193,16 @@ public void notifyUserAboutNewResult(Result result, ProgrammingExerciseParticipa
         if (participation instanceof ProgrammingExerciseStudentParticipation studentParticipation) {
             // do not try to report results for template or solution participations
             ltiNewResultService.ifPresent(newResultService -> newResultService.onNewResult(studentParticipation));
-            // Inform Iris about the submission status
+            // Inform Iris about the submission status (when certain conditions are met)
             notifyIrisAboutSubmissionStatus(result, studentParticipation);
         }
     }
 
     /**
      * Notify Iris about the submission status for the given result and student participation.
+     * Only notifies if the user has accepted Iris, the exercise is not an exam exercise, and the exercise chat is enabled in the exercise settings
+     * NOTE: we check those settings early to prevent unnecessary database queries and exceptions later on in most cases. More sophisticated checks are done in the Iris service.
+     * <p>
      * If the submission was successful, Iris will be informed about the successful submission.
      * If the submission failed, Iris will be informed about the submission failure.
      * Iris will only be informed about the submission status if the participant is a user.
@@ -203,14 +211,18 @@ public void notifyUserAboutNewResult(Result result, ProgrammingExerciseParticipa
      * @param studentParticipation the student participation for which Iris should be informed about the submission status
      */
     private void notifyIrisAboutSubmissionStatus(Result result, ProgrammingExerciseStudentParticipation studentParticipation) {
-        if (studentParticipation.getParticipant() instanceof User) {
+        if (studentParticipation.getParticipant() instanceof User user) {
             pyrisEventService.ifPresent(eventService -> {
-                // Inform event service about the new result
-                try {
-                    eventService.trigger(new NewResultEvent(result));
-                }
-                catch (Exception e) {
-                    log.error("Could not trigger service for result {}", result.getId(), e);
+                final var exercise = studentParticipation.getExercise();
+                if (user.hasAcceptedIris() && !exercise.isExamExercise() && irisExerciseSettingsRepository.get().isExerciseChatEnabled(exercise.getId())) {
+                    // Inform event service about the new result
+                    try {
+                        // This is done asynchronously to prevent blocking the current thread
+                        eventService.trigger(new NewResultEvent(result));
+                    }
+                    catch (Exception e) {
+                        log.error("Could not trigger service for result {}", result.getId(), e);
+                    }
                 }
             });
         }

diff --git a/src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java b/src/test/java/de/tum/cit/aet/artemis/buildagent/service/BuildAgentDockerServiceTest.java
@@ -4,6 +4,7 @@
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -83,7 +84,7 @@ void testDeleteOldDockerImages_NoOutdatedImages() {
         buildAgentDockerService.deleteOldDockerImages();
 
         // Verify that removeImageCmd() was not called.
-        verify(dockerClient, times(0)).removeImageCmd(anyString());
+        verify(dockerClient, never()).removeImageCmd(anyString());
     }
 
     @Test

diff --git a/...t/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.java b/...t/java/de/tum/cit/aet/artemis/exercise/service/ParticipationTeamWebsocketServiceTest.java
@@ -2,6 +2,7 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.RETURNS_MOCKS;
+import static org.mockito.Mockito.after;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.timeout;
@@ -139,7 +140,7 @@ void testPatchModelingSubmissionWithWrongPrincipal() {
         // when we submit a patch, but with the wrong user ...
         participationTeamWebsocketService.patchModelingSubmission(participation.getId(), patch, getPrincipalMock("student2"));
         // the patch should not be broadcast.
-        verify(websocketMessagingService, timeout(2000).times(0)).sendMessage(websocketTopic(participation), List.of());
+        verify(websocketMessagingService, after(1000).never()).sendMessage(websocketTopic(participation), List.of());
     }
 
     @Test
@@ -152,7 +153,7 @@ void testUpdateModelingSubmission() {
         // the submission should be handled by the service (i.e. saved), ...
         verify(modelingSubmissionService, timeout(2000).times(1)).handleModelingSubmission(any(), any(), any());
         // but it should NOT be broadcast (sync is handled with patches only).
-        verify(websocketMessagingService, timeout(2000).times(0)).sendMessage(websocketTopic(participation), List.of());
+        verify(websocketMessagingService, after(1000).never()).sendMessage(websocketTopic(participation), List.of());
     }
 
     @Test
@@ -163,9 +164,9 @@ void testUpdateModelingSubmissionWithWrongPrincipal() {
         // when we submit a new modeling submission with the wrong user ...
         participationTeamWebsocketService.updateModelingSubmission(participation.getId(), submission, getPrincipalMock("student2"));
         // the submission is NOT saved ...
-        verify(modelingSubmissionService, timeout(2000).times(0)).handleModelingSubmission(any(), any(), any());
+        verify(modelingSubmissionService, after(1000).never()).handleModelingSubmission(any(), any(), any());
         // it is also not broadcast.
-        verify(websocketMessagingService, timeout(2000).times(0)).sendMessage(websocketTopic(participation), List.of());
+        verify(websocketMessagingService, after(1000).never()).sendMessage(websocketTopic(participation), List.of());
     }
 
     @Test