Skip to content

Commit

Permalink
Development: Improve error handling when generating student exams
Browse files Browse the repository at this point in the history
  • Loading branch information
@krusche
krusche committed Dec 15, 2024
1 parent 5c11585 commit c4bc5a8
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 41 deletions.
39 changes: 24 additions & 15 deletions src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -79,30 +79,39 @@ public StudentExam getExamInCourseElseThrow(Long courseId, Long examId) {
Optional<StudentExam> 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);
}
}

Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/de/tum/cit/aet/artemis/exam/web/ExamResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1101,7 +1101,7 @@ public ResponseEntity<Void> 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
Expand All @@ -1111,7 +1111,7 @@ public ResponseEntity<Void> removeAllStudentsFromExam(@PathVariable Long courseI
@EnforceAtLeastStudent
public ResponseEntity<StudentExam> 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);
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down Expand Up @@ -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
Expand Down
13 changes: 12 additions & 1 deletion src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
38 changes: 18 additions & 20 deletions src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,27 +329,27 @@ 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
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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);
}

Expand All @@ -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);
}
}

0 comments on commit c4bc5a8

Please sign in to comment.