Exam mode: Participate in the test exam multiple times
#10340
Conversation
…d and exercise_id
…in-test-exam-multiple-times # Conflicts: # src/test/java/de/tum/in/www1/artemis/service/ParticipationServiceTest.java
…ciseParticipationService
WalkthroughThis pull request refactors and extends exam functionality across backend and frontend components. It streamlines exam participation logic by reducing redundant method calls, introduces a many-to-many relationship between student exams and participations, and enhances repository methods to support test exam attempts. Additionally, the PR refines exam access, submission, and participation services, modifies Angular routes and components for exam participation, and updates localization files to support test exam attempt terminology. Comprehensive test updates further validate these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant EPC as ExamParticipationComponent
participant EAS as ExamAccessService
participant SER as StudentExamRepository
participant EDS as ExamDateService
participant SES as StudentExamService
U->>EPC: Initiate exam participation
EPC->>EAS: getOrCreateStudentExamElseThrow(courseId, examId)
EAS->>SER: Retrieve StudentExam with exercises & participations
SER-->>EAS: Return StudentExam data
EAS->>EDS: isExamOver(exam)
EDS-->>EAS: Return exam over status
EAS->>SES: Setup or retrieve test exam participations/submissions
SES-->>EAS: Return configured StudentExam
EAS-->>EPC: Return final StudentExam
EPC->>U: Load exam participation view or summary
Possibly related PRs
Suggested labels
✨ Finishing Touches
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: 8
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
210-212:⚠️ Potential issueSecure the regex pattern against injection.
The regex pattern is constructed from user input (
projectKey) without proper sanitization, which could lead to regex injection vulnerabilities.Apply this fix to escape special regex characters in the project key:
- String pattern = projectKey.toLowerCase() + "\\d*-"; + String pattern = Pattern.quote(projectKey.toLowerCase()) + "\\d*-";Don't forget to add the import:
import java.util.regex.Pattern;
🧹 Nitpick comments (23)
src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java (1)
128-129: Enhance parameter documentation.The documentation for the
attemptparameter should clarify its purpose and valid range of values.Add more details to the parameter documentation:
- * @param attempt The attempt number + * @param attempt The attempt number (1-based) for test exam participations, or null for other modes. Used to create unique repository names for multiple attempts.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java (1)
148-176: Consider consolidating duplicate error messages.The error message in this method is identical to the one in
findStudentParticipationByExerciseAndStudentId. Consider extracting it to a constant to maintain consistency and ease maintenance.+ private static final String PARTICIPATION_NOT_FOUND_MESSAGE = "Participation could not be found by exerciseId %d and user %s"; @NotNull public ProgrammingExerciseStudentParticipation findStudentParticipationByExerciseAndStudentLoginAndTestRunOrThrow(ProgrammingExercise exercise, String username, boolean isTestRun, boolean withSubmissions) { // ... if (participationOptional.isEmpty()) { - throw new EntityNotFoundException("Participation could not be found by exerciseId " + exercise.getId() + " and user " + username); + throw new EntityNotFoundException(String.format(PARTICIPATION_NOT_FOUND_MESSAGE, exercise.getId(), username)); } // ... }src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (2)
143-156: LGTM! Query is well-structured with efficient subquery.The query efficiently retrieves the latest participation with legal submissions using a subquery. Consider adding an index on
(exercise_id, student_login, id)to optimize the subquery performance.
782-809: LGTM! Well-structured queries for test exam participations.The queries efficiently retrieve test exam participations with appropriate eager loading. Consider adding indexes on
(student_exam_id, test_exam)to optimize the JOIN performance.src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java (2)
106-110: Consider enhancing the attempt handling logic.While the implementation is functional, consider these improvements:
- Add validation for maximum attempt number (e.g., 256 as per PR objectives)
- Use
String.formator StringBuilder for better performance- Extract "practice-" as a constant
Here's a suggested implementation:
+ private static final String PRACTICE_REPO_PREFIX = "practice-"; + private static final int MAX_ATTEMPTS = 256; + public VcsRepositoryUri copyRepository(...) { String targetProjectKeyLowerCase = targetProjectKey.toLowerCase(); - if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { + if (attempt != null && attempt > 0 && attempt <= MAX_ATTEMPTS + && !targetRepositoryName.contains(PRACTICE_REPO_PREFIX)) { - targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; + targetProjectKeyLowerCase = String.format("%s%d", targetProjectKeyLowerCase, attempt); } final String targetRepoSlug = targetProjectKeyLowerCase + "-" + targetRepositoryName;
124-148:❓ Verification inconclusive
Consider implementing the TODO for push verification.
The error handling is robust, but there's an unimplemented TODO for verifying successful push operations when encountering 'Short read of block' errors.
Let's verify if this is a common issue by searching for similar occurrences:
Would you like me to help implement the verification logic for comparing commit counts between source and target repositories?
🏁 Script executed:
#!/bin/bash # Search for similar error handling patterns or TODOs rg -A 2 "Short read of block" --type javaLength of output: 192
Implement push verification logic via commit count comparison
The error handling in
AbstractVersionControlService.javais robust, but the unimplemented TODO regarding verifying that a push operation actually succeeded—by, for example, comparing commit counts between the source and target repositories—remains. Since our initial search using ripgrep returned no meaningful output (likely due to filtering issues), please manually verify that the "Short read of block" log and associated cleanup logic exist solely in this exception handler. It would be beneficial, as a next step, to implement logic that confirms the push's success when aShort read of blockerror is caught.• Location:
src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java(Lines 124–148)
• Suggestion: Consider adding a push verification step (e.g., commit count comparison) and corresponding tests to ensure that the repository states remain consistent, even when the exception is encountered.Would you like assistance in designing or implementing this verification mechanism?
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
101-121: Consider optimizing the JPQL query to reduce N+1 queries.The query in
findFirstWithSubmissionsByExerciseIdAndStudentLoginOrderByIdDesccould be optimized to fetch related entities that are commonly accessed together.Apply this diff to optimize the query:
@Query(""" SELECT participation FROM ProgrammingExerciseStudentParticipation participation LEFT JOIN FETCH participation.submissions s + LEFT JOIN FETCH participation.exercise e + LEFT JOIN FETCH participation.student WHERE participation.exercise.id = :exerciseId AND participation.student.login = :username ORDER BY participation.id DESC """)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (2)
121-125: Consider using Optional for auxiliary repository operations.The auxiliary repository operations could benefit from using Optional to handle potential null cases more elegantly.
- var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null).toString(); - AuxiliaryRepository newAuxRepo = newExercise.getAuxiliaryRepositories().get(i); - newAuxRepo.setRepositoryUri(repoUri); - auxiliaryRepositoryRepository.save(newAuxRepo); + Optional.ofNullable(newExercise.getAuxiliaryRepositories().get(i)) + .ifPresent(newAuxRepo -> { + var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null).toString(); + newAuxRepo.setRepositoryUri(repoUri); + auxiliaryRepositoryRepository.save(newAuxRepo); + });
272-272: Consider using a constant for the commit message.The commit message "Template adjusted by Artemis" should be defined as a constant to maintain consistency across the codebase.
+ private static final String TEMPLATE_ADJUSTMENT_COMMIT_MESSAGE = "Template adjusted by Artemis";Then use it in the code:
- gitService.commitAndPush(repository, "Template adjusted by Artemis", true, user); + gitService.commitAndPush(repository, TEMPLATE_ADJUSTMENT_COMMIT_MESSAGE, true, user);src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java (2)
175-194: Consider simplifying normal and regular exam branching logic.
This block contains nested calls and reassignments. You may improve clarity by extracting the practice-mode handling to a dedicated method or returning early once the existing participation is set inactive.
291-294: Inconsistent attempt numbering between practice mode and test exam.
Here, attempt is forced to 1, while the test-exam scenario uses a different scheme. Harmonize if you aim for uniform behavior across participation modes.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (2)
52-65: Constructor dependency injection is valid but consider reviewing class responsibilities.
The constructor has grown to eight injected dependencies, which might indicate multiple responsibilities in the same class. Consider splitting services or applying additional design patterns to maintain single responsibility.
83-104: Ensure consistent date checks across the application.
UsingZonedDateTime.now()is fine, but confirm you apply the same approach throughout to avoid discrepancies in time zone handling. It might be beneficial to centralize time checks inExamDateServicefor improved maintainability.src/main/webapp/app/overview/course-exams/course-exams.component.ts (1)
79-79: Consider a more descriptive name thanwithinWorkingTime.
“Naming: be explicit” is a best practice. For instance,isExamWithinWorkingTimeimproves clarity.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (1)
155-156: Consider improving readability of the condition.While the logic is correct, the condition could be more readable by extracting it into a descriptive method.
Consider this refactor:
- if (!exercise.isExamExercise() || exercise instanceof ProgrammingExercise || exercise.getExam().isTestExam()) { + if (shouldAllowMultipleSubmissions(exercise)) { return submission; } + + private boolean shouldAllowMultipleSubmissions(Exercise exercise) { + return !exercise.isExamExercise() + || exercise instanceof ProgrammingExercise + || exercise.getExam().isTestExam(); + }src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java (1)
255-257: Consider adding null checks in isFinished method.The method could be more robust with null checks for the Boolean values.
Consider this refactor:
- public boolean isFinished() { - return Boolean.TRUE.equals(this.isStarted()) && (Boolean.TRUE.equals(this.isEnded()) || Boolean.TRUE.equals(this.isSubmitted())); + public boolean isFinished() { + Boolean started = this.isStarted(); + Boolean ended = this.isEnded(); + Boolean submitted = this.isSubmitted(); + + if (started == null || (ended == null && submitted == null)) { + return false; + } + + return Boolean.TRUE.equals(started) && + (Boolean.TRUE.equals(ended) || Boolean.TRUE.equals(submitted)); + }src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (1)
255-262: Add descriptive error messages to assertions.While the test methods correctly verify the HTTP status codes, adding descriptive messages to the assertions would make test failures more informative.
Apply this diff to improve error messages:
- request.get("/api/courses/" + course1.getId() + "/exams/" + testExam1.getId() + "/own-student-exam", HttpStatus.BAD_REQUEST, StudentExam.class); + request.get("/api/courses/" + course1.getId() + "/exams/" + testExam1.getId() + "/own-student-exam", HttpStatus.BAD_REQUEST, StudentExam.class, + "Should return BAD_REQUEST when exam has ended"); - request.get("/api/courses/" + course1.getId() + "/exams/" + testExam1.getId() + "/own-student-exam", HttpStatus.INTERNAL_SERVER_ERROR, StudentExam.class); + request.get("/api/courses/" + course1.getId() + "/exams/" + testExam1.getId() + "/own-student-exam", HttpStatus.INTERNAL_SERVER_ERROR, StudentExam.class, + "Should return INTERNAL_SERVER_ERROR when multiple unfinished attempts exist");Also applies to: 264-271
src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
654-702: LGTM! Well-structured implementation with proper error handling.The method effectively:
- Handles both test and regular exams
- Uses proper error handling and logging
- Maintains thread safety for parallel execution
- Correctly saves student participations
Minor suggestion regarding the TODO comment on line 644:
- // TODO: Michael Allgaier: schedule a lock operation for all involved student repositories of this student exam (test exam) at the end of the individual working time + // TODO: Schedule a lock operation for all involved student repositories of this student exam (test exam) at the end of the individual working timesrc/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (2)
420-434: Uncomment and implement test for exam end condition.This test would verify important functionality around exam access after the exam has ended.
Consider implementing this test to ensure proper handling of ended test exams:
-// @Test -// @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") -// void testGetExamInCourseElseThrow_testExamEnded() { -// testExam1.setEndDate(ZonedDateTime.now().minusHours(5)); -// examRepository.save(testExam1); -// assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(BadRequestAlertException.class); -// } +@Test +@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") +void testGetExamInCourseElseThrow_testExamEnded() { + testExam1.setEndDate(ZonedDateTime.now().minusHours(5)); + examRepository.save(testExam1); + assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(BadRequestAlertException.class); +}
428-434: Uncomment and implement test for multiple unfinished student exams.This test would verify the handling of multiple unfinished student exams, which is an important edge case.
Consider implementing this test to ensure proper handling of multiple unfinished exams:
-// @Test -// @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") -// void testGetExamInCourseElseThrow_multipleUnfinishedStudentExams() { -// User user = studentExamForTestExam1.getUser(); -// examUtilService.addStudentExamForTestExam(testExam1, user); -// assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(IllegalStateException.class); -// } +@Test +@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") +void testGetExamInCourseElseThrow_multipleUnfinishedStudentExams() { + User user = studentExamForTestExam1.getUser(); + examUtilService.addStudentExamForTestExam(testExam1, user); + assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(IllegalStateException.class); +}src/main/webapp/app/overview/course-overview.service.ts (3)
300-304: Enhance type safety and input validation.The method could be more robust with non-optional parameters and input validation.
- mapTestExamAttemptsToSidebarCardElements(attempts?: StudentExam[], indices?: number[]) { - if (attempts && indices) { - return attempts.map((attempt, index) => this.mapAttemptToSidebarCardElement(attempt, index)); - } + mapTestExamAttemptsToSidebarCardElements(attempts: StudentExam[], indices: number[]): SidebarCardElement[] { + if (attempts.length !== indices.length) { + throw new Error('Attempts and indices arrays must have the same length'); + } + return attempts.map((attempt, index) => this.mapAttemptToSidebarCardElement(attempt, indices[index])); + }
352-368: Add JSDoc documentation for better maintainability.The method would benefit from JSDoc documentation explaining the purpose of the new parameters and properties.
+ /** + * Maps an exam to a sidebar card element. + * @param exam - The exam to map + * @param studentExam - Optional student exam data + * @param numberOfAttempts - Optional number of test exam attempts + * @returns A sidebar card element representing the exam + */ mapExamToSidebarCardElement(exam: Exam, studentExam?: StudentExam, numberOfAttempts?: number): SidebarCardElement {
467-475: Improve readability with extracted conditions and documentation.The method would benefit from extracted conditions and JSDoc documentation.
+ /** + * Calculates the working time used for a student exam. + * For test exams, the working time is limited to the regular exam duration. + * @param studentExam - The student exam to calculate working time for + * @returns The used working time in seconds + */ calculateUsedWorkingTime(studentExam: StudentExam): number { - let usedWorkingTime = 0; - if (studentExam.exam!.testExam && studentExam.started && studentExam.submitted && studentExam.workingTime && studentExam.startedDate && studentExam.submissionDate) { - const regularExamDuration = studentExam.workingTime; - // As students may submit during the grace period, the workingTime is limited to the regular exam duration - usedWorkingTime = Math.min(regularExamDuration, dayjs(studentExam.submissionDate).diff(dayjs(studentExam.startedDate), 'seconds')); + if (!studentExam.exam?.testExam) { + return 0; } - return usedWorkingTime; + + const hasRequiredData = studentExam.started && + studentExam.submitted && + studentExam.workingTime && + studentExam.startedDate && + studentExam.submissionDate; + + if (!hasRequiredData) { + return 0; + } + + const regularExamDuration = studentExam.workingTime; + const actualWorkingTime = dayjs(studentExam.submissionDate) + .diff(dayjs(studentExam.startedDate), 'seconds'); + + // Limit working time to regular exam duration to account for grace period submissions + return Math.min(regularExamDuration, actualWorkingTime); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/main/resources/config/liquibase/changelog/20240513101552_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101555_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101557_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (44)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(1 hunks)src/main/webapp/app/exam/participate/exam-participation.component.ts(3 hunks)src/main/webapp/app/exam/participate/exam-participation.route.ts(0 hunks)src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts(2 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html(1 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/overview/course-exams/course-exams.component.ts(9 hunks)src/main/webapp/app/overview/course-overview.service.ts(4 hunks)src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html(1 hunks)src/main/webapp/app/types/sidebar.ts(2 hunks)src/main/webapp/i18n/de/exam.json(2 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/exam.json(2 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java(1 hunks)src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts(7 hunks)src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts(1 hunks)src/test/javascript/spec/service/exam-participation.service.spec.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/exam/participate/exam-participation.route.ts
- src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
🧰 Additional context used
📓 Path-based instructions (6)
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.tssrc/test/javascript/spec/component/exam/participate/exam-participation.component.spec.tssrc/test/javascript/spec/service/exam-participation.service.spec.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/exam/service/ExamServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.htmlsrc/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/exam/service/ExamDateService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.javasrc/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.javasrc/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.javasrc/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.tssrc/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.tssrc/main/webapp/app/types/sidebar.tssrc/main/webapp/app/exam/participate/exam-participation.component.tssrc/main/webapp/app/overview/course-overview.service.tssrc/main/webapp/app/overview/course-exams/course-exams.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/student-dashboard.jsonsrc/main/webapp/i18n/de/exam.json
🧠 Learnings (8)
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java (2)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (3)
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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
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().
src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (3)
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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:975-980
Timestamp: 2024-11-12T12:51:51.200Z
Learning: The `testSubmitStudentExam_notInTime` 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.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (2)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
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().
src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (1)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
🪛 GitHub Actions: CodeQL
src/main/webapp/app/exam/participate/exam-participation.component.ts
[error] 68-68: Execution failed for task ':webapp'. Process 'command '/home/runner/work/Artemis/Artemis/.gradle/npm/npm-v10.9.0/bin/npm'' finished with non-zero exit value 1.
🪛 GitHub Actions: Build
src/main/webapp/app/exam/participate/exam-participation.component.ts
[error] 68-68: Execution failed for task ':webapp'. Process 'command '/home/runner/work/Artemis/Artemis/.gradle/npm/npm-v10.9.0/bin/npm'' finished with non-zero exit value 1.
🪛 GitHub Actions: Test
src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts
[error] 321-321: Expected number of calls: 0, Received number of calls: 1 for loadStudentExamSpy.
[error] NG0304: 'jhi-exam-exercise-overview-page' is not a known element. Verify that it is included in the '@Component.imports' of this component.
[error] NG0303: Can't bind to 'studentExam' since it isn't a known property of 'jhi-exam-exercise-overview-page'. Verify that it is included in the '@Component.imports' of this component.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (79)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java (1)
564-564: LGTM! Memory optimization by removing unused data.The change aligns with the method's purpose of filtering unnecessary information. Setting the course attribute to null helps optimize memory usage by removing unused data.
src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java (1)
132-133: LGTM! Method signature change aligns with PR objectives.The addition of the
attemptparameter tocopyRepositorysupports the requirement for multiple test exam attempts while maintaining a clean interface design.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java (2)
115-134: LGTM! Well-structured method with clear documentation.The implementation follows best practices with:
- Clear Javadoc documentation
- Proper null-safety with
@NotNullannotation- Appropriate error handling using repository methods
- Efficient conditional loading of submissions
162-169: LGTM! Efficient conditional loading of submissions.The implementation correctly handles both scenarios:
- Loading with submissions using
findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRun- Loading without submissions using
findFirstByExerciseIdAndStudentLoginAndTestRunOrderByIdDescsrc/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (2)
130-130: LGTM! Method follows Spring Data JPA conventions.The method name is well-structured and follows Spring Data JPA's method naming convention, using
findFirstBywithOrderByIdDescto retrieve the latest participation.
1024-1042: LGTM! Clean implementation of test exam handling.The changes effectively distinguish between real exams, test exams, and test runs, with appropriate method calls for each case. The implementation aligns well with the PR objectives for supporting test exam functionality.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java (2)
113-115: LGTM! Clear and informative documentation.The comments effectively explain the distinction between test and real exams, and clearly document the rationale for supporting multiple participations.
113-116:❓ Verification inconclusive
LGTM! Verify repository method implementation.
The change correctly implements support for multiple test exam participations by retrieving the latest student exam. The comments clearly explain the rationale.
Let's verify the repository method implementation:
🏁 Script executed:
#!/bin/bash # Description: Verify the repository method implementation and usage # Test: Check if the repository method is properly defined ast-grep --pattern $'interface StudentExamRepository { $$$ Optional<StudentExam> findFirstByExamIdAndUserIdOrderByIdDesc(Long examId, Long userId); $$$ }' # Test: Check for consistent usage across the codebase rg "findFirstByExamIdAndUserIdOrderByIdDesc|findByExamIdAndUserId" -A 2Length of output: 235
LGTM – Manual Verification Needed for Repository Method
The change to use
findFirstByExamIdAndUserIdOrderByIdDesccorrectly supports multiple test exam participations by retrieving the latest student exam, and the accompanying comments clearly explain this rationale over the previousfindByExamIdAndUserIdusage. However, the automated verification script did not produce any output. Please manually verify that the method is properly defined in theStudentExamRepositoryinterface and that its usage is consistent across the repository.src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java (1)
102-103: LGTM! Good choice of parameter type.The use of
Integerinstead ofintappropriately allows for null values when copying repositories outside of test exam context.src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2)
80-81: LGTM! Methods follow Spring Data JPA naming conventions.The new methods for retrieving the latest participation by exercise ID and student login are well-designed and follow the repository's pattern of providing both Optional and throwing variants.
Also applies to: 97-99
123-126:❓ Verification inconclusive
Verify the impact of the new test run methods on database performance.
The new methods supporting test runs add complexity to the query layer. While the implementation is correct, we should verify the performance impact, especially for courses with many students and test attempts.
Let's analyze the query patterns:
Also applies to: 181-195
🏁 Script executed:
#!/bin/bash # Description: Analyze query patterns for test run methods # Look for similar patterns in the codebase that might indicate performance issues # Search for similar ORDER BY DESC patterns that might need pagination rg -A 5 "ORDER BY.*DESC" --type java # Look for potential N+1 query patterns in repository methods ast-grep --pattern 'interface $_ extends $_ { $$$ @Query($$$) $$$OrderBy$$$Desc($$$) $$$ }'Length of output: 289
Manual Verification: Benchmark Test Run Query Performance
While the new test run repository methods are implemented correctly, their use of ORDER BY (e.g., in
findFirstByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc) could potentially impact performance under high load (e.g., courses with many students and numerous test attempts). Our initial automated searches using rg and ast-grep did not yield any conclusive output regarding problematic query patterns. However, this lack of output might be due to filtering issues rather than an absence of performance risks.Action Items:
- Verify in Production-like Environment: Manually benchmark the queries to assess performance, particularly for scenarios with large datasets.
- Review Query Plans: Analyze the execution plans for these queries to ensure that the ORDER BY operations are optimized and that pagination (if needed) is applied.
- Double-check Query Patterns: Re-examine related repository methods (including those on lines 181-195) to confirm that no unintended N+1 or inefficient query patterns exist.
Please perform manual validation or run further detailed query analysis to ensure the performance is not negatively impacted.
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (1)
14-14: LGTM!The binding is correctly simplified to use the getter method, which improves code maintainability by centralizing the router link logic in the component class.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
114-116:❓ Verification inconclusive
Verify the impact of the new
nullparameter.The addition of the
nullparameter tocopyRepositorycalls suggests support for multiple attempts, but the impact needs verification.
🏁 Script executed:
#!/bin/bash # Search for all copyRepository method calls to ensure consistent usage rg -A 2 "copyRepository\(" --type javaLength of output: 189
Below is an updated review comment that reflects the inconclusive output from the original search. To help verify the impact of the new
nullparameter, please run the additional verification script to search for all occurrences ofcopyRepositoryacross the repository (without the restrictive file-type filter):Manual Verification Required: Impact of
nullParameter in copyRepository Calls
- File:
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(Lines: 114-116)- Diff Snippet under review:
versionControl.copyRepository(sourceProjectKey, templateRepoName, sourceBranch, targetProjectKey, RepositoryType.TEMPLATE.getName(), null); versionControl.copyRepository(sourceProjectKey, solutionRepoName, sourceBranch, targetProjectKey, RepositoryType.SOLUTION.getName(), null); versionControl.copyRepository(sourceProjectKey, testRepoName, sourceBranch, targetProjectKey, RepositoryType.TESTS.getName(), null);The initial search using
rgwith the--type javaflag didn’t yield any results due to an unexpected filter. Please run the above script and examine the output to ensure that all occurrences ofcopyRepository(and especially the changes incorporatingnull) are consistent and safe. If any inconsistencies or unintended side effects are identified, please address them accordingly.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search across all files for calls to versionControl.copyRepository to evaluate consistent usage. rg "versionControl.copyRepository\(" .Length of output: 162
Action Required: Manual Verification Needed on
copyRepositoryChangesThe automated search did not yield conclusive results to verify that the new
nullparameter is consistently applied across the codebase. Please manually inspect the usage ofversionControl.copyRepositoryto ensure that the inclusion of thenullparameter does not introduce unintended side effects (e.g., issues in handling multiple attempts).
- File:
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(Lines: 114–116)versionControl.copyRepository(sourceProjectKey, templateRepoName, sourceBranch, targetProjectKey, RepositoryType.TEMPLATE.getName(), null); versionControl.copyRepository(sourceProjectKey, solutionRepoName, sourceBranch, targetProjectKey, RepositoryType.SOLUTION.getName(), null); versionControl.copyRepository(sourceProjectKey, testRepoName, sourceBranch, targetProjectKey, RepositoryType.TESTS.getName(), null);- Action: Confirm that all instances of this method call across the repository are handling the new parameter consistently and as intended.
If uncertain, consider running a revised search with a simpler command (e.g., using
grep -R "copyRepository(" .) for further validation.src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java (10)
142-152: Clear and comprehensive JavaDoc.
The updated documentation thoroughly explains the revised multi-attempt logic in a concise manner.
154-157: Accurate parameter and return documentation.
The doc comments correctly match the method’s parameters and return value.
161-162: Variable declarations are conventional and consistent.
No issues found with the initialization approach.
164-172: Verify off-by-one possibility for test-exam attempts.
When creating new test-exam participations, the attempt is set toparticipations.size(), which may yield a zero-based attempt index unless explicitly intended.
199-199: Re-invocation of startProgrammingExercise is appropriate.
No concerns; logic is clean and consistent.
201-201: Comment enhances readability.
No issues with this addition.
297-297: Method extraction improves maintainability.
Refactoring intostartProgrammingParticipationis a welcome enhancement.
467-467: Guard logic for repository copy is correct.
Properly prevents redundant copies if initialization state is already complete or the URI is set.
593-594: Enhanced lookup for latest test-exam participation.
Filtering by the most recent participation aligns with the new multi-attempt feature for test exams.Also applies to: 596-596
672-674: Consistent approach for retrieving last attempt in test exams.
Mirrors logic above to ensure the latest submission is returned for multi-attempt scenarios.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (2)
6-6: No issues with theListimport.
The explicit import is aligned with the coding guidelines.
106-146: Logic for creating or retrieving normal exams looks good.
The flow adequately checks if the exam ended, if the user is registered, and whether the student can start.src/main/webapp/app/overview/course-exams/course-exams.component.ts (13)
4-5: New imports look appropriate.
They are needed for navigation logic and RxJS usage.
23-36: Additional accordion group category “attempt” is clear.
The approach aligns with the existing structure for real and test exam groups.
63-63: Subscription property is fine.
Ensure it’s unsubscribed inngOnDestroyor a dedicated method to avoid memory leaks.
72-72: Map for test exam attempts is well-defined.
Initializing as an empty Map is good for type safety.
110-110: Initiating sidebar data repopulation here is acceptable.
No issues detected with callingprepareSidebarData()at this stage.
239-254: Logic for grouping test exam attempts is coherent.
Iterating over test exams and mapping attempts into a separate accordion category is straightforward and maintainable.
299-305: Combining sidebar items for real exams, test exams, and attempts is properly structured.
No issues found; the flow is consistent with the rest of the component’s logic.
321-331: Collecting all test exam attempts in one method is a good approach.
This flattening function enhances reusability and clarity.
333-340: Generating a list of indices is straightforward.
The logic is correct, though you may consider returning an array directly withArray.from({ length: numberOfStudentExams }, (_, i) => i + 1)if brevity is desired.
342-345: Retrieving the number of attempts for a test exam is concise and correct.
No issues found.
352-367: Verify consistent cleanup of the interval subscription.
While you do unsubscribe when!withinWorkingTime, ensure the user leaving the page also triggersunsubscribeFromExamStateSubscription()inngOnDestroy.
372-374: Dedicated unsubscribe method is clear.
This helps avoid subscription leaks.
379-384: Time calculation logic for ongoing exams is clear.
The approach to checkexam.workingTimewithdayjsis straightforward.src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts (1)
31-44: End date property addition is correctly integrated.
Settingthis.endDate = this.exam.endDate;inngOnInitis aligned with the existing pattern for date properties.src/main/webapp/app/types/sidebar.ts (2)
10-10: LGTM!The addition of 'attempt' to
ExamGroupCategoryaligns with the PR objectives to support multiple test exam attempts.
138-154: LGTM!The new properties are well-defined with clear types and descriptive comments, supporting the test exam attempt functionality.
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts (1)
142-158: LGTM!The new test cases are well-structured and verify important functionality for test exams, maintaining consistency with existing tests.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (1)
104-117: LGTM!The refactored method effectively handles test exams by finding the latest unsubmitted exam, with clear logic and good comments.
src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java (1)
88-92: LGTM!The many-to-many relationship with StudentParticipation is well-defined with appropriate annotations and consistent caching strategy.
src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java (1)
74-86: LGTM! Well-structured test method.The test method follows the class's testing patterns, provides good coverage for the
LocalVCRepositoryUriconstructor, and includes comprehensive assertions.src/test/javascript/spec/service/exam-participation.service.spec.ts (1)
273-283: LGTM! Comprehensive test coverage for exam submission.The test methods provide thorough coverage of success and error scenarios, with appropriate error handling and message verification. The use of
HttpTestingControllerand error responses is well-implemented.Also applies to: 285-301, 303-319, 321-335
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java (1)
266-266: LGTM! Necessary initialization for test.Setting the student exam ID ensures the test object is properly initialized before testing instructor access.
src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java (2)
127-136: LGTM! Well-documented field for tracking test exam attempts.The field declaration and documentation clearly explain the purpose and constraints of the attempt field. The default value of 0 aligns with the documented behavior for graded course exercises and real exams.
290-296: LGTM! Clean getter/setter implementation.The getter and setter methods follow Java bean conventions and provide straightforward access to the attempt field.
src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java (3)
45-46: LGTM! Efficient eager loading of related entities.The
@EntityGraphannotation correctly specifies eager loading of both exercises and student participations, which helps prevent N+1 query issues.
196-206: LGTM! Well-structured query for finding latest student exam.The method uses
findFirstByExamIdAndUserIdOrderByIdDescto efficiently retrieve the latest attempt for a student in a test exam.
215-217: LGTM! Comprehensive documentation for helper method.The documentation clearly explains the purpose and behavior of
findOneByExamIdAndUserIdElseThrow, including the handling of test exams.src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (1)
502-512: LGTM! Well-optimized query for finding active exams.The query efficiently retrieves active exams by:
- Using appropriate LEFT JOIN for exam users
- Including all necessary filtering conditions
- Following SQL best practices with clear parameter usage
src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1)
378-381: LGTM! Clean implementation of test exam check.The method follows good practices by:
- Reusing existing methods to avoid code duplication
- Using proper annotation to control JSON serialization
- Having a clear and concise implementation
src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
641-646: LGTM! Clean refactoring to support test exam participations.The method has been simplified by delegating to the new
setUpExerciseParticipationsAndSubmissionsmethod while maintaining its core functionality.src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java (1)
900-901: LGTM! Consistent update to mock method calls.The mock method calls have been properly updated to include the new
attemptparameter, maintaining consistency across both usages.Also applies to: 916-916
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (5)
181-181: LGTM! Method name change enhances clarity.The new method name better reflects its functionality by explicitly mentioning student participations in the method name.
465-465: LGTM! Method name change improves specificity.The new method name better indicates its purpose of finding student exams specifically for test exams.
489-489: LGTM! Method name change enhances clarity.The new method name better reflects its functionality by explicitly mentioning student participations.
564-564: LGTM! Method name change improves error handling.The new method name with 'ElseThrow' suffix clearly indicates that it will throw an exception if no result is found.
749-750: LGTM! Simplified test exam setup.The code has been simplified by removing timestamp truncation and directly invoking the setup method.
src/main/webapp/app/exam/participate/exam-participation.component.ts (3)
222-226: LGTM! Improved route parameter handling.The code now uses a more reliable way to access route parameters through the firstChild snapshot.
240-248: LGTM! Enhanced test exam handling.Added proper error handling for loading student exams in test mode.
452-452: LGTM! Improved navigation for test exams.The navigation path has been updated to use a more consistent route structure.
src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts (1)
1096-1097: LGTM! Improved test setup.The test now properly sets the testExam flag before calling loadAndDisplaySummary, ensuring correct behavior testing.
Also applies to: 1116-1117
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (4)
14-16: LGTM! Enhanced visual feedback.Added conditional class binding to highlight attempt items.
19-22: LGTM! Improved translation handling.Dynamic translation key selection based on item type enhances context-awareness.
24-34: LGTM! Enhanced date display logic.Improved date formatting with clear conditional logic for different item types.
38-78: LGTM! Comprehensive conditional rendering.Well-structured conditional sections for displaying attempts and working time information.
src/main/webapp/i18n/en/student-dashboard.json (2)
60-62: New Test Exam Attempt Keys Added
The new keys"attempt": "Test Exam Attempts","testExamAttempt": "Attempt", and"attempts": "Attempts"added in thesidebarsection clearly provide the labels required to support test exam attempt tracking. The wording is clear and consistent with the rest of the English localization.
66-67: New Submission and Working Time Keys Added
The additions of"submissionDate": "Submission Date"and"usedWorkingTime": "Used Working Time"enhance the sidebar by allowing the display of submission metadata and working time metrics. These keys integrate well with the overall UI language.src/main/webapp/i18n/de/student-dashboard.json (2)
60-62: Neue Testklausur-Versuch Schlüssel hinzugefügt
Die neuen Schlüssel"attempt": "Testklausurversuche","testExamAttempt": "Versuch"und"attempts": "Versuche"in dersidebar-Sektion ermöglichen die Anzeige von Testklausurversuch-Informationen. Die verwendeten Formulierungen sind informell und konsistent mit dem restlichen Text, wodurch das "Du"-Prinzip eingehalten wird.
66-67: Neue Schlüssel für Abgabedatum und genutzte Arbeitszeit
Mit der Einführung von"submissionDate": "Abgabedatum"und"usedWorkingTime": "Gebrauchte Arbeitszeit"wird der Benutzeroberfläche zusätzliche Information bereitgestellt. Die Übersetzungen sind klar und kommen dem informellen Sprachstil gerecht.src/main/webapp/i18n/en/exam.json (1)
59-61: Resume Attempt Feature Enhancement
A new key"resume": "Resume Attempt"has been introduced in the"testExam"section. This addition offers users a straightforward option to resume an exam attempt and aligns well with the enhanced test exam functionality described in this PR.src/main/webapp/i18n/de/exam.json (2)
59-61: Feature-Erweiterung: Schlüssel "resume" hinzugefügt
Der neue Schlüssel"resume": "Versuch fortführen"im Bereich"testExam"ermöglicht es den Benutzern, einen laufenden Testklausurversuch fortzusetzen. Diese Erweiterung entspricht der aktualisierten Testexam-Funktionalität und ist sachgerecht übersetzt.
302-304: Korrektur der Pluralform in der Prüfungsbeteiligung
In der"examParticipation"-Sektion wurde die Formulierung von „Abgabe“ zu „Abgaben“ geändert (z. B. „Um deine Abgaben zu betrachten, navigiere zur Klausurseite“). Diese Anpassung stellt klar, dass der Benutzer mehrere Abgaben bzw. Versuche einsehen kann – ein indirekter Hinweis auf die neu unterstützte Mehrfachteilnahme an Testklausuren.src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (1)
352-352: LGTM!The test case correctly verifies that unregistered users cannot access exams in a different course.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
525-525: LGTM! Improved result visibility logic for test exams.The condition correctly checks both exam type and test exam status to determine result visibility. This aligns with the PR objective of allowing multiple test exam attempts.
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
361-362: LGTM! Simplified student exam retrieval logic.The change improves code readability by directly using the participation ID to fetch the student exam, rather than going through the participant object.
| /** | ||
| * Returns if the exam is over by checking if the exam end date has passed. | ||
| * | ||
| * @param exam the exam | ||
| * @return true if the exam is over | ||
| */ | ||
| public boolean isExamOver(Exam exam) { | ||
| return exam.getEndDate().isBefore(ZonedDateTime.now()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and consistency of isExamOver method.
The method should be made more consistent with existing exam date checks and handle edge cases:
- Consider grace period like
isExamWithGracePeriodOver - Handle null end date case
- Use consistent time handling with other methods
Apply this diff to improve the implementation:
/**
* Returns if the exam is over by checking if the exam end date has passed.
*
* @param exam the exam
* @return true if the exam is over
+ * @throws IllegalArgumentException if exam end date is null
*/
public boolean isExamOver(Exam exam) {
+ if (exam.getEndDate() == null) {
+ throw new IllegalArgumentException("Exam end date must not be null");
+ }
+ var now = ZonedDateTime.now();
- return exam.getEndDate().isBefore(ZonedDateTime.now());
+ return exam.getEndDate().isBefore(now);
}📝 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.
| /** | |
| * Returns if the exam is over by checking if the exam end date has passed. | |
| * | |
| * @param exam the exam | |
| * @return true if the exam is over | |
| */ | |
| public boolean isExamOver(Exam exam) { | |
| return exam.getEndDate().isBefore(ZonedDateTime.now()); | |
| } | |
| /** | |
| * Returns if the exam is over by checking if the exam end date has passed. | |
| * | |
| * @param exam the exam | |
| * @return true if the exam is over | |
| * @throws IllegalArgumentException if exam end date is null | |
| */ | |
| public boolean isExamOver(Exam exam) { | |
| if (exam.getEndDate() == null) { | |
| throw new IllegalArgumentException("Exam end date must not be null"); | |
| } | |
| var now = ZonedDateTime.now(); | |
| return exam.getEndDate().isBefore(now); | |
| } |
| @Query(""" | ||
| SELECT participation | ||
| FROM ProgrammingExerciseStudentParticipation participation | ||
| LEFT JOIN FETCH participation.submissions s | ||
| WHERE participation.exercise.id = :exerciseId | ||
| AND participation.student.login = :username | ||
| AND participation.testRun = :testRun | ||
| ORDER BY participation.id DESC | ||
| """) | ||
| List<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(@Param("exerciseId") long exerciseId, | ||
| @Param("username") String username, @Param("testRun") boolean testRun); | ||
|
|
||
| default Optional<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRun(long exerciseId, String username, boolean testRun) { | ||
| return findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(exerciseId, username, testRun).stream().findFirst(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding pagination support for test run queries.
The findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc method returns a List without pagination, which could lead to memory issues if there are many test attempts.
Apply this diff to add pagination:
@Query("""
SELECT participation
FROM ProgrammingExerciseStudentParticipation participation
LEFT JOIN FETCH participation.submissions s
WHERE participation.exercise.id = :exerciseId
AND participation.student.login = :username
AND participation.testRun = :testRun
ORDER BY participation.id DESC
""")
- List<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(
+ Page<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(
@Param("exerciseId") long exerciseId,
- @Param("username") String username, @Param("testRun") boolean testRun);
+ @Param("username") String username, @Param("testRun") boolean testRun,
+ Pageable pageable);
default Optional<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRun(
long exerciseId, String username, boolean testRun) {
- return findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(exerciseId, username, testRun)
- .stream().findFirst();
+ return findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(
+ exerciseId, username, testRun, Pageable.ofSize(1))
+ .stream().findFirst();
}📝 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.
| @Query(""" | |
| SELECT participation | |
| FROM ProgrammingExerciseStudentParticipation participation | |
| LEFT JOIN FETCH participation.submissions s | |
| WHERE participation.exercise.id = :exerciseId | |
| AND participation.student.login = :username | |
| AND participation.testRun = :testRun | |
| ORDER BY participation.id DESC | |
| """) | |
| List<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(@Param("exerciseId") long exerciseId, | |
| @Param("username") String username, @Param("testRun") boolean testRun); | |
| default Optional<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRun(long exerciseId, String username, boolean testRun) { | |
| return findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc(exerciseId, username, testRun).stream().findFirst(); | |
| } | |
| @Query(""" | |
| SELECT participation | |
| FROM ProgrammingExerciseStudentParticipation participation | |
| LEFT JOIN FETCH participation.submissions s | |
| WHERE participation.exercise.id = :exerciseId | |
| AND participation.student.login = :username | |
| AND participation.testRun = :testRun | |
| ORDER BY participation.id DESC | |
| """) | |
| Page<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc( | |
| @Param("exerciseId") long exerciseId, | |
| @Param("username") String username, @Param("testRun") boolean testRun, | |
| Pageable pageable); | |
| default Optional<ProgrammingExerciseStudentParticipation> findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRun( | |
| long exerciseId, String username, boolean testRun) { | |
| return findFirstWithSubmissionsByExerciseIdAndStudentLoginAndTestRunOrderByIdDesc( | |
| exerciseId, username, testRun, Pageable.ofSize(1)) | |
| .stream().findFirst(); | |
| } |
| get routerLinkForRepositoryView(): (string | number)[] { | ||
| if (this.isInCourseManagement) { | ||
| return ['..', 'programming-exercises', this.exercise.id!, 'repository', 'USER', this.participation.id!]; | ||
| } | ||
| if (this.routerLink.includes('test-exam')) { | ||
| const parts = this.routerLink.split('/'); | ||
| const examLink = parts.slice(0, parts.length - 2).join('/'); | ||
| return [examLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | ||
| } | ||
| return [this.routerLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and improve type safety.
The router link construction could be more robust with the following improvements:
- Add null checks for
exercise.idandparticipation.id - Use Angular's URL tree manipulation for safer path construction
Apply this diff to improve the implementation:
- get routerLinkForRepositoryView(): (string | number)[] {
+ get routerLinkForRepositoryView(): string[] {
+ if (!this.exercise?.id || !this.participation?.id) {
+ return [];
+ }
+
if (this.isInCourseManagement) {
- return ['..', 'programming-exercises', this.exercise.id!, 'repository', 'USER', this.participation.id!];
+ return ['..', 'programming-exercises', this.exercise.id.toString(), 'repository', 'USER', this.participation.id.toString()];
}
+
if (this.routerLink.includes('test-exam')) {
- const parts = this.routerLink.split('/');
- const examLink = parts.slice(0, parts.length - 2).join('/');
- return [examLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!];
+ const urlTree = this.router.parseUrl(this.routerLink);
+ const segments = urlTree.root.children['primary'].segments;
+ const examSegments = segments.slice(0, -2).map(s => s.path);
+ return [...examSegments, 'exercises', this.exercise.id.toString(), 'repository', this.participation.id.toString()];
}
- return [this.routerLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!];
+ return [this.routerLink, 'exercises', this.exercise.id.toString(), 'repository', this.participation.id.toString()];
}📝 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.
| get routerLinkForRepositoryView(): (string | number)[] { | |
| if (this.isInCourseManagement) { | |
| return ['..', 'programming-exercises', this.exercise.id!, 'repository', 'USER', this.participation.id!]; | |
| } | |
| if (this.routerLink.includes('test-exam')) { | |
| const parts = this.routerLink.split('/'); | |
| const examLink = parts.slice(0, parts.length - 2).join('/'); | |
| return [examLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | |
| } | |
| return [this.routerLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | |
| } | |
| get routerLinkForRepositoryView(): string[] { | |
| if (!this.exercise?.id || !this.participation?.id) { | |
| return []; | |
| } | |
| if (this.isInCourseManagement) { | |
| return ['..', 'programming-exercises', this.exercise.id.toString(), 'repository', 'USER', this.participation.id.toString()]; | |
| } | |
| if (this.routerLink.includes('test-exam')) { | |
| const urlTree = this.router.parseUrl(this.routerLink); | |
| const segments = urlTree.root.children['primary'].segments; | |
| const examSegments = segments.slice(0, -2).map(s => s.path); | |
| return [...examSegments, 'exercises', this.exercise.id.toString(), 'repository', this.participation.id.toString()]; | |
| } | |
| return [this.routerLink, 'exercises', this.exercise.id.toString(), 'repository', this.participation.id.toString()]; | |
| } |
| @@ -111,14 +111,14 @@ public void importRepositories(final ProgrammingExercise templateExercise, final | |||
| String sourceBranch = versionControl.getOrRetrieveBranchOfExercise(templateExercise); | |||
|
|
|||
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |||
There was a problem hiding this comment.
Implement error handling for repository operations.
The TODO comment indicates missing error handling for repository operations. This is critical as failed operations need to be reverted to maintain data consistency.
Consider implementing a transaction-like pattern:
- Track each successful operation
- On failure, revert the tracked operations in reverse order
- Use a try-catch block with cleanup in the finally clause
+ private void revertRepositoryOperations(String sourceProjectKey, String targetProjectKey, List<String> createdRepos) {
+ for (String repo : createdRepos) {
+ try {
+ versionControl.deleteRepository(targetProjectKey, repo);
+ } catch (Exception e) {
+ log.error("Failed to revert repository creation for {}/{}", targetProjectKey, repo, e);
+ }
+ }
+ }📝 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.
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |
| @Service | |
| public class ProgrammingExerciseImportService { | |
| // ... other methods and dependencies (versionControl, log, etc.) | |
| public void importRepositories() { | |
| // Example repository import logic | |
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |
| // Implementation of repository operations goes here. | |
| } | |
| // Newly added helper method for reverting repository operations | |
| private void revertRepositoryOperations(String sourceProjectKey, String targetProjectKey, List<String> createdRepos) { | |
| for (String repo : createdRepos) { | |
| try { | |
| versionControl.deleteRepository(targetProjectKey, repo); | |
| } catch (Exception e) { | |
| log.error("Failed to revert repository creation for {}/{}", targetProjectKey, repo, e); | |
| } | |
| } | |
| } | |
| // ... rest of the class | |
| } |
| private StudentExam getOrCreateTestExam(Exam exam, Course course, User currentUser) { | ||
| StudentExam studentExam; | ||
|
|
||
| if (!examId.equals(exam.getId())) { | ||
| throw new ConflictException("The provided examId does not match with the examId of the studentExam", ENTITY_NAME, "examIdMismatch"); | ||
| if (this.examDateService.isExamOver(exam)) { | ||
| throw new BadRequestAlertException("Test exam has already ended", ENTITY_NAME, "examHasAlreadyEnded", true); | ||
| } | ||
|
|
||
| // Check that the exam is visible | ||
| if (exam.getVisibleDate() != null && exam.getVisibleDate().isAfter(ZonedDateTime.now())) { | ||
| throw new AccessForbiddenException(ENTITY_NAME, examId); | ||
| } | ||
| List<StudentExam> unfinishedStudentExams = studentExamRepository.findStudentExamsForTestExamsByUserIdAndExamId(currentUser.getId(), exam.getId()).stream() | ||
| .filter(attempt -> !attempt.isFinished()).toList(); | ||
|
|
||
| if (exam.isTestExam()) { | ||
| // Check that the current user is registered for the test exam. Otherwise, the student can self-register | ||
| examRegistrationService.checkRegistrationOrRegisterStudentToTestExam(course, exam.getId(), currentUser); | ||
| if (unfinishedStudentExams.isEmpty()) { | ||
| studentExam = studentExamService.generateIndividualStudentExam(exam, currentUser); | ||
| // For the start of the exam, the exercises are not needed. They are later loaded via StudentExamResource | ||
| studentExam.setExercises(null); | ||
| } | ||
| // NOTE: the check examRepository.isUserRegisteredForExam is not necessary because we already checked before that there is a student exam in this case for the current user | ||
|
|
||
| else if (unfinishedStudentExams.size() == 1) { | ||
| studentExam = unfinishedStudentExams.getFirst(); | ||
| } | ||
| else { | ||
| throw new IllegalStateException( | ||
| "User " + currentUser.getId() + " has " + unfinishedStudentExams.size() + " unfinished test exams for exam " + exam.getId() + " in course " + course.getId()); | ||
| } | ||
| // Check that the current user is registered for the test exam. Otherwise, the student can self-register | ||
| examRegistrationService.checkRegistrationOrRegisterStudentToTestExam(course, exam.getId(), currentUser); | ||
| return studentExam; | ||
| } | ||
|
|
There was a problem hiding this comment.
Likely bug in retrieving the single unfinished test exam attempt.
Using unfinishedStudentExams.getFirst() is not standard for Java List; correct usage is typically get(0). This causes a compile or runtime error if no extension method is provided.
- studentExam = unfinishedStudentExams.getFirst();
+ studentExam = unfinishedStudentExams.get(0);📝 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.
| private StudentExam getOrCreateTestExam(Exam exam, Course course, User currentUser) { | |
| StudentExam studentExam; | |
| if (!examId.equals(exam.getId())) { | |
| throw new ConflictException("The provided examId does not match with the examId of the studentExam", ENTITY_NAME, "examIdMismatch"); | |
| if (this.examDateService.isExamOver(exam)) { | |
| throw new BadRequestAlertException("Test exam has already ended", ENTITY_NAME, "examHasAlreadyEnded", true); | |
| } | |
| // Check that the exam is visible | |
| if (exam.getVisibleDate() != null && exam.getVisibleDate().isAfter(ZonedDateTime.now())) { | |
| throw new AccessForbiddenException(ENTITY_NAME, examId); | |
| } | |
| List<StudentExam> unfinishedStudentExams = studentExamRepository.findStudentExamsForTestExamsByUserIdAndExamId(currentUser.getId(), exam.getId()).stream() | |
| .filter(attempt -> !attempt.isFinished()).toList(); | |
| if (exam.isTestExam()) { | |
| // Check that the current user is registered for the test exam. Otherwise, the student can self-register | |
| examRegistrationService.checkRegistrationOrRegisterStudentToTestExam(course, exam.getId(), currentUser); | |
| if (unfinishedStudentExams.isEmpty()) { | |
| studentExam = studentExamService.generateIndividualStudentExam(exam, currentUser); | |
| // For the start of the exam, the exercises are not needed. They are later loaded via StudentExamResource | |
| studentExam.setExercises(null); | |
| } | |
| // NOTE: the check examRepository.isUserRegisteredForExam is not necessary because we already checked before that there is a student exam in this case for the current user | |
| else if (unfinishedStudentExams.size() == 1) { | |
| studentExam = unfinishedStudentExams.getFirst(); | |
| } | |
| else { | |
| throw new IllegalStateException( | |
| "User " + currentUser.getId() + " has " + unfinishedStudentExams.size() + " unfinished test exams for exam " + exam.getId() + " in course " + course.getId()); | |
| } | |
| // Check that the current user is registered for the test exam. Otherwise, the student can self-register | |
| examRegistrationService.checkRegistrationOrRegisterStudentToTestExam(course, exam.getId(), currentUser); | |
| return studentExam; | |
| } | |
| private StudentExam getOrCreateTestExam(Exam exam, Course course, User currentUser) { | |
| StudentExam studentExam; | |
| if (this.examDateService.isExamOver(exam)) { | |
| throw new BadRequestAlertException("Test exam has already ended", ENTITY_NAME, "examHasAlreadyEnded", true); | |
| } | |
| List<StudentExam> unfinishedStudentExams = studentExamRepository.findStudentExamsForTestExamsByUserIdAndExamId(currentUser.getId(), exam.getId()).stream() | |
| .filter(attempt -> !attempt.isFinished()).toList(); | |
| if (unfinishedStudentExams.isEmpty()) { | |
| studentExam = studentExamService.generateIndividualStudentExam(exam, currentUser); | |
| // For the start of the exam, the exercises are not needed. They are later loaded via StudentExamResource | |
| studentExam.setExercises(null); | |
| } | |
| else if (unfinishedStudentExams.size() == 1) { | |
| studentExam = unfinishedStudentExams.get(0); | |
| } | |
| else { | |
| throw new IllegalStateException( | |
| "User " + currentUser.getId() + " has " + unfinishedStudentExams.size() + " unfinished test exams for exam " + exam.getId() + " in course " + course.getId()); | |
| } | |
| // Check that the current user is registered for the test exam. Otherwise, the student can self-register | |
| examRegistrationService.checkRegistrationOrRegisterStudentToTestExam(course, exam.getId(), currentUser); | |
| return studentExam; | |
| } |
| this.router.events.subscribe((event) => { | ||
| if (event instanceof NavigationEnd) { | ||
| this.examParticipationService.loadStudentExamsForTestExamsPerCourseAndPerUserForOverviewPage(this.courseId).subscribe((response: StudentExam[]) => { | ||
| this.studentExams = response!; | ||
| this.prepareSidebarData(); | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing unsubscribe for router events subscription.
The subscription to this.router.events.subscribe may persist if the component is destroyed. Track and unsubscribe in ngOnDestroy to prevent memory leaks.
+ private routerEventsSubscription?: Subscription;
this.routerEventsSubscription = this.router.events.subscribe((event) => {
// ...
});
+ ngOnDestroy(): void {
+ this.routerEventsSubscription?.unsubscribe();
+ // ...
+ }📝 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.
| this.router.events.subscribe((event) => { | |
| if (event instanceof NavigationEnd) { | |
| this.examParticipationService.loadStudentExamsForTestExamsPerCourseAndPerUserForOverviewPage(this.courseId).subscribe((response: StudentExam[]) => { | |
| this.studentExams = response!; | |
| this.prepareSidebarData(); | |
| }); | |
| } | |
| }); | |
| // At the top of the component class | |
| private routerEventsSubscription?: Subscription; | |
| // In the appropriate method (e.g., ngOnInit or after constructor) | |
| this.routerEventsSubscription = this.router.events.subscribe((event) => { | |
| if (event instanceof NavigationEnd) { | |
| this.examParticipationService.loadStudentExamsForTestExamsPerCourseAndPerUserForOverviewPage(this.courseId) | |
| .subscribe((response: StudentExam[]) => { | |
| this.studentExams = response!; | |
| this.prepareSidebarData(); | |
| }); | |
| } | |
| }); | |
| // Add the OnDestroy lifecycle hook to unsubscribe | |
| ngOnDestroy(): void { | |
| this.routerEventsSubscription?.unsubscribe(); | |
| // ... any other cleanup logic | |
| } |
| @@ -318,7 +318,7 @@ describe('ExamParticipationComponent', () => { | |||
| studentExam.ended = true; | |||
| studentExam.submitted = true; | |||
| comp.ngOnInit(); | |||
| expect(loadStudentExamSpy).toHaveBeenCalledOnce(); | |||
| expect(loadStudentExamSpy).not.toHaveBeenCalled(); | |||
There was a problem hiding this comment.
Fix test expectation for loadStudentExamSpy.
The test is failing because it expects loadStudentExamSpy not to be called, but it's being called once. Update the test to match the new component behavior.
-expect(loadStudentExamSpy).not.toHaveBeenCalled();
+expect(loadStudentExamSpy).toHaveBeenCalledOnce();📝 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.
| expect(loadStudentExamSpy).not.toHaveBeenCalled(); | |
| expect(loadStudentExamSpy).toHaveBeenCalledOnce(); |
🧰 Tools
🪛 GitHub Actions: Test
[error] 321-321: Expected number of calls: 0, Received number of calls: 1 for loadStudentExamSpy.
| mapAttemptToSidebarCardElement(attempt: StudentExam, index: number): SidebarCardElement { | ||
| const examCardItem: SidebarCardElement = { | ||
| title: attempt.exam!.title ?? '', | ||
| id: attempt.exam!.id + '/test-exam/' + attempt.id, | ||
| icon: faGraduationCap, | ||
| subtitleLeft: this.translate.instant('artemisApp.courseOverview.sidebar.testExamAttempt') + ' ' + index, | ||
| submissionDate: attempt.submissionDate, | ||
| usedWorkingTime: this.calculateUsedWorkingTime(attempt), | ||
| size: 'L', | ||
| isAttempt: true, | ||
| }; | ||
| return examCardItem; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace non-null assertions with safe property access.
The use of non-null assertion operator (!) could lead to runtime errors if the exam property is undefined.
mapAttemptToSidebarCardElement(attempt: StudentExam, index: number): SidebarCardElement {
+ if (!attempt.exam) {
+ throw new Error('Attempt must have an associated exam');
+ }
const examCardItem: SidebarCardElement = {
- title: attempt.exam!.title ?? '',
- id: attempt.exam!.id + '/test-exam/' + attempt.id,
+ title: attempt.exam.title ?? '',
+ id: `${attempt.exam.id}/test-exam/${attempt.id}`,
icon: faGraduationCap,
subtitleLeft: this.translate.instant('artemisApp.courseOverview.sidebar.testExamAttempt') + ' ' + index,📝 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.
| mapAttemptToSidebarCardElement(attempt: StudentExam, index: number): SidebarCardElement { | |
| const examCardItem: SidebarCardElement = { | |
| title: attempt.exam!.title ?? '', | |
| id: attempt.exam!.id + '/test-exam/' + attempt.id, | |
| icon: faGraduationCap, | |
| subtitleLeft: this.translate.instant('artemisApp.courseOverview.sidebar.testExamAttempt') + ' ' + index, | |
| submissionDate: attempt.submissionDate, | |
| usedWorkingTime: this.calculateUsedWorkingTime(attempt), | |
| size: 'L', | |
| isAttempt: true, | |
| }; | |
| return examCardItem; | |
| } | |
| mapAttemptToSidebarCardElement(attempt: StudentExam, index: number): SidebarCardElement { | |
| if (!attempt.exam) { | |
| throw new Error('Attempt must have an associated exam'); | |
| } | |
| const examCardItem: SidebarCardElement = { | |
| title: attempt.exam.title ?? '', | |
| id: `${attempt.exam.id}/test-exam/${attempt.id}`, | |
| icon: faGraduationCap, | |
| subtitleLeft: this.translate.instant('artemisApp.courseOverview.sidebar.testExamAttempt') + ' ' + index, | |
| submissionDate: attempt.submissionDate, | |
| usedWorkingTime: this.calculateUsedWorkingTime(attempt), | |
| size: 'L', | |
| isAttempt: true, | |
| }; | |
| return examCardItem; | |
| } |
WalkthroughThis PR implements broad refinements related to exam participation and test exam handling across the backend and frontend. It updates method signatures in repositories and services to streamline student exam retrieval, especially for test exam scenarios, and adds new helper methods and fields in domain classes. Frontend components and localization files are also modified to support the refined logic and display detailed test exam attempt information. Several tests have been added or updated to cover these new control flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExamAccessService
participant ExamDateService
participant StudentExamRepository
participant ExamSubmissionService
User->>ExamAccessService: Request exam participation
ExamAccessService->>ExamDateService: Check exam visibility and if exam is over
ExamDateService-->>ExamAccessService: Return exam status
ExamAccessService->>StudentExamRepository: getOrCreateTestExam(...)
StudentExamRepository-->>ExamAccessService: Return StudentExam data
ExamAccessService->>ExamSubmissionService: Process exam submission or retrieval
ExamSubmissionService-->>User: Return exam result/status
Possibly related PRs
Suggested labels
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (1)
502-512: 🛠️ Refactor suggestionAdd JavaDoc documentation for the new method.
The method lacks documentation explaining its purpose, parameters, and return value. This is inconsistent with the documentation style used in other methods in this file.
Add JavaDoc documentation:
+ /** + * Find all active exams for a given user in the specified courses. + * An exam is considered active if: + * - It belongs to one of the specified courses + * - It is visible (current time >= visible date) + * - It hasn't ended (current time <= end date) + * - It is not a test exam + * - The user is registered for it + * + * @param courseIds Set of course IDs to search in + * @param userId ID of the user + * @param visible Current time for visibility check + * @param end Current time for end date check + * @return Set of active exams matching the criteria + */ @Query("""
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
210-211:⚠️ Potential issuePotential security vulnerability: Regular expression injection.
The regular expression pattern is constructed from a user-provided value (
projectKey) without proper escaping, which could lead to regex injection vulnerabilities.Apply this diff to escape special regex characters and validate input:
- String pattern = projectKey.toLowerCase() + "\\d*-"; - String repositoryTypeOrUserNameWithPracticePrefix = repositorySlug.toLowerCase().replaceAll(pattern, ""); + // Escape special regex characters in projectKey + String escapedProjectKey = Pattern.quote(projectKey.toLowerCase()); + String pattern = escapedProjectKey + "\\d*-"; + // Validate input before processing + if (!projectKey.matches("[a-zA-Z0-9-]+")) { + throw new IllegalArgumentException("Invalid project key format: " + projectKey); + } + String repositoryTypeOrUserNameWithPracticePrefix = repositorySlug.toLowerCase().replaceAll(pattern, "");Don't forget to add the import:
import java.util.regex.Pattern;
🧹 Nitpick comments (14)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
525-525: LGTM! Consider adding a clarifying comment.The condition correctly implements the visibility requirements for test exam results while maintaining the existing behavior for normal exam results.
Add a comment to explain the logic:
+ // Test exam results are always visible, while normal exam results are hidden based on the exam service's logic if (participation.getProgrammingExercise().isExamExercise() && !participation.getProgrammingExercise().isTestExamExercise()) {src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java (2)
127-137: Fix typos in documentation.There are typos in the documentation comments:
- "parcitipation" → "participation"
- "particpiations" → "participations"
Apply this diff to fix the typos:
- * Graded course exercises, practice mode course exercises and real exam exercises always have only one parcitipation per exercise - * In case of a test exam, there are multiple participations possible for one exercise - * This field is necessary to preserve the constraint of one partipation per exercise, while allowing multiple particpiations per exercise for test exams + * Graded course exercises, practice mode course exercises and real exam exercises always have only one participation per exercise + * In case of a test exam, there are multiple participations possible for one exercise + * This field is necessary to preserve the constraint of one participation per exercise, while allowing multiple participations per exercise for test exams
294-296: Improve parameter naming in setAttempt method.The parameter name 'index' is less descriptive than 'attempt' which better reflects its purpose.
Apply this diff to improve clarity:
- public void setAttempt(int index) { - this.attempt = index; + public void setAttempt(int attempt) { + this.attempt = attempt;src/test/javascript/spec/service/exam-participation.service.spec.ts (2)
273-283: Consider validating the request body in successful submission test.While the test correctly verifies the API endpoint and successful response, it would be more robust to also validate that the correct student exam data is sent in the request body.
const req = httpMock.expectOne({ method: 'POST' }); expect(req.request.url).toBe('api/courses/1/exams/1/student-exams/submit'); +expect(req.request.body).toEqual(studentExamCopy); req.flush(null);
285-335: Consider using constants for error messages to improve maintainability.The error message strings are duplicated across test cases. Consider extracting them into constants to make maintenance easier and prevent typos.
const ERROR_MESSAGES = { SUBMISSION_NOT_IN_TIME: 'artemisApp.studentExam.submissionNotInTime', ALREADY_SUBMITTED: 'artemisApp.studentExam.alreadySubmitted', HAND_IN_FAILED: 'artemisApp.studentExam.handInFailed', } as const;Then use them in the tests:
-error: (err) => expect(err.message).toBe('artemisApp.studentExam.submissionNotInTime'), +error: (err) => expect(err.message).toBe(ERROR_MESSAGES.SUBMISSION_NOT_IN_TIME),src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
654-702: LGTM! Implementation supports multiple test exam participations.The changes correctly implement the ability to participate in test exams multiple times while maintaining the existing behavior for regular exams.
Consider optimizing database queries.
The comment indicates a potential performance issue with multiple database calls:
// NOTE: it's not ideal to invoke the next line several times (2000 student exams with 10 exercises would lead to 20.000 database calls to find all participations).Consider implementing the suggested optimization to load all participations per exercise once and check existence in memory. While this might require more memory, it would significantly reduce database load. You could:
- Load participations in batches
- Use a caching mechanism
- Implement the TODO comment about directly checking participation existence in the database
Would you like me to propose a specific implementation for any of these approaches?
src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (2)
502-512: Improve parameter naming for better clarity.The parameter names
visibleandendcould be more descriptive to better convey their purpose.Apply this diff to improve parameter naming:
- Set<Exam> findActiveExams(@Param("courseIds") Set<Long> courseIds, @Param("userId") long userId, @Param("visible") ZonedDateTime visible, @Param("end") ZonedDateTime end); + Set<Exam> findActiveExams(@Param("courseIds") Set<Long> courseIds, @Param("userId") long userId, @Param("visible") ZonedDateTime visibilityDate, @Param("end") ZonedDateTime endDate);
502-512:❓ Verification inconclusive
Consider adding database indexes for query optimization.
The query filters on multiple columns which could benefit from appropriate database indexes to improve performance, especially for courses with many exams and users.
Consider adding the following indexes:
- On
Exam.course_idandExam.visible_date- On
Exam.end_date- On
ExamUser.user_idYou can verify the query performance and index usage with:
🏁 Script executed:
#!/bin/bash # Description: Check if the necessary indexes exist for optimal query performance # Search for index definitions in migration files fd -e sql -e java | xargs rg -A 5 'CREATE.*INDEX.*exam|ALTER.*TABLE.*exam.*ADD.*INDEX'Length of output: 88
Database Indexes Verification: Manual Check Recommended
The review comment advises adding indexes to improve query performance, which remains valid. Our initial script did not reveal any index definitions. However, the absence of output may indicate that index declarations are managed elsewhere (e.g., via JPA annotations) or weren’t detected. Please manually verify whether indexes exist in migration scripts or through JPA mappings. Specifically, review the following:
- Exam table: Ensure indexes are created on
course_id,visible_date, andend_date.- ExamUser table: Confirm an index exists for
user_id.If neither migrations nor JPA annotations include these indexes, consider adding them as recommended.
src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts (1)
42-44: Add null check for exam object.The initialization of
startDateandendDateshould include a null check for theexamobject to prevent potential runtime errors.- this.startDate = this.exam.startDate; - this.endDate = this.exam.endDate; + this.startDate = this.exam?.startDate; + this.endDate = this.exam?.endDate;src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts (1)
142-158: Add edge case tests for undefined dates.Consider adding tests for scenarios where dates are undefined in test exams.
it('should handle undefined start date in test exam', () => { exam.testExam = true; exam.startDate = undefined; component.exam = exam; component.studentExam = studentExam; fixture.detectChanges(); expect(component.startDate).toBeUndefined(); }); it('should handle undefined end date in test exam', () => { exam.testExam = true; exam.endDate = undefined; component.exam = exam; component.studentExam = studentExam; fixture.detectChanges(); expect(component.endDate).toBeUndefined(); });src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (1)
148-174: Consider filtering unfinished attempts in the repository query.
Fetching all test exam attempts and filtering them in memory (.filter(...)) may be suboptimal for large datasets. A specialized repository method to retrieve only unfinished attempts could improve performance.src/main/webapp/app/overview/course-exams/course-exams.component.ts (2)
72-72: Mapping test exams by exam ID.
UsingMap<number, StudentExam[]>to store multiple attempts per test exam is a clean approach, though you could consider an object literal if needed for Angular change detection.
347-367: Periodic re-check for working time.
Usinginterval(1000)is straightforward for a live countdown, but it could become resource-heavy if many are active. A single shared timer or managing multiple subscriptions carefully might be worth considering.src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (1)
143-156: Consider indexing for performance optimization.The query effectively retrieves the latest participation with legal submissions. However, for optimal performance:
- Consider adding an index on
exercise_idandstudent_logincolumns- Consider adding an index on
submission.typeif not already present
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/main/resources/config/liquibase/changelog/20240513101552_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101555_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101557_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (44)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(1 hunks)src/main/webapp/app/exam/participate/exam-participation.component.ts(5 hunks)src/main/webapp/app/exam/participate/exam-participation.route.ts(0 hunks)src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts(2 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html(1 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/overview/course-exams/course-exams.component.ts(9 hunks)src/main/webapp/app/overview/course-overview.service.ts(4 hunks)src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html(1 hunks)src/main/webapp/app/types/sidebar.ts(2 hunks)src/main/webapp/i18n/de/exam.json(2 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/exam.json(2 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java(1 hunks)src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts(7 hunks)src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts(1 hunks)src/test/javascript/spec/service/exam-participation.service.spec.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java
- src/main/webapp/app/exam/participate/exam-participation.route.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
🧰 Additional context used
📓 Path-based instructions (6)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/exam/service/ExamServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.tssrc/test/javascript/spec/service/exam-participation.service.spec.tssrc/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.htmlsrc/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.tssrc/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.tssrc/main/webapp/app/types/sidebar.tssrc/main/webapp/app/overview/course-overview.service.tssrc/main/webapp/app/overview/course-exams/course-exams.component.tssrc/main/webapp/app/exam/participate/exam-participation.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/exam.jsonsrc/main/webapp/i18n/de/student-dashboard.json
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/exercise/domain/participation/Participation.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.javasrc/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.javasrc/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java
🧠 Learnings (7)
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java (1)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (3)
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.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (3)
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:975-980
Timestamp: 2024-11-12T12:51:51.200Z
Learning: The `testSubmitStudentExam_notInTime` 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.
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
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().
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (2)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
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().
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (4)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/web/rest/StudentExamResource.java:329-332
Timestamp: 2024-11-12T12:51:40.391Z
Learning: The error message for an already abandoned exam in `submitStudentExam` method of `StudentExamResource.java` does not need to include the student exam ID as it is already included in the error logging.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/web/rest/StudentExamResource.java:892-892
Timestamp: 2024-11-12T12:51:46.554Z
Learning: Valentin-boehm has indicated that including detailed error messages for self-explanatory exceptions such as a BadRequestException when a student exam is already submitted or abandoned is not necessary in the context of `StudentExamResource.java`.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (103)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
361-363: LGTM! The changes align with the PR objectives.The modification to use
findByExamIdAndParticipationIdinstead of retrieving the student exam through the participant is a good change that:
- Supports the new multiple participations feature for test exams
- Simplifies the code by removing an unnecessary indirection through the participant
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java (1)
564-565: LGTM! Good optimization to reduce response payload size.Removing the redundant course reference helps minimize the data sent to clients, as the course information is already available through the exercise group and exam hierarchy.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java (2)
115-134: LGTM! Well-structured method with clear documentation.The method is well-implemented with proper error handling and follows best practices.
148-176: LGTM! Well-implemented method for test run participation retrieval.The method effectively handles test run participations with proper error handling and Optional usage.
src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java (2)
900-901: LGTM! Correctly updated mock setup for the new attempt parameter.The mock setup is properly updated to handle the new
Integer attemptparameter using the appropriate Mockito matcher.
916-916: LGTM! Correctly updated mock setup for the new attempt parameter.The mock setup is properly updated to handle the new
Integer attemptparameter using the appropriate Mockito matcher.src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
1-297: Well-structured implementation with comprehensive documentation.The class demonstrates good practices:
- Follows single responsibility principle
- Uses OS-independent path handling
- Provides comprehensive documentation
- Implements consistent error handling
- Uses appropriate access modifiers
src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java (1)
127-137: Implementation successfully supports multiple test exam attempts!The new
attemptfield and its accessors effectively enable tracking multiple test exam participations while maintaining existing constraints for other exercise types. The implementation is well-documented and aligns perfectly with the PR objectives.Also applies to: 290-296
src/test/javascript/spec/service/exam-participation.service.spec.ts (2)
22-22: LGTM!The additional imports are necessary for testing error scenarios and are correctly placed.
49-49: LGTM!Proper initialization of the
exercisesarray prevents potential null pointer issues in tests.src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (2)
641-646: LGTM! But please address the TODO comment.The method signature change aligns with the PR objectives. However, there's a TODO comment about scheduling a lock operation for student repositories that needs to be addressed.
Please ensure that the locking mechanism for student repositories at the end of individual working time is implemented to prevent unauthorized access after the time limit.
648-654: LGTM! Documentation is clear and accurate.The JavaDoc has been properly updated to reflect the parameter changes.
src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (5)
181-181: LGTM! Method name better reflects its functionality.The renamed method clearly indicates that it fetches exercises, sessions, and student participations, improving code readability and maintainability.
465-465: LGTM! Method name updated to reflect multiple results.The change from singular to plural form accurately represents that multiple test exams can be returned for a user, aligning with the new multiple participation feature.
489-489: LGTM! Method now includes student participations in the fetch.The updated method eagerly loads student participations along with exercises, which is necessary for handling multiple participations in test exams.
564-564: LGTM! Method name now clearly indicates its behavior.The renamed method better expresses that it returns a single result and throws an exception if not found, improving code clarity.
749-750: LGTM! Clear setup for test exam participations.The added code properly handles the setup of new participations for test exam exercises, with clear comments explaining the purpose.
src/main/webapp/i18n/en/exam.json (2)
60-60: Added "resume" key for Test Exam resume functionality.
The new key"resume": "Resume Attempt"is clearly added in thetestExamsection. This provides a succinct label for users to resume an exam attempt.
303-303: Updated "noFurtherAttempts" message for clarity.
The text change updates "submission" to "submissions" to better indicate that multiple submissions may be reviewed. This improves grammatical consistency and clarity for users.src/main/webapp/i18n/de/exam.json (2)
60-60: Hinzugefügter "resume"-Schlüssel für die Fortführung eines Testversuchs.
Der neue Schlüssel"resume": "Versuch fortführen"wurde imtestExam-Bereich eingefügt. Die Formulierung ist klar und entspricht dem informellen Sprachstil (du/dein), wie in den Vorgaben verlangt.
303-303: Aktualisierte Nachricht bei "noFurtherAttempts" für verbesserte Klarheit.
Die Änderung von "Abgabe" zu "Abgaben" (im Plural) verbessert die Aussage, dass mehrere Abgaben eingesehen werden können. Dies sorgt für höhere grammatische und inhaltliche Konsistenz.src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (6)
14-14: Conditional Styling Update:
The addition of[ngClass]="{ 'fw-bold': sidebarItem.isAttempt }"on this<div>correctly applies a bold font style when the exam attempt is marked. Ensure that theisAttemptproperty is reliably set in the component logic.
19-22: Dynamic Translation Key Binding:
The<small>element now dynamically assigns the translation key based onsidebarItem.isAttempt. This ensures that the label switches between 'artemisApp.courseOverview.sidebar.submissionDate' and 'artemisApp.courseOverview.sidebar.start' as required. Verify that both translation keys are present in your localization files.
24-33: Conditional Date Display:
The template now distinguishes between displaying dates for attempted exams and non-attempts. WhensidebarItem.isAttemptis true, it showssubmissionDate(formatted usingartemisDate); otherwise, it checks and displaysstartDateWithTimeif available. Consider adding a fallback or default text if neither date is provided, though this behavior may be handled upstream.
38-49: Used Working Time Display for Attempts:
A new section has been introduced to display the used working time whensidebarItem.isAttemptis true. It conditionally formats the working time usingartemisDurationFromSecondsand falls back to a '-' placeholder if no value exists. This enhances the clarity of test exam results.
50-59: Non-Attempt Working Time Presentation:
In the non-attempt branch, the code now displays working time using a dual approach: ifsidebarItem.studentExamis present, the<jhi-student-exam-working-time>component is rendered; otherwise, it uses theworkingTimevalue formatted via the duration pipe. This conditional rendering is clear and aligns with the intended functionality.
61-77: Test Exam vs. Attainable Points Display:
The updated block cleanly differentiates between test exam and regular exam UI behavior. WhensidebarItem.testExamis true, it displays the number of attempts; if false, it shows attainable points. This change supports the PR objective of handling multiple test exam attempts while preserving functionality for normal and practice exams.src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java (1)
128-133: LGTM! Well-documented interface changes.The addition of the
attemptparameter to thecopyRepositorymethod is clean and properly documented with JavaDoc.src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java (1)
107-111:❓ Verification inconclusive
Consider potential repository name collisions.
The current implementation might lead to repository name collisions in edge cases. For example, if
targetProjectKeyis "exam1" andattemptis 2, it would collide with a repository wheretargetProjectKeyis "exam12".Let's verify if this is a potential issue by checking for existing repositories with similar naming patterns:
🏁 Script executed:
#!/bin/bash # Description: Search for potential repository name collisions # Example: "exam1" with attempt 2 vs "exam12" # Search for repository patterns that could lead to collisions rg -i 'exam\d+\d+-' --type-add 'config:*.{java,xml,properties,yml}' --type configLength of output: 232
Repository Naming Collision Verification
The current concatenation logic (i.e., appending the numeric attempt directly to the lower-cased project key) may lead to ambiguous repository names—for instance,
"exam1" + 2becomes"exam12", which could collide with an already existing"exam12"if formed differently. The automated search did not yield direct evidence due to file filter limitations, so please manually verify that this naming strategy does not result in unintended collisions. If collisions are a concern, consider inserting a clear delimiter (such as an underscore or additional hyphen) between the project key and the attempt number.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
114-116:❓ Verification inconclusive
Verify the usage of null for the attempt parameter.
The
attemptparameter is being set tonullfor all repository copies. Let's verify if this is the intended behavior and if it aligns with the multiple test exam attempts feature.
🏁 Script executed:
#!/bin/bash # Search for other usages of copyRepository to understand the expected values for the attempt parameter ast-grep --pattern 'copyRepository($_, $_, $_, $_, $_, $_)'Length of output: 61
Action Required: Manual Verification Needed for the Attempt Parameter in copyRepository Calls
The script to search for usages of
copyRepositoryreturned no results, so it's unclear whether passingnullfor the attempt parameter is the intended behavior. Please manually verify the following:
- Confirm if
nullis acceptable within the context of the multiple test exam attempts feature or if a specific value should be provided.- Review the implementation of
versionControl.copyRepositoryto understand how the attempt parameter is processed.- Check related tests and documentation for any guidance on the expected value.
src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts (2)
31-31: LGTM! Property declaration follows Angular style guide.The new
endDateproperty is correctly typed as an optionaldayjs.Dayjsand follows camelCase naming convention.
31-31:❓ Verification inconclusive
Verify usage of the new endDate property.
The
endDateproperty is added and initialized but doesn't appear to be used in the template or other methods. Please verify if this property should be displayed in the information box similar tostartDate.Also applies to: 42-44
🏁 Script executed:
#!/bin/bash # Search for endDate usage in template and related files rg -l "endDate" src/main/webapp/app/exam/participate/exam-start-information/Length of output: 177
Verify and Integrate
endDateUsage in Exam Start Information
TheendDateproperty is declared (line 31 in the component) but appears not to be referenced anywhere in the TypeScript code or in the associated HTML template. Please verify the following:
- Confirm whether
endDateshould be displayed in the UI (e.g., in the information box alongsidestartDate).- If it’s intended to be used, add the corresponding template bindings and any related logic.
- Otherwise, consider removing the unused property to keep the code clean.
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts (2)
142-149: LGTM! Well-structured test for test exam start date.The test follows AAA pattern and uses appropriate Jest matchers.
151-158: LGTM! Well-structured test for test exam end date.The test follows AAA pattern and uses appropriate Jest matchers.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (8)
6-6: No issues with the new import.
The newly introducedListimport is consistent with the code changes below.
52-56: Constructor injection looks good.
InjectingExamDateServicevia the constructor follows best practices. This addition is appropriately declared asfinal.Also applies to: 64-64
83-85: Exam retrieval and course validation are clear.
Fetching the exam with its exercise groups before validating ownership ensures consistency and avoids partial data issues.
86-89: Visibility check is appropriate.
ThrowingAccessForbiddenExceptionwhen the exam isn't visible prevents early access. The error handling aligns with the intended logic.
91-104: Test exam vs. normal exam branching is straightforward.
Splitting offgetOrCreateTestExamorgetOrCreateNormalExamclarifies the logic. The final conflict check is valid to prevent mismatched IDs.
126-126: Helpful clarification in the comment.
The updated comment clearly states the requirement for user registration and exam start eligibility.
133-134: Consistent access restriction.
Guarding the exam generation for unregistered users withAccessForbiddenExceptionmatches the principle of least access perfectly.
145-146: Return statement is fine.
No concerns with the final return of theStudentExamobject here.src/main/webapp/app/overview/course-exams/course-exams.component.ts (12)
4-5: Expanded RxJS imports.
Newly importedNavigationEnd,Subscription,interval, andlastValueFromsupport the router event subscription and timed logic.
23-23: Adding an 'attempt' category.
Introducing the “attempt” group and collapse options is consistent with the new test exam attempts feature.Also applies to: 29-29, 35-35
63-63: Tracking state with a dedicated Subscription.
studentExamStateis declared to handle ongoing updates. Make sure it’s unsubscribed to prevent leaks; the code below addresses that.
79-79:withinWorkingTimeproperty introduced.
This boolean flags whether the current exam attempt is active. It’s straightforward and suits the single-threaded Angular environment.
110-110: Re-invoking sidebar data preparation.
CallingprepareSidebarData()after receiving student exams updates the UI promptly. No issues here.
113-119: Router event subscription reloads test exam data.
Be mindful of performance if these reloads become frequent. For typical exam usage, this is likely acceptable.
289-295: Populating testExamMap with submitted attempts.
Filtering out only submitted attempts is logical. InvokingcalculateIndividualWorkingTimeForTestExamsfor each attempt is also correct for display logic.
305-310: Accumulating sidebar items.
Merging real exams, test exams, and attempts into one array provides a unified display. The approach is consistent with your grouping strategy.
321-341: Helper methods for retrieving student exams and indices.
These utility functions improve clarity. The naming is consistent with Angular best practices (camelCase).
342-345: Quick attempt count retrieval.
getNumberOfAttemptsForTestExamis concise and easy to follow.
369-374: Safe unsubscription.
unsubscribeFromExamStateSubscriptionensures no memory leaks from thestudentExamState. Good practice.
379-384: Working time logic.
isWithinWorkingTimeprecisely checks the relevant date constraints. The usage ofdayjsfor date comparison is consistent with the rest of the codebase.src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts (1)
104-115: New dynamic repository router link.
The getter gracefully handles multiple routing scenarios, slicing paths for test exams vs. normal usage. This approach is flexible and enhances code clarity.src/main/webapp/app/types/sidebar.ts (2)
10-10: LGTM!The addition of 'attempt' to ExamGroupCategory type aligns with the PR objective of supporting multiple test exam attempts.
135-154: LGTM!The new properties are well-documented and appropriately typed to support test exam attempts functionality. The naming follows the camelCase convention as per coding guidelines.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (2)
105-116: LGTM!The changes improve the logic by clearly separating test exam and real exam handling. The use of
max(Comparator.comparing(StudentExam::getId))ensures retrieval of the latest unsubmitted test exam.
155-156: LGTM!The condition has been appropriately updated to exclude test exam exercises from multiple submission prevention, aligning with the PR objective of allowing multiple test exam attempts.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java (2)
40-48: LGTM!The new method is well-documented and provides a clear, focused implementation for checking if an exam is over.
113-116: LGTM!The changes appropriately handle multiple student exams for test exams while maintaining the existing behavior for real exams. The comments clearly explain the logic.
src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java (3)
88-92: LGTM!The studentParticipations field is properly configured with appropriate JPA annotations and follows the class's established patterns for collections.
209-215: LGTM!The getter and setter methods follow Java bean convention and provide appropriate access to the studentParticipations field.
249-257: LGTM!The isFinished method is well-documented and correctly implements the logic for determining exam completion based on both submission status and time constraints.
src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java (1)
74-86: Well-structured test method with comprehensive assertions!The test method effectively validates the
LocalVCRepositoryUriconstructor for test exam attempts, with clear assertions checking all relevant properties.src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (2)
255-262: Well-structured test for expired exam scenario!The test effectively validates that accessing an expired exam returns a BAD_REQUEST status.
264-271: Well-structured test for multiple unfinished attempts!The test effectively validates that having multiple unfinished attempts returns an INTERNAL_SERVER_ERROR status.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
80-195: Well-structured repository methods with comprehensive functionality!The new methods provide a complete set of operations for managing student participations, with proper entity graphs, error handling, and support for test exam scenarios.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java (1)
266-266: Good addition for proper test data setup!Setting the studentExam ID ensures proper test execution by providing complete test data.
src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java (2)
45-46: LGTM! Efficient query implementation.The method correctly uses
@EntityGraphfor optimized loading of the related entities.
198-205: LGTM! Well-structured query.The JPQL query is correctly implemented with proper joins and parameter annotations.
src/main/webapp/app/overview/course-overview.service.ts (3)
300-304: LGTM! Clean implementation.The method handles optional parameters correctly and follows TypeScript best practices.
352-368: LGTM! Good property additions.The method correctly extends the sidebar card element with test exam properties.
467-475: LGTM! Robust time calculation.The method handles time calculations safely with proper null checks and correctly limits the working time to the regular exam duration.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (1)
352-352: LGTM! Test case correctly updated.The test now properly verifies access denial for unregistered users in a different course.
src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1)
378-381: LGTM! Clear and concise implementation.The method correctly combines the exam exercise and test exam checks with proper JSON serialization handling.
src/main/webapp/app/exam/participate/exam-participation.component.ts (4)
63-63: LGTM! Component import and registration.The addition of
ExamExerciseOverviewPageComponentis properly structured with both the import statement and registration in the component's imports array.Also applies to: 89-89
242-250: LGTM! Test exam handling.The new conditional block properly handles test exam scenarios by:
- Checking both
testExamandstudentExamIdconditions- Using the appropriate service method
- Including proper error handling
454-454: LGTM! Test exam navigation.The navigation route structure for test exams has been updated to use a more specific path:
['courses', courseId, 'exams', examId, 'test-exam', studentExam.id].
224-228:❓ Verification inconclusive
Verify route parameter access.
The logic for retrieving
studentExamIdhas been simplified to usefirstChild?.snapshot.params. While this change is valid, ensure that:
- The route structure consistently provides the studentExamId parameter in the child route.
- The parameter is always available when needed.
Run this script to verify the route structure:
🏁 Script executed:
#!/bin/bash # Description: Check route configuration for studentExamId parameter # Test: Search for route definitions containing studentExamId. Expect: Consistent route structure. rg -A 5 "path.*:.*studentExamId"Length of output: 34
Below is an updated review comment based on the verification steps so far. Since the initial search did not return any evidence for a route definition containing
:studentExamId, it’s important to manually verify that the parameter is indeed defined in the child route. You might also consider running an additional search with a more specific pattern. For example:If this command still returns no output, please manually inspect your route configuration files (especially those corresponding to the child routes of exam participation) to confirm that the
studentExamIdparameter is provided as expected.Action:
- Manually verify that the child route supplying the exam participation component defines the
:studentExamIdparameter.- Ensure that when navigating to the component,
studentExamIdis correctly passed and available—if not, adjust your route configuration or retrieval code accordingly.This confirmation is key to ensuring that accessing
this.route.firstChild?.snapshot.params['studentExamId']works reliably at runtime.
🏁 Script executed:
[run_scripts] #!/bin/bash # Description: Re-check for the presence of the studentExamId parameter in route configurations using a more precise pattern. rg -A 5 ":studentExamId"Length of output: 146
Review Verification: Confirm Route Parameter
The current verification steps did not return conclusive evidence that the child route consistently provides the
studentExamIdparameter. Please ensure that:
- The route configuration for the component at
src/main/webapp/app/exam/participate/exam-participation.component.tsdefines the:studentExamIdparameter in its child routes.- Manual verification of the child route configuration confirms that
studentExamIdis reliably available when navigating to the exam participation component.If the parameter is not defined, update the route configuration accordingly. Otherwise, if the parameter is defined elsewhere or passed by a resolver, please document its usage to avoid confusion.
src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts (3)
285-303: LGTM! Test case for loading test exam.The test case properly verifies:
- Loading of test exam data with 'start' as studentExamId
- Correct API call sequence
- Proper assignment of studentExam properties
305-326: LGTM! Test case for loading test exam summary.The test case thoroughly validates:
- Loading of test exam summary data
- Correct API call expectations
- Proper studentExam ID handling
1096-1096: LGTM! Test exam layout handling.The test cases properly verify exam layout behavior by:
- Setting testExam flag appropriately
- Checking layout reset behavior based on testExam status
Also applies to: 1116-1116
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (3)
130-130: LGTM! Method for retrieving latest participation.The method follows Spring Data JPA naming conventions and properly orders results by ID in descending order.
782-808: LGTM! Test exam participation methods.The new methods for test exam participations:
- Use appropriate JOIN clauses for eager loading
- Include proper filtering conditions
- Follow consistent naming patterns
1024-1024: LGTM! Test exam handling enhancement.The changes properly:
- Update documentation to clarify test exam handling
- Add specific logic for test exam scenarios
- Maintain backward compatibility with existing code
Also applies to: 1035-1042
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (1)
14-14: Refactored Router Link Binding
The inline conditional logic for determining the router link has been replaced with a direct property binding ([routerLinkForRepositoryView]="routerLinkForRepositoryView"). This simplifies the template and pushes the logic into the component’s TypeScript file (via the getter). Verify that the getter in the TS file returns the correct routing array as expected.src/main/webapp/i18n/en/student-dashboard.json (1)
60-67: Added Localization Keys for Test Exam Details
New keys—"attempt": "Test Exam Attempts","testExamAttempt": "Attempt","attempts": "Attempts","submissionDate": "Submission Date", and"usedWorkingTime": "Used Working Time"—have been added to support the enhanced UI for test exam participations. Please ensure these keys are utilized consistently throughout the UI and match the intended functionality.src/main/webapp/i18n/de/student-dashboard.json (1)
60-67: Updated German Localization for Test Exam Details
The German localization now includes keys tailored for test exam attempts:"attempt": "Testklausurversuche","testExamAttempt": "Versuch","attempts": "Versuche","submissionDate": "Abgabedatum", and"usedWorkingTime": "Gebrauchte Arbeitszeit". These entries use informal language consistent with the coding guidelines. Confirm that they integrate smoothly with the rest of the dashboard labels.src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java (14)
142-149: Doc update is clear and descriptive.These changes provide a thorough explanation of how the test exam logic is handled when starting an exercise. No problems identified.
154-157: Good parameter documentation.It concisely explains the reasoning for creating an initial submission in non-programming exercises. No issues found.
161-162: Local variable declaration is neatly organized.No concerns regarding the initialization of these variables.
164-172: Confirm off-by-one logic for test exam attempts.Currently, the new attempt is set to
participations.size(), which labels the first attempt as 0 if there are no prior participations. Verify if this is the intended numbering or if you want the first attempt labeled as 1.
175-194: Practice mode reuse logic looks consistent.Marking existing practice participations as inactive and then creating or reusing a new one appears correct.
199-202: Smoothly switches between programming and non-programming flow.The branching to either
startProgrammingExerciseor non-programming initialization is straightforward and clear.
229-230: Param documentation for createNewParticipation is concise.No improvements needed here.
260-261: Updated doc lines for startProgrammingExercise are clear.These succinctly describe the parameters. Looks good.
263-263: Signature refactor for startProgrammingExercise is logical.Removing the initialization date parameter clarifies the method usage.
291-294: Setting attempt to 1 in practice mode aligns with the overall approach.Helpful to keep practice attempts clearly distinct from test exam attempts.
297-297: Refined private method signature.No issues with removing setInitializationDate calls here.
476-476: Including the attempt index for copyRepository is sensible.This ensures each repository copy is uniquely identifiable among multiple test exam attempts.
593-596: Retrieving latest test exam participation looks appropriate.Using the highest ID as a proxy for the latest attempt is generally reliable. No issues found.
671-674: Consistently returns the last test exam participation with eager submissions.The logic mirrors the approach used elsewhere for test exams. No concerns.
Also applies to: 676-676
| public VcsRepositoryUri copyRepository(String sourceProjectKey, String sourceRepositoryName, String sourceBranch, String targetProjectKey, String targetRepositoryName, | ||
| Integer attempt) throws VersionControlException { | ||
| sourceRepositoryName = sourceRepositoryName.toLowerCase(); | ||
| targetRepositoryName = targetRepositoryName.toLowerCase(); | ||
| final String targetRepoSlug = targetProjectKey.toLowerCase() + "-" + targetRepositoryName; | ||
| String targetProjectKeyLowerCase = targetProjectKey.toLowerCase(); | ||
| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | ||
| targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; | ||
| } | ||
| final String targetRepoSlug = targetProjectKeyLowerCase + "-" + targetRepositoryName; | ||
| // get the remote url of the source repo |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the attempt parameter.
The implementation should validate the attempt number to ensure it stays within reasonable bounds and follows the expected format.
Apply this diff to add validation:
public VcsRepositoryUri copyRepository(String sourceProjectKey, String sourceRepositoryName, String sourceBranch, String targetProjectKey, String targetRepositoryName,
Integer attempt) throws VersionControlException {
+ // Validate attempt number
+ if (attempt != null && (attempt < 0 || attempt > 255)) {
+ throw new IllegalArgumentException("Attempt number must be between 0 and 255");
+ }
sourceRepositoryName = sourceRepositoryName.toLowerCase();
targetRepositoryName = targetRepositoryName.toLowerCase();
String targetProjectKeyLowerCase = targetProjectKey.toLowerCase();
if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) {
targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt;
}
final String targetRepoSlug = targetProjectKeyLowerCase + "-" + targetRepositoryName;📝 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.
| public VcsRepositoryUri copyRepository(String sourceProjectKey, String sourceRepositoryName, String sourceBranch, String targetProjectKey, String targetRepositoryName, | |
| Integer attempt) throws VersionControlException { | |
| sourceRepositoryName = sourceRepositoryName.toLowerCase(); | |
| targetRepositoryName = targetRepositoryName.toLowerCase(); | |
| final String targetRepoSlug = targetProjectKey.toLowerCase() + "-" + targetRepositoryName; | |
| String targetProjectKeyLowerCase = targetProjectKey.toLowerCase(); | |
| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | |
| targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; | |
| } | |
| final String targetRepoSlug = targetProjectKeyLowerCase + "-" + targetRepositoryName; | |
| // get the remote url of the source repo | |
| public VcsRepositoryUri copyRepository(String sourceProjectKey, String sourceRepositoryName, String sourceBranch, String targetProjectKey, String targetRepositoryName, | |
| Integer attempt) throws VersionControlException { | |
| // Validate attempt number | |
| if (attempt != null && (attempt < 0 || attempt > 255)) { | |
| throw new IllegalArgumentException("Attempt number must be between 0 and 255"); | |
| } | |
| sourceRepositoryName = sourceRepositoryName.toLowerCase(); | |
| targetRepositoryName = targetRepositoryName.toLowerCase(); | |
| String targetProjectKeyLowerCase = targetProjectKey.toLowerCase(); | |
| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | |
| targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; | |
| } | |
| final String targetRepoSlug = targetProjectKeyLowerCase + "-" + targetRepositoryName; | |
| // get the remote url of the source repo |
|
|
||
| List<AuxiliaryRepository> auxRepos = templateExercise.getAuxiliaryRepositories(); | ||
| for (int i = 0; i < auxRepos.size(); i++) { | ||
| AuxiliaryRepository auxRepo = auxRepos.get(i); | ||
| var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName()).toString(); | ||
| var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null).toString(); |
There was a problem hiding this comment.
Add null check to prevent NullPointerException.
The toString() call on the repository URI could throw NullPointerException if the copy operation fails.
Apply this diff to add null check:
-var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null).toString();
+var copiedRepo = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null);
+if (copiedRepo == null) {
+ throw new IllegalStateException("Failed to copy auxiliary repository: " + auxRepo.getName());
+}
+var repoUri = copiedRepo.toString();📝 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.
| var repoUri = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null).toString(); | |
| var copiedRepo = versionControl.copyRepository(sourceProjectKey, auxRepo.getRepositoryName(), sourceBranch, targetProjectKey, auxRepo.getName(), null); | |
| if (copiedRepo == null) { | |
| throw new IllegalStateException("Failed to copy auxiliary repository: " + auxRepo.getName()); | |
| } | |
| var repoUri = copiedRepo.toString(); |
| @@ -111,14 +111,14 @@ | |||
| String sourceBranch = versionControl.getOrRetrieveBranchOfExercise(templateExercise); | |||
|
|
|||
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |||
There was a problem hiding this comment.
Implement error handling and rollback mechanism.
The TODO comment indicates missing error handling. With the addition of the new attempt parameter, proper error handling becomes more critical to ensure data consistency.
Consider implementing a rollback mechanism:
+ private void rollbackRepositoryCopies(String sourceProjectKey, String targetProjectKey, List<String> copiedRepos) {
+ for (String repo : copiedRepos) {
+ try {
+ versionControl.deleteRepository(targetProjectKey, repo);
+ } catch (Exception e) {
+ log.error("Failed to rollback repository copy: {}", repo, e);
+ }
+ }
+ }Then use it in the import method:
+ List<String> copiedRepos = new ArrayList<>();
try {
versionControl.copyRepository(...);
+ copiedRepos.add(RepositoryType.TEMPLATE.getName());
versionControl.copyRepository(...);
+ copiedRepos.add(RepositoryType.SOLUTION.getName());
versionControl.copyRepository(...);
+ copiedRepos.add(RepositoryType.TESTS.getName());
} catch (Exception e) {
+ rollbackRepositoryCopies(sourceProjectKey, targetProjectKey, copiedRepos);
throw e;
}📝 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.
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |
| // Previous code context ... | |
| // TODO: in case one of those operations fail, we should do error handling and revert all previous operations | |
| private void rollbackRepositoryCopies(String sourceProjectKey, String targetProjectKey, List<String> copiedRepos) { | |
| for (String repo : copiedRepos) { | |
| try { | |
| versionControl.deleteRepository(targetProjectKey, repo); | |
| } catch (Exception e) { | |
| log.error("Failed to rollback repository copy: {}", repo, e); | |
| } | |
| } | |
| } | |
| public void importProgrammingExercise() { | |
| // ... pre-repository copy logic ... | |
| List<String> copiedRepos = new ArrayList<>(); | |
| try { | |
| versionControl.copyRepository(sourceProjectKey, targetProjectKey, RepositoryType.TEMPLATE, /* attempt */ null); | |
| copiedRepos.add(RepositoryType.TEMPLATE.getName()); | |
| versionControl.copyRepository(sourceProjectKey, targetProjectKey, RepositoryType.SOLUTION, /* attempt */ null); | |
| copiedRepos.add(RepositoryType.SOLUTION.getName()); | |
| versionControl.copyRepository(sourceProjectKey, targetProjectKey, RepositoryType.TESTS, /* attempt */ null); | |
| copiedRepos.add(RepositoryType.TESTS.getName()); | |
| } catch (Exception e) { | |
| rollbackRepositoryCopies(sourceProjectKey, targetProjectKey, copiedRepos); | |
| throw e; | |
| } | |
| // ... post-repository copy logic ... | |
| } |
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| // void testGetExamInCourseElseThrow_testExamEnded() { | ||
| // testExam1.setEndDate(ZonedDateTime.now().minusHours(5)); | ||
| // examRepository.save(testExam1); | ||
| // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(BadRequestAlertException.class); | ||
| // } | ||
| // | ||
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| // void testGetExamInCourseElseThrow_multipleUnfinishedStudentExams() { | ||
| // User user = studentExamForTestExam1.getUser(); | ||
| // examUtilService.addStudentExamForTestExam(testExam1, user); | ||
| // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(IllegalStateException.class); | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out test methods.
These test methods should either be implemented or removed, not left as comments in the codebase.
Apply this diff to remove the commented code:
- // @Test
- // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
- // void testGetExamInCourseElseThrow_testExamEnded() {
- // testExam1.setEndDate(ZonedDateTime.now().minusHours(5));
- // examRepository.save(testExam1);
- // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(BadRequestAlertException.class);
- // }
- //
- // @Test
- // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
- // void testGetExamInCourseElseThrow_multipleUnfinishedStudentExams() {
- // User user = studentExamForTestExam1.getUser();
- // examUtilService.addStudentExamForTestExam(testExam1, user);
- // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(IllegalStateException.class);
- // }📝 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.
| // @Test | |
| // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | |
| // void testGetExamInCourseElseThrow_testExamEnded() { | |
| // testExam1.setEndDate(ZonedDateTime.now().minusHours(5)); | |
| // examRepository.save(testExam1); | |
| // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(BadRequestAlertException.class); | |
| // } | |
| // | |
| // @Test | |
| // @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | |
| // void testGetExamInCourseElseThrow_multipleUnfinishedStudentExams() { | |
| // User user = studentExamForTestExam1.getUser(); | |
| // examUtilService.addStudentExamForTestExam(testExam1, user); | |
| // assertThatThrownBy(() -> examAccessService.getExamInCourseElseThrow(course1.getId(), testExam1.getId())).isInstanceOf(IllegalStateException.class); | |
| // } |
WalkthroughThis pull request refactors and extends exam and participation management. It updates the retrieval logic in exam services to use participation IDs rather than user IDs, introduces a many‐to‐many relationship between student exams and participations, and adds several new repository methods to fetch comprehensive exam data. Changes span backend domain classes, repositories, and services (including exam access, submission, and date validation), along with corresponding modifications in UI components, routing, localization files, and test cases to support the new test exam and participation features. Changes
Sequence Diagram(s)sequenceDiagram
participant Student
participant ExamAccessService
participant ExamDateService
participant StudentExamRepository
Student->>ExamAccessService: Request exam start/retrieval
ExamAccessService->>ExamDateService: Check exam visibility (isExamOver)
ExamDateService-->>ExamAccessService: Return visibility status
ExamAccessService->>StudentExamRepository: Retrieve StudentExam (by examId & participationId)
StudentExamRepository-->>ExamAccessService: Return StudentExam data
ExamAccessService-->>Student: Return the StudentExam for conduction
Possibly related PRs
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (1)
502-512: 🛠️ Refactor suggestionAdd JavaDoc documentation for the
findActiveExamsmethod.The method lacks documentation explaining its purpose, parameters, and return value. This is inconsistent with the documentation style used for other methods in the repository.
Add the following documentation:
+ /** + * Find all non-test exams for which the user is registered and that are currently active + * (visible and not ended). + * + * @param courseIds the IDs of the courses to search in + * @param userId the ID of the user + * @param visible the date before which exams should be visible + * @param end the date after which exams should end + * @return a set of active exams for which the user is registered + */ @Query("""src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
654-702: 🛠️ Refactor suggestionConsider adding transaction boundary for participation setup.
The method saves both participations and student exam, but lacks transaction boundary which could lead to inconsistent state if either operation fails.
Add transaction boundary:
+ @Transactional public void setUpExerciseParticipationsAndSubmissions(StudentExam studentExam, List<StudentParticipation> generatedParticipations) {
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
210-211:⚠️ Potential issueFix potential regular expression injection vulnerability.
The regex pattern is constructed using user-provided input (
projectKey), which could be exploited if it contains regex metacharacters. This is a security issue that could lead to denial of service or other vulnerabilities.Apply this diff to escape regex metacharacters in the pattern:
- String pattern = projectKey.toLowerCase() + "\\d*-"; + String pattern = Pattern.quote(projectKey.toLowerCase()) + "\\d*-";
🧹 Nitpick comments (28)
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
157-157: Consider improving access checks to reduce database queries.The TODO comment indicates a need to optimize access checks to avoid fetching the user multiple times.
Consider caching the user object or consolidating the access checks into a single method that performs all necessary validations with minimal database queries. For example:
+ /** + * Performs all necessary access checks for a participation with optimized user fetching. + * + * @param participation the participation to check + * @param user the pre-fetched user object to avoid multiple database queries + * @throws AccessForbiddenException if the user doesn't have access + */ + private void validateParticipationAccess(ProgrammingExerciseStudentParticipation participation, User user) { + // Perform all access checks using the pre-fetched user + // Add implementation here + }src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java (1)
148-176: Enhance error message specificity.The implementation is clean and well-structured. However, the error message could be more specific by including the
isTestRunparameter value to help with debugging.- throw new EntityNotFoundException("Participation could not be found by exerciseId " + exercise.getId() + " and user " + username); + throw new EntityNotFoundException("Participation could not be found by exerciseId " + exercise.getId() + ", user " + username + ", and testRun " + isTestRun);src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java (2)
502-512: Consider renaming the method to better reflect its purpose.The current name
findActiveExamsis somewhat ambiguous. A more descriptive name would better convey that it finds active exams for which a specific user is registered.Consider renaming to:
- Set<Exam> findActiveExams(@Param("courseIds") Set<Long> courseIds, @Param("userId") long userId, @Param("visible") ZonedDateTime visible, @Param("end") ZonedDateTime end); + Set<Exam> findActiveExamsForRegisteredUser(@Param("courseIds") Set<Long> courseIds, @Param("userId") long userId, @Param("visible") ZonedDateTime visible, @Param("end") ZonedDateTime end);
502-512: Consider adding an index hint for better query performance.The query filters on multiple columns (course.id, visibleDate, endDate, testExam, user.id) and would benefit from appropriate indexing.
Consider adding a composite index on the frequently queried columns. You can use the following script to verify the current indexes:
#!/bin/bash # Description: Check existing indexes on the exam table ast-grep --pattern 'CREATE INDEX' | grep -i 'exam'src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (1)
104-121: Consider adding DISTINCT to prevent duplicate results.The implementation looks good, but the query might return duplicate results due to the collection join with submissions. Consider adding DISTINCT to the query:
- SELECT participation + SELECT DISTINCT participationOtherwise, the implementation is well-structured and follows best practices for fetching related entities.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (2)
104-117: LGTM! Consider enhancing the documentation.The implementation correctly handles both test and regular exams, with appropriate repository method usage. The stream operation efficiently finds the latest unsubmitted test exam.
Consider expanding the comment to explain why we need the latest exam:
- // Since multiple student exams for a test exam might exist, find the latest (=the highest id) unsubmitted student exam + // Since multiple student exams for a test exam might exist, find the latest (=the highest id) unsubmitted student exam + // to ensure we're working with the most recent attempt and prevent conflicts with older attemptsConsider adding null validation for the exam parameter:
private Optional<StudentExam> findStudentExamForUser(User user, Exam exam) { + if (exam == null) { + return Optional.empty(); + }
154-158: Update method documentation to reflect test exam handling.The implementation correctly excludes test exams from multiple submission prevention, but the method documentation needs to be updated.
Update the comment to include test exam exception:
- * We want to prevent multiple submissions for text, modeling, file upload and quiz exercises. Therefore we check if - * a submission for this exercise+student already exists. + * We want to prevent multiple submissions for text, modeling, file upload and quiz exercises in regular exams. + * For test exams, multiple submissions are allowed. For other cases, we check if a submission for this + * exercise+student already exists.src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (1)
14-14: Handle undefined router link case.The binding needs to handle the case where
routerLinkForRepositoryViewreturns undefined.Add a conditional to disable the button when the link is undefined:
-[routerLinkForRepositoryView]="routerLinkForRepositoryView" +[routerLinkForRepositoryView]="routerLinkForRepositoryView" +[disabled]="!routerLinkForRepositoryView"src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts (4)
285-303: Test case description could be more descriptive.The test case title "should load existing testExam if studentExam id is start" doesn't fully capture what's being tested. Consider renaming it to better reflect that it's testing the loading of an existing test exam without loading the summary.
- it('should load existing testExam if studentExam id is start', () => { + it('should load existing test exam without summary when studentExamId is "start"', () => {
1095-1097: Test setup could be more maintainable.The
testExamflag is set directly before callingloadAndDisplaySummary(). Consider moving this setup to a beforeEach block or a helper method to improve test maintainability and readability.+ function setupTestExamFlag(isTestExam: boolean) { + comp.testExam = isTestExam; + } it('should reset Exam Layout if the summary is loaded and displayed', () => { - comp.testExam = false; + setupTestExamFlag(false); comp.loadAndDisplaySummary(); }); it('should not reset Exam Layout in loadAndDisplaySummary if it is a test exam', () => { - comp.testExam = true; + setupTestExamFlag(true); comp.loadAndDisplaySummary(); });Also applies to: 1115-1117
321-326: Test assertions could be more specific.The test assertions for loading student exam could be more specific about the expected behavior. Consider adding assertions to verify the exact reason why
loadStudentExamSpyshould not be called.expect(loadStudentExamSpy).not.toHaveBeenCalled(); + // Verify that we skip loading student exam when studentExamId matches an existing exam + expect(comp.studentExam.id).toBe(studentExamWithExercises.id); + expect(comp.studentExam.exam).toBe(studentExam.exam); expect(loadStudentExamWithExercisesForSummary).toHaveBeenCalledOnce(); expect(comp.studentExam).toEqual(studentExamWithExercises); expect(comp.studentExam).not.toEqual(studentExam);
271-283: Consider adding more test cases for error scenarios.The test suite would benefit from additional test cases covering:
- Error handling when loading a test exam fails
- Edge cases for test exam state transitions (e.g., when a test exam is interrupted)
- Race conditions when multiple test exam sessions are started
Example test case structure:
it('should handle errors when loading test exam fails', () => { const httpError = new HttpErrorResponse({ error: { message: 'Failed to load test exam' }, status: 500 }); TestBed.inject(ActivatedRoute).params = of({ courseId: '1', examId: '2', studentExamId: 'start' }); const loadTestRunStub = jest.spyOn(examParticipationService, 'getOwnStudentExam') .mockReturnValue(throwError(() => httpError)); const alertErrorSpy = jest.spyOn(alertService, 'error'); comp.ngOnInit(); expect(loadTestRunStub).toHaveBeenCalledOnce(); expect(alertErrorSpy).toHaveBeenCalledOnce(); expect(comp.studentExam).toBeUndefined(); });src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java (1)
128-128: Enhance parameter documentation.The documentation for the
attemptparameter should be more descriptive to explain its purpose, valid values, and impact on repository naming.- * @param attempt The attempt number + * @param attempt The attempt number for tracking multiple participations in test exams. + * Must be null or >= 0. When > 0, it will be appended to the target + * project key for non-practice repositories.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java (1)
40-48: Add null safety checks to prevent potential NPEs.The method implementation is clean and follows the single responsibility principle. However, consider adding null safety checks:
+ @NotNull public boolean isExamOver(Exam exam) { + if (exam.getEndDate() == null) { + return false; + } return exam.getEndDate().isBefore(ZonedDateTime.now()); }src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java (6)
142-157: Improve Javadoc consistency.
The documentation is thorough and covers test exam logic, programming, and non-programming exercises. However, consider revalidating the mention of “participant” vs. “student” for clarity and consistency. A few references in the doc block still use "student" in places where "participant" might be more accurate.
175-195: Consider extracting normal vs. test exam logic into separate methods.
The branching for test exam vs. normal exercise modes is handled in the same method, making it a bit large and harder to follow. Extracting each logic branch into a helper method could improve readability and align with single responsibility principles.+ private StudentParticipation handleTestExamExercise(Exercise exercise, Participant participant, StudentParticipation existingParticipation) { + // Implementation that marks old participations finished, etc. + } + + private StudentParticipation handleRegularExercise(Exercise exercise, Participant participant, StudentParticipation existingParticipation) { + // Implementation for normal exercise / exam. + }
210-217: Use plain null check instead of Optional for initialization date.
UsingOptional.ofNullable(...).isEmpty()is a bit less idiomatic than checkingparticipation.getInitializationDate() == null. Simplifying the check might improve readability without changing functionality.- if (Optional.ofNullable(participation.getInitializationDate()).isEmpty()) { + if (participation.getInitializationDate() == null) { participation.setInitializationDate(ZonedDateTime.now()); }
229-230: Complete the Javadoc sentence.
The Javadoc comment “Helper Method to create a new Participation for the” seems truncated. Consider finalizing or rephrasing for clarity.
477-477: Double-check repository copy logic for hundreds of participants.
Cloning or copying a repository for each attempt can be heavy. If the test exam allows many attempts, consider caching or more efficient strategies to avoid potential performance bottlenecks under high load.
672-674: Confirm retrieval of “latest” participation for test exams.
Loading the latest participation for a test exam ensures multiple attempts are possible, but be sure any submission merges or final grading logic that depends on older attempts is correctly accounted for.Do you want me to open an issue to explore storing or merging results from older test exam participations?
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (1)
83-104: Consider separating the handling of normal exams vs. test exams.
While the logic here is correct, the method does both checks (visibility, test/normal) in one flow. For clarity, you could move test-exam-specific checks into a dedicated flow and keep normal exam checks separate.src/main/webapp/app/overview/course-exams/course-exams.component.ts (3)
113-119: Unsubscribe from router events to prevent potential memory leaks.
You’re subscribing to router events inngOnInitbut not unsubscribing inngOnDestroy. Consider storing the subscription in a class property and unsubscribing upon component teardown.- this.router.events.subscribe((event) => { + this.routerEventSubscription = this.router.events.subscribe((event) => { if (event instanceof NavigationEnd) { // ... } }); ... ngOnDestroy(): void { // ... + this.routerEventSubscription?.unsubscribe(); }
259-264: String trimming from session storage is fine.
Removing surrounding quotes from the stored exam ID is a viable technique. If the session storage data can be stored in JSON form, parsing might reduce manual string checks, but this approach is acceptable.
352-367: Handling the working time with a singlewithinWorkingTimeflag.
Currently, only the latest attempt is tracked for an active subscription. If multiple attempts can run in parallel, consider separate boolean flags or a more granular approach. Otherwise, this mechanism is clear for single active attempts.Also applies to: 369-374, 379-384
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts (1)
142-158: Enhance test coverage for end date initialization.The test cases verify the basic functionality, but the end date test could be more robust:
- Test with different durations, not just 1 hour
- Add negative test cases (e.g., when
exam.endDateis undefined)- Verify that the end date is correctly updated when the start date changes
Here's an example of additional test cases:
it('should initialize end date of the test exam correctly', () => { const examEndDate = dayjs('2022-02-06 02:00:00').add(1, 'hours'); exam.testExam = true; component.exam = exam; component.studentExam = studentExam; fixture.detectChanges(); expect(component.endDate).toStrictEqual(examEndDate); }); +it('should handle undefined end date in test exam', () => { + exam.testExam = true; + exam.endDate = undefined; + component.exam = exam; + component.studentExam = studentExam; + fixture.detectChanges(); + expect(component.endDate).toBeUndefined(); +}); + +it('should update end date when start date changes', () => { + const newStartDate = dayjs('2022-02-06 03:00:00'); + const newEndDate = dayjs(newStartDate).add(2, 'hours'); + exam.testExam = true; + exam.startDate = newStartDate; + exam.endDate = newEndDate; + component.exam = exam; + component.studentExam = studentExam; + fixture.detectChanges(); + expect(component.endDate).toStrictEqual(newEndDate); +});src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java (1)
92-92: Initialize list withCollections.emptyList().Consider initializing the list with
Collections.emptyList()for consistency withgetQuizQuestions()method and to prevent potential null pointer issues.- private List<StudentParticipation> studentParticipations = new ArrayList<>(); + private List<StudentParticipation> studentParticipations = Collections.emptyList();src/main/webapp/app/overview/course-overview.service.ts (2)
352-368: Consider adding JSDoc comments for the new parameters.The method signature has been updated to include
numberOfAttempts, but lacks documentation for the new parameter.Add JSDoc comments:
+ /** + * Maps an exam to a sidebar card element. + * @param exam The exam to map + * @param studentExam Optional student exam data + * @param numberOfAttempts Optional number of attempts for test exams + * @returns The mapped sidebar card element + */ mapExamToSidebarCardElement(exam: Exam, studentExam?: StudentExam, numberOfAttempts?: number): SidebarCardElement {
467-475: Consider adding input validation for working time calculation.The method assumes valid input for working time calculation but should handle edge cases.
Add input validation:
calculateUsedWorkingTime(studentExam: StudentExam): number { let usedWorkingTime = 0; + if (!studentExam?.exam) { + return usedWorkingTime; + } if (studentExam.exam!.testExam && studentExam.started && studentExam.submitted && studentExam.workingTime && studentExam.startedDate && studentExam.submissionDate) { const regularExamDuration = studentExam.workingTime; usedWorkingTime = Math.min(regularExamDuration, dayjs(studentExam.submissionDate).diff(dayjs(studentExam.startedDate), 'seconds')); } return usedWorkingTime; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/main/resources/config/liquibase/changelog/20240513101552_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101555_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/changelog/20240513101557_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (44)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java(7 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java(1 hunks)src/main/webapp/app/exam/participate/exam-participation.component.ts(5 hunks)src/main/webapp/app/exam/participate/exam-participation.route.ts(0 hunks)src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts(2 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html(1 hunks)src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts(1 hunks)src/main/webapp/app/overview/course-exams/course-exams.component.ts(9 hunks)src/main/webapp/app/overview/course-overview.service.ts(4 hunks)src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html(1 hunks)src/main/webapp/app/types/sidebar.ts(2 hunks)src/main/webapp/i18n/de/exam.json(2 hunks)src/main/webapp/i18n/de/student-dashboard.json(1 hunks)src/main/webapp/i18n/en/exam.json(2 hunks)src/main/webapp/i18n/en/student-dashboard.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java(1 hunks)src/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts(7 hunks)src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.ts(1 hunks)src/test/javascript/spec/service/exam-participation.service.spec.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/main/webapp/app/exam/participate/exam-participation.route.ts
- src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/create-exercise-unit/create-exercise-unit.component.ts
- src/test/java/de/tum/cit/aet/artemis/exam/service/ExamServiceTest.java
🧰 Additional context used
📓 Path-based instructions (6)
`src/main/java/**/*.java`: naming:CamelCase; principles:{sin...
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/exercise/domain/Exercise.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/VersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.javasrc/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.javasrc/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/vcs/AbstractVersionControlService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.javasrc/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
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/programming/icl/RepositoryUriTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMock...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/exam/participate/exam-start-information/exam-start-information.component.spec.tssrc/test/javascript/spec/service/exam-participation.service.spec.tssrc/test/javascript/spec/component/exam/participate/exam-participation.component.spec.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.htmlsrc/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.tssrc/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.tssrc/main/webapp/app/overview/course-overview.service.tssrc/main/webapp/app/exam/participate/exam-participation.component.tssrc/main/webapp/app/types/sidebar.tssrc/main/webapp/app/overview/course-exams/course-exams.component.ts
`src/main/webapp/i18n/de/**/*.json`: German language transla...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/student-dashboard.jsonsrc/main/webapp/i18n/de/exam.json
🧠 Learnings (5)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
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().
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
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().
src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (3)
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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:975-980
Timestamp: 2024-11-12T12:51:51.200Z
Learning: The `testSubmitStudentExam_notInTime` 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.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (2)
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.
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `testSubmitStudentExam_differentUser` 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.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamSubmissionService.java (2)
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-11-12T12:51:40.391Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:266-267
Timestamp: 2024-11-12T12:51:58.050Z
Learning: When reviewing code in this project, avoid suggesting code changes that are outside the scope of the PR.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (72)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
361-362: LGTM! The change aligns with multiple test exam participation support.The modification to use
findByExamIdAndParticipationIdinstead of user ID is correct and consistent with the PR's objective of supporting multiple test exam participations. This change ensures proper filtering of sensitive feedback based on the specific participation rather than just the user.src/main/java/de/tum/cit/aet/artemis/exam/service/ExamService.java (1)
564-564: LGTM! Good optimization.Setting the course attribute to null helps reduce memory usage by removing redundant course information, as the course details are already available in the exercise group and exam objects. This change aligns with the coding guidelines for minimal DTOs and avoiding duplication.
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamAccessServiceTest.java (2)
352-352: LGTM!The assertion correctly tests that an unregistered user cannot access
exam2incourse2.
420-434: Verify if these tests should be removed or updated for multiple participations.These commented-out tests appear to be obsolete with the new multiple participation feature in test exams. The test
testGetExamInCourseElseThrow_multipleUnfinishedStudentExamsspecifically checks for anIllegalStateExceptionwhen multiple unfinished student exams exist, which contradicts the new feature allowing multiple participations.Please confirm if these tests should be:
- Removed as they no longer align with the new functionality
- Updated to reflect the new behavior with multiple participations
src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java (1)
524-533: LGTM! The exam result visibility logic is correctly updated.The modification to exclude test exam exercises from result hiding aligns with the PR objectives to allow multiple test exam participations. The implementation maintains proper access control for regular exam exercises while enabling visibility for test exams.
src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java (4)
130-130: LGTM! Clear and concise method for retrieving latest participation.The method follows Spring Data JPA naming conventions and correctly orders by ID in descending order to get the latest participation.
143-156: LGTM! Well-structured query for fetching latest legal submissions.The query effectively:
- Uses JOIN FETCH for eager loading of submissions
- Employs a subquery to get the latest participation
- Properly handles illegal submission filtering
782-809: LGTM! Comprehensive methods for test exam participation retrieval.The methods provide clear separation of concerns:
- One method includes assessor information for detailed views
- One method excludes assessor for basic views
- Both methods properly handle eager loading of required associations
1024-1042: LGTM! Clear distinction between exam types in participation retrieval.The method now properly handles:
- Regular exams
- Test exams
- Test runs
Each with appropriate data loading strategies.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java (1)
115-134: LGTM! Well-structured method with clear responsibility.The implementation is clean, follows the single responsibility principle, and handles both cases (with and without submissions) appropriately.
src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java (2)
80-81: LGTM! Well-structured methods for retrieving latest participation.The methods follow Spring Data JPA naming conventions and properly handle the retrieval of the most recent participation, which is essential for managing multiple test exam attempts.
Also applies to: 97-99
181-195: LGTM! Well-implemented test run participation handling.The implementation:
- Uses proper parameter annotations
- Follows SQL best practices with uppercase keywords
- Efficiently fetches related entities using LEFT JOIN FETCH
- Provides a clean default method for Optional handling
src/main/webapp/app/exam/participate/summary/exercises/programming-exam-summary/programming-exam-summary.component.html (1)
6-8: LGTM! Proper usage of @if directives.The template correctly uses the new Angular @if syntax as recommended in the coding guidelines.
Also applies to: 18-20, 30-33, 51-53
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportService.java (1)
114-116: LGTM! Appropriate use of null for attempt parameter.The null values for the attempt parameter are correct here since this service handles importing exercise repositories (templates, solutions, tests) rather than student attempts.
src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java (1)
900-901: LGTM! Mocks properly updated for new parameter.The mock definitions have been correctly updated to handle the new attempt parameter while maintaining existing test behavior.
Also applies to: 916-916
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamDateService.java (1)
113-116:❓ Verification inconclusive
Verify database performance and data consistency.
The implementation correctly supports multiple participations for test exams while maintaining compatibility with real exams. However, let's verify:
- Database performance with the new ordering clause
- Reliability of using ID for temporal ordering
🏁 Script executed:
#!/bin/bash # Description: Check if appropriate indexes exist for the new query pattern # Check if there's an index on exam_id, user_id, and id columns echo "Checking indexes on student_exam table..." ast-grep --pattern 'CREATE INDEX $_ ON student_exam ($$$)' # Check if there are any existing performance-related configurations echo "Checking for existing performance configurations..." rg -i "performance|index" --type javaLength of output: 455
#!/bin/bash # Re-verify index creation in SQL migration files for the student_exam table echo "Searching for index creation definitions in SQL migration files..." rg -i "create index" -e "student_exam" --type sql .#!/bin/bash # Re-verify usage of the repository method in the codebase echo "Searching for repository method definition 'findFirstByExamIdAndUserIdOrderByIdDesc'..." rg -i "findFirstByExamIdAndUserIdOrderByIdDesc" .Verify DB Indexes and Consistent Ordering
The updated logic in
ExamDateService.java(lines 113–116) correctly supports multiple participations in test exams by fetching the latest student exam record using an ID ordering. However, further verification is needed to ensure that:
- Database Performance: An appropriate index exists for the query pattern (based on exam_id, user_id, and id).
- Data Consistency & Ordering: The assumption that ordering by ID descending reliably reflects the temporal order (i.e., the highest ID indeed corresponds to the latest exam participation) holds true.
Please run the additional shell scripts above to check for SQL migration entries defining the needed indexes and to confirm that the repository method is implemented as expected. Manual verification on the actual database schema and id behavior is recommended.
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java (4)
199-202: Validate quiz exercise reinitialization conditions.
In the non-programming branch, the participation is re-initialized when it's in stateFINISHEDfor most exercises, but specifically excluded for quiz exercises. Ensure this exclusion is correct and that no further submissions are needed for quiz retakes.
291-294: Clarify practice mode attempt usage.
Hardcodingparticipation.setAttempt(1);can be confusing if the student enters practice mode multiple times. Confirm whether repeated practice attempts are expected to always reflect "attempt=1," or if you plan to track them differently in the future.
593-595: Ensure team-based test exams are handled.
Currently, the code falls into the “team or user” branching logic. For test exam exercises that are also team-based, the block at line 593 might not be reached due to the earlier team check. Verify that team test exams are either unsupported or handled as intended.
161-172:❓ Verification inconclusive
Recheck attempt indexing for test exams.
When creating a new participation in test exam mode, the code setsparticipation.setAttempt(participations.size()). This potentially assigns attempt = 2 for the third attempt (since the first attempt is zero-based). Verify if this off-by-one is intended or if usingparticipations.size() + 1would be more natural.
🏁 Script executed:
#!/bin/bash # Searching for usage of 'setAttempt' in the codebase to ensure consistent attempt indexing rg -A 3 "setAttempt"Length of output: 20
Attention: Verify Attempt Indexing in Test Exam Participations
It’s unclear whether the zero-based attempt index (via
participation.setAttempt(participations.size())) is intentional. In test exam scenarios, if attempts are expected to be one-based (i.e. the first attempt should be 1), then usingparticipations.size() + 1would be more appropriate. Please manually verify the intended indexing semantics across the codebase.
- Location:
src/main/java/de/tum/cit/aet/artemis/exercise/service/ParticipationService.java(lines 161–172)- Key Point: Confirm whether a new test exam participation should have an attempt value starting at 0 or 1.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamAccessService.java (3)
52-56: Constructor injection looks good.
The introduction ofExamDateServicevia constructor injection follows best practices and aligns with the guidelines for dependency injection.
106-146: Validation checks for normal exams look consistent.
The step-by-step checks for ended exams, user registration, and exam start date are coherent and align with the requirement to avoid generating incomplete or invalid student exams.
148-172: Potential concurrency concern for test exam creation.
When a user rapidly starts multiple test exams, it might create duplicates. Consider synchronizing or adding a concurrency check (e.g., via a transactional boundary) to prevent parallel creation.Would you like me to generate a script to scan for any repeated test exam creation logic within the codebase?
src/main/webapp/app/overview/course-exams/course-exams.component.ts (7)
4-5: New imports appear necessary for subscription and interval usage.
The usage ofNavigationEndfor listening to route changes andintervalfor countdown logic is appropriate here.
23-36: Accordion group extension is consistent.
Adding “attempt” to the group definitions, collapse state, and always-show settings is coherent with the existing approach to real/test groupings.
63-80: Ensure unsubscribing or initializing new dynamic properties properly.
AddingstudentExamStateandwithinWorkingTimehelps track exam status. Confirm they are properly reset or unsubscribed to avoid memory leaks, especially if the component is destroyed and recreated.
239-254: Attempt-based sidebar entries enhance clarity.
This approach neatly displays test exam attempts in a separate accordion group. The logic for labeling attempts in reverse order looks well-structured.
288-295: Filtering only submitted attempts inprepareSidebarData.
Here, unsubmitted attempts are left out oftestExamMap. Verify that this is the intended behavior so that in-progress attempts do not appear in the sidebar.Also applies to: 299-306
321-341: New helper methods increase maintainability.
Collecting student exams across real and test exams, along with index management, is nicely separated into these utility methods, improving code reuse.
342-345: Retrieving the count of test exam attempts is straightforward.
Using the map’s.lengthshould serve well for the UI. Handle null or undefined cases gracefully in any extended logic.src/main/java/de/tum/cit/aet/artemis/exam/repository/StudentExamRepository.java (8)
45-46: Entity graph addition for exercises and studentParticipations is beneficial.
This can reduce N+1 queries when retrieving student exams for advanced analytics, but watch out for potential large data loads.
53-56: ExtendedfindWithExercisesAndSessionsAndStudentParticipationsById.
Fetching exam sessions together with participations is a convenient approach. Ensure it aligns with performance demands for large cohorts.
196-197: Retrieving the last exam attempt with descending ID is a typical pattern.
Looks good for picking the user’s latestStudentExam.
198-205: Efficient association viaparticipationId.
This query is helpful for linking an exam to a specific participation.
207-218: Convenient default method for exam retrieval with fallback.
ThiselseThrowpattern fosters cleaner code and consistent exception handling.
356-360: Augmented retrieval with exercises and studentParticipations.
Providing a single call to fetch both is succinct and aligns with the new many-to-many relationships.
367-370: Fetching full exam details.
Pulling in exercises, sessions, and participations in one method will help in scenarios requiring complete exam data in one go. Use carefully if exam sizes are large.
286-287: Filtering and retrieving test exam attempts by user and course or exam.
These queries align well with the new multi-attempt test-exam logic, ensuring we can fetch all relevant records.Also applies to: 288-296
src/main/webapp/app/exam/participate/exam-start-information/exam-start-information.component.ts (1)
31-31: LGTM!The addition of the
endDateproperty and its initialization inngOnInitis clean and follows the existing patterns in the component.Also applies to: 43-43
src/main/webapp/app/types/sidebar.ts (1)
10-10: LGTM!The changes enhance the sidebar types to support test exam attempts:
- Added 'attempt' to
ExamGroupCategory- Added well-documented properties for tracking test exam attempts
- Fixed typo in the
rightIconcommentAlso applies to: 132-154
src/main/java/de/tum/cit/aet/artemis/exam/domain/StudentExam.java (1)
88-92: LGTM!The changes are well-structured:
- Many-to-many relationship with
StudentParticipationis properly configured- The
isFinished()method has clear logic and documentationAlso applies to: 209-215, 255-257
src/test/java/de/tum/cit/aet/artemis/programming/icl/RepositoryUriTest.java (1)
74-86: LGTM! Well-structured test case.The test method is well-designed with clear assertions validating the key aspects of the
LocalVCRepositoryUriinstantiation for test exam attempts.src/test/java/de/tum/cit/aet/artemis/exam/TestExamIntegrationTest.java (2)
255-262: LGTM! Well-structured test for expired exam scenario.The test correctly verifies that accessing an expired exam returns a BAD_REQUEST status.
264-271: LGTM! Well-structured test for multiple unfinished attempts.The test properly verifies that having multiple unfinished attempts results in an INTERNAL_SERVER_ERROR status.
src/test/javascript/spec/service/exam-participation.service.spec.ts (4)
273-283: LGTM! Well-structured test for successful submission.The test properly verifies both the response and the request URL.
285-301: LGTM! Comprehensive error handling test.The test properly verifies the error message for submissions not made in time.
303-319: LGTM! Well-structured duplicate submission test.The test properly verifies the error message for already submitted exams.
321-335: LGTM! Proper generic error handling test.The test properly verifies the error message for general submission failures.
src/main/java/de/tum/cit/aet/artemis/exercise/domain/participation/Participation.java (1)
290-297: LGTM! Clean getter and setter implementation.The getter and setter methods are properly implemented.
src/main/webapp/app/overview/course-overview.service.ts (2)
300-304: LGTM! Method handles test exam attempts mapping.The method correctly maps test exam attempts to sidebar card elements, with proper null checks.
370-382: LGTM! Method handles individual attempt mapping.The method correctly maps individual test exam attempts to sidebar card elements, with proper translation keys and time calculations.
src/main/java/de/tum/cit/aet/artemis/exercise/domain/Exercise.java (1)
378-381: LGTM! Method provides clear test exam exercise check.The method correctly combines
isExamExercise()check with test exam status check. The@JsonIgnoreannotation is appropriately used to prevent serialization.src/main/java/de/tum/cit/aet/artemis/exam/service/StudentExamService.java (1)
641-646: LGTM! Method signature simplified by removing unused parameter.The method no longer requires the
startedDateparameter, which aligns with the test exam participation changes.src/main/java/de/tum/cit/aet/artemis/exam/web/StudentExamResource.java (4)
181-181: LGTM! Method name change improves clarity.The method name change from
findByIdWithExercisesSubmissionPolicyAndSessionsElseThrowtofindByIdWithExercisesAndSessionsAndStudentParticipationsElseThrowbetter reflects its functionality by explicitly mentioning student participations.
465-465: LGTM! Method name change improves readability.The method name change from
findStudentExamForTestExamsByUserIdAndCourseIdtofindStudentExamsForTestExamsByUserIdAndCourseIdbetter reflects that it returns multiple student exams.
564-564: LGTM! Method name change improves error handling.The method name change from
findByExamIdAndUserIdtofindOneByExamIdAndUserIdElseThrowbetter indicates that it throws an exception when no result is found.
749-750: LGTM! New code for test exam setup.The added code for setting up test exam participations and submissions aligns with the PR objective of allowing multiple test exam attempts.
src/main/webapp/app/exam/participate/exam-participation.component.ts (4)
63-63: LGTM! Added ExamExerciseOverviewPageComponent.The component is correctly imported and added to the imports array.
Also applies to: 89-89
224-228: LGTM! Improved studentExamId handling.The code now correctly retrieves studentExamId from the route's first child, improving the routing logic for test exams.
242-250: LGTM! Added test exam summary loading.The new code block correctly handles loading the test exam summary when both
testExamis true andstudentExamIdis available.
454-454: LGTM! Updated test exam navigation.The navigation path has been updated to correctly handle test exam routing after exam completion.
src/main/webapp/app/shared/sidebar/sidebar-card-item/sidebar-card-item.component.html (4)
14-14: LGTM! Added visual distinction for attempts.The ngClass directive correctly applies bold font weight to distinguish attempt items.
19-22: LGTM! Dynamic translation for title.The translation key is now dynamically selected based on whether the item is an attempt, improving user experience.
24-34: LGTM! Conditional date display.The code correctly handles displaying either submission date or start date based on whether the item is an attempt.
38-78: LGTM! Comprehensive attempt information display.The new structure effectively organizes and displays attempt-related information, including used working time and attempts count.
src/main/webapp/i18n/en/student-dashboard.json (2)
60-62: LGTM! Added attempt-related translations.The new translation keys properly support the display of test exam attempts in the UI.
66-67: LGTM! Added time-related translations.The new translation keys for submission date and used working time enhance the user experience by providing clear temporal information.
src/main/webapp/i18n/de/student-dashboard.json (1)
60-67: New Localization Entries for Test Exams Added
The newly introduced keys ("attempt","testExamAttempt","attempts","submissionDate", and"usedWorkingTime") clearly communicate the new test exam features. The translations are consistent with the existing language (informal "du/dein") and align well with their English counterparts.src/main/webapp/i18n/en/exam.json (2)
59-62: Added Resume Functionality for Test Exams
The addition of the"resume": "Resume Attempt"key along with the updated"resumeAttempt"entry provides clear guidance for users to resume their test exam attempts. This enhancement directly supports the updated exam participation functionality.
303-303: Updated Message for No Further Attempts
The modified"noFurtherAttempts"string now correctly uses the plural form ("review your submissions") to indicate that multiple submissions can be reviewed. This subtle change improves clarity and aligns with the overall update in functionality.src/main/webapp/i18n/de/exam.json (2)
60-61: Einführung der "resume" Funktion in Testklausuren
Der neue Schlüssel"resume": "Versuch fortführen"ermöglicht es den Nutzern, ihren Testklausurversuch fortzusetzen. Die Formulierung ist klar, informell und passt gut zur erweiterten Funktionalität.
303-303: Angepasste "noFurtherAttempts" Nachricht
Die Anpassung der Nachricht unter"noFurtherAttempts"(nun: "Derzeit sind keine weiteren Versuche für die Testklausur möglich. Um deine Abgaben zu betrachten, navigiere zur Klausurseite") spiegelt korrekt wider, dass mehrere Abgaben vorhanden sein können. Diese Änderung verbessert die Benutzerkommunikation und entspricht den englischen Änderungen.
| get routerLinkForRepositoryView(): (string | number)[] { | ||
| if (this.isInCourseManagement) { | ||
| return ['..', 'programming-exercises', this.exercise.id!, 'repository', 'USER', this.participation.id!]; | ||
| } | ||
| if (this.routerLink.includes('test-exam')) { | ||
| const parts = this.routerLink.split('/'); | ||
| const examLink = parts.slice(0, parts.length - 2).join('/'); | ||
| return [examLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | ||
| } | ||
| return [this.routerLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety checks and type improvements.
The new getter method has potential issues:
- Non-null assertions on IDs could lead to runtime errors
- String manipulation of router path is fragile
Consider these improvements:
- get routerLinkForRepositoryView(): (string | number)[] {
+ get routerLinkForRepositoryView(): (string | number)[] | undefined {
+ if (!this.exercise?.id || !this.participation?.id) {
+ return undefined;
+ }
+
if (this.isInCourseManagement) {
- return ['..', 'programming-exercises', this.exercise.id!, 'repository', 'USER', this.participation.id!];
+ return ['..', 'programming-exercises', this.exercise.id, 'repository', 'USER', this.participation.id];
}
if (this.routerLink.includes('test-exam')) {
const parts = this.routerLink.split('/');
const examLink = parts.slice(0, parts.length - 2).join('/');
- return [examLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!];
+ return [examLink, 'exercises', this.exercise.id, 'repository', this.participation.id];
}
- return [this.routerLink, 'exercises', this.exercise.id!, 'repository', this.participation.id!];
+ return [this.routerLink, 'exercises', this.exercise.id, 'repository', this.participation.id];
}Also consider extracting the path segments into constants to make the code more maintainable:
private readonly REPOSITORY_PATH_SEGMENTS = {
EXERCISES: 'exercises',
REPOSITORY: 'repository',
USER: 'USER',
} as const;| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | ||
| targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for attempt parameter and improve string handling.
The implementation should validate the attempt parameter and use a more robust string concatenation method.
+ if (attempt != null && attempt < 0) {
+ throw new IllegalArgumentException("Attempt number must be >= 0");
+ }
if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) {
- targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt;
+ targetProjectKeyLowerCase = String.format("%s%d", targetProjectKeyLowerCase, attempt);
}📝 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.
| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | |
| targetProjectKeyLowerCase = targetProjectKeyLowerCase + attempt; | |
| } | |
| if (attempt != null && attempt < 0) { | |
| throw new IllegalArgumentException("Attempt number must be >= 0"); | |
| } | |
| if (attempt != null && attempt > 0 && !targetRepositoryName.contains("practice-")) { | |
| targetProjectKeyLowerCase = String.format("%s%d", targetProjectKeyLowerCase, attempt); | |
| } |
| /** | ||
| * Graded course exercises, practice mode course exercises and real exam exercises always have only one parcitipation per exercise | ||
| * In case of a test exam, there are multiple participations possible for one exercise | ||
| * This field is necessary to preserve the constraint of one partipation per exercise, while allowing multiple particpiations per exercise for test exams | ||
| * The value is 0 for graded course exercises and exercises in the real exams | ||
| * The value is 1 for practice mode course exercises | ||
| * The value is 0-255 for test exam exercises. For each subsequent participation the number is increased by one | ||
| */ | ||
| @Column(name = "attempt") | ||
| private int attempt = 0; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for the attempt field.
While the documentation clearly specifies the valid ranges for different exercise types, there's no validation to ensure these constraints are enforced.
Add validation to ensure the value stays within the documented ranges:
+import jakarta.validation.constraints.Max;
+import jakarta.validation.constraints.Min;
@Column(name = "attempt")
+@Min(value = 0, message = "Attempt must be at least 0")
+@Max(value = 255, message = "Attempt must not exceed 255")
private int attempt = 0;📝 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.
| /** | |
| * Graded course exercises, practice mode course exercises and real exam exercises always have only one parcitipation per exercise | |
| * In case of a test exam, there are multiple participations possible for one exercise | |
| * This field is necessary to preserve the constraint of one partipation per exercise, while allowing multiple particpiations per exercise for test exams | |
| * The value is 0 for graded course exercises and exercises in the real exams | |
| * The value is 1 for practice mode course exercises | |
| * The value is 0-255 for test exam exercises. For each subsequent participation the number is increased by one | |
| */ | |
| @Column(name = "attempt") | |
| private int attempt = 0; | |
| import jakarta.validation.constraints.Max; | |
| import jakarta.validation.constraints.Min; | |
| /** | |
| * Graded course exercises, practice mode course exercises and real exam exercises always have only one parcitipation per exercise | |
| * In case of a test exam, there are multiple participations possible for one exercise | |
| * This field is necessary to preserve the constraint of one partipation per exercise, while allowing multiple particpiations per exercise for test exams | |
| * The value is 0 for graded course exercises and exercises in the real exams | |
| * The value is 1 for practice mode course exercises | |
| * The value is 0-255 for test exam exercises. For each subsequent participation the number is increased by one | |
| */ | |
| @Column(name = "attempt") | |
| @Min(value = 0, message = "Attempt must be at least 0") | |
| @Max(value = 255, message = "Attempt must not exceed 255") | |
| private int attempt = 0; |
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.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-test2.artemis.cit.tum.de
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
Currently, students are limited to a single attempt per test exam. This pull request introduces a feature that allows students to take the test exam multiple times.
Description
StudentExam now maintains a list of participations for exercises linked to a particular student exam, enabling students to attempt the test exam multiple times. A student can start a new test exam only if the previous one has been submitted or if its working time has expired. The database index in the participation table has been updated to account for the number of attempts. Normal and practice mode exercises have one participation each, while test exam exercises can have up to 256 participations.
Steps for Testing
Prerequisites:
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Summary by CodeRabbit
New Features
Enhancements