-
Notifications
You must be signed in to change notification settings - Fork 291
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -86,8 +91,6 @@ public class ExerciseResource { | |
|
||
private final TutorParticipationService tutorParticipationService; | ||
|
||
private final ExamDateService examDateService; | ||
|
||
private final GradingCriterionRepository gradingCriterionRepository; | ||
|
||
private final ExampleSubmissionRepository exampleSubmissionRepository; | ||
|
@@ -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, | ||
|
@@ -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; | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
/** | ||
|
@@ -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 | ||
} | ||
} |
There was a problem hiding this comment.
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