Exam mode: Merge student exams page into students page#12558
Exam mode: Merge student exams page into students page#12558
Exam mode: Merge student exams page into students page#12558Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.html (2)
21-29: Optional: simplify the search input binding.With Angular 21 signal-aware forms,
[(ngModel)]="searchText"works directly with a writablesignal, removing the need for the split[ngModel]/(ngModelChange)pair.Suggested change
<input pInputText class="w-100" type="text" [placeholder]="'artemisApp.examManagement.examStudents.addDialog.searchPlaceholder' | artemisTranslate" - [ngModel]="searchText()" - (ngModelChange)="searchText.set($event)" + [(ngModel)]="searchText" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.html` around lines 21 - 29, The search input uses a split binding ([ngModel]="searchText()" and (ngModelChange)="searchText.set($event)") but can be simplified to a two-way signal-aware binding; replace those two bindings on the pInputText input with a single [(ngModel)]="searchText" (target the input element with pInputText and the writable signal named searchText) so the writable signal handles updates automatically. Ensure the component exposes searchText as a writable signal compatible with Angular 21 signal-aware forms before making the change.
85-104: Consider debouncing / truncating / alt-styled register button.Two minor observations on the register action button:
class="p-button-sm ..."uses the legacy size class, whereas the rest of the file usessize="small"onpButton. Prefer the attribute for consistency with PrimeNG 20 conventions.- The button content can overflow when all three states (registering/registered/register) are combined with longer translations. A
min-widthorwhite-space: nowrapon the cell keeps it from reflowing.No functional bug; feel free to skip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.html` around lines 85 - 104, Replace the legacy size class on the register button with the PrimeNG attribute and prevent content wrapping: remove "p-button-sm" and add size="small" to the pButton element used in the button that calls registerStudent(student) and checks isRegistering(student)/isAlreadyRegistered(student); also ensure the button or its containing cell (the td using pFrozenColumn) has a non-wrapping layout and a minimum width (e.g. apply white-space: nowrap and a min-width via a class or inline style) so the icon+label combos for the three states don’t overflow or reflow.src/main/webapp/app/exam/manage/students/exam-students.component.ts (3)
755-771: Inconsistent fallback for missing working time between exam types.For regular exams this helper returns
studentExam.workingTime(possiblyundefined→ rows sort to the end/start via nullable compare), while for test exams it returns0when any field is missing. Rows with0will interleave with the lowest real values rather than cluster at one edge. Consider returningundefinedin both missing-data cases socompareNullableNumbershandles them consistently.Suggested change
if (!studentExam.started || !studentExam.submitted || !studentExam.workingTime || !studentExam.startedDate || !studentExam.submissionDate) { - return 0; + return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts` around lines 755 - 771, The getWorkingTimeSortValue function currently returns studentExam.workingTime (possibly undefined) for normal exams but returns 0 for test exams when data is missing; change getWorkingTimeSortValue so that for test exams you also return undefined when required fields are missing (i.e., when !studentExam.started || !studentExam.submitted || !studentExam.workingTime || !studentExam.startedDate || !studentExam.submissionDate) instead of 0, and keep the existing usedWorkingTime/min logic otherwise—this makes nullability consistent so compareNullableNumbers will handle missing values uniformly.
280-289:routeDataeffect can overwrite user state on later navigation data emissions.
toSignal(this.route.data)will re-emit wheneverroute.datachanges (e.g. route reuse on parameter change). The effect unconditionally callssetUpExamInformation(exam), which resetsstudentExams,isLoading, and re-fetches. If the route doesn’t actually change exams this is harmless, but if navigation re-emits the same exam it re-triggers a full reload on every change-detection cycle whereroute.datafires. Consider gating byexam.id !== this.exam().idto avoid redundant loads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts` around lines 280 - 289, The effect listening to routeData() unconditionally calls setUpExamInformation(exam) on every route.data emission, which can reset studentExams/isLoading and refetch repeatedly; change the effect in exam-students.component.ts to first compare the incoming exam id with the current stored exam (use exam?.id !== this.exam()?.id) and only call setUpExamInformation(exam) when they differ (or when there is no current exam), so redundant re-loads are avoided; refer to routeData, effect(...), setUpExamInformation, and this.exam() when implementing the guard.
483-496: Dead-code opportunity:openIndividualExamsStatusPopover(event)branch ignores a possibly-undefined event.In the else branch you pass
event(the original click event), while the confirmed branch passesundefined, true(defer). This works, but becauseeventhere is alreadyEvent | undefined, the non-defer path can also receiveundefined— in which casepopover.show(new MouseEvent('click'), target)is used with an unrelated synthetic event. Not a bug, but consider always passing{ defer: true }for consistency so the popover anchors reliably after the sort/paginator re-render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts` around lines 483 - 496, handleGenerateStudentExams currently calls openIndividualExamsStatusPopover(event) in the else branch which can pass an undefined Event and cause a synthetic MouseEvent to be used; change the else branch to call openIndividualExamsStatusPopover(undefined, true) (i.e., always pass defer:true) so the popover is consistently anchored after sort/paginator re-rendering—update the call in handleGenerateStudentExams to use openIndividualExamsStatusPopover(undefined, true) and keep generateStudentExams() following it.src/main/webapp/app/exam/manage/students/exam-students.component.html (2)
216-217: Non-null assertion onasExamUserWithExamData(...).The helper returns
ExamUserWithExamData | undefined, so@let examUser = asExamUserWithExamData(student)!;hides a potential undefined at the type level. In practicestudentalways comes from the tablevalue, but you can drop the cast entirely and type the row directly, which is safer:Suggested change
- <ng-template `#body` let-student> - `@let` examUser = asExamUserWithExamData(student)!; + <ng-template `#body` let-examUser>and remove the
asExamUserWithExamDatahelper in the TS file (since[value]="allRegisteredUsers()"is already typed asExamUserWithExamData[],let-examUserin the body template will be correctly typed with Angular's signal-strict-template checking).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.html` around lines 216 - 217, Remove the non-null assertion and helper usage in the template: stop using "@let examUser = asExamUserWithExamData(student)!;" and instead rely on the row typing provided by the table's [value]="allRegisteredUsers()" so the template uses "let-examUser" (or equivalent let binding) directly; then delete the now-unused asExamUserWithExamData helper from the component TS (exam-students.component) so types flow from allRegisteredUsers(): ExamUserWithExamData[] into the template without forcing a non-null assertion.
51-56: Prefer explicit boolean binding forshowValue.
showValue="false"passes the string"false". PrimeNG’sbooleanAttributetransform coerces it correctly today, but the intent is clearer and future-proof as a property binding:Suggested change
- <p-progress-bar - [value]="exercisePreparationPercentage()" - color="var(--bs-success)" - showValue="false" - [style]="{ height: '100%' }" - /> + <p-progress-bar + [value]="exercisePreparationPercentage()" + color="var(--bs-success)" + [showValue]="false" + [style]="{ height: '100%' }" + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.html` around lines 51 - 56, The template uses showValue="false" which binds a string; change it to an explicit property binding [showValue]="false" on the p-progress-bar to pass a boolean value (keep the rest of the attributes including [value]="exercisePreparationPercentage()" and [style] as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/webapp/app/exam/manage/students/exam-students.component.html`:
- Around line 3-6: The template currently renders "({{ exam().title }})" even
when exam().title is undefined; update the exam-students component template to
only render the parentheses and title when exam().title is truthy (e.g. using an
*ngIf on the span or a conditional expression) so the header reads "Students"
until exam().title is populated; locate the span that contains "({{ exam().title
}})" in exam-students.component.html and wrap or replace it with a conditional
render that shows the parentheses and title only when exam().title has a value.
In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts`:
- Around line 338-346: The openUploadImagesDialog function assigns signal
references to the modal's inputs, which bypasses Angular's signal initialization
in StudentsUploadImagesDialogComponent; change the assignments on
modalRef.componentInstance (courseId and exam) to pass the current signal values
instead of the signals themselves by invoking the signals (e.g., use
this.courseId() and this.exam()) so the modal receives concrete values.
---
Nitpick comments:
In
`@src/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.html`:
- Around line 21-29: The search input uses a split binding
([ngModel]="searchText()" and (ngModelChange)="searchText.set($event)") but can
be simplified to a two-way signal-aware binding; replace those two bindings on
the pInputText input with a single [(ngModel)]="searchText" (target the input
element with pInputText and the writable signal named searchText) so the
writable signal handles updates automatically. Ensure the component exposes
searchText as a writable signal compatible with Angular 21 signal-aware forms
before making the change.
- Around line 85-104: Replace the legacy size class on the register button with
the PrimeNG attribute and prevent content wrapping: remove "p-button-sm" and add
size="small" to the pButton element used in the button that calls
registerStudent(student) and checks
isRegistering(student)/isAlreadyRegistered(student); also ensure the button or
its containing cell (the td using pFrozenColumn) has a non-wrapping layout and a
minimum width (e.g. apply white-space: nowrap and a min-width via a class or
inline style) so the icon+label combos for the three states don’t overflow or
reflow.
In `@src/main/webapp/app/exam/manage/students/exam-students.component.html`:
- Around line 216-217: Remove the non-null assertion and helper usage in the
template: stop using "@let examUser = asExamUserWithExamData(student)!;" and
instead rely on the row typing provided by the table's
[value]="allRegisteredUsers()" so the template uses "let-examUser" (or
equivalent let binding) directly; then delete the now-unused
asExamUserWithExamData helper from the component TS (exam-students.component) so
types flow from allRegisteredUsers(): ExamUserWithExamData[] into the template
without forcing a non-null assertion.
- Around line 51-56: The template uses showValue="false" which binds a string;
change it to an explicit property binding [showValue]="false" on the
p-progress-bar to pass a boolean value (keep the rest of the attributes
including [value]="exercisePreparationPercentage()" and [style] as-is).
In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts`:
- Around line 755-771: The getWorkingTimeSortValue function currently returns
studentExam.workingTime (possibly undefined) for normal exams but returns 0 for
test exams when data is missing; change getWorkingTimeSortValue so that for test
exams you also return undefined when required fields are missing (i.e., when
!studentExam.started || !studentExam.submitted || !studentExam.workingTime ||
!studentExam.startedDate || !studentExam.submissionDate) instead of 0, and keep
the existing usedWorkingTime/min logic otherwise—this makes nullability
consistent so compareNullableNumbers will handle missing values uniformly.
- Around line 280-289: The effect listening to routeData() unconditionally calls
setUpExamInformation(exam) on every route.data emission, which can reset
studentExams/isLoading and refetch repeatedly; change the effect in
exam-students.component.ts to first compare the incoming exam id with the
current stored exam (use exam?.id !== this.exam()?.id) and only call
setUpExamInformation(exam) when they differ (or when there is no current exam),
so redundant re-loads are avoided; refer to routeData, effect(...),
setUpExamInformation, and this.exam() when implementing the guard.
- Around line 483-496: handleGenerateStudentExams currently calls
openIndividualExamsStatusPopover(event) in the else branch which can pass an
undefined Event and cause a synthetic MouseEvent to be used; change the else
branch to call openIndividualExamsStatusPopover(undefined, true) (i.e., always
pass defer:true) so the popover is consistently anchored after sort/paginator
re-rendering—update the call in handleGenerateStudentExams to use
openIndividualExamsStatusPopover(undefined, true) and keep
generateStudentExams() following it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6ec6724-f72b-485e-9689-5181c097d864
📒 Files selected for processing (6)
src/main/webapp/app/exam/manage/student-exams/student-exam-status/student-exam-status.component.htmlsrc/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students-menu-button/exam-students-menu-button.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students.component.tssrc/test/playwright/support/pageobjects/exam/StudentExamManagementPage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/playwright/support/pageobjects/exam/StudentExamManagementPage.ts
|
@matyasht Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@matyasht Nice consolidation of the two exam pages into one. The signal migration, PrimeNG table, and the new add-students dialog all look clean. Just a few small things inline.
There was a problem hiding this comment.
Actionable comments posted: 3
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/exam-students.component.spec.ts (1)
168-188:⚠️ Potential issue | 🟠 MajorMock the reload request in removal tests.
removeFromExam()andremoveAllStudents()update the table throughreloadExamWithRegisteredUsers(), which callsexamManagementService.find(...). These tests only mock the remove call, so the asserted component state is never produced unless the reload request is mocked too.Example fix pattern
it('should remove users from the exam', () => { const examServiceStub = vi.spyOn(examManagementService, 'removeStudentFromExam').mockReturnValue(of(new HttpResponse<void>())); + const examWithOneUser = { + ...examWithCourse, + examUsers: [{ didCheckImage: false, didCheckLogin: false, didCheckName: false, didCheckRegistrationNumber: false, ...user1, user: user1 } as ExamUser], + } as Exam; + vi.spyOn(examManagementService, 'find').mockReturnValue(of(new HttpResponse({ body: examWithOneUser }))); fixture.detectChanges();Apply the same pattern in the two
removeAllStudentstests with an exam body whoseexamUsersarray is empty.Also applies to: 209-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts` around lines 168 - 188, The tests for removeFromExam and removeAllStudents need to also mock the reload request that reloadExamWithRegisteredUsers triggers (examManagementService.find) so the component state reflects the server-side change; update the spec for removeFromExam (and the two removeAllStudents tests) to spyOn/mock examManagementService.find to return an Observable<HttpResponse<Exam>> with an exam body having the updated examUsers array (e.g., one user removed or empty), then run fixture.detectChanges()/changeDetectorRef.detectChanges() and assert against component.allRegisteredUsers() as before; locate these changes around the removeFromExam, removeAllStudents, and reloadExamWithRegisteredUsers usages in exam-students.component.spec.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/webapp/app/exam/manage/students/exam-students.component.html`:
- Around line 51-56: In exam-students.component.html the p-progress-bar
currently sets showValue as a string which passes a truthy value; change the
attribute to a property binding [showValue]="false" on the <p-progress-bar>
element so the boolean false is passed correctly (locate the p-progress-bar
element rendering exercisePreparationPercentage()).
In `@src/main/webapp/app/exam/manage/students/exam-students.component.ts`:
- Around line 590-615: The current setExercisePreparationStatus can mark
isAllExercisesPrepared true when remainingExams === 0 even if some exams failed;
update setExercisePreparationStatus so isAllExercisesPrepared is set to true
only when there are no remaining exams AND no failures (e.g., remainingExams ===
0 && (newStatus?.failed ?? 0) === 0); locate the logic in
setExercisePreparationStatus and change the branch that sets
this.isAllExercisesPrepared.set(remainingExams === 0) to use the combined
condition, keeping existing updates to exercisePreparationStatus,
exercisePreparationRunning, exercisePreparationPercentage, and
exercisePreparationEta intact.
- Around line 380-416: The loading flag is only cleared on successful responses,
so add error handlers to the relevant subscriptions to always call
this.isLoading.set(false) and surface a user-visible error (or at least log it).
Concretely, update the subscribe calls on examManagementService.find,
examManagementService.getExerciseStartStatus, and
studentExamService.findAllForExam so their error callbacks set
this.isLoading.set(false) and show an error via the app's alert/notification
mechanism (e.g., this.alertService.error(...)) or fallback to console.error; you
can also factor the behavior into a small helper method like
handleLoadError(error) that logs/shows the message and sets isLoading to false,
and call that from each subscription error handler.
---
Outside diff comments:
In `@src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts`:
- Around line 168-188: The tests for removeFromExam and removeAllStudents need
to also mock the reload request that reloadExamWithRegisteredUsers triggers
(examManagementService.find) so the component state reflects the server-side
change; update the spec for removeFromExam (and the two removeAllStudents tests)
to spyOn/mock examManagementService.find to return an
Observable<HttpResponse<Exam>> with an exam body having the updated examUsers
array (e.g., one user removed or empty), then run
fixture.detectChanges()/changeDetectorRef.detectChanges() and assert against
component.allRegisteredUsers() as before; locate these changes around the
removeFromExam, removeAllStudents, and reloadExamWithRegisteredUsers usages in
exam-students.component.spec.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2b016fe-7014-45e3-abff-bcd2582019d9
📒 Files selected for processing (3)
src/main/webapp/app/exam/manage/students/exam-students.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students.component.spec.tssrc/main/webapp/app/exam/manage/students/exam-students.component.ts
|
@matyasht Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@matyasht Nice work merging these two pages — the PrimeNG migration and signal-based architecture look solid overall. The main blocker is that 3 client tests are failing in CI because the removal methods now reload from the server but the tests don't mock find(). The server test failure (IrisTutorSuggestion) is unrelated.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts (2)
171-181: Use anExamUserfixture factory instead of repeated object spreads.This avoids duplicating the check flags and keeps the test fixtures consistently shaped.
Example refactor
const createExamUser = (user: User): ExamUser => Object.assign(new ExamUser(), { didCheckImage: false, didCheckLogin: false, didCheckName: false, didCheckRegistrationNumber: false, user, });Then replace the repeated inline objects with
createExamUser(user1)/createExamUser(user2).As per coding guidelines,
**/*.{ts,tsx}: “Avoid spread operator for objects in TypeScript”.Also applies to: 217-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts` around lines 171 - 181, The test fixtures create ExamUser objects by repeating object spreads for flags and user data which duplicates shape and violates the guideline against object spreads; define a factory like createExamUser(user: User): ExamUser that returns a new ExamUser instance with didCheckImage/ didCheckLogin/ didCheckName/ didCheckRegistrationNumber set to false and user assigned, then replace the inline spread objects in exam-students.component.spec.ts (both places where examUsers are constructed) with createExamUser(user1)/createExamUser(user2) to keep fixtures consistent and avoid repeated spreads.
74-83: Return a freshStudentExamfixture from the mock.This keeps tests isolated if the component or future assertions mutate the returned response body.
Refactor fixture creation
- const studentExams: StudentExam[] = [ + const createStudentExams = (): StudentExam[] => [ Object.assign(new StudentExam(), { id: 123, user: user1, workingTime: 3600, started: true, submitted: false, examSessions: [{}, {}], }), ]; ... MockProvider(StudentExamService, { - findAllForExam: () => of(new HttpResponse({ body: studentExams })), + findAllForExam: () => of(new HttpResponse({ body: createStudentExams() })), }),Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts` around lines 74 - 83, The test currently reuses a shared StudentExam object in the studentExams array, which can be mutated across assertions; update the mocks in exam-students.component.spec.ts (the studentExams fixtures used around the studentExams variable and the similar fixture at lines ~106-108) to return fresh StudentExam instances for each mock response—create new StudentExam() objects with Object.assign (or a small factory function) each time the mock is called (e.g., when stubbing the service method used by ExamStudentsComponent) so every test gets an independent fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/webapp/app/exam/manage/students/exam-students.component.spec.ts`:
- Around line 171-181: The test fixtures create ExamUser objects by repeating
object spreads for flags and user data which duplicates shape and violates the
guideline against object spreads; define a factory like createExamUser(user:
User): ExamUser that returns a new ExamUser instance with didCheckImage/
didCheckLogin/ didCheckName/ didCheckRegistrationNumber set to false and user
assigned, then replace the inline spread objects in
exam-students.component.spec.ts (both places where examUsers are constructed)
with createExamUser(user1)/createExamUser(user2) to keep fixtures consistent and
avoid repeated spreads.
- Around line 74-83: The test currently reuses a shared StudentExam object in
the studentExams array, which can be mutated across assertions; update the mocks
in exam-students.component.spec.ts (the studentExams fixtures used around the
studentExams variable and the similar fixture at lines ~106-108) to return fresh
StudentExam instances for each mock response—create new StudentExam() objects
with Object.assign (or a small factory function) each time the mock is called
(e.g., when stubbing the service method used by ExamStudentsComponent) so every
test gets an independent fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 59c6eb28-dc18-4628-aebe-4f7e3e3682d2
📒 Files selected for processing (5)
src/main/webapp/app/exam/manage/students/exam-students.component.htmlsrc/main/webapp/app/exam/manage/students/exam-students.component.spec.tssrc/main/webapp/app/exam/manage/students/exam-students.component.tssrc/main/webapp/i18n/de/exam.jsonsrc/main/webapp/i18n/en/exam.json
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/exam/manage/students/exam-students.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/i18n/de/exam.json
|
@matyasht Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@matyasht Great consolidation of the two exam pages — the PrimeNG migration, signal architecture, and the new add-students dialog all look clean. Just one race condition to fix with the loading state, see inline.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@matyasht Nice consolidation of the two exam pages — the signal architecture, PrimeNG table, add-students dialog, and the server-side cache invalidation all look clean. One remaining issue with error handling on the student exam fetch, see inline.
|
@matyasht Test coverage has been automatically updated in the PR description. |
|
@matyasht Test coverage has been automatically updated in the PR description. |
matyasht
force-pushed
the
feature/exam-mode/students-page-merge
branch
from
3627103 to
ac8736a
Compare
|
@matyasht Test coverage has been automatically updated in the PR description. |
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@matyasht Great job addressing all the earlier feedback — the signal architecture, PrimeNG table, and server-side cache invalidation all look solid. Two things remaining: the new add-students dialog needs a test file, and there's a missing error handler on the exercise start status fetch. See inline.
| return !!student.login && this.currentlyRegisteringLogins().has(student.login); | ||
| } | ||
|
|
||
| registerStudent(student: User): void { |
There was a problem hiding this comment.
@matyasht [medium] This component has 130+ lines of non-trivial logic (three-set registration tracking, optimistic UI, HTTP calls, search filtering) but no test file at all. Your own coverage table shows "not found (added)". You should add at least a basic spec covering registerStudent success/error, loadCourseStudents, and the isAlreadyRegistered/isRegistering guards.
🤖 Prompt for AI agents
Create a new test file src/main/webapp/app/exam/manage/students/add-students-dialog/exam-add-students-dialog.component.spec.ts for ExamAddStudentsDialogComponent. Mock CourseManagementService.getAllUsersInCourseGroup and ExamManagementService.addStudentToExam. Write tests for: (1) openDialog loads students, (2) registerStudent calls the service and updates localRegisteredLogins, (3) registerStudent error shows alert, (4) isAlreadyRegistered returns true for registered logins, (5) search filtering works correctly.
| } | ||
|
|
||
| const courseId = this.courseId(); | ||
| this.examManagementService.getExerciseStartStatus(courseId, exam.id).subscribe((res) => this.setExercisePreparationStatus(res.body ?? undefined)); |
There was a problem hiding this comment.
@matyasht [low] This getExerciseStartStatus subscription has no error handler. If it fails (e.g. network blip), the exercise preparation status silently stays empty. Not a crash, but the user gets no feedback. A simple error: () => {} or onError(this.alertService, error) would be consistent with how you handle the other subscriptions in this method.
🤖 Prompt for AI agents
In src/main/webapp/app/exam/manage/students/exam-students.component.ts line 429, the getExerciseStartStatus subscription has no error handler. Add an error callback like error: (error: HttpErrorResponse) => onError(this.alertService, error) to match the pattern used by the findAllForExam subscription a few lines below.
2 participants
Summary
This PR reworks the students page in exam mode and merges the student exams page into it.
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
With the student page in exam mode you can add, remove and generally manage students in a exam.
The student exams page is for managing individual exams of students, such as creating them and extending working time for an individual exam.
As each student can only have at most one individual exam, therefore, it does not make sense to have two separate pages for this.
Description
Steps for Testing
Exam mode only
Exam Mode Testing
Prerequisites:
Tip
You can use the scripts below to quickly setup dummy students with 4 different exam states on a server you have admin rights on (TS3 for instance).
To create the students and exams: cleanup-exam-state-scenarios.js
node supporting_scripts/course-scripts/quick-course-setup/setup-exam-state-scenarios.js --server-url https://artemis-test3.artemis.cit.tum.de/api --course-id <your-course-id>Run this after you are done for cleanup: setup-exam-state-scenarios.js
node supporting_scripts/course-scripts/quick-course-setup/cleanup-exam-state-scenarios.js --server-url https://artemis-test3.artemis.cit.tum.de/api --course-id <your-course-id>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
Test Coverage
Client
Server
Last updated: 2026-04-23 16:50:11 UTC
Screenshots
(design has changed a bit since the video but functionality is the same)
https://youtu.be/a2GT5TerQ9A
Summary by CodeRabbit
New Features
UI Improvements
Behavior
Localization