Development: Remove quiz view and switch to DTOs
#10571
Conversation
# Conflicts: # src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizExercise.java
github-actions
bot
added
server
KonstiAnon
changed the title
Quiz exercises: Remove QuizView and switch to DTOs
KonstiAnon
changed the title
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
There was a problem hiding this comment.
This file is specific to quiz exercises and should therefore be within the quiz module
| protected Course getCourse() { | ||
| public Course getCourse() { |
| @@ -798,6 +802,7 @@ public ResponseEntity<MappingJacksonValue> getParticipationForCurrentUser(@PathV | |||
| } | |||
|
|
|||
| @Nullable | |||
| // TODO: use a proper DTO (or interface here for the return type and avoid MappingJacksonValue) | |||
| private MappingJacksonValue participationForQuizExercise(QuizExercise quizExercise, User user) { | |||
There was a problem hiding this comment.
In this method you still return a raw StudentParticipation in Line 822.
| Object responseDTO = null; | ||
| if (participation != null) { | ||
| if (quizExercise.isQuizEnded()) { | ||
| responseDTO = StudentQuizParticipationWithSolutionsDTO.of(participation); | ||
| } | ||
| else if (quizBatch.get().isStarted()) { | ||
| responseDTO = StudentQuizParticipationWithQuestionsDTO.of(participation); | ||
| } | ||
| else { | ||
| responseDTO = StudentQuizParticipationWithoutQuestionsDTO.of(participation); | ||
| } | ||
| } | ||
|
|
||
| return responseDTO != null ? new MappingJacksonValue(responseDTO) : null; |
There was a problem hiding this comment.
These null checks is either redundant or you should probably throw a proper exception
| @@ -126,7 +119,8 @@ public void setSimilarityValue(Integer similarityValue) { | |||
| this.similarityValue = similarityValue; | |||
| } | |||
|
|
|||
| public Boolean matchLetterCase() { | |||
| @JsonInclude | |||
There was a problem hiding this comment.
Is this needed? Might make more sense to put it at the attribute
| @@ -0,0 +1,5 @@ | |||
| package de.tum.cit.aet.artemis.quiz.dto.participation; | |||
|
|
|||
| public interface StudentQuizParticipation { | |||
There was a problem hiding this comment.
Rename this to StudentQuizParticipationDTO for now. We'll think of a possible better naming later.
| public static StudentQuizParticipationWithQuestionsDTO of(final StudentParticipation studentParticipation) { | ||
| Exercise participationExercise = studentParticipation.getExercise(); | ||
| if (!(participationExercise instanceof QuizExercise quizExercise)) { | ||
| // TODO: Figure out error handling here |
There was a problem hiding this comment.
Comment just to track this for a follow up
| import de.tum.cit.aet.artemis.quiz.domain.QuizQuestion; | ||
| import de.tum.cit.aet.artemis.quiz.domain.ShortAnswerQuestion; | ||
|
|
||
| // Note: Only one of the three questions will be non-null depending on the question type |
There was a problem hiding this comment.
Please add a more detailed JavaDoc describing this "hack"
| StudentQuizParticipation responseDTO; | ||
| if (exercise.isQuizEnded()) { | ||
| responseDTO = StudentQuizParticipationWithSolutionsDTO.of(participation); | ||
| } | ||
| else if (quizBatch.isPresent() && quizBatch.get().isStarted()) { | ||
| responseDTO = StudentQuizParticipationWithQuestionsDTO.of(participation); | ||
| } | ||
| else { | ||
| responseDTO = StudentQuizParticipationWithoutQuestionsDTO.of(participation); | ||
| } |
There was a problem hiding this comment.
See if this can be moved into a method in the QuizExercise
KonstiAnon
dismissed stale reviews from AjayvirS, krusche, raffifasaro, and coderabbitai[bot]
via
b2b6b1f
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithSolutionsDTO.java (1)
25-30:⚠️ Potential issueImplement error handling instead of returning null
The factory method has a TODO comment about error handling but currently returns
nullin case of type mismatch. This could lead to NullPointerExceptions. Consider implementing proper error handling by throwing an appropriate exception.public static StudentQuizParticipationWithSolutionsDTO of(final StudentParticipation studentParticipation) { Exercise participationExercise = studentParticipation.getExercise(); if (!(participationExercise instanceof QuizExercise quizExercise)) { - // TODO: Figure out error handling here - return null; + throw new IllegalArgumentException("Cannot create StudentQuizParticipationWithSolutionsDTO: " + + "The provided participation's exercise is not a QuizExercise"); }src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithQuestionsDTO.java (2)
27-30:⚠️ Potential issueFix the error handling instead of returning null.
The current implementation returns null when the participation exercise is not a QuizExercise. This can lead to NullPointerExceptions downstream. Throw an appropriate exception with a descriptive message instead.
- if (!(participationExercise instanceof QuizExercise quizExercise)) { - // TODO: Figure out error handling here - return null; - } + if (!(participationExercise instanceof QuizExercise quizExercise)) { + throw new IllegalArgumentException("StudentParticipation is not associated with a QuizExercise"); + }
31-33: 🛠️ Refactor suggestionRemove deprecated "Results" mapping now that QuizView is removed.
Since this PR is removing QuizView and the TODO comment indicates that Results mapping should be removed after QuizView is removed, now is the appropriate time to address this TODO.
- // ToDo: Results is deprecated, will be removed after QuizView is removed - return new StudentQuizParticipationWithQuestionsDTO(StudentQuizParticipationBaseDTO.of(studentParticipation), QuizExerciseWithQuestionsDTO.of(quizExercise), - studentParticipation.getResults().stream().map(ResultBeforeEvaluationDTO::of).collect(Collectors.toSet())); + return new StudentQuizParticipationWithQuestionsDTO(StudentQuizParticipationBaseDTO.of(studentParticipation), + QuizExerciseWithQuestionsDTO.of(quizExercise), null);
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (2)
314-317: Consider extracting batch creation logic to a helper method.This block creates a quiz batch and joins it for non-synchronized quiz modes. Since this pattern appears in other test methods (e.g., lines 831-836), consider extracting it to a reusable helper method to avoid duplication.
- if (quizMode != QuizMode.SYNCHRONIZED) { - var batch = quizBatchService.save(QuizExerciseFactory.generateQuizBatch(quizExercise, ZonedDateTime.now().minusSeconds(10))); - request.postWithResponseBody("/api/quiz/quiz-exercises/" + quizExercise.getId() + "/join", new QuizBatchJoinDTO(batch.getPassword()), QuizBatch.class, HttpStatus.OK); - } + if (quizMode != QuizMode.SYNCHRONIZED) { + joinQuizBatch(quizExercise); + }With a helper method like:
private void joinQuizBatch(QuizExercise quizExercise) throws Exception { var batch = quizBatchService.save(QuizExerciseFactory.generateQuizBatch(quizExercise, ZonedDateTime.now().minusSeconds(10))); request.postWithResponseBody("/api/quiz/quiz-exercises/" + quizExercise.getId() + "/join", new QuizBatchJoinDTO(batch.getPassword()), QuizBatch.class, HttpStatus.OK); }
321-344: Enhance test assertions with descriptive messages.The assertions would be more maintainable with descriptive failure messages that explain the expected behavior.
- assertThat(participation).isNotNull(); - Exercise exercise = participation.getExercise(); - assertThat(exercise).isNotNull(); - assertThat(exercise).isInstanceOf(QuizExercise.class); + assertThat(participation).as("Participation should not be null").isNotNull(); + Exercise exercise = participation.getExercise(); + assertThat(exercise).as("Exercise in participation should not be null").isNotNull(); + assertThat(exercise).as("Exercise should be a QuizExercise").isInstanceOf(QuizExercise.class);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithQuestionsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithSolutionsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithoutQuestionsDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationDTO.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithoutQuestionsDTO.java
- src/main/java/de/tum/cit/aet/artemis/quiz/web/QuizParticipationResource.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
src/main/java/**/*.java: 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
src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithSolutionsDTO.javasrc/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithQuestionsDTO.java
🧠 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (2)
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:0-0
Timestamp: 2025-03-22T12:51:55.492Z
Learning: In Artemis, an `ExerciseGroup` always has an associated `Exam`, so `exerciseGroup.exam` is never null.
Learnt from: julian-christl
PR: ls1intum/Artemis#9322
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:170-172
Timestamp: 2025-03-22T12:51:51.144Z
Learning: In Artemis, `exercise.exerciseGroup` may be null, as not all exercises belong to an `ExerciseGroup`.
🧬 Code Definitions (1)
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (1)
src/test/java/de/tum/cit/aet/artemis/quiz/util/QuizExerciseFactory.java (1)
QuizExerciseFactory(49-784)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/quiz/dto/participation/StudentQuizParticipationWithSolutionsDTO.java (2)
1-17: Good use of Java records for DTOs and data minimization.The implementation follows the coding guidelines by using Java records for DTOs, which provides immutability and reduces boilerplate code. The
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation ensures data economy by excluding empty fields from JSON serialization. The record implementation of theStudentQuizParticipationDTOinterface adheres to the single responsibility principle.
31-34: Review the ToDo comment about deprecated ResultsSince this PR aims to remove QuizView, you should revisit this ToDo comment. If QuizView is being removed as part of this PR, then the comment may need to be updated or the deprecated functionality properly addressed.
Additionally, the code follows best practices by using streams for collection transformation and maintains clean, explicit imports as per the coding guidelines.
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (1)
304-345: LGTM! Comprehensive test for quiz participation.The new test method thoroughly verifies that the appropriate data is sent to students when starting a quiz participation. It correctly ensures that solution data (explanations, correct mappings) isn't exposed during the active phase.
KonstiAnon
dismissed stale reviews from florian-glombik and coderabbitai[bot]
via
0eaff4e
Checklist
General
Server
Motivation and Context
Up until now, a JsonView was used in parts of the codebase to filter which information is sent to users based on the current context. This approach has been replaced by the usage of DTOs over time and as such the QuizView has become deprecated. This PR removes the QuizView from the code and replaces its usage in endpoints with new DTOs.
Description
I have replaced the usage of the QuizView in the following functions:
with the usage of DTOs. For this I have implemented a number of DTOs, which currently replicate the exact same data structure as before. There are three situations based on which an appropriate DTO is selected:
The
StudentQuizParticipationWithoutQuestionsDTOis used as a replacement forQuizView.Before, theStudentQuizParticipationWithQuestionsDTOreplacesQuizView.Duringand theStudentQuizParticipationWithSolutionsreplaces theQuizView.After.Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
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
Performance Review
Code Review
Manual Tests
Exam Mode Test
Test Coverage
Summary by CodeRabbit
New Features
Refactor
Tests