Development: Split participation resource
#11460
Conversation
github-actions
bot
added
server
WalkthroughAdds new REST controllers for participation retrieval, deletion, and updates; introduces QuizParticipationService for quiz-specific participation responses; centralizes authorization via ParticipationAuthorizationService; refactors ParticipationResource to a leaner dependency set and scope; removes logger from ParticipationService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant PR as ParticipationRetrievalResource
participant ER as ExerciseRepository
participant PS as ParticipationService
participant QPS as QuizParticipationService
U->>PR: GET /api/exercise/exercises/{exerciseId}/participation
PR->>ER: findById(exerciseId)
alt QuizExercise
PR->>QPS: participationForQuizExercise(quiz, user)
QPS->>QPS: Determine state (not started / started / ended)
alt Not started
QPS->>QPS: Filter data, attach batches, remaining attempts
QPS-->>PR: MappingJacksonValue(participation minimal)
else Started
QPS->>QPS: Load with questions, filter, attach submissions/results
QPS-->>PR: MappingJacksonValue(appropriate DTO)
else Ended
QPS->>QPS: Load full, enrich with submission/result
QPS-->>PR: MappingJacksonValue(full participation)
end
else Non-quiz
PR->>PS: findOneByExerciseAndCurrentUser(...)
PS-->>PR: StudentParticipation
end
PR-->>U: 200 OK (MappingJacksonValue or entity)
sequenceDiagram
autonumber
actor Admin as Instructor/TA
participant PDR as ParticipationDeletionResource
participant SPR as StudentParticipationRepository
participant PAS as ParticipationAuthorizationService
participant PDS as ParticipationDeletionService
participant FTS as FeatureToggleService
Admin->>PDR: DELETE /api/exercise/participations/{id}
PDR->>SPR: findByIdWithExerciseAndStudent(id)
PDR->>PAS: checkAccessPermissionAtLeastInstructor(participation, user)
alt ProgrammingParticipation
PDR->>FTS: check(ProgrammingExercises)
end
PDR->>PDS: delete(participation)
PDR-->>Admin: 200 OK
sequenceDiagram
autonumber
actor TA as TA/Instructor
participant PUR as ParticipationUpdateResource
participant ER as ExerciseRepository
participant SPR as StudentParticipationRepository
participant PS as ParticipationService
participant PES as ProgrammingExerciseParticipationService
TA->>PUR: PUT /api/exercise/exercises/{exerciseId}/participations/update-individual-due-date
PUR->>ER: findById(exerciseId)
PUR->>PS: updateDueDates(participations, newDueDate)
PS-->>PUR: Updated participations
opt Programming exercise
PUR->>PES: updateScheduling(exercise)
end
PUR-->>TA: 200 OK (updated participations)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PMD (7.17.0)src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java[ERROR] Error at ruleset.xml:58:5 59| ... [truncated 11412 characters] ... prone.xml/InaccurateNumericLiteral instead of the deprecated Rule name category/ecmascript/errorprone.xml/InnaccurateNumericLiteral. PMD 8.0.0 will remove support for this deprecated Rule name usage. 185| src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java[ERROR] Error at ruleset.xml:58:5 59| ... [truncated 11412 characters] ... prone.xml/InaccurateNumericLiteral instead of the deprecated Rule name category/ecmascript/errorprone.xml/InnaccurateNumericLiteral. PMD 8.0.0 will remove support for this deprecated Rule name usage. 185| src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java[ERROR] Error at ruleset.xml:58:5 59| ... [truncated 11412 characters] ... prone.xml/InaccurateNumericLiteral instead of the deprecated Rule name category/ecmascript/errorprone.xml/InnaccurateNumericLiteral. PMD 8.0.0 will remove support for this deprecated Rule name usage. 185| Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/quality.ymlis excluded by!**/*.yml
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationUpdateResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationUpdateResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧠 Learnings (3)
📚 Learning: 2025-02-11T12:05:49.151Z
Learnt from: janthoXO
PR: ls1intum/Artemis#9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
📚 Learning: 2025-06-17T12:31:09.178Z
Learnt from: jfr2102
PR: ls1intum/Artemis#10983
File: src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java:110-126
Timestamp: 2025-06-17T12:31:09.178Z
Learning: The query `findByExamIdWithEagerLatestLegalSubmissionsRatedResultAndIgnoreTestRunParticipation` in StudentParticipationRepository fetches all rated results (not just the latest) because the second correction round feature requires access to multiple assessment results per submission for proper correction round management.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java
📚 Learning: 2025-08-26T13:23:05.331Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#11318
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java:549-552
Timestamp: 2025-08-26T13:23:05.331Z
Learning: The method findGradeScoresForAllExercisesForCourseAndStudent in StudentParticipationRepository handles both individual and team exercises by combining results from separate queries for individual grades, individual quiz grades, and team grades.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧬 Code graph analysis (4)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (1)
Profile(38-169)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-439)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java (1)
Profile(51-243)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationUpdateResource.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-439)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Analyse
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationUpdateResource.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (1)
98-120: Race condition: Re-check quiz ended status before filtering.This is the same issue flagged in the previous review. After reloading the quiz at line 98, you call
filterForStudentsDuringQuiz()at line 100 before re-checking whether the quiz has ended. If the quiz transitions from "running" to "ended" between the initial check at line 82 and the reload at line 98, students lose solution data even though the quiz is already finished.Move the
isQuizEnded()check to immediately after the reload and only filter when the quiz is still running.Apply the fix suggested in the previous review:
quizExercise = quizExerciseRepository.findByIdWithQuestionsElseThrow(quizExercise.getId()); quizExercise.setQuizBatches(quizBatch.stream().collect(Collectors.toSet())); -quizExercise.filterForStudentsDuringQuiz(); +boolean quizEnded = quizExercise.isQuizEnded(); +if (!quizEnded) { + quizExercise.filterForStudentsDuringQuiz(); +} StudentParticipation participation = participationForQuizWithSubmissionAndResult(quizExercise, user.getLogin(), quizBatch.get()); -// TODO: Duplicate Object responseDTO = null; if (participation != null) { var submissions = submissionRepository.findAllWithResultsByParticipationIdOrderBySubmissionDateAsc(participation.getId()); participation.setSubmissions(new HashSet<>(submissions)); - if (quizExercise.isQuizEnded()) { + if (quizEnded) {
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (2)
71-80: Address TODO: Consider replacing MappingJacksonValue with a proper DTO.The TODO comment on line 79 indicates that
MappingJacksonValueshould be replaced with a proper DTO or interface. This would improve type safety and API clarity.Would you like me to help design a proper DTO hierarchy to replace
MappingJacksonValue? I can propose a solution that handles the three quiz states (ended, active, not-started) with type-safe DTOs.
103-119: Extract duplicated DTO creation logic into a shared helper methodDuplicate code in
- src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java:103–119
- src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java:116–123
both load submissions and choose the appropriate StudentQuizParticipation*DTO; consolidate into a common method to eliminate this duplication.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java (1)
Profile(51-243)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: client-style
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java (1)
35-69: LGTM: Clean service structure with proper dependency injection.The service follows Spring and coding guidelines with constructor injection, private final fields, and appropriate annotations.
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/QuizParticipationService.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
SamuelRoettgermann
left a comment
SamuelRoettgermann
left a comment
There was a problem hiding this comment.
Please adapt the JavaDocs as described, otherwise it's just moving code, so nothing really happening here.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
424-437: Extract duplicated helper methods to a shared utility or service.These helper methods are duplicated in
ParticipationDeletionResource.java(lines 135-146). Both resources share identical implementations ofcheckAccessPermissionOwner(here) /checkAccessPermissionAtLeastInstructor(in DeletionResource) andfindCourseFromParticipation.Since this refactor spans multiple files and addresses code duplication introduced by the resource split, consider extracting these methods to a shared component such as:
- A dedicated
ParticipationAuthorizationHelperservice- The existing
ParticipationAuthorizationCheckServiceif one exists- A package-private utility class if the methods don't require dependency injection beyond what's already available
This would centralize the authorization logic and maintain DRY principles across the newly split resources.
As per coding guidelines (no_duplication principle).
♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (2)
42-44: The JavaDoc has already been flagged by a previous reviewer as needing improvement. The current description "REST controller for deleting a participation and cleaning up a build plan for a participation" is more specific than "Managing participation," but could still be more concise and action-oriented.
123-133: The unguarded cast creates a ClassCastException risk.This issue has already been flagged in a previous review. When
studentParticipationRepository.findByIdElseThrow(participationId)returns a non-programming participation, the cast on line 127 will throwClassCastException, resulting in a 500 error instead of a controlled 400 response.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧠 Learnings (1)
📚 Learning: 2025-08-26T13:23:05.331Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#11318
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java:549-552
Timestamp: 2025-08-26T13:23:05.331Z
Learning: The method findGradeScoresForAllExercisesForCourseAndStudent in StudentParticipationRepository handles both individual and team exercises by combining results from separate queries for individual grades, individual quiz grades, and team grades.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-439)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (2)
72-74: LGTM! JavaDoc improvement addresses previous feedback.The updated JavaDoc "REST controller for starting or resuming participations, as well as requesting feedback for one" is clearer and more specific than the previous version, directly addressing the reviewer's feedback.
95-122: LGTM! Constructor refactoring aligns with resource split.The constructor changes properly reflect the PR's goal of splitting responsibilities:
- Removed deletion-related dependencies (participationDeletionService, auditEventRepository)
- Removed quiz-specific dependencies (quizExerciseRepository, quizSubmissionService, quizBatchService, submittedAnswerRepository)
- Added feedback-related dependencies (programmingExerciseCodeReviewFeedbackService, textFeedbackApi, modelingExerciseFeedbackService, studentExamApi)
- All new fields are properly initialized
This aligns with the introduction of
ParticipationDeletionResourceandQuizParticipationServicementioned in the PR objectives.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (1)
122-132: The unsafe cast issue from the previous review remains unresolved.Line 126 still contains an unguarded cast from
StudentParticipationtoProgrammingExerciseStudentParticipation. If a caller passes the ID of a non-programming participation (e.g., text/modeling), this will throwClassCastExceptionand return HTTP 500 instead of a controlled 400 error.Apply the fix suggested in the previous review:
- ProgrammingExerciseStudentParticipation participation = (ProgrammingExerciseStudentParticipation) studentParticipationRepository.findByIdElseThrow(participationId); + StudentParticipation rawParticipation = studentParticipationRepository.findByIdElseThrow(participationId); + if (!(rawParticipation instanceof ProgrammingExerciseStudentParticipation participation)) { + throw new BadRequestAlertException("Cleanup is only supported for programming participations", "participation", "invalidParticipationType"); + } User user = userRepository.getUserWithGroupsAndAuthorities(); participationAuthorizationHelper.checkAccessPermissionAtLeastInstructor(participation, user);
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (1)
103-112: Add JavaDoc to the private helper method.The private
deleteParticipationmethod lacks documentation. While it's internal, documenting its purpose, parameters, and return value improves maintainability.+ /** + * Deletes the participation, including build plan and repository if applicable, and logs the event in the audit database. + * + * @param participation the participation to be deleted + * @param user the currently logged-in user who initiated the delete operation + * @return the response to the client with deletion confirmation headers + */ @NotNull private ResponseEntity<Void> deleteParticipation(StudentParticipation participation, User user) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationHelper.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationHelper.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧬 Code graph analysis (2)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationHelper.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (1)
Profile(43-134)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-428)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-428)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
- GitHub Check: submit-gradle
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
56-56: LGTM! Authorization refactoring successfully centralizes access control.The refactoring to use
ParticipationAuthorizationHelpersuccessfully centralizes authorization logic:
- Constructor properly injects the helper (lines 119-125, 141)
- Authorization checks at lines 255 and 320 now delegate to the helper instead of inline logic
- This reduces code duplication and makes authorization patterns more consistent across resources
The changes align with the DRY principle and improve maintainability.
Based on coding guidelines (no_duplication, code_reuse principles).
Also applies to: 99-99, 119-125, 141-141, 255-255, 320-320
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java
Show resolved
Hide resolved
SamuelRoettgermann
left a comment
SamuelRoettgermann
left a comment
There was a problem hiding this comment.
look at the comment, but generally it lgtm
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationHelper.java
Outdated
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
florian-glombik
left a comment
florian-glombik
left a comment
There was a problem hiding this comment.
Approve the restructuring, thanks for introducing some helper methods
florian-glombik
left a comment
florian-glombik
left a comment
There was a problem hiding this comment.
Some checks are failing that appear to be related to the changes in this PR
tobias-lippert
dismissed stale reviews from SamuelRoettgermann and coderabbitai[bot]
via
a515fd7
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java (2)
50-55: Ensure associations are fetched and null-checked.
- studentParticipationRepository.findByIdElseThrow uses default lazy loading—add an @entitygraph or JOIN FETCH query on the repository method to load exercise and its course in one call.
- After loading, guard against missing associations: check loaded.getExercise() and loaded.getExercise().getCourseViaExerciseGroupOrCourseMember() and throw a clear exception if null.
63-68: Use the two-argisOwnerOfParticipationoverload for consistency.ParticipationAuthorizationService.java
- if (!authCheckService.isOwnerOfParticipation(participation)) { + if (!authCheckService.isOwnerOfParticipation(participation, user)) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
🧠 Learnings (1)
📚 Learning: 2025-02-11T12:05:49.151Z
Learnt from: janthoXO
PR: ls1intum/Artemis#9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(8-526)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationDeletionResource.java (1)
Profile(43-134)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (1)
Profile(75-428)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (13)
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java (4)
19-31: LGTM!The service correctly uses constructor injection, and all fields are properly declared as final with appropriate Spring annotations (@service, @lazy, @Profile).
50-55: Fix potential null return when course is not loaded.The condition on line 51 checks if
participation.getExercise()is not null, but ifgetCourseViaExerciseGroupOrCourseMember()returns null, the method returns null at line 52 without attempting the fallback repository fetch. The method should only return early if the course is actually present.Apply this diff to fix the logic:
public Course findCourseFromParticipation(StudentParticipation participation) { - if (participation.getExercise() != null && participation.getExercise().getCourseViaExerciseGroupOrCourseMember() != null) { - return participation.getExercise().getCourseViaExerciseGroupOrCourseMember(); + if (participation.getExercise() != null) { + Course course = participation.getExercise().getCourseViaExerciseGroupOrCourseMember(); + if (course != null) { + return course; + } } return studentParticipationRepository.findByIdElseThrow(participation.getId()).getExercise().getCourseViaExerciseGroupOrCourseMember(); }⛔ Skipped due to learnings
Learnt from: tobias-lippert PR: ls1intum/Artemis#11318 File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java:549-552 Timestamp: 2025-08-26T13:23:05.331Z Learning: The method findGradeScoresForAllExercisesForCourseAndStudent in StudentParticipationRepository handles both individual and team exercises by combining results from separate queries for individual grades, individual quiz grades, and team grades.
39-42: LGTM!The method correctly delegates course resolution and authorization checks. JavaDoc is clear and complete.
50-55: LGTM!The defensive pattern correctly handles both fully-loaded and partially-initialized participations by falling back to a repository fetch when needed.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java (9)
56-56: LGTM!The import statement and field declarations properly follow the constructor injection pattern with private final fields. The addition of
ParticipationAuthorizationServicealigns with the PR's goal of centralizing authorization logic.Also applies to: 93-117
119-143: LGTM!The constructor properly implements dependency injection following the coding guidelines. All dependencies, including the newly added
ParticipationAuthorizationService, are correctly injected and assigned.
255-255: LGTM!The authorization check correctly delegates to the centralized
ParticipationAuthorizationService, following the refactoring pattern of moving authorization logic out of the resource class.
320-320: LGTM!The authorization check correctly delegates to the centralized
ParticipationAuthorizationService, consistent with the refactoring pattern applied throughout the PR.
72-74: LGTM!The JavaDoc clearly describes the controller's focused responsibilities after the refactoring, addressing the previous review feedback about improving the documentation.
119-143: Constructor properly integrates new authorization service.The refactoring correctly adds
ParticipationAuthorizationServicevia constructor injection, following the project's dependency injection guidelines. The centralized authorization approach improves maintainability.
56-56: LGTM!The addition of
ParticipationAuthorizationServiceas a dependency properly supports the centralized authorization pattern introduced by this PR.Also applies to: 93-117
119-125: LGTM!Constructor properly injects
ParticipationAuthorizationServicefollowing the project's constructor injection pattern.Also applies to: 141-141
255-255: LGTM!The refactoring successfully centralizes authorization checks in
ParticipationAuthorizationService, improving code reuse and maintainability.Also applies to: 320-320
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationAuthorizationService.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
florian-glombik
left a comment
florian-glombik
left a comment
There was a problem hiding this comment.
Code, the build failure seems unrelated and to be caused by the pipeline
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
Checklist
General
Server
Motivation and Context
ParticipationResource is our class with the most dependencies
Description
Extracted a QuizParticipationService and split the ParticipationResource into ParticipationResource, ParticipationUpdateResource and ParticipationRetrievalResource
Steps for Testing
Code Review
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Test Coverage
unchanged
Summary by CodeRabbit
New Features
Refactor
Chores