-
Notifications
You must be signed in to change notification settings - Fork 288
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
Learning analytics
: Improve student course dashboard for better self-learning by showing active exercises
#8839
Learning analytics
: Improve student course dashboard for better self-learning by showing active exercises
#8839
Conversation
Learning analytics
: Improve student dashboard for better self-learning by showing active exercisesLearning analytics
: Improve student course dashboard for better self-learning by showing active exercises
WalkthroughThe recent changes involve enhancing score calculations, improving conditional rendering in the competency accordion component, updating competency exercise handling, and adding new congratulatory messages in the student analytics dashboard. These modifications ensure more accurate score computations and enhance user engagement and interface responsiveness by refining component logic and improving user feedback. Changes
Sequence DiagramsSequence diagrams generation is not applicable for the provided changes due to their scope and straightforward nature. Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 1
Outside diff range comments (2)
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts (2)
Line range hint
37-39
: Remove unnecessary type annotations.Type annotations for
confidence
,mastery
, andprogress
are inferred from their initial values and are therefore redundant. Removing these can clean up the code slightly.- confidence: number = 0; - mastery: number = 0; - progress: number = 0; + confidence = 0; + mastery = 0; + progress = 0;
Line range hint
149-150
: Avoid using non-null assertion operators.The use of non-null assertion operators (
!
) is generally discouraged as they can lead to runtime errors if assumptions about non-nullability prove incorrect. Consider using optional chaining or explicit checks to handle potential null or undefined values safely.- this.router.navigate(['/courses', this.course!.id, 'competencies', this.competency.id]); + this.router.navigate(['/courses', this.course?.id, 'competencies', this.competency.id]);Also applies to: 218-218
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 1
Outside diff range comments (2)
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts (2)
Line range hint
37-39
: Redundant Type Annotations:TypeScript's type inference is capable of deducing the types of
confidence
,mastery
, andprogress
from their initial values. The explicit type annotations (number = 0
) are redundant and can be omitted for cleaner code.- confidence: number = 0; - mastery: number = 0; - progress: number = 0; + confidence = 0; + mastery = 0; + progress = 0;
Line range hint
148-149
: Avoid Non-Null Assertions:Using non-null assertions (
!
) bypasses TypeScript's strict null checks, which can lead to runtime errors if assumptions about non-nullability prove incorrect. Use optional chaining (?.
) where possible to safely access nested properties.- this.course!.id + this.course?.id - this.competency.masteryThreshold! + this.competency.masteryThresholdAlso applies to: 217-217
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts
Outdated
Show resolved
Hide resolved
…t-dashboard-for-self-learning
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 TS1.
- Programming exercise -> Participated -> Set the due date -> Practice was available and worked.
- Quiz with 90% score -> Participated via the dashboard -> was gone after completion
- Participated again in the programming exercises and got 100% -> "Congratulations! You finished all released activities in this competency to at least 80% completion 🎉" ✅
Works as expected! 👍
Can you check if we need to adjust the coverage / add tests for the branch coverage?
Jest: "global" coverage threshold for branches (73.18%) not met: 73.16%
…t-dashboard-for-self-learning
const exerciseIdToExercise = Object.fromEntries(courseExercises.map((exercise) => [exercise.id, exercise] as [number, Exercise])); | ||
const activeCompetencyExercises = (this.metrics.competencyMetrics?.exercises?.[this.competency.id] ?? []) | ||
.flatMap((exerciseId) => [exerciseIdToExercise[exerciseId]] ?? []) | ||
.filter((exercise) => exercise.releaseDate?.isBefore(dayjs())) |
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.
.filter((exercise) => exercise.releaseDate?.isBefore(dayjs())) | |
.filter((exercise) => !exercise.dueDate || exercise.releaseDate.isBefore(dayjs())) |
Currently all exercises without a release date are filtered out
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.
I do not think this would solve this problem. An exercise can have a due date without having a release date, so it would still filter out those exercises.
Edit: (this is a response to Johannes's message)
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.
You're right, it was supposed to be the release Date each time:
.filter((exercise) => exercise.releaseDate?.isBefore(dayjs())) | |
.filter((exercise) => ! exercise.releaseDate || exercise.releaseDate?.isBefore(dayjs())) |
const activeCompetencyExercises = (this.metrics.competencyMetrics?.exercises?.[this.competency.id] ?? []) | ||
.flatMap((exerciseId) => [exerciseIdToExercise[exerciseId]] ?? []) | ||
.filter((exercise) => exercise.releaseDate?.isBefore(dayjs())) | ||
.filter((exercise) => exercise.dueDate?.isAfter(dayjs()) || isStartPracticeAvailable(exercise)); |
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.
.filter((exercise) => exercise.dueDate?.isAfter(dayjs()) || isStartPracticeAvailable(exercise)); | |
.filter((exercise) => !exercise.dueDate || exercise.dueDate.isAfter(dayjs()) || isStartPracticeAvailable(exercise)); |
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/in/www1/artemis/repository/metrics/ExerciseMetricsRepository.java
Show resolved
Hide resolved
Co-authored-by: Johannes Stöhr <38322605+JohannesStoehr@users.noreply.github.com>
src/main/java/de/tum/in/www1/artemis/repository/metrics/ExerciseMetricsRepository.java
Show resolved
Hide resolved
const exerciseIdToExercise = Object.fromEntries(courseExercises.map((exercise) => [exercise.id, exercise] as [number, Exercise])); | ||
const activeCompetencyExercises = (this.metrics.competencyMetrics?.exercises?.[this.competency.id] ?? []) | ||
.flatMap((exerciseId) => [exerciseIdToExercise[exerciseId]] ?? []) | ||
.filter((exercise) => exercise.releaseDate?.isBefore(dayjs())) |
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.
I do not think this would solve this problem. An exercise can have a due date without having a release date, so it would still filter out those exercises.
Edit: (this is a response to Johannes's message)
const exerciseIdToMaxScore = Object.fromEntries( | ||
activeCompetencyExercises.map((exercise) => { | ||
const score = | ||
exercise.studentParticipations?.flatMap((participation) => participation.results ?? []).reduce((max, result) => Math.max(max, result.score ?? 0), -1) ?? 0; |
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.
This statement could likely be simplified a bit, as Math.max can take a variable amount of args, so something similar to Math.max(-1, ...(ex.sp?.flatMap(...).map((result)=>result.score ?? 0)))
should also work. For better readability you could put the flatMap/map result on a separate line.
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.
Actionable comments posted: 0
Outside diff range comments (2)
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts (2)
Line range hint
37-39
: Remove unnecessary type annotations.The type annotations for
confidence
,mastery
, andprogress
are inferred and thus redundant. Removing these will clean up the code.- confidence: number = 0; - mastery: number = 0; - progress: number = 0; + confidence = 0; + mastery = 0; + progress = 0;
Line range hint
148-149
: Avoid non-null assertions.Non-null assertions have been used, which can lead to runtime errors if not handled properly. It's safer to use optional chaining.
- this.router.navigate(['/courses', this.course!.id, 'competencies', this.competency.id]); + this.router.navigate(['/courses', this.course?.id, 'competencies', this.competency.id]);Also applies to: 217-217
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.
Actionable comments posted: 2
Outside diff range comments (2)
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts (2)
Line range hint
149-150
: Avoid using non-null assertions.Non-null assertions have been used, which can potentially lead to runtime errors if assumptions about data presence are incorrect.
- Replace non-null assertions with safer checks like optional chaining or conditional checks to ensure robustness.
- this.course!.id + this.course?.idAlso applies to: 218-218
Line range hint
38-40
: Remove unnecessary type annotations.Type annotations for
confidence
,mastery
, andprogress
are inferred from their initial values and can be removed to clean up the code.- confidence: number = 0; - mastery: number = 0; - progress: number = 0; + confidence = 0; + mastery = 0; + progress = 0;
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts
Show resolved
Hide resolved
src/main/webapp/app/course/competencies/competency-accordion/competency-accordion.component.ts
Show resolved
Hide resolved
…ve-student-dashboard-for-self-learning' into feature/learning-analytics/improve-student-dashboard-for-self-learning
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.
Code looks good enough to be an improvement, especially for GAD where the exercise dates are set. Please fix the selection of exercises in a follow up to also work without exercise dates
Checklist
General
Server
I added multiple integration tests (Spring) related to the features (with a high test coverage).I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).I documented the Java code using JavaDoc style.Client
I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.I addedauthorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently the dashboard does not show a good overview of active exercises where the student has the biggest learning effect #8829
Description
In the competency accordion of the student course dashboard:
80%
ofmax(gradedScore, practiceScore)
max(gradedScore, practiceScore)
) first so the student knows immediately where they can make the biggest learning impactSteps for Testing
Prerequisites:
Test Course Felix T.J. Dietrich
)Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
N/A
Screenshots
Summary by CodeRabbit
New Features
lastRatedScore
more accurately in user scores and average scores.Improvements
course
,nextLectureUnits
, andnextExercises
.Localization
Refactor