Development: iPad modern attendance checker endpoints
#11419
Conversation
…gatable to without explicit URL
… transient fields to ExamUser
…e exam_user table
…es/ways to reference one
… needs to be an AbstractAuditingEntity, still need to figure out what to do with capacity and if I even need it but might store capacity with the layouts directly
…l need to return a DTO, still need helper functions
… code cleaned up, returns DTO now
…exam_user, various code cleanups
…unctionality for deletion of the DB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java (4)
54-57: Remove @NotNull on primitive booleans (no effect).Bean Validation can’t null‑check primitives; drop @NotNull or switch to wrappers if you truly need nullability constraints.
- @NotNull boolean didCheckImage, - @NotNull boolean didCheckName, - @NotNull boolean didCheckRegistrationNumber, - @NotNull boolean didCheckLogin, + boolean didCheckImage, + boolean didCheckName, + boolean didCheckRegistrationNumber, + boolean didCheckLogin,
97-97: Same here: primitive boolean doesn’t need @NotNull.- @NotNull boolean isTestExam, + boolean isTestExam,
91-116: Use Lists with explicit sorting for stable JSON (avoid Set iteration randomness).Sets serialize in unspecified order and deduplicate by full-record equality. For API determinism, prefer sorted Lists.
-@JsonInclude(JsonInclude.Include.NON_EMPTY) -public record AttendanceCheckerAppExamInformationDTO( +@JsonInclude(JsonInclude.Include.NON_EMPTY) +public record AttendanceCheckerAppExamInformationDTO( @@ - @NotNull Set<ExamRoomForAttendanceCheckerDTO> examRoomsUsedInExam, // might be empty - @NotEmpty Set<ExamUserWithExamRoomAndSeatDTO> examUsersWithExamRoomAndSeat + @NotNull List<ExamRoomForAttendanceCheckerDTO> examRoomsUsedInExam, // might be empty + @NotEmpty List<ExamUserWithExamRoomAndSeatDTO> examUsersWithExamRoomAndSeat ) { public static AttendanceCheckerAppExamInformationDTO from(Exam exam, Set<ExamRoom> examRooms) { return new AttendanceCheckerAppExamInformationDTO( @@ - examRooms.stream().map(ExamRoomForAttendanceCheckerDTO::from).collect(Collectors.toSet()), - exam.getExamUsers().stream().map(ExamUserWithExamRoomAndSeatDTO::from).collect(Collectors.toSet()) + examRooms.stream() + .map(ExamRoomForAttendanceCheckerDTO::from) + .sorted(java.util.Comparator.comparing(ExamRoomForAttendanceCheckerDTO::roomNumber, String.CASE_INSENSITIVE_ORDER)) + .collect(Collectors.toList()), + exam.getExamUsers().stream() + .map(ExamUserWithExamRoomAndSeatDTO::from) + .sorted(java.util.Comparator.comparing(ExamUserWithExamRoomAndSeatDTO::login, String.CASE_INSENSITIVE_ORDER)) + .collect(Collectors.toList()) ); } }As per coding guidelines (dtos:min_data, predictable API payloads).
52-54: PII minimization: drop unused email/imageUrl fields – no references found in the attendance-checker UI; remove these PII fields if they aren’t strictly needed.src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java (1)
64-74: Leverage Bean Validation on the request body.Annotate the body with @notempty to fail fast before controller logic.
-import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestBody; +import jakarta.validation.constraints.NotEmpty; @@ - public ResponseEntity<Void> distributeRegisteredStudents(@PathVariable long courseId, @PathVariable long examId, @RequestBody Set<Long> examRoomIds) { + public ResponseEntity<Void> distributeRegisteredStudents(@PathVariable long courseId, @PathVariable long examId, @RequestBody @NotEmpty Set<Long> examRoomIds) {src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (3)
94-101: Deterministic room iteration and seat ordering.HashMap + Set iteration yields non‑deterministic distribution. Use a key‑sorted map and (if needed) sort seats.
-import java.util.HashMap; +import java.util.HashMap; +import java.util.Comparator; +import java.util.TreeMap; @@ - Map<String, List<ExamSeatDTO>> roomNumberToUsableSeatsDefaultLayout = new HashMap<>(); + Map<String, List<ExamSeatDTO>> roomNumberToUsableSeatsDefaultLayout = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); for (ExamRoom examRoom : examRoomsForExam) { - roomNumberToUsableSeatsDefaultLayout.put(examRoom.getRoomNumber(), examRoomService.getDefaultUsableSeats(examRoom)); + var usable = examRoomService.getDefaultUsableSeats(examRoom); + roomNumberToUsableSeatsDefaultLayout.put( + examRoom.getRoomNumber(), + usable + ); }Note: ExamUserService matches plannedRoom by roomNumber, so using roomNumber here is correct. Based on learnings.
106-124: Distribution is not random; either rename or sort users for reproducibility.Method name says “Randomly”, but current logic is iteration order. For stability, sort by login before assigning (or actually shuffle with a seeded Random).
- private void setPlannedRoomAndPlannedSeatForExamUsersRandomly(Exam exam, Map<String, List<ExamSeatDTO>> roomNumberToUsableSeats) { - Iterator<ExamUser> examUsersIterator = exam.getExamUsers().iterator(); + private void setPlannedRoomAndPlannedSeatForExamUsersRandomly(Exam exam, Map<String, List<ExamSeatDTO>> roomNumberToUsableSeats) { + Iterator<ExamUser> examUsersIterator = exam.getExamUsers().stream() + .sorted(Comparator.comparing(eu -> eu.getUser().getLogin(), String.CASE_INSENSITIVE_ORDER)) + .iterator();Alternatively, rename the method to setPlannedRoomAndSeatInIterationOrder for clarity.
102-104: Saving Exam entity may be redundant.Only ExamUser entities are changed here; consider dropping examRepository.save(exam) to avoid an extra write.
examUserRepository.saveAll(exam.getExamUsers()); - examRepository.save(exam);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
🧠 Learnings (7)
📓 Common learnings
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:72-79
Timestamp: 2025-09-15T11:21:15.983Z
Learning: SamuelRoettgermann prefers iterative development approach - initial implementations should be working starting points rather than full-fledged, finished implementations. Optimization and robustness improvements can be added later.
📚 Learning: 2025-09-25T20:43:41.712Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
📚 Learning: 2025-09-15T11:18:26.439Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
📚 Learning: 2025-08-30T20:20:17.236Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java:0-0
Timestamp: 2025-08-30T20:20:17.236Z
Learning: In ExamRoomIntegrationTest.validateAdminOverview(), the first assertion should use subset containment (contains) rather than exact equality because the admin overview shows all newest unique exam rooms in the system including those from previous uploads, not just rooms from the current upload being tested. The test only needs to verify that the expected rooms from the current upload are present in the response.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
📚 Learning: 2025-08-14T21:24:50.201Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java:150-152
Timestamp: 2025-08-14T21:24:50.201Z
Learning: In the ExamRoomService.parseAndStoreExamRoomDataFromZipFile method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java, IllegalArgumentException cannot be thrown in the try block, so only IOException needs to be caught when parsing ZIP files containing exam room JSON data.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
📚 Learning: 2025-09-22T08:53:47.961Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java:412-422
Timestamp: 2025-09-22T08:53:47.961Z
Learning: SamuelRoettgermann prefers trusting input data quality over defensive programming for floating-point coordinate grouping in ExamRoomService, reasoning that data from the same source/script should be consistent without precision errors.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (3)
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java (1)
Conditional(31-99)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java (1)
Conditional(47-242)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java (1)
Conditional(45-532)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: submit-gradle
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: bean-instantiation-check
- GitHub Check: Check if translation keys are consistent
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java (1)
22-31: Seats list semantics: all seats vs usable seats vs assigned seats.Attendance view may only need default-usable seats or highlight assigned seats. Confirm FE expectation; adjust seats to match (e.g., default usable seats via ExamRoomService).
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java (1)
62-79: Validation scope is right (no “belongs-to-exam” pre-check).Given rooms are assigned during distribution, the current validation (existence + newest versions) is correct; do not re‑introduce a belongs‑to‑exam check here.
Based on learnings.
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (1)
69-92: Atomicity of multi‑step writes: consider a transaction or compensating logic.Delete→insert assignments, then mutate users. A mid‑flow exception can leave assignments updated but users unassigned. If @transactional is intentionally avoided, add compensating cleanup or reorder to minimize partial states.
Would you accept wrapping this method with @transactional, or prefer try/catch with rollback of new assignments on failure?
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
…in the Attendance Checker DTO
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java (2)
210-221: Normalize input strings before matchingTrim room/seat to avoid mismatches due to stray whitespace (while keeping case-insensitive compare).
Apply:
- final String roomNumber = roomGetter.apply(examUser); - final String seatName = seatGetter.apply(examUser); + final String roomNumber = Optional.ofNullable(roomGetter.apply(examUser)).map(String::strip).orElse(null); + final String seatName = Optional.ofNullable(seatGetter.apply(examUser)).map(String::strip).orElse(null);
207-235: Reduce per-user O(R) scans by indexing rooms (and optionally seats)Pre-index rooms by normalized roomNumber to avoid repeated stream scans; optional: per-room seat map by lowercased name.
Apply:
- Set<ExamRoom> examRoomsUsedInExam = examRoomRepository.findAllByExamId(examId); + Set<ExamRoom> examRoomsUsedInExam = examRoomRepository.findAllByExamId(examId); + Map<String, ExamRoom> roomsByNumber = examRoomsUsedInExam.stream() + .collect(java.util.stream.Collectors.toMap(r -> r.getRoomNumber().toLowerCase(java.util.Locale.ROOT), + java.util.function.Function.identity(), (a, b) -> a)); @@ - Optional<ExamRoom> matchingRoom = examRoomsUsedInExam.stream().filter(room -> room.getRoomNumber().equalsIgnoreCase(roomNumber)).findFirst(); + Optional<ExamRoom> matchingRoom = Optional.ofNullable(roomsByNumber.get(roomNumber.toLowerCase(java.util.Locale.ROOT)));Optionally (within the block) pre-index seats:
var seatsByName = matchingRoom.get().getSeats().stream() .collect(java.util.stream.Collectors.toMap(s -> s.name().toLowerCase(Locale.ROOT), java.util.function.Function.identity(), (a,b)->a)); var matchingSeat = Optional.ofNullable(seatsByName.get(seatName.toLowerCase(Locale.ROOT)));src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (5)
73-79: Seat capacity: compute from actual usable seatsUse
getDefaultUsableSeats(examRoom).size()to align the check with the exact seats you later assign (avoids drift if capacity metadata and computed seats diverge).Apply:
- final int numberOfUsableSeats = examRoomsForExam.stream().mapToInt(examRoom -> examRoomService.getDefaultLayoutStrategyOrElseThrow(examRoom).getCapacity()).sum(); + final int numberOfUsableSeats = examRoomsForExam.stream().mapToInt(examRoom -> examRoomService.getDefaultUsableSeats(examRoom).size()).sum();
81-92: Replace-all assignments: consider failure windowDelete-then-insert without a transaction can briefly leave an exam with zero assignments if a failure occurs in between. If transactions are intentionally avoided, consider a repository-level “replaceAssignments(examId, roomIds)” or retry to reduce inconsistency windows.
95-101: Deterministic iteration for reproducible distributions
HashMapiteration order is unspecified; results vary across runs/VMs. Prefer a deterministic key order (e.g.,TreeMapcase-insensitive or build aLinkedHashMapfrom sorted keys) so assignments are reproducible and testable.Apply:
- Map<String, List<ExamSeatDTO>> roomNumberToUsableSeatsDefaultLayout = new HashMap<>(); + Map<String, List<ExamSeatDTO>> roomNumberToUsableSeatsDefaultLayout = new java.util.TreeMap<>(String.CASE_INSENSITIVE_ORDER);
106-124: Method name vs behavior: not actually random; make order explicitCurrent assignment is sequential over (non-deterministically ordered) rooms and ascending seats, plus unspecified user set order. Either:
- make it deterministic (sort users by login/id and seats by name/position), or
- actually randomize with a seeded
Random.Also consider renaming to
setPlannedRoomAndSeatSequentiallyif you keep sequential behavior.Apply (deterministic example):
- private void setPlannedRoomAndPlannedSeatForExamUsersRandomly(Exam exam, Map<String, List<ExamSeatDTO>> roomNumberToUsableSeats) { - Iterator<ExamUser> examUsersIterator = exam.getExamUsers().iterator(); + private void setPlannedRoomAndPlannedSeatForExamUsersRandomly(Exam exam, Map<String, List<ExamSeatDTO>> roomNumberToUsableSeats) { + var examUsers = new ArrayList<>(exam.getExamUsers()); + examUsers.sort(java.util.Comparator.comparing(eu -> eu.getUser().getLogin(), String.CASE_INSENSITIVE_ORDER)); + Iterator<ExamUser> examUsersIterator = examUsers.iterator();
118-122: Redundantexam.addExamUser(nextExamUser)inside iterationYou iterate over
exam.getExamUsers(); adding the same user back is unnecessary and risks confusion. Remove the call.Apply:
- exam.addExamUser(nextExamUser);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java(4 hunks)src/main/webapp/i18n/de/error.json(1 hunks)src/main/webapp/i18n/en/error.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/i18n/de/error.json
- src/main/webapp/i18n/en/error.json
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
🧠 Learnings (19)
📓 Common learnings
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:72-79
Timestamp: 2025-09-15T11:21:15.983Z
Learning: SamuelRoettgermann prefers iterative development approach - initial implementations should be working starting points rather than full-fledged, finished implementations. Optimization and robustness improvements can be added later.
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
📚 Learning: 2025-09-25T20:43:41.712Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
📚 Learning: 2025-09-15T11:18:26.439Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: florian-glombik
PR: ls1intum/Artemis#8597
File: src/main/webapp/app/exercises/programming/manage/repositories-checkout-directories-dto.ts:8-8
Timestamp: 2024-06-10T19:44:09.116Z
Learning: DTOs are typically defined in the `src/main/webapp/app/entities` folder on the client side in the Artemis project.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java
📚 Learning: 2025-09-10T16:35:23.211Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/room/ExamRoom.java:69-75
Timestamp: 2025-09-10T16:35:23.211Z
Learning: Artemis convention: The term “DTO” is used broadly (includes DAOs/value objects), not strictly client-transport models. Avoid assuming dto.* packages are always API-facing.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java
📚 Learning: 2025-08-30T20:21:10.048Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/main/java/de/tum/cit/aet/artemis/exam/dto/room/ExamRoomAdminOverviewDTO.java:12-14
Timestamp: 2025-08-30T20:21:10.048Z
Learning: SamuelRoettgermann prefers guidance on when primitive types are appropriate in DTOs and was not initially aware that primitives are preferred over wrapper types for required, non-nullable fields like counters in the Artemis codebase.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java
📚 Learning: 2025-09-25T20:48:13.391Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/test/java/de/tum/cit/aet/artemis/exam/test_repository/ExamRoomTestRepository.java:18-34
Timestamp: 2025-09-25T20:48:13.391Z
Learning: In ExamRoomTestRepository JPQL queries in src/test/java/de/tum/cit/aet/artemis/exam/test_repository/ExamRoomTestRepository.java, CTE (Common Table Expression) syntax with bare attribute names like `roomNumber`, `name`, `createdDate` inside the WITH clause works correctly without requiring entity aliases, confirmed working by SamuelRoettgermann in their Artemis/Hibernate setup.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java
📚 Learning: 2025-08-30T20:20:17.236Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java:0-0
Timestamp: 2025-08-30T20:20:17.236Z
Learning: In ExamRoomIntegrationTest.validateAdminOverview(), the first assertion should use subset containment (contains) rather than exact equality because the admin overview shows all newest unique exam rooms in the system including those from previous uploads, not just rooms from the current upload being tested. The test only needs to verify that the expected rooms from the current upload are present in the response.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2025-08-14T21:24:50.201Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java:150-152
Timestamp: 2025-08-14T21:24:50.201Z
Learning: In the ExamRoomService.parseAndStoreExamRoomDataFromZipFile method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java, IllegalArgumentException cannot be thrown in the try block, so only IOException needs to be caught when parsing ZIP files containing exam room JSON data.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.javasrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
PR: ls1intum/Artemis#7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.javasrc/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: Strohgelaender
PR: ls1intum/Artemis#8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2025-09-22T09:02:57.726Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java:315-321
Timestamp: 2025-09-22T09:02:57.726Z
Learning: The createdDate field in ExamRoom entities is non-null as it's required by the database schema, so null-safe comparators are not needed when comparing ExamRoom creation dates.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-06-10T19:44:09.116Z
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-06-10T19:44:09.116Z
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`.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-10-20T18:37:45.365Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-10-20T18:37:45.365Z
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.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-10-08T15:35:48.768Z
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/test/javascript/spec/component/shared/exercise-filter/exercise-filter-modal.component.spec.ts:201-225
Timestamp: 2024-10-08T15:35:48.768Z
Learning: Non-null assertions should not be flagged or suggested for removal in the context of PR ls1intum/Artemis#8858.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2025-09-20T16:12:57.735Z
Learnt from: MoritzSpengler
PR: ls1intum/Artemis#11382
File: src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizQuestionProgressService.java:0-0
Timestamp: 2025-09-20T16:12:57.735Z
Learning: In QuizQuestionProgressService.areQuestionsDue, MoritzSpengler confirmed that defensive null/empty guards are unnecessary because the method is only called from getQuestionsForSession, which already handles null/empty questionIds by adding a sentinel value (-1L) before the call.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-07-09T19:10:33.905Z
Learnt from: florian-glombik
PR: ls1intum/Artemis#8858
File: src/main/webapp/app/shared/exercise-filter/exercise-filter-modal.component.ts:97-107
Timestamp: 2024-07-09T19:10:33.905Z
Learning: In the context of PR ls1intum/Artemis#8858, avoid suggesting the removal of non-null assertions.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
📚 Learning: 2024-10-27T12:51:29.409Z
Learnt from: magaupp
PR: ls1intum/Artemis#9609
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/scaparser/strategy/sarif/SarifParser.java:55-55
Timestamp: 2024-10-27T12:51:29.409Z
Learning: The project targets Java 21.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java
🧬 Code graph analysis (1)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (3)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java (1)
Conditional(49-258)src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java (1)
Conditional(31-99)src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java (1)
Conditional(45-532)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java (1)
36-136: Transient attendance fields wired correctlyThe transient room/seat holders are scoped with
@JsonIgnore+@Transient, and the paired setters keep the two values in sync. The established DTO exception for this entity still applies, so the layering stays acceptable. Nice work. Based on learningssrc/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java (1)
202-206: Empty input: prefer no-op over throwing (keeps callers simple)When
examUsersis empty, this throws with “foundExams: 0”. Consider early-return to make the method idempotent for empty cohorts (useful for attendance info on exams with zero users).Apply:
- List<Exam> usedExams = examUsers.stream().map(ExamUser::getExam).distinct().toList(); + if (examUsers.isEmpty()) { + return; + } + List<Exam> usedExams = examUsers.stream().map(ExamUser::getExam).distinct().toList();src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java (1)
132-141: Attendance info: tolerate exams with zero usersIf
exam.getExamUsers()is empty, the downstream transient mapping currently throws on “foundExams: 0”. Either guard here or adopt the no-op change suggested in ExamUserService to keep this endpoint working for empty exams (useful for setup/preview).
| static ExamUserLocationDTO actualFrom(ExamUser examUser) { | ||
| final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null; | ||
|
|
||
| return new ExamUserLocationDTO( | ||
| isLegacy ? null : examUser.getActualRoomTransient().getId(), | ||
| isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getName(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getBuilding(), | ||
| isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name() | ||
| ); |
There was a problem hiding this comment.
Fix actual location legacy detection
actualFrom decides “legacy” based on the planned transient fields, but still dereferences getActualRoomTransient()/getActualSeatTransient() unconditionally. In the typical case before any attendance is recorded those actual transients are null, so this crashes with an NPE (and even if it didn’t, it would violate the @NotBlank constraints by emitting empty strings). Please guard using the actual transients and return null when no actual placement exists yet.
Apply this diff:
- static ExamUserLocationDTO actualFrom(ExamUser examUser) {
- final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null;
-
- return new ExamUserLocationDTO(
- isLegacy ? null : examUser.getActualRoomTransient().getId(),
- isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(),
- isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(),
- isLegacy ? null : examUser.getActualRoomTransient().getName(),
- isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(),
- isLegacy ? null : examUser.getActualRoomTransient().getBuilding(),
- isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name()
- );
- }
+ static @Nullable ExamUserLocationDTO actualFrom(ExamUser examUser) {
+ var actualRoomTransient = examUser.getActualRoomTransient();
+ var actualSeatTransient = examUser.getActualSeatTransient();
+ final boolean usesLegacyFields = actualRoomTransient == null || actualSeatTransient == null;
+
+ if (usesLegacyFields) {
+ if (examUser.getActualRoom() == null || examUser.getActualSeat() == null) {
+ return null;
+ }
+ return new ExamUserLocationDTO(
+ null,
+ examUser.getActualRoom(),
+ null,
+ null,
+ null,
+ null,
+ examUser.getActualSeat()
+ );
+ }
+
+ return new ExamUserLocationDTO(
+ actualRoomTransient.getId(),
+ actualRoomTransient.getRoomNumber(),
+ actualRoomTransient.getAlternativeRoomNumber(),
+ actualRoomTransient.getName(),
+ actualRoomTransient.getAlternativeName(),
+ actualRoomTransient.getBuilding(),
+ actualSeatTransient.name()
+ );
+ }📝 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.
| static ExamUserLocationDTO actualFrom(ExamUser examUser) { | |
| final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null; | |
| return new ExamUserLocationDTO( | |
| isLegacy ? null : examUser.getActualRoomTransient().getId(), | |
| isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getName(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getBuilding(), | |
| isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name() | |
| ); | |
| // Guard against missing actual placement and return null if none exists | |
| static @Nullable ExamUserLocationDTO actualFrom(ExamUser examUser) { | |
| var actualRoomTransient = examUser.getActualRoomTransient(); | |
| var actualSeatTransient = examUser.getActualSeatTransient(); | |
| final boolean usesLegacyFields = actualRoomTransient == null || actualSeatTransient == null; | |
| if (usesLegacyFields) { | |
| // No actual placement recorded yet | |
| if (examUser.getActualRoom() == null || examUser.getActualSeat() == null) { | |
| return null; | |
| } | |
| return new ExamUserLocationDTO( | |
| /* id */ null, | |
| /* roomNumber */ examUser.getActualRoom(), | |
| /* altRoomNumber */ null, | |
| /* name */ null, | |
| /* altName */ null, | |
| /* building */ null, | |
| /* seat */ examUser.getActualSeat() | |
| ); | |
| } | |
| // Fully initialized actual transients | |
| return new ExamUserLocationDTO( | |
| actualRoomTransient.getId(), | |
| actualRoomTransient.getRoomNumber(), | |
| actualRoomTransient.getAlternativeRoomNumber(), | |
| actualRoomTransient.getName(), | |
| actualRoomTransient.getAlternativeName(), | |
| actualRoomTransient.getBuilding(), | |
| actualSeatTransient.name() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
around lines 69-80, the method actualFrom currently decides "legacy" by
inspecting plannedRoomTransient/plannedSeatTransient but unconditionally
dereferences getActualRoomTransient()/getActualSeatTransient(), causing NPEs
when no actual placement exists; update the logic to first check whether
actualRoomTransient or actualSeatTransient is null and if both are null return
null (no actual placement yet), otherwise build the ExamUserLocationDTO using
guarded accesses to actualRoomTransient/actualSeatTransient (use
actualRoom/actualSeat for legacy cases only when actual transients are absent),
and avoid emitting empty strings by returning null for fields that truly do not
exist.
| * @throws BadRequestAlertException If ignoreExamUsersWithoutRoomAndSeat is false and the conditions are met as described, | ||
| * or if th room or seat cannot be mapped to actual entities. | ||
| */ |
There was a problem hiding this comment.
Javadoc: fix typo and clarify error clause
“or if th room…” → “or if the room…”. Also clarify “cannot be mapped to actual entities (room/seat)”.
🤖 Prompt for AI Agents
In src/main/java/de/tum/cit/aet/artemis/exam/service/ExamUserService.java around
lines 197 to 199, fix the Javadoc typo and clarify the error clause: change “or
if th room or seat cannot be mapped to actual entities.” to “or if the room or
seat cannot be mapped to existing Room or Seat entities.” so the sentence reads
clearly and specifies which entities are meant.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java (2)
55-67: Add null-guard for legacy planned locations.In legacy mode, if
plannedRoomorplannedSeatare null (e.g., data integrity issue or partial migration), the DTO would violate@NotBlankconstraints onroomNumberandseatName. Add a guard to handle this case defensively.Apply this diff:
static ExamUserLocationDTO plannedFrom(ExamUser examUser) { final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null; + if (isLegacy) { + // Legacy mode: ensure string fields exist + if (examUser.getPlannedRoom() == null || examUser.getPlannedSeat() == null) { + throw new IllegalStateException("Legacy ExamUser must have plannedRoom and plannedSeat set"); + } + return new ExamUserLocationDTO(null, examUser.getPlannedRoom(), null, null, null, null, examUser.getPlannedSeat()); + } + + // New mode: ensure transients are set + if (examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null) { + throw new IllegalStateException("Modern ExamUser must have plannedRoomTransient and plannedSeatTransient set"); + } + return new ExamUserLocationDTO( - isLegacy ? null : examUser.getPlannedRoomTransient().getId(), - isLegacy ? examUser.getPlannedRoom() : examUser.getPlannedRoomTransient().getRoomNumber(), - isLegacy ? null : examUser.getPlannedRoomTransient().getAlternativeRoomNumber(), - isLegacy ? null : examUser.getPlannedRoomTransient().getName(), - isLegacy ? null : examUser.getPlannedRoomTransient().getAlternativeName(), - isLegacy ? null : examUser.getPlannedRoomTransient().getBuilding(), - isLegacy ? examUser.getPlannedSeat() : examUser.getPlannedSeatTransient().name() + examUser.getPlannedRoomTransient().getId(), + examUser.getPlannedRoomTransient().getRoomNumber(), + examUser.getPlannedRoomTransient().getAlternativeRoomNumber(), + examUser.getPlannedRoomTransient().getName(), + examUser.getPlannedRoomTransient().getAlternativeName(), + examUser.getPlannedRoomTransient().getBuilding(), + examUser.getPlannedSeatTransient().name() ); }
136-148: Validate input or adjust @notempty constraint.If
exam.getExamUsers()is empty, line 146 produces an empty set, violating the@NotEmptyconstraint on line 134. Either validate the input at the call site, throw an exception here, or change the constraint to@NotNullif empty sets are acceptable for edge cases.Option 1 - Add validation:
public static AttendanceCheckerAppExamInformationDTO from(Exam exam, Set<ExamRoom> examRooms) { + if (exam.getExamUsers() == null || exam.getExamUsers().isEmpty()) { + throw new IllegalArgumentException("Exam must have at least one registered user for attendance checking"); + } return new AttendanceCheckerAppExamInformationDTO(Option 2 - If empty is valid, change the constraint:
- @NotEmpty Set<ExamUserWithExamRoomAndSeatDTO> examUsersWithExamRoomAndSeat + @NotNull Set<ExamUserWithExamRoomAndSeatDTO> examUsersWithExamRoomAndSeat
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:72-79
Timestamp: 2025-09-15T11:21:15.983Z
Learning: SamuelRoettgermann prefers iterative development approach - initial implementations should be working starting points rather than full-fledged, finished implementations. Optimization and robustness improvements can be added later.
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
📚 Learning: 2025-09-25T20:43:41.712Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java (1)
22-43: LGTM - Clean DTO mapping.The record structure and factory method correctly map ExamRoom domain data with appropriate validation annotations.
| static ExamUserLocationDTO actualFrom(ExamUser examUser) { | ||
| if (examUser.getActualRoomTransient() == null || examUser.getActualSeatTransient() == null) { | ||
| return null; | ||
| } | ||
|
|
||
| final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null; | ||
|
|
||
| return new ExamUserLocationDTO( | ||
| isLegacy ? null : examUser.getActualRoomTransient().getId(), | ||
| isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getName(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(), | ||
| isLegacy ? null : examUser.getActualRoomTransient().getBuilding(), | ||
| isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Logic error: Legacy actual locations are blocked.
The null-guard at lines 70-72 prevents legacy exams from reporting actual locations. For legacy exams, actualRoomTransient and actualSeatTransient are expected to be null (the system uses string fields actualRoom and actualSeat instead). The current guard returns null for all legacy exams, even when actualRoom and actualSeat are populated.
Apply this diff to fix the logic:
static ExamUserLocationDTO actualFrom(ExamUser examUser) {
- if (examUser.getActualRoomTransient() == null || examUser.getActualSeatTransient() == null) {
- return null;
- }
-
- final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null;
+ final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null;
+
+ if (isLegacy) {
+ // Legacy mode: use string fields
+ if (examUser.getActualRoom() == null || examUser.getActualSeat() == null) {
+ return null;
+ }
+ return new ExamUserLocationDTO(null, examUser.getActualRoom(), null, null, null, null, examUser.getActualSeat());
+ }
+
+ // New mode: use transient DTOs
+ if (examUser.getActualRoomTransient() == null || examUser.getActualSeatTransient() == null) {
+ return null;
+ }
return new ExamUserLocationDTO(
- isLegacy ? null : examUser.getActualRoomTransient().getId(),
- isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(),
- isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(),
- isLegacy ? null : examUser.getActualRoomTransient().getName(),
- isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(),
- isLegacy ? null : examUser.getActualRoomTransient().getBuilding(),
- isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name()
+ examUser.getActualRoomTransient().getId(),
+ examUser.getActualRoomTransient().getRoomNumber(),
+ examUser.getActualRoomTransient().getAlternativeRoomNumber(),
+ examUser.getActualRoomTransient().getName(),
+ examUser.getActualRoomTransient().getAlternativeName(),
+ examUser.getActualRoomTransient().getBuilding(),
+ examUser.getActualSeatTransient().name()
);
}📝 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.
| static ExamUserLocationDTO actualFrom(ExamUser examUser) { | |
| if (examUser.getActualRoomTransient() == null || examUser.getActualSeatTransient() == null) { | |
| return null; | |
| } | |
| final boolean isLegacy = examUser.getPlannedRoomTransient() == null || examUser.getPlannedSeatTransient() == null; | |
| return new ExamUserLocationDTO( | |
| isLegacy ? null : examUser.getActualRoomTransient().getId(), | |
| isLegacy ? examUser.getActualRoom() : examUser.getActualRoomTransient().getRoomNumber(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeRoomNumber(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getName(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getAlternativeName(), | |
| isLegacy ? null : examUser.getActualRoomTransient().getBuilding(), | |
| isLegacy ? examUser.getActualSeat() : examUser.getActualSeatTransient().name() | |
| ); | |
| } | |
| static ExamUserLocationDTO actualFrom(ExamUser examUser) { | |
| // Determine exam mode by planned transients | |
| final boolean isLegacy = examUser.getPlannedRoomTransient() == null | |
| || examUser.getPlannedSeatTransient() == null; | |
| // Legacy mode: use string fields | |
| if (isLegacy) { | |
| if (examUser.getActualRoom() == null || examUser.getActualSeat() == null) { | |
| return null; | |
| } | |
| return new ExamUserLocationDTO( | |
| null, | |
| examUser.getActualRoom(), | |
| null, | |
| null, | |
| null, | |
| null, | |
| examUser.getActualSeat() | |
| ); | |
| } | |
| // New mode: use transient DTOs | |
| if (examUser.getActualRoomTransient() == null | |
| || examUser.getActualSeatTransient() == null) { | |
| return null; | |
| } | |
| return new ExamUserLocationDTO( | |
| examUser.getActualRoomTransient().getId(), | |
| examUser.getActualRoomTransient().getRoomNumber(), | |
| examUser.getActualRoomTransient().getAlternativeRoomNumber(), | |
| examUser.getActualRoomTransient().getName(), | |
| examUser.getActualRoomTransient().getAlternativeName(), | |
| examUser.getActualRoomTransient().getBuilding(), | |
| examUser.getActualSeatTransient().name() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/AttendanceCheckerAppExamInformationDTO.java
around lines 69 to 85, the initial null-guard wrongly returns null for legacy
exams (which use string fields actualRoom/actualSeat) because it checks only
actualRoomTransient/actualSeatTransient; change the guard so it returns null
only when neither transient nor legacy fields are present (i.e., if both
actualRoomTransient and actualSeatTransient are null AND either actualRoom or
actualSeat is null -> return null), leaving legacy cases with populated
actualRoom/actualSeat to proceed and construct the DTO accordingly.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
| @Nullable String roomAlternativeNumber, | ||
| @Nullable String roomName, | ||
| @Nullable String roomAlternativeName, | ||
| @Nullable String roomBuilding, |
There was a problem hiding this comment.
DTOs generally look good like this for the iPad app.
Technically, we don't need these 4 properties. We already get this information from the ExamRoomForAttendanceCheckerDTO by matching the roomId. We can reduce the amount of data sent by quite a bit if we leave them out
| examUser.getUser().getLastName(), | ||
| examUser.getUser().getRegistrationNumber(), | ||
| examUser.getUser().getEmail(), | ||
| examUser.getUser().getImageUrl(), |
There was a problem hiding this comment.
Just as a note so we don't forget, we will need to add the actual exam user image here as this would show the profile picture the user has set
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts (1)
206-208: Verify that capacity cannot be undefined.Based on the AI summary,
ExamRoomLayoutStrategyDTO.capacitywas changed to a non-undefined number in this PR. If this is accurate, the past review concern about NaN due to undefined capacity values is resolved.However, please confirm that the DTO definition now guarantees capacity is always defined to ensure
Math.maxoperations are safe.Run this script to verify the capacity field type:
#!/bin/bash # Description: Verify that ExamRoomLayoutStrategyDTO.capacity is non-optional # Search for the ExamRoomLayoutStrategyDTO interface definition rg -nP --type=java -A 10 'class ExamRoomLayoutStrategyDTO' | grep -A 3 'capacity'
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java (2)
274-276: LGTM! Consider adding capacity validation.The refactoring to extract capacity calculation into dedicated methods improves code reusability and maintainability. The helper methods will throw exceptions if the layout parameters are invalid.
As an optional improvement, consider validating that the calculated capacity is positive and reasonable to catch configuration errors early.
Also applies to: 283-284
501-512: Consider performance optimization for large rooms.The
applyXSpaceAndYSpaceFiltermethod has O(n²) time complexity due to checking every previously selected seat for each candidate seat (line 504-505). For large exam rooms with hundreds of seats, this could become slow.Consider using a spatial data structure (e.g., grid-based bucketing) to reduce the complexity of proximity checks:
private static List<ExamSeatDTO> applyXSpaceAndYSpaceFilter(List<ExamSeatDTO> sortedSeatsAfterFirstRowFilter, double xSpace, double ySpace) { List<ExamSeatDTO> usableSeats = new ArrayList<>(); // Grid-based bucketing could be used here for O(n) average case // Bucket seats by (x/xSpace, y/ySpace) grid coordinates // Only check seats in nearby buckets for (ExamSeatDTO examSeatDTO : sortedSeatsAfterFirstRowFilter) { boolean isFarEnough = usableSeats.stream().noneMatch( existing -> Math.abs(existing.yCoordinate() - examSeatDTO.yCoordinate()) <= ySpace && Math.abs(existing.xCoordinate() - examSeatDTO.xCoordinate()) <= xSpace); if (isFarEnough) { usableSeats.add(examSeatDTO); } } return usableSeats; }Based on learnings: SamuelRoettgermann prefers iterative development, so this optimization could be deferred if current performance is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java(10 hunks)src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.spec.ts(2 hunks)src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/**/*.ts
⚙️ CodeRabbit configuration file
Files:
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.tssrc/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.spec.ts
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java
🧠 Learnings (4)
📓 Common learnings
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:72-79
Timestamp: 2025-09-15T11:21:15.983Z
Learning: SamuelRoettgermann prefers iterative development approach - initial implementations should be working starting points rather than full-fledged, finished implementations. Optimization and robustness improvements can be added later.
📚 Learning: 2025-09-06T19:03:45.260Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11111
File: src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts:71-76
Timestamp: 2025-09-06T19:03:45.260Z
Learning: In the ExamRoomsComponent (src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts), the numberOfUniqueLayoutStrategies computed signal is intentionally designed to sum the total number of layout strategies across all unique exam rooms, not to count distinct layout strategies by name. The author prefers the more descriptive name "numberOfLayoutStrategiesOfUniqueRooms" to better reflect this intention.
Applied to files:
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts
📚 Learning: 2025-09-15T11:18:26.439Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java
📚 Learning: 2025-09-25T20:43:41.712Z
Learnt from: SamuelRoettgermann
PR: ls1intum/Artemis#11419
File: src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomDistributionResource.java:62-78
Timestamp: 2025-09-25T20:43:41.712Z
Learning: In the ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, ExamRoom entities are shared resources that are dynamically assigned to exams through the ExamRoomExamAssignment join table. Rooms don't "belong" to exams until the distribution process creates these assignments. The method first clears existing assignments for the exam, then creates new ones for the provided room IDs.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java
🧬 Code graph analysis (1)
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts (1)
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.model.ts (2)
ExamRoomDTOExtended(17-21)ExamRoomDTO(8-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (10)
src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java (3)
315-322: LGTM!The refactoring correctly implements selection of the newest room version per (roomNumber, name) pair. The use of the null character as a separator is safe and well-documented.
Based on learnings: ExamRoom entities can have multiple versions, and this ensures the admin view displays the most recent ones.
342-367: LGTM!The new public APIs are well-structured with clear javadocs and appropriate error handling. The methods follow single responsibility principle and delegate to helper methods appropriately.
Also applies to: 514-522
369-394: LGTM!The seat selection logic is well-structured with clear separation of concerns:
- Type-specific parsing with proper error handling for invalid JSON
- Delegation to focused helper methods
- Good use of records for input parsing
- Appropriate exception handling with informative error messages
The implementation correctly handles both FIXED_SELECTION and RELATIVE_DISTANCE layout strategies.
Also applies to: 434-459, 472-499
src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.spec.ts (2)
110-116: LGTM!The test correctly verifies the new
defaultCapacityfield, which should be extracted from the 'default' layout strategy's capacity (30) in the mock data. The test expectations align well with the component'sgetDefaultCapacityOfExamRoommethod.
288-316: LGTM!The mock data correctly includes the new
idfield and properly sets up the layout strategy data for testing thedefaultCapacitycalculation.src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts (5)
210-217: LGTM!The
getDefaultCapacityOfExamRoommethod correctly filters for the 'default' layout strategy (case-insensitive) and safely handles missing strategies with a fallback to 0.
219-226: LGTM!The
getLayoutStrategyNamesmethod cleanly extracts, sorts, and joins layout strategy names with proper null handling.
90-90: LGTM!Good refactoring to extract the exam room data calculation into a dedicated method, improving code organization and maintainability.
93-93: LGTM!The expanded sort attributes appropriately include the new
idanddefaultCapacityfields, with a logical default sort byid.
103-114: LGTM!Calling
sortRows()after loading the overview ensures the newly loaded data is properly sorted according to the current sort state.
| import de.tum.cit.aet.artemis.exam.dto.room.ExamRoomUploadInformationDTO; | ||
| import de.tum.cit.aet.artemis.exam.dto.room.ExamSeatDTO; | ||
| import de.tum.cit.aet.artemis.exam.repository.ExamRoomRepository; | ||
| import de.tum.cit.aet.artemis.exam.repository.ExamUserRepository; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Remove unused dependency injection.
ExamUserRepository is injected but never used in this service. This violates the YAGNI principle and adds unnecessary coupling.
Run the following script to verify the repository is unused:
If the repository is needed for future functionality, add it when that functionality is implemented.
As per coding guidelines: avoid unnecessary dependencies and follow YAGNI.
Also applies to: 56-56, 58-61
🏁 Script executed:
#!/bin/bash
# Description: Verify ExamUserRepository usage in ExamRoomService
# Search for any usage of examUserRepository field
rg -n 'examUserRepository\.' src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.javaLength of output: 0
Remove unused ExamUserRepository injection.
examUserRepository isn’t referenced anywhere in ExamRoomService; delete the import, field, constructor parameter, and assignment.
🤖 Prompt for AI Agents
In src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java around
line 40, the ExamUserRepository import and injected dependency are unused;
remove the import statement, delete the corresponding private field, remove the
constructor parameter for ExamUserRepository, and eliminate the assignment
inside the constructor so the class no longer declares or assigns that
repository.
| final int seatIndex = seatInput.seatIndex(); | ||
|
|
||
| if (rowIndex < 0 || sortedRows.size() <= rowIndex || seatIndex < 0 || sortedRows.get(rowIndex).size() <= seatIndex) { | ||
| throw new BadRequestAlertException("Sire, the selected seat " + seatInput + " does not exist in room " + roomNumber, ENTITY_NAME, "room.seatNotFoundFixedSelection", |
There was a problem hiding this comment.
Fix typo in error message.
The error message contains "Sire, the selected seat" where "Sire" appears to be either a typo or inappropriate formality for a technical error message.
Apply this diff:
- throw new BadRequestAlertException("Sire, the selected seat " + seatInput + " does not exist in room " + roomNumber, ENTITY_NAME, "room.seatNotFoundFixedSelection",
+ throw new BadRequestAlertException("The selected seat " + seatInput + " does not exist in room " + roomNumber, ENTITY_NAME, "room.seatNotFoundFixedSelection",📝 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.
| throw new BadRequestAlertException("Sire, the selected seat " + seatInput + " does not exist in room " + roomNumber, ENTITY_NAME, "room.seatNotFoundFixedSelection", | |
| throw new BadRequestAlertException("The selected seat " + seatInput + " does not exist in room " + roomNumber, ENTITY_NAME, "room.seatNotFoundFixedSelection", |
🤖 Prompt for AI Agents
In src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java around
line 424, the error message starts with "Sire, the selected seat" which is a
typo/inappropriate; change the message to a neutral phrasing such as "The
selected seat " + seatInput + " does not exist in room " + roomNumber and keep
the rest of the exception parameters unchanged.
| private calculateExamRoomData() { | ||
| return this.overview()?.newestUniqueExamRooms?.map( | ||
| (examRoomDTO) => | ||
| ({ | ||
| ...examRoomDTO, | ||
| defaultCapacity: this.getDefaultCapacityOfExamRoom(examRoomDTO), | ||
| maxCapacity: this.getMaxCapacityOfExamRoom(examRoomDTO), | ||
| layoutStrategyNames: this.getLayoutStrategyNames(examRoomDTO), | ||
| }) as ExamRoomDTOExtended, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
The calculateExamRoomData method should have an explicit return type annotation for better type safety and code documentation.
Apply this diff:
- private calculateExamRoomData() {
+ private calculateExamRoomData(): ExamRoomDTOExtended[] | undefined {
return this.overview()?.newestUniqueExamRooms?.map(
(examRoomDTO) =>
({
...examRoomDTO,
defaultCapacity: this.getDefaultCapacityOfExamRoom(examRoomDTO),
maxCapacity: this.getMaxCapacityOfExamRoom(examRoomDTO),
layoutStrategyNames: this.getLayoutStrategyNames(examRoomDTO),
}) as ExamRoomDTOExtended,
);
}As per coding guidelines (TypeScript requires explicit type annotations).
📝 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 calculateExamRoomData() { | |
| return this.overview()?.newestUniqueExamRooms?.map( | |
| (examRoomDTO) => | |
| ({ | |
| ...examRoomDTO, | |
| defaultCapacity: this.getDefaultCapacityOfExamRoom(examRoomDTO), | |
| maxCapacity: this.getMaxCapacityOfExamRoom(examRoomDTO), | |
| layoutStrategyNames: this.getLayoutStrategyNames(examRoomDTO), | |
| }) as ExamRoomDTOExtended, | |
| ); | |
| } | |
| private calculateExamRoomData(): ExamRoomDTOExtended[] | undefined { | |
| return this.overview()?.newestUniqueExamRooms?.map( | |
| (examRoomDTO) => | |
| ({ | |
| ...examRoomDTO, | |
| defaultCapacity: this.getDefaultCapacityOfExamRoom(examRoomDTO), | |
| maxCapacity: this.getMaxCapacityOfExamRoom(examRoomDTO), | |
| layoutStrategyNames: this.getLayoutStrategyNames(examRoomDTO), | |
| }) as ExamRoomDTOExtended, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/main/webapp/app/core/admin/exam-rooms/exam-rooms.component.ts around
lines 228 to 238, the calculateExamRoomData method lacks an explicit return
type; add a TypeScript return type annotation describing the possible return
value (e.g. ExamRoomDTOExtended[] | undefined) to match the current expression
which maps overview()?.newestUniqueExamRooms and can be undefined, and ensure
ExamRoomDTOExtended is imported or referenced correctly.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
SamuelRoettgermann
changed the title
Development: iPad modern attendance checker endpoints
2 participants
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Description
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
UI Improvements
Bug Fixes / Localization