Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Split logic of ExerciseResource into endpoints per exercise-type #9570

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.springframework.web.servlet.support.ServletUriComponentsBuilder;

import de.tum.cit.aet.artemis.assessment.domain.TutorParticipation;
import de.tum.cit.aet.artemis.assessment.repository.GradingCriterionRepository;
import de.tum.cit.aet.artemis.assessment.repository.TutorParticipationRepository;
import de.tum.cit.aet.artemis.assessment.service.AssessmentDashboardService;
import de.tum.cit.aet.artemis.communication.domain.conversation.Channel;
Expand Down Expand Up @@ -107,6 +108,7 @@
import de.tum.cit.aet.artemis.exam.service.ExamSessionService;
import de.tum.cit.aet.artemis.exam.service.ExamUserService;
import de.tum.cit.aet.artemis.exam.service.StudentExamService;
import de.tum.cit.aet.artemis.exercise.api.ExerciseApi;
import de.tum.cit.aet.artemis.exercise.domain.Exercise;
import de.tum.cit.aet.artemis.exercise.domain.Submission;
import de.tum.cit.aet.artemis.exercise.dto.ExerciseForPlagiarismCasesOverviewDTO;
Expand Down Expand Up @@ -178,12 +180,17 @@ public class ExamResource {

private final ExamUserService examUserService;

private final GradingCriterionRepository gradingCriterionRepository;

private final ExerciseApi exerciseApi;

public ExamResource(UserRepository userRepository, CourseRepository courseRepository, ExamService examService, ExamDeletionService examDeletionService,
ExamAccessService examAccessService, InstanceMessageSendService instanceMessageSendService, ExamRepository examRepository, SubmissionService submissionService,
AuthorizationCheckService authCheckService, ExamDateService examDateService, TutorParticipationRepository tutorParticipationRepository,
AssessmentDashboardService assessmentDashboardService, ExamRegistrationService examRegistrationService, ExamImportService examImportService,
CustomAuditEventRepository auditEventRepository, ChannelService channelService, ChannelRepository channelRepository, ExerciseRepository exerciseRepository,
ExamSessionService examSessionRepository, ExamLiveEventsService examLiveEventsService, StudentExamService studentExamService, ExamUserService examUserService) {
ExamSessionService examSessionRepository, ExamLiveEventsService examLiveEventsService, StudentExamService studentExamService, ExamUserService examUserService,
GradingCriterionRepository gradingCriterionRepository, ExerciseApi exerciseApi) {
this.userRepository = userRepository;
this.courseRepository = courseRepository;
this.examService = examService;
Expand All @@ -206,6 +213,8 @@ public ExamResource(UserRepository userRepository, CourseRepository courseReposi
this.examLiveEventsService = examLiveEventsService;
this.studentExamService = studentExamService;
this.examUserService = examUserService;
this.gradingCriterionRepository = gradingCriterionRepository;
this.exerciseApi = exerciseApi;
}

/**
Expand Down Expand Up @@ -303,6 +312,45 @@ public ResponseEntity<Exam> updateExam(@PathVariable Long courseId, @RequestBody
return ResponseEntity.ok(savedExam);
}

/**
* GET /exercises/:exerciseId : get an exam exercise by id.
* Users with at least editor rights can always access the exercise.
* Users with at least tutor rights can only access the exercise after the it has been finished.
* Students cannot see the exercise.
*
* @param exerciseId the exerciseId of the exercise to retrieve
* @return the ResponseEntity with status 200 (OK) and with body the exercise, or with status 404 (Not Found)
*/
@GetMapping("exam/exercises/{exerciseId}")
@EnforceAtLeastEditor
public ResponseEntity<Exercise> getExamExercise(@PathVariable Long exerciseId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependency from exam->specific exercise should be fine for now, as one needs to know which exercises are created for an exam

log.debug("REST request to get Exam Exercise : {}", exerciseId);

User user = userRepository.getUserWithGroupsAndAuthorities();
Exercise exercise = exerciseRepository.findByIdWithCategoriesAndTeamAssignmentConfigElseThrow(exerciseId);

Exam exam = exercise.getExerciseGroup().getExam();
if (authCheckService.isAtLeastEditorForExercise(exercise, user)) {
// instructors editors and admins should always be able to see exam exercises
// continue
}
else if (authCheckService.isAtLeastTeachingAssistantForExercise(exercise, user)) {
// tutors should only be able to see exam exercises when the exercise has finished
ZonedDateTime latestIndividualExamEndDate = examDateService.getLatestIndividualExamEndDate(exam);
if (latestIndividualExamEndDate == null || latestIndividualExamEndDate.isAfter(ZonedDateTime.now())) {
// When there is no due date or the due date is in the future, we return forbidden here
throw new AccessForbiddenException();
}
}
else {
// Students should never access exercises
throw new AccessForbiddenException();
}

exerciseApi.setGradingCriteria(exercise);
return ResponseEntity.ok(exercise);
}

/**
* PATCH /courses/{courseId}/exams/{examId}/working-time : Update the working time of all exams
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package de.tum.cit.aet.artemis.exercise.api;

import java.util.Set;

import org.springframework.stereotype.Controller;

import de.tum.cit.aet.artemis.assessment.domain.ExampleSubmission;
import de.tum.cit.aet.artemis.assessment.domain.GradingCriterion;
import de.tum.cit.aet.artemis.assessment.repository.ExampleSubmissionRepository;
import de.tum.cit.aet.artemis.assessment.repository.GradingCriterionRepository;
import de.tum.cit.aet.artemis.communication.domain.conversation.Channel;
import de.tum.cit.aet.artemis.communication.repository.conversation.ChannelRepository;
import de.tum.cit.aet.artemis.core.domain.User;
import de.tum.cit.aet.artemis.core.service.AuthorizationCheckService;
import de.tum.cit.aet.artemis.exercise.domain.Exercise;
import de.tum.cit.aet.artemis.exercise.service.ExerciseService;

// This is a very simple implementation, and should pbl be split up
@Controller
public class ExerciseApi {

// ToDo: Should be some kind of ChannelApi
private final ChannelRepository channelRepository;

// ToDo: Should be some kind of AssessmentApi
private final GradingCriterionRepository gradingCriterionRepository;

// ToDo: Should be some kind of AssessmentApi
private final ExampleSubmissionRepository exampleSubmissionRepository;

private final AuthorizationCheckService authCheckService;

private final ExerciseService exerciseService;

public ExerciseApi(AuthorizationCheckService authCheckService, ChannelRepository channelRepository, GradingCriterionRepository gradingCriterionRepository,
ExerciseService exerciseService, ExampleSubmissionRepository exampleSubmissionRepository) {
this.authCheckService = authCheckService;
this.channelRepository = channelRepository;
this.gradingCriterionRepository = gradingCriterionRepository;
this.exerciseService = exerciseService;
this.exampleSubmissionRepository = exampleSubmissionRepository;
}

public boolean isStudentAllowedToSee(Exercise exercise) {
return isStudentAllowedToSee(exercise, null);
}

public boolean isStudentAllowedToSee(Exercise exercise, User user) {
if (exercise.isExamExercise()) {
return false;
}

return authCheckService.isAllowedToSeeExercise(exercise, user);
}

public boolean isCorrectExerciseType(Exercise exercise, Class<? extends Exercise> expectedExerciseClass) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be needed here, we don't gain a lot from sharing this method.

return expectedExerciseClass.isInstance(exercise);
}

public void setChannelName(Exercise exercise) {
Copy link
Contributor Author

@ole-ve ole-ve Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to improve the naming here. For me, it's something similar to hydrateWithXY or loadAndSetXY to express that we are loading something from the DB.

Channel channel = channelRepository.findChannelByExerciseId(exercise.getId());
if (channel != null) {
exercise.setChannelName(channel.getName());
}
}

public void setGradingCriteria(Exercise exercise) {
Set<GradingCriterion> gradingCriteria = gradingCriterionRepository.findByExerciseIdWithEagerGradingCriteria(exercise.getId());
exercise.setGradingCriteria(gradingCriteria);
}

public void checkExerciseIfStructuredGradingInstructionFeedbackUsed(Set<GradingCriterion> gradingCriteria, Exercise exercise) {
exerciseService.checkExerciseIfStructuredGradingInstructionFeedbackUsed(gradingCriteria, exercise);
}

public void setExampleSubmissions(Exercise exercise) {
Set<ExampleSubmission> exampleSubmissions = this.exampleSubmissionRepository.findAllWithResultByExerciseId(exercise.getId());
exercise.setExampleSubmissions(exampleSubmissions);
}

public void filterSensitiveData(Exercise exercise) {
exercise.filterSensitiveInformation();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_CORE;

import java.io.IOException;
import java.time.ZonedDateTime;
import java.util.Collections;
import java.util.HashSet;
Expand All @@ -10,6 +11,11 @@
import java.util.Set;
import java.util.stream.Collectors;

import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.annotation.Profile;
Expand Down Expand Up @@ -38,10 +44,9 @@
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastInstructor;
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastStudent;
import de.tum.cit.aet.artemis.core.security.annotations.EnforceAtLeastTutor;
import de.tum.cit.aet.artemis.core.security.annotations.enforceRoleInExercise.EnforceAtLeastStudentInExercise;
import de.tum.cit.aet.artemis.core.service.AuthorizationCheckService;
import de.tum.cit.aet.artemis.exam.domain.Exam;
import de.tum.cit.aet.artemis.exam.service.ExamAccessService;
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.participation.StudentParticipation;
import de.tum.cit.aet.artemis.exercise.dto.ExerciseDetailsDTO;
Expand Down Expand Up @@ -86,8 +91,6 @@ public class ExerciseResource {

private final TutorParticipationService tutorParticipationService;

private final ExamDateService examDateService;

private final GradingCriterionRepository gradingCriterionRepository;

private final ExampleSubmissionRepository exampleSubmissionRepository;
Expand All @@ -107,7 +110,7 @@ public class ExerciseResource {
private final ExerciseHintService exerciseHintService;

public ExerciseResource(ExerciseService exerciseService, ExerciseDeletionService exerciseDeletionService, ParticipationService participationService,
UserRepository userRepository, ExamDateService examDateService, AuthorizationCheckService authCheckService, TutorParticipationService tutorParticipationService,
UserRepository userRepository, AuthorizationCheckService authCheckService, TutorParticipationService tutorParticipationService,
ExampleSubmissionRepository exampleSubmissionRepository, ProgrammingExerciseRepository programmingExerciseRepository,
GradingCriterionRepository gradingCriterionRepository, ExerciseRepository exerciseRepository, QuizBatchService quizBatchService,
ParticipationRepository participationRepository, ExamAccessService examAccessService, Optional<IrisSettingsService> irisSettingsService,
Expand All @@ -120,7 +123,6 @@ public ExerciseResource(ExerciseService exerciseService, ExerciseDeletionService
this.tutorParticipationService = tutorParticipationService;
this.exampleSubmissionRepository = exampleSubmissionRepository;
this.gradingCriterionRepository = gradingCriterionRepository;
this.examDateService = examDateService;
this.exerciseRepository = exerciseRepository;
this.programmingExerciseRepository = programmingExerciseRepository;
this.quizBatchService = quizBatchService;
Expand All @@ -138,47 +140,19 @@ public ExerciseResource(ExerciseService exerciseService, ExerciseDeletionService
* @return the ResponseEntity with status 200 (OK) and with body the exercise, or with status 404 (Not Found)
*/
@GetMapping("exercises/{exerciseId}")
@EnforceAtLeastStudent
public ResponseEntity<Exercise> getExercise(@PathVariable Long exerciseId) {

log.debug("REST request to get Exercise : {}", exerciseId);
@EnforceAtLeastStudentInExercise
public ResponseEntity<Exercise> getExercise(@PathVariable Long exerciseId, HttpServletRequest request, HttpServletResponse response) {

User user = userRepository.getUserWithGroupsAndAuthorities();
Exercise exercise = exerciseRepository.findByIdWithCategoriesAndTeamAssignmentConfigElseThrow(exerciseId);
log.debug("REST request to get generic Exercise : {}", exerciseId);

// Exam exercise
Exercise exercise = exerciseRepository.findByIdElseThrow(exerciseId);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could reduce the repository call to just fetch the exercise type

if (exercise.isExamExercise()) {
Exam exam = exercise.getExerciseGroup().getExam();
if (authCheckService.isAtLeastEditorForExercise(exercise, user)) {
// instructors editors and admins should always be able to see exam exercises
// continue
}
else if (authCheckService.isAtLeastTeachingAssistantForExercise(exercise, user)) {
// tutors should only be able to see exam exercises when the exercise has finished
ZonedDateTime latestIndividualExamEndDate = examDateService.getLatestIndividualExamEndDate(exam);
if (latestIndividualExamEndDate == null || latestIndividualExamEndDate.isAfter(ZonedDateTime.now())) {
// When there is no due date or the due date is in the future, we return forbidden here
throw new AccessForbiddenException();
}
}
else {
// Students should never access exercises
throw new AccessForbiddenException();
}
}
// Normal exercise
else {
if (!authCheckService.isAllowedToSeeExercise(exercise, user)) {
throw new AccessForbiddenException();
}
if (!authCheckService.isAtLeastTeachingAssistantForExercise(exercise, user)) {
exercise.filterSensitiveInformation();
}
return forwardToExamExerciseURL(request, response, exerciseId);
}

Set<GradingCriterion> gradingCriteria = gradingCriterionRepository.findByExerciseIdWithEagerGradingCriteria(exerciseId);
exercise.setGradingCriteria(gradingCriteria);
return ResponseEntity.ok(exercise);
// Regular exercise
return forwardToExerciseURL(request, response, exercise);
}

/**
Expand Down Expand Up @@ -387,4 +361,48 @@ public ResponseEntity<ZonedDateTime> getLatestDueDate(@PathVariable Long exercis
authCheckService.checkHasAtLeastRoleForExerciseElseThrow(Role.STUDENT, exercise, null);
return ResponseEntity.ok(participationRepository.findLatestIndividualDueDate(exerciseId).orElse(exercise.getDueDate()));
}

private ResponseEntity<Exercise> forwardToExamExerciseURL(HttpServletRequest request, HttpServletResponse response, Long exerciseId) {
String url = String.format("/api/exams/exercises/%s", exerciseId);
return forwardToUrl(request, response, url);
}

/**
* Handles the forwarding to the correct exercise URL based on the exercise type.
* ToDo: Consider already throwing an error here, if a module is not present. Otherwise, it produces a 404
*/
private ResponseEntity<Exercise> forwardToExerciseURL(HttpServletRequest request, HttpServletResponse response, Exercise exercise) {
Long exerciseId = exercise.getId();
String url = switch (exercise.getExerciseType()) {
case PROGRAMMING -> "/api/programming-exercises/" + exerciseId;
case QUIZ -> "/api/quiz-exercises/" + exerciseId;
case MODELING -> "/api/modeling-exercises/" + exerciseId;
case TEXT -> "/api/text-exercises/" + exerciseId;
case FILE_UPLOAD -> "/api/file-upload-exercises/" + exerciseId;
};

// For students, call a separate endpoint
if (!authCheckService.isAtLeastTeachingAssistantInExercise(exerciseId)) {
url += "/for-students";
}

return forwardToUrl(request, response, url);
}

// ToDo: Move to common class
// ToDo: Consider using redirects (302) instead of internal forwards. An exercise with the same ID won't change its type
private <T> ResponseEntity<T> forwardToUrl(HttpServletRequest request, HttpServletResponse response, String url) {
RequestDispatcher dispatcher = request.getRequestDispatcher(url);
try {
dispatcher.forward(request, response);
}
catch (ServletException e) {
throw new RuntimeException(e); // ToDo: Handle gracefully
}
catch (IOException e) {
throw new RuntimeException(e); // ToDo: Handle gracefully
}

return null; // Will be overridden by response
}
}
Loading
Loading