From c4bc5a80cd124b04118e1bfec981c0c614501698 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sun, 15 Dec 2024 08:38:25 +0100 Subject: [PATCH] Development: Improve error handling when generating student exams --- .../exam/service/ExamAccessService.java | 39 ++++++++++++------- .../aet/artemis/exam/web/ExamResource.java | 4 +- .../aet/artemis/exam/ExamIntegrationTest.java | 6 +-- .../artemis/exam/TestExamIntegrationTest.java | 13 ++++++- .../exam/service/ExamAccessServiceTest.java | 38 +++++++++--------- 5 files changed, 59 insertions(+), 41 deletions(-) diff --git a/src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java b/src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java index ca97f86bce7c..5de7270514c7 100644 --- a/src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java +++ b/src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java @@ -60,17 +60,17 @@ public ExamAccessService(ExamRepository examRepository, StudentExamRepository st } /** - * Real Exams: Checks if the current user is allowed to see the requested exam. If he is allowed the exam will be returned. + * TODO: we should distinguish the whole method between test exam and real exam to improve the readability of the code + * Real Exams: Checks if the current user is allowed to see the requested exam. If he is allowed the student exam will be returned (Fallback: create a new one) * Test Exams: Either retrieves an existing StudentExam from the Database or generates a new StudentExam * * @param courseId The id of the course * @param examId The id of the exam * @return a ResponseEntity with the exam */ - public StudentExam getExamInCourseElseThrow(Long courseId, Long examId) { + public StudentExam getOrCreateStudentExamElseThrow(Long courseId, Long examId) { User currentUser = userRepository.getUserWithGroupsAndAuthorities(); - // TODO: we should distinguish the whole method between test exam and real exam to improve the readability of the code // Check that the current user is at least student in the course. Course course = courseRepository.findByIdElseThrow(courseId); authorizationCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.STUDENT, course, currentUser); @@ -79,30 +79,39 @@ public StudentExam getExamInCourseElseThrow(Long courseId, Long examId) { Optional optionalStudentExam = studentExamRepository.findByExamIdAndUserId(examId, currentUser.getId()); StudentExam studentExam; - // If an studentExam can be fund, we can proceed + // If an studentExam can be found, we can immediately proceed if (optionalStudentExam.isPresent()) { studentExam = optionalStudentExam.get(); } else { - Exam examWithExerciseGroupsAndExercises = examRepository.findWithExerciseGroupsAndExercisesByIdOrElseThrow(examId); + Exam exam = examRepository.findWithExerciseGroupsAndExercisesByIdOrElseThrow(examId); + ZonedDateTime now = ZonedDateTime.now(); + ZonedDateTime unlockDate = ExamDateService.getExamProgrammingExerciseUnlockDate(exam); + // An exam can be started 5 minutes before the start time, which is when programming exercises are unlocked - boolean canExamBeStarted = ZonedDateTime.now().isAfter(ExamDateService.getExamProgrammingExerciseUnlockDate(examWithExerciseGroupsAndExercises)); - boolean isExamEnded = ZonedDateTime.now().isAfter(examWithExerciseGroupsAndExercises.getEndDate()); + boolean canExamBeStarted = now.isAfter(unlockDate); + boolean isTestExam = exam.isTestExam(); + boolean isUserRegistered = examRegistrationService.isUserRegisteredForExam(examId, currentUser.getId()); + boolean isExamEnded = ZonedDateTime.now().isAfter(exam.getEndDate()); // Generate a student exam if the following conditions are met: // 1. The exam has not ended. // 2. The exam is either a test exam, OR it is a normal exam where the user is registered and can click the start button. // Allowing student exams to be generated only when students can click the start button prevents inconsistencies. // For example, this avoids a scenario where a student generates an exam and an instructor adds an exercise group afterward. - if (!isExamEnded - && (examWithExerciseGroupsAndExercises.isTestExam() || (examRegistrationService.isUserRegisteredForExam(examId, currentUser.getId()) && canExamBeStarted))) { - studentExam = studentExamService.generateIndividualStudentExam(examWithExerciseGroupsAndExercises, currentUser); - // For the start of the exam, the exercises are not needed. They are later loaded via StudentExamResource - studentExam.setExercises(null); + if (isExamEnded) { + throw new BadRequestAlertException("The exam has already ended. Cannot generate student exam.", ENTITY_NAME, "examEnded", true); + } + if (!isTestExam && !isUserRegistered) { + throw new AccessForbiddenException("User is not registered for the exam. Cannot generate student exam."); } + if (!canExamBeStarted) { + throw new AccessForbiddenException("The exam cannot be started yet. Cannot generate student exam."); + } + // Proceed only if the exam has not ended and the user meets the conditions else { - // We skip the alert since this can happen when a tutor sees the exam card or the user did not participate yet is registered for the exam - throw new BadRequestAlertException("Cannot generate student exam for exam ID " + examId + ".", ENTITY_NAME, "cannotGenerateStudentExam", true); + studentExam = studentExamService.generateIndividualStudentExam(exam, currentUser); + studentExam.setExercises(null); } } @@ -111,7 +120,7 @@ public StudentExam getExamInCourseElseThrow(Long courseId, Long examId) { checkExamBelongsToCourseElseThrow(courseId, exam); if (!examId.equals(exam.getId())) { - throw new BadRequestAlertException("The provided examId does not match with the examId of the studentExam", ENTITY_NAME, "examIdMismatch"); + throw new ConflictException("The provided examId does not match with the examId of the studentExam", ENTITY_NAME, "examIdMismatch"); } // Check that the exam is visible diff --git a/src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java b/src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java index b986d0366683..22941fff85e7 100644 --- a/src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java +++ b/src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java @@ -1101,7 +1101,7 @@ public ResponseEntity removeAllStudentsFromExam(@PathVariable Long courseI * GET /courses/{courseId}/exams/{examId}/own-student-exam: Get the own student exam for the exam * Real Exams: StudentExam needs to be generated by an instructor * Test Exam: StudentExam can be self-created by the user - * Note: The Access control is performed in the {@link ExamAccessService#getExamInCourseElseThrow(Long, Long)} to limit the DB-calls + * Note: The Access control is performed in the {@link ExamAccessService#getOrCreateStudentExamElseThrow(Long, Long)} to limit the DB-calls * * @param courseId the id of the course * @param examId the id of the exam @@ -1111,7 +1111,7 @@ public ResponseEntity removeAllStudentsFromExam(@PathVariable Long courseI @EnforceAtLeastStudent public ResponseEntity getOwnStudentExam(@PathVariable Long courseId, @PathVariable Long examId) { log.debug("REST request to get exam {} for conduction", examId); - StudentExam exam = examAccessService.getExamInCourseElseThrow(courseId, examId); + StudentExam exam = examAccessService.getOrCreateStudentExamElseThrow(courseId, examId); exam.getUser().setVisibleRegistrationNumber(); return ResponseEntity.ok(exam); } diff --git a/src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java index e86f315c26fc..57368e33484b 100644 --- a/src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java @@ -1040,7 +1040,7 @@ void testGetStudentExamForStart() throws Exception { exam.setVisibleDate(ZonedDateTime.now().minusHours(1).minusMinutes(5)); StudentExam response = request.get("/api/courses/" + course1.getId() + "/exams/" + exam.getId() + "/own-student-exam", HttpStatus.OK, StudentExam.class); assertThat(response.getExam()).isEqualTo(exam); - verify(examAccessService).getExamInCourseElseThrow(course1.getId(), exam.getId()); + verify(examAccessService).getOrCreateStudentExamElseThrow(course1.getId(), exam.getId()); } @ParameterizedTest(name = "{displayName} [{index}] {argumentsWithNames}") @@ -1460,13 +1460,13 @@ void testRetrieveOwnStudentExam_noStudentExam() throws Exception { examUser1 = examUserRepository.save(examUser1); exam.addExamUser(examUser1); examRepository.save(exam); - request.get("/api/courses/" + course1.getId() + "/exams/" + exam1.getId() + "/own-student-exam", HttpStatus.BAD_REQUEST, StudentExam.class); + request.get("/api/courses/" + course1.getId() + "/exams/" + exam1.getId() + "/own-student-exam", HttpStatus.FORBIDDEN, StudentExam.class); } @Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void testRetrieveOwnStudentExam_instructor() throws Exception { - request.get("/api/courses/" + course1.getId() + "/exams/" + exam1.getId() + "/own-student-exam", HttpStatus.BAD_REQUEST, StudentExam.class); + request.get("/api/courses/" + course1.getId() + "/exams/" + exam1.getId() + "/own-student-exam", HttpStatus.FORBIDDEN, StudentExam.class); } @Test diff --git a/src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java index 0853bed96f6c..15063df87150 100644 --- a/src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java @@ -218,9 +218,20 @@ void testGetStudentExamForTestExamForStart_notVisible() throws Exception { @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") - void testGetStudentExamForTestExamForStart_ExamDoesNotBelongToCourse() throws Exception { + void testGetStudentExamForTestExamForStart_tooEarly() throws Exception { Exam testExam = examUtilService.addTestExam(course2); + testExam.setStartDate(now().plusMinutes(10)); + testExam = examRepository.save(testExam); + // the request fails because the exam start is 10 min in the future + request.get("/api/courses/" + course1.getId() + "/exams/" + testExam.getId() + "/own-student-exam", HttpStatus.FORBIDDEN, StudentExam.class); + } + @Test + @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") + void testGetStudentExamForTestExamForStart_ExamDoesNotBelongToCourse() throws Exception { + Exam testExam = examUtilService.addTestExam(course2); + testExam.setStartDate(now().plusMinutes(4)); + testExam = examRepository.save(testExam); request.get("/api/courses/" + course1.getId() + "/exams/" + testExam.getId() + "/own-student-exam", HttpStatus.CONFLICT, StudentExam.class); } diff --git a/src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java b/src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java index 73467cf82f0a..1a3e3d6e5d23 100644 --- a/src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java @@ -329,13 +329,13 @@ void testCheckAndGetCourseAndExamAccessForConduction_isStudentInCourse() { Course course = courseUtilService.addEmptyCourse(); course.setStudentGroupName("another"); courseRepository.save(course); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course.getId(), exam1.getId())).isInstanceOf(AccessForbiddenException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course.getId(), exam1.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testCheckAndGetCourseAndExamAccessForConduction_examExists() { - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), 123155L)).isInstanceOf(EntityNotFoundException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), 123155L)).isInstanceOf(EntityNotFoundException.class); } @Test @@ -343,13 +343,13 @@ void testCheckAndGetCourseAndExamAccessForConduction_examExists() { void testCheckAndGetCourseAndExamAccessForConduction_examBelongsToCourse() { studentExam2.setUser(userUtilService.getUserByLogin(TEST_PREFIX + "student1")); studentExamRepository.save(studentExam2); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam2.getId())).isInstanceOf(ConflictException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam2.getId())).isInstanceOf(ConflictException.class); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testCheckAndGetCourseAndExamAccessForConduction_notRegisteredUser() { - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam2.getId())).isInstanceOf(BadRequestAlertException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam2.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @@ -360,7 +360,7 @@ void testCheckAndGetCourseAndExamAccessForConduction_registeredUser_noStudentExa exam1.setEndDate(ZonedDateTime.now().plusMinutes(10)); examRepository.save(exam1); studentExamRepository.delete(studentExam1); - assertThatNoException().isThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam1.getId())); + assertThatNoException().isThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam1.getId())); } @Test @@ -370,8 +370,7 @@ void testCheckAndGetCourseAndExamAccessForConduction_registeredUser_noStudentExa exam1.setStartDate(ZonedDateTime.now().plusMinutes(7)); examRepository.save(exam1); studentExamRepository.delete(studentExam1); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam1.getId())).asInstanceOf(type(BadRequestAlertException.class)) - .satisfies(error -> assertThat(error.getParameters().get("skipAlert")).isEqualTo(Boolean.TRUE)); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam1.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @@ -382,14 +381,14 @@ void testCheckAndGetCourseAndExamAccessForConduction_registeredUser_noStudentExa exam1.setEndDate(ZonedDateTime.now().minusMinutes(7)); examRepository.save(exam1); studentExamRepository.delete(studentExam1); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam1.getId())).asInstanceOf(type(BadRequestAlertException.class)) + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam1.getId())).asInstanceOf(type(BadRequestAlertException.class)) .satisfies(error -> assertThat(error.getParameters().get("skipAlert")).isEqualTo(Boolean.TRUE)); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void testCheckAndGetCourseAndExamAccessForConduction_registeredUser_studentExamPresent() { - StudentExam studentExam = examAccessService.getExamInCourseElseThrow(course1.getId(), exam1.getId()); + StudentExam studentExam = examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam1.getId()); assertThat(studentExam.equals(studentExam1)).isEqualTo(true); } @@ -398,40 +397,39 @@ void testCheckAndGetCourseAndExamAccessForConduction_registeredUser_studentExamP void testCheckAndGetCourseAndExamAccessForConduction_examIsVisible() { testExam1.setVisibleDate(ZonedDateTime.now().plusMinutes(5)); examRepository.save(testExam1); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") - void testGetExamInCourseElseThrow_noCourseAccess() { + void testGetOrCreateStudentExamAccess() { User student1 = userUtilService.getUserByLogin(TEST_PREFIX + "student1"); student1.setGroups(Set.of()); userRepository.save(student1); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") - void testGetExamInCourseElseThrow_notVisible() { + void testGetOrCreateStudentExamElseThrow_notVisible() { testExam1.setVisibleDate(ZonedDateTime.now().plusHours(5)); examRepository.save(testExam1); - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(AccessForbiddenException.class); } @Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") - void testGetExamInCourseElseThrow_success_studentExamPresent() { - StudentExam studentExam = examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId()); + void testGetExamInCourseElseThrow_success_studentOrCreateStudentExamPresent() { + StudentExam studentExam = examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), testExam1.getId()); assertThat(studentExam.equals(studentExamForTestExam1)).isEqualTo(true); - StudentExam studentExam2 = examAccessService.getExamInCourseElseThrow(course2.getId(), testExam2.getId()); + StudentExam studentExam2 = examAccessService.getOrCreateStudentExamElseThrow(course2.getId(), testExam2.getId()); assertThat(studentExam2.equals(studentExamForTestExam2)).isEqualTo(true); } @Test @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA") - void testGetExamInCourseElseThrow_tutor_skipAlert() { - assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), exam1.getId())).asInstanceOf(type(BadRequestAlertException.class)) - .satisfies(error -> assertThat(error.getParameters().get("skipAlert")).isEqualTo(Boolean.TRUE)); + void testGetOrCreateStudentExamElseThrow_tutor_skipAlert() { + assertThatThrownBy(() -> examAccessService.getOrCreateStudentExamElseThrow(course1.getId(), exam1.getId())).isInstanceOf(AccessForbiddenException.class); } }