Exam mode: Allow submission upon extension
#9833
Conversation
github-actions
bot
added
tests
server
meltemarsl
changed the title
Exam mode: Allow submission upon extension
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (2)
Line range hint
149-171: Constructor Contains Many Parameters Indicating Potential Class Responsibility OverloadThe
StudentExamResourceconstructor now has a large number of parameters (over 20 dependencies), which can make the code harder to maintain and understand. This might indicate that the class is handling too many responsibilities.Consider refactoring the class to adhere to the single responsibility principle by extracting related functionalities into separate services or components. This can improve maintainability and readability.
269-281: Refactor Logic into a Separate Method for Improved ReadabilityThe logic within the
updateWorkingTimemethod, specifically from lines 269 to 281, is complex and involves multiple nested operations. Extracting this block into a separate private method would enhance readability and maintainability.Apply this diff to refactor the code:
if (!studentExam.isEnded() && wasEndedOriginally) { + unlockProgrammingExerciseParticipations(studentExam); } ... + private void unlockProgrammingExerciseParticipations(StudentExam studentExam) { + studentExam.getExercises().stream() + .filter(ProgrammingExercise.class::isInstance) + .forEach(exercise -> { + var participation = programmingExerciseStudentParticipationRepository + .findByExerciseIdAndStudentLogin(exercise.getId(), studentExam.getUser().getLogin()); + var submissionPolicy = ((ProgrammingExercise) exercise).getSubmissionPolicy(); + participation.ifPresent(participationObj -> { + long inTimeSubmissions = participationObj.getSubmissions().stream() + .filter(submission -> !submission.isLate()) + .count(); + if (submissionPolicy == null || inTimeSubmissions < submissionPolicy.getSubmissionLimit()) { + programmingExerciseParticipationService.unlockStudentRepositoryAndParticipation(participationObj); + } + }); + }); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (1)
Pattern 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/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
Pattern 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
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:2836-2846
Timestamp: 2024-11-12T12:51:51.201Z
Learning: The `testAbandonStudentExamNotInTime` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (2)
131-132: Dependency Injection Via Constructor Is Correct
The addition of ProgrammingExerciseStudentParticipationRepository as a private final member and its injection via the constructor adheres to best practices and the project's dependency injection guidelines.
138-139: Dependency Injection Via Constructor Is Correct
The addition of ProgrammingExerciseParticipationService as a private final member and its injection via the constructor is appropriate and aligns with the project's conventions for dependency management.
| boolean wasEndedOriginally = now.isAfter(exam.getEndDate()); | ||
| if (!studentExam.isEnded() && wasEndedOriginally) { | ||
| studentExam.getExercises().stream().filter(ProgrammingExercise.class::isInstance).forEach(exercise -> { | ||
| var programmingExerciseStudentParticipation = programmingExerciseStudentParticipationRepository.findByExerciseIdAndStudentLogin(exercise.getId(), | ||
| studentExam.getUser().getLogin()); | ||
| var programmingExerciseSubmissionPolicy = ((ProgrammingExercise) exercise).getSubmissionPolicy(); | ||
| // Unlock if there is no submission policy | ||
| // or there is a submission policy, but its limit was not reached yet | ||
| if (programmingExerciseSubmissionPolicy == null || exercise.getNumberOfSubmissions().inTime() < programmingExerciseSubmissionPolicy.getSubmissionLimit()) { | ||
| programmingExerciseStudentParticipation.ifPresent(programmingExerciseParticipationService::unlockStudentRepositoryAndParticipation); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential Logical Error: Retrieving Submission Count from Exercise Instead of Participation
In the current implementation, the method exercise.getNumberOfSubmissions().inTime() is used to check the student's number of in-time submissions. However, submissions are associated with student participations, not exercises. Using this method may not correctly reflect the individual student's submission count.
To accurately determine if a student's repository should be unlocked, retrieve the number of submissions from the student's participation. Apply this diff to correct the issue:
if (!studentExam.isEnded() && wasEndedOriginally) {
studentExam.getExercises().stream()
.filter(ProgrammingExercise.class::isInstance)
.forEach(exercise -> {
var programmingExerciseStudentParticipation = programmingExerciseStudentParticipationRepository
.findByExerciseIdAndStudentLogin(exercise.getId(), studentExam.getUser().getLogin());
var programmingExerciseSubmissionPolicy = ((ProgrammingExercise) exercise).getSubmissionPolicy();
programmingExerciseStudentParticipation.ifPresent(participation -> {
+ long inTimeSubmissions = participation.getSubmissions().stream()
+ .filter(submission -> !submission.isLate())
+ .count();
+ if (programmingExerciseSubmissionPolicy == null || inTimeSubmissions < programmingExerciseSubmissionPolicy.getSubmissionLimit()) {
programmingExerciseParticipationService.unlockStudentRepositoryAndParticipation(participation);
+ }
});
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| boolean wasEndedOriginally = now.isAfter(exam.getEndDate()); | |
| if (!studentExam.isEnded() && wasEndedOriginally) { | |
| studentExam.getExercises().stream().filter(ProgrammingExercise.class::isInstance).forEach(exercise -> { | |
| var programmingExerciseStudentParticipation = programmingExerciseStudentParticipationRepository.findByExerciseIdAndStudentLogin(exercise.getId(), | |
| studentExam.getUser().getLogin()); | |
| var programmingExerciseSubmissionPolicy = ((ProgrammingExercise) exercise).getSubmissionPolicy(); | |
| // Unlock if there is no submission policy | |
| // or there is a submission policy, but its limit was not reached yet | |
| if (programmingExerciseSubmissionPolicy == null || exercise.getNumberOfSubmissions().inTime() < programmingExerciseSubmissionPolicy.getSubmissionLimit()) { | |
| programmingExerciseStudentParticipation.ifPresent(programmingExerciseParticipationService::unlockStudentRepositoryAndParticipation); | |
| } | |
| }); | |
| } | |
| boolean wasEndedOriginally = now.isAfter(exam.getEndDate()); | |
| if (!studentExam.isEnded() && wasEndedOriginally) { | |
| studentExam.getExercises().stream() | |
| .filter(ProgrammingExercise.class::isInstance) | |
| .forEach(exercise -> { | |
| var programmingExerciseStudentParticipation = programmingExerciseStudentParticipationRepository | |
| .findByExerciseIdAndStudentLogin(exercise.getId(), studentExam.getUser().getLogin()); | |
| var programmingExerciseSubmissionPolicy = ((ProgrammingExercise) exercise).getSubmissionPolicy(); | |
| programmingExerciseStudentParticipation.ifPresent(participation -> { | |
| long inTimeSubmissions = participation.getSubmissions().stream() | |
| .filter(submission -> !submission.isLate()) | |
| .count(); | |
| if (programmingExerciseSubmissionPolicy == null || inTimeSubmissions < programmingExerciseSubmissionPolicy.getSubmissionLimit()) { | |
| programmingExerciseParticipationService.unlockStudentRepositoryAndParticipation(participation); | |
| } | |
| }); | |
| }); | |
| } |
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testUpdateWorkingTime_ShouldTriggerUnlock() throws Exception { | ||
| ProgrammingExercise programmingExercise = programmingExerciseUtilService.addCourseExamExerciseGroupWithOneProgrammingExercise(); | ||
| programmingExerciseRepository.save(programmingExercise); | ||
|
|
||
| Course course = programmingExercise.getCourseViaExerciseGroupOrCourseMember(); | ||
| courseRepository.save(course); | ||
|
|
||
| userUtilService.addUsers(TEST_PREFIX, NUMBER_OF_STUDENTS, 0, 0, NUMBER_OF_INSTRUCTORS); | ||
| User student = userUtilService.getUserByLogin(TEST_PREFIX + "student1"); | ||
|
|
||
| ProgrammingExerciseStudentParticipation participation = participationUtilService.addStudentParticipationForProgrammingExercise(programmingExercise, student.getLogin()); | ||
| participationRepository.save(participation); | ||
|
|
||
| Exam exam = programmingExercise.getExam(); | ||
| exam.setStartDate(ZonedDateTime.now().minusHours(2)); | ||
| exam.setEndDate(ZonedDateTime.now().minusHours(1)); | ||
| examRepository.save(exam); | ||
|
|
||
| StudentExam studentExam = new StudentExam(); | ||
| studentExam.setUser(student); | ||
| studentExam.setExercises(List.of(programmingExercise)); | ||
| studentExam.setExam(exam); | ||
| studentExam.setTestRun(false); | ||
| studentExam.setWorkingTime(1); | ||
| studentExamRepository.save(studentExam); | ||
|
|
||
| doNothing().when(programmingExerciseParticipationService).unlockStudentRepositoryAndParticipation(any()); | ||
|
|
||
| int newWorkingTime = 180 * 60; | ||
|
|
||
| StudentExam updatedExam = request.patchWithResponseBody( | ||
| "/api/courses/" + course.getId() + "/exams/" + exam.getId() + "/student-exams/" + studentExam.getId() + "/working-time", newWorkingTime, StudentExam.class, | ||
| HttpStatus.OK); | ||
|
|
||
| assertThat(updatedExam).isNotNull(); | ||
| assertThat(updatedExam.getWorkingTime()).isEqualTo(newWorkingTime); | ||
|
|
||
| verify(programmingExerciseParticipationService, times(1)).unlockStudentRepositoryAndParticipation(participation); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Minimize database interactions in tests by using mocks
The testUpdateWorkingTime_ShouldTriggerUnlock method performs multiple database operations, such as saving programmingExercise, course, participation, exam, and studentExam. According to the coding guidelines, tests should avoid direct database access and prefer mocking to improve performance and ensure test isolation.
Consider mocking the repositories and services involved to simulate database interactions. Utilize Mockito or similar frameworks to mock the behavior of these components. This will make the test faster, more reliable, and in alignment with the avoid_db_access guideline.
| @Autowired | ||
| private ProgrammingExerciseStudentParticipationRepository participationRepository; | ||
|
|
||
| @Autowired | ||
| private ProgrammingExerciseRepository programmingExerciseRepository; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid direct autowiring of repositories in test classes
The test class directly autowires repositories like ProgrammingExerciseStudentParticipationRepository, ProgrammingExerciseRepository, CourseRepository, and UserUtilService. According to the coding guidelines, tests should utilize utility services or factory patterns to manage test data instead of directly accessing repositories. This approach enhances test isolation and adheres to the service-oriented architecture.
Consider refactoring to use existing utility services (e.g., ProgrammingExerciseUtilService, UserUtilService) for data setup and manipulation. If necessary, extend these utilities to cover additional functionalities required by the tests.
Also applies to: 220-225
meltemarsl
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
meltemarsl
added
deploy:artemis-test5
deploy:artemis-test2
and removed
deploy:artemis-test4
deploy:artemis-test5
labels
meltemarsl
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
eceeeren
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Description
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests