Development: Make scores and participations code more consistent
#12037
Conversation
Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
tests
server
krusche
changed the title
Development: Make scores and participations code more consistent
WalkthroughRestricts exam-exercise participation retrieval to instructors (TAs allowed only for course exercises), migrates a non-programming exercise detail component to Angular signals with a computed access predicate, adds a shared getExerciseCompetencies utility, updates multiple detail components/tests to use it, and adds related i18n keys and integration tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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}
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 (2)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java (1)
110-114: Duplicate submission count computation causes unnecessary DB query.Lines 110-111 compute and set the submission count inside the
elseblock, and lines 113-114 do the exact same computation unconditionally after the if-else block. WhenwithLatestResultsis false, this results in two identical database queries and redundant iteration over participations.🔧 Proposed fix
else { if (exercise.isTeamMode()) { participations = studentParticipationRepository.findWithTeamInformationByExerciseId(exerciseId); } else { participations = studentParticipationRepository.findByExerciseId(exerciseId); } - - Map<Long, Integer> submissionCountMap = studentParticipationRepository.countSubmissionsPerParticipationByExerciseIdAsMap(exerciseId); - participations.forEach(participation -> participation.setSubmissionCount(submissionCountMap.get(participation.getId()))); } Map<Long, Integer> submissionCountMap = studentParticipationRepository.countSubmissionsPerParticipationByExerciseIdAsMap(exerciseId); participations.forEach(participation -> participation.setSubmissionCount(submissionCountMap.get(participation.getId())));src/main/webapp/app/exercise/util/utils.ts (1)
65-81: Avoid rendering missing competency titles.
CourseCompetency.titleis optional; joining directly can show"undefined"or stray separators. Consider filtering titles first.🐛 Proposed fix
- if (hasCompetencies) { - details.push({ - title: 'artemisApp.competency.link.title', - type: DetailType.Text, - data: { text: competencies.map((competency) => competency.title).join(', ') }, - }); - } + if (hasCompetencies) { + const competencyTitles = competencies + .map((competency) => competency.title) + .filter((title): title is string => title !== undefined && title !== ''); + details.push({ + title: 'artemisApp.competency.link.title', + type: DetailType.Text, + data: { text: competencyTitles.join(', ') }, + }); + }
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/exercise/shared/entities/exercise/exercise.model.spec.ts`:
- Around line 1-3: The Vitest config is missing the exercise test pattern so
tests under src/main/webapp/app/exercise/... aren't run; update the include
array in vitest.config.ts to add the pattern
'src/main/webapp/app/exercise/**/*.spec.ts' so files like
src/main/webapp/app/exercise/shared/entities/exercise/exercise.model.spec.ts
(testing Exercise and getExerciseCompetencies) are picked up by the test runner.
In `@src/main/webapp/app/exercise/shared/entities/exercise/exercise.model.ts`:
- Around line 304-305: The getExerciseCompetencies function currently only
filters out undefined values and can let null competencies through; update the
filter predicate in getExerciseCompetencies to exclude both undefined and null
(e.g., use competency != null or explicit checks competency !== undefined &&
competency !== null) while keeping the type guard signature (competency is
CourseCompetency) so the returned array is correctly typed as
CourseCompetency[].
In
`@src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.ts`:
- Around line 256-258: The teamBaseResource for exam exercises mistakenly uses
this.programmingExercise.exerciseGroup?.exam?.id as the final path segment;
update the URL construction in programming-exercise-detail.component.ts (the
teamBaseResource assignment) to use the exercise ID instead (the same exerciseId
used in the non-exam path, e.g., this.programmingExercise.id or the existing
exerciseId variable) so the last segment is the exercise's ID rather than the
exam ID.
🧹 Nitpick comments (7)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationRetrievalResource.java (1)
90-97: Consider adding a defensive else clause.The if-else-if structure lacks a final
elseclause. While exercises should always be either course or exam exercises, adding an else clause that throws an exception would provide a safety net against unexpected states.🛡️ Suggested defensive guard
if (exercise.isCourseExercise()) { // teaching assistants can access scores and participations in course exercises authCheckService.checkHasAtLeastRoleForExerciseElseThrow(Role.TEACHING_ASSISTANT, exercise, null); } else if (exercise.isExamExercise()) { // only instructors can access scores and participations in exam exercises authCheckService.checkHasAtLeastRoleForExerciseElseThrow(Role.INSTRUCTOR, exercise, null); } +else { + throw new IllegalStateException("Exercise must be either a course exercise or an exam exercise"); +}src/main/webapp/app/modeling/manage/detail/modeling-exercise-detail.component.spec.ts (1)
122-125: Avoid object spread per app TS guidelines.
UseObject.assign(or explicit assignment) instead of{ ...obj }in app code. As per coding guidelines, consider:♻️ Proposed refactor
- const exerciseWithCompetencies = { - ...modelingExercise, - competencyLinks: [{ competency: competency1 } as CompetencyExerciseLink, { competency: competency2 } as CompetencyExerciseLink], - } as ModelingExercise; + const exerciseWithCompetencies = Object.assign({}, modelingExercise, { + competencyLinks: [{ competency: competency1 } as CompetencyExerciseLink, { competency: competency2 } as CompetencyExerciseLink], + }) as ModelingExercise;src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.ts (1)
168-177: Consider using Angular signals for component state.Per coding guidelines, Angular components should use signals for component state. The
canAccessParticipationsAndScoresproperty could be a computed signal that automatically updates when its dependencies change.♻️ Suggested refactor using signals
// Convert to signal-based state private programmingExerciseSignal = signal<ProgrammingExercise | undefined>(undefined); private isExamExerciseSignal = signal(false); canAccessParticipationsAndScores = computed(() => { const exercise = this.programmingExerciseSignal(); const isExam = this.isExamExerciseSignal(); return (exercise?.isAtLeastTutor && !isExam) || !!exercise?.isAtLeastInstructor; });src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.spec.ts (1)
327-387: Test design could be improved to exercise actual component behavior.The
computeCanAccessParticipationsAndScoreshelper manually replicates the logic fromhandleRouteData. If the component logic changes, these tests would still pass despite being out of sync. Consider testing throughngOnInitwith proper route data setup, similar to the course/exam exercise tests.♻️ Suggested approach
describe('canAccessParticipationsAndScores', () => { it('should return true for course exercise when user is at least tutor', () => { const programmingExercise = new ProgrammingExercise(new Course(), undefined); programmingExercise.id = 1; programmingExercise.isAtLeastTutor = true; programmingExercise.isAtLeastInstructor = false; const route = TestBed.inject(ActivatedRoute); route.snapshot.data = { programmingExercise }; comp.ngOnInit(); expect(comp.canAccessParticipationsAndScores).toBeTrue(); }); // ... similar for other test cases });src/main/webapp/app/exercise/exercise-detail-common-actions/non-programming-exercise-detail-common-actions.component.html (2)
11-11: Consider removing redundantcourse()check.Since
courseis defined asinput.required<Course>()in the component, it will always have a value. Thecourse() &&guard is unnecessary—Angular would throw an error if a required input is not provided.You could simplify to just
canAccessParticipationsAndScores():♻️ Suggested simplification
- `@if` (course() && canAccessParticipationsAndScores()) { + `@if` (canAccessParticipationsAndScores()) {Apply the same change at line 31.
Also applies to: 31-31
43-43: Minor inconsistency:exercise.coursevscourse()signal.This condition uses
exercise.course(checking the entity property) while lines 11 and 31 usecourse()(the input signal). Theexercise.coursecheck combined with!isExamExercise()appears redundant—if it's not an exam exercise, it must be a course exercise.If the intent is to verify the exercise has its course property populated, the current approach is acceptable. Otherwise, consider aligning with the
course()signal usage for consistency.src/main/webapp/app/exercise/exercise-detail-common-actions/non-programming-exercise-detail-common-actions.component.ts (1)
76-89: Consider adding defensive checks for undefined IDs.The code uses non-null assertions (
!) oncourse.idand relies on optional chaining forexercise.exerciseGroup?.exam?.idandexercise.exerciseGroup?.id. If any of these are undefined, the constructed URLs will containundefinedstring segments, leading to broken routes.While these IDs are expected to exist in practice, consider adding a guard or logging to catch misconfigured data early:
♻️ Optional defensive check
ngOnInit(): void { const exercise = this.exercise(); const course = this.course(); if (!course.id || !exercise.id) { // Log warning or handle gracefully - this shouldn't happen in normal flow return; } if (!this.isExamExercise()) { // ... existing code } else { if (!exercise.exerciseGroup?.exam?.id || !exercise.exerciseGroup?.id) { return; } // ... existing code } // ... }Based on learnings, internal safety checks for impossible runtime states (like missing route configuration) can be implemented without user-facing error notifications, as these are development-time safeguards.
src/main/webapp/app/exercise/shared/entities/exercise/exercise.model.spec.ts
Show resolved
Hide resolved
src/main/webapp/app/exercise/shared/entities/exercise/exercise.model.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.ts
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
|
@krusche Test coverage has been automatically updated in the PR description. |
- Filter out null competencies in getExerciseCompetencies (defensive) - Fix pre-existing teamBaseResource bug for exam exercises Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@krusche Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
There was a problem hiding this comment.
Tested on TS4
- It's not possible to link competencies to programming exercises (I'm not sure if it was ever possible to link competencies to programming exercises, though)
- Regarding testing step 3: While the linked competencies are shown, the associated priority (low / medium / high) is not
- Regarding testing step 3: For quiz exercises the linked competencies are in the
Modesection, not the (non-existent)Problemsection
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
maxgutke
left a comment
maxgutke
left a comment
There was a problem hiding this comment.
I tested this PR on TS 1. Everything worked as expected: For every exercise type the competency is shown and the "Scores" and "Pacrticipations" buttons worked just fine
The 2. and 3. point was like that before and is out of scope. For your 1. point, I am confused, I tested this and it was possible, however you have to switch to the advance mode. Could you try again and let me know if it does not work? In that case, please provide a screenshot |
Summary
This PR improves consistency across the codebase for handling scores and participations data. It introduces a reusable helper function for extracting competencies from exercises, modernizes UI components to use Angular signals, and adds comprehensive test coverage.
Checklist
General
Server
Client
Motivation and Context
The existing code for handling scores and participations across different exercise types lacked consistency. Competency extraction logic was duplicated across components, and some UI components used older patterns instead of modern Angular signals. This made the codebase harder to maintain and extend.
Description
Server Changes:
ParticipationRetrievalResourceto use consistent authorization checks that align with existing patterns in the codebaseClient Changes:
getExerciseCompetencies()helper function inexercise.model.tsfor consistent extraction of competencies from exercise competency linksNonProgrammingExerciseDetailCommonActionsComponentto use Angular signals (input(),input.required(),computed())canAccessParticipationsAndScorescomputed signal for consistent access control logicinconsistenciesFoundAlertin both English and GermanTest Coverage:
getExerciseCompetencies()helper functionSteps for 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
Code Review
Manual Tests
Test Coverage
Client
Server
Last updated: 2026-01-26 20:16:08 UTC
Screenshots
This PR contains internal code refactoring (migrating to Angular signals, extracting helper functions) without visual design changes. The client changes affect internal access control logic and code organization, not the visual appearance of the UI.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.