Exam mode: Exam Room Management for Instructors
#12044
Open
+457
−502
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to the distribution footer
…vilege-requirement
…vilege-requirement
…n privileges; adapted failing tests
…vilege-requirement
…vilege-requirement
…vilege-requirement
…room-management-privilege-requirement
…vilege-requirement
…vilege-requirement
…vilege-requirement
SamuelRoettgermann
changed the title
Exam mode: Exam mode: Exam Room Management for Instructors
github-actions
bot
added
tests
server
Contributor
WalkthroughAdds an instructor-scoped ExamRoomManagementResource with upload/overview/delete endpoints, renames/simplifies the exam room overview DTO to contain newestUniqueExamRooms only, updates repository queries to fetch newest room versions with eager layout strategies, and adapts frontend routes, components, tests, and translations to a management-oriented flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Instructor)
participant Controller as ExamRoomManagementResource
participant Service as ExamRoomService
participant Repository as ExamRoomRepository
participant DB as Database
rect rgba(0,128,255,0.5)
Note over Client,DB: Upload flow
Client->>Controller: POST /api/exam/rooms/upload (zip)
Controller->>Service: parseAndStoreExamRoomDataFromZipFile(zip)
Service->>Repository: save/update ExamRoom entities (with layoutStrategies)
Repository->>DB: persist exam rooms & layout strategies
DB-->>Repository: OK
Repository-->>Service: saved rooms
Service-->>Controller: ExamRoomUploadInformationDTO
Controller-->>Client: 200 OK + upload details
end
rect rgba(0,200,100,0.5)
Note over Client,DB: Overview flow
Client->>Controller: GET /api/exam/rooms/overview
Controller->>Service: getExamRoomOverview()
Service->>Repository: findAllNewestExamRoomVersionsWithEagerLayoutStrategies()
Repository->>DB: SELECT newest versions with eager layoutStrategy join
DB-->>Repository: newest ExamRoom set
Repository-->>Service: ExamRoom entities
Service->>Service: map to ExamRoomDTO set
Service-->>Controller: ExamRoomOverviewDTO
Controller-->>Client: 200 OK + overview
end
rect rgba(255,128,0,0.5)
Note over Client,DB: Deletion flow
Client->>Controller: DELETE /api/exam/rooms/outdated-and-unused
Controller->>Service: deleteOutdatedAndUnusedExamRooms()
Service->>Repository: find/delete outdated rooms
Repository->>DB: DELETE outdated exam rooms
DB-->>Repository: deletion summary
Repository-->>Service: summary
Service-->>Controller: ExamRoomDeletionSummaryDTO
Controller-->>Client: 200 OK + summary
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.component.spec.ts (1)
1-62: This spec is excluded from Vitest execution and uses incorrect test APIsThe spec file is not included in Vitest's test patterns (lines 27–41 in
vitest.config.ts). Theincludearray lists 13 module paths but omitssrc/main/webapp/app/exam/**/*.spec.ts, so this file will not be executed by Vitest at all. Additionally, the file usesjest.spyOnandjest.restoreAllMocks, which are Jest-specific APIs. The Vitest setup file (vitest-test-setup.ts) does not provide a Jest compatibility layer—it uses only Vitest'sviAPI. Even if the file were added to the include patterns, these Jest APIs would fail withReferenceError.Replace
jest.spyOnwithvi.spyOnandjest.restoreAllMocks()withvi.restoreAllMocks(), then add the exam module pattern to Vitest's include array:'src/main/webapp/app/exam/**/*.spec.ts'.
🤖 Fix all issues with AI agents
In `@src/main/java/de/tum/cit/aet/artemis/exam/dto/room/ExamRoomOverviewDTO.java`:
- Around line 9-11: The JsonInclude policy on ExamRoomOverviewDTO causes
newestUniqueExamRooms to be omitted when empty, conflicting with the TypeScript
contract and tests; update the annotation on the ExamRoomOverviewDTO record (or
remove it) so the field is always serialized (e.g., change
`@JsonInclude`(JsonInclude.Include.NON_EMPTY) to
`@JsonInclude`(JsonInclude.Include.NON_NULL) or remove the annotation entirely)
ensuring newestUniqueExamRooms is emitted as an empty array instead of being
omitted.
In
`@src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRoomRepository.java`:
- Around line 178-202: The new repository method
findAllNewestExamRoomVersionsWithEagerLayoutStrategies has inconsistent grouping
compared to findAllIdsOfNewestExamRoomVersions (it GROUPs BY roomNumber, name
instead of selecting newest per roomNumber) and its Javadoc/@implNote/@return
are inaccurate; update the query to group by roomNumber only (or switch to the
same ROW_NUMBER() OVER (PARTITION BY roomNumber) approach used by
findAllIdsOfNewestExamRoomVersions to guarantee the same "newest" semantics),
change the `@implNote` to correctly describe the chosen technique (either the
GROUP BY+MAX(createdDate) strategy or the PARTITION BY/ROW_NUMBER approach), and
fix the `@return` to state that full ExamRoom entities with eagerly loaded
layoutStrategies are returned (or adjust return type if you intended to return
only basic info). Ensure references to ExamRoom and layoutStrategies remain in
the query and method signature
findAllNewestExamRoomVersionsWithEagerLayoutStrategies.
In
`@src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.service.spec.ts`:
- Around line 29-33: Update the test description in the getRoomOverview suite to
reflect the new naming (replace "admin overview" with the current naming such as
"room overview" or "exam room overview"); locate the test inside the
describe('getRoomOverview', ...) block where the it(...) call currently reads
'should send GET request to retrieve admin overview' and change the string to
the updated description so the test name matches the renamed functionality.
In `@src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java`:
- Around line 321-325: The test method
testUploadSingleRoomRepeatedDuplicatesRejected is still calling the removed
admin route; update the request.postMultipartFileOnly call to use the new upload
endpoint used by the current controller (replace the
"/api/exam/admin/exam-rooms/upload" path with the new upload path) so that
ExamRoomZipFiles.zipFileSingleExamRoomRepeated is posted to the correct
controller endpoint.
🧹 Nitpick comments (3)
src/main/webapp/app/app.routes.ts (1)
229-237: Consider addingusesModuleBackgroundfor consistency.Most other routes in this file include the
usesModuleBackgrounddata property. For visual consistency with other management pages, consider adding it:♻️ Suggested change
{ path: 'exams/rooms', loadComponent: () => import('app/exam/manage/students/room-distribution/exam-rooms.component').then((m) => m.ExamRoomsComponent), data: { authorities: IS_AT_LEAST_INSTRUCTOR, pageTitle: 'artemisApp.examRooms.management.title', + usesModuleBackground: true, }, canActivate: [UserRouteAccessService], },src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.component.ts (1)
65-73: Avoidasassertions for response body and derived totals.This keeps nulls out of the signal state and preserves type safety.
♻️ Suggested adjustment
- readonly numberOfAvailable: Signal<NumberOfAvailable | undefined> = computed(() => { + readonly numberOfAvailable = computed<NumberOfAvailable | undefined>(() => { if (!this.hasOverview()) { return undefined; } return { examRooms: this.numberOfUniqueExamRooms(), examSeats: this.numberOfUniqueExamSeats(), - } as NumberOfAvailable; + }; }); @@ - this.examRoomsService.getRoomOverview().subscribe({ - next: (examRoomOverviewResponse: HttpResponse<ExamRoomOverviewDTO>) => { - this.overview.set(examRoomOverviewResponse.body as ExamRoomOverviewDTO); + this.examRoomsService.getRoomOverview().subscribe({ + next: (examRoomOverviewResponse: HttpResponse<ExamRoomOverviewDTO>) => { + this.overview.set(examRoomOverviewResponse.body ?? undefined); this.sortRows(); },Also applies to: 90-93
src/main/java/de/tum/cit/aet/artemis/exam/web/ExamRoomManagementResource.java (1)
92-96: Rename “admin” wording to match new instructor-facing scopeThe log message and local variable still use “admin” terminology; consider aligning with the new overview naming (and keep log level consistent with other methods).
♻️ Suggested rename
- public ResponseEntity<ExamRoomOverviewDTO> getExamRoomOverview() { - log.info("REST request to get exam room admin overview"); - - var examRoomAdminOverviewDTO = examRoomService.getExamRoomOverview(); - return ResponseEntity.ok(examRoomAdminOverviewDTO); - } + public ResponseEntity<ExamRoomOverviewDTO> getExamRoomOverview() { + log.debug("REST request to get exam room overview"); + + var examRoomOverviewDTO = examRoomService.getExamRoomOverview(); + return ResponseEntity.ok(examRoomOverviewDTO); + }
src/main/java/de/tum/cit/aet/artemis/exam/dto/room/ExamRoomOverviewDTO.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRoomRepository.java
Show resolved
Hide resolved
src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.service.spec.ts
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
|
@SamuelRoettgermann Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.service.spec.ts`:
- Around line 9-10: The Vitest config is missing the exam spec pattern so tests
like exam-rooms.service.spec.ts won't run; open vitest.config.ts and add an
include pattern for the exam module (e.g.,
'src/main/webapp/app/exam/**/*.spec.ts') to the include array so files under the
exam folder (such as ExamRoomsService tests) are picked up by Vitest; ensure the
pattern matches existing spec filename conventions and restart the test runner.
In `@src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java`:
- Around line 269-287: The test currently treats only a null
newestUniqueExamRooms as the "no results" case; update validateExamRoomOverview
to treat an empty list the same: in the early guard, check if
roomOverview.newestUniqueExamRooms() == null ||
roomOverview.newestUniqueExamRooms().isEmpty(), then
assertThat(expectedNewRoomNames).isEmpty() and return. This ensures
validateExamRoomOverview (method name) handles both null and empty responses
from the API and avoids brittle contains assertions later when no expected names
are provided.
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/exam/repository/ExamRoomRepository.java (1)
183-198: Minor inconsistency with similar methods: Group-by approach vs ROW_NUMBER().
findAllNewestExamRoomVersionsWithEagerLayoutStrategies()usesGROUP BY roomNumber + MAX(createdDate), which can return multiple rows if two ExamRooms share the sameroomNumberandcreatedDate. This differs fromfindAllIdsOfNewestExamRoomVersions()andfindAllCurrentExamRoomsForDistribution(), both of which useROW_NUMBER() OVER (PARTITION BY roomNumber ORDER BY createdDate DESC, id DESC)with anid-based tie-breaker.In practice, timestamp precision prevents ties, but consider aligning all three methods for consistency.
♻️ Optional: Use ROW_NUMBER() for consistency
`@Query`(""" - WITH latestRooms AS ( - SELECT - roomNumber AS roomNumber, - MAX(createdDate) AS maxCreatedDate - FROM ExamRoom - GROUP BY roomNumber - ) - SELECT examRoom - FROM ExamRoom examRoom - JOIN latestRooms latestRoom - ON examRoom.roomNumber = latestRoom.roomNumber - AND examRoom.createdDate = latestRoom.maxCreatedDate - LEFT JOIN FETCH examRoom.layoutStrategies + SELECT examRoom + FROM ExamRoom examRoom + LEFT JOIN FETCH examRoom.layoutStrategies + WHERE examRoom.id IN ( + SELECT id + FROM ( + SELECT er.id AS id, ROW_NUMBER() OVER ( + PARTITION BY er.roomNumber + ORDER BY er.createdDate DESC, er.id DESC + ) AS rowNumber + FROM ExamRoom er + ) + WHERE rowNumber = 1 + ) """)
src/main/webapp/app/exam/manage/students/room-distribution/exam-rooms.service.spec.ts
Show resolved
Hide resolved
|
@SamuelRoettgermann Test coverage has been automatically updated in the PR description. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java`:
- Around line 309-326: The tests have contradictory expectations for
duplicate-room zips: testGetExamRoomOverviewSingleRoomRepeated posts
ExamRoomZipFiles.zipFileSingleExamRoomRepeated expecting HttpStatus.OK but
ExamRoomService.parseAndStoreExamRoomDataFromZipFile rejects duplicates
(throwing BadRequest), which is what
testUploadSingleRoomRepeatedDuplicatesRejected expects; fix by making
testGetExamRoomOverviewSingleRoomRepeated either expect HttpStatus.BAD_REQUEST
on the first upload or change the uploaded fixture to
ExamRoomZipFiles.zipFileSingleExamRoom (a non-duplicate zip) so the loop can
succeed—update the assertion in testGetExamRoomOverviewSingleRoomRepeated or
swap the fixture accordingly and keep
testUploadSingleRoomRepeatedDuplicatesRejected as-is.
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java (3)
279-283: Update stale comment reference.The comment mentions "admin overview" but this validation helper now serves the regular exam room overview endpoint. The comment should be updated to reflect the refactoring.
📝 Suggested fix
List<ExamRoomDTO> newestUniqueExamRoomsFromDb = examRoomRepository.findAllNewestExamRoomVersionsWithEagerLayoutStrategies().stream().map(er -> new ExamRoomDTO( er.getRoomNumber(), er.getName(), er.getBuilding(), er.getSeats().size(), - // Because of the INCLUDE.NON_EMPTY on the admin overview, we need to map to null on empty lists to match the returned DTO + // Because of the INCLUDE.NON_EMPTY on the overview DTO, we need to map to null on empty lists to match the returned DTO er.getLayoutStrategies().isEmpty() ? null
312-313: Local variable should use camelCase, not ALL_CAPS.Per Java naming conventions,
ALL_CAPSis reserved for class-level constants (static final). Localfinalvariables should use camelCase.📝 Suggested fix
- final int ITERATIONS = 3; - for (int i = 0; i < ITERATIONS; i++) { + final int iterations = 3; + for (int i = 0; i < iterations; i++) {
398-398: Same naming convention issue here.The local variable
ITERATIONSshould beiterationsto follow Java naming conventions.📝 Suggested fix
- final int ITERATIONS = 10; + final int iterations = 10;
|
@SamuelRoettgermann Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
|
@SamuelRoettgermann Test coverage has been automatically updated in the PR description. |
|
@SamuelRoettgermann Your PR description needs attention before it can be reviewed: Issues Found
How to Fix
This check validates that your PR description follows the PR template. A complete description helps reviewers understand your changes and speeds up the review process.
|
End-to-End (E2E) Test Results Summary
|
|||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Allows instructors to access the exam room management/upload page by putting a button to the exam-distribution dialog. Also simplified the exam room management page.
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently only administrators can upload new rooms. The reason for this feature being admin-only was the existence of a "delete all rooms" button, but after that functionality was removed shortly after introduction, the room management lost its reason for remaining admin only.
Description
Moves the exam room management to be accessible via the
Distributebutton at the Students view, so that both heavily related features can be accessed together.This PR most a lot of code, while also lowering a few access privileges along the way.
Additionally, this PR simplifies the UI of the Exam Room Management page a bit, as reviews have shown me that it was still tremendously confusing.
Steps for Testing
Prerequisites:
Studentsthere to take you to the list of all registered studentsDistributebuttonExam Room Managementin the bottom left cornerDistribute Studentsdialog and select some rooms to distribute students to.Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Server
Last updated: 2026-01-27 13:46:15 UTC
Screenshots
Summary by CodeRabbit
New Features
User Interface
Behavior
Tests
✏️ Tip: You can customize this high-level summary in your review settings.