-
Notifications
You must be signed in to change notification settings - Fork 358
Exam mode: Attendance checker iPad app shows unassigned students
#12046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Exam mode: Attendance checker iPad app shows unassigned students
#12046
Conversation
…bugfix/exam-mode/ipad-attendance-checker-show-unseated-students
WalkthroughThe changes refactor the attendance checker DTO to distinguish between legacy and modern data paths for exam user location handling. The distribution logic is simplified by directly mapping exam users instead of using intermediate filtering. Test files are updated to reflect renamed distribution methods from "modern" to "automatic," with helper methods generalized to accept configurable student counts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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 |
|
@SamuelRoettgermann Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
| if (isLegacy) { | ||
| return new ExamUserLocationDTO( | ||
| null, | ||
| StringUtils.hasText(examUser.getPlannedRoom()) ? examUser.getPlannedRoom() : "No Room set", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internationalization is not possible this way. Why not use null or at least add a flag that clearly marks this student as unassigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of right now the iPad attendance checker app only supports English, and it is not currently planned to support more languages.
The reason why we chose to do it this way, instead of directly returning null, is because returning null causes older iPad versions to crash, because they expect the fields to always be non-null. Over the past few weeks we had two exams, and in both cases had some issues because many of the iPads were outdated, both the iOS version as well as the attendance checker app version itself. This means we can't roll out changes that would break backwards-compatibility.
For these reasons, we decided not to return A) a (translated) translation-key, or B) null.
An extra flag might work, but it'd also run into some backwards-compatibility issues.
anian03
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS4 with the iPad app. All students were correctly returned, including those without a room/seat assignment. They were grouped into a separate "room" called "No room set".
Code also LGTM, hardcoding english strings is fine here as we don't support any other languages in the iPad app (and it's the only place this is used). The web app does not show this string
Summary
Show unassigned students in a separate "No Room Set" section.
Checklist
General
Server
Motivation and Context
When using the iPad attendance checker app, students who were not assigned a room or seat would not be shown in the iPad app.
Description
Now, instead of filtering out unused students, it sets the room and seat fields to reasonable default texts. The iPad app only supports English, so a translation is not required.
Steps for Testing - if you do have an iPad
Prerequisites:
Distributebutton (you can select more rooms than are needed).Steps for Testing - if you don't have an iPad
Prerequisites:
GETrequests. I personally use Postman, I wrote an explanation on how it can be used in theSteps for Testingof this PR:Development: Add server option for dynamic exam seating distribution #11425GETrequest to<artemis-server>/api/exam/courses/{courseId}/exams/{examId}/attendance-checker-information. Check that it contains the students without seat assignments with "No Room set"/"No Seat set".Distributebutton (you can select more rooms than are needed).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
Server
Last updated: 2026-01-27 14:29:01 UTC
Screenshots
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.